>I think it would be better to split this patch into separate files, >one for each global variable that is being shadowed. Ok, I agree.
> The reasonI 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? In general I do not agree to use global variables. But I understand when you use it, I believe it is a necessary evil. So I think that maybe the original author, has the same thought and when using a local parameter to pass the variable, and there is a way to further eliminate the use of the global variable, maybe it was unfortunate in choosing the name. And what it would do in this case, with the aim of eliminating the global variable in the future. In my own systems, I make use of only one global variable, and in many functions I pass as a parameter, with another name. >Another function within xlog.c behaves similarly: > RemoveXlogFile(...) >Only here, the callers sometimes pass the global RedoRecPtr >(tough 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. Again in this case, it would have to be checked whether postgres really will make use of the global variable forever. Which for me is a bad design. Another complicated case of global variable is PGconn * conn. It is defined as global somewhere, but there is widespread use of the name "conn" in many places in the code, many in / bin, so if it is in the interest of postgres to correct this, it would be better to rename the global variable to something like pg_conn, or gconn. >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. I appreciate your help. >Thanks again for the hard work and the patch! You are welcome. regards, Ranier Vilela -- Mark Dilger
