Re: AlterSubscription_refresh "wrconn" wrong variable?

On Wed, May 5, 2021 at 3:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, May 5, 2021 at 8:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Peter Smith <smithpb2250@gmail.com> writes:
> > > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name,
makingit 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
stillposslble 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