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

On Monday, January 23, 2023 9:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sun, Jan 22, 2023 at 6:12 PM Takamichi Osumi (Fujitsu)
> <osumi.takamichi@fujitsu.com> wrote:
> >
> >
> > Attached the updated patch v19.
> >
> 
> Few comments:
> =============
> 1.
> }
> +
> +
> +/*
> 
> Only one empty line is sufficient between different functions.
Fixed.


> 2.
> + if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> + opts->min_apply_delay > 0 && opts->streaming ==
> + opts->LOGICALREP_STREAM_PARALLEL)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("%s and %s are mutually exclusive options",
> +    "min_apply_delay > 0", "streaming = parallel"));
>  }
> 
> I think here we should add a comment for the translator as we are doing in
> some other nearby cases.
Fixed.


> 3.
> + /*
> + * The combination of parallel streaming mode and
> + * min_apply_delay is not allowed.
> + */
> + 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))
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot enable %s mode for subscription with %s",
> +    "streaming = parallel", "min_apply_delay"));
> +
> 
> A. When can second condition ((!IsSet(opts.specified_opts,
> SUBOPT_MIN_APPLY_DELAY) && sub->minapplydelay > 0)) in above check
> be true?
> B. In comments, you can say "See parse_subscription_options."
(1) In the alter statement, streaming = parallel is set.
Also, (2) in the alter statement, min_apply_delay isn't set.
and (3) an existing subscription has non-zero min_apply_delay.

Added the comment.
> 4.
> +/*
> + * When min_apply_delay parameter is set on the subscriber, we wait
> +long enough
> + * to make sure a transaction is applied at least that interval behind
> +the
> + * publisher.
> 
> Shouldn't this part of the comment needs to be updated after the patch has
> stopped using interval?
Yes. I removed "interval" in descriptions so that we don't get
confused with types.


> 5. How does this feature interacts with the SKIP feature? Currently, it doesn't
> care whether the changes of a particular xact are skipped or not. I think that
> might be okay because anyway the purpose of this feature is to make
> subscriber lag from publishers. What do you think?
> I feel we can add some comments to indicate the same.
Added the comment in the commit message.
I didn't add this kind of comment as code comments,
since both features are independent. If there is a need to write it anywhere,
then please let me know. The latest patch is posted in [1].

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



Best Regards,
    Takamichi Osumi