Re: FETCH FIRST clause PERCENT option

Sorry for the dealy. I started to look this.

At Fri, 25 Sep 2020 12:25:24 +0300, Surafel Temesgen <surafel3000(at)gmail(dot)com> wrote in
> Hi Michael
> On Thu, Sep 24, 2020 at 6:58 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> > On Mon, Aug 10, 2020 at 01:23:44PM +0300, Surafel Temesgen wrote:
> > > I also Implement PERCENT WITH TIES option. patch is attached
> > > i don't start a new tread because the patches share common code
> >
> > This fails to apply per the CF bot. Could you send a rebase?

This still applies on the master HEAD.

percent-incremental-v11.patch

The existing nodeLimit passes the slot of the subnode to the
caller. but this patch changes that behavior. You added a new function
to tuplestore.c not to store a minimal tuple into the slot that passed
from subnode, but we should refrain from scribbling on the slot passed
from the subnode. Instead, the PERCENT path of the limit node should
use its own ResultTupleSlot for the purpose. See nodeSort for a
concrete example.

+ LIMIT_OPTION_PER_WITH_TIES, /* FETCH FIRST... PERCENT WITH TIES */

That name is a bit hard to read. We should spell it with complete
words.

case LIMIT_INWINDOW:
...
+ if (IsPercentOption(node->limitOption) && node->backwardPosition
...
+ if (IsPercentOption(node->limitOption) && node->reachEnd)
...
+ if (IsPercentOption(node->limitOption))

I think we can use separate lstate state for each condition above
since IsPercentOption() gives a constant result through the execution
time. For example, LIMIT_PERCENT_TUPLESLOT_NOT_FILLED and
LIMIT_PERCENT_TUPLESLOT_FILLED and some derived states similar to the
non-percent path. I *feel* that makes code simpler.

What do you think about this?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center