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

On Fri, Jan 27, 2023 at 1:39 PM Takamichi Osumi (Fujitsu)
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Wednesday, January 25, 2023 11:24 PM I wrote:
> > Attached the updated v22.
> Hi,
>
> During self-review, I noticed some changes are
> required for some variable types related to 'min_apply_delay' value,
> so have conducted the adjustment changes for the same.
>

So, you have changed min_apply_delay from int64 to int32, but you
haven't mentioned the reason for the same? We use 'int' for the
similar parameter recovery_min_apply_delay, so, ideally, it makes
sense but still better to tell your reason explicitly.

Few comments
=============
1.
@@ -70,6 +70,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
XLogRecPtr subskiplsn; /* All changes finished at this LSN are
* skipped */

+ int32 subminapplydelay; /* Replication apply delay (ms) */
+
NameData subname; /* Name of the subscription */

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

Why are you placing this after subskiplsn? Earlier it was okay because
we want the 64 bit value to be aligned but now, isn't it better to
keep it after subowner?

2.
+
+ diffms = TimestampDifferenceMilliseconds(GetCurrentTimestamp(),
+ TimestampTzPlusMilliseconds(finish_ts, MySubscription->minapplydelay));

The above code appears a bit unreadable. Can we store the result of
TimestampTzPlusMilliseconds() in a separate variable say "TimestampTz
delayUntil;"?

--
With Regards,
Amit Kapila.