On Tue, Nov 14, 2023 at 4:50 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some review comments for patch v13-0001.
Thanks. > ====== > doc/src/sgml/config.sgml > > 1. > > Should that also mention about walsender? > > e.g. > "and slot acquisition/release" ==> "and <literal>walsender</literal> > slot acquisition/release" Changed. > 2a. > Instead of calling SlotIsLogical() and then again calling > SlotIsPhysical(), it might be better to assign this one time to a > local variable. > > 2b. > IMO it is better to continue using variable 's' here instead of > 'MyReplicationSlot'. Code is not only shorter but is also consistent > with the rest of the function which never uses MyReplicationSlot, even > in the places where it could have. > > SUGGESTION (for #2a and #2b) > is_logical = SlotIsLogical(s); > if (is_logical) > pgstat_acquire_replslot(s); > > if (am_walsender) > ereport(log_replication_commands ? LOG : DEBUG1, > errmsg_internal("acquired %s replication slot \"%s\"", > is_logical ? "logical" : "physical", NameStr(s->data.name))); Use of a separate variable isn't good IMO, I used SlotIsLogical(s); directly. > 3a. > Notice 'MyReplicationSlot' is already assigned to the local 'slot' > variable, so IMO it is better if this new code also uses that 'slot' > variable for consistency with the rest of the function. > > 3b. > Consider flipping the flag to be 'is_logical' instead of > 'is_physical', so the ereport substitution will match the other > ReplicationSlotAcquirecode suggested above (#2a). > > SUGGESTION (For #3a and #3b) > if (am_walsender) > { > slotname = pstrdup(NameStr(slot->data.name)); > is_logical = SlotIsLogical(slot); > } Done. PSA v14 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From 87d04d040fc048af2065f34c6c5887c84d4e15da Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Tue, 14 Nov 2023 06:53:02 +0000 Subject: [PATCH v14] Log messages for replication slot acquisition and release This commit adds log messages (at LOG level when log_replication_commands is set, otherwise at DEBUG1 level) when walsenders acquire and release replication slots. These messages help to know the lifetime of a replication slot - one can know how long a streaming standby or logical subscriber or replication slot consumer is down. In other words, one can know how long a replication slot is inactive - the time between released and acquired slot messages is the inactive replication slot duration. These messages will be extremely useful on production servers to debug and analyze inactive replication slot issues. Note that these messages are emitted only for walsenders when replication slot is acquired and released, but not for backends. This is because walsenders are the ones that typically hold replication slots for longer durations unlike backends which hold replication slots for executing some replication related functions. --- doc/src/sgml/config.sgml | 7 ++++--- src/backend/replication/slot.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index fc35a46e5e..d87320141a 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7510,9 +7510,10 @@ log_line_prefix = '%m [%p] %q%u@%d/%a ' </term> <listitem> <para> - Causes each replication command to be logged in the server log. - See <xref linkend="protocol-replication"/> for more information about - replication command. The default value is <literal>off</literal>. + Causes each replication command and <literal>walsender</literal> + process replication slot acquisition/release to be logged in the server + log. See <xref linkend="protocol-replication"/> for more information + about replication command. The default value is <literal>off</literal>. Only superusers and users with the appropriate <literal>SET</literal> privilege can change this setting. </para> diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 781aa43cc4..5d6ba0aef1 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -537,6 +537,12 @@ retry: */ if (SlotIsLogical(s)) pgstat_acquire_replslot(s); + + if (am_walsender) + ereport(log_replication_commands ? LOG : DEBUG1, + errmsg_internal("acquired %s replication slot \"%s\"", + SlotIsLogical(s) ? "logical" : "physical", + NameStr(s->data.name))); } /* @@ -549,9 +555,17 @@ void ReplicationSlotRelease(void) { ReplicationSlot *slot = MyReplicationSlot; + char *slotname = NULL; /* keep compiler quiet */ + bool is_logical = false; /* keep compiler quiet */ Assert(slot != NULL && slot->active_pid != 0); + if (am_walsender) + { + slotname = pstrdup(NameStr(slot->data.name)); + is_logical = SlotIsLogical(slot); + } + if (slot->data.persistency == RS_EPHEMERAL) { /* @@ -596,6 +610,16 @@ ReplicationSlotRelease(void) MyProc->statusFlags &= ~PROC_IN_LOGICAL_DECODING; ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; LWLockRelease(ProcArrayLock); + + if (am_walsender) + { + ereport(log_replication_commands ? LOG : DEBUG1, + errmsg_internal("released %s replication slot \"%s\"", + is_logical ? "logical" : "physical", + slotname)); + + pfree(slotname); + } } /* -- 2.34.1