MIME-Version: 1.0
References: <CAHut+Pvi-g+9+hjmjg44OzTN9L3YGQiCXBDAVaTVWvSn5SSwmw@mail.gmail.com>
 <CABdArM4QR7LPoJ_ES+_1miQZ9ziwsOUun4FGSDhPdrxsugH_0A@mail.gmail.com> <TYAPR01MB56927564EEE26E5433198405F5292@TYAPR01MB5692.jpnprd01.prod.outlook.com>
In-Reply-To: <TYAPR01MB56927564EEE26E5433198405F5292@TYAPR01MB5692.jpnprd01.prod.outlook.com>
From: Nisha Moond <nisha.moond412@gmail.com>
Date: Fri, 29 Nov 2024 18:06:56 +0530
Message-ID: <CABdArM5ENt1fFxGjx6-NQ863ia1U9poqC7XCMcc178syr7sdYg@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: vignesh C <vignesh21@gmail.com>, shveta malik <shveta.malik@gmail.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	Amit Kapila <amit.kapila16@gmail.com>, Ajin Cherian <itsajin@gmail.com>, 
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, 
	Nathan Bossart <nathandbossart@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Peter Smith <smithpb2250@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CABdArM5ENt1fFxGjx6-NQ863ia1U9poqC7XCMcc178syr7sdYg%40mail.gmail.com>
Precedence: bulk

On Thu, Nov 28, 2024 at 1:29=E2=80=AFPM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Nisha,
>
> >
> > Attached v51 patch-set addressing all comments in [1] and [2].
> >
>
> Thanks for working on the feature! I've stated to review the patch.
> Here are my comments - sorry if there are something which have already be=
en discussed.
> The thread is too long to follow correctly.
>
> Comments for 0001
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> 01. binary_upgrade_logical_slot_has_caught_up
>
> ISTM that error_if_invalid is set to true when the slot can be moved forw=
ard, otherwise
> it is set to false. Regarding the binary_upgrade_logical_slot_has_caught_=
up, however,
> only valid slots will be passed to the funciton (see pg_upgrade/info.c) s=
o I feel
> it is OK to set to true. Thought?
>

Right, corrected the call with error_if_invalid as true.

> Comments for 0002
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> 03. check_replication_slot_inactive_timeout
>
> Can we overwrite replication_slot_inactive_timeout to zero when pg_uprade=
 (and also
> pg_createsubscriber?) starts a server process? Several parameters have al=
ready been
> specified via -c option at that time. This can avoid an error while the u=
pgrading.
> Note that this part is still needed even if you accept the comment. Users=
 can
> manually boot with upgrade mode.
>

Done.

> 06. found bug
>
> While testing the patch, I found that slots can be invalidated too early =
when when
> the GUC is quite large. I think because an overflow is caused in Invalida=
tePossiblyObsoleteSlot().
>
> - Reproducer
>
> I set the replication_slot_inactive_timeout to INT_MAX and executed below=
 commands,
> and found that the slot is invalidated.
>
> ```
> postgres=3D# SHOW replication_slot_inactive_timeout;
>  replication_slot_inactive_timeout
> -----------------------------------
>  2147483647s
> (1 row)
> postgres=3D# SELECT * FROM pg_create_logical_replication_slot('test', 'te=
st_decoding');
>  slot_name |    lsn
> -----------+-----------
>  test      | 0/18B7F38
> (1 row)
> postgres=3D# CHECKPOINT ;
> CHECKPOINT
> postgres=3D# SELECT slot_name, inactive_since, invalidation_reason FROM p=
g_replication_slots ;
>  slot_name |        inactive_since         | invalidation_reason
> -----------+-------------------------------+---------------------
>  test      | 2024-11-28 07:50:25.927594+00 | inactive_timeout
> (1 row)
> ```
>
> - analysis
>
> In InvalidatePossiblyObsoleteSlot(), replication_slot_inactive_timeout_se=
c * 1000
> is passed to the third argument of TimestampDifferenceExceeds(), which is=
 also the
> integer datatype. This causes an overflow and parameter is handled as the=
 small
> value.
>
> - solution
>
> I think there are two possible solutions. You can choose one of them:
>
> a. Make the maximum INT_MAX/1000, or
> b. Change the unit to millisecond.
>

Fixed. It is reasonable to align with other timeout parameters by
using milliseconds as the unit.

--
Thanks,
Nisha