Re: Time delayed LR (WAS Re: logical replication restrictions)

At Tue, 31 Jan 2023 15:12:14 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in
> On Tue, Jan 31, 2023 at 1:40 PM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >
> > Hi, Kuroda-san, Thanks for the detailed study.
> >
> > At Tue, 31 Jan 2023 07:06:40 +0000, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> wrote in
> > > Therefore, I think we can say that modern platforms that are supported by PostgreSQL define int as 32-bit.
> > > It satisfies the condition sizeof(int) <= sizeof(int32), so we can keep to use INT_MAX.
> >
> > Yeah, I know that that's practically correct. Just I wanted to make
> > clear is whether we (always) assume int == int32. I don't want to do
> > that just because that works. Even though we cannot be perfect, in
> > this particular case the destination space is explicitly made as
> > int32.
> >
>
> So, shall we check if the result of parse_int is in the range 0 and
> PG_INT32_MAX to ameliorate this concern?

Yeah, it is exactly what I wanted to suggest.

> If this works then we need to
> probably change the return value of defGetMinApplyDelay() to int32.

I didn't thought doing that, int can store all values in the valid
range (I'm assuming we implicitly assume int >= int32 in bit width)
and it is the natural integer in C. Either will do for me but I
slightly prefer to use int there.

As the result I'd like to propose the following change.

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 489eae85ee..9de2745623 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2293,16 +2293,16 @@ defGetMinApplyDelay(DefElem *def)
hintmsg ? errhint("%s", _(hintmsg)) : 0));

/*
- * Check lower bound. parse_int() has already been confirmed that result
- * is less than or equal to INT_MAX.
+ * Check the both boundary. Although parse_int() checked the result against
+ * INT_MAX, this value is to be stored in a catalog column of int32.
*/
- if (result < 0)
+ if (result < 0 || result > PG_INT32_MAX)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%d ms is outside the valid range for parameter \"%s\" (%d .. %d)",
result,
"min_apply_delay",
- 0, INT_MAX)));
+ 0, PG_INT32_MAX)));

return result;
}

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center