At Wed, 28 Sep 2022 16:30:37 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota....@gmail.com> writes: > > Okay. the points you brought up above are sufficient grounds for not > > doing so. Now they are in the following format. > > > LOG: terminating process 16034 to release replication slot "rep1" > > because its restart_lsn 0/3158000 exceeds the limit by 15368192 bytes > > This seems to me to be a pretty blatant violation of our first message > style guideline [1]:
Thanks! It seems that I was waiting for a comment on that line. I thought that way at first but finally returned to the current message as the result of discussion (in my memory). I will happily make the main message shorter. > I think you should leave the primary message alone and add a DETAIL, > as the first version of the patch did. > > The existing "invalidating slot" message is already in violation > of this guideline, so splitting off a DETAIL from that seems > indicated as well. So I'm going to change the mssage as: LOG: terminating process %d to release replication slot \"%s\" DETAIL: The slot's restart_lsn %X/%X exceeds the limit by %lld bytes. HINT: You might need to increase max_slot_wal_keep_size. LOG: invalidating *replication* slot \"%s\" DETAILS: (ditto) HINTS: (ditto) It seems that it's no longer useful to split out the first patch so I merged them into one. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
>From 7fe33ec1e2e706d7b1d95edfcfe0c0a418408712 Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <horikyota....@gmail.com> Date: Fri, 24 Dec 2021 12:52:07 +0900 Subject: [PATCH v8] Make a message on process termination more dscriptive Add the details to the process termination message so that the error details are shown in case of process terminations due to temporary slots. Addition to that the messages are changed to tell how much the current LSN exceeded the limit instead of just saying "exceeding the limit". In passing, shorten the main messages then move their details to DETAILS line so that the messages follow our policy. Reported-by: Alex Enachioaie <a...@altmetric.com> Author: Kyotaro Horiguchi <horikyota....@gmail.com> Reviewed-by: Ashutosh Bapat <ashutosh.bapat....@gmail.com> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Reviewed-by: Masahiko Sawada <sawada.m...@gmail.com> Reviewed-by: "Drouvot, Bertrand" <bdrou...@amazon.com> Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://www.postgresql.org/message-id/17327-89d0efa8b9ae6271%40postgresql.org --- src/backend/replication/slot.c | 17 ++++++++++++----- src/test/recovery/t/019_replslot_limit.pl | 11 ++++------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 0bd0031188..e622e6ddda 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1293,8 +1293,12 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, XLogRecPtr oldestLSN, if (last_signaled_pid != active_pid) { ereport(LOG, - (errmsg("terminating process %d to release replication slot \"%s\"", - active_pid, NameStr(slotname)))); + errmsg("terminating process %d to release replication slot \"%s\"", + active_pid, NameStr(slotname)), + errdetail("The slot's restart_lsn %X/%X exceeds the limit by %llu bytes.", + LSN_FORMAT_ARGS(restart_lsn), + (unsigned long long)(oldestLSN - restart_lsn)), + errhint("You might need to increase max_slot_wal_keep_size.")); (void) kill(active_pid, SIGTERM); last_signaled_pid = active_pid; @@ -1331,9 +1335,12 @@ InvalidatePossiblyObsoleteSlot(ReplicationSlot *s, XLogRecPtr oldestLSN, ReplicationSlotRelease(); ereport(LOG, - (errmsg("invalidating slot \"%s\" because its restart_lsn %X/%X exceeds max_slot_wal_keep_size", - NameStr(slotname), - LSN_FORMAT_ARGS(restart_lsn)))); + errmsg("invalidating replication slot \"%s\"", + NameStr(slotname)), + errdetail("The slot's restart_lsn %X/%X exceeds the limit by %llu bytes.", + LSN_FORMAT_ARGS(restart_lsn), + (unsigned long long)(oldestLSN - restart_lsn)), + errhint("You might need to increase max_slot_wal_keep_size.")); /* done with this slot for now */ break; diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl index ce8d36b4c6..b917f74bb6 100644 --- a/src/test/recovery/t/019_replslot_limit.pl +++ b/src/test/recovery/t/019_replslot_limit.pl @@ -184,10 +184,8 @@ $node_primary->safe_psql('postgres', "CHECKPOINT;"); my $invalidated = 0; for (my $i = 0; $i < 10000; $i++) { - if (find_in_log( - $node_primary, - "invalidating slot \"rep1\" because its restart_lsn [0-9A-F/]+ exceeds max_slot_wal_keep_size", - $logstart)) + if (find_in_log($node_primary, + 'invalidating replication slot "rep1"', $logstart)) { $invalidated = 1; last; @@ -405,9 +403,8 @@ $node_primary3->poll_query_until('postgres', $max_attempts = $PostgreSQL::Test::Utils::timeout_default; while ($max_attempts-- >= 0) { - if (find_in_log( - $node_primary3, - 'invalidating slot "rep3" because its restart_lsn', $logstart)) + if (find_in_log($node_primary3, + 'invalidating replication slot "rep3"', $logstart)) { ok(1, "slot invalidation logged"); last; -- 2.31.1
