On Fri, Feb 3, 2023 at 3:12 PM wangw.f...@fujitsu.com <wangw.f...@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.
