Re: Re: FETCH FIRST clause WITH TIES option

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