[Proposal] Level4 Warnings show many shadow vars

On Tue, Dec 3, 2019 at 8:24 PM Ranier Vilela <ranier_gyn@hotmail.com> wrote:

I believe PostgreSQL can benefit from changing the alert level of compilation warnings.
The current Level3 level for windows does not show any alerts, but that does not mean that there are no problems.
Changing the level to Level4 and its equivalent for GCC in Unix environments will show many warnings for shadow variables, including global variables.
True, there will also be many silly alerts that can be safely disabled.
Shadow variables, although they may not currently represent bugs, may be hiding errors, or at the very least, it is a waste of variable declaration.
With the current Level3 level, development is no longer checking and correcting shadow variables.

Any comments?

Most of us don't develop on Windows, so changing warning levels on
Windows won't really affect what developers see on their own machines,
and thus probably doesn't have much value. It might be a good idea to
try to clean up some/many cases of shadowed variables, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert wrote:

Most of us don't develop on Windows, so changing warning levels on
Windows won't really affect what developers see on their own machines,
and thus probably doesn't have much value.

Yes the report is from msvc 2017.
Even so, there is some failure to review or compile in Unix environment, because there are so many cases.
-Wshadow with GCC can show the alerts.

It might be a good idea to>try to clean up some/many cases of shadowed >variables, though.

Interested in submitting the msvc 2017 report?

Ranier Vilela

On Thu, Dec 5, 2019 at 11:26 AM Ranier Vilela <ranier_gyn@hotmail.com> wrote:

Even so, there is some failure to review or compile in Unix environment, because there are so many cases.
-Wshadow with GCC can show the alerts.

I mean, compiler warnings are not errors, and there's no requirement
that we fix every warning. I compile with -Wall -Werror regularly and
that works fine. I don't necessarily feel like I have to turn on more
warnings that aren't shown by default on the platforms I use. One way
of looking at it: if a warning isn't enabled by -Wall, it's probably
something that either isn't that important or would generate too many
false positives.

Interested in submitting the msvc 2017 report?

I think if this is an issue you care about, it's up to you to think of
doing something about it, like going through the report and submitting
patches for whichever cases you think need to be addressed. Cleaning
up stuff like this is potentially a lot of work, and I struggle to
keep up with all the work I've already got.

If you do decide to work on this, I recommend against preparing a
single giant patch that changes every single one blindly. Try to think
about which cases are the most egregious/dangerous and propose patches
for those first. If those are accepted then you can move on to other
cases.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/3/19 5:24 PM, Ranier Vilela wrote:

Hi,
I believe PostgreSQL can benefit from changing the alert level of compilation warnings.
The current Level3 level for windows does not show any alerts, but that does not mean that there are no problems.
Changing the level to Level4 and its equivalent for GCC in Unix environments will show many warnings for shadow variables, including global variables.
True, there will also be many silly alerts that can be safely disabled.
Shadow variables, although they may not currently represent bugs, may be hiding errors, or at the very least, it is a waste of variable declaration.
With the current Level3 level, development is no longer checking and correcting shadow variables.

Any comments?

I suggested increasing the default warnings in an email some time ago,
to which Tom made reasonable objections. You might want to take a
look at his comments, and consider if you can overcome the concerns
he had:

/messages/by-id/25938.1487367117@sss.pgh.pa.us

and

/messages/by-id/30007.1487374499@sss.pgh.pa.us

--
Mark Dilger

Mark Dilger <hornschnorter@gmail.com> writes:

On 12/3/19 5:24 PM, Ranier Vilela wrote:

Any comments?

I suggested increasing the default warnings in an email some time ago,
to which Tom made reasonable objections.

Yes, that whole thread is worth a read in this context:

/messages/by-id/CAEepm=15e9L695yVCO-_OkBVbsPupyXqzYWzzDmj-bdJ6o2+Pw@mail.gmail.com

The original concern about --disable-integer-datetimes is history
now, but I think the variability of error reports between compilers
is still instructive and relevant.

regards, tom lane

On Thu, Dec 05, 2019 at 01:41:20PM -0500, Robert Haas wrote:

If you do decide to work on this, I recommend against preparing a
single giant patch that changes every single one blindly. Try to think
about which cases are the most egregious/dangerous and propose patches
for those first. If those are accepted then you can move on to other
cases.

+1.  An case-by-case analysis is key here because it depends on the
context of the code.  I am ready to bet that we don't care about most
of them, still that there are cases which actually matter a lot.
--
Michael

De: Mark Dilger <hornschnorter@gmail.com>
Enviado: quinta-feira, 5 de dezembro de 2019 21:06

I suggested increasing the default warnings in an email some time ago,
to which Tom made reasonable objections. You might want to take a
look at his comments, and consider if you can overcome the concerns
he had:

I understand Tom's considerations.
What I mean is, everyone already knows, it's easier and safer to fix this kind of mistake early.
I'll do as Robert asked, but as with global variables, it's hard to fix.
What did the original author want, use the global variable or not use it by overriding the name.
If it was to use the global variable, it will affect the behavior of the function, I believe.

regards,
Ranier Vilela

On Fri, Dec 6, 2019 at 7:59 AM Ranier Vilela <ranier_gyn@hotmail.com> wrote:

What did the original author want, use the global variable or not use it by overriding the name.
If it was to use the global variable, it will affect the behavior of the function, I believe.

Well, you haven't provided any examples, so it's hard to be sure, but
I suspect that the vast majority of these are not actually bugs, but
just name collisions that don't really matter. Some of them could even
be Windows-specific things. For example, if Windows - or any other
platform - happened to have a variable declared in a library header
file someplace that is relatively commonly used within PostgreSQL as a
local variable name (e.g. "lc"), it would produce tons of name
collisions, none of which would be bugs.

The thing is, it's well-known that this is not good programming
practice, and I doubt that any committer would intentionally commit
code that used the same variable name for a file-level global and a
local variable in that same file. Perhaps a few such cases have crept
in by accident, but I bet they are rare. What's probably more likely
is that somebody - either a PostgreSQL developer or a Microsoft
developer - carelessly exposed a global name that's not very
distinctive, and it then collided -- either then or later -- with some
local variables in various places within the PostgreSQL code. If those
are names exposed by PostgreSQL, we should just rename the global
variables we exposed to have more distinctive names. If they're
exposed by Microsoft, we don't have that option, so we either have to
rename the local variables that shadow them, or decide that we don't
care.

Based on previous discussion in this forum, my guess is that popular
sentiment will depend quite a bit on how reasonable it seems that
Microsoft chose to use the name in the first place. If there's an
"extern int i;" declaration someplace in a Windows header file, we are
not for that reason going to abandon our practice of using "i" for
loop variables; we're (probably) just going to say nasty things about
Microsoft and keep doing what we're doing. If there's an "extern int
__msft_ftw;" declaration in a Windows header file and for some reason
we've used that same name in our code, we're going to decide we were
dumb to use that as a name and change it. The amount of code churn
also plays a role. People will be reluctant to change thousands of
lines of PostgreSQL code to work around Microsoft-specific problems,
but if it's only a little bit of code then people won't mind very
much.

Maybe you want to post a few examples. It's hard to discuss in the abstract.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:

Maybe you want to post a few examples. It's hard to discuss in the abstract.

I am working on the patch.
I think this is a great example.
I do not know if it is better to rename the local parameter, or if it should be renamed the global variable.

line: 68
var char **synchronous_commit
backend/commands/subscriptioncmds.c

global var declared here:
/include/access/xact.h(82)

One question, is it better to submit the patch on this topic, or create a new one?

regards,
Ranier Vilela

On 12/6/19 3:18 PM, Ranier Vilela wrote:

Robert Haas wrote:

Maybe you want to post a few examples. It's hard to discuss in the abstract.

I am working on the patch.
I think this is a great example.
I do not know if it is better to rename the local parameter, or if it should be renamed the global variable.

line: 68
var char **synchronous_commit
backend/commands/subscriptioncmds.c

global var declared here:
/include/access/xact.h(82)

The local variables in subscriptioncmds.c named "synchronous_commit"
appear more times in that one file than the global variable appears
in total in the rest of the system, but that doesn't include other
references to the guc in code comments, in user facing strings, etc.

I think it is better to change this just in subscriptioncmds.c than
to change the global variable name everywhere else. I also tend to
agree with you that shadowing the global variable is bad practice.

One question, is it better to submit the patch on this topic, or create a new one?

You appear to be planning to submit lots of patches about lots of
different shadowed variables. If you start a new thread for this
particular variable, it seems you'd probably do that again and again
for the other ones, and that might be tedious for readers of the
-hackers list who aren't interested. To start, I'd prefer to see
the patch on this thread.

--
Mark Dilger

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.

regards,
Ranier Vilela

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

Ranier Vilela <ranier_gyn@hotmail.com> writes:

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.

I don't think I'm actually on board with the goal here.

Basically, if we take this seriously, we're throwing away the notion of
nested variable scopes and programming as if we had just a flat namespace.
That wasn't any fun when we had to do it back in assembly-code days, and
I don't see a good reason to revert to that methodology today.

In a few of these cases, like the RedoRecPtr changes, there *might* be
an argument that there's room for confusion about whether the code could
have meant to reference the similarly-named global variable. But it's
just silly to make that argument in places like your substitution of
/days/ndays/ in date.c.

Based on this sample, I reject the idea that we ought to be trying to
eliminate this class of warnings just because somebody's compiler can be
induced to emit them. If you want to make a case-by-case argument that
particular situations of this sort are bad programming style, let's have
that argument by all means. But it needs to be case-by-case, not just
dropping a large patch on us containing a bunch of unrelated changes
and zero specific justification for any of them.

IOW, I don't think you've taken to heart Robert's upthread advice that
this needs to be case-by-case and based on literary not mechanical
considerations.

BTW, if we do go forward with changing the RedoRecPtr uses, I'm not
in love with "XRedoRecPtr" as a replacement parameter name; it conveys
nothing much, and the "X" prefix is already overused in that area of
the code. Perhaps "pRedoRecPtr" would be a better choice? Or maybe
make the local variables be all-lower-case "redorecptr", which would
fit well in context in places like

-RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
+RemoveXlogFile(const char *segname, XLogRecPtr XRedoRecPtr, XLogRecPtr endptr)

regards, tom lane

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

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

I don't think I'm actually on board with the goal here.

Ok, I understand.

Basically, if we take this seriously, we're throwing away the notion of
nested variable scopes and programming as if we had just a flat namespace.
That wasn't any fun when we had to do it back in assembly-code days, and
I don't see a good reason to revert to that methodology today.

In general I think the use global variables its a bad design. But I understand the use.

In a few of these cases, like the RedoRecPtr changes, there *might* be
an argument that there's room for confusion about whether the code could
have meant to reference the similarly-named global variable. But it's
just silly to make that argument in places like your substitution of
/days/ndays/ in date.c.

I would rather fix everything, including days name.

Based on this sample, I reject the idea that we ought to be trying to
eliminate this class of warnings just because somebody's compiler can be
induced to emit them. If you want to make a case-by-case argument that
particular situations of this sort are bad programming style, let's have
that argument by all means. But it needs to be case-by-case, not just
dropping a large patch on us containing a bunch of unrelated changes
and zero specific justification for any of them.

This is why I suggested activating the alert in the development and review process, so that any cases that arose would be corrected very early.

IOW, I don't think you've taken to heart Robert's upthread advice that
this needs to be case-by-case and based on literary not mechanical
considerations.

Ok, right.
But I was working on the second class of shadow variables, which are local variables, within the function itself, where the patch would lead to a removal of the variable declaration, maintaining the same logic and functionality, which would lead to better performance and reduction. of memory usage as well as very small.
In that case, too, would it have to be case by case?
Wow, there are many and many shadow variables ...

BTW, if we do go forward with changing the RedoRecPtr uses, I'm not
in love with "XRedoRecPtr" as a replacement parameter name; it conveys
nothing much, and the "X" prefix is already overused in that area of
the code. Perhaps "pRedoRecPtr" would be a better choice? Or maybe
make the local variables be all-lower-case "redorecptr", which would
fit well in context in places like

pRedoRecPtr, It's perfect for me.

regards,
Ranier Vilela

Hello.

At Mon, 9 Dec 2019 01:30:33 +0000, Ranier Vilela <ranier_gyn@hotmail.com> wrote in

I don't think I'm actually on board with the goal here.

Ok, I understand.

Basically, if we take this seriously, we're throwing away the notion of
nested variable scopes and programming as if we had just a flat namespace.
That wasn't any fun when we had to do it back in assembly-code days, and
I don't see a good reason to revert to that methodology today.

In general I think the use global variables its a bad design. But I understand the use.

The file-scoped variable is needed to be process-persistent in any
way. If we inhibit them, the upper-modules need to create the
persistent area instead, for example, by calling XLogInitGlobals() or
such, which makes things messier. Globality doens't necessarily mean
evil and there're reasons for -Wall doesn't warn the case. I believe
we, and especially committers are not who should be kept away from
knives for the reason that knives generally have a possibility to
injure someone.

In a few of these cases, like the RedoRecPtr changes, there *might* be
an argument that there's room for confusion about whether the code could
have meant to reference the similarly-named global variable. But it's
just silly to make that argument in places like your substitution of
/days/ndays/ in date.c.

I would rather fix everything, including days name.

I might be too accustomed there, but the functions that define
overriding locals don't modify the local variables and only the
functions that don't override the globals modifies the glboals. I see
no significant confusion here. By the way changes like "conf_file" ->
"conffile" seems really useless as a fix patch.

Based on this sample, I reject the idea that we ought to be trying to
eliminate this class of warnings just because somebody's compiler can be
induced to emit them. If you want to make a case-by-case argument that
particular situations of this sort are bad programming style, let's have
that argument by all means. But it needs to be case-by-case, not just
dropping a large patch on us containing a bunch of unrelated changes
and zero specific justification for any of them.

This is why I suggested activating the alert in the development and review process, so that any cases that arose would be corrected very early.

I don't think it contributes to the argument on programming style in
any way.

IOW, I don't think you've taken to heart Robert's upthread advice that
this needs to be case-by-case and based on literary not mechanical
considerations.

Ok, right.
But I was working on the second class of shadow variables, which are local variables, within the function itself, where the patch would lead to a removal of the variable declaration, maintaining the same logic and functionality, which would lead to better performance and reduction. of memory usage as well as very small.
In that case, too, would it have to be case by case?
Wow, there are many and many shadow variables ...

As Robert said, they are harmless as far as we notice. Actual bugs
caused by variable overriding would be welcomed to fix. I don't
believe "lead to better performance and reduction (of code?)" without
an evidence since modern compilers I think are not so stupid. Even if
any, performance change in such extent doesn't support the proposal to
remove variable overrides that way.

BTW, if we do go forward with changing the RedoRecPtr uses, I'm not
in love with "XRedoRecPtr" as a replacement parameter name; it conveys
nothing much, and the "X" prefix is already overused in that area of
the code. Perhaps "pRedoRecPtr" would be a better choice? Or maybe
make the local variables be all-lower-case "redorecptr", which would
fit well in context in places like

pRedoRecPtr, It's perfect for me.

Anyway I strongly object to the name 'pRedoRecPtr', which suggests as
if it is a C-pointer to some variable. (And I believe we use Hungarian
notation only if we don't have a better way...) LatestRedoRecPtr
looks better to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 12/8/19 10:25 AM, Mark Dilger wrote:

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.

To start off, I've taken just six of the 22 or so variables
that you renamed and created patches for them. I'm not
endorsing these in any way. I chose these mostly based on
which ones showed up first in your patch file, with one
exception.

I stopped when I got to 'progname' => 'prog_name' as the
whole exercise was getting too absurd even for me. That
clearly looks like one where the structure of the code
needs to be reconsidered, rather than just renaming stuff.

I'll create the commitfest entry based on this email once
this has been sent.

Patches attached.

--
Mark Dilger

On 12/8/19 8:50 PM, Mark Dilger wrote:

On 12/8/19 10:25 AM, Mark Dilger wrote:

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.

To start off, I've taken just six of the 22 or so variables
that you renamed and created patches for them.� I'm not
endorsing these in any way.� I chose these mostly based on
which ones showed up first in your patch file, with one
exception.

I stopped when I got to 'progname' => 'prog_name' as the
whole exercise was getting too absurd even for me.� That
clearly looks like one where the structure of the code
needs to be reconsidered, rather than just renaming stuff.

I'll create the commitfest entry based on this email once
this has been sent.

Patches attached.

The commitfest item now exists at

https://commitfest.postgresql.org/26/2371/

--
Mark Dilger