Re: [Proposal] Level4 Warnings show many shadow vars

On 12/7/19 3:42 PM, Ranier Vilela wrote:
> This is the first part of the variable shadow fixes.
> Basically it consists of renaming the variables in collision with the global ones, with the minimum change in the
semantics.
> 
> make check pass all the tests.

I think it would be better to split this patch into separate files,
one for each global variable that is being shadowed.  The reason
I say so is apparent looking at the first one in the patch,
RedoRecPtr.  This process global variable is defined in xlog.c:

   static XLogRecPtr RedoRecPtr;

and then, somewhat surprisingly, passed around between static
functions defined within that same file, such as:

   RemoveOldXlogFiles(...)

which in the current code only ever gets a copy of the global,
which begs the question why it needs this passed as a parameter
at all.  All the places calling RemoveOldXlogFiles are within
this file, and all of them pass the global, so why bother?

Another function within xlog.c behaves similarly:

   RemoveXlogFile(...)

Only here, the callers sometimes pass the global RedoRecPtr
(though indirectly, since they themselves received it as an
argument) and sometimes they pass in InvalidXLogRecPtr, which
is just a constant:

   src/include/access/xlogdefs.h:#define InvalidXLogRecPtr    0

So it might make sense to remove the parameter from this
function, too, and replace it with a flag parameter named
something like "is_valid", or perhaps split the function
into two functions, one for valid and one for invalid.

I'm not trying to redesign xlog.c's functions in this email
thread, but only suggesting that these types of arguments
may ensue for each global variable in your patch, and it will
be easier for a committer to know if there is a consensus
about any one of them than about the entire set.  When I
suggested you do this patch set all on this thread, I was
still expecting multiple patches, perhaps named along the
lines of:

   unshadow.RedoRecPtr.patch.1
   unshadow.wal_segment_size.patch.1
   unshadow.synchronous_commit.patch.1
   unshadow.wrconn.patch.1
   unshadow.progname.patch.1
   unshadow.am_syslogger.patch.1
   unshadow.days.patch.1
   unshadow.months.patch.1

etc.  I'm uncomfortable giving you negative feedback of this
sort, since I think you are working hard to improve postgres
and I really appreciate it, so later tonight I'll try to come
back, split your patch for you as described, add an entry to
the commitfest if you haven't already, and mark myself as a
reviewer.

Thanks again for the hard work and the patch!


-- 
Mark Dilger