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