Re: Add "-Wimplicit-fallthrough" to default flags (was Re: pgsql: Support FETCH FIRST WITH TIES)

Support FETCH FIRST WITH TIES

WITH TIES is an option to the FETCH FIRST N ROWS clause (the SQL
standard's spelling of LIMIT), where you additionally get rows that
compare equal to the last of those N rows by the columns in the
mandatory ORDER BY clause.

There was a proposal by Andrew Gierth to implement this functionality in
a more powerful way that would yield more features, but the other patch
had not been finished at this time, so we decided to use this one for
now in the spirit of incremental development.

Author: Surafel Temesgen <surafel3000(at)gmail(dot)com>
Reviewed-by: Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Reviewed-by: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Discussion: https://postgr.es/m/CALAY4q9ky7rD_A4vf=FVQvCGngm3LOes-ky0J6euMrg=_Se+ag@mail.gmail.com
Discussion: https://postgr.es/m/87o8wvz253.fsf@news-spur.riddles.org.uk

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/357889eb17bb9c9336c4f324ceb1651da616fe57

Modified Files
--------------
doc/src/sgml/ref/select.sgml | 15 +--
src/backend/catalog/sql_features.txt | 2 +-
src/backend/executor/nodeLimit.c | 160 +++++++++++++++++++++++++++---
src/backend/nodes/copyfuncs.c | 7 ++
src/backend/nodes/equalfuncs.c | 2 +
src/backend/nodes/outfuncs.c | 7 ++
src/backend/nodes/readfuncs.c | 6 ++
src/backend/optimizer/plan/createplan.c | 45 ++++++++-
src/backend/optimizer/plan/planner.c | 1 +
src/backend/optimizer/util/pathnode.c | 2 +
src/backend/parser/analyze.c | 21 ++--
src/backend/parser/gram.y | 117 +++++++++++++++++-----
src/backend/parser/parse_clause.c | 15 ++-
src/backend/utils/adt/ruleutils.c | 27 ++++--
src/include/catalog/catversion.h | 2 +-
src/include/nodes/execnodes.h | 5 +
src/include/nodes/nodes.h | 13 +++
src/include/nodes/parsenodes.h | 2 +
src/include/nodes/pathnodes.h | 1 +
src/include/nodes/plannodes.h | 5 +
src/include/optimizer/pathnode.h | 1 +
src/include/optimizer/planmain.h | 5 +-
src/include/parser/parse_clause.h | 3 +-
src/test/regress/expected/limit.out | 167 ++++++++++++++++++++++++++++++++
src/test/regress/sql/limit.sql | 48 +++++++++
25 files changed, 610 insertions(+), 69 deletions(-)

Hi Álvaro,

On Tue, Apr 7, 2020 at 10:28 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Support FETCH FIRST WITH TIES
>
> WITH TIES is an option to the FETCH FIRST N ROWS clause (the SQL
> standard's spelling of LIMIT), where you additionally get rows that
> compare equal to the last of those N rows by the columns in the
> mandatory ORDER BY clause.
>
> There was a proposal by Andrew Gierth to implement this functionality in
> a more powerful way that would yield more features, but the other patch
> had not been finished at this time, so we decided to use this one for
> now in the spirit of incremental development.
>
> Author: Surafel Temesgen <surafel3000(at)gmail(dot)com>
> Reviewed-by: Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> Reviewed-by: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
> Discussion: https://postgr.es/m/CALAY4q9ky7rD_A4vf=FVQvCGngm3LOes-ky0J6euMrg=_Se+ag@mail.gmail.com
> Discussion: https://postgr.es/m/87o8wvz253.fsf@news-spur.riddles.org.uk
>
> Branch
> ------
> master
>
> Details
> -------
> https://git.postgresql.org/pg/commitdiff/357889eb17bb9c9336c4f324ceb1651da616fe57
>
> Modified Files
> --------------
> doc/src/sgml/ref/select.sgml | 15 +--
> src/backend/catalog/sql_features.txt | 2 +-
> src/backend/executor/nodeLimit.c | 160 +++++++++++++++++++++++++++---
> src/backend/nodes/copyfuncs.c | 7 ++
> src/backend/nodes/equalfuncs.c | 2 +
> src/backend/nodes/outfuncs.c | 7 ++
> src/backend/nodes/readfuncs.c | 6 ++
> src/backend/optimizer/plan/createplan.c | 45 ++++++++-
> src/backend/optimizer/plan/planner.c | 1 +
> src/backend/optimizer/util/pathnode.c | 2 +
> src/backend/parser/analyze.c | 21 ++--
> src/backend/parser/gram.y | 117 +++++++++++++++++-----
> src/backend/parser/parse_clause.c | 15 ++-
> src/backend/utils/adt/ruleutils.c | 27 ++++--
> src/include/catalog/catversion.h | 2 +-
> src/include/nodes/execnodes.h | 5 +
> src/include/nodes/nodes.h | 13 +++
> src/include/nodes/parsenodes.h | 2 +
> src/include/nodes/pathnodes.h | 1 +
> src/include/nodes/plannodes.h | 5 +
> src/include/optimizer/pathnode.h | 1 +
> src/include/optimizer/planmain.h | 5 +-
> src/include/parser/parse_clause.h | 3 +-
> src/test/regress/expected/limit.out | 167 ++++++++++++++++++++++++++++++++
> src/test/regress/sql/limit.sql | 48 +++++++++
> 25 files changed, 610 insertions(+), 69 deletions(-)
>

FTR I now get the following when compiling with -Wimplicit-fallthrough -Werror:

nodeLimit.c: In function ‘ExecLimit’:
nodeLimit.c:136:7: error: this statement may fall through
[-Werror=implicit-fallthrough=]
136 | if (ScanDirectionIsForward(direction))
| ^
nodeLimit.c:216:3: note: here
216 | case LIMIT_WINDOWEND_TIES:
| ^~~~

It seems that this fall-through comment:

[...]
else
{
node->lstate = LIMIT_WINDOWEND_TIES;
/* fall-through */
}
[...]

Is not recognized by my compiler (gcc (Gentoo 9.3.0 p1) 9.3.0). If
that's something we should fix, PFA a naive patch for that.

Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> On Tue, Apr 7, 2020 at 10:28 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>> Support FETCH FIRST WITH TIES

> FTR I now get the following when compiling with -Wimplicit-fallthrough -Werror:

Yeah, assorted buildfarm animals are mentioning that too. I wonder
if we should add that to the default warning options selected by
configure? I don't remember if that's been discussed before.

> It seems that this fall-through comment:
> /* fall-through */
> Is not recognized by my compiler (gcc (Gentoo 9.3.0 p1) 9.3.0). If
> that's something we should fix, PFA a naive patch for that.

Hmm, I feel like this logic is baroque enough to need more of a rewrite
than that :-(. But not sure exactly what would be better, so your
patch seems reasonable for now. The comments could use some help too.

regards, tom lane

On Sat, Apr 11, 2020 at 02:47:34PM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > On Tue, Apr 7, 2020 at 10:28 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >> Support FETCH FIRST WITH TIES
>
> > FTR I now get the following when compiling with -Wimplicit-fallthrough -Werror:
>
> Yeah, assorted buildfarm animals are mentioning that too. I wonder
> if we should add that to the default warning options selected by
> configure? I don't remember if that's been discussed before.
>

I'm all for it. It seems like a trap easy to catch up early, and we do want to
enforce it anyway. I'm attaching a simple patch for that if needed, hopefully
with the correct autoconf version.

> > It seems that this fall-through comment:
> > /* fall-through */
> > Is not recognized by my compiler (gcc (Gentoo 9.3.0 p1) 9.3.0). If
> > that's something we should fix, PFA a naive patch for that.
>
> Hmm, I feel like this logic is baroque enough to need more of a rewrite
> than that :-(. But not sure exactly what would be better, so your
> patch seems reasonable for now. The comments could use some help too.

Yes I just checked the state machine to make sure that the fallthrough was
expected, but the comments are now way better, thanks!

Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> On Sat, Apr 11, 2020 at 02:47:34PM -0400, Tom Lane wrote:
>> Yeah, assorted buildfarm animals are mentioning that too. I wonder
>> if we should add that to the default warning options selected by
>> configure? I don't remember if that's been discussed before.

> I'm all for it. It seems like a trap easy to catch up early, and we do want to
> enforce it anyway. I'm attaching a simple patch for that if needed, hopefully
> with the correct autoconf version.

Poking around in the archives, it seems like the only previous formal
proposal to add -Wimplicit-fallthrough was in the context of a much
more aggressive proposal to make a lot of non-Wall warnings into
errors [1], which people did not like.

-Wimplicit-fallthrough does have some issues, eg it seems that it's
applied at a stage where gcc hasn't yet figured out that elog(ERROR)
doesn't return, so you need to add breaks after those. But we had
sort of agreed that we could have it on-by-default in one relevant
discussion [2], and then failed to pull the trigger.

If we do this, I suggest we use -Wimplicit-fallthrough=4, which
uses a more-restrictive-than-default definition of how a "fallthrough"
comment can be spelled. The default, per the gcc manual, is

* -Wimplicit-fallthrough=3 case sensitively matches one of the
following regular expressions:

*<"-fallthrough">
*<"@fallthrough@">
*<"lint -fallthrough[ \t]*">
*<"[ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?FALL(S |
|-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?">
*<"[ \t.!]*(Else,? |Intentional(ly)? )?Fall((s |
|-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)?">
*<"[ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )?fall(s |
|-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?">

which to my eyes is not exactly encouraging a project-standard style
for these, plus it seems like it might accept some things we'd rather
it didn't. The only more-restrictive alternative, short of disabling
the comments altogether, is

* -Wimplicit-fallthrough=4 case sensitively matches one of the
following regular expressions:

*<"-fallthrough">
*<"@fallthrough@">
*<"lint -fallthrough[ \t]*">
*<"[ \t]*FALLTHR(OUGH|U)[ \t]*">

Thoughts?

regards, tom lane

[1] /message-id/B9FB1155-B39D-43C9-A7E6-B67E1C59E4CE%40gmail.com
[2] /message-id/flat/E1fDenm-0000C8-IJ%40gemulon.postgresql.org

> On Apr 12, 2020, at 7:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Poking around in the archives, it seems like the only previous formal
> proposal to add -Wimplicit-fallthrough was in the context of a much
> more aggressive proposal to make a lot of non-Wall warnings into
> errors [1], which people did not like.

That was from me.

> The only more-restrictive alternative, short of disabling
> the comments altogether, is
>
> * -Wimplicit-fallthrough=4 case sensitively matches one of the
> following regular expressions:
>
> *<"-fallthrough">
> *<"@fallthrough@">
> *<"lint -fallthrough[ \t]*">
> *<"[ \t]*FALLTHR(OUGH|U)[ \t]*">
>
> Thoughts?

Naturally, I'm +1 for this.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Apr 12, 2020 at 5:25 PM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>
> > On Apr 12, 2020, at 7:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Poking around in the archives, it seems like the only previous formal
> > proposal to add -Wimplicit-fallthrough was in the context of a much
> > more aggressive proposal to make a lot of non-Wall warnings into
> > errors [1], which people did not like.
>
> That was from me.
>
> > The only more-restrictive alternative, short of disabling
> > the comments altogether, is
> >
> > * -Wimplicit-fallthrough=4 case sensitively matches one of the
> > following regular expressions:
> >
> > *<"-fallthrough">
> > *<"@fallthrough@">
> > *<"lint -fallthrough[ \t]*">
> > *<"[ \t]*FALLTHR(OUGH|U)[ \t]*">
> >
> > Thoughts?
>
> Naturally, I'm +1 for this.

+1 too, obviously.

Do we intend to see this done in the current cycle?

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Do we intend to see this done in the current cycle?

I don't have an objection to doing it now. It's just a new compiler
warning option, it shouldn't be able to break any code. (Plus there's
plenty of time to revert, if somehow it causes a problem.)

regards, tom lane

On 2020-Apr-12, Tom Lane wrote:

> The only more-restrictive alternative, short of disabling
> the comments altogether, is
>
> * -Wimplicit-fallthrough=4 case sensitively matches one of the
> following regular expressions:
>
> *<"-fallthrough">
> *<"@fallthrough@">
> *<"lint -fallthrough[ \t]*">
> *<"[ \t]*FALLTHR(OUGH|U)[ \t]*">
>
> Thoughts?

This doesn't allow whitespace between "fall" and "through", which means
we generate 217 such warnings currently. Or we can just use
-Wimplicit-fallthrough=3, which does allow whitespace (among other
detritus).

For my own reference, the manual is at
https://gcc.gnu.org/onlinedocs/gcc-8.3.0/gcc/Warning-Options.html#index-Wimplicit-fallthrough

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

On 2020-May-06, Alvaro Herrera wrote:

> This doesn't allow whitespace between "fall" and "through", which means
> we generate 217 such warnings currently. Or we can just use
> -Wimplicit-fallthrough=3, which does allow whitespace (among other
> detritus).

If we're OK with patching all those places, I volunteer to do so. Any
objections? Or I can keep it at level 3, which can be done with minimal
patching.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> On 2020-May-06, Alvaro Herrera wrote:
>> This doesn't allow whitespace between "fall" and "through", which means
>> we generate 217 such warnings currently. Or we can just use
>> -Wimplicit-fallthrough=3, which does allow whitespace (among other
>> detritus).

> If we're OK with patching all those places, I volunteer to do so. Any
> objections? Or I can keep it at level 3, which can be done with minimal
> patching.

If we're gonna do it at all, I think we should go for the level 4
warnings. Level 3's idea of a fallthrough comment is too liberal
for my tastes...

regards, tom lane

On Mon, May 11, 2020 at 6:47 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > On 2020-May-06, Alvaro Herrera wrote:
> >> This doesn't allow whitespace between "fall" and "through", which means
> >> we generate 217 such warnings currently. Or we can just use
> >> -Wimplicit-fallthrough=3, which does allow whitespace (among other
> >> detritus).
>
> > If we're OK with patching all those places, I volunteer to do so. Any
> > objections? Or I can keep it at level 3, which can be done with minimal
> > patching.
>
> If we're gonna do it at all, I think we should go for the level 4
> warnings. Level 3's idea of a fallthrough comment is too liberal
> for my tastes...

Here's a patch that also takes care of cleaning all warning due to the
level 4 setting (at least the one I got with my other config options).
I went with "FALLTHRU" as this seemed more used.

Note that timezone/zic.c would also have to be changed.

On 5/11/20 7:29 AM, Julien Rouhaud wrote:
> On Mon, May 11, 2020 at 6:47 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>>> On 2020-May-06, Alvaro Herrera wrote:
>>>> This doesn't allow whitespace between "fall" and "through", which means
>>>> we generate 217 such warnings currently. Or we can just use
>>>> -Wimplicit-fallthrough=3, which does allow whitespace (among other
>>>> detritus).
>>> If we're OK with patching all those places, I volunteer to do so. Any
>>> objections? Or I can keep it at level 3, which can be done with minimal
>>> patching.
>> If we're gonna do it at all, I think we should go for the level 4
>> warnings. Level 3's idea of a fallthrough comment is too liberal
>> for my tastes...
> Here's a patch that also takes care of cleaning all warning due to the
> level 4 setting (at least the one I got with my other config options).
> I went with "FALLTHRU" as this seemed more used.
>
> Note that timezone/zic.c would also have to be changed.

Since this is external code maybe we should leave that at level 3? I
think that should be doable via a Makefile override.

cheers

andrew

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

On Mon, May 11, 2020 at 2:07 PM Andrew Dunstan
<andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>
>
> On 5/11/20 7:29 AM, Julien Rouhaud wrote:
> > On Mon, May 11, 2020 at 6:47 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> >>> On 2020-May-06, Alvaro Herrera wrote:
> >>>> This doesn't allow whitespace between "fall" and "through", which means
> >>>> we generate 217 such warnings currently. Or we can just use
> >>>> -Wimplicit-fallthrough=3, which does allow whitespace (among other
> >>>> detritus).
> >>> If we're OK with patching all those places, I volunteer to do so. Any
> >>> objections? Or I can keep it at level 3, which can be done with minimal
> >>> patching.
> >> If we're gonna do it at all, I think we should go for the level 4
> >> warnings. Level 3's idea of a fallthrough comment is too liberal
> >> for my tastes...
> > Here's a patch that also takes care of cleaning all warning due to the
> > level 4 setting (at least the one I got with my other config options).
> > I went with "FALLTHRU" as this seemed more used.
> >
> > Note that timezone/zic.c would also have to be changed.
>
>
>
> Since this is external code maybe we should leave that at level 3? I
> think that should be doable via a Makefile override.

Yes that was my concern. The best way I found to avoid changing zic.c
is something like that in src/timezone/Makefile, before the zic
target:

ifneq (,$(filter -Wimplicit-fallthrough=4,$(CFLAGS)))
CFLAGS := $(filter-out -Wimplicit-fallthrough=4,$(CFLAGS))
CFLAGS += '-Wimplicit-fallthrough=3'
endif

Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> Note that timezone/zic.c would also have to be changed.

Why? It uses "fallthrough" which is a legal spelling per level 4.

regards, tom lane

On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > Note that timezone/zic.c would also have to be changed.
>
> Why? It uses "fallthrough" which is a legal spelling per level 4.

GCC documentation mentions [ \t]*FALLTHR(OUGH|U)[ \t]* for level 4
(out of the view other alternatives), which AFAICT is case sensitive
(level 3 has fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?).

Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Why? It uses "fallthrough" which is a legal spelling per level 4.

> GCC documentation mentions [ \t]*FALLTHR(OUGH|U)[ \t]* for level 4
> (out of the view other alternatives), which AFAICT is case sensitive
> (level 3 has fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?).

Oh, I'd missed that that was case sensitive. Ugh --- that seems
unreasonable. Maybe we'd better settle for level 3 after all;
I don't think there's much room to doubt the intentions of a
comment spelled that way.

regards, tom lane

On Mon, May 11, 2020 at 4:40 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Why? It uses "fallthrough" which is a legal spelling per level 4.
>
> > GCC documentation mentions [ \t]*FALLTHR(OUGH|U)[ \t]* for level 4
> > (out of the view other alternatives), which AFAICT is case sensitive
> > (level 3 has fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?).
>
> Oh, I'd missed that that was case sensitive. Ugh --- that seems
> unreasonable. Maybe we'd better settle for level 3 after all;
> I don't think there's much room to doubt the intentions of a
> comment spelled that way.

Agreed.

On 2020-May-11, Julien Rouhaud wrote:

> On Mon, May 11, 2020 at 4:40 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > > On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >> Why? It uses "fallthrough" which is a legal spelling per level 4.
> >
> > > GCC documentation mentions [ \t]*FALLTHR(OUGH|U)[ \t]* for level 4
> > > (out of the view other alternatives), which AFAICT is case sensitive
> > > (level 3 has fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?).
> >
> > Oh, I'd missed that that was case sensitive. Ugh --- that seems
> > unreasonable. Maybe we'd better settle for level 3 after all;
> > I don't think there's much room to doubt the intentions of a
> > comment spelled that way.
>
> Agreed.

Pushed, thanks.

I ended up using level 4 and dialling back to 3 for zic.c only
(different make trickery though). I also settled on FALLTHROUGH rather
than FALLTHRU because the latter seems ugly as a spelling to me. I'm
not a fan of the uppercase, but the alternative would be to add a - or
@s.

I get no warnings with this (gcc 8), but ccache seems to save warnings
in one run so that they can be thrown in a later one. I'm not sure what
to make of that, but ccache -d proved that beyond reasonable doubt and
ccache -clear got rid of the lot.

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

On 2020-May-12, Alvaro Herrera wrote:

> I get no warnings with this (gcc 8), but ccache seems to save warnings
> in one run so that they can be thrown in a later one. I'm not sure what
> to make of that, but ccache -d proved that beyond reasonable doubt and
> ccache -clear got rid of the lot.

Fixed one straggler in contrib, and while testing it I realized why
ccache doesn't pay attention to the changes I was doing in the file:
ccache compares the *preprocessed* version of the file and only if that
differs from the version that was cached last, ccache sends the new one
to the compiler; and of course these comments are not present in the
preprocessed version, so changing only the comment accomplishes nothing.
You have to touch one byte outside of any comments.

I bet this is going to bite someone ... maybe we'd be better off going
all the way to -Wimplicit-fallthrough=5 and use the
__attribute__((fallthrough)) stuff instead.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> I ended up using level 4 and dialling back to 3 for zic.c only
> (different make trickery though).

Meh ... if we're going to use level 4, I'm inclined to just change zic.c
to match. It's not like we're using the upstream code verbatim anyway.
We could easily add s/fallthrough/FALLTHROUGH/ to the conversion recipe.

> I get no warnings with this (gcc 8), but ccache seems to save warnings
> in one run so that they can be thrown in a later one. I'm not sure what
> to make of that, but ccache -d proved that beyond reasonable doubt and
> ccache -clear got rid of the lot.

Sounds like a ccache bug: maybe it's not accounting for different
fallthrough warning levels. ccache knows a *ton* about gcc options,
so I'd not be surprised if it's doing something special with this one.

regards, tom lane

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Fixed one straggler in contrib, and while testing it I realized why
> ccache doesn't pay attention to the changes I was doing in the file:
> ccache compares the *preprocessed* version of the file and only if that
> differs from the version that was cached last, ccache sends the new one
> to the compiler; and of course these comments are not present in the
> preprocessed version, so changing only the comment accomplishes nothing.
> You have to touch one byte outside of any comments.

Ugh. So the only way ccache could avoid this is to drop the
preprocessed-file comparison check if -Wimplicit-fallthrough is on.
Doesn't really sound like something we'd want to ask them to do.

> I bet this is going to bite someone ... maybe we'd be better off going
> all the way to -Wimplicit-fallthrough=5 and use the
> __attribute__((fallthrough)) stuff instead.

I'm not really in favor of the __attribute__ solution --- seems too
gcc-specific. FALLTHROUGH-type comments are understood by other
sorts of tools besides gcc.

In practice, it doesn't seem like this'll be a huge problem once
we're past the initial fixup stage. We can revisit it later if
that prediction proves wrong, of course.

regards, tom lane

At Tue, 12 May 2020 17:12:51 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Fixed one straggler in contrib, and while testing it I realized why
> > ccache doesn't pay attention to the changes I was doing in the file:
> > ccache compares the *preprocessed* version of the file and only if that
> > differs from the version that was cached last, ccache sends the new one
> > to the compiler; and of course these comments are not present in the
> > preprocessed version, so changing only the comment accomplishes nothing.
> > You have to touch one byte outside of any comments.
>
> Ugh. So the only way ccache could avoid this is to drop the
> preprocessed-file comparison check if -Wimplicit-fallthrough is on.
> Doesn't really sound like something we'd want to ask them to do.
>
> > I bet this is going to bite someone ... maybe we'd be better off going
> > all the way to -Wimplicit-fallthrough=5 and use the
> > __attribute__((fallthrough)) stuff instead.
>
> I'm not really in favor of the __attribute__ solution --- seems too
> gcc-specific. FALLTHROUGH-type comments are understood by other
> sorts of tools besides gcc.
>
> In practice, it doesn't seem like this'll be a huge problem once
> we're past the initial fixup stage. We can revisit it later if
> that prediction proves wrong, of course.

FWIW, I got a warning for jsonpath_gram.c.

> jsonpath_gram.c:1026:16: warning: this statement may fall through [-Wimplicit-fallthrough=]
> if (*++yyp != '\\')
> ^
> jsonpath_gram.c:1029:11: note: here
> default:
> ^~~~~~~

jsonpath_gram.c:1025
> case '\\':
> if (*++yyp != '\\')
> goto do_not_strip_quotes;
> /* Fall through. */
> default:

It is generated code by bison.

$ bison --version
bison (GNU Bison) 3.0.4

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, May 13, 2020 at 4:13 PM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
wrote:

> At Tue, 12 May 2020 17:12:51 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > > Fixed one straggler in contrib, and while testing it I realized why
> > > ccache doesn't pay attention to the changes I was doing in the file:
> > > ccache compares the *preprocessed* version of the file and only if that
> > > differs from the version that was cached last, ccache sends the new one
> > > to the compiler; and of course these comments are not present in the
> > > preprocessed version, so changing only the comment accomplishes
> nothing.
> > > You have to touch one byte outside of any comments.
> >
> > Ugh. So the only way ccache could avoid this is to drop the
> > preprocessed-file comparison check if -Wimplicit-fallthrough is on.
> > Doesn't really sound like something we'd want to ask them to do.
> >
> > > I bet this is going to bite someone ... maybe we'd be better off going
> > > all the way to -Wimplicit-fallthrough=5 and use the
> > > __attribute__((fallthrough)) stuff instead.
> >
> > I'm not really in favor of the __attribute__ solution --- seems too
> > gcc-specific. FALLTHROUGH-type comments are understood by other
> > sorts of tools besides gcc.
> >
> > In practice, it doesn't seem like this'll be a huge problem once
> > we're past the initial fixup stage. We can revisit it later if
> > that prediction proves wrong, of course.
>
> FWIW, I got a warning for jsonpath_gram.c.
>
> > jsonpath_gram.c:1026:16: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> > if (*++yyp != '\\')
> > ^
> > jsonpath_gram.c:1029:11: note: here
> > default:
> > ^~~~~~~
>
> jsonpath_gram.c:1025
> > case '\\':
> > if (*++yyp != '\\')
> > goto do_not_strip_quotes;
> > /* Fall through. */
> > default:
>
> It is generated code by bison.
>
> $ bison --version
> bison (GNU Bison) 3.0.4
>
>
I just found this just serval minutes ago. Upgrading your bison to the
latest
version (3.6) is ok. I'd like we have a better way to share this knowledge
through.
I spend ~30 minutes to troubleshooting this issue.

Best Regards
Andy Fan

At Wed, 13 May 2020 16:17:50 +0800, Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote in
> On Wed, May 13, 2020 at 4:13 PM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> wrote:
> > > jsonpath_gram.c:1026:16: warning: this statement may fall through
> > [-Wimplicit-fallthrough=]
> > > if (*++yyp != '\\')
> > > ^
> > > jsonpath_gram.c:1029:11: note: here
> > > default:
...
> > It is generated code by bison.
> >
> > $ bison --version
> > bison (GNU Bison) 3.0.4
> >
> >
> I just found this just serval minutes ago. Upgrading your bison to the
> latest
> version (3.6) is ok. I'd like we have a better way to share this knowledge
> through.
> I spend ~30 minutes to troubleshooting this issue.

Thanks. I'm happy to know that! But AFAICS 3.0.4 is the current
version of bison in AppStream and PowerTools of CentOS8...

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, May 13, 2020 at 12:16 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Wed, 13 May 2020 16:17:50 +0800, Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote in
> > On Wed, May 13, 2020 at 4:13 PM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> > wrote:
> > > > jsonpath_gram.c:1026:16: warning: this statement may fall through
> > > [-Wimplicit-fallthrough=]
> > > > if (*++yyp != '\\')
> > > > ^
> > > > jsonpath_gram.c:1029:11: note: here
> > > > default:
> ...
> > > It is generated code by bison.
> > >
> > > $ bison --version
> > > bison (GNU Bison) 3.0.4
> > >
> > >
> > I just found this just serval minutes ago. Upgrading your bison to the
> > latest
> > version (3.6) is ok. I'd like we have a better way to share this knowledge
> > through.
> > I spend ~30 minutes to troubleshooting this issue.
>
> Thanks. I'm happy to know that! But AFAICS 3.0.4 is the current
> version of bison in AppStream and PowerTools of CentOS8...

FTR I'm using bison 3.5 and I didn't hit any issue. However that may
be because of ccache, as mentioned by Alvaro.

Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> writes:
>> FWIW, I got a warning for jsonpath_gram.c.

Ugh. Confirmed here on Fedora 30 (bison 3.0.5).

> I just found this just serval minutes ago. Upgrading your bison to the
> latest version (3.6) is ok. I'd like we have a better way to share this
> knowledge through. I spend ~30 minutes to troubleshooting this issue.

I fear that is going to mean that we revert this patch.
We are *NOT* moving the minimum bison requirement for this,
especially not to a bleeding-edge bison version.

Alternatively, it might work to go back down to warning level 3;
I see that the code in question has

/* Fall through. */

which I believe would work at the lower warning level. But that
raises the question of how far back bison generates code that
is clean --- and, again, I'm not willing to move the minimum
bison requirement. (On the other hand, if you have an old bison,
you likely also have an old gcc that doesn't know this warning
switch, so maybe it'd be all right in practice?)

regards, tom lane

On 2020-May-13, Tom Lane wrote:

> Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> writes:
> >> FWIW, I got a warning for jsonpath_gram.c.
>
> Ugh. Confirmed here on Fedora 30 (bison 3.0.5).

Ugh.

> > I just found this just serval minutes ago. Upgrading your bison to the
> > latest version (3.6) is ok. I'd like we have a better way to share this
> > knowledge through. I spend ~30 minutes to troubleshooting this issue.
>
> I fear that is going to mean that we revert this patch.

Or we can filter-out the -Wimplicit-fallthrough, or change to level 3,
for bison-emitted files.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> I fear that is going to mean that we revert this patch.

> Or we can filter-out the -Wimplicit-fallthrough, or change to level 3,
> for bison-emitted files.

Let's just go to level 3 overall (and revert the changes you made for
level 4 compliance --- they're more likely to cause back-patching
pain than do anything useful).

regards, tom lane

On 2020-May-13, Tom Lane wrote:

> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> >> I fear that is going to mean that we revert this patch.
>
> > Or we can filter-out the -Wimplicit-fallthrough, or change to level 3,
> > for bison-emitted files.
>
> Let's just go to level 3 overall (and revert the changes you made for
> level 4 compliance --- they're more likely to cause back-patching
> pain than do anything useful).

Ok.

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

On 2020-May-13, Alvaro Herrera wrote:

> On 2020-May-13, Tom Lane wrote:

> > Let's just go to level 3 overall (and revert the changes you made for
> > level 4 compliance --- they're more likely to cause back-patching
> > pain than do anything useful).
>
> Ok.

And done.

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

On Wed, May 13, 2020 at 10:02 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> writes:
> >> FWIW, I got a warning for jsonpath_gram.c.
>
> Ugh. Confirmed here on Fedora 30 (bison 3.0.5).
>
> > I just found this just serval minutes ago. Upgrading your bison to the
> > latest version (3.6) is ok. I'd like we have a better way to share this
> > knowledge through. I spend ~30 minutes to troubleshooting this issue.
>
> I fear that is going to mean that we revert this patch.
> We are *NOT* moving the minimum bison requirement for this,
> especially not to a bleeding-edge bison version.

Yes, I didn't mean revert the patch, but I was thinking moving the minimum
bison. But since down to the warning level 3 also resolved the issue,
looks it is a better way to do it.

(On the other hand, if you have an old bison,
>
you likely also have an old gcc that doesn't know this warning
> switch, so maybe it'd be all right in practice?)
>
>
I just use an old bision and a newer gcc:( and I used "echo "COPT=-Wall
-Werror"
> src/Makefile.custom" which is same as our cfbot system. Thank you all
for so quick
fix!

Best Regards
Andy Fan