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

On Thursday, January 12, 2023 12:04 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> 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.
Horiguchi-san, thanks for your review !


> +         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..)
Fixed. If necessary we can create another thread for this.

> +            if (strspn(tmp, "-0123456789 ") == strlen(tmp))
>
> Do we need to bother spending another memory block for apparent non-digits
> here?
Yes. The characters are necessary to handle an issue reported in [1].
The issue happened if the user inputs a negative value,
then the length comparison became different between strspn and strlen
and the input value was recognized as seconds, when
the unit wasn't described. This led to a wrong error message for the user.

Those addition of such characters solve the issue.

> +                        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.
Thanks for teaching us. Fixed.

> 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)"
Makes sense. I incorporated the valid range with the aligned format of recovery_min_apply_delay.
FYI, the physical replication's GUC doesn't write the unites for the range like below.
I followed and applied this style.

---
LOG:  -1 ms is outside the valid range for parameter "recovery_min_apply_delay" (0 .. 2147483647)
FATAL:  configuration file "/home/k5user/new/pg/l/make_v15/slave/postgresql.conf" contains errors
---

> +    int64        min_apply_delay;
> ..
> +            if (ms < 0 || ms > INT_MAX)
>
> Why is the variable wider than required?
You are right. Fixed.

> +                    errmsg("%s and %s are mutually
> exclusive options",
> +                           "min_apply_delay > 0",
> "streaming = parallel"));
>
> Mmm. Couldn't we refuse 0 as min_apply_delay?
Sorry, the previous patch's behavior wasn't consistent with this error message.

In the previous patch, if we conducted alter subscription
with stream = parallel and min_apply_delay = 0 (from a positive value) at the same time,
the alter command failed, although this should succeed by this time-delayed feature specification.
We fixed this part accordingly by some more tests in AlterSubscription().

By the way, we should allow users to change min_apply_dealy to 0
whenever they want from different value. Then, we didn't restrict
this kind of operation.

> +                        sub->minapplydelay > 0)
> ...
> +                    if (opts.min_apply_delay > 0 &&
>
> Is there any reason for the differenciation?
Yes. The former is the object for an existing subscription configuration.
For example, if we alter subscription with setting streaming = 'parallel'
for a subscription created with min_apply_delay = '1 day', we
need to reject the alter command. The latter is new settings.


> +
>     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.
Reworded the error messages. Please check.

> +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.
Changed it to finish_ts, since it indicates commit/prepare time.
This terminology should be aligned with finish lsn.

> +    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().
Fixed.


[1] - https://www.postgresql.org/message-id/CALDaNm3Bpzhh60nU-keuGxMPb-OhcqsfpCN3ysfCfCJ-2ShYPA%40mail.gmail.com


Best Regards,
    Takamichi Osumi