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

On Mon, Jan 23, 2023 at 9:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jan 23, 2023 at 1:36 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here are my review comments for v19-0001.
> >
> ...
> >
> > 5. parse_subscription_options
> >
> > + /*
> > + * The combination of parallel streaming mode and min_apply_delay is not
> > + * allowed. The subscriber in the parallel streaming mode applies each
> > + * stream on arrival without the time of commit/prepare. So, the
> > + * subscriber needs to depend on the arrival time of the stream in this
> > + * case, if we apply the time-delayed feature for such transactions. Then
> > + * there is a possibility where some unnecessary delay will be added on
> > + * the subscriber by network communication break between nodes or other
> > + * heavy work load on the publisher. On the other hand, applying the delay
> > + * at the end of transaction with parallel apply also can cause issues of
> > + * used resource bloat and locks kept in open for a long time. Thus, those
> > + * features can't work together.
> > + */
> >
> > IMO some re-wording might be warranted here. I am not sure quite how
> > to do it. Perhaps like below?
> >
> > SUGGESTION
> >
> > The combination of parallel streaming mode and min_apply_delay is not allowed.
> >
> > Here are some reasons why these features are incompatible:
> > a. In the parallel streaming mode the subscriber applies each stream
> > on arrival without knowledge of the commit/prepare time. This means we
> > cannot calculate the underlying network/decoding lag between publisher
> > and subscriber, and so always waiting for the full 'min_apply_delay'
> > period might include unnecessary delay.
> > b. If we apply the delay at the end of the transaction of the parallel
> > apply then that would cause issues related to resource bloat and locks
> > being held for a long time.
> >
> > ~~~
> >
>
> How about something like:
> The combination of parallel streaming mode and min_apply_delay is not
> allowed. This is because we start applying the transaction stream as
> soon as the first change arrives without knowing the transaction's
> prepare/commit time. This means we cannot calculate the underlying
> network/decoding lag between publisher and subscriber, and so always
> waiting for the full 'min_apply_delay' period might include
> unnecessary delay.
>
> The other possibility is to apply the delay at the end of the parallel
> apply transaction but that would cause issues related to resource
> bloat and locks being held for a long time.
>

+1. That's better.

>
> > 6. defGetMinApplyDelay
> >
...
> >
> > 6b.
> > I thought this function should be implemented as static and located at
> > the top of the subscriptioncmds.c source file.
> >
>
> I agree that this should be a static function but I think its current
> location is a better place as other similar function is just above it.
>

But, why not do everything, instead of settling on a half-fix?

e.g.
1. Change the new function (defGetMinApplyDelay) to be static as it should be
2. And move defGetMinApplyDelay to the top of the file where IMO it
really belongs
3. And then remove the (now) redundant forward declaration of
defGetMinApplyDelay
4. And also move the existing function (defGetStreamingMode) to the
top of the file so that those similar functions (defGetMinApplyDelay
and defGetStreamingMode) can remain together

------
Kind Regards,
Peter Smith.
Fujitsu Australia