Hi, On Friday, January 27, 2023 8:00 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > On Fri, Jan 27, 2023 at 1:39 PM Takamichi Osumi (Fujitsu) > <osumi.takami...@fujitsu.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. Yes. It's because I thought I need to make this feature consistent with the recovery_min_apply_delay. This feature handles the range same as the recovery_min_apply delay from 0 to INT_MAX now so should be adjusted to match it.
> 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?
Moved 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;"?
Agreed. Fixed.
Attached the updated patch v24.
Best Regards,
Takamichi Osumi
v24-0001-Time-delayed-logical-replication-subscriber.patch
Description: v24-0001-Time-delayed-logical-replication-subscriber.patch
