Re: AlterSubscription_refresh "wrconn" wrong variable?
От | Peter Smith |
---|---|
Тема | Re: AlterSubscription_refresh "wrconn" wrong variable? |
Дата | |
Msg-id | CAHut+PvOy6bp+ZneqdjwSAE5ZfHOyFL8Fs8sMLpbyed7aNH92g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: AlterSubscription_refresh "wrconn" wrong variable? (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>) |
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления: