On Monday, January 30, 2023 12:02 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> At Sat, 28 Jan 2023 04:28:29 +0000, "Takamichi Osumi (Fujitsu)"
> <osumi.takamichi@fujitsu.com> wrote in
> > On Friday, January 27, 2023 8:00 PM Amit Kapila
> <amit.kapila16@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.
>
> IMHO, I think we don't use int as a catalog column and I agree that
> int32 is sufficient since I don't think more than 49 days delay is practical. On
> the other hand, maybe I wouldn't want to use int32 for intermediate
> calculations.
Hi, Horiguchi-san. Thanks for your comments !
IIUC, in the last sentence, you proposed the type of
SubOpts min_apply_delay should be change to "int". But
I couldn't find actual harm of the current codes, because
we anyway insert the SubOpts value to the catalog after holding it in SubOpts.
Also, it seems there is no explicit rule where we should use "int" local variables
for "int32" system catalog values internally. I had a look at other
variables for int32 system catalog members and either looked fine.
So, I'd like to keep the current code as it is, until actual harm is found.
The latest patch can be seen in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373E26884C385EFFFB8965FEDD39%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi