Re: AlterSubscription_refresh "wrconn" wrong variable?

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