From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | Surafel Temesgen <surafel3000(at)gmail(dot)com> |
Cc: | vik(dot)fearing(at)2ndquadrant(dot)com, Mark Dilger <hornschnorter(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, andrew(at)tao11(dot)riddles(dot)org(dot)uk |
Subject: | Re: FETCH FIRST clause PERCENT option |
Date: | 2019-02-09 23:32:59 |
Message-ID: | ca30c6c7-625a-a77a-5e5d-a14eb30d8ffa@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Julien Rouhaud | 2019-02-10 00:00:58 | Re: executor relation handling |
Previous Message | Tom Lane | 2019-02-09 23:31:49 | Re: executor relation handling |