Re: FETCH FIRST clause PERCENT option - Mailing list pgsql-hackers

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.