Re: Time delayed LR (WAS Re: logical replication restrictions)

At Wed, 11 Jan 2023 12:46:24 +0000, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> wrote in
> them. Which version is better?

Some comments by a quick loock, different from the above.

+ CONNECTION 'host=192.168.1.50 port=5432 user=foo dbname=foodb'

I understand that we (not PG people, but IT people) are supposed to
use in documents a certain set of special addresses that is guaranteed
not to be routed in the field.

> TEST-NET-1 : 192.0.2.0/24
> TEST-NET-2 : 198.51.100.0/24
> TEST-NET-3 : 203.0.113.0/24

(I found 192.83.123.89 in the postgres_fdw doc, but it'd be another issue..)

+ if (strspn(tmp, "-0123456789 ") == strlen(tmp))

Do we need to bother spending another memory block for apparent
non-digits here?

+ errmsg(INT64_FORMAT " ms is outside the valid range for parameter \"%s\"",

We don't use INT64_FORMAT in translatable message strings. Cast then
use %lld instead.

This message looks unfriendly as it doesn't suggest the valid range,
and it shows the input value in a different unit from what was in the
input. A I think we can spell it as "\"%s\" is outside the valid range
for subsciription parameter \"%s\" (0 .. <INT_MAX> in millisecond)"

+ int64 min_apply_delay;
..
+ if (ms < 0 || ms > INT_MAX)

Why is the variable wider than required?

+ errmsg("%s and %s are mutually exclusive options",
+ "min_apply_delay > 0", "streaming = parallel"));

Mmm. Couldn't we refuse 0 as min_apply_delay?

+ sub->minapplydelay > 0)
...
+ if (opts.min_apply_delay > 0 &&

Is there any reason for the differenciation?

+ errmsg("cannot set %s for subscription with %s",
+ "streaming = parallel", "min_apply_delay > 0"));

I think that this shoud be more like human-speking. Say, "cannot
enable min_apply_delay for subscription in parallel streaming mode" or
something.. The same is applicable to the nearby message.

+static void maybe_delay_apply(TimestampTz ts);

apply_spooled_messages(FileSet *stream_fileset, TransactionId xid,
- XLogRecPtr lsn)
+ XLogRecPtr lsn, TimestampTz ts)

"ts" looks too generic. Couldn't it be more specific?
We need a explanation for the parameter in the function comment.

+ if (!am_parallel_apply_worker())
+ {
+ Assert(ts > 0);
+ maybe_delay_apply(ts);

It seems to me better that the if condition and assertion are checked
inside maybe_delay_apply().

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center