AlterSubscription_refresh "wrconn" wrong variable?

Started by Peter Smithover 4 years ago23 messages

smithpb2250@gmail.com

over 4 years ago

1 attachment(s)

While reviewing some logical replication code I stumbled across a
variable usage that looks suspicious to me.

Note that the AlterSubscription_refresh function (unlike other
functions in the subscriptioncmds.c) is using the global variable
"wrconn" instead of a local stack variable of the same name. I was
unable to think of any good reason why it would be deliberately doing
this, so my guess is that it is simply an accidental mistake that has
gone unnoticed because the compiler was silently equally happy just
using the global var.

Apparently, this is not causing any reported problems because it seems
like the code has been this way for ~4 years [1]https://github.com/postgres/postgres/commit/7c4f52409a8c7d85ed169bbbc1f6092274d03920#.

Even so, it doesn't look intentional to me and I felt that there may
be unknown consequences (e.g. resource leakage?) of just blatting over
the global var. So, PSA a small patch to make this
AlterSubscription_refresh function use a stack variable consistent
with the other nearby functions.

Thoughts?

------
[1]: https://github.com/postgres/postgres/commit/7c4f52409a8c7d85ed169bbbc1f6092274d03920#

Kind Regards,
Peter Smith.
Fujitsu Australia

Attachments:

v1-0001-Fix-wrconn.-Use-stack-variable.patchapplication/octet-stream; name=v1-0001-Fix-wrconn.-Use-stack-variable.patchDownload

From 70ab0e6ddc7514af58bf92ca47ea37b02eec8a04 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Fri, 30 Apr 2021 12:12:28 +1000
Subject: [PATCH v1] Fix wrconn. Use stack variable.

Unlike the other function in the subscriptioncmds.c, the AlterSubscription_refresh was accidentally using
the global variable "wrconn" instead of a local stack variable.

Confusion probably due to same named local/global variables.
---
 src/backend/commands/subscriptioncmds.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ec5c409..668cecd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -601,18 +601,19 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 		char		state;
 	} SubRemoveRels;
 	SubRemoveRels *sub_remove_rels;
+	WalReceiverConn *wrconn;
 
 	/* Load the library providing us libpq calls. */
 	load_file("libpqwalreceiver", false);
 
+	/* Try to connect to the publisher. */
+	wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
+	if (!wrconn)
+		ereport(ERROR,
+				(errmsg("could not connect to the publisher: %s", err)));
+
 	PG_TRY();
 	{
-		/* Try to connect to the publisher. */
-		wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
-		if (!wrconn)
-			ereport(ERROR,
-					(errmsg("could not connect to the publisher: %s", err)));
-
 		/* Get the table list from publisher. */
 		pubrel_names = fetch_table_list(wrconn, sub->publications);
 
@@ -782,8 +783,7 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	}
 	PG_FINALLY();
 	{
-		if (wrconn)
-			walrcv_disconnect(wrconn);
+		walrcv_disconnect(wrconn);
 	}
 	PG_END_TRY();
 
-- 
1.8.3.1