Re: Re: FETCH FIRST clause WITH TIES option

From: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
To: Surafel Temesgen <surafel3000(at)gmail(dot)com>
Cc: David Steele <david(at)pgmasters(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, Robert Haas <robertmhaas(at)gmail(dot)com>, andrew(at)tao11(dot)riddles(dot)org(dot)uk, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Re: FETCH FIRST clause WITH TIES option
Date: 2019-03-31 00:42:25
Message-ID: 20190331004225.GB10804@development
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(at)pgmasters(dot)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.
>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".


Tomas Vondra
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2019-03-31 00:50:53 Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Previous Message Tomas Vondra 2019-03-31 00:14:46 Re: Re: FETCH FIRST clause WITH TIES option