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. 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.
