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: 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>
Date: 2024-11-27T03:09: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

Hi Nisha,

Here are some review comments for the patch v50-0002.

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

InvalidatePossiblyObsoleteSlot:

1.
+ if (now &&
+ TimestampDifferenceExceeds(s->inactive_since, now,
+    replication_slot_inactive_timeout_sec * 1000))

Previously this was using an additional call to SlotInactiveTimeoutCheckAllowed:

+ if (SlotInactiveTimeoutCheckAllowed(s) &&
+ TimestampDifferenceExceeds(s->inactive_since, now,
+    replication_slot_inactive_timeout * 1000))

Is it OK to skip that call? e.g. can the slot fields possibly change
between assigning the 'now' and acquiring the mutex? If not, then the
current code is fine. The only reason for asking is because it is
slightly suspicious that it was not done this "easy" way in the first
place.

~~~

check_replication_slot_inactive_timeout:

2.
+/*
+ * GUC check_hook for replication_slot_inactive_timeout
+ *
+ * We don't allow the value of replication_slot_inactive_timeout other than 0
+ * during the binary upgrade.
+ */

The "We don't allow..." sentence seems like a backward way of saying:
The value of replication_slot_inactive_timeout must be set to 0 during
the binary upgrade.

======
src/test/recovery/t/050_invalidate_slots.pl

3.
+# Despite inactive timeout being set, the synced slot won't get invalidated on
+# its own on the standby.

What does "on its own" mean here? Do you mean it won't get invalidated
unless the invalidation state is propagated from the primary? Maybe
the comment can be clearer.

~

4.
+# Wait for slot to first become inactive and then get invalidated
+sub wait_for_slot_invalidation
+{
+ my ($node, $slot, $offset, $inactive_timeout_1s) = @_;
+ my $node_name = $node->name;
+

It was OK to change the variable name to 'inactive_timeout_1s' outside
of here, but within the subroutine, I don't think it is appropriate
because this is a parameter that potentially could have any value.

~

5.
+# Trigger slot invalidation and confirm it in the server log
+sub trigger_slot_invalidation
+{
+ my ($node, $slot, $offset, $inactive_timeout_1s) = @_;
+ my $node_name = $node->name;
+ my $invalidated = 0;

It was OK to change the variable name to 'inactive_timeout_1s' outside
of here, but within the subroutine, I don't think it is appropriate
because this is a parameter that potentially could have any value.

~

6.
+ # Give enough time to avoid multiple checkpoints
+ sleep($inactive_timeout_1s + 1);
+
+ # Run a checkpoint
+ $node->safe_psql('postgres', "CHECKPOINT");

Since you are not doing multiple checkpoints anymore, it looks like
that "Give enough time..." comment needs updating.

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