Re: Time delayed LR (WAS Re: logical replication restrictions) - Mailing list pgsql-hackers

From Kyotaro Horiguchi
Subject Re: Time delayed LR (WAS Re: logical replication restrictions)
Date
Msg-id 20230112.120338.534662300633460797.horikyota.ntt@gmail.com
Whole thread Raw
In response to RE: Time delayed LR (WAS Re: logical replication restrictions)  ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>)
Responses RE: Time delayed LR (WAS Re: logical replication restrictions)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Amit Langote
Date:
Subject: Re: ATTACH PARTITION seems to ignore column generation status
Next
From: "Takamichi Osumi (Fujitsu)"
Date:
Subject: RE: Allow logical replication to copy tables in binary format