AlterSubscription_refresh "wrconn" wrong variable?

On Tue, May 4, 2021 at 5:00 AM Peter Smith <smithpb2250@gmail.com> wrote:

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].

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. It looks like the global variable wrconn defined/declared in
worker_internal.h/worker.c, is for logical apply/table sync worker and
it doesn't make sense to use it for CREATE/ALTER subscription refresh
code that runs on a backend. And I couldn't think of any unknown
consequences/resource leakage, because that global variable is being
used by different processes which have their own copy.

And, the patch basically looks good to me, except a bit of rewording
the commit message to something like "Use local variable wrconn in
AlterSubscription_refresh instead of global a variable with the same
name which is meant to be used for logical apply/table sync worker.
Having the wrconn global variable in AlterSubscription_refresh doesn't
cause any real issue as such but it keeps the code in
subscriptioncmds.c inconsistent with other functions which use a local
variable named wrconn." or some other better wording?

Regression tests were passed on my dev system with the patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Hi,

On 2021-05-04 09:29:42 +1000, Peter Smith wrote:

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].

This sounded vaguely familiar. After a bit of searching I found that's
because I debugged a crash related to it:
/messages/by-id/20201111215820.qihhrz7fayu6myfi@alap3.anarazel.de

Peter?

Greetings,

Andres Freund

On Tue, May 4, 2021 at 1:56 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Tue, May 4, 2021 at 5:00 AM Peter Smith <smithpb2250@gmail.com> wrote:

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].

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. It looks like the global variable wrconn defined/declared in
worker_internal.h/worker.c, is for logical apply/table sync worker and
it doesn't make sense to use it for CREATE/ALTER subscription refresh
code that runs on a backend. And I couldn't think of any unknown
consequences/resource leakage, because that global variable is being
used by different processes which have their own copy.

And, the patch basically looks good to me, except a bit of rewording
the commit message to something like "Use local variable wrconn in
AlterSubscription_refresh instead of global a variable with the same
name which is meant to be used for logical apply/table sync worker.
Having the wrconn global variable in AlterSubscription_refresh doesn't
cause any real issue as such but it keeps the code in
subscriptioncmds.c inconsistent with other functions which use a local
variable named wrconn." or some other better wording?

Regression tests were passed on my dev system with the patch.

Thanks for your feedback.

I can post another patch (or same patch with an improved commit
comment) later, but I will just wait a day first in case there is more
information to say about it. e.g. my suspicion that there would be
"consequences" seems to have come to fruition after all [1]/messages/by-id/20210504043149.vg4w66vuh4qjrbph@alap3.anarazel.de although I
never would have thought of that tricky trigger / refresh scenario.

------
[1]: /messages/by-id/20210504043149.vg4w66vuh4qjrbph@alap3.anarazel.de

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, May 4, 2021 at 2:31 PM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2021-05-04 09:29:42 +1000, Peter Smith wrote:

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].

This sounded vaguely familiar. After a bit of searching I found that's
because I debugged a crash related to it:
/messages/by-id/20201111215820.qihhrz7fayu6myfi@alap3.anarazel.de

Oh! No wonder it sounded familiar.

It looks like I've just re-discovered the identical problem 5 months
after your post.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

PSA v2 of this patch - it has the same content, but an improved commit comment.

I have also added a commitfest entry, https://commitfest.postgresql.org/33/3109/

------
Kind Regards,
Peter Smith
Fujitsu Australia

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

regards, tom lane

On Tue, May 04, 2021 at 10:35:02PM -0400, Tom Lane wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

Yea, I think global vars should have at least 1) an underscore, or 2) a
capital, and in any case be 3) longer than 6 chars.

There's very few which violate both "arms" of that rule - should anything else
be renamed, too ?

$ git grep -E '^static [^(=]*\<[[:lower:]]{,6}(;$| =)' src/backend/'*.c'
src/backend/access/heap/vacuumlazy.c:static int elevel = -1;
src/backend/access/transam/xloginsert.c:static XLogRecData *rdatas;
src/backend/bootstrap/bootstrap.c:static MemoryContext nogc = NULL; /* special no-gc mem context */
src/backend/libpq/be-fsstubs.c:static MemoryContext fscxt = NULL;
src/backend/replication/walreceiver.c:static WalReceiverConn *wrconn = NULL;
src/backend/replication/walsender.c:static StringInfoData tmpbuf;
src/backend/storage/file/fd.c:static int nfile = 0;
src/backend/utils/misc/sampling.c:static ReservoirStateData oldrs;

pryzbyj@pryzbyj:~/src/postgres$ git grep -lE '^static [^(=]*\<[[:lower:]]{,6}(;$| =)' src/backend/'*.c' |xargs wc -l |sort -nr
4326 src/backend/access/heap/vacuumlazy.c
3781 src/backend/storage/file/fd.c
3698 src/backend/replication/walsender.c
1428 src/backend/replication/walreceiver.c
1227 src/backend/access/transam/xloginsert.c
1155 src/backend/bootstrap/bootstrap.c
864 src/backend/libpq/be-fsstubs.c
296 src/backend/utils/misc/sampling.c

--
Justin

On Wed, May 5, 2021 at 8:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

I don't think "tablesync_wrconn" is the right name, because wrconn is
also being used in logical replication apply worker. So something like
"apply_worker_wrconn" would be more meaningful.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 5, 2021 at 3:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, May 5, 2021 at 8:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

OK, I am happy to change this but firstly just need some consensus on
the new name to use. I hope to avoid changing it, and then changing it
5 more times.

I don't think "tablesync_wrconn" is the right name, because wrconn is
also being used in logical replication apply worker. So something like
"apply_worker_wrconn" would be more meaningful.

Yes. that is better except I wonder if "apply_worker_wrconn" might
seem unusual when used by the tablesync worker.

My suggestion is "lrep_worker_wrconn" which seems ok for both apply /
tablesyn workers.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

------
Kind Regards,
Peter Smith
Fujitsu Australia

On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
PG_TRY() block?
+       /* 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);

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, May 6, 2021 at 7:18 PM Japin Li <japinli@hotmail.com> wrote:

On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
PG_TRY() block?
+       /* 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);

Thanks for your review. Reason for moving that out of the PG_TRY are:

a) It makes code now consistent with other functions using wrconn. See
CreateSubscription, DropSubscription etc

b) It means don't need the wrconn NULL check anymore in the PG_FINALLY
so it simplifies the disconnect.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, 06 May 2021 at 17:30, Peter Smith <smithpb2250@gmail.com> wrote:

On Thu, May 6, 2021 at 7:18 PM Japin Li <japinli@hotmail.com> wrote:

On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
PG_TRY() block?
+       /* 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);

Thanks for your review. Reason for moving that out of the PG_TRY are:

a) It makes code now consistent with other functions using wrconn. See
CreateSubscription, DropSubscription etc

b) It means don't need the wrconn NULL check anymore in the PG_FINALLY
so it simplifies the disconnect.

Thanks for your explanation!

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, May 6, 2021 at 3:00 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Thu, May 6, 2021 at 7:18 PM Japin Li <japinli@hotmail.com> wrote:

On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:

On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making it consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
The global wrconn is only meant to be used for logical apply/tablesync worker.
Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]

Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
PG_TRY() block?
+       /* 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);

Thanks for your review. Reason for moving that out of the PG_TRY are:

a) It makes code now consistent with other functions using wrconn. See
CreateSubscription, DropSubscription etc

b) It means don't need the wrconn NULL check anymore in the PG_FINALLY
so it simplifies the disconnect.

And even if any error occurs after the connection is established and
while libpqrcv_PQexec is being done in libpqrcv_connect, we reach
PG_FINALLY() block to disconnect the connection, so no connection leak
can occur.

Patch looks good to me except for the comments in the commit message:
1) it crosses 80 char limit 2) a typo : "posslble"

Please add it to the current commitfest if not done already so that we
don't lose track of it and the patch gets a chance to be tested.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021-May-06, Peter Smith wrote:

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

I think there are two patches here -- the changes to
AlterSubscription_refresh are a backpatchable bugfix, and the rest of it
can just be applied to master.

In my mind we make a bit of a distinction for global variables by using
CamelCase rather than undercore_separated_words. There are plenty that
violate that "rule" of course, but ISTM that makes them stand more and
it's less likely we've made this mistake. So I would name the variable
LogRepWALRcvConn or something like that. My €0.02.

--
Álvaro Herrera Valdivia, Chile
"Entristecido, Wutra (canción de Las Barreras)
echa a Freyr a rodar
y a nosotros al mar"

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-May-06, Peter Smith wrote:

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

I think there are two patches here -- the changes to
AlterSubscription_refresh are a backpatchable bugfix, and the rest of it
can just be applied to master.

The rename of that variable is just cosmetic, true, but I'd still be
inclined to back-patch it. If we don't do so then I'm afraid that
future back-patched fixes might be bitten by the same confusion,
possibly introducing new real bugs.

Having said that, keeping the two aspects in separate patches might
ease review and testing.

In my mind we make a bit of a distinction for global variables by using
CamelCase rather than undercore_separated_words.

I think it's about 50/50, TBH. I'd stick with whichever style is
being used in nearby code.

regards, tom lane

On Fri, May 7, 2021 at 7:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-May-06, Peter Smith wrote:

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

I think there are two patches here -- the changes to
AlterSubscription_refresh are a backpatchable bugfix, and the rest of it
can just be applied to master.

The rename of that variable is just cosmetic, true, but I'd still be
inclined to back-patch it. If we don't do so then I'm afraid that
future back-patched fixes might be bitten by the same confusion,
possibly introducing new real bugs.

Having said that, keeping the two aspects in separate patches might
ease review and testing.

Done.

In my mind we make a bit of a distinction for global variables by using
CamelCase rather than undercore_separated_words.

I think it's about 50/50, TBH. I'd stick with whichever style is
being used in nearby code.

The nearby code was a random mixture of Camels and Snakes, so instead
of flipping a coin I went with the suggestion from Alvaro.

~~

PSA v4 of the patch.

0001 - Fixes the AlterSubscription_refresh as before.
0002 - Renames the global var "wrconn" -> "LogRepWorkerWalRcvConn" as suggested.

------
Kind Regards,
Peter Smith
Fujitsu Australia

On Fri, May 7, 2021 at 6:09 PM Peter Smith <smithpb2250@gmail.com> wrote:

On Fri, May 7, 2021 at 7:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-May-06, Peter Smith wrote:

PSA v3 of the patch. Same as before, but now also renames the global
variable from "wrconn" to "lrep_worker_wrconn".

I think there are two patches here -- the changes to
AlterSubscription_refresh are a backpatchable bugfix, and the rest of it
can just be applied to master.

The rename of that variable is just cosmetic, true, but I'd still be
inclined to back-patch it. If we don't do so then I'm afraid that
future back-patched fixes might be bitten by the same confusion,
possibly introducing new real bugs.

Having said that, keeping the two aspects in separate patches might
ease review and testing.

Done.

In my mind we make a bit of a distinction for global variables by using
CamelCase rather than undercore_separated_words.

I think it's about 50/50, TBH. I'd stick with whichever style is
being used in nearby code.

The nearby code was a random mixture of Camels and Snakes, so instead
of flipping a coin I went with the suggestion from Alvaro.

~~

PSA v4 of the patch.

0001 - Fixes the AlterSubscription_refresh as before.
0002 - Renames the global var "wrconn" -> "LogRepWorkerWalRcvConn" as suggested.

It seems that the 0001 part of this patch was pushed in the weekend [1]https://github.com/postgres/postgres/commit/4e8c0f1a0d0d095a749a329a216c88a340a455b6. Thanks!

But what about the 0002 part? If there is no immediate plan to push
that also then I will post a v5 just to stop the cfbot complaining.

--------
[1]: https://github.com/postgres/postgres/commit/4e8c0f1a0d0d095a749a329a216c88a340a455b6

KInd Regards,
Peter Smith
Fujitsu Australia

On Mon, May 10, 2021 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote:

0001 - Fixes the AlterSubscription_refresh as before.
0002 - Renames the global var "wrconn" -> "LogRepWorkerWalRcvConn" as suggested.

It seems that the 0001 part of this patch was pushed in the weekend [1]. Thanks!

But what about the 0002 part? If there is no immediate plan to push
that also then I will post a v5 just to stop the cfbot complaining.

I think the 0002 patch can be posted here, if it looks good, it can be
made "Ready For Committer".

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com