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

On Fri, 31 Jan 2025 at 17:50, Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Thanks for the review! I have incorporated the above comments. The
> test in patch-002 has been optimized as suggested and now completes in
> less than a second.
> Please find the attached v66 patch set. The base patch(v65-001) is
> committed now, so I have rebased the patches.

Few comments:
1)We should set inactive_since only if the slot can be invalidated:
+                                       /* For testing timeout slot
invalidation */
+                                       if
(IS_INJECTION_POINT_ATTACHED("slot-time-out-inval"))
+                                               s->inactive_since = 1;
+

2) Instead of "alter system set" and reload, let's do this in
$node->append_conf itself:
+# Set timeout GUC so that the next checkpoint will invalidate inactive slots
+$node->safe_psql(
+       'postgres', qq[
+    ALTER SYSTEM SET idle_replication_slot_timeout TO '1min';
+]);
+$node->reload;

3) No need to trigger checkpoint twice, we can move it outside so that
just a single checkpoint will invalidate both the slots:
+sub trigger_slot_invalidation
+{
+       my ($node, $slot, $offset) = @_;
+       my $node_name = $node->name;
+       my $invalidated = 0;
+
+       # Run a checkpoint
+       $node->safe_psql('postgres', "CHECKPOINT");

4) I fel this trigger_slot_invalidation is not required after removing
the checkpoint from the function, let's move the waiting for
"invalidating obsolete replication slot" also to
wait_for_slot_invalidation function:
+       # The slot's invalidation should be logged
+       $node->wait_for_log(qr/invalidating obsolete replication slot
\"$slot\"/,
+               $offset);
+
+       # Check that the invalidation reason is 'idle_timeout'
+       $node->poll_query_until(
+               'postgres', qq[
+               SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+                       WHERE slot_name = '$slot' AND
+                       invalidation_reason = 'idle_timeout';
+       ])

5) Can we move the subroutine to the beginning, I noticed in other
places we have kept it before the tests like in 027_nosuperuser and
040_createsubscriber:
+# Wait for slot to first become idle and then get invalidated
+sub wait_for_slot_invalidation
+{
+       my ($node, $slot, $offset) = @_;
+       my $node_name = $node->name;
+
+       trigger_slot_invalidation($node, $slot, $offset);
+
+       # Check that an invalidated slot cannot be acquired
+       my ($result, $stdout, $stderr);
+       ($result, $stdout, $stderr) = $node->psql(
+               'postgres', qq[
+                       SELECT pg_replication_slot_advance('$slot', '0/1');
+       ]);

6) Since idle_replication_slot_timeout is related more closely with
max_slot_wal_keep_size, let's keep it along with it.
diff --git a/src/backend/utils/misc/postgresql.conf.sample
b/src/backend/utils/misc/postgresql.conf.sample
index 079efa1baa..0ed9eb057e 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -329,6 +329,7 @@
 #wal_sender_timeout = 60s      # in milliseconds; 0 disables
 #track_commit_timestamp = off  # collect timestamp of transaction commit
                                # (change requires restart)
+#idle_replication_slot_timeout = 1d    # in minutes; 0 disables

If you accept the comments, you can merge the changes from the attached patch.

Regards,
Vignesh