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

On Mon, Jan 30, 2023 at 12:38 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Mon, 30 Jan 2023 11:56:33 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in
> > >
> > > 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.
>

But here tz is always TimestampTz (which is int64), so do, we need to worry?

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

I am not aware of such cases. Anyway, if any such case is discovered
then we need to change the checks in defGetMinApplyDelay(), right? If
so, then I think it is better to keep it as it is unless we know that
this could be an issue on some platform.

--
With Regards,
Amit Kapila.