Date: Sun, 31 Mar 2019 01:42:25 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Surafel Temesgen <surafel3000@gmail.com>
Cc: David Steele <david@pgmasters.net>,
	Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>, andrew@tao11.riddles.org.uk,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Re: FETCH FIRST clause WITH TIES option
Message-ID: <20190331004225.GB10804@development>
References: <e5c5129c-2518-4a5d-fc28-0a080a08526f@2ndquadrant.com>
 <CALAY4q9Zd24E8OEBxOHfqQbp6u+xePXrFsnvM5xoy1pq1APUgw@mail.gmail.com>
 <c1430163-a5b0-2adb-c81c-29d1029cb8cf@2ndquadrant.com>
 <CALAY4q-+CKiFkgCL5VSK-9P8i5Z9uoQQh6ZEZLkjzsUT2mySsw@mail.gmail.com>
 <20190204052857.GP29064@paquier.xyz>
 <CALAY4q8vDyK4_jhFkXyWnJRcuWGgkwOzJTw-j2Dw0ddyTiZM4g@mail.gmail.com>
 <cd96a537-a302-48ef-b41b-2e247e156745@pgmasters.net>
 <CALAY4q8X-17+AFaAKZ+YX781b+pqjdU4S=z=ZUGi3QjyHpZ6hQ@mail.gmail.com>
 <20190329005648.GA1136@development>
 <20190331001446.GA10804@development>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20190331001446.GA10804@development>
User-Agent: Mutt/1.11.4+135 (aae3e555) (2019-03-21)
Precedence: bulk

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 <david@pgmasters.net> 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
>>I'd like to commit in this CF - likely tomorrow after one more round of
>>review, or so.
>>
>
>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 patch, to keep the cfbot happy).
>
>
>1) the docs documented this as
>
>  ... [ ONLY | WITH TIES ]
>
>but that's wrong, because it implies those options are optional (i.e.
>the user may not specify anything). That's not the case, exactly one
>of those options needs to be specified, so it should have been
>
>  ... { ONLY | WITH TIES }
>
>
>2) The comment in ExecLimit() needs to be updated to explain that WITH
>TIES changes the behavior.
>
>
>3) Minor code style issues (no space before * on comment lines, {}
>around single-line if statements, ...).
>
>
>4) The ExecLimit() does this
>
>   if (node->limitOption == WITH_TIES)
>       ExecCopySlot(node->last_slot, slot);
>
>but I think we only really need to do that for the last tuple in the
>window, no? Would it be a useful optimization?
>
>
>5) Two issues in _outLimit(). Firstly, when printing uniqCollations the
>code actually prints uniqOperators. Secondly, why does the code use
>these loops at all, instead of using WRITE_ATTRNUMBER_ARRAY and
>WRITE_OID_ARRAY, like other places? Perhaps there's an issue with empty
>arrays? I haven't tested this, but looking at the READ_ counterparts, I
>don't see why that would be the case.
>

Actually, two more minor comments:

6) There's some confusing naming - in plannodes.h the fields added to
the Limit node are called uniqSomething, but in other places the patch
uses sortSomething, ordSomething. I suggest more consistent naming.

7) The LimitOption enum has two items - WITH_ONLY and WITH_TIES. That's
a bit strange, because there's nothing like "WITH ONLY".


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services