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

On Fri, Feb 17, 2023 at 12:14 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Thank you for replying! This direction seems OK, so I started to revise the patch.
> PSA new version.
>

Few comments:
=============
1.
+ <para>
+ The minimum delay for publisher sends data, in milliseconds
+ </para></entry>
+ </row>

It would probably be better to write it as "The minimum delay, in
milliseconds, by the publisher to send changes"

2. The subminsenddelay is placed inconsistently in the patch. In the
docs (catalogs.sgml), system_views.sql, and in some places in the
code, it is after subskiplsn, but in the catalog table and
corresponding structure, it is placed after subowner. It should be
consistently placed after the subscription owner.

3.
+ <row>
+ <entry><literal>WalSenderSendDelay</literal></entry>
+ <entry>Waiting for sending changes to subscriber in WAL sender
+ process.</entry>

How about writing it as follows: "Waiting while sending changes for
time-delayed logical replication in the WAL sender process."?

4.
+ <para>
+ Any delay becomes effective only after all initial table
+ synchronization has finished and occurs before each transaction
+ starts to get applied on the subscriber. The delay does not take into
+ account the overhead of time spent in transferring the transaction,
+ which means that the arrival time at the subscriber may be delayed
+ more than the given time.
+ </para>

This needs to change based on a new approach. It should be something
like: "The delay is effective only when the publisher decides to send
a particular transaction downstream."

5.
+ * allowed. This is because in parallel streaming mode, we start applying
+ * the transaction stream as soon as the first change arrives without
+ * knowing the transaction's prepare/commit time. Always waiting for the
+ * full 'min_send_delay' period might include unnecessary delay.
+ *
+ * The other possibility was 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.
+ */

This part of the comments seems to imply more of a subscriber-side
delay approach. I think we should try to adjust these as per the
changed approach.

6.
@@ -666,6 +666,10 @@ DecodeCommit(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf,
buf->origptr, buf->endptr);
}

+ /* Delay given time if the context has 'delay' callback */
+ if (ctx->delay)
+ ctx->delay(ctx, commit_time);
+

I think we should invoke delay functionality only when
ctx->min_send_delay > 0. Otherwise, there will be some unnecessary
overhead. We can change the comment along the lines of: "Delay sending
the changes if required. For streaming transactions, this means a
delay in sending the last stream but that is okay because on the
downstream the changes will be applied only after receiving the last
stream."

7. For 2PC transactions, I think we should add the delay in
DecodePrerpare. Because after receiving the PREPARE, the downstream
will apply the xact. In this case, we shouldn't add a delay for the
commit_prepared.

8.
+#
+# If the subscription sets min_send_delay parameter, the logical replication
+# worker will delay the transaction apply for min_send_delay milliseconds.

I think here also comments should be updated as per the changed
approach for applying the delay on the publisher side.

--
With Regards,
Amit Kapila.