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