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?
--
With Regards,
Amit Kapila.