Re: FETCH FIRST clause PERCENT option

Hello.

At Thu, 1 Aug 2019 15:54:11 +0300, Surafel Temesgen <surafel3000@gmail.com> wrote in
<CALAY4q_icXPTcAVxnPckpLGrZgER9-FWM0VPFvvOsPiJrnK6Dg@mail.gmail.com>
> > Other than that, we can rip the clause if it is 100%
> >
> >
> > You mean if PERCENT=100 it should short circuit and run the query
> > normally?  I like that.
> >
> 
> The patch did not did it automatically. Its query writer obligation to do
> that currently

I have some comments.

This patch uses distinct parameters for exact number and
percentage. On the othe hand planner has a notion of
tuple_fraction covering the both. The same handling is also used
for tuple number estimation. Using the same scheme will make
things far simpler. See the comment of grouping_planner().

In executor part, addition to LimiteState.position, this patch
uses LimiteState.backwardPosition to count how many tuples we're
back from the end of the current tuplestore. But things will get
simpler by just asking the tuplestore for the number of holding
tuples.


+    slot = node->subSlot;

Why is this needed? The variable is properly set before use and
the assignment is bogus in the first place.

The new code block in LIMIT_RESCAN in ExecLimit is useless since
it is exatctly the same with existing code block. Why didn't you
use the existing if block?

>                if (node->limitOption == PERCENTAGE)
+                {
+                    node->perExactCount = ceil(node->percent * node->position / 100.0);
+
+

node->position is the number of tuples returned to upper node so
far (not the number of tuples this node has read from the lower
node so far).  I don't understand what the expression means.


+                    if (node->perExactCount == node->perExactCount + 1)
+                        node->perExactCount++;

What? The condition never be true. As the result, the following
if block below won't run.

>                /*
+                 * Return the tuple up to the number of exact count in OFFSET
+                 * clause without percentage value consideration.
+                 */
+                if (node->perExactCount > 0)
+                {
+




+            /*
+             * We may needed this tuple in backward scan so put it into
+             * tuplestore.
+             */
+            if (node->limitOption == PERCENTAGE)
+            {
+                tuplestore_puttupleslot(node->tupleStore, slot);
+                tuplestore_advance(node->tupleStore, true);
+            }

"needed"->"need" ? The comment says that this is needed for
backward scan, but it is actually required even in forward
scan. More to the point, tuplestore_advance lacks comment.

Anyway, the code in LIMIT_RESCAN is broken in some ways. For
example, if it is used as the inner scan of a NestLoop, the
tuplestore accumulates tuples by every scan. You will see that
the tuplestore stores up to 1000 tuples (10 times of the inner)
by the following query.

create table t0 as (select a from generate_series(0, 99) a);
create table t1 as (select a from generate_series(0, 9) a);
analyze;
set enable_hashjoin to false;
set enable_mergejoin to false;
set enable_material to false;
explain analyze select t.*, t1.* from (select a from t0 fetch first 10 percent rows only) as t join t1 on (t.a =
t1.a);
                               QUERY PLAN                             
-------------------------------------------------------------------------------
 Nested Loop  (cost=0.20..7.35 rows=10 width=8) (actual ...)
   ->  Seq Scan on t1  (cost=0.00..1.10 rows=10 width=4) (actual ...)
   ->  Limit  (cost=0.20..0.40 rows=10 width=4) (actual ...)
         ->  Seq Scan on t0  (cost=0.00..2.00 rows=100 width=4) (actual ...)

That's it for now.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center