Re: Time delayed LR (WAS Re: logical replication restrictions) - Mailing list pgsql-hackers

From Kyotaro Horiguchi
Subject Re: Time delayed LR (WAS Re: logical replication restrictions)
Date
Msg-id 20230201.114304.1001055277411106736.horikyota.ntt@gmail.com
Whole thread Raw
In response to Re: Time delayed LR (WAS Re: logical replication restrictions)  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: Time delayed LR (WAS Re: logical replication restrictions)
List pgsql-hackers
At Tue, 31 Jan 2023 15:12:14 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Tue, Jan 31, 2023 at 1:40 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > Hi, Kuroda-san, Thanks for the detailed study.
> >
> > At Tue, 31 Jan 2023 07:06:40 +0000, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.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



pgsql-hackers by date:

Previous
From: Justin Pryzby
Date:
Subject: Re: Generating "Subplan Removed" in EXPLAIN
Next
From: vignesh C
Date:
Subject: Re: [Commitfest 2023-01] has started