Обсуждение: [Proposal] Level4 Warnings show many shadow vars

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
wasteof variable declaration. 
With the current Level3 level, development is no longer checking and correcting shadow variables.

Any comments?

Best regards,
Ranier Vilela

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
wasteof 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
wasteof 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:

https://www.postgresql.org/message-id/25938.1487367117%40sss.pgh.pa.us

and

https://www.postgresql.org/message-id/30007.1487374499%40sss.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:

https://www.postgresql.org/message-id/flat/CAEepm%3D15e9L695yVCO-_OkBVbsPupyXqzYWzzDmj-bdJ6o2%2BPw%40mail.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,
andthere 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
widespreaduse of the name "conn" in many places in the code, many in / bin, so if it is in the interest of postgres to
correctthis, 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
becorrected 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
thepatch would lead to a removal of the variable declaration, maintaining the same logic and functionality, which would
leadto 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
wouldbe 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,
wherethe patch would lead to a removal of the variable declaration, maintaining the same logic and functionality, which
wouldlead 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

On Sun, Dec 08, 2019 at 02:14:03PM -0500, Tom Lane wrote:
> 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.

Indeed.  That part was ill-thought.

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

Yeah, those APIs may have a slightly different meaning in the future,
so I agree that it makes the most sense to rename the variables of the
functions from RedoRecPtr to lastRedoPtr to outline the fact that we
are referring to the redo LSN of the last checkpoint.  Attached is a
patch for that.  I'd rather back-patch that to avoid any conflicts
when working on bug fixes for stable branches.  Thoughts?
--
Michael

De: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Enviado: segunda-feira, 9 de dezembro de 2019 03:40
>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.
Which harms the reusability of the code anyway.

>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.
Well i was trying to fix everything.

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

It's clear to me now that unless "the thing" is clearly a bug, don't touch it.
I love C, so for me it's very hard to resist getting stupid things like:
foo ()
{
int i, n;
for (i-0; i < n; i ++);
{
  int i;
  for (i=0; i < n; i ++);
}
{
  int i;
  for (i=0; i < n; i ++);
}
return;

I don't know how you can do it.

Of course, there are cases and cases, let's look at the example of multixact.c
diff --git a / src / backend / access / transam / multixact.c b / src / backend / access / transam / multixact.c
index 7b2448e05b..6364014fb3 100644
--- a / src / backend / access / transam / multixact.c
+++ b / src / backend / access / transam / multixact.c
@@ -1589.10 +1589.10 @@ mXactCachePut (MultiXactId multi, int nmembers, MultiXactMember * members)
 qsort (entry-> members, nmembers, sizeof (MultiXactMember), mxactMemberComparator);

 dlist_push_head (& MXactCache, & entry-> node);
+ pfree (entry); // <- is it really necessary?
 if (MXactCacheMembers ++> = MAX_CACHE_ENTRIES)
 {
 dlist_node * node;
- mXactCacheEnt * entry;

 node = dlist_tail_node (& MXactCache);
 dlist_delete (node);

I still can't decide if it's a bug or not.

If it is a bug the correct function here is pfree or what is the equivalent function to free memory?

>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.
I don't have enough information to decide if the lastest is the proper name, so I tried to change the nomenclature as
littleas possible. 

I'll submit a patch sample, which depending on the answer, will give me if it's worth it or not, keep working on it.

regards,
Ranier Vilela

> On 9 Dec 2019, at 12:02, Ranier Vilela <ranier_gyn@hotmail.com> wrote:

> diff --git a / src / backend / access / transam / multixact.c b / src / backend / access / transam / multixact.c
> index 7b2448e05b..6364014fb3 100644
> --- a / src / backend / access / transam / multixact.c
> +++ b / src / backend / access / transam / multixact.c
> @@ -1589.10 +1589.10 @@ mXactCachePut (MultiXactId multi, int nmembers, MultiXactMember * members)
> qsort (entry-> members, nmembers, sizeof (MultiXactMember), mxactMemberComparator);
>
> dlist_push_head (& MXactCache, & entry-> node);
> + pfree (entry); // <- is it really necessary?

Pushing an object to a dlist doesn't copy the object, so freeing entry here
would cause a dangling pointer on the list unless I'm misreading.  Note that
entry is allocated in a specific context to ensure it has the correct lifespan.
The README in backend/utils/mmgr is a good primer on how memory contexts work
in postgres.

As a matter of fact, the pfree call in the cache purge if block isn't really
required either since the entire cache will be freed at the end of the
transaction.

> if (MXactCacheMembers ++> = MAX_CACHE_ENTRIES)
> {
> dlist_node * node;
> - mXactCacheEnt * entry;

I can agree that reusing the name entry here isn't ideal, as it's so close, but
removing it is worse.  I'd prefer to rename it purged, or purged_entry or
something along those lines.

cheers ./daniel

On Sun, Dec 8, 2019 at 1:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

I don't know what to do about the RedoRecPtr mess, but surely
subscriptioncmds.c's use of synchronous_commit as a char * when it's
already exists as a global variable of type int is not good practice.
We've been known to do things like reference globals from within macro
definitions, and while static inlining is likely to make that practice
less common in the future, we've got plenty of existing instances,
including one that uses that exact variable name (SyncRepRequested()).

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Dec 8, 2019 at 1:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think I'm actually on board with the goal here.

> I don't know what to do about the RedoRecPtr mess, but surely
> subscriptioncmds.c's use of synchronous_commit as a char * when it's
> already exists as a global variable of type int is not good practice.

Well, again, this is a case-by-case question.  I tend to agree that
changing that usage in subscriptioncmds.c might be a good idea.
That doesn't mean I need to be on board with wholesale removal
of shadowing warnings.

            regards, tom lane

On Mon, Dec 9, 2019 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sun, Dec 8, 2019 at 1:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I don't think I'm actually on board with the goal here.
> > I don't know what to do about the RedoRecPtr mess, but surely
> > subscriptioncmds.c's use of synchronous_commit as a char * when it's
> > already exists as a global variable of type int is not good practice.
> Well, again, this is a case-by-case question.  I tend to agree that
> changing that usage in subscriptioncmds.c might be a good idea.
> That doesn't mean I need to be on board with wholesale removal
> of shadowing warnings.

I agree that those things are different, but I'm not sure I understand
the nuances of your view. I think my view is that if something in our
code is shadowing something else in our code, that's probably
something we ought to look at fixing. If you disagree, I'd be curious
to know why; I suspect that, as in this case, such cases are just
creating a risk of confusion without any real benefit. To me, the grey
area is in conflicts between stuff in our code and stuff in system
header files. I'm not sure I'd want to try to have precisely 0
conflicts with every crazy decision by every OS / libc maintainer out
there, and I suspect on that point at least we are in agreement.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 9, 2019 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, again, this is a case-by-case question.  I tend to agree that
>> changing that usage in subscriptioncmds.c might be a good idea.
>> That doesn't mean I need to be on board with wholesale removal
>> of shadowing warnings.

> I agree that those things are different, but I'm not sure I understand
> the nuances of your view. I think my view is that if something in our
> code is shadowing something else in our code, that's probably
> something we ought to look at fixing. If you disagree, I'd be curious
> to know why; I suspect that, as in this case, such cases are just
> creating a risk of confusion without any real benefit.

I think it depends a lot on the particular identifiers in use.  You
mentioned examples like "i" and "lc", and I'd add other obviously
nonce variable names like "oldcxt".  I'm not particularly concerned
about shadowing arising from somebody writing a five-line loop using
a local "i" inside a much larger loop also using "i" --- yeah, in
theory there could be an issue, but in practice there isn't.  Being
picky about that just adds difficulty when writing/reviewing a patch
that adds such a five-line loop.

Your point about risking macro breakage from shadowing of global
variable names is a good one, but again I don't think it holds up
as an argument that we have to get rid of all shadowing.

> To me, the grey
> area is in conflicts between stuff in our code and stuff in system
> header files. I'm not sure I'd want to try to have precisely 0
> conflicts with every crazy decision by every OS / libc maintainer out
> there, and I suspect on that point at least we are in agreement.

I believe the relevant C standards (and practice) are that random
names exposed by system headers ought to start with some underscores.
If there's a conflict there, it's a bug in the header and cause
for a bug report to the OS vendor, not us.

Now, if a conflict of that sort exists and is causing a live bug in PG
on a popular OS, then I'd likely be on board with adjusting our code
to dodge the problem.  But not with doing so just to silence a
compiler warning.

A final point here is that in practice, we've had way more problems
with conflicts against system headers' definitions of functions,
macros, and typedefs than global variables, which is unsurprising
considering how few of the latter are actually exported by typical
C library APIs.  So I'm not sure that there is any big problem to
be solved there in the first place.

The only thing I think is really a substantial bug risk here is your
point about our own macros referencing our own global variables.
We might be better off fixing that in a localized way by establishing
a policy that any such macros should be converted to static inlines.

            regards, tom lane

On Mon, Dec 9, 2019 at 11:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think it depends a lot on the particular identifiers in use.  You
> mentioned examples like "i" and "lc", and I'd add other obviously
> nonce variable names like "oldcxt". I'm not particularly concerned
> about shadowing arising from somebody writing a five-line loop using
> a local "i" inside a much larger loop also using "i" --- yeah, in
> theory there could be an issue, but in practice there isn't.  Being
> picky about that just adds difficulty when writing/reviewing a patch
> that adds such a five-line loop.

I think I would take the contrary view here. I think reusing the same
variable names in a single function is confusing, and if I noticed it
while reviewing, I would ask for it to be changed. It's not a
five-alarm fire, but it's not good, either.

> > To me, the grey
> > area is in conflicts between stuff in our code and stuff in system
> > header files. I'm not sure I'd want to try to have precisely 0
> > conflicts with every crazy decision by every OS / libc maintainer out
> > there, and I suspect on that point at least we are in agreement.
>
> I believe the relevant C standards (and practice) are that random
> names exposed by system headers ought to start with some underscores.
> If there's a conflict there, it's a bug in the header and cause
> for a bug report to the OS vendor, not us.

Sure. I mean we'd have to look at individual cases, but in general I agree.

> Now, if a conflict of that sort exists and is causing a live bug in PG
> on a popular OS, then I'd likely be on board with adjusting our code
> to dodge the problem.  But not with doing so just to silence a
> compiler warning.

Sounds reasonable.

> A final point here is that in practice, we've had way more problems
> with conflicts against system headers' definitions of functions,
> macros, and typedefs than global variables, which is unsurprising
> considering how few of the latter are actually exported by typical
> C library APIs.  So I'm not sure that there is any big problem to
> be solved there in the first place.
>
> The only thing I think is really a substantial bug risk here is your
> point about our own macros referencing our own global variables.
> We might be better off fixing that in a localized way by establishing
> a policy that any such macros should be converted to static inlines.

That would be a lot of work, but it would probably have some side
benefits, like making things more type-safe.

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

This the second version of the global unshadow patch.
Taking into consideration made. In case anyone else revises.

regards
Ranier Vilela

>I'll create the commitfest entry based on this email once
>this has been sent.
Can you add the patch attached?

regards,
Ranier Vilela

On 12/9/19 9:28 AM, Ranier Vilela wrote:
>> I'll create the commitfest entry based on this email once
>> this has been sent.
> Can you add the patch attached?

That showed up in the commitfest entry automatically when you
replied to this thread with the attachment.

You might consider signing up so you can log into the commitfest
app.  https://www.postgresql.org/account/signup/

-- 
Mark Dilger

Hi,

On 2019-12-09 11:59:23 -0500, Robert Haas wrote:
> On Mon, Dec 9, 2019 at 11:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I think it depends a lot on the particular identifiers in use.  You
> > mentioned examples like "i" and "lc", and I'd add other obviously
> > nonce variable names like "oldcxt". I'm not particularly concerned
> > about shadowing arising from somebody writing a five-line loop using
> > a local "i" inside a much larger loop also using "i" --- yeah, in
> > theory there could be an issue, but in practice there isn't.  Being
> > picky about that just adds difficulty when writing/reviewing a patch
> > that adds such a five-line loop.
> 
> I think I would take the contrary view here. I think reusing the same
> variable names in a single function is confusing, and if I noticed it
> while reviewing, I would ask for it to be changed. It's not a
> five-alarm fire, but it's not good, either.

+1. For me it leaves mildly bad taste seing code like that.


> > > To me, the grey
> > > area is in conflicts between stuff in our code and stuff in system
> > > header files. I'm not sure I'd want to try to have precisely 0
> > > conflicts with every crazy decision by every OS / libc maintainer out
> > > there, and I suspect on that point at least we are in agreement.
> >
> > I believe the relevant C standards (and practice) are that random
> > names exposed by system headers ought to start with some underscores.
> > If there's a conflict there, it's a bug in the header and cause
> > for a bug report to the OS vendor, not us.
> 
> Sure. I mean we'd have to look at individual cases, but in general I agree.

We do have a few files where we have names starting with underscores
ourselves, imo not a great idea for most of them.


> > Now, if a conflict of that sort exists and is causing a live bug in PG
> > on a popular OS, then I'd likely be on board with adjusting our code
> > to dodge the problem.  But not with doing so just to silence a
> > compiler warning.
> 
> Sounds reasonable.

FWIW, I've had bugs in code submitted to PG (both by myself and me
merging other people's work IIRC) that were related to such naming
conflicts.


> > The only thing I think is really a substantial bug risk here is your
> > point about our own macros referencing our own global variables.
> > We might be better off fixing that in a localized way by establishing
> > a policy that any such macros should be converted to static inlines.
> 
> That would be a lot of work, but it would probably have some side
> benefits, like making things more type-safe.

It's also not always possible in C99, as we have plenty macros with
essentially dynamic types. And there's no typeof() in standard C,
unfortunately (C11's _Generic can help, but isn't great either).

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-12-09 11:59:23 -0500, Robert Haas wrote:
>> On Mon, Dec 9, 2019 at 11:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> The only thing I think is really a substantial bug risk here is your
>>> point about our own macros referencing our own global variables.
>>> We might be better off fixing that in a localized way by establishing
>>> a policy that any such macros should be converted to static inlines.

>> That would be a lot of work, but it would probably have some side
>> benefits, like making things more type-safe.

> It's also not always possible in C99, as we have plenty macros with
> essentially dynamic types. And there's no typeof() in standard C,
> unfortunately (C11's _Generic can help, but isn't great either).

How much overlap is there between macros referencing global variables
and macros with indeterminate types?  Not much I bet.  I'd mostly
be worried about things like CHECK_FOR_INTERRUPTS().

            regards, tom lane

On 2019-Dec-09, Kyotaro Horiguchi wrote:


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

We have a not-consistently-used convention that names in CamelCase are
used for global variables.  Naming a function parameter in that style
seems pointlessly confusing.  I would rather use redorecptr as Tom
suggested, which fits with the style used for the other arguments of
that function.  BTW prepending an X or a p looks like minimum effort...
I'd stay away from that.

It's probably a lost cause to enforce such a style with ironclad
consistency, but I suggest we make at least a token effort at it, and
keep our source as least confusing as possible.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Dec-09, Ranier Vilela wrote:

> --- a/src/backend/access/transam/xlogreader.c
> +++ b/src/backend/access/transam/xlogreader.c
> @@ -70,7 +70,7 @@ report_invalid_record(XLogReaderState *state, const char *fmt,...)
>   * Returns NULL if the xlogreader couldn't be allocated.
>   */
>  XLogReaderState *
> -XLogReaderAllocate(int wal_segment_size, const char *waldir,
> +XLogReaderAllocate(int wallog_segment_size, const char *waldir,
>                     XLogPageReadCB pagereadfunc, void *private_data)
>  {
>      XLogReaderState *state;

I find this choice a bit ugly and even more confusing than the original.
I'd change this to be just "segsize".

I would tend to name the GUC variable as if it were a global in the
sense that I proposed in my previous response (ie. WalSegmentSize), but
that creates a bit of a problem when one greps the source looking for
reference to the GUCs.  Some GUCs do have CamelCase names and others
don't; I've grown fond of the current style of using the same name for
the variable as for the GUC itself, for grepping reasons.  So I'm not
going to propose to do that.  But let's not make a function parameter
have a name that vaguely suggests that it itself is a GUC.

> @@ -430,14 +430,14 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
>      {
>          XLogRecPtr    lsn;
>          char       *err;
> -        WalReceiverConn *wrconn;
> +        WalReceiverConn *walrconn;
>          List       *tables;
>          ListCell   *lc;
>          char        table_state;
>  
>          /* Try to connect to the publisher. */
> -        wrconn = walrcv_connect(conninfo, true, stmt->subname, &err);
> -        if (!wrconn)
> +        walrconn = walrcv_connect(conninfo, true, stmt->subname, &err);
> +        if (!walrconn)
>              ereport(ERROR,
>                      (errmsg("could not connect to the publisher: %s", err)));
>  

Here I propose to rename the global instead (to WalReceiverConn maybe).
There's nothing about the name "wrconn" that suggests it's a global
variable.  In any other place where the object is used as a local
variable, I'd just use "conn".  Trying to be clever and adding a letter
here or a letter there makes it *more* likely that you'll reference the
wrong one in some function.

> index a9edbfd4a4..1f5921b6e7 100644
> --- a/src/backend/main/main.c
> +++ b/src/backend/main/main.c
> @@ -225,7 +225,7 @@ main(int argc, char *argv[])
>   * without help.  Avoid adding more here, if you can.
>   */
>  static void
> -startup_hacks(const char *progname)
> +startup_hacks(const char *prog_name)
>  {
>      /*
>       * Windows-specific execution environment hacking.

I don't agree with this change very much.  I think "progname" in
particular is a bit of a debacle right now but I don't think this is the
best fix.  I'd leave this alone.

> diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
> index 8b2a2be1c0..e12f41cea4 100644
> --- a/src/backend/replication/walsender.c
> +++ b/src/backend/replication/walsender.c
> @@ -3223,7 +3223,7 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
>      for (i = 0; i < max_wal_senders; i++)
>      {
>          WalSnd       *walsnd = &WalSndCtl->walsnds[i];
> -        XLogRecPtr    sentPtr;
> +        XLogRecPtr    walsentPtr;
>          XLogRecPtr    write;
>          XLogRecPtr    flush;
>          XLogRecPtr    apply;

As before: let's rename the file-level static instead.  "sentPtr" is not
a good name.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> We have a not-consistently-used convention that names in CamelCase are
> used for global variables.  Naming a function parameter in that style
> seems pointlessly confusing.  I would rather use redorecptr as Tom
> suggested, which fits with the style used for the other arguments of
> that function.  BTW prepending an X or a p looks like minimum effort...
> I'd stay away from that.

Actually, for the particular case of RemoveXlogFile(s), I wonder if it
shouldn't be "startptr" to go with the other argument "endptr".  This line
of thinking might not lead to nicer names in other functions, of course.
But we shouldn't assume that a one-size-fits-all solution is going to
improve legibility, and in the end, legibility is what this should be
about.

            regards, tom lane

De: Alvaro Herrera <alvherre@2ndquadrant.com>
Enviado: segunda-feira, 9 de dezembro de 2019 22:06

>I find this choice a bit ugly and even more confusing than the original.
>I'd change this to be just "segsize".
Ok.

>I would tend to name the GUC variable as if it were a global in the
>sense that I proposed in my previous response (ie. WalSegmentSize), but
>that creates a bit of a problem when one greps the source looking for
>reference to the GUCs.  Some GUCs do have CamelCase names and others
>don't; I've grown fond of the current style of using the same name for
>the variable as for the GUC itself, for grepping reasons.  So I'm not
>going to propose to do that.  But let's not make a function parameter
>have a name that vaguely suggests that it itself is a GUC.
I understand the ease of grepping.
But ideally, having global names that by convention would make it very clear that they are global, something like:
pg_conn or gconn or guc_synch_commit
The prefix does not matter, as long as once all the new variables and the ones that might have to be changed were
chosen,they adopted the agreed nomenclature. 
That way, when looking for global names, it would be easy.

>Here I propose to rename the global instead (to WalReceiverConn maybe).
>There's nothing about the name "wrconn" that suggests it's a global
>variable.  In any other place where the object is used as a local
>variable, I'd just use "conn".  Trying to be clever and adding a letter
>here or a letter there makes it *more* likely that you'll reference the
>wrong one in some function.
Again, it could be that name, WalReceiverConn, but nothing in it suggests it is a global one.
For a project that makes extensive use of globals, it would help to have a nomenclature defined at least for the
prefix:
pg_WalReceiverConn or gWalReceiverConn and if it is a guc, guc_WalReceiverConn?

>I don't agree with this change very much.  I think "progname" in
>particular is a bit of a debacle right now but I don't think this is the
>best fix.  I'd leave this alone.
Ok. In such cases, it doesn't hurt today. But for future reasons, it would be better to fix everything, imo.

>As before: let's rename the file-level static instead.  "sentPtr" is not
>a good name.
gsent_Ptr or pg_sentPtr?

regards,
Ranier Vilela

New version the global patch, with the considerations.
Unfortunately WalReceiverConn cannot be used because it is currently the typedef name for the structure.
I switched to WalReceiverConnection, it was long but it looks good.
RedoRecPtr proper name has no consensus yet, so it was still lastRedoRecPtr.

regards,
Ranier Vilela

De: John W Higgins <wishdev@gmail.com>
Enviado: terça-feira, 10 de dezembro de 2019 15:58

>For someone that expounds consistency - this patch is the furthest thing from it.
>In some places names are randomly changed to have an underscore >(authmethodlocal to authmethod_local with the obvious
inconsistencyas well) - >in some places names are changed to remove underscores (stop_t to stopt). >Some places random
lettersare added (checkPoint to xcheckPoint) some places >perfectly good names are truncated (conf_file to file). 
The first purpose of the patch was to remove collisions from shadow global variable names.
The second was not to change the semantics of variable names, hence the use of x or putting or remove underscore.
But I agree with you that the choice of names can improve.
xcheckpoint sounds ugly.
stopt sounds ugly too.

>Random places remove perfectly good prefixes and replace with single letters >(numTables to nTables)
numTables already a global variable name.
nTables It seems very reasonable to me to contain the number of tables.

>Random places switch from lower case names to upper case names (sentPtr to >WalSentPtr) most places leave lower case
names(days to ndays). 
again senPtr already a global variable name.
Well, I tried to follow the local source style a little, since the project does not have a default for global names.
There we have some WalSntCtl por example.

ndays sounds very good to me for number of days.

>Please at least be consistent within the patch itself.....
I'm trying.

regards,
Ranier Vilela

Greetings,

* Ranier Vilela (ranier_gyn@hotmail.com) wrote:
> >For someone that expounds consistency - this patch is the furthest thing from it.
> >In some places names are randomly changed to have an underscore >(authmethodlocal to authmethod_local with the
obviousinconsistency as well) - >in some places names are changed to remove underscores (stop_t to stopt). >Some places
randomletters are added (checkPoint to xcheckPoint) some places >perfectly good names are truncated (conf_file to
file).
> The first purpose of the patch was to remove collisions from shadow global variable names.

There's multiple ways to get there though and I think what you're seeing
is that the "just change it to something else" answer isn't necessairly
going to be viewed as an improvement (or, at least, not enough of an
improvement to accept the cost of the change).

> The second was not to change the semantics of variable names, hence the use of x or putting or remove underscore.

Why not change the variables?  Changes that also improve the code itself
along with eliminating the shadowing of the global variable are going to
be a lot easier to be accepted.

> >Random places remove perfectly good prefixes and replace with single letters >(numTables to nTables)
> numTables already a global variable name.

Sure, but have you looked at how it's used?  Instead of just renaming
the numTables variables in the functions that accept it- could those
variables just be removed instead of changing their name to make it look
like they're something different when they aren't actually different?

I've only spent a bit of time looking at it, but it sure looks like the
variables could just be removed, and doing so doesn't break the
regression tests, which supports the idea that maybe there's a better
way to deal with those particular variables rather than renaming them.

Another approach to consider might be to move some global variables into
structures that are then global with better names to indicate that's
what they are.

In short, a hack-and-slash patch that doesn't really spend much time
considering the changes beyond "let's just change these to be different
to avoid shadowing globals" isn't really a good way to go about
addressing these cases and has a good chance of making things more
confusing, not less.

Thanks,

Stephen

New version the global patch unshadow.
* names more consistent and readable.
* without big changes.
* goal,, unshadow all global variables, only, even the simplest.

regards,
Ranier Vilela

Greetings,

* Ranier Vilela (ranier_gyn@hotmail.com) wrote:
> New version the global patch unshadow.
> * names more consistent and readable.
> * without big changes.
> * goal,, unshadow all global variables, only, even the simplest.

This didn't address any of the comments that I raised elsewhere on this
thread...  I certainly don't like the changes being proposed here for
pg_dump.

Thanks,

Stephen

Greetings,

Didn't see this previously (it's our typical approach to 'reply-all' to
people), though I don't think it changes my feelings about the latest
proposed patch.

* Ranier Vilela (ranier_gyn@hotmail.com) wrote:
> De: Stephen Frost
> Enviadas: Terça-feira, 10 de Dezembro de 2019 17:52
>
> >There's multiple ways to get there though and I think what you're seeing
> >is that the "just change it to something else" answer isn't necessairly
> >going to be viewed as an improvement (or, at least, not enough of an
> >improvement to accept the cost of the change).
> Well, I was trying to apply another non-implied rule, "break nothing".

I agree with not breaking things but that doesn't mean the only
reasonable approach is to do the absolute minimum- you might not be
breaking something today, but it's going to confuse people later on down
the road and may lead to bugs being introduced due to that confusion, or
at the very least will add to people's time to figure out what's really
going on.

> >Why not change the variables?  Changes that also improve the code itself
> >along with eliminating the shadowing of the global variable are going to
> >be a lot easier to be accepted.
> Contrary to what I was initially thinking, it seems to me that changing the names of global variables is more
acceptableto the people of the project. 

I wasn't suggesting to change the names of the global variables in this
specific case, though I could see that being a better approach in some
instances- but it really depends.  Each case needs to be reviewed and
considered and the best approach taken.

> >Sure, but have you looked at how it's used?  Instead of just renaming
> >the numTables variables in the functions that accept it- could those
> >variables just be removed instead of changing their name to make it look
> >like they're something different when they aren't actually different?
> No. I didn't look.

I think we need to be looking at the changes and considering them, and
the person proposing the changes should be doing that and not just
expecting everyone else to do so.

> >I've only spent a bit of time looking at it, but it sure looks like the
> >variables could just be removed, and doing so doesn't break the
> >regression tests, which supports the idea that maybe there's a better
> >way to deal with those particular variables rather than renaming them.
> >Another approach to consider might be to move some global variables into
> >structures that are then global with better names to indicate that's
> >what they are.
> It does not seem reasonable to me what you are asking.
> Because as I was told here and I agree in part. I do not have the necessary knowledge of structures and logic to
proposebig changes. 

I'd suggest that we work through that then and get you up to speed on
the structures and logic- the pg_dump code is pretty ugly but the
specific usage of numTables isn't too bad.  Each of these should be
looked at independently and thought about "what's the right way to fix
this?"  The right way isn't necessairly to just rename the variables, as
I was saying, and doing so may lead to more confusion, not less.

> For the work I set out to, find bugs and make minor performance improvements, I believe, can contribute safely and
withoutruining anything. 

Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
a bug.  I'm not sure what "minor performance improvements" are being
claimed here but there's a whole lot of work involved in demonstrating
that a change is a performance improvement.

> By just changing the names of variables to something consistent and readable, the goal will be done without break
anything.

but.. the changes you're proposing are making them inconsistent and
confusing when there isn't actually a difference between the global and
the local, it's just the somewhere along the way someone thought they
needed to pass in numTables when they really didn't, and we should go
fix *that*, not rename the variable to something else to make someone
later on go "wait, why did we need to pass in this variable?  how is
this different from the global?"

> Who is best to make these changes, are the authors and reviewers. Once we no longer have the problem of shadow
variables,we can turn on the alert without breaking automatic compilation, as Tom Lane is concerned. 

Perhaps I'm a bit confused, but it seems that you're the author of these
specific changes, and I'm trying to provide feedback as to how you can
improve what you're proposing in a way that will improve the code base
overall and reduce the confusion while also eliminating the shadow
variables.  If the author of a patch isn't open to this kind of review
and willing to adjust the patch to improve it because they aren't sure
that their changes will be correct then they could at least post them
back here and ask, or better, go look at the code and get a better
understanding of what's going on to build confidence in the change.

The goal here also shouldn't be "we just want to turn on this alert, so
we're going to make changes to the source without thinking just to
appease the compiler".

> That's why I'd like to fix all collisions of variables, even the dumbest.

I agree with fixing collisions, but not in a rote way like this.

> >In short, a hack-and-slash patch that doesn't really spend much time
> >considering the changes beyond "let's just change these to be different
> >to avoid shadowing globals" isn't really a good way to go about
> >addressing these cases and has a good chance of making things more
> >confusing, not less.
> This is totally contrary to what I think about it.

-1 from me then on this whole thread of changes.

Thanks,

Stephen

Greetings,

* Ranier Vilela (ranier_gyn@hotmail.com) wrote:
> De: Stephen Frost
> Enviadas: Quarta-feira, 11 de Dezembro de 2019 15:34
>
> >I agree with not breaking things but that doesn't mean the only
> >reasonable approach is to do the absolute minimum- you might not be
> >breaking something today, but it's going to confuse people later on down
> >the road and may lead to bugs being introduced due to that confusion, or
> >at the very least will add to people's time to figure out what's really
> >going on.
> I don't know how such fixes could lead to more bugs.
> Currently there is a risk of having bugs by mixing access to shadow variables with macros.

I really don't have any doubts that it's going to lead to confusion,
particularly in a case like the numTables vs. nTables thing you're
proposing in the one case that I spent some time looking at, and that
confusion certainly could lead to bugs.  Sure, having shadow variables
also could- but you haven't identified an actual bug there today, so why
not just fix it in a way that eliminates the confusion here?

Here's an example of my concern- we change the name of the numTables
variable in these pg_dump functions to nTables as you propose...  And
then later on someone starts hacking on these functions and they know
about the global and they start using it, so now we've got two
variables, both able to be used in the same scope, but one of them is a
global and the other is a local.  As long as both are always the same,
sure everything works- but what happens if, for whatever reason, someone
uses the function in a new way and passes in a different value as an
argument, one that doesn't match what the global has?  Well, some of the
code will use the argument, and some of the code won't.  At least today,
there's no chance that the global variable will be used inside that
function- it's *always* going to use the argument passed in.

I don't think that's even that far-fetched of a possibility considering
most of the code is using the global variable directly and these
functions are really the odd ones where numTables is being passed in as
an argument, so ending up with a mix in the function looks rather likely
to happen, and a bug resulting from that inconsistency entirely
possible.

It's also possbile that the changes you're proposing might themselves
induce bugs- by keeping the variable and just renaming it, you had
better be ABSOLUTELY sure you rename every case because, if you don't,
everything will still work just *fine*, except where you missed a case,
the code will reference the global and the compiler won't complain and
it might very well look like everything is working.

Either way, in my view, you had better review the code, have an
understanding of how it works, and make sure that the change you're
making is correct and makes sense, and that you've tested it well.

> >I wasn't suggesting to change the names of the global variables in this
> >specific case, though I could see that being a better approach in some
> >instances- but it really depends.  Each case needs to be reviewed and
> >considered and the best approach taken.
> Again, depending on the case, whether the best approach is to promote structure creation, variable removal, and logic
changes,for now, is really beyond my reach. 

Then I'd suggest that you spend some time looking at each case and
working through what the best approach is and proposing patches that use
the best approach in each case.  If you don't wish to spend time on
that, that's fine, but I don't agree with this approach of just pushing
through and making things changes just to satisfy a particular compiler
warning.  I don't anticipate further discussion on this changing my mind
on this point.

> >I think we need to be looking at the changes and considering them, and
> >the person proposing the changes should be doing that and not just
> >expecting everyone else to do so.
> Again, I am considering only the range of my changes, which are minimal, so less likely to do something wrong, or
hinderfuture development. 

I've already pointed out why I don't think this is the right approach to
be addressing these issues, and it seems that you don't disagree with me
about the recommended changes I've suggested, you've just said that you
only want to think about or care about renaming of variables and I am
specifically saying that's not an acceptable approach to addressing
these issues.

> >I'd suggest that we work through that then and get you up to speed on
> >the structures and logic- the pg_dump code is pretty ugly but the
> >specific usage of numTables isn't too bad.  Each of these should be
> >looked at independently and thought about "what's the right way to fix
> >this?"  The right way isn't necessairly to just rename the variables, as
> >I was saying, and doing so may lead to more confusion, not less.
> This way it will take a long time to eliminate all name collisions.

Why is that an issue?

> And worse, in my opinion, will continue to be adding new cases, since there is no rule, so check if this happens in
thecurrent development. 

Feel free to monitor the situation and complain about new patches which
are proposed that add to them.  I don't have any particular problem with
that.  Nor do I object to generally pushing forward with the goal of
eliminating the existing ones.

Let me lay this out in a different way- we could do the exact same thing
you're doing here by just mindlessly changing, right before we commit,
any variables that shadow global variables, we'd eliminate the compiler
error, but it doesn't directly make anything *better* by itself, and
ultimately isn't really all that different from the current situation
where the compiler is essentially doing this for us by manging the
variables as shadowing the globals thanks to C scoping rules, except
that we add in the possibility of mixing usage of the local and the
global throughout the functions therefore adding to the confusion.

> Not only are they global, there are dozens, perhaps hundreds of shadow local variables.

That doesn't actually make any of them bugs though.

> I was working on this second class of variables, which, in my opinion, would lead to less code, less bugs, and more
securityfor the code, but I realize that my effort may not be worth it. 

I'm all for working to eliminate these shadow variables, but, again, not
through rote renaming of the locals without putting in any real thought
about what the code is doing and working out what the right approach to
such a change to eliminate the shadow variables should be.

> >Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
> >a bug.  I'm not sure what "minor performance improvements" are being
> >claimed here but there's a whole lot of work involved in demonstrating
> >that a change is a performance improvement.
> I was referring to contributions like this:
> https://github.com/postgres/postgres/commit/91da65f4ac2837e0792071e42b2e2101059f1b1b
> and not specifically, performance improvements in this global unshadow patch.

Ok, so this specific "global unshadow patch" is all about bugs, but
without actually showing that there's actual bugs here, just that there
are shadowed variables... In which case, the real question is "is this
change an improvement to the code" and I'm arguing that just the act of
changing the variable names to avoid shadowing isn't necessairly a code
improvement- that has to be evaluated on a case-by-case basis.  If
you're not going to do that evaluation then you're just throwing changes
at the community with the expectation that someone else will do the
analysis and decide if the changes are worthwhile or not and that
strikes me as not really being very helpful.  I'd really rather work
towards patches that are clear improvements which have been well
considered by the proposer of the patch.

Thanks,

Stephen

Greetings,

* Ranier Vilela (ranier_gyn@hotmail.com) wrote:
> 1.So I would like to ask you if at least what has consensus could be used.
> Or is it better to leave everything as it is?

As I tried to say before- I'm all for working to eliminate the shadow
variables, but it should be on a case-by-case basis where the change
proposed is a well considered change by someone who has taken the time
to understand what the code is doing, looked at the call sites, made a
well reasoned argument for why the change is an improvement and reduces
confusion (which can't just be "because the compiler said we should make
this change"- compilers aren't nearly intelligent enough to give us the
right answer about what the code should look like- they can only point
out potential issues, and they're often bad at even doing that), and
then proposed a patch for each particular case where the patch is
addressing a specific set of shadow variable cases that somehow go
together.

A patch to address the numTables issues in pg_dump would be great, for
example.  A single patch that renames numTables in pg_dump and then
makes a bunch of completely unrelated changes to things in the backend
isn't what I'd consider a reasonable grouping of changes.

> 2.About local shadow variables, would you find it safe to do redundant declaration removals of the type: int i? Is it
worthit to work on that?
 

I really don't think you're going to find much in the PG code where
there would be general consensus of a broad renaming or modifying of
variables without having to put serious thought into the specific
change.

At least, I hope people would push back on that kind of rote change.

In other words, without looking at the specific cases you're talking
about, I don't know if I'd agree with them or not, but please don't just
submit patches that just rename things without having looked at the
code, gained some understanding of what the code does, and considered if
the change you want to make is a well reasoned improvement and makes the
code easier to read and understand.

Thanks,

Stephen

On Mon, Dec 09, 2019 at 05:11:10PM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> We have a not-consistently-used convention that names in CamelCase are
>> used for global variables.  Naming a function parameter in that style
>> seems pointlessly confusing.  I would rather use redorecptr as Tom
>> suggested, which fits with the style used for the other arguments of
>> that function.  BTW prepending an X or a p looks like minimum effort...
>> I'd stay away from that.
>
> Actually, for the particular case of RemoveXlogFile(s), I wonder if it
> shouldn't be "startptr" to go with the other argument "endptr".  This line
> of thinking might not lead to nicer names in other functions, of course.
> But we shouldn't assume that a one-size-fits-all solution is going to
> improve legibility, and in the end, legibility is what this should be
> about.

Hmm.  In the case of this logic, we are referring to the current end
of WAL with endptr, and what you are calling the startptr is really
the redo LSN of the last checkpoint in all the routines which are now
confused with RedoRecPtr: RemoveOldXlogFile, RemoveXlogFile and
XLOGfileslop.  Using lower-case for all the characters of the variable
name sounds like a good improvement as well, so taking a combination
of all that I would just use "lastredoptr" in those three code paths
(note that we used to have PriorRedoPtr before).  As that's a
confusion I introduced with d9fadbf, I would like to fix that and
backpatch this change down to 11.  (Ranier gets the authorship
per se as that's extracted from a larger patch).
--
Michael

On Tue, Dec 17, 2019 at 09:36:13AM +0900, Michael Paquier wrote:
> As that's a confusion I introduced with d9fadbf, I would like to fix
> that and backpatch this change down to 11.  (Ranier gets the
> authorship per se as that's extracted from a larger patch).

Committed that part.

I got to look at the rest of the stuff discussed, and I am not sure
that any of the changes are actually things which improve
readability.

Let's take one example.  The changes in pg_dump/ like
/progname/prog_name/ have just been done in haste, without actual
thoughts about how the problem ought to be fixed.  And in this case,
something which could be more adapted is to remove the argument from
usage() because progname is a global variable, initialized from the
beginning in pg_restore.
--
Michael

On 2019-Dec-18, Michael Paquier wrote:

> Let's take one example.  The changes in pg_dump/ like
> /progname/prog_name/ have just been done in haste, without actual
> thoughts about how the problem ought to be fixed.  And in this case,
> something which could be more adapted is to remove the argument from
> usage() because progname is a global variable, initialized from the
> beginning in pg_restore.

We discussed progname as a global/local before -- IIRC in the thread
that introduced the frontend logging API -- and while I think the whole
issue could stand some improvement, we shouldn't let it be driven by
minor changes; that'll only make it more confusing.  IMO if we want it
improved, a larger change (involving the bunch of frontend programs) is
what to look for.  Maybe what you suggest is an improvement, though
(certainly the "prog_name" patch wasn't).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-12-18 10:55, Alvaro Herrera wrote:
> On 2019-Dec-18, Michael Paquier wrote:
> 
>> Let's take one example.  The changes in pg_dump/ like
>> /progname/prog_name/ have just been done in haste, without actual
>> thoughts about how the problem ought to be fixed.  And in this case,
>> something which could be more adapted is to remove the argument from
>> usage() because progname is a global variable, initialized from the
>> beginning in pg_restore.
> 
> We discussed progname as a global/local before -- IIRC in the thread
> that introduced the frontend logging API -- and while I think the whole
> issue could stand some improvement, we shouldn't let it be driven by
> minor changes; that'll only make it more confusing.  IMO if we want it
> improved, a larger change (involving the bunch of frontend programs) is
> what to look for.  Maybe what you suggest is an improvement, though
> (certainly the "prog_name" patch wasn't).

This thread is still in the commit fest, but it's apparently gone as far 
as it will, so I've set it to "Committed" for lack of a "partial" status.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Feb 27, 2020 at 10:43:50AM +0100, Peter Eisentraut wrote:
> This thread is still in the commit fest, but it's apparently gone as far as
> it will, so I've set it to "Committed" for lack of a "partial" status.

Thanks, that sounds right to me.  I was just looking at the latest
patch presented after seeing your reply, and I did not spot
immediately any issues standing out compared to the others.
--
Michael