Tidy recent code bloat in pg_creatersubscriber::cleanup_objects_atexit
During a recent review of pg_creatersubscriber I saw that commit
e117cfb introduced a common 'dbinfos' struct to contain the array of
individual 'dbinfo[i]' infos. But, this now means that getting to each
dbinfo[i] requires another level of referencing.
In some places, e.g. function cleanup_objects_atexit, this caused
unnecessary code bloat. IMO this function is crying out for a local
variable to simplify the code again.
Please see the attached patch that implements this suggestion.
======
Kind Regards,
Peter Smith
Fujitsu Australia
Attachments:
v1-0001-Add-cleanup_objects_atexit-local-var.patchapplication/octet-stream; name=v1-0001-Add-cleanup_objects_atexit-local-var.patchDownload
From 92dbd7503211e604b72a438b186dce4e09c69cb8 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 11 Mar 2025 12:15:57 +1100
Subject: [PATCH v1] Add cleanup_objects_atexit local var
---
src/bin/pg_basebackup/pg_createsubscriber.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index a5a2d61..6baf92e 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -184,17 +184,19 @@ cleanup_objects_atexit(void)
for (int i = 0; i < num_dbs; i++)
{
- if (dbinfos.dbinfo[i].made_publication || dbinfos.dbinfo[i].made_replslot)
+ struct LogicalRepInfo *dbinfo = &dbinfos.dbinfo[i];
+
+ if (dbinfo->made_publication || dbinfo->made_replslot)
{
PGconn *conn;
- conn = connect_database(dbinfos.dbinfo[i].pubconninfo, false);
+ conn = connect_database(dbinfo->pubconninfo, false);
if (conn != NULL)
{
- if (dbinfos.dbinfo[i].made_publication)
- drop_publication(conn, &dbinfos.dbinfo[i]);
- if (dbinfos.dbinfo[i].made_replslot)
- drop_replication_slot(conn, &dbinfos.dbinfo[i], dbinfos.dbinfo[i].replslotname);
+ if (dbinfo->made_publication)
+ drop_publication(conn, dbinfo);
+ if (dbinfo->made_replslot)
+ drop_replication_slot(conn, dbinfo, dbinfo->replslotname);
disconnect_database(conn, false);
}
else
@@ -204,18 +206,18 @@ cleanup_objects_atexit(void)
* that some objects were left on primary and should be
* removed before trying again.
*/
- if (dbinfos.dbinfo[i].made_publication)
+ if (dbinfo->made_publication)
{
pg_log_warning("publication \"%s\" created in database \"%s\" on primary was left behind",
- dbinfos.dbinfo[i].pubname,
- dbinfos.dbinfo[i].dbname);
+ dbinfo->pubname,
+ dbinfo->dbname);
pg_log_warning_hint("Drop this publication before trying again.");
}
- if (dbinfos.dbinfo[i].made_replslot)
+ if (dbinfo->made_replslot)
{
pg_log_warning("replication slot \"%s\" created in database \"%s\" on primary was left behind",
- dbinfos.dbinfo[i].replslotname,
- dbinfos.dbinfo[i].dbname);
+ dbinfo->replslotname,
+ dbinfo->dbname);
pg_log_warning_hint("Drop this replication slot soon to avoid retention of WAL files.");
}
}
--
1.8.3.1
On Tue, Mar 11, 2025 at 12:29:42PM +1100, Peter Smith wrote:
During a recent review of pg_creatersubscriber I saw that commit
e117cfb introduced a common 'dbinfos' struct to contain the array of
individual 'dbinfo[i]' infos. But, this now means that getting to each
dbinfo[i] requires another level of referencing.In some places, e.g. function cleanup_objects_atexit, this caused
unnecessary code bloat. IMO this function is crying out for a local
variable to simplify the code again.
Right. This improves the clarity of the code, so agreed about the use
of a local variable here.
--
Michael
On Tue, Mar 11, 2025 at 04:06:14PM +0900, Michael Paquier wrote:
Right. This improves the clarity of the code, so agreed about the use
of a local variable here.
More code paths of pg_createsubscriber.c have similar loops, but this
is the only one where LogicalRepInfo can be used. So, applied as you
have suggested, without touching the other loops.
--
Michael