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
