v24-0001-Use-less-confusing-name-for-slot-s-last_inactive.patch

From 942b8742f108fadf776e3ae1f6fe800878d27b55 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 26 Mar 2024 16:17:55 +0000
Subject: [PATCH v24 1/4] Use less confusing name for slot's last_inactive_time
 property.

The slot's last_inactive_time property added by commit a11f330b55
seems confusing. With last_inactive_time one expects it to tell
the last time that the slot was inactive. But, it tells the last
time that a currently-inactive slot previously *WAS* active.

This commit uses a less confusing and better name for the property
called inactive_since. Othernames considered were released_time,
deactivated_at but inactive_since won the race since the word
inactive is predominant as far as the replication slots are
concerned.

Reported-by: Robert Haas
Author: Bharath Rupireddy
Reviewed-by: Bertrand Drouvot, Amit Kapila
Discussion: https://www.postgresql.org/message-id/ZgGrCBQoktdLi1Ir%40ip-10-97-1-34.eu-west-3.compute.internal
---
 doc/src/sgml/system-views.sgml            |  4 +-
 src/backend/catalog/system_views.sql      |  2 +-
 src/backend/replication/slot.c            | 17 ++++---
 src/backend/replication/slotfuncs.c       |  4 +-
 src/include/catalog/pg_proc.dat           |  2 +-
 src/include/replication/slot.h            |  4 +-
 src/test/recovery/t/019_replslot_limit.pl | 62 +++++++++++------------
 src/test/regress/expected/rules.out       |  4 +-
 8 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 5f4165a945..3c8dca8ca3 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2525,10 +2525,10 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
 
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
-       <structfield>last_inactive_time</structfield> <type>timestamptz</type>
+       <structfield>inactive_since</structfield> <type>timestamptz</type>
       </para>
       <para>
-        The time at which the slot became inactive.
+        The time since the slot has become inactive.
         <literal>NULL</literal> if the slot is currently being used.
       </para></entry>
      </row>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index bc70ff193e..401fb35947 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1023,7 +1023,7 @@ CREATE VIEW pg_replication_slots AS
             L.wal_status,
             L.safe_wal_size,
             L.two_phase,
-            L.last_inactive_time,
+            L.inactive_since,
             L.conflicting,
             L.invalidation_reason,
             L.failover,
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 45f7a28f7d..d778c0b921 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -409,7 +409,7 @@ ReplicationSlotCreate(const char *name, bool db_specific,
 	slot->candidate_restart_valid = InvalidXLogRecPtr;
 	slot->candidate_restart_lsn = InvalidXLogRecPtr;
 	slot->last_saved_confirmed_flush = InvalidXLogRecPtr;
-	slot->last_inactive_time = 0;
+	slot->inactive_since = 0;
 
 	/*
 	 * Create the slot on disk.  We haven't actually marked the slot allocated
@@ -623,9 +623,12 @@ retry:
 	if (SlotIsLogical(s))
 		pgstat_acquire_replslot(s);
 
-	/* Reset the last inactive time as the slot is active now. */
+	/*
+	 * Reset the time since the slot has become inactive as the slot is active
+	 * now.
+	 */
 	SpinLockAcquire(&s->mutex);
-	s->last_inactive_time = 0;
+	s->inactive_since = 0;
 	SpinLockRelease(&s->mutex);
 
 	if (am_walsender)
@@ -703,14 +706,14 @@ ReplicationSlotRelease(void)
 		 */
 		SpinLockAcquire(&slot->mutex);
 		slot->active_pid = 0;
-		slot->last_inactive_time = now;
+		slot->inactive_since = now;
 		SpinLockRelease(&slot->mutex);
 		ConditionVariableBroadcast(&slot->active_cv);
 	}
 	else
 	{
 		SpinLockAcquire(&slot->mutex);
-		slot->last_inactive_time = now;
+		slot->inactive_since = now;
 		SpinLockRelease(&slot->mutex);
 	}
 
@@ -2373,9 +2376,9 @@ RestoreSlotFromDisk(const char *name)
 		 * inactive as decoding is not allowed on those.
 		 */
 		if (!(RecoveryInProgress() && slot->data.synced))
-			slot->last_inactive_time = GetCurrentTimestamp();
+			slot->inactive_since = GetCurrentTimestamp();
 		else
-			slot->last_inactive_time = 0;
+			slot->inactive_since = 0;
 
 		restored = true;
 		break;
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 24f5e6d90a..da57177c25 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -410,8 +410,8 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 
 		values[i++] = BoolGetDatum(slot_contents.data.two_phase);
 
-		if (slot_contents.last_inactive_time > 0)
-			values[i++] = TimestampTzGetDatum(slot_contents.last_inactive_time);
+		if (slot_contents.inactive_since > 0)
+			values[i++] = TimestampTzGetDatum(slot_contents.inactive_since);
 		else
 			nulls[i++] = true;
 
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 0d26e5b422..2f7cfc02c6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -11135,7 +11135,7 @@
   proargtypes => '',
   proallargtypes => '{name,name,text,oid,bool,bool,int4,xid,xid,pg_lsn,pg_lsn,text,int8,bool,timestamptz,bool,text,bool,bool}',
   proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,last_inactive_time,conflicting,invalidation_reason,failover,synced}',
+  proargnames => '{slot_name,plugin,slot_type,datoid,temporary,active,active_pid,xmin,catalog_xmin,restart_lsn,confirmed_flush_lsn,wal_status,safe_wal_size,two_phase,inactive_since,conflicting,invalidation_reason,failover,synced}',
   prosrc => 'pg_get_replication_slots' },
 { oid => '3786', descr => 'set up a logical replication slot',
   proname => 'pg_create_logical_replication_slot', provolatile => 'v',
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index eefd7abd39..7b937d1a0c 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -202,8 +202,8 @@ typedef struct ReplicationSlot
 	 */
 	XLogRecPtr	last_saved_confirmed_flush;
 
-	/* The time at which this slot becomes inactive */
-	TimestampTz last_inactive_time;
+	/* The time since the slot has become inactive */
+	TimestampTz inactive_since;
 } ReplicationSlot;
 
 #define SlotIsPhysical(slot) ((slot)->data.database == InvalidOid)
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index 3409cf88cd..3b9a306a8b 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -411,7 +411,7 @@ $node_primary3->stop;
 $node_standby3->stop;
 
 # =============================================================================
-# Testcase start: Check last_inactive_time property of the streaming standby's slot
+# Testcase start: Check inactive_since property of the streaming standby's slot
 #
 
 # Initialize primary node
@@ -440,45 +440,45 @@ $primary4->safe_psql(
     SELECT pg_create_physical_replication_slot(slot_name := '$sb4_slot');
 ]);
 
-# Get last_inactive_time value after the slot's creation. Note that the slot
-# is still inactive till it's used by the standby below.
-my $last_inactive_time =
-	capture_and_validate_slot_last_inactive_time($primary4, $sb4_slot, $slot_creation_time);
+# Get inactive_since value after the slot's creation. Note that the slot is
+# still inactive till it's used by the standby below.
+my $inactive_since =
+	capture_and_validate_slot_inactive_since($primary4, $sb4_slot, $slot_creation_time);
 
 $standby4->start;
 
 # Wait until standby has replayed enough data
 $primary4->wait_for_catchup($standby4);
 
-# Now the slot is active so last_inactive_time value must be NULL
+# Now the slot is active so inactive_since value must be NULL
 is( $primary4->safe_psql(
 		'postgres',
-		qq[SELECT last_inactive_time IS NULL FROM pg_replication_slots WHERE slot_name = '$sb4_slot';]
+		qq[SELECT inactive_since IS NULL FROM pg_replication_slots WHERE slot_name = '$sb4_slot';]
 	),
 	't',
 	'last inactive time for an active physical slot is NULL');
 
-# Stop the standby to check its last_inactive_time value is updated
+# Stop the standby to check its inactive_since value is updated
 $standby4->stop;
 
-# Let's restart the primary so that the last_inactive_time is set upon
-# loading the slot from the disk.
+# Let's restart the primary so that the inactive_since is set upon loading the
+# slot from the disk.
 $primary4->restart;
 
 is( $primary4->safe_psql(
 		'postgres',
-		qq[SELECT last_inactive_time > '$last_inactive_time'::timestamptz FROM pg_replication_slots WHERE slot_name = '$sb4_slot' AND last_inactive_time IS NOT NULL;]
+		qq[SELECT inactive_since > '$inactive_since'::timestamptz FROM pg_replication_slots WHERE slot_name = '$sb4_slot' AND inactive_since IS NOT NULL;]
 	),
 	't',
 	'last inactive time for an inactive physical slot is updated correctly');
 
 $standby4->stop;
 
-# Testcase end: Check last_inactive_time property of the streaming standby's slot
+# Testcase end: Check inactive_since property of the streaming standby's slot
 # =============================================================================
 
 # =============================================================================
-# Testcase start: Check last_inactive_time property of the logical subscriber's slot
+# Testcase start: Check inactive_since property of the logical subscriber's slot
 my $publisher4 = $primary4;
 
 # Create subscriber node
@@ -499,10 +499,10 @@ $publisher4->safe_psql('postgres',
 	"SELECT pg_create_logical_replication_slot(slot_name := '$lsub4_slot', plugin := 'pgoutput');"
 );
 
-# Get last_inactive_time value after the slot's creation. Note that the slot
-# is still inactive till it's used by the subscriber below.
-$last_inactive_time =
-	capture_and_validate_slot_last_inactive_time($publisher4, $lsub4_slot, $slot_creation_time);
+# Get inactive_since value after the slot's creation. Note that the slot is
+# still inactive till it's used by the subscriber below.
+$inactive_since =
+	capture_and_validate_slot_inactive_since($publisher4, $lsub4_slot, $slot_creation_time);
 
 $subscriber4->start;
 $subscriber4->safe_psql('postgres',
@@ -512,54 +512,54 @@ $subscriber4->safe_psql('postgres',
 # Wait until subscriber has caught up
 $subscriber4->wait_for_subscription_sync($publisher4, 'sub');
 
-# Now the slot is active so last_inactive_time value must be NULL
+# Now the slot is active so inactive_since value must be NULL
 is( $publisher4->safe_psql(
 		'postgres',
-		qq[SELECT last_inactive_time IS NULL FROM pg_replication_slots WHERE slot_name = '$lsub4_slot';]
+		qq[SELECT inactive_since IS NULL FROM pg_replication_slots WHERE slot_name = '$lsub4_slot';]
 	),
 	't',
 	'last inactive time for an active logical slot is NULL');
 
-# Stop the subscriber to check its last_inactive_time value is updated
+# Stop the subscriber to check its inactive_since value is updated
 $subscriber4->stop;
 
-# Let's restart the publisher so that the last_inactive_time is set upon
+# Let's restart the publisher so that the inactive_since is set upon
 # loading the slot from the disk.
 $publisher4->restart;
 
 is( $publisher4->safe_psql(
 		'postgres',
-		qq[SELECT last_inactive_time > '$last_inactive_time'::timestamptz FROM pg_replication_slots WHERE slot_name = '$lsub4_slot' AND last_inactive_time IS NOT NULL;]
+		qq[SELECT inactive_since > '$inactive_since'::timestamptz FROM pg_replication_slots WHERE slot_name = '$lsub4_slot' AND inactive_since IS NOT NULL;]
 	),
 	't',
 	'last inactive time for an inactive logical slot is updated correctly');
 
-# Testcase end: Check last_inactive_time property of the logical subscriber's slot
+# Testcase end: Check inactive_since property of the logical subscriber's slot
 # =============================================================================
 
 $publisher4->stop;
 $subscriber4->stop;
 
-# Capture and validate last_inactive_time of a given slot.
-sub capture_and_validate_slot_last_inactive_time
+# Capture and validate inactive_since of a given slot.
+sub capture_and_validate_slot_inactive_since
 {
 	my ($node, $slot_name, $slot_creation_time) = @_;
 
-	my $last_inactive_time = $node->safe_psql('postgres',
-		qq(SELECT last_inactive_time FROM pg_replication_slots
-			WHERE slot_name = '$slot_name' AND last_inactive_time IS NOT NULL;)
+	my $inactive_since = $node->safe_psql('postgres',
+		qq(SELECT inactive_since FROM pg_replication_slots
+			WHERE slot_name = '$slot_name' AND inactive_since IS NOT NULL;)
 		);
 
 	# Check that the captured time is sane
 	is( $node->safe_psql(
 			'postgres',
-			qq[SELECT '$last_inactive_time'::timestamptz > to_timestamp(0) AND
-				'$last_inactive_time'::timestamptz >= '$slot_creation_time'::timestamptz;]
+			qq[SELECT '$inactive_since'::timestamptz > to_timestamp(0) AND
+				'$inactive_since'::timestamptz >= '$slot_creation_time'::timestamptz;]
 		),
 		't',
 		"last inactive time for an active slot $slot_name is sane");
 
-	return $last_inactive_time;
+	return $inactive_since;
 }
 
 done_testing();
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index dfcbaec387..f53c3036a6 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1473,12 +1473,12 @@ pg_replication_slots| SELECT l.slot_name,
     l.wal_status,
     l.safe_wal_size,
     l.two_phase,
-    l.last_inactive_time,
+    l.inactive_since,
     l.conflicting,
     l.invalidation_reason,
     l.failover,
     l.synced
-   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, last_inactive_time, conflicting, invalidation_reason, failover, synced)
+   FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, temporary, active, active_pid, xmin, catalog_xmin, restart_lsn, confirmed_flush_lsn, wal_status, safe_wal_size, two_phase, inactive_since, conflicting, invalidation_reason, failover, synced)
      LEFT JOIN pg_database d ON ((l.datoid = d.oid)));
 pg_roles| SELECT pg_authid.rolname,
     pg_authid.rolsuper,
-- 
2.34.1