From 942b8742f108fadf776e3ae1f6fe800878d27b55 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy 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 - last_inactive_time timestamptz + inactive_since timestamptz - The time at which the slot became inactive. + The time since the slot has become inactive. NULL if the slot is currently being used. 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