Re: FETCH FIRST clause PERCENT option

On 2/1/19 12:10 PM, Surafel Temesgen wrote:
> here is a rebased version of  previous  patch with planner
> comment included
> 

It's really hard to say which comment is that ..

FWIW I find the changes in nodeLimit lacking sufficient comments. The
comments present are somewhat obvious - what we need are comments
explaining why things happen. For example the LIMIT_INITIAL now includes
this chunk of code:

    case LIMIT_INITIAL:

        if (node->limitOption == PERCENTAGE)
        {

        /*
         * Find all rows the plan will return.
         */
            for (;;)
            {
                slot = ExecProcNode(outerPlan);
                if (TupIsNull(slot))
                {
                    break;
                }
                tuplestore_puttupleslot(node->totalTuple, slot);
            }
        }

Ignoring the fact that the comment is incorrectly indented, it states a
rather obvious fact - that we fetch all rows from a plan and stash them
into a tuplestore. What is missing is comment explaining why we need to
do that.

This applies to other changes in nodeLimit too, and elsewhere.

Another detail is that we generally leave a free line before "if", i.e.
instead of

    }
    if (node->limitOption == PERCENTAGE)
    {

it should be

    }

    if (node->limitOption == PERCENTAGE)
    {

because it's fairly easy to misread as "else if". Even better, there
should be a comment before the "if" explaining what the branch does.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services