Re: Fix for FETCH FIRST syntax problems

>>>>> "Stephen" == Stephen Frost <sfrost(at)snowman(dot)net> writes:

Stephen> Just to be clear, based on what I saw on IRC, this
Stephen> specifically came out of someone complaining that it didn't
Stephen> work and caused difficulty for them.

Specifically, as I said at the start, it's from bug #15200, see
http://postgr.es/m/152647780335.27204.16895288237122418685@wrigleys.postgresql.org

Stephen> As such, my inclination on this would be to back-patch it, but
Stephen> we'd need to be sure to test it and be confident that it won't
Stephen> break things which worked before.

Well, that's kind of what I have been doing (and why I posted a second
version of the patch).

So let's go over the full detail. The old syntax is:

OFFSET select_offset_value2 row_or_rows
FETCH first_or_next opt_select_fetch_first_value row_or_rows ONLY

opt_select_fetch_first_value: SignedIconst | '(' a_expr ')' | /*EMPTY*/;
select_offset_value2: c_expr;
SignedIconst: Iconst | '+' Iconst | '-' Iconst;

The new syntax is:

OFFSET select_fetch_first_value row_or_rows
FETCH first_or_next select_fetch_first_value row_or_rows ONLY
FETCH first_or_next row_or_rows ONLY

select_fetch_first_value: c_expr | '+' I_or_F_const | '-' I_or_F_const;

In both cases note that the sequence '(' a_expr ')' is a valid c_expr.

The old syntax for OFFSET x ROWS clearly simplifies to

OFFSET c_expr [ROW|ROWS]

and the new syntax clearly accepts a strict superset of that, since it
just adds +/- I_or_F_const as an alternative to c_expr, fixing the
(probably inconsequential) bug that OFFSET +1 ROWS didn't work despite
being legal in the spec.

The old syntax for FETCH FIRST expands to:

1) FETCH first_or_next Iconst row_or_rows ONLY
2) FETCH first_or_next '+' Iconst row_or_rows ONLY
3) FETCH first_or_next '-' Iconst row_or_rows ONLY
4) FETCH first_or_next '(' a_expr ')' row_or_rows ONLY
5) FETCH first_or_next row_or_rows ONLY

5) is explicitly matched in the new syntax. 1) and 4) both match via the
fact that Iconst and '(' a_expr ')' are valid for c_expr. 2) and 3) are
matched in the new syntax with the addition of accepting FCONST as well
as Iconst.

So all input that satisfied the old syntax will be accepted by the new
one.

Of course, I have done actual tests comparing the old and new versions,
with results consistent with the above. I do note that there seems to be
no test coverage at all - none was added in commit 361bfc357 which
created the feature, and nobody seems to have cared about it since other
than some doc tweaks.

I've also checked (by splitting into separate ROW and ROWS alternatives)
that there aren't any grammar conflicts being "hidden" inappropriately
by precedence (ROWS has a precedence assigned, but ROW does not).
Inspection of the bison verbose output confirms this.

Normally when messing with the grammar one would also have to consider
the effect on dump/restore. But in this case, we never actually generate
this syntax when deparsing - offset/limit clauses are always deparsed as
the OFFSET x LIMIT y syntax instead. So we don't have to worry about,
for example, dumping from a newer point release to an older one; the
only time we see this syntax is if the client generates it.

--
Andrew (irc:RhodiumToad)