Hi,
On Friday, January 27, 2023 8:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jan 27, 2023 at 1:39 PM Takamichi Osumi (Fujitsu)
> <osumi.takamichi@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