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

Hi,


On wangw.fnst@fujitsu.com Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Feb 3, 2023 at 3:12 PM wangw.fnst@fujitsu.com
> <wangw.fnst@fujitsu.com> wrote:
> >
> > Here is a comment:
> >
> > 1. The checks in function AlterSubscription
> > +                                       /*
> > +                                        * The combination of parallel
> streaming mode and
> > +                                        * min_apply_delay is not
> allowed. See
> > +                                        * parse_subscription_options
> for details of the reason.
> > +                                        */
> > +                                       if (opts.streaming ==
> LOGICALREP_STREAM_PARALLEL)
> > +                                               if
> ((IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> opts.min_apply_delay > 0) ||
> > +
> > + (!IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> > + sub->minapplydelay > 0))
> > and
> > +                                       /*
> > +                                        * The combination of parallel
> streaming mode and
> > +                                        * min_apply_delay is not
> allowed.
> > +                                        */
> > +                                       if (opts.min_apply_delay > 0)
> > +                                               if
> ((IsSet(opts.specified_opts, SUBOPT_STREAMING) && opts.streaming ==
> LOGICALREP_STREAM_PARALLEL) ||
> > +
> > + (!IsSet(opts.specified_opts, SUBOPT_STREAMING) && sub->stream ==
> > + LOGICALREP_STREAM_PARALLEL))
> >
> > I think the case where the options "min_apply_delay>0" and
> "streaming=parallel"
> > are set at the same time seems to have been checked in the function
> > parse_subscription_options, how about simplifying these two
> > if-statements here to the following:
> > ```
> > if (opts.streaming == LOGICALREP_STREAM_PARALLEL &&
> >         !IsSet(opts.specified_opts, SUBOPT_MIN_APPLY_DELAY) &&
> >         sub->minapplydelay > 0)
> >
> > and
> >
> > if (opts.min_apply_delay > 0 &&
> >         !IsSet(opts.specified_opts, SUBOPT_STREAMING) &&
> >         sub->stream == LOGICALREP_STREAM_PARALLEL) ```
> >
>
> Won't just checking if ((opts.streaming ==
> LOGICALREP_STREAM_PARALLEL && sub->minapplydelay > 0) ||
> (opts.min_apply_delay > 0 && sub->stream ==
> LOGICALREP_STREAM_PARALLEL)) be sufficient in that case?
We need checks for !IsSet(). If we don't have those,
we error out when executing the alter subscription with min_apply_delay = 0
and streaming = parallel, at the same time for a subscription whose min_apply_delay
setting is bigger than 0, for instance. In this case, we pass (don't error out)
parse_subscription_options()'s test for the combination of mutual exclusive options
and then, error out the condition by matching the first condition
opts.streaming == parallel and sub->minapplydelay > 0 above.

Also, the Wang-san's refactoring proposal makes sense. Adopted.
Regarding the style how to write min_apply_delay > 0
(or just putting min_apply_delay in 'if' conditions) for checking parameters,
I agreed with Amit-san so I keep them as it is in the latest patch v27.


Kindly have a look at v27 posted in [1]

[1] -
https://www.postgresql.org/message-id/TYCPR01MB83738F2BEF83DE525410E3ACEDD49%40TYCPR01MB8373.jpnprd01.prod.outlook.com



Best Regards,
    Takamichi Osumi