Re: On login trigger: take three
Hi Nikita, > Mikhail, I've checked the patch and previous discussion, > the condition mentioned earlier is still actual: Thanks for pointing this out! My bad, I forgot to fix the documentation example. Attached v34 has this issue fixed, as well as a couple other problems with the example code. -- best regards, Mikhail A. Gribkov v34-On_client_login_event_trigger.patch Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Dec 16, 2022 at 4:34 PM Amit Kapila wrote:
>
> On Fri, Dec 16, 2022 at 2:47 PM houzj.f...@fujitsu.com
> wrote:
> >
> > > ---
> > > + active_workers = list_copy(ParallelApplyWorkerPool);
> > > +
> > > + foreach(lc, active_workers)
> > > + {
> > > + int slot_no;
> > > + uint16 generation;
> > > + ParallelApplyWorkerInfo *winfo =
> > > (ParallelApplyWorkerInfo *) lfirst(lc);
> > > +
> > > + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
> > > + napplyworkers =
> > > logicalrep_pa_worker_count(MyLogicalRepWorker->subid);
> > > + LWLockRelease(LogicalRepWorkerLock);
> > > +
> > > + if (napplyworkers <=
> > > max_parallel_apply_workers_per_subscription / 2)
> > > + return;
> > > +
> > >
> > > Calling logicalrep_pa_worker_count() with lwlock for each worker seems
> > > not efficient to me. I think we can get the number of workers once at
> > > the top of this function and return if it's already lower than the
> > > maximum pool size. Otherwise, we attempt to stop extra workers.
> >
> > How about we directly check the length of worker pool list here which
> > seems simpler and don't need to lock ?
> >
>
> I don't see any problem with that. Also, if such a check is safe then
> can't we use the same in pa_free_worker() as well? BTW, shouldn't
> pa_stop_idle_workers() try to free/stop workers unless the active
> number reaches below max_parallel_apply_workers_per_subscription?
>
BTW, can we move pa_stop_idle_workers() functionality to a later patch
(say into v61-0006*)? That way we can focus on it separately once the
main patch is committed.
--
With Regards,
Amit Kapila.
Re: On login trigger: take three
On Sat, Dec 17, 2022 at 3:46 AM Mikhail Gribkov wrote: > Hi Nikita, > > > Mikhail, I've checked the patch and previous discussion, > > the condition mentioned earlier is still actual: > > Thanks for pointing this out! My bad, I forgot to fix the documentation > example. > Attached v34 has this issue fixed, as well as a couple other problems with > the example code. > > -- > best regards, > Mikhail A. Gribkov > Hi, bq. in to the system 'in to' -> into bq. Any bugs in a trigger procedure Any bugs -> Any bug + if (event == EVT_Login) + dbgtag = CMDTAG_LOGIN; + else + dbgtag = CreateCommandTag(parsetree); The same snippet appears more than once. It seems CMDTAG_LOGIN handling can be moved into `CreateCommandTag`. Cheers
Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX
On Fri, Dec 16, 2022 at 10:04 PM Nathan Bossart
wrote:
> On Thu, Dec 15, 2022 at 10:10:43AM -0800, Jeff Davis wrote:
> > The proposal to skip privilege checks for partitions would be
> > consistent with INSERT, SELECT, REINDEX that flow through to the
> > underlying partitions regardless of permissions/ownership (and even
> > RLS). It would be very minor behavior change on 15 for this weird case
> > of superuser-owned partitions, but I doubt anyone would be relying on
> > that.
>
> I've attached a work-in-progress patch that aims to accomplish this.
> Instead of skipping the privilege checks, I added logic to trawl through
> pg_inherits and pg_class to check whether the user has privileges for the
> partitioned table or for the main relation of a TOAST table. This means
> that MAINTAIN on a partitioned table is enough to execute maintenance
> commands on all the partitions, and MAINTAIN on a main relation is enough
> to execute maintenance commands on its TOAST table. Also, the maintenance
> commands that flow through to the partitions or the TOAST table should no
> longer error due to permissions when the user only has MAINTAIN on the
> paritioned table or main relation.
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
Hi,
+cluster_is_permitted_for_relation(Oid relid, Oid userid)
+{
+ return pg_class_aclcheck(relid, userid, ACL_MAINTAIN) ==
ACLCHECK_OK ||
+ has_parent_privs(relid, userid, ACL_MAINTAIN);
Since the func only contains one statement, it seems this can be defined as
a macro instead.
+ List *ancestors = get_partition_ancestors(relid);
+ Oid root = InvalidOid;
nit: it would be better if the variable `root` can be aligned with variable
`ancestors`.
Cheers
Re: Progress report of CREATE INDEX for nested partitioned tables
On Tue, Dec 13, 2022 at 10:18:58PM +0400, Ilya Gladyshev wrote: > > > I actually think that the progress view would be better off without > > > the total number of partitions, > > > > Just curious - why ? > > We don't really know how many indexes we are going to create, unless we > have some kind of preliminary "planning" stage where we acumulate all > the relations that will need to have indexes created (rather than > attached). And if someone wants the total, it can be calculated > manually without this view, it's less user-friendly, but if we can't do > it well, I would leave it up to the user. Thanks. One other reason is that the partitions (and sub-partitions) may not be equally sized. Also, I've said before that it's weird to report macroscopic progress about the number of partitions finihed in the same place as reporting microscopic details like the number of blocks done of the relation currently being processed. > > I have another proposal: since the original patch 3.5 years ago > > didn't > > consider or account for sub-partitions, let's not start counting them > > now. It was never defined whether they were included or not (and I > > guess that they're not common) so we can take this opportunity to > > clarify the definition. > > I have had this thought initially, but then I thought that it's not > what I would want, if I was to track progress of multi-level > partitioned tables (but yeah, I guess it's pretty uncommon). In this > respect, I like your initial counter-proposal more, because it leaves > us room to improve this in the future. Otherwise, if we commit to > reporting only top-level partitions now, I'm not sure we will have the > opportunity to change this. We have the common problem of too many patches. https://www.postgresql.org/message-id/a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel%40gmail.com This changes the progress reporting to show indirect children as "total", and adds a global variable to track recursion into DefineIndex(), allowing it to be incremented without the value being lost to the caller. https://www.postgresql.org/message-id/20221211063334.GB27893%40telsasoft.com This also counts indirect children, but only increments the progress reporting in the parent. This has the disadvantage that when intermediate partitions are in use, the done_partitions counter will "jump" from (say) 20 to 30 without ever hitting 21-29. https://www.postgresql.org/message-id/20221213044331.GJ27893%40telsasoft.com This has two alternate patches: - One patch changes to only update progress reporting of *direct* children. This is minimal, but discourages any future plan to track progress involving intermediate partitions with finer granularity. - A alternative patch adds IndexStmt.nparts_done, and allows reporting fine-grained progress involving intermediate partitions. https://www.postgresql.org/message-id/flat/039564d234fc3d014c555a7ee98be69a9e724836.ca...@gmail.com This also reports progress of intermediate children. The first patch does it by adding an argument to DefineIndex() (which isn't okay to backpatch). And an alternate patch does it by adding to IndexStmt. @committers: Is it okay to add nparts_done to IndexStmt ? -- Justin
Re: [PATCH] random_normal function
Hello Paul, My 0.02€ about the patch: Patch did not apply with "git apply", I had to "patch -p1 <" and there was a bunch of warnings. Patches compile and make check is okay. The first patch adds empty lines at the end of "sql/random.sql", I think that it should not. # About random_normal: I'm fine with providing a random_normal implementation at prng and SQL levels. There is already such an implementation in "pgbench.c", which outputs integers, I'd suggest that it should also use the new prng function, there should not be two box-muller transformations in pg. # About pg_prng_double_normal: On the comment, I'd write "mean + stddev * val" instead of starting with the stddev part. Usually there is an empty line between the variable declarations and the first statement. There should be a comment about why it needs u1 larger than some epsilon. This constraint seems to generate a small bias? I'd suggest to add the UNLIKELY() compiler hint on the loop. # About random_string: Should the doc about random_string tell that the output bytes are expected to be uniformly distributed? Does it return "random values" or "pseudo random values"? I do not understand why the "drandom_string" function is in "float.c", as it is not really related to floats. Also it does not return a string but a bytea, so why call it "_string" in the first place? I'm do not think that it should use pg_strong_random which may be very costly on some platform. Also, pg_strong_random does not use the seed, so I do not understand why it needs to be checked. I'd suggest that the output should really be uniform pseudo-random, possibly based on the drandom() state, or maybe not. Overall, I think that there should be a clearer discussion and plan about which random functionS postgres should provide to complement the standard instead of going there… randomly:-) -- Fabien.
Re: Infinite Interval
Hi Ashutosh,
I've added tests for all the operators and functions involving
intervals and what I think the expected behaviors to be. The
formatting might be slightly off and I've left the contents of the
error messages as TODOs. Hopefully it's a good reference for the
implementation.
> Adding infinite interval to an infinite timestamp with opposite
> direction is not going to yield 0 but some infinity. Since we are adding
> interval to the timestamp the resultant timestamp is an infinity
> preserving the direction.
I think I disagree with this. Tom Lane in one of the previous threads
said:
> tl;dr: we should model it after the behavior of IEEE float infinities,
> except we'll want to throw errors where those produce NaNs.
and I agree with this opinion. I believe that means that adding an
infinite interval to an infinite timestamp with opposite directions
should yield an error instead of some infinity. Since with floats this
would yield a NaN.
> Dividing infinite interval by finite number keeps it infinite.
> TODO: Do we change the sign of infinity if factor is negative?
Again if we model this after the IEEE float behavior, then the answer
is yes, we do change the sign of infinity.
- Joe Koshakow
From 4c1be4e2aa7abd56967fdce14b100715f3a63fee Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/timestamp.c | 166 +++-
src/test/regress/expected/interval.out | 565 -
src/test/regress/sql/interval.sql | 105 +
4 files changed, 824 insertions(+), 14 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e98c6dc78 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3634,6 +3634,8 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_STRING:
case DTK_SPECIAL:
type = DecodeUnits(i, field[i], &uval);
+if (type == UNKNOWN_FIELD)
+ type = DecodeSpecial(i, field[i], &uval);
if (type == IGNORE_DTF)
continue;
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 3f2508c0c4..0c7286b06e 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -79,6 +79,12 @@ static bool AdjustIntervalForTypmod(Interval *interval, int32 typmod,
static TimestampTz timestamp2timestamptz(Timestamp timestamp);
static Timestamp timestamptz2timestamp(TimestampTz timestamp);
+static void EncodeSpecialInterval(Interval *interval, char *str);
+static void interval_noend(Interval *interval);
+static bool interval_is_noend(Interval *interval);
+static void interval_nobegin(Interval *interval);
+static bool interval_is_nobegin(Interval *interval);
+static bool interval_not_finite(Interval *interval);
/* common code for timestamptypmodin and timestamptztypmodin */
static int32
@@ -943,6 +949,14 @@ interval_in(PG_FUNCTION_ARGS)
errmsg("interval out of range")));
break;
+ case DTK_LATE:
+ interval_noend(result);
+ break;
+
+ case DTK_EARLY:
+ interval_nobegin(result);
+ break;
+
default:
elog(ERROR, "unexpected dtype %d while parsing interval \"%s\"",
dtype, str);
@@ -965,8 +979,13 @@ interval_out(PG_FUNCTION_ARGS)
*itm = &tt;
char buf[MAXDATELEN + 1];
- interval2itm(*span, itm);
- EncodeInterval(itm, IntervalStyle, buf);
+ if (interval_not_finite(span))
+ EncodeSpecialInterval(span, buf);
+ else
+ {
+ interval2itm(*span, itm);
+ EncodeInterval(itm, IntervalStyle, buf);
+ }
result = pstrdup(buf);
PG_RETURN_CSTRING(result);
@@ -1352,6 +1371,13 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod,
INT64CONST(0)
};
+ /*
+ * Infinite interval after being subjected to typmod conversion remains
+ * infinite.
+ */
+ if (interval_not_finite(interval))
+ return;
+
/*
* Unspecified range and precision? Then not necessary to adjust. Setting
* typmod to -1 is the convention for all data types.
@@ -1545,6 +1571,17 @@ EncodeSpecialTimestamp(Timestamp dt, char *str)
elog(ERROR, "invalid argument for EncodeSpecialTimestamp");
}
+static void
+EncodeSpecialInterval(Interval *interval, char *str)
+{
+ if (interval_is_nobegin(interval))
+ strcpy(str, EARLY);
+ else if (interval_is_noend(interval))
+ strcpy(str, LATE);
+ else /* shouldn't happen */
+ elog(ERROR, "invalid argument for EncodeSpecialInterval");
+}
+
Datum
now(PG_FUNCTION_ARGS)
{
@@ -2080,10 +2117,12 @@ timestamp_finite(PG_FUNCTION
Re: check_strxfrm_bug()
On Thu, Dec 15, 2022 at 3:22 PM Thomas Munro wrote: > ... If you're worried that the bugs might > come back, then the test is insufficient: modern versions of both OSes > have strxfrm_l(), which we aren't checking. With my garbage collector hat on, that made me wonder if there was some more potential cleanup here: could we require locale_t yet? The last straggler systems on our target OS list to add the POSIX locale_t stuff were Solaris 11.4 (2018) and OpenBSD 6.2 (2018). Apparently it's still too soon: we have two EOL'd OSes in the farm that are older than that. But here's an interesting fact about wrasse, assuming its host is gcc211: it looks like it can't even apply further OS updates because the hardware[1] is so old that Solaris doesn't support it anymore[2]. [1] https://cfarm.tetaneutral.net/machines/list/ [2] https://support.oracle.com/knowledge/Sun%20Microsystems/2382427_1.html
Re: Error-safe user functions
On 2022-12-15 Th 09:12, Robert Haas wrote: > On Wed, Dec 14, 2022 at 6:24 PM Tom Lane wrote: >> I've gone through these now and revised/pushed most of them. > Tom, I just want to extend huge thanks to you for working on this > infrastructure. jsonpath aside, I think this is going to pay dividends > in many ways for many years to come. It's something that we've needed > for a really long time, and I'm very happy that we're moving forward > with it. > > Thanks so much. > Robert beat me to it, but I will heartily second this. Many thanks. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Add LZ4 compression in pg_dump
001: still refers to "gzip", which is correct for -Fp and -Fd but not for -Fc, for which it's more correct to say "zlib". That affects the name of the function, structures, comments, etc. I'm not sure if it's an issue to re-use the basebackup compression routines here. Maybe we should accept "-Zn" for zlib output (-Fc), but reject "gzip:9", which I'm sure some will find confusing, as it does not output. Maybe 001 should be split into a patch to re-use the existing "cfp" interface (which is a clear win), and 002 to re-use the basebackup interfaces for user input and constants, etc. 001 still doesn't compile on freebsd, and 002 doesn't compile on windows. Have you checked test results from cirrusci on your private github account ? 002 says: + save_errno = errno; + errno = save_errno; I suppose that's intended to wrap the preceding library call. 002 breaks "pg_dump -Fc -Z2" because (I think) AllocateCompressor() doesn't store the passed-in compression_spec. 003 still uses and not "lz4.h". Earlier this year I also suggested to include an 999 patch to change to use LZ4 as the default compression, to exercise the new code under CI. I suggest to re-open the cf patch entry after that passes tests on all platforms and when it's ready for more review. BTW, some of these review comments are the same as what I sent earlier this year. https://www.postgresql.org/message-id/20220326162156.GI28503%40telsasoft.com https://www.postgresql.org/message-id/20220705151328.GQ13040%40telsasoft.com -- Justin
Re: Infinite Interval
On Sat, Dec 17, 2022 at 2:34 PM Joseph Koshakow wrote:
>
> Hi Ashutosh,
>
> I've added tests for all the operators and functions involving
> intervals and what I think the expected behaviors to be. The
> formatting might be slightly off and I've left the contents of the
> error messages as TODOs. Hopefully it's a good reference for the
> implementation.
>
> > Adding infinite interval to an infinite timestamp with opposite
> > direction is not going to yield 0 but some infinity. Since we are adding
> > interval to the timestamp the resultant timestamp is an infinity
> > preserving the direction.
>
> I think I disagree with this. Tom Lane in one of the previous threads
> said:
> > tl;dr: we should model it after the behavior of IEEE float infinities,
> > except we'll want to throw errors where those produce NaNs.
> and I agree with this opinion. I believe that means that adding an
> infinite interval to an infinite timestamp with opposite directions
> should yield an error instead of some infinity. Since with floats this
> would yield a NaN.
>
> > Dividing infinite interval by finite number keeps it infinite.
> > TODO: Do we change the sign of infinity if factor is negative?
> Again if we model this after the IEEE float behavior, then the answer
> is yes, we do change the sign of infinity.
>
> - Joe Koshakow
I ended up doing some more work in the attached patch. Here are some
updates:
- I modified the arithmetic operators to more closely match IEEE
floats. Error messages are still all TODO, and they may have the wrong
error code.
- I implemented some more operators and functions.
- I moved the helper functions you created into macros in timestamp.h
to more closely match the implementation of infinite timestamps and
dates. Also so dates.c could access them.
- There seems to be an existing overflow error with interval
subtraction. Many of the arithmetic operators of the form
`X - Interval` are converted to `X + (-Interval)`. This will overflow
in the case that some interval field is INT32_MIN or INT64_MIN.
Additionally, negating a positive infinity interval won't result in a
negative infinity interval and vice versa. We'll have to come up with
an efficient solution for this.
- Joe Koshakow
From e6e764dd8f8423f2aec0fb3782f170c59557adf6 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 17 Dec 2022 14:21:26 -0500
Subject: [PATCH] This is WIP.
Following things are supported
1. Accepts '+/-infinity' as a valid string input for interval type.
2. Support interval_pl, interval_div
3. Tests in interval.sql for comparison operators working fine.
TODOs
1. Various TODOs in code
2. interval_pl: how to handle infinite values with opposite signs
3. timestamp, timestamptz, date and time arithmetic
4. Fix horology test.
Ashutosh Bapat
---
src/backend/utils/adt/date.c | 20 +
src/backend/utils/adt/datetime.c | 2 +
src/backend/utils/adt/timestamp.c | 188 +++-
src/include/datatype/timestamp.h | 22 +
src/test/regress/expected/interval.out | 613 -
src/test/regress/sql/interval.sql | 121 +
6 files changed, 949 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 1cf7c7652d..a2c9214bcf 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -2073,6 +2073,11 @@ time_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = time + span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2091,6 +2096,11 @@ time_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeADT result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = time - span->time;
result -= result / USECS_PER_DAY * USECS_PER_DAY;
if (result < INT64CONST(0))
@@ -2605,6 +2615,11 @@ timetz_pl_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time + span->time;
@@ -2627,6 +2642,11 @@ timetz_mi_interval(PG_FUNCTION_ARGS)
Interval *span = PG_GETARG_INTERVAL_P(1);
TimeTzADT *result;
+ if (INTERVAL_NOT_FINITE(span))
+ ereport(ERROR,
+(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+ errmsg("TODO")));
+
result = (TimeTzADT *) palloc(sizeof(TimeTzADT));
result->time = time->time - span->time;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index b5b117a8ca..1e98c6dc78 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -3634,6 +3634,8 @@ D
Re: Rework of collation code, extensibility
On Sat, Dec 17, 2022 at 7:14 PM Jeff Davis wrote: > Attached is a new patch series. I think there are enough changes that > this has become more of a "rework" of the collation code rather than > just a refactoring. This is a continuation of some prior work[1][2] in > a new thread given its new scope. > > Benefits: > > 1. Clearer division of responsibilities. > 2. More consistent between libc and ICU providers. > 3. Hooks that allow extensions to replace collation provider libraries. > 4. New tests for the collation provider library hooks. > > There are a lot of changes, and still some loose ends, but I believe a > few of these patches are close to ready. > > This set of changes does not express an opinion on how we might want to > support multiple provider libraries in core; but whatever we choose, it > should be easier to accomplish. Right now, the hooks have limited > information on which to make the choice for a specific version of a > collation provider library, but that's because there's limited > information in the catalog. If the discussion here[3] concludes in > adding collation provider library or library version information to the > catalog, we can add additional parameters to the hooks. > > [1] > > https://postgr.es/m/99aa79cceefd1fe84fda23510494b8fbb7ad1e70.ca...@j-davis.com > [2] > > https://postgr.es/m/c4fda90ec6a7568a896f243a38eb273c3b5c3d93.ca...@j-davis.com > [3] > > https://postgr.es/m/ca+hukgleqmhnpzrgacisoueyfgz8w6ewdhtk2h-4qn0iosf...@mail.gmail.com > > > -- > Jeff Davis > PostgreSQL Contributor Team - AWS > > Hi, For pg_strxfrm_libc in v4-0002-Add-pg_strxfrm-and-pg_strxfrm_prefix.patch: +#ifdef HAVE_LOCALE_T + if (locale) + return strxfrm_l(dest, src, destsize, locale->info.lt); + else +#endif + return strxfrm(dest, src, destsize); It seems the `else` is not needed (since when the if branch is taken, we return from the func). + /* nul-terminate arguments */ nul-terminate -> null-terminate For pg_strnxfrm(), I think `result` can be removed - we directly return the result from pg_strnxfrm_libc or pg_strnxfrm_icu. Cheers
Re: Rework of collation code, extensibility
On Sun, Dec 18, 2022 at 10:28 AM Ted Yu wrote: > It seems the `else` is not needed (since when the if branch is taken, we return from the func). By that same logic, this review comment is not needed, since compiler vendors don't charge license fees by the number of keywords. ;-) Joking aside, we don't really have a project style preference for this case. > nul-terminate -> null-terminate NUL is a common abbreviation for the zero byte (but not for zero pointers). See the ascii manpage. -- John Naylor EDB: http://www.enterprisedb.com
Re: Rework of collation code, extensibility
On Sat, Dec 17, 2022 at 8:54 PM John Naylor wrote: > > > nul-terminate -> null-terminate > > NUL is a common abbreviation for the zero byte (but not for zero > pointers). See the ascii manpage. > > -- > John Naylor > EDB: http://www.enterprisedb.com > > Ah. `nul-terminated` does appear in the codebase. Should have checked earlier.
