Re: 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