Re: Re: FETCH FIRST clause WITH TIES option

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