Re: [Proposal] Level4 Warnings show many shadow vars

On 2019-Dec-09, Ranier Vilela wrote:

> --- a/src/backend/access/transam/xlogreader.c
> +++ b/src/backend/access/transam/xlogreader.c
> @@ -70,7 +70,7 @@ report_invalid_record(XLogReaderState *state, const char *fmt,...)
> * Returns NULL if the xlogreader couldn't be allocated.
> */
> XLogReaderState *
> -XLogReaderAllocate(int wal_segment_size, const char *waldir,
> +XLogReaderAllocate(int wallog_segment_size, const char *waldir,
> XLogPageReadCB pagereadfunc, void *private_data)
> {
> XLogReaderState *state;

I find this choice a bit ugly and even more confusing than the original.
I'd change this to be just "segsize".

I would tend to name the GUC variable as if it were a global in the
sense that I proposed in my previous response (ie. WalSegmentSize), but
that creates a bit of a problem when one greps the source looking for
reference to the GUCs. Some GUCs do have CamelCase names and others
don't; I've grown fond of the current style of using the same name for
the variable as for the GUC itself, for grepping reasons. So I'm not
going to propose to do that. But let's not make a function parameter
have a name that vaguely suggests that it itself is a GUC.

> @@ -430,14 +430,14 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
> {
> XLogRecPtr lsn;
> char *err;
> - WalReceiverConn *wrconn;
> + WalReceiverConn *walrconn;
> List *tables;
> ListCell *lc;
> char table_state;
>
> /* Try to connect to the publisher. */
> - wrconn = walrcv_connect(conninfo, true, stmt->subname, &err);
> - if (!wrconn)
> + walrconn = walrcv_connect(conninfo, true, stmt->subname, &err);
> + if (!walrconn)
> ereport(ERROR,
> (errmsg("could not connect to the publisher: %s", err)));
>

Here I propose to rename the global instead (to WalReceiverConn maybe).
There's nothing about the name "wrconn" that suggests it's a global
variable. In any other place where the object is used as a local
variable, I'd just use "conn". Trying to be clever and adding a letter
here or a letter there makes it *more* likely that you'll reference the
wrong one in some function.

> index a9edbfd4a4..1f5921b6e7 100644
> --- a/src/backend/main/main.c
> +++ b/src/backend/main/main.c
> @@ -225,7 +225,7 @@ main(int argc, char *argv[])
> * without help. Avoid adding more here, if you can.
> */
> static void
> -startup_hacks(const char *progname)
> +startup_hacks(const char *prog_name)
> {
> /*
> * Windows-specific execution environment hacking.

I don't agree with this change very much. I think "progname" in
particular is a bit of a debacle right now but I don't think this is the
best fix. I'd leave this alone.

> diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
> index 8b2a2be1c0..e12f41cea4 100644
> --- a/src/backend/replication/walsender.c
> +++ b/src/backend/replication/walsender.c
> @@ -3223,7 +3223,7 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
> for (i = 0; i < max_wal_senders; i++)
> {
> WalSnd *walsnd = &WalSndCtl->walsnds[i];
> - XLogRecPtr sentPtr;
> + XLogRecPtr walsentPtr;
> XLogRecPtr write;
> XLogRecPtr flush;
> XLogRecPtr apply;

As before: let's rename the file-level static instead. "sentPtr" is not
a good name.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services