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

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>
To: Nisha Moond <nisha.moond412@gmail.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@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: 2024-12-05T00:35:20Z
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

Hi Nisha,

Here are my review comments for the v53* patch set

//////////

Patch v53-0001.

======
src/backend/replication/slot.c

1.
+ if (error_if_invalid &&
+ s->data.invalidated != RS_INVAL_NONE)

Looks like some unnecessary wrapping here. I think this condition can
be on one line.

//////////

Patch v53-0002.

======
GENERAL - How about using the term "idle"?

1.
I got to wondering why this new GUC was called
"replication_slot_inactive_timeout", with invalidation_reason =
"inactive_timeout". When I look at similar GUCs I don't see words like
"inactivity" or "inactive" anywhere; Instead, they are using the term
"idle" to refer to when something is inactive:
e.g.
#idle_in_transaction_session_timeout = 0 # in milliseconds, 0 is disabled
#idle_session_timeout = 0 # in milliseconds, 0 is disabled

I know the "inactive" term is used a bit in the slot code but that is
(mostly) not exposed to the user. Therefore, I am beginning to feel it
would be better (e.g. more consistent) to use "idle" for the
user-facing stuff. e.g.
New Slot GUC = "idle_replication_slot_timeout"
Slot invalidation_reason = "idle_timeout"

Of course, changing this will cascade to impact quite a lot of other
things in the patch -- comments, error messages, some function names
etc.

======
doc/src/sgml/logical-replication.sgml

2.
+   <para>
+    Logical replication slot is also affected by
+    <link linkend="guc-replication-slot-inactive-timeout"><varname>replication_slot_inactive_timeout</varname></link>.
+   </para>
+

/Logical replication slot is also affected by/Logical replication
slots are also affected by/

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