pgsql: Support FETCH FIRST WITH TIES

Hi Álvaro,

On Tue, Apr 7, 2020 at 10:28 PM Alvaro Herrera <alvherre@alvh.no-ip.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@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Discussion: /messages/by-id/CALAY4q9ky7rD_A4vf=FVQvCGngm3LOes-ky0J6euMrg=_Se+ag@mail.gmail.com
Discussion: /messages/by-id/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@gmail.com> writes:

On Tue, Apr 7, 2020 at 10:28 PM Alvaro Herrera <alvherre@alvh.no-ip.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@gmail.com> writes:

On Tue, Apr 7, 2020 at 10:28 PM Alvaro Herrera <alvherre@alvh.no-ip.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@gmail.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]/messages/by-id/B9FB1155-B39D-43C9-A7E6-B67E1C59E4CE@gmail.com, 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]/messages/by-id/E1fDenm-0000C8-IJ@gemulon.postgresql.org, 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]: /messages/by-id/B9FB1155-B39D-43C9-A7E6-B67E1C59E4CE@gmail.com
[2]: /messages/by-id/E1fDenm-0000C8-IJ@gemulon.postgresql.org

On Apr 12, 2020, at 7:55 AM, Tom Lane <tgl@sss.pgh.pa.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.dilger@enterprisedb.com> wrote:

On Apr 12, 2020, at 7:55 AM, Tom Lane <tgl@sss.pgh.pa.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@2ndquadrant.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@2ndquadrant.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@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.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@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.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.dunstan@2ndquadrant.com> wrote:

On 5/11/20 7:29 AM, Julien Rouhaud wrote:

On Mon, May 11, 2020 at 6:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.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@gmail.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@sss.pgh.pa.us> wrote:

Julien Rouhaud <rjuju123@gmail.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@gmail.com> writes:

On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:

Julien Rouhaud <rjuju123@gmail.com> writes:

On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:

Julien Rouhaud <rjuju123@gmail.com> writes:

On Mon, May 11, 2020 at 3:41 PM Tom Lane <tgl@sss.pgh.pa.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