Horton

Thread

Commits

  1. Fix SQL:2008 FETCH FIRST syntax to allow parameters.

  2. SQL:2008 alternative syntax for LIMIT/OFFSET:

  1. Fix for FETCH FIRST syntax problems

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-05-19T22:33:22Z

    Per bug #15200, our support for sql2008 FETCH FIRST syntax is incomplete
to the extent that it should be regarded as a bug; the spec quite
clearly allows parameters/host variables in addition to constants, but
we don't.

Attached is a draft patch for fixing that, which additionally fixes the
ugly wart that OFFSET <x> ROW/ROWS and FETCH FIRST [<x>] ROW/ROWS ONLY
had very different productions for <x>; both now accept c_expr there.

Shift/reduce conflict is avoided by taking advantage of the fact that
ONLY is a fully reserved word.

I've checked that this change would not make it any harder to add
(post-2008 features) WITH TIES or PERCENT in the event that someone
wants to do that.

I think a case can be made that this should be backpatched; thoughts?

(While I can't find any visible change for existing working queries, one
change that does occur is that FETCH FIRST -1 ROWS ONLY now returns a
different error message; but that was already inconsistent with the
error from OFFSET -1 ROWS.)

-- 
Andrew (irc:RhodiumToad)

  2. Re: Fix for FETCH FIRST syntax problems

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-19T22:57:55Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Attached is a draft patch for fixing that, which additionally fixes the
> ugly wart that OFFSET <x> ROW/ROWS and FETCH FIRST [<x>] ROW/ROWS ONLY
> had very different productions for <x>; both now accept c_expr there.

LGTM, except please s/presense/presence/ in grammar comment.
Also, why'd you back off "must write" to "should write" in the docs?
This doesn't make the parens any more optional than before.

> I think a case can be made that this should be backpatched; thoughts?

Meh, -0.5.  This is not really a bug, because it's operating as designed.
You've found a reasonably painless way to improve the grammar, which is
great, but it seems more like a feature addition than a bug fix.

I'd be fine with sneaking this into v11, though.

			regards, tom lane

  3. Re: Fix for FETCH FIRST syntax problems

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-05-19T23:27:30Z

    On 20/05/18 00:57, Tom Lane wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> Attached is a draft patch for fixing that, which additionally fixes the
>> ugly wart that OFFSET <x> ROW/ROWS and FETCH FIRST [<x>] ROW/ROWS ONLY
>> had very different productions for <x>; both now accept c_expr there.
> 
> LGTM, except please s/presense/presence/ in grammar comment.
> Also, why'd you back off "must write" to "should write" in the docs?
> This doesn't make the parens any more optional than before.

It certainly does.  It allows this now:

PREPARE foo AS TABLE bar FETCH FIRST $1 ROWS ONLY;

>> I think a case can be made that this should be backpatched; thoughts?
> 
> Meh, -0.5.  This is not really a bug, because it's operating as designed.
> You've found a reasonably painless way to improve the grammar, which is
> great, but it seems more like a feature addition than a bug fix.
> 
> I'd be fine with sneaking this into v11, though.
I'm +1 for backpatching it.  It may be operating as designed by PeterE
ten years ago, but it's not operating as designed by the SQL standard.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  4. Re: Fix for FETCH FIRST syntax problems

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-05-19T23:30:00Z

    On 20/05/18 01:27, Vik Fearing wrote:
>> Also, why'd you back off "must write" to "should write" in the docs?
>> This doesn't make the parens any more optional than before.
>> It certainly does.  It allows this now:
> 
> PREPARE foo AS TABLE bar FETCH FIRST $1 ROWS ONLY;

Please disregard this comment.

My +1 for backpatching still stands.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  5. Re: Fix for FETCH FIRST syntax problems

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-19T23:41:10Z

    Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> On 20/05/18 00:57, Tom Lane wrote:
>> Also, why'd you back off "must write" to "should write" in the docs?
>> This doesn't make the parens any more optional than before.

> It certainly does.  It allows this now:
> PREPARE foo AS TABLE bar FETCH FIRST $1 ROWS ONLY;

No, it makes the parens omittable in the cases specified in the previous
sentence.  The sentence I'm complaining about is describing other cases,
in which they're still required.

> I'm +1 for backpatching it.  It may be operating as designed by PeterE
> ten years ago, but it's not operating as designed by the SQL standard.

By that argument, *anyplace* where we're missing a SQL-spec feature
is a back-patchable bug.  I don't buy it.

It may be that this fix is simple and safe enough that the risk/reward
tradeoff favors back-patching, but I think you have to argue it as a
favorable tradeoff rather than just saying "this isn't per standard".
Consider: if Andrew had completely rewritten gram.y to get the same
visible effect, would you think that was back-patchable?

			regards, tom lane

  6. Re: Fix for FETCH FIRST syntax problems

    Michael Paquier <michael@paquier.xyz> — 2018-05-19T23:57:06Z

    On Sat, May 19, 2018 at 07:41:10PM -0400, Tom Lane wrote:
> Vik Fearing <vik.fearing@2ndquadrant.com> writes:
>> I'm +1 for backpatching it.  It may be operating as designed by PeterE
>> ten years ago, but it's not operating as designed by the SQL standard.
> 
> By that argument, *anyplace* where we're missing a SQL-spec feature
> is a back-patchable bug.  I don't buy it.

+1.
--
Michael

  7. Re: Fix for FETCH FIRST syntax problems

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-05-20T00:36:10Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> Also, why'd you back off "must write" to "should write" in the docs?
 Tom> This doesn't make the parens any more optional than before.

Currently, the requirement for parens is inconsistent - FETCH FIRST
wants them for absolutely anything that's not a literal constant, but
OFFSET x ROWS allows any c_expr (which covers a whole lot of ground in
addition to the spec's requirements). The docs don't distinguish these
two and just say "must write" parens even though some cases clearly work
without.

(There's also the slight wart that OFFSET -1 ROWS is a syntax error,
whereas the spec defines it as valid syntax but a semantic error. Do we
care?)

With the patch, c_exprs would be accepted without parens, so saying
"must write" parens in the docs clearly isn't entirely accurate. I'm
open to better phrasing.

I found some more warts: OFFSET +1 ROWS isn't accepted (but should be),
and FETCH FIRST 10000000000 ROWS ONLY fails on 32bit and Windows builds.
The patch as posted fixes the second of those but makes FETCH FIRST +1
fail instead; I'm working on that.

-- 
Andrew (irc:RhodiumToad)

  8. Re: Fix for FETCH FIRST syntax problems

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-05-20T00:39:27Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> I'm +1 for backpatching it. It may be operating as designed by
 >> PeterE ten years ago, but it's not operating as designed by the SQL
 >> standard.

 Tom> By that argument, *anyplace* where we're missing a SQL-spec
 Tom> feature is a back-patchable bug. I don't buy it.

But this is a feature we already claimed to actually support (it's
listed in sql_features with a bunch of unqualified YES entries), but in
fact doesn't work properly.

-- 
Andrew (irc:RhodiumToad)

  9. Re: Fix for FETCH FIRST syntax problems

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-05-20T03:25:33Z

    Updated patch.

This one explicitly accepts +/- ICONST/FCONST in addition to c_expr for
both offset and limit, removing the inconsistencies mentioned
previously.

I changed the wording of the docs part a bit; does that look better? or
worse?

Old behavior:
select 1 offset +1 rows;  -- ERROR:  syntax error at or near "rows"
select 1 fetch first +1 rows only;  -- works
select 1 offset -1 rows;  -- ERROR:  syntax error at or near "rows"
select 1 fetch first -1 rows only;  -- ERROR:  LIMIT must not be negative

New behavior:
select 1 offset +1 rows;  -- works
select 1 fetch first +1 rows only;  -- works
select 1 offset -1 rows;  -- ERROR:  OFFSET must not be negative
select 1 fetch first -1 rows only;  -- ERROR:  LIMIT must not be negative

-- 
Andrew (irc:RhodiumToad)

  10. Re: Fix for FETCH FIRST syntax problems

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-05-20T11:07:04Z

    On 20/05/18 01:41, Tom Lane wrote:
> Vik Fearing <vik.fearing@2ndquadrant.com> writes:
>> On 20/05/18 00:57, Tom Lane wrote:
>> I'm +1 for backpatching it.  It may be operating as designed by PeterE
>> ten years ago, but it's not operating as designed by the SQL standard.
> 
> By that argument, *anyplace* where we're missing a SQL-spec feature
> is a back-patchable bug.  I don't buy it.

Only features we claim to support.  I obviously wouldn't consider
backpatching ASSERTIONs, for example.

> It may be that this fix is simple and safe enough that the risk/reward
> tradeoff favors back-patching, but I think you have to argue it as a
> favorable tradeoff rather than just saying "this isn't per standard".
> Consider: if Andrew had completely rewritten gram.y to get the same
> visible effect, would you think that was back-patchable?

Is the decision to backpatch based on behavior, or code churn?
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  11. Re: Fix for FETCH FIRST syntax problems

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2018-05-20T13:38:50Z

    On 20/05/18 05:25, Andrew Gierth wrote:
> +select_fetch_first_value:
> +			c_expr									{ $$ = $1; }
> +			| '+' I_or_F_const
> +				{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "+", NULL, $2, @1); }
> +			| '-' I_or_F_const
> +				{ $$ = doNegate($2, @1); }

I think there should be a comment for why you're accepting FCONST when
the value has to be integral.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  12. Re: Fix for FETCH FIRST syntax problems

    David Fetter <david@fetter.org> — 2018-05-20T19:26:50Z

    On Sun, May 20, 2018 at 01:39:27AM +0100, Andrew Gierth wrote:
> >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
>  >> I'm +1 for backpatching it. It may be operating as designed by
>  >> PeterE ten years ago, but it's not operating as designed by the SQL
>  >> standard.
> 
>  Tom> By that argument, *anyplace* where we're missing a SQL-spec
>  Tom> feature is a back-patchable bug. I don't buy it.
> 
> But this is a feature we already claimed to actually support (it's
> listed in sql_features with a bunch of unqualified YES entries), but in
> fact doesn't work properly.

This looks like a bug fix to me, for what it's worth.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

  13. Re: Fix for FETCH FIRST syntax problems

    Peter Geoghegan <pg@bowt.ie> — 2018-05-20T20:13:10Z

    On Sat, May 19, 2018 at 4:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It may be that this fix is simple and safe enough that the risk/reward
> tradeoff favors back-patching, but I think you have to argue it as a
> favorable tradeoff rather than just saying "this isn't per standard".
> Consider: if Andrew had completely rewritten gram.y to get the same
> visible effect, would you think that was back-patchable?

I strongly agree with the general principle that back-patching a bug
fix needs to have a benefit that outweighs its cost. There have been
cases where we chose to not back-patch an unambiguous bug fix even
though it was clear that incorrect user-visible behavior remained.
Conversely, there have been cases where we back-patched a commit that
was originally introduced as a performance feature.

Whether or not Andrew's patch is formally classified as a bug fix is
subjective. I'm inclined to accept it as a bug fix, but I also think
that it shouldn't matter very much. The practical implication is that
I don't think it's completely out of the question to back-patch, but
AFAICT nobody else thinks it's out of the question anyway. Why bother
debating something that's inconsequential?

FWIW, I am neutral on the important question of whether or not this
patch should actually be back-patched.

-- 
Peter Geoghegan

  14. Re: Fix for FETCH FIRST syntax problems

    David G. Johnston <david.g.johnston@gmail.com> — 2018-05-20T20:38:18Z

    On Sun, May 20, 2018 at 1:13 PM, Peter Geoghegan <pg@bowt.ie> wrote:

> There have been
> cases where we chose to not back-patch an unambiguous bug fix even
> though it was clear that incorrect user-visible behavior remained.
>

​The risk here is significantly reduced since the existing user-visible
behavior is an error which presumably no one is relying upon.  Between that
and being able to conform to the standard syntax for a long-standing
feature I would say the benefit outweighs the cost and risk.

+0.5 to back-patching

David J.
​

  15. Re: Fix for FETCH FIRST syntax problems

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-05-20T21:16:45Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> ​The risk here is significantly reduced since the existing user-visible
> behavior is an error which presumably no one is relying upon.  Between that
> and being able to conform to the standard syntax for a long-standing
> feature I would say the benefit outweighs the cost and risk.

The risk you're ignoring is that this patch will break something that
*did* work before.  Given that the first version did exactly that,
I do not think that risk should be considered negligible.  I'm going
to change my vote for back-patching from -0.5 to -1.

			regards, tom lane

  16. Re: Fix for FETCH FIRST syntax problems

    Stephen Frost <sfrost@snowman.net> — 2018-05-20T22:56:28Z

    Greetings,

* Peter Geoghegan (pg@bowt.ie) wrote:
> Whether or not Andrew's patch is formally classified as a bug fix is
> subjective. I'm inclined to accept it as a bug fix, but I also think
> that it shouldn't matter very much. The practical implication is that
> I don't think it's completely out of the question to back-patch, but
> AFAICT nobody else thinks it's out of the question anyway. Why bother
> debating something that's inconsequential?

Just to be clear, based on what I saw on IRC, this specifically came out
of someone complaining that it didn't work and caused difficulty for
them.  As such, my inclination on this would be to back-patch it, but
we'd need to be sure to test it and be confident that it won't break
things which worked before.

Thanks!

Stephen

  17. Re: Fix for FETCH FIRST syntax problems

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2018-05-21T00:38:36Z

    >>>>> "Stephen" == Stephen Frost <sfrost@snowman.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)

  18. Re: Fix for FETCH FIRST syntax problems

    Robert Haas <robertmhaas@gmail.com> — 2018-05-22T12:59:17Z

    On Sun, May 20, 2018 at 5:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> The risk here is significantly reduced since the existing user-visible
>> behavior is an error which presumably no one is relying upon.  Between that
>> and being able to conform to the standard syntax for a long-standing
>> feature I would say the benefit outweighs the cost and risk.
>
> The risk you're ignoring is that this patch will break something that
> *did* work before.  Given that the first version did exactly that,
> I do not think that risk should be considered negligible.  I'm going
> to change my vote for back-patching from -0.5 to -1.

I'm also -1 for back-patching, although it seems that the ship has
already sailed.  I don't think that the failure of something to work
that could have been made to work if the original feature author had
tried harder rises to the level of a bug.  If we start routinely
back-patching things that fall into that category, we will certainly
manage to destabilize older releases on a regular basis.

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

  19. Re: Fix for FETCH FIRST syntax problems

    David G. Johnston <david.g.johnston@gmail.com> — 2018-05-22T14:22:20Z

    On Tue, May 22, 2018 at 5:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> If we start routinely
> back-patching things that fall into that category, we will certainly
> manage to destabilize older releases on a regular basis.
>

Just because something is bad if done in excess doesn't mean specific
moderate partaking is bad too.

We actually did backpatch the NaN stuff and reverted that because, for me,
it was a silent change of functioning behavior.​  I find the decision to
back-patch this syntax oversight considerably more obvious than that one
was.

David J.