agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Surafel Temesgen <surafel3000@gmail.com>
Cc: David Steele <david@pgmasters.net>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: andrew@tao11.riddles.org.uk, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Re: FETCH FIRST clause WITH TIES option
Date: Sun, 31 Mar 2019 01:42:25 +0100
Message-ID: <20190331004225.GB10804@development> (raw)
In-Reply-To: <20190331001446.GA10804@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>
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
view thread (61+ messages) latest in thread
Message-ID: <20190331004225.GB10804@development>
Permalink: ../20190331004225.GB10804@development/
Also on: postgresql.org/message-id/20190331004225.GB10804@development
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, surafel3000@gmail.com, david@pgmasters.net, michael@paquier.xyz, robertmhaas@gmail.com
Subject: Re: Re: FETCH FIRST clause WITH TIES option
In-Reply-To: <20190331004225.GB10804@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox