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:14:46
Message-ID: 20190331001446.GA10804@development
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.
>

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.

regards

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

Attachment Content-Type Size
0001-fetch_first_with_ties_v7.patch text/plain 30.4 KB
0002-fixes-and-comments.patch text/plain 7.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2019-03-31 00:42:25 Re: Re: FETCH FIRST clause WITH TIES option
Previous Message Haoran Yu 2019-03-30 23:12:40 Re: GSoC proposal for pgAdmin 4 bytea support