public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Kapila <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
Date: Mon, 25 Mar 2024 10:28:31 +0530
Message-ID: <CAA4eK1LG+2S7SdJdfbwbPOyKdMU41oZPEaS5OQoo7navT0ER_g@mail.gmail.com> (raw)
In-Reply-To: <CAA4eK1KvNxhHOf8qCnYw9fJ954UPFVGYrDW3rYYnOQFPVxR=Fw@mail.gmail.com>
References: <CAA4eK1KZyjdn7oXS3d7qJU5ag9ss4xSk0Jkjsh5NOrRdkPkV1g@mail.gmail.com>
<[email protected]>
<CALj2ACUvK7ShPyirAURWf62=qOQqu=NwnyL5CMfVjM5Ody7Oxw@mail.gmail.com>
<[email protected]>
<CALj2ACWk5a-LfBPziAjjFA--syoUSD_4jY6dZyhU8g8XuD-i7g@mail.gmail.com>
<CAA4eK1+46kJF3Z2Tv=xdkyy0nuurrS7RfzqvqY8bq1MF-JLM=A@mail.gmail.com>
<CALj2ACVKncd+JZR8BeXSgPO4akqic9kO99Z6pCfeXQKwmye2Og@mail.gmail.com>
<CAA4eK1JwJdsKMxczGO9tYgEzxp_L2=j0QTeDDk2Tx4-ipGSppw@mail.gmail.com>
<[email protected]>
<CALj2ACWuNHJ3qBMmmy6i1YA9V8JDRSaciL2bBM+72bRcqiBE2g@mail.gmail.com>
<[email protected]>
<CALj2ACVUCo48FXWAcxC9LEV6P=xgd-8O=e9aPmTRUZvvH_L7Zw@mail.gmail.com>
<CAA4eK1LjNB8jBgqLHtf+FKnd=SfMz3TXXs871p7KjG4VHnxafg@mail.gmail.com>
<CALj2ACXQmGx-L073=8CUSOk3hhe=HL_e6dveYeR9WJcEh+7jvg@mail.gmail.com>
<CAA4eK1L_UP=ZjCj=+cGEauYXHC1icUEfx16p+B7qrAHZuCx5Ew@mail.gmail.com>
<CALj2ACU6tV0bh_MiFBYU7-VK2cj8Gas73htcQjoOv0uvSEG_HA@mail.gmail.com>
<CAA4eK1KvNxhHOf8qCnYw9fJ954UPFVGYrDW3rYYnOQFPVxR=Fw@mail.gmail.com>
On Mon, Mar 25, 2024 at 9:48 AM Amit Kapila <[email protected]> wrote:
>
>
> Such a test looks reasonable but shall we add equal to in the second
> part of the test (like '$last_inactive_time'::timestamptz >=
> > '$slot_creation_time'::timestamptz;). This is just to be sure that even if the test ran fast enough to give the same time, the test shouldn't fail. I think it won't matter for correctness as well.
>
Apart from this, I have made minor changes in the comments. See and
let me know what you think of attached.
--
With Regards,
Amit Kapila.
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 2b36b5fef1..5f4165a945 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2529,8 +2529,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</para>
<para>
The time at which the slot became inactive.
- <literal>NULL</literal> if the slot is currently actively being
- used.
+ <literal>NULL</literal> if the slot is currently being used.
</para></entry>
</row>
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 0f48d6dc7c..77cb633812 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -623,7 +623,7 @@ retry:
if (SlotIsLogical(s))
pgstat_acquire_replslot(s);
- /* The slot is active by now, so reset the last inactive time. */
+ /* Reset the last inactive time as the slot is active now. */
SpinLockAcquire(&s->mutex);
s->last_inactive_time = 0;
SpinLockRelease(&s->mutex);
@@ -687,8 +687,8 @@ ReplicationSlotRelease(void)
}
/*
- * Set the last inactive time after marking slot inactive. We get current
- * time beforehand to avoid system call while holding the lock.
+ * Set the last inactive time after marking the slot inactive. We get the
+ * current time beforehand to avoid a system call while holding the lock.
*/
now = GetCurrentTimestamp();
@@ -2363,9 +2363,9 @@ RestoreSlotFromDisk(const char *name)
slot->active_pid = 0;
/*
- * We set last inactive time after loading the slot from the disk into
- * memory. Whoever acquires the slot i.e. makes the slot active will
- * anyway reset it.
+ * We set the last inactive time after loading the slot from the disk
+ * into memory. Whoever acquires the slot i.e. makes the slot active
+ * will reset it.
*/
slot->last_inactive_time = GetCurrentTimestamp();
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index 2f18433ecc..eefd7abd39 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -202,7 +202,7 @@ typedef struct ReplicationSlot
*/
XLogRecPtr last_saved_confirmed_flush;
- /* The time at which this slot become inactive */
+ /* The time at which this slot becomes inactive */
TimestampTz last_inactive_time;
} ReplicationSlot;
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index bff84cc9c4..81bd36f5d8 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 streaming standby's slot
+# Testcase start: Check last_inactive_time property of the streaming standby's slot
#
# Initialize primary node
@@ -440,8 +440,8 @@ $primary4->safe_psql(
SELECT pg_create_physical_replication_slot(slot_name := '$sb4_slot');
]);
-# Get last_inactive_time value after slot's creation. Note that the slot is still
-# inactive unless it's used by the standby below.
+# 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 = $primary4->safe_psql('postgres',
qq(SELECT last_inactive_time FROM pg_replication_slots WHERE slot_name = '$sb4_slot' AND last_inactive_time IS NOT NULL;)
);
@@ -470,8 +470,8 @@ is( $primary4->safe_psql(
# Stop the standby to check its last_inactive_time value is updated
$standby4->stop;
-# Let's also restart the primary so that the last_inactive_time is set upon
-# loading the slot from disk.
+# Let's restart the primary so that the last_inactive_time is set upon
+# loading the slot from the disk.
$primary4->restart;
is( $primary4->safe_psql(
@@ -483,11 +483,11 @@ is( $primary4->safe_psql(
$standby4->stop;
-# Testcase end: Check last_inactive_time property of streaming standby's slot
+# Testcase end: Check last_inactive_time property of the streaming standby's slot
# =============================================================================
# =============================================================================
-# Testcase start: Check last_inactive_time property of logical subscriber's slot
+# Testcase start: Check last_inactive_time property of the logical subscriber's slot
my $publisher4 = $primary4;
# Create subscriber node
@@ -508,8 +508,8 @@ $publisher4->safe_psql('postgres',
"SELECT pg_create_logical_replication_slot(slot_name := '$lsub4_slot', plugin := 'pgoutput');"
);
-# Get last_inactive_time value after slot's creation. Note that the slot is still
-# inactive unless it's used by the subscriber below.
+# 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 = $publisher4->safe_psql('postgres',
qq(SELECT last_inactive_time FROM pg_replication_slots WHERE slot_name = '$lsub4_slot' AND last_inactive_time IS NOT NULL;)
);
@@ -541,8 +541,8 @@ is( $publisher4->safe_psql(
# Stop the subscriber to check its last_inactive_time value is updated
$subscriber4->stop;
-# Let's also restart the publisher so that the last_inactive_time is set upon
-# loading the slot from disk.
+# Let's restart the publisher so that the last_inactive_time is set upon
+# loading the slot from the disk.
$publisher4->restart;
is( $publisher4->safe_psql(
@@ -552,7 +552,7 @@ is( $publisher4->safe_psql(
't',
'last inactive time for an inactive logical slot is updated correctly');
-# Testcase end: Check last_inactive_time property of logical subscriber's slot
+# Testcase end: Check last_inactive_time property of the logical subscriber's slot
# =============================================================================
$publisher4->stop;
Attachments:
[text/plain] v18_0001_diff_amit.patch.txt (5.8K, ../CAA4eK1LG+2S7SdJdfbwbPOyKdMU41oZPEaS5OQoo7navT0ER_g@mail.gmail.com/2-v18_0001_diff_amit.patch.txt)
download | inline diff:
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 2b36b5fef1..5f4165a945 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -2529,8 +2529,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</para>
<para>
The time at which the slot became inactive.
- <literal>NULL</literal> if the slot is currently actively being
- used.
+ <literal>NULL</literal> if the slot is currently being used.
</para></entry>
</row>
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 0f48d6dc7c..77cb633812 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -623,7 +623,7 @@ retry:
if (SlotIsLogical(s))
pgstat_acquire_replslot(s);
- /* The slot is active by now, so reset the last inactive time. */
+ /* Reset the last inactive time as the slot is active now. */
SpinLockAcquire(&s->mutex);
s->last_inactive_time = 0;
SpinLockRelease(&s->mutex);
@@ -687,8 +687,8 @@ ReplicationSlotRelease(void)
}
/*
- * Set the last inactive time after marking slot inactive. We get current
- * time beforehand to avoid system call while holding the lock.
+ * Set the last inactive time after marking the slot inactive. We get the
+ * current time beforehand to avoid a system call while holding the lock.
*/
now = GetCurrentTimestamp();
@@ -2363,9 +2363,9 @@ RestoreSlotFromDisk(const char *name)
slot->active_pid = 0;
/*
- * We set last inactive time after loading the slot from the disk into
- * memory. Whoever acquires the slot i.e. makes the slot active will
- * anyway reset it.
+ * We set the last inactive time after loading the slot from the disk
+ * into memory. Whoever acquires the slot i.e. makes the slot active
+ * will reset it.
*/
slot->last_inactive_time = GetCurrentTimestamp();
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index 2f18433ecc..eefd7abd39 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -202,7 +202,7 @@ typedef struct ReplicationSlot
*/
XLogRecPtr last_saved_confirmed_flush;
- /* The time at which this slot become inactive */
+ /* The time at which this slot becomes inactive */
TimestampTz last_inactive_time;
} ReplicationSlot;
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index bff84cc9c4..81bd36f5d8 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 streaming standby's slot
+# Testcase start: Check last_inactive_time property of the streaming standby's slot
#
# Initialize primary node
@@ -440,8 +440,8 @@ $primary4->safe_psql(
SELECT pg_create_physical_replication_slot(slot_name := '$sb4_slot');
]);
-# Get last_inactive_time value after slot's creation. Note that the slot is still
-# inactive unless it's used by the standby below.
+# 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 = $primary4->safe_psql('postgres',
qq(SELECT last_inactive_time FROM pg_replication_slots WHERE slot_name = '$sb4_slot' AND last_inactive_time IS NOT NULL;)
);
@@ -470,8 +470,8 @@ is( $primary4->safe_psql(
# Stop the standby to check its last_inactive_time value is updated
$standby4->stop;
-# Let's also restart the primary so that the last_inactive_time is set upon
-# loading the slot from disk.
+# Let's restart the primary so that the last_inactive_time is set upon
+# loading the slot from the disk.
$primary4->restart;
is( $primary4->safe_psql(
@@ -483,11 +483,11 @@ is( $primary4->safe_psql(
$standby4->stop;
-# Testcase end: Check last_inactive_time property of streaming standby's slot
+# Testcase end: Check last_inactive_time property of the streaming standby's slot
# =============================================================================
# =============================================================================
-# Testcase start: Check last_inactive_time property of logical subscriber's slot
+# Testcase start: Check last_inactive_time property of the logical subscriber's slot
my $publisher4 = $primary4;
# Create subscriber node
@@ -508,8 +508,8 @@ $publisher4->safe_psql('postgres',
"SELECT pg_create_logical_replication_slot(slot_name := '$lsub4_slot', plugin := 'pgoutput');"
);
-# Get last_inactive_time value after slot's creation. Note that the slot is still
-# inactive unless it's used by the subscriber below.
+# 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 = $publisher4->safe_psql('postgres',
qq(SELECT last_inactive_time FROM pg_replication_slots WHERE slot_name = '$lsub4_slot' AND last_inactive_time IS NOT NULL;)
);
@@ -541,8 +541,8 @@ is( $publisher4->safe_psql(
# Stop the subscriber to check its last_inactive_time value is updated
$subscriber4->stop;
-# Let's also restart the publisher so that the last_inactive_time is set upon
-# loading the slot from disk.
+# Let's restart the publisher so that the last_inactive_time is set upon
+# loading the slot from the disk.
$publisher4->restart;
is( $publisher4->safe_psql(
@@ -552,7 +552,7 @@ is( $publisher4->safe_psql(
't',
'last inactive time for an inactive logical slot is updated correctly');
-# Testcase end: Check last_inactive_time property of logical subscriber's slot
+# Testcase end: Check last_inactive_time property of the logical subscriber's slot
# =============================================================================
$publisher4->stop;
view thread (43+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
In-Reply-To: <CAA4eK1LG+2S7SdJdfbwbPOyKdMU41oZPEaS5OQoo7navT0ER_g@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox