Horton

Re: Introduce XID age and inactive timeout based replication slot invalidation

Nisha Moond <nisha.moond412@gmail.com>

View thread
From: Nisha Moond <nisha.moond412@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Shlok Kyal <shlok.kyal.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@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>
Date: 2025-01-27T05:30:15Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add a test for commit ac0e33136a using the injection point.

  2. Invalidate inactive replication slots.

  3. Fix incorrect slot type in BuildTupleHashTableExt

  4. Allow synced slots to have their inactive_since.

  5. Change last_inactive_time to inactive_since in pg_replication_slots.

  6. Track last_inactive_time in pg_replication_slots.

  7. Track invalidation_reason in pg_replication_slots.

  8. Add option force_initdb to PostgreSQL::Test::Cluster:init()

  9. Add a failover option to subscriptions.

  10. Allow setting failover property in the replication command.

  11. Allow to enable failover property for replication slots via SQL API.

  12. Track conflict_reason in pg_replication_slots.

  13. Log messages for replication slot acquisition and release.

  14. Remove vacuum_defer_cleanup_age

  15. Fix corruption due to vacuum_defer_cleanup_age underflowing 64bit xids

  16. meson: Add initial version of meson based build system

Attachments

On Wed, Jan 22, 2025 at 10:49 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Tue, Jan 21, 2025 at 8:22 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Some review comments for patch v61-0001.
> >
> > ======
> > src/backend/replication/slot.c
> >
> > 2.
> > + /*
> > + * The logical replication slots shouldn't be invalidated as GUC
> > + * max_slot_wal_keep_size is set to -1 during the binary upgrade.
> > + * See check_old_cluster_for_valid_slots() where we ensure that no
> > + * invalidated before the upgrade.
> > + */
> > + Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));
> >
> > 2a.
> > I know this sentence was the same before you moved it, but "ensure
> > that no invalidated" seems like there are some words missing.
> >
>
> Corrected.
>
> > 2b.
> > TBH, I this part confused me (the code is repeated in a couple of
> > places). AFAICT the code/comment does not match quite right. The
> > comment refers to a setting that is "during binary upgrade", yet the
> > Assert can only pass if IsBinaryUpgrade is false. (??)
> >
> > I'm unsure of the intent; perhaps it should be like:
> >
> > if (IsBinaryUpgrade)
> > Assert(!(*invalidated && SlotIsLogical(s)));
> >
>
> This Assert condition is correct, as we don’t want to invalidate slots
> during a binary upgrade and it only triggers(raise error) when all
> three conditions are true, meaning when 'IsBinaryUpgrade' is also
> true.
> That said, I agree with your point that we are unnecessarily checking
> "(*invalidated && SlotIsLogical(s))" even when not in binary upgrade
> mode.
> To optimize this, we can first check 'IsBinaryUpgrade' before
> evaluating the other conditions:
>     Assert(!(IsBinaryUpgrade && *invalidated && SlotIsLogical(s)));
>  - Since 'IsBinaryUpgrade' is 'false' most of the time, this approach
> short-circuits evaluation, making it more efficient.
>
> However, if you feel that this condition isn’t as clear to read or
> understand, we can separate the 'IsBinaryUpgrade' check outside the
> 'Assert', as you suggested. Let me know what you think!
>

I discussed the above comments further with Peter off-list, and here
are the v63 patches with the following changes:
 patch-001: The Assert and related comments have been updated for clarity.
 patch-002: Comments have been updated at the top of
InvalidateObsoleteReplicationSlots().

--
Thanks,
Nisha