Re: [Proposal] Level4 Warnings show many shadow vars

Mark Dilger <hornschnorter@gmail.com> writes:
> 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?

I was wondering about that too.  A look in the git history seems
to say that it is the fault of the fairly-recent commit d9fadbf13,
which did things like this:

 /*
  * Recycle or remove all log files older or equal to passed segno.
  *
- * endptr is current (or recent) end of xlog, and PriorRedoRecPtr is the
- * redo pointer of the previous checkpoint. These are used to determine
+ * endptr is current (or recent) end of xlog, and RedoRecPtr is the
+ * redo pointer of the last checkpoint. These are used to determine
  * whether we want to recycle rather than delete no-longer-wanted log files.
  */
 static void
-RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr PriorRedoPtr, XLogRecPtr endptr)
+RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
 {
    DIR        *xldir;
    struct dirent *xlde;

That is, these arguments *used* to be a different LSN pointer, and that
commit changed them to be mostly equal to RedoRecPtr, and made what
seems like a not very well-advised renaming to go with that.

> 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.

Don't think I buy that.  The fact that these arguments were until recently
different from RedoRecPtr suggests that they might someday be different
again, whereupon we'd have to laboriously revert such a parameter redesign.
I think I'd just go for names that don't have a hard implication that
the parameter values are the same as any particular global variable.

> 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,

Indeed.  Once again, these are case-by-case issues, not something
that can be improved by a global search-and-replace without much
consideration for the details of each case.

			regards, tom lane