Horton

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

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: Nisha Moond <nisha.moond412@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>, Shlok Kyal <shlok.kyal.oss@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-02-10T06:03:02Z
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

Hi Nisha,

Some review comments for v72-0001.

======
GENERAL

My preference was to just keep the enum as per v70 for the *actual*
cause, and introduce a separate set of bit flags for *possible* causes
to be checked. This creates a clear code separation between the actual
and possible. It also eliminates the need to jump through hoops just
to map a cause to its name.

You wrote:

> OTOH, keeping the enums as they are in v70, and defining new macros
for the very similar purpose could add unnecessary complexity to code
management.

Since both the enum and the bit flags would be defined in slot.h
adjacent to each other I don't foresee much complexity. I concede, a
dev might write code and accidentally muddle the enum instead of the
flag  or vice versa but that's an example of sloppiness, not
complexity. Certainly there would be fewer necessary changes than what
are in the v72 patch due to all the cause/causename mappings. For
example,

slot.h - Now, introduces a NEW typedef SlotInvalidationCauseMap
slot.h - Now, need extern for NEW function GetSlotInvalidationCauseName

slot.c - Now, needed minor rewrite of GetSlotInvalidationCause instead
of leaving it as-is
slot.c - Now, needs a whole NEW looping function
GetSlotInvalidationCauseName instead of direct array index.

Several place now must call to the GetSlotInvalidationCauseName where
previously a simple direct array lookup was done
slot.c - NEW call in ReplicationSlotAcquire
slotfuncs.c - NEW call in pg_get_replication_slots

~

FWIW, I've attached a topup patch using my idea just to see what it
might look like. The result was 20 lines less code.

Anyway, YMMV.

======

Other review comments follow ...

src/backend/replication/slot.c

InvalidatePossiblyObsoleteSlot:

1.
Having all those 'gotos' seems like something best avoided. Can you
try removing them to see if it improves this function? IIUC you maybe
can try rid all of them using  logic like:

- assign invalidation_cause = NONE outside the loop
- loop until invalidation_cause != NONE
- include 'invalidation_cause == NONE' condition with all the bit flag checks
- reassign invalidation_cause = NONE in the racy place where you want
to continue the loop.

and instead just keep looping and checking while the
'invalidation_cause' remains NONE.

======
Kind Regards,
Peter Smith.
Fujitsu Australia