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 review comments for patch v51-0002.
======
doc/src/sgml/system-views.sgml
1.
The time when the slot became inactive. <literal>NULL</literal> if the
- slot is currently being streamed.
+ slot is currently being streamed. Once the slot is invalidated, this
+ value will remain unchanged until we shutdown the server.
.
I think "Once the ..." kind of makes it sound like invalidation is
inevitable. Also maybe it's better to remove the "we".
SUGGESTION:
If the slot becomes invalidated, this value will remain unchanged
until server shutdown.
======
src/backend/replication/slot.c
ReplicationSlotAcquire:
2.
GENERAL.
This just is a question/idea. It may not be feasible to change. It
seems like there is a lot of overlap between the error messages in
'ReplicationSlotAcquire' which are saying "This slot has been
invalidated because...", and with the other function
'ReportSlotInvalidation' which is kind of the same but called in
different circumstances and with slightly different message text. I
wondered if there is a way to use common code to unify these messages
instead of having a nearly duplicate set of messages for all the
invalidation causes?
~~~
3.
+ case RS_INVAL_INACTIVE_TIMEOUT:
+ appendStringInfo(&err_detail, _("inactivity exceeded the time limit
set by \"%s\"."),
+ "replication_slot_inactive_timeout");
+ break;
Should this err_detail also say "This slot has been invalidated
because ..." like all the others?
~~~
InvalidatePossiblyObsoleteSlot:
4.
+ case RS_INVAL_INACTIVE_TIMEOUT:
+
+ /*
+ * Check if the slot needs to be invalidated due to
+ * replication_slot_inactive_timeout GUC.
+ */
+ if (IsSlotInactiveTimeoutPossible(s) &&
+ TimestampDifferenceExceeds(s->inactive_since, now,
+ replication_slot_inactive_timeout_sec * 1000))
+ {
Maybe this code should have Assert(now > 0); before the condition just
as a way to 'document' that it is assumed 'now' was already set this
outside the mutex.
======
Kind Regards,
Peter Smith.
Fujitsu Australia