On Tue, May 4, 2021 at 5:00 AM Peter Smith <smithpb2...@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
