At Mon, 30 Jan 2023 11:56:33 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in > On Mon, Jan 30, 2023 at 9:43 AM Kyotaro Horiguchi > <horikyota....@gmail.com> wrote: > > > > At Mon, 30 Jan 2023 08:51:05 +0530, Amit Kapila <amit.kapil...@gmail.com> > > wrote in > > > On Mon, Jan 30, 2023 at 8:32 AM Kyotaro Horiguchi > > > <horikyota....@gmail.com> wrote: > > > > > > > > At Sat, 28 Jan 2023 04:28:29 +0000, "Takamichi Osumi (Fujitsu)" > > > > <osumi.takami...@fujitsu.com> wrote in > > > > > On Friday, January 27, 2023 8:00 PM Amit Kapila > > > > > <amit.kapil...@gmail.com> wrote: > > > > > > 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. > > > > > > > > INT_MAX can stick out of int32 on some platforms. (I'm not sure where > > > > that actually happens, though.) We can use PG_INT32_MAX instead. > > > > > > > > > > But in other integer GUCs including recovery_min_apply_delay, we use > > > INT_MAX, so not sure if it is a good idea to do something different > > > here. > > > > The GUC is not stored in a catalog, but.. oh... it is multiplied by > > 1000. > > Which part of the patch you are referring to here? Isn't the check in
Where recovery_min_apply_delay is used. It is allowed to be set up to INT_MAX but it is used as: > delayUntil = TimestampTzPlusMilliseconds(xtime, > recovery_min_apply_delay); Where the macro is defined as: > #define TimestampTzPlusMilliseconds(tz,ms) ((tz) + ((ms) * (int64) 1000)) Which can lead to overflow, which is practically harmless. > the function defGetMinApplyDelay() sufficient to ensure that the > 'delay' value stored in the catalog will always be lesser than > INT_MAX? I'm concerned about cases where INT_MAX is wider than int32. If we don't assume such cases, I'm fine with INT_MAX there. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
