Re: FETCH FIRST clause WITH TIES option

On 2020-Apr-08, Andrew Gierth wrote: > > "Alvaro" == Alvaro Herrera writes: > > >> This needs to be fixed in ruleutils, IMO, not by changing what the > >> grammar accepts. > > Alvaro> Fair. I didn't change what the grammar accepts. I ended up only > Alvaro> throwing an error in parse

Re: FETCH FIRST clause WITH TIES option

> "Alvaro" == Alvaro Herrera writes: >> This needs to be fixed in ruleutils, IMO, not by changing what the >> grammar accepts. Alvaro> Fair. I didn't change what the grammar accepts. I ended up only Alvaro> throwing an error in parse analysis when a bare NULL const is Alvaro> seen.

Re: FETCH FIRST clause WITH TIES option

On 2020-Apr-08, Andrew Gierth wrote: > > "Alvaro" == Alvaro Herrera writes: > > Alvaro> It turns out that the SQL standard is much more limited in what > Alvaro> it will accept there. But our grammar (what we'll accept for > Alvaro> the ancient LIMIT clause) is very lenient -- it'll take

Re: FETCH FIRST clause WITH TIES option

> "Alvaro" == Alvaro Herrera writes: Alvaro> It turns out that the SQL standard is much more limited in what Alvaro> it will accept there. But our grammar (what we'll accept for Alvaro> the ancient LIMIT clause) is very lenient -- it'll take just Alvaro> any expression. I thought about

Re: FETCH FIRST clause WITH TIES option

Hello On 2020-Apr-07, Andres Freund wrote: > On 2020-04-07 16:36:54 -0400, Alvaro Herrera wrote: > > Pushed, with some additional changes. > > This triggers a new warning for me (gcc-10): > /home/andres/src/postgresql/src/backend/executor/nodeLimit.c: In function > ‘ExecLimit’: >

Re: FETCH FIRST clause WITH TIES option

Hi, On 2020-04-07 16:36:54 -0400, Alvaro Herrera wrote: > Pushed, with some additional changes. This triggers a new warning for me (gcc-10): /home/andres/src/postgresql/src/backend/executor/nodeLimit.c: In function ‘ExecLimit’: /home/andres/src/postgresql/src/backend/executor/nodeLimit.c:136:7:

Re: FETCH FIRST clause WITH TIES option

Pushed, with some additional changes. So *of course* when I add tests to verify that ruleutils, I find a case that does not work properly -- meaning, you get a view that pg_dump emits in a way that won't be accepted: CREATE VIEW limit_thousand_v_3 AS SELECT thousand FROM onek WHERE thousand <

Re: FETCH FIRST clause WITH TIES option

Some ruleutils.c code added by this patch is not covered by tests: 5246 : /* Add the LIMIT clause if given */ 52471115 : if (query->limitOffset != NULL) 5248 : { 5249 0 : appendContextKeyword(context, " OFFSET ",

Re: FETCH FIRST clause WITH TIES option

There was a minor conflict in planmain.h. Here's a refreshed version. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >From 51dee439e5165efad186c48d1a3b9119f90542e8 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera

Re: FETCH FIRST clause WITH TIES option

On Thu, Nov 28, 2019 at 5:49 PM Alvaro Herrera wrote: > On 2019-Nov-28, Surafel Temesgen wrote: > > > On Thu, Nov 28, 2019 at 12:36 AM Alvaro Herrera < > alvhe...@2ndquadrant.com> > > wrote: > > > > > I think you should add a /* fall-though */ comment after changing > state. > > > Like this

Re: FETCH FIRST clause WITH TIES option

On Fri, Nov 29, 2019 at 05:19:00AM +, Andrew Gierth wrote: "Alvaro" == Alvaro Herrera writes: Alvaro> First, I noticed that there's a significant unanswered issue Alvaro> from Andrew Gierth about this using a completely different Alvaro> mechanism, namely an implicit window function.

Re: FETCH FIRST clause WITH TIES option

> "Alvaro" == Alvaro Herrera writes: Alvaro> First, I noticed that there's a significant unanswered issue Alvaro> from Andrew Gierth about this using a completely different Alvaro> mechanism, namely an implicit window function. Robert was Alvaro> concerned about the performance of

Re: FETCH FIRST clause WITH TIES option

On 2019-Nov-28, Surafel Temesgen wrote: > On Thu, Nov 28, 2019 at 12:36 AM Alvaro Herrera > wrote: > > > I think you should add a /* fall-though */ comment after changing state. > > Like this (this flow seems clearer; also DRY): > > > > if (!node->noCount && > >

Re: FETCH FIRST clause WITH TIES option

On Thu, Nov 28, 2019 at 12:36 AM Alvaro Herrera wrote: > Thanks. > > (I would suggest renaming the new state LIMIT_WINDOWEND_TIES, because it > seems to convey what it does a little better.) > > I think you should add a /* fall-though */ comment after changing state. > Like this (this flow seems

Re: FETCH FIRST clause WITH TIES option

Thanks. (I would suggest renaming the new state LIMIT_WINDOWEND_TIES, because it seems to convey what it does a little better.) I think you should add a /* fall-though */ comment after changing state. Like this (this flow seems clearer; also DRY): if (!node->noCount &&

Re: FETCH FIRST clause WITH TIES option

On Tue, Nov 26, 2019 at 6:09 PM Alvaro Herrera wrote: > I rebased this patch, and my proposed changes are in 0002. > Thank you > > Looking at the changes in ExecLimit, I'm wondering if it would be better > to add a new state to the state machine there -- instead of doing all > the work in

Re: FETCH FIRST clause WITH TIES option

I rebased this patch, and my proposed changes are in 0002. Looking at the changes in ExecLimit, I'm wondering if it would be better to add a new state to the state machine there -- instead of doing all the work in duplicative code in the LIMIT_INWINDOW case, have that one only save the current

Re: FETCH FIRST clause WITH TIES option

(Prior to posting this delta patch, the CF bot appeared happy with this patch.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: FETCH FIRST clause WITH TIES option

On 2019-Nov-11, Alvaro Herrera wrote: > I'm not sure the proposed changes to gram.y are all that great, though. Here's a proposed simplification of the gram.y changes. There are two things here: 1. cosmetic: we don't need the LimitClause struct; we can use just SelectLimit, and return that

Re: FETCH FIRST clause WITH TIES option

On Mon, Nov 11, 2019 at 5:56 PM Alvaro Herrera wrote: > First, I noticed that there's a significant unanswered issue from Andrew > Gierth about this using a completely different mechanism, namely an > implicit window function. I see that Robert was concerned about the > performance of Andrew's

Re: FETCH FIRST clause WITH TIES option

(BTW another small issue in the patch is the comments in gram.y that talk about ONLY being a reserved word about the SQL:2008 syntax; ISTM that that comment should now mention both ONLY as well as WITH being fully reserved words.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/

Re: FETCH FIRST clause WITH TIES option

Looking at this patch again. (Attached v13 fixes a typo in ruleutils, fixes a bogus edit in the SGML docs plus minor rewording, and is rebased to current master). First, I noticed that there's a significant unanswered issue from Andrew Gierth about this using a completely different mechanism,

Re: FETCH FIRST clause WITH TIES option

Alvaro Herrera writes: > create table w (a point); > # select * from w order by a fetch first 2 rows with ties; > ERROR: could not identify an ordering operator for type point > LINE 1: select * from w order by a fetch first 2 rows with ties; > ^ > HINT: Use

Re: FETCH FIRST clause WITH TIES option

Hmm, create table w (a point); # select * from w order by a fetch first 2 rows with ties; ERROR: could not identify an ordering operator for type point LINE 1: select * from w order by a fetch first 2 rows with ties; ^ HINT: Use an explicit ordering operator

Re: FETCH FIRST clause WITH TIES option

On Fri, Sep 6, 2019 at 5:07 PM Alvaro Herrera from 2ndQuadrant < alvhe...@alvh.no-ip.org> wrote: > On 2019-Sep-06, Surafel Temesgen wrote: > > > > ... yet this doesn't appear to have resulted in any change in the code, > > > or I just missed it. Are you going to update the patch per that? > > >

Re: FETCH FIRST clause WITH TIES option

On 2019-Sep-06, Surafel Temesgen wrote: > > ... yet this doesn't appear to have resulted in any change in the code, > > or I just missed it. Are you going to update the patch per that? > > Its already done in v9 of the patch attached by Tomas Oh, I missed that. Great, so we're expecting some

Re: FETCH FIRST clause WITH TIES option

Hi Alvaro, On Fri, Sep 6, 2019 at 1:52 AM Alvaro Herrera from 2ndQuadrant < alvhe...@alvh.no-ip.org> wrote: > As Tom just said in the thread for PERCENT, the gram.y changes need a > better representation. Also, rename EXACT_NUMBER, per that thread. > > As far as I can tell, this concerns feature

Re: FETCH FIRST clause WITH TIES option

As Tom just said in the thread for PERCENT, the gram.y changes need a better representation. Also, rename EXACT_NUMBER, per that thread. As far as I can tell, this concerns feature F867. I think we should mark that as supported after this patch -- please edit

Re: FETCH FIRST clause WITH TIES option

Hi Erik, Thank you for looking at it. Can you have a look? > > It is because of the patch didn't handle that edge case. attache patch contain a fix for it regards Surafel diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 06d611b64c..e83d309c5b 100644 ---

Re: FETCH FIRST clause WITH TIES option

On 2019-07-01 19:38, Surafel Temesgen wrote: Thank you for informing. attach is a rebased patch against current master [...] [fetch_first_with_ties_v10.patch] Hi Surafel, The patch applies OK, make check is OK, compiles OK. But I get: TRAP: FailedAssertion("!(!(((slot)->tts_flags & (1 <<

Re: FETCH FIRST clause WITH TIES option

Hi Thomas On Mon, Jul 1, 2019 at 1:31 PM Thomas Munro wrote: > On Mon, Apr 8, 2019 at 8:26 PM Surafel Temesgen > wrote: > > agree > > Hi Surafel, > > A new Commitfest is starting. Can you please post a fresh rebase of this > patch? > > Thank you for informing. attach is a rebased patch against

Re: FETCH FIRST clause WITH TIES option

On Mon, Apr 8, 2019 at 8:26 PM Surafel Temesgen wrote: > agree Hi Surafel, A new Commitfest is starting. Can you please post a fresh rebase of this patch? Thanks, -- Thomas Munro https://enterprisedb.com

Re: FETCH FIRST clause WITH TIES option

On Sun, Apr 7, 2019 at 1:39 AM Tomas Vondra wrote: > > 1) I've removed the costing changes. As Tom mentions elsewhere in this > thread, that's probably not needed for v1 and it's true those estimates > are probably somewhat sketchy anyway. > > > 2) v8 did this (per my comment): > > -

Re: FETCH FIRST clause WITH TIES option

Hi Surafel, I've been looking at the patch with the intent to commit it, but once again I ran into stuff that seems suspicious to me but am not familiar with enough to say if it's OK. I'm sorry about that :-( First, a couple of tweaks in the attached v9 of the patch: 1) I've removed the

Re: FETCH FIRST clause WITH TIES option

On Wed, Apr 03, 2019 at 03:08:05PM -0400, Tom Lane wrote: Tomas Vondra writes: I've tried to fix the merge conflict (essentially by moving some of the code to adjust_limit_rows_costs(), but I'm wondering if the code added to create_limit_path is actually correct ... Firstly, this seriously

Re: FETCH FIRST clause WITH TIES option

Tomas Vondra writes: > I've tried to fix the merge conflict (essentially by moving some of the > code to adjust_limit_rows_costs(), but I'm wondering if the code added to > create_limit_path is actually correct > ... > Firstly, this seriously needs some comment explaining why we do this. I've

Re: FETCH FIRST clause WITH TIES option

Hi, Unfortunately this got broken again, this time by aef65db676 :-( I've tried to fix the merge conflict (essentially by moving some of the code to adjust_limit_rows_costs(), but I'm wondering if the code added to create_limit_path is actually correct if (count_est != 0) { double

Re: Re: FETCH FIRST clause WITH TIES option

On Sun, Mar 31, 2019 at 3:14 AM Tomas Vondra wrote: > > > Hi, > > I got to look at the patch today, with the intent to commit, but sadly I > ran into a couple of minor issues that I don't feel comfortable fixing > on my own. Attached is a patch highlighling some of the places (0001 is > your v7

Re: Re: FETCH FIRST clause WITH TIES option

On Sun, Mar 31, 2019 at 01:14:46AM +0100, Tomas Vondra wrote: On Fri, Mar 29, 2019 at 01:56:48AM +0100, Tomas Vondra wrote: On Tue, Mar 26, 2019 at 10:46:00AM +0300, Surafel Temesgen wrote: On Mon, Mar 25, 2019 at 11:56 AM David Steele wrote: This patch no longer passes testing so marked

Re: Re: FETCH FIRST clause WITH TIES option

On Fri, Mar 29, 2019 at 01:56:48AM +0100, Tomas Vondra wrote: On Tue, Mar 26, 2019 at 10:46:00AM +0300, Surafel Temesgen wrote: On Mon, Mar 25, 2019 at 11:56 AM David Steele wrote: This patch no longer passes testing so marked Waiting on Author. Thank you for informing. Fixed Thanks

Re: Re: FETCH FIRST clause WITH TIES option

On Tue, Mar 26, 2019 at 10:46:00AM +0300, Surafel Temesgen wrote: On Mon, Mar 25, 2019 at 11:56 AM David Steele wrote: This patch no longer passes testing so marked Waiting on Author. Thank you for informing. Fixed Thanks for the updated patch. I do have this on my list of patches that

Re: Re: FETCH FIRST clause WITH TIES option

On Mon, Mar 25, 2019 at 11:56 AM David Steele wrote: > This patch no longer passes testing so marked Waiting on Author. > > Thank you for informing. Fixed diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 06d611b64c..b3b045ea87 100644 ---

Re: Re: FETCH FIRST clause WITH TIES option

On 2/4/19 2:37 PM, Surafel Temesgen wrote: On Mon, Feb 4, 2019 at 8:29 AM Michael Paquier > wrote: This patch needs a rebase because of conflicts done recently for pluggable storage, so moved to next CF, waiting on author. -- Thank you . rebased

Re: FETCH FIRST clause WITH TIES option

On Mon, Feb 4, 2019 at 8:29 AM Michael Paquier wrote: > > This patch needs a rebase because of conflicts done recently for > pluggable storage, so moved to next CF, waiting on author. > -- > Thank you . rebased against current master regards Surafel diff --git a/doc/src/sgml/ref/select.sgml

Re: FETCH FIRST clause WITH TIES option

On Wed, Jan 16, 2019 at 11:45:46AM +0300, Surafel Temesgen wrote: > The attached patch use your suggestion uptread This patch needs a rebase because of conflicts done recently for pluggable storage, so moved to next CF, waiting on author. -- Michael signature.asc Description: PGP signature

Re: FETCH FIRST clause WITH TIES option

On Tue, Jan 15, 2019 at 2:51 PM Tomas Vondra wrote: > What do you mean by "raw statistic"? > I mean without further calculation to consider other operation > > IMHO the best thing you can do is call estimate_num_groups() and combine > that with the number of input rows. That shall benefit

Re: FETCH FIRST clause WITH TIES option

On 1/15/19 11:07 AM, Surafel Temesgen wrote: > > > On Wed, Jan 2, 2019 at 6:19 PM Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> wrote: > > After looking at the "FETCH FIRST ... PERCENT" patch, I wonder if this > patch should tweak estimates in some way. Currently, the

Re: FETCH FIRST clause WITH TIES option

On Wed, Jan 2, 2019 at 6:19 PM Tomas Vondra wrote: > After looking at the "FETCH FIRST ... PERCENT" patch, I wonder if this > patch should tweak estimates in some way. Currently, the cardinality > estimate is the same as for plain LIMIT, using the requested number of > rows. But let's say there

Re: FETCH FIRST clause WITH TIES option

On 1/2/19 11:51 AM, Surafel Temesgen wrote: > > > On Tue, Jan 1, 2019 at 8:38 PM Tomas Vondra > mailto:tomas.von...@2ndquadrant.com>> wrote: > > > > >     The attached patch include all the comment given by Tomas and i > >     check sql standard about LIMIT and this feature >

Re: FETCH FIRST clause WITH TIES option

On Tue, Jan 1, 2019 at 8:38 PM Tomas Vondra wrote: > > > > The attached patch include all the comment given by Tomas and i > > check sql standard about LIMIT and this feature > > > > Unfortunately, it seems the "limit" regression tests fail for some > reason - the output mismatches the

Re: FETCH FIRST clause WITH TIES option

Hi, On 11/24/18 10:28 AM, Surafel Temesgen wrote: > Attach is rebased patch against the current master > regards > Surafel > > On Thu, Nov 1, 2018 at 2:28 PM Surafel Temesgen > wrote: > > hi, > > The attached patch include all the comment given by Tomas

Re: FETCH FIRST clause WITH TIES option

Attach is rebased patch against the current master regards Surafel On Thu, Nov 1, 2018 at 2:28 PM Surafel Temesgen wrote: > hi, > > The attached patch include all the comment given by Tomas and i check sql > standard about LIMIT and this feature > > it did not say anything about it but I think

Re: FETCH FIRST clause WITH TIES option

hi, The attached patch include all the comment given by Tomas and i check sql standard about LIMIT and this feature it did not say anything about it but I think its good idea to include it to LIMIT too and I will add it if we have consensus on it. regards surafel diff --git

Re: FETCH FIRST clause WITH TIES option

On 10/31/2018 06:17 PM, Robert Haas wrote: On Mon, Oct 29, 2018 at 12:48 PM Andrew Gierth wrote: Then FETCH FIRST N WITH TIES becomes "stop when the expression rank() over (order by ...) <= N is no longer true" (where the ... is the existing top level order by) Wouldn't that be wicked

Re: FETCH FIRST clause WITH TIES option

On Mon, Oct 29, 2018 at 12:48 PM Andrew Gierth wrote: > Then FETCH FIRST N WITH TIES becomes "stop when the expression > rank() over (order by ...) <= N > is no longer true" (where the ... is the existing top level order by) Wouldn't that be wicked expensive compared to something hard-coded

Re: FETCH FIRST clause WITH TIES option

On 10/29/2018 05:47 PM, Andrew Gierth wrote: "Tomas" == Tomas Vondra writes: >> I still think that this is the wrong approach. Implementing WITH >> TIES and PERCENT together using an implicit window function call >> kills two birds with one very small stone (the only executor change

Re: FETCH FIRST clause WITH TIES option

> "Tomas" == Tomas Vondra writes: >> I still think that this is the wrong approach. Implementing WITH >> TIES and PERCENT together using an implicit window function call >> kills two birds with one very small stone (the only executor change >> needed would be teaching LIMIT to be able to

Re: FETCH FIRST clause WITH TIES option

On 10/29/2018 04:17 PM, Andrew Gierth wrote: "Tomas" == Tomas Vondra writes: > On 10/26/2018 12:28 PM, Surafel Temesgen wrote: >> hello , >> >> The WITH TIES keyword is sql standard that specifies any peers of >> retained rows to retained in the result set too .which means >>

Re: FETCH FIRST clause WITH TIES option

> "Tomas" == Tomas Vondra writes: > On 10/26/2018 12:28 PM, Surafel Temesgen wrote: >> hello , >> >> The WITH TIES keyword is sql standard that specifies any peers of >> retained rows to retained in the result set too .which means >> according to ordering key the result set can

Re: FETCH FIRST clause WITH TIES option

Hello Surafel, On 10/26/2018 12:28 PM, Surafel Temesgen wrote: > hello , > > The WITH TIES keyword is sql standard that specifies any peers of > retained rows to retained in the result set too .which means > according to ordering key the result set can includes additional rows > which have