Horton

Thread

Commits

  1. Rename the logical replication global "wrconn"

  2. AlterSubscription_refresh: avoid stomping on global variable

  3. Logical replication support for initial data copy

  1. AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-03T23:29:42Z

    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] https://github.com/postgres/postgres/commit/7c4f52409a8c7d85ed169bbbc1f6092274d03920#

Kind Regards,
Peter Smith.
Fujitsu Australia

  2. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-04T03:56:36Z

    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

  3. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Andres Freund <andres@anarazel.de> — 2021-05-04T04:31:49Z

    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:
https://www.postgresql.org/message-id/20201111215820.qihhrz7fayu6myfi%40alap3.anarazel.de

Peter?

Greetings,

Andres Freund

  4. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-04T05:08:11Z

    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] although I
never would have thought of that tricky trigger / refresh scenario.

------
[1] https://www.postgresql.org/message-id/20210504043149.vg4w66vuh4qjrbph%40alap3.anarazel.de

Kind Regards,
Peter Smith.
Fujitsu Australia

  5. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-04T05:13:24Z

    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:
> https://www.postgresql.org/message-id/20201111215820.qihhrz7fayu6myfi%40alap3.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

  6. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-05T00:15:58Z

    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

  7. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-05T02:35:02Z

    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

  8. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Justin Pryzby <pryzby@telsasoft.com> — 2021-05-05T03:56:01Z

    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

  9. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-05T05:20:09Z

    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

  10. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-05T05:38:12Z

    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

  11. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-06T09:08:49Z

    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

  12. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Japin Li <japinli@hotmail.com> — 2021-05-06T09:17:59Z

    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.

  13. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-06T09:30:23Z

    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

  14. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Japin Li <japinli@hotmail.com> — 2021-05-06T09:42:31Z

    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.

  15. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-06T10:09:53Z

    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

  16. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-06T20:58:28Z

    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"

  17. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-06T21:08:50Z

    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

  18. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-07T08:09:05Z

    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

  19. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-10T02:20:00Z

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

  20. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-10T03:53:01Z

    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

  21. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Peter Smith <smithpb2250@gmail.com> — 2021-05-10T06:15:06Z

    PSA v5 of the patch. It is the same as v4 but with the v4-0001 part
omitted because that was already pushed.

(reposted to keep cfbot happy).

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

  22. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-10T18:19:17Z

    On 2021-May-10, Peter Smith wrote:

> PSA v5 of the patch. It is the same as v4 but with the v4-0001 part
> omitted because that was already pushed.

I made a few whitespace adjustments on Friday that I didn't get time to
push, so I left the whole set to after the minors are finalized this
week.  I'll get them pushed on Wednesday or so.  (The back branches have
a few conflicts, on every release, but I see no reason to post those and
it'd upset the cfbot).

-- 
Álvaro Herrera       Valdivia, Chile

  23. Re: AlterSubscription_refresh "wrconn" wrong variable?

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-05-12T23:19:27Z

    On 2021-May-10, Peter Smith wrote:

> PSA v5 of the patch. It is the same as v4 but with the v4-0001 part
> omitted because that was already pushed.

Thanks, I have pushed this.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W