Re: AlterSubscription_refresh "wrconn" wrong variable?
Justin Pryzby <pryzby@telsasoft.com>
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Smith <smithpb2250@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2021-05-05T03:56:01Z
Lists: pgsql-hackers
On Tue, May 04, 2021 at 10:35:02PM -0400, Tom Lane wrote:
> Peter Smith <smithpb2250@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).
Yea, I think global vars should have at least 1) an underscore, or 2) a
capital, and in any case be 3) longer than 6 chars.
There's very few which violate both "arms" of that rule - should anything else
be renamed, too ?
$ git grep -E '^static [^(=]*\<[[:lower:]]{,6}(;$| =)' src/backend/'*.c'
src/backend/access/heap/vacuumlazy.c:static int elevel = -1;
src/backend/access/transam/xloginsert.c:static XLogRecData *rdatas;
src/backend/bootstrap/bootstrap.c:static MemoryContext nogc = NULL; /* special no-gc mem context */
src/backend/libpq/be-fsstubs.c:static MemoryContext fscxt = NULL;
src/backend/replication/walreceiver.c:static WalReceiverConn *wrconn = NULL;
src/backend/replication/walsender.c:static StringInfoData tmpbuf;
src/backend/storage/file/fd.c:static int nfile = 0;
src/backend/utils/misc/sampling.c:static ReservoirStateData oldrs;
pryzbyj@pryzbyj:~/src/postgres$ git grep -lE '^static [^(=]*\<[[:lower:]]{,6}(;$| =)' src/backend/'*.c' |xargs wc -l |sort -nr
4326 src/backend/access/heap/vacuumlazy.c
3781 src/backend/storage/file/fd.c
3698 src/backend/replication/walsender.c
1428 src/backend/replication/walreceiver.c
1227 src/backend/access/transam/xloginsert.c
1155 src/backend/bootstrap/bootstrap.c
864 src/backend/libpq/be-fsstubs.c
296 src/backend/utils/misc/sampling.c
--
Justin
Commits
-
Rename the logical replication global "wrconn"
- e5c071bc0f93 13.4 landed
- db16c656478b 14.0 landed
- b5b7cba7643c 10.18 landed
- b0e6e08b1981 11.13 landed
- 6e9723cde5c6 12.8 landed
-
AlterSubscription_refresh: avoid stomping on global variable
- ef70b6f8e28d 13.3 landed
- cfebc7080c99 12.7 landed
- b40865ebd41a 11.12 landed
- 4e8c0f1a0d0d 14.0 landed
- 1803c7626000 10.17 landed
-
Logical replication support for initial data copy
- 7c4f52409a8c 10.0 cited