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

On Monday, January 23, 2023 5:07 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Here are my review comments for v19-0001.
Thanks for your review !

>
> ======
> Commit message
>
> 1.
> 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.
> ~
>
> I think the above is just cut/paste from a code comment within
> subscriptioncmds.c. See review comments #5 below -- so if the code is
> changed then this commit message should also change to match it.
Now, updated this. Kindly have a look at the latest patch in [1].

>
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 2.
> + <varlistentry>
> + <term><literal>min_apply_delay</literal>
> (<type>integer</type>)</term>
> + <listitem>
> + <para>
> + By default, the subscriber applies changes as soon as possible.
> This
> + parameter allows the user to delay the application of changes by a
> + given time interval. If the value is specified without units, it is
> + taken as milliseconds. The default is zero (no delay).
> + </para>
>
> 2a.
> The pgdocs says this is an integer default to “ms” unit. Also, the example on
> this same page shows it is set to '4h'. But I did not see any mention of what
> other units are available to the user. Maybe other time units should be
> mentioned here, or maybe a link should be given to the section “20.1.1.
> Parameter Names and Values".
Added.

> ~
>
> 2b.
> Previously the word "interval" was deliberately used because this parameter
> had interval support. But maybe now it should be changed so it is not
> misleading.
>
> "a given time interval" --> "a given time period" ??
Fixed.

> ======
> src/backend/commands/subscriptioncmds.c
>
> 3. Forward declare
>
> +static int defGetMinApplyDelay(DefElem *def);
>
> If the new function is implemented as static near the top of this source file then
> this forward declare would not even be necessary, right?
This declaration has been kept as discussed.

> ~~~
>
> 4. parse_subscription_options
>
> @@ -324,6 +328,12 @@ parse_subscription_options(ParseState *pstate, List
> *stmt_options,
> opts->specified_opts |= SUBOPT_LSN;
> opts->lsn = lsn;
> }
> + else if (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
> + strcmp(defel->defname, "min_apply_delay") == 0) {
> + opts->specified_opts |= SUBOPT_MIN_APPLY_DELAY; min_apply_delay =
> + opts->defGetMinApplyDelay(defel);
> + }
>
> Should this code fragment be calling errorConflictingDefElem so it will report
> an error if the same min_apply_delay parameter is redundantly repeated?
> (IIUC, this appears to be the code pattern for other parameters nearby).
Added.

> ~~~
>
> 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.
Now, this has been changed to the one suggested by Amit-san.
Thanks for your help.

> ~~~
>
> 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" ??
Fixed.

> ~
>
> 6b.
> I thought this function should be implemented as static and located at the top
> of the subscriptioncmds.c source file.
Made it static but didn't change the place, as Amit-san mentioned.

> ======
> src/backend/replication/logical/worker.c
>
> 7. maybe_delay_apply
>
> +static void maybe_delay_apply(TransactionId xid, TimestampTz
> +finish_ts);
>
> Is there a reason why this is here? AFAIK the static implementation precedes
> any usage so I doubt this forward declaration is required.
Removed.

> ~~~
>
> 8. send_feedback
>
> @@ -3775,11 +3912,12 @@ send_feedback(XLogRecPtr recvpos, bool force,
> bool requestReply)
> pq_sendint64(reply_message, now); /* sendTime */
> pq_sendbyte(reply_message, requestReply); /* replyRequested */
>
> - elog(DEBUG2, "sending feedback (force %d) to recv %X/%X, write %X/%X,
> flush %X/%X",
> + elog(DEBUG2, "sending feedback (force %d) to recv %X/%X, write
> %X/%X, flush %X/%X in-delayed: %d",
> force,
> LSN_FORMAT_ARGS(recvpos),
> LSN_FORMAT_ARGS(writepos),
> - LSN_FORMAT_ARGS(flushpos));
> + LSN_FORMAT_ARGS(flushpos),
> + in_delayed_apply);
>
> Wondering if it is better to write this as:
> "sending feedback (force %d, in_delayed_apply %d) to recv %X/%X,
> write %X/%X, flush %X/%X"
Adopted and merged with the modification Euler-san provided.

> ~
>
> 10. Add new tests?
>
> Should there be other tests just to verify different units (like 'd', 'h', 'min') are
> working OK?
No need. The current subscription.sql does the check
of "invalid value for parameter..." error message, which ensures we call
the defGetMinApplyDelay(). Additionally, we have the test of one unit 'd'
for unit iteration loopin convert_to_base_unit().
So, the current test sets should suffice.

> ======
> src/test/subscription/t/032_apply_delay.pl
>
> 11.
> +# Confirm the time-delayed replication has been effective from the
> +server log # message where the apply worker emits for applying delay.
> +Moreover, verifies # that the current worker's delayed time is
> +sufficiently bigger than the # expected value, in order to check any update of
> the min_apply_delay.
> +sub check_apply_delay_log
>
> "the current worker's delayed time..." --> "the current worker's remaining wait
> time..." ??
Fixed.

> ~~~
>
> 12.
> + # Get the delay time from the server log my $contents =
> + slurp_file($node_subscriber->logfile, $offset);
>
> "Get the delay time...." --> "Get the remaining wait time..."
Fixed.

> ~~~
>
> 13.
> +# Create a subscription that applies the trasaction after 50
> +milliseconds delay $node_subscriber->safe_psql('postgres',
> + "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
> application_name=$appname' PUBLICATION tap_pub WITH (copy_data = off,
> min_apply_delay = '50ms', streaming = 'on')"
> +);
>
> 13a.
> typo: "trasaction"
Fixed.

> ~
>
> 13b
> 50ms seems an extremely short time – How do you even know if this is testing
> anything related to the time delay? You may just be detecting the normal lag
> between publisher and subscriber without time delay having much to do with
> anything.
The wait time has been updated to 1 second now.
Also, the TAP tests now search for the emitted logs by the apply worker.
The path to emit the log is in the maybe_apply_delay and
it does writes the log only if the "diffms" is bigger than zero,
which invokes the wait. So, this will ensure we use the feature
by this flow.

> ~
>
> 14.
>
> +# Note that we cannot call check_apply_delay_log() here because there
> +is a # possibility that the delay is skipped. The event happens when
> +the WAL # replication between publisher and subscriber is delayed due
> +to a mechanical # problem. The log output will be checked later - substantial
> delay-time case.
> +
> +# Verify that the subscriber lags the publisher by at least 50
> +milliseconds check_apply_delay_time($node_publisher, $node_subscriber,
> +'2', '0.05');
>
> 14a.
> "The event happens..." ??
>
> Did you mean "This might happen if the WAL..."
This part has been removed.

> ~
>
> 14b.
> The log output will be checked later - substantial delay-time case.
>
> I think that needs re-wording to clarify.
> e.g1. you have nothing called a "substantial delay-time" case.
> e.g2. the word "later" confused me. Originally, I thought you meant it is not
> tested yet but that you will check it "later", but now IIUC you are just referring
> to the "1 day 5 minutes" test that comes below in this location TAP file (??)
Also, removed.

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

Best Regards,
Takamichi Osumi