On Wed, May 5, 2021 at 3:20 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > On Wed, May 5, 2021 at 8:05 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Peter Smith <smithpb2...@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
