Date: Wed, 27 Nov 2019 18:36:50 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Surafel Temesgen <surafel3000@gmail.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Andrew Gierth <andrew@tao11.riddles.org.uk>
Subject: Re: FETCH FIRST clause WITH TIES option
Message-ID: <20191127213650.GA17453@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALAY4q8y3sLEEqWmLcLT0u5pdiebCdQd8BVuWbpLfqBYJ8zujQ@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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 &&
                    node->position - node->offset >= node->count)
                {
                    if (node->limitOption == LIMIT_OPTION_COUNT)
                    {
                        node->lstate = LIMIT_WINDOWEND;
                        return NULL;
                    }
                    else
                    {
                        node->lstate = LIMIT_WINDOWEND_TIES;
                        /* fall-through */
                    }
                }
                else
                    ...

I've been playing with this a little more, and I think you've overlooked
a few places in ExecLimit that need to be changed.  In the regression
database, I tried this:

55432 13devel 17282=# begin;
BEGIN
55432 13devel 17282=# declare c1 cursor for select * from int8_tbl order by q1 fetch first 2 rows with ties;
DECLARE CURSOR
55432 13devel 17282=# fetch all in c1;
 q1  │        q2        
─────┼──────────────────
 123 │              456
 123 │ 4567890123456789
(2 filas)

55432 13devel 17282=# fetch all in c1;
 q1 │ q2 
────┼────
(0 filas)

55432 13devel 17282=# fetch forward all in c1;
 q1 │ q2 
────┼────
(0 filas)

So far so good .. things look normal.  But here's where things get
crazy:

55432 13devel 17282=# fetch backward all in c1;
        q1        │        q2        
──────────────────┼──────────────────
 4567890123456789 │              123
              123 │ 4567890123456789
(2 filas)

(huh???)

55432 13devel 17282=# fetch backward all in c1;
 q1 │ q2 
────┼────
(0 filas)

Okay -- ignoring the fact that we got a row that should not be in the
result, we've exhausted the cursor going back.  Let's scan it again from
the start:

55432 13devel 17282=# fetch forward all in c1;
        q1        │        q2         
──────────────────┼───────────────────
              123 │  4567890123456789
 4567890123456789 │               123
 4567890123456789 │  4567890123456789
 4567890123456789 │ -4567890123456789
(4 filas)

This is just crazy.

I think you need to stare a that thing a little harder.

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