duplicate logging in pg_createsubscriber
Hi hackers,
While reviewing pg_createsubscriber logs (in another thread) I saw
some unexpected (almost) duplicate consecutive logs for the slot
creation in --dry-run mode.
e.g.
----------
pg_createsubscriber: creating the replication slot
"pg_createsubscriber_16386_e9a70df3" in database "db2"
pg_createsubscriber: create replication slot
"pg_createsubscriber_16386_e9a70df3" on publisher
----------
It didn't look right to me.
I found the code is doing:
- pg_log_info("creating the replication slot unconditional inside
create_logical_replication_slot
- pg_log_info("create replication slot...") immediately after call to
create_logical_replication_slot
Perhaps that 2nd log was once supposed to say "created" (past tense),
but even that seemed redundant.
Here is a small patch to remove the log duplication by keeping only
the log *within* the function.
Thoughts?
======
Kind Regards,
Peter Smith.
Fujitsu Asutralia
Attachments:
v1-0001-remove-duplicate-logging-for-slot-creation.patchapplication/octet-stream; name=v1-0001-remove-duplicate-logging-for-slot-creation.patchDownload
From 1f5f026dd81d4d989bb0cd28c5d3449dedeb62ab Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Wed, 8 Oct 2025 16:59:51 +1100
Subject: [PATCH v1] remove duplicate logging for slot creation
---
src/bin/pg_basebackup/pg_createsubscriber.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 3986882..d294074 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -801,10 +801,7 @@ setup_publisher(struct LogicalRepInfo *dbinfo)
if (lsn)
pg_free(lsn);
lsn = create_logical_replication_slot(conn, &dbinfo[i]);
- if (lsn != NULL || dry_run)
- pg_log_info("create replication slot \"%s\" on publisher",
- dbinfo[i].replslotname);
- else
+ if (lsn == NULL && !dry_run)
exit(1);
/*
@@ -1375,7 +1372,7 @@ create_logical_replication_slot(PGconn *conn, struct LogicalRepInfo *dbinfo)
Assert(conn != NULL);
- pg_log_info("creating the replication slot \"%s\" in database \"%s\"",
+ pg_log_info("creating the replication slot \"%s\" in database \"%s\" on publisher",
slot_name, dbinfo->dbname);
slot_name_esc = PQescapeLiteral(conn, slot_name, strlen(slot_name));
--
1.8.3.1
On Wed, Oct 08, 2025 at 05:29:20PM +1100, Peter Smith wrote:
It didn't look right to me.
I found the code is doing:
- pg_log_info("creating the replication slot unconditional inside
create_logical_replication_slot
- pg_log_info("create replication slot...") immediately after call to
create_logical_replication_slotPerhaps that 2nd log was once supposed to say "created" (past tense),
but even that seemed redundant.Here is a small patch to remove the log duplication by keeping only
the log *within* the function.Thoughts?
Agreed. This was bugging me a bit today while looking at some logs
generated by the TAP tests. We don't gain any additional information
here, so let's remove the duplicates. I'll check that later, it's
late now.
--
Michael
On Oct 8, 2025, at 14:29, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps that 2nd log was once supposed to say "created" (past tense),
but even that seemed redundant.Here is a small patch to remove the log duplication by keeping only
the log *within* the function.Thoughts?
======
Kind Regards,
Peter Smith.
Fujitsu Asutralia
<v1-0001-remove-duplicate-logging-for-slot-creation.patch>
The change is straightforward, especially I like the second change of adding “on publisher”, which makes the message clearer.
For dry-run mode, I see in setup_recovery(), it debug-logs a message with “# dry run mode recovery_target_lsn = xxx”, I think it would be good to add similar debug logs in create_logical_replication_slot(), create_publication() and drop_publication().
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Thu, Oct 9, 2025 at 1:31 PM Chao Li <li.evan.chao@gmail.com> wrote:
On Oct 8, 2025, at 14:29, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps that 2nd log was once supposed to say "created" (past tense),
but even that seemed redundant.Here is a small patch to remove the log duplication by keeping only
the log *within* the function.Thoughts?
======
Kind Regards,
Peter Smith.
Fujitsu Asutralia
<v1-0001-remove-duplicate-logging-for-slot-creation.patch>The change is straightforward, especially I like the second change of adding “on publisher”, which makes the message clearer.
Thanks.
For dry-run mode, I see in setup_recovery(), it debug-logs a message with “# dry run mode recovery_target_lsn = xxx”, I think it would be good to add similar debug logs in create_logical_replication_slot(), create_publication() and drop_publication().
Yes, I am already attempting to address those other dry-run logging
issues in another thread [1]/messages/by-id/CAHut+Pu84M8tgTjn++4WCTETOX=NR4YmLn-+4B29UJeDTgoJ9Q@mail.gmail.com.
======
[1]: /messages/by-id/CAHut+Pu84M8tgTjn++4WCTETOX=NR4YmLn-+4B29UJeDTgoJ9Q@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Oct 09, 2025 at 01:36:36PM +1100, Peter Smith wrote:
Thanks.
Done this one.
--
Michael