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

Here are my review comments for v19-0001.

======
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.

======
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".

~

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" ??

======
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?

~~~

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;
+ opts->min_apply_delay = 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).

~~~

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.

~~~

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.

======
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.

~~~

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"

======
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)"

(this is related to the review comment #4)

~

10. Add new tests?

Should there be other tests just to verify different units (like 'd',
'h', 'min') are working OK?

======
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..." ??

~~~

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..."

~~~

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"

~

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.

~

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..."

~

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 (??)

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