At Wed, 11 Jan 2023 12:46:24 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.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
