MIME-Version: 1.0
References: <CAA4eK1+NUeb21bYa9K5mVsFHcw-xodGLkN9QoqJXym7sD3LtZA@mail.gmail.com>
 <CALj2ACVr8ra-yp8CwiBq=Pc9Kkq64z=zDvoFxMK3K4wSDhKMmg@mail.gmail.com>
 <CAA4eK1KxsMg4bnxRZ0U3dZ3irFoR2=TfUCOFig3ccHoRPp5iWg@mail.gmail.com>
 <CALj2ACU-sJdiLfAcWd+8hnt4HSojH0UWFGnaGr9gxb2bYzYYSA@mail.gmail.com>
 <CAA4eK1++=fOpmsQKugQQRtQr2wmiintjKEhpuOch9=phxirAfg@mail.gmail.com>
 <CALj2ACVd_dizYQiZwwUfsb+hG-fhGYo_kEDq0wn_vNwQvOrZHg@mail.gmail.com>
 <CAA4eK1+02XhKht3EB=RgzQ=2zyb9Sv4=9iC=yt1j4FfVy0-5kg@mail.gmail.com>
 <CALj2ACVRS2LZbycpM8rZT8QynR05AtvttOHfFOhczjyqU7zZSA@mail.gmail.com>
 <CAJpy0uChxSm7pod6CQwU4z4pSvV03b4iRCy_9zb0zVbSs__qOg@mail.gmail.com>
 <CALj2ACWBbSa7ccOGDHNfH1OX7_ArBJm1kNmc=iWKSfqq1NHnuA@mail.gmail.com> <ZfgTf77NL2xOSldF@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <ZfgTf77NL2xOSldF@ip-10-97-1-34.eu-west-3.compute.internal>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 20 Mar 2024 11:17:47 +0530
Message-ID: <CALj2ACW=pTH3Op-biAjJMgCyRREFDiSiCxKuN91Q98L4yDfi8A@mail.gmail.com>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, 
	Nathan Bossart <nathandbossart@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALj2ACW%3DpTH3Op-biAjJMgCyRREFDiSiCxKuN91Q98L4yDfi8A%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 18, 2024 at 3:42=E2=80=AFPM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> Looking at 0001:

Thanks for reviewing.

> 1 =3D=3D=3D
>
> +       True if this logical slot conflicted with recovery (and so is now
> +       invalidated). When this column is true, check
>
> Worth to add back the physical slot mention "Always NULL for physical slo=
ts."?

Will change.

> 2 =3D=3D=3D
>
> @@ -1023,9 +1023,10 @@ CREATE VIEW pg_replication_slots AS
>              L.wal_status,
>              L.safe_wal_size,
>              L.two_phase,
> -            L.conflict_reason,
> +            L.conflicting,
>              L.failover,
> -            L.synced
> +            L.synced,
> +            L.invalidation_reason
>
> What about making invalidation_reason close to conflict_reason?

Not required I think. One can pick the required columns in the SELECT
clause anyways.

> 3 =3D=3D=3D
>
> - * Maps a conflict reason for a replication slot to
> + * Maps a invalidation reason for a replication slot to
>
> s/a invalidation/an invalidation/?

Will change.

> 4 =3D=3D=3D
>
> While at it, shouldn't we also rename "conflict" to say "invalidation_cau=
se" in
> InvalidatePossiblyObsoleteSlot()?

That's inline with our understanding about conflict vs invalidation,
and keeps the function generic. Will change.

> 5 =3D=3D=3D
>
> + * rows_removed and wal_level_insufficient are only two reasons
>
> s/are only two/are the only two/?

Will change..

--=20
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com