Re: Introduce XID age and inactive timeout based replication slot invalidation
Peter Smith <smithpb2250@gmail.com>
Commits
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add a test for commit ac0e33136a using the injection point.
- 8a695d7998be 18.0 landed
-
Invalidate inactive replication slots.
- ac0e33136abc 18.0 landed
-
Fix incorrect slot type in BuildTupleHashTableExt
- d96d1d5152f3 18.0 cited
-
Allow synced slots to have their inactive_since.
- 6f132ed693b6 17.0 landed
-
Change last_inactive_time to inactive_since in pg_replication_slots.
- 6d49c8d4b4f4 17.0 landed
-
Track last_inactive_time in pg_replication_slots.
- a11f330b5584 17.0 landed
-
Track invalidation_reason in pg_replication_slots.
- 6ae701b4378d 17.0 landed
-
Add option force_initdb to PostgreSQL::Test::Cluster:init()
- ff9e1e764fcc 17.0 cited
-
Add a failover option to subscriptions.
- 776621a5e479 17.0 cited
-
Allow setting failover property in the replication command.
- 73292404370c 17.0 cited
-
Allow to enable failover property for replication slots via SQL API.
- c393308b69d2 17.0 cited
-
Track conflict_reason in pg_replication_slots.
- 007693f2a3ac 17.0 cited
-
Log messages for replication slot acquisition and release.
- 7c3fb505b14e 17.0 cited
-
Remove vacuum_defer_cleanup_age
- 1118cd37eb61 16.0 cited
-
Fix corruption due to vacuum_defer_cleanup_age underflowing 64bit xids
- be504a3e974d 16.0 cited
-
meson: Add initial version of meson based build system
- e6927270cd18 16.0 cited
Hi Nisha,
Here are some minor review comments for patch v58-0002.
======
src/backend/replication/slot.c
check_replication_slot_inactive_timeout:
1.
+
+/*
+ * GUC check_hook for idle_replication_slot_timeout
+ *
+ * We don't allow the value of idle_replication_slot_timeout other
+ * than 0 during the binary upgrade.
+ * See start_postmaster() in pg_upgrade for more details.
+ */
If you want to express it this way, then it seems there are some
wrong/missing words:
SUGGESTION #1.
We don't allow any value of idle_replication_slot_timeout other than 0
during a binary upgrade.
SUGGESTION #2.
We don't allow the value of idle_replication_slot_timeout to be other
than 0 during a binary upgrade.
~
(But, I prefer more terse comments which are not negative-sounding. YMMV).
SUGGESTION #3 (nearly identical text to the actual error message)
The value of idle_replication_slot_timeout must be set to 0 during a
binary upgrade.
======
src/test/recovery/README
2.
+If you want to test idle_replication_slot_timeout, add
+PG_TEST_EXTRA=idle_replication_slot_timeout
+to the "make" command. The test takes over 2 minutes, so not done
+by default.
+
Maybe it's better to use consistent wording with the other tests like
this one already in the README:
/The test/This test/
/so not done by default./so it's not done by default./
======
.../t/043_invalidate_inactive_slots.pl
3.
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
Happy New Year.
/2024/2025/
~~~
4.
+
+# The test takes over two minutes to complete. Run it only if
+# idle_replication_slot_timeout is specified in PG_TEST_EXTRA.
+if ( !$ENV{PG_TEST_EXTRA}
+ || $ENV{PG_TEST_EXTRA} !~ /\bidle_replication_slot_timeout\b/)
+{
+ plan skip_all =>
+ 'A time consuming test, idle_replication_slot_timeout is not
enabled in PG_TEST_EXTRA';
+}
4a.
I noticed the other skipping TAP tests like this have a simpler
message without giving a reason, so maybe it's better to be consistent
with those:
SUGGESTION:
plan skip_all => "test idle_replication_slot_timeout not enabled in
PG_TEST_EXTRA";
~
4b.
Should the check be done right at the top of the file (e.g. even
before the "# Testcase start" comment)?
======
Kind Regards,
Peter Smith.
Fujitsu Australia