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

On Tue, Feb 7, 2023 at 8:22 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Thank you for reviewing! PSA new version.
>

Few comments:
=============
1.
@@ -74,6 +74,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW

Oid subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */

+ int32 subminapplydelay; /* Replication apply delay (ms) */
+
bool subenabled; /* True if the subscription is enabled (the
* worker should be running) */

@@ -120,6 +122,7 @@ typedef struct Subscription
* in */
XLogRecPtr skiplsn; /* All changes finished at this LSN are
* skipped */
+ int32 minapplydelay; /* Replication apply delay (ms) */
char *name; /* Name of the subscription */
Oid owner; /* Oid of the subscription owner */

Why the new parameter is placed at different locations in above two
strcutures? I think it should be after owner in both cases and
accordingly its order should be changed in GetSubscription() or any
other place it is used.

2. A minor comment change suggestion:
/*
* Common spoolfile processing.
*
- * The commit/prepare time (finish_ts) for streamed transactions is required
- * for time-delayed logical replication.
+ * The commit/prepare time (finish_ts) is required for time-delayed logical
+ * replication.
*/

3. I find the newly added tests take about 8s on my machine which is
close highest in the subscription folder. I understand that it can't
be less than 3s because of the delay but checking multiple cases makes
it take that long. I think we can avoid the tests for streaming and
disable the subscription. Also, after removing those, I think it would
be better to add the remaining test in 001_rep_changes to save set-up
and tear-down costs as well.

4.
+$node_publisher->append_conf('postgresql.conf',
+ 'logical_decoding_work_mem = 64kB');

I think this setting is also not required.

--
With Regards,
Amit Kapila.