Horton

Thread

Commits

  1. Remove shadow variables linked to RedoRecPtr in xlog.c

  2. Fix calculation for WAL segment recycling and removal

  1. [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-04T01:24:28Z

    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?

Best regards,
Ranier Vilela

  2. Re: [Proposal] Level4 Warnings show many shadow vars

    Robert Haas <robertmhaas@gmail.com> — 2019-12-05T15:28:35Z

    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

  3. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-05T16:26:32Z

    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

  4. Re: [Proposal] Level4 Warnings show many shadow vars

    Robert Haas <robertmhaas@gmail.com> — 2019-12-05T18:41:20Z

    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

  5. Re: [Proposal] Level4 Warnings show many shadow vars

    Mark Dilger <hornschnorter@gmail.com> — 2019-12-05T21:06:38Z

    
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:

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

  6. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-05T23:41:24Z

    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

  7. Re: [Proposal] Level4 Warnings show many shadow vars

    Michael Paquier <michael@paquier.xyz> — 2019-12-06T01:11:37Z

    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

  8. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-06T12:59:15Z

    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

  9. Re: [Proposal] Level4 Warnings show many shadow vars

    Robert Haas <robertmhaas@gmail.com> — 2019-12-06T15:48:53Z

    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

  10. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-06T23:18:14Z

    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

  11. Re: [Proposal] Level4 Warnings show many shadow vars

    Mark Dilger <hornschnorter@gmail.com> — 2019-12-07T00:44:47Z

    
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

  12. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-07T23:42:38Z

    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

  13. Re: [Proposal] Level4 Warnings show many shadow vars

    Mark Dilger <hornschnorter@gmail.com> — 2019-12-08T18:25:21Z

    
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

  14. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-08T18:51:57Z

    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

  15. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-08T19:14:03Z

    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

  16. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-09T01:16:22Z

    >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

  17. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-09T01:30:33Z

    >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

  18. Re: [Proposal] Level4 Warnings show many shadow vars

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2019-12-09T03:40:58Z

    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

  19. Re: [Proposal] Level4 Warnings show many shadow vars

    Mark Dilger <hornschnorter@gmail.com> — 2019-12-09T04:50:41Z

    
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

  20. Re: [Proposal] Level4 Warnings show many shadow vars

    Mark Dilger <hornschnorter@gmail.com> — 2019-12-09T04:55:19Z

    
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

  21. Re: [Proposal] Level4 Warnings show many shadow vars

    Michael Paquier <michael@paquier.xyz> — 2019-12-09T09:03:29Z

    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

  22. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-09T11:02:27Z

    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 little as 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

  23. Re: [Proposal] Level4 Warnings show many shadow vars

    Daniel Gustafsson <daniel@yesql.se> — 2019-12-09T12:02:14Z

    > 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

  24. Re: [Proposal] Level4 Warnings show many shadow vars

    Robert Haas <robertmhaas@gmail.com> — 2019-12-09T14:33:15Z

    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

  25. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-09T15:23:08Z

    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

  26. Re: [Proposal] Level4 Warnings show many shadow vars

    Robert Haas <robertmhaas@gmail.com> — 2019-12-09T15:50:00Z

    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

  27. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-09T16:48:43Z

    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

  28. Re: [Proposal] Level4 Warnings show many shadow vars

    Robert Haas <robertmhaas@gmail.com> — 2019-12-09T16:59:23Z

    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

  29. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-09T17:26:33Z

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

regards
Ranier Vilela

  30. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-09T17:28:19Z

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

regards,
Ranier Vilela

  31. Re: [Proposal] Level4 Warnings show many shadow vars

    Mark Dilger <hornschnorter@gmail.com> — 2019-12-09T17:32:25Z

    
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

  32. Re: [Proposal] Level4 Warnings show many shadow vars

    Andres Freund <andres@anarazel.de> — 2019-12-09T21:32:17Z

    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

  33. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-09T21:38:04Z

    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

  34. Re: [Proposal] Level4 Warnings show many shadow vars

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-09T21:49:42Z

    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

  35. Re: [Proposal] Level4 Warnings show many shadow vars

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-09T22:06:28Z

    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

  36. Re: [Proposal] Level4 Warnings show many shadow vars

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-09T22:11:10Z

    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

  37. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-10T11:20:44Z

    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

  38. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-10T13:20:57Z

    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

  39. Re: [Proposal] Level4 Warnings show many shadow vars

    John W Higgins <wishdev@gmail.com> — 2019-12-10T15:58:41Z

    On Tue, Dec 10, 2019 at 5:21 AM Ranier Vilela <ranier_gyn@hotmail.com>
wrote:

> 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.
>
>
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 inconsistency as
well) - in some places names are changed to remove underscores (stop_t to
stopt). Some places random letters are added (checkPoint to xcheckPoint)
some places perfectly good names are truncated (conf_file to file).

Random places remove perfectly good prefixes and replace with single
letters (numTables to nTables)

Random places switch from lower case names to upper case names (sentPtr to
WalSentPtr) most places leave lower case names (days to ndays).

Please at least be consistent within the patch itself.....

John W Higgins

  40. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-10T17:13:47Z

    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 inconsistency as well) - >in some places names are changed to remove underscores (stop_t to stopt). >Some places random letters 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.
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

  41. Re: [Proposal] Level4 Warnings show many shadow vars

    Stephen Frost <sfrost@snowman.net> — 2019-12-10T17:52:57Z

    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 obvious inconsistency as well) - >in some places names are changed to remove underscores (stop_t to stopt). >Some places random letters 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

  42. RE: [Proposal] Level4 Warnings show many shadow vars

    Ranier VF <ranier_gyn@hotmail.com> — 2019-12-11T12:52:38Z

    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

  43. Re: [Proposal] Level4 Warnings show many shadow vars

    Stephen Frost <sfrost@snowman.net> — 2019-12-11T15:19:08Z

    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

  44. Re: [Proposal] Level4 Warnings show many shadow vars

    Michael Paquier <michael@paquier.xyz> — 2019-12-17T00:36:13Z

    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

  45. Re: [Proposal] Level4 Warnings show many shadow vars

    Michael Paquier <michael@paquier.xyz> — 2019-12-18T01:18:52Z

    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

  46. Re: [Proposal] Level4 Warnings show many shadow vars

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-12-18T09:55:16Z

    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

  47. Re: [Proposal] Level4 Warnings show many shadow vars

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-02-27T09:43:50Z

    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

  48. Re: [Proposal] Level4 Warnings show many shadow vars

    Michael Paquier <michael@paquier.xyz> — 2020-02-27T12:55:31Z

    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