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

On Mon, Jan 23, 2023 at 1:36 PM Peter Smith <smithpb2250(at)gmail(dot)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.

> 6. defGetMinApplyDelay
>
> +
> +
> +/*
> + * Extract the min_apply_delay mode value from a DefElem. This is very similar
> + * to PGC_INT case of parse_and_validate_value(), because min_apply_delay
> + * accepts the same string as recovery_min_apply_delay.
> + */
> +int
> +defGetMinApplyDelay(DefElem *def)
>
> 6a.
> "same string" -> "same parameter format" ??
>
> ~
>
> 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.

>
> ======
> src/test/regress/sql/subscription.sql
>
> 9. Add new test?
>
> Should there be an additional test to check redundant parameter
> setting -- eg. "... WITH (min_apply_delay=123, min_apply_delay=456)"
>

I don't think that will be of much help. We don't seem to have other
tests for subscription parameters.

--
With Regards,
Amit Kapila.