From 7f5549614c423a1da060da5f7598e9d0836d03a8 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sun, 28 Oct 2018 18:38:47 +0100 Subject: [PATCH 2/2] review --- doc/src/sgml/ref/select.sgml | 6 ++-- src/backend/executor/nodeLimit.c | 52 +++++++++++++++++++-------------- src/backend/optimizer/plan/createplan.c | 25 +++++++--------- src/include/nodes/execnodes.h | 4 +-- src/include/nodes/parsenodes.h | 4 +-- src/include/nodes/plannodes.h | 2 +- src/include/nodes/relation.h | 2 +- 7 files changed, 49 insertions(+), 46 deletions(-) diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index c81ac04108..b649b1ca7a 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -1408,9 +1408,9 @@ FETCH { FIRST | NEXT } [ count ] { If count is omitted in a FETCH clause, it defaults to 1. ROW - WITH TIES option is Used to return two or more rows - that tie for last place in the limit results set according to ORDER BY - clause (ORDER BY clause must be specified in this case). + WITH TIES option is used to return two or more rows + that tie for last place in the limit results set according to ORDER BY + clause (ORDER BY clause must be specified in this case). and ROWS as well as FIRST and NEXT are noise words that don't influence the effects of these clauses. diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c index 93f8813972..724a8e09c1 100644 --- a/src/backend/executor/nodeLimit.c +++ b/src/backend/executor/nodeLimit.c @@ -132,7 +132,8 @@ ExecLimit(PlanState *pstate) * the state machine state to record having done so. */ if (!node->noCount && - node->position - node->offset >= node->count && node->limitOption == WITH_ONLY) + node->position - node->offset >= node->count && + node->limitOption == WITH_ONLY) { node->lstate = LIMIT_WINDOWEND; @@ -145,23 +146,24 @@ ExecLimit(PlanState *pstate) return NULL; } - else if (!node->noCount && - node->position - node->offset >= node->count && node->limitOption == WITH_TIES) + node->position - node->offset >= node->count && + node->limitOption == WITH_TIES) { /* - * Get next tuple from subplan, if any. - */ + * Get next tuple from subplan, if any. + */ slot = ExecProcNode(outerPlan); if (TupIsNull(slot)) { node->lstate = LIMIT_SUBPLANEOF; return NULL; } + /* - * Test if the new tuple and the last tuple match. - * If so we return the tuple. - */ + * Test if the new tuple and the last tuple match. + * If so we return the tuple. + */ econtext->ecxt_innertuple = slot; econtext->ecxt_outertuple = node->last_slot; if (ExecQualAndReset(node->eqfunction, econtext)) @@ -184,22 +186,25 @@ ExecLimit(PlanState *pstate) return NULL; } - }else - /* - * Get next tuple from subplan, if any. - */ - slot = ExecProcNode(outerPlan); - if (TupIsNull(slot)) - { - node->lstate = LIMIT_SUBPLANEOF; - return NULL; } - if (node->limitOption == WITH_TIES) + else /* XXX So what exactly should be part of this else branch? */ { - ExecCopySlot(node->last_slot, slot); + /* + * Get next tuple from subplan, if any. + */ + slot = ExecProcNode(outerPlan); + if (TupIsNull(slot)) + { + node->lstate = LIMIT_SUBPLANEOF; + return NULL; + } + if (node->limitOption == WITH_TIES) + { + ExecCopySlot(node->last_slot, slot); + } + node->subSlot = slot; + node->position++; } - node->subSlot = slot; - node->position++; } else { @@ -356,7 +361,7 @@ recompute_limits(LimitState *node) * previous time we got a different result. */ if(node->limitOption == WITH_ONLY) - ExecSetTupleBound(compute_tuples_needed(node), outerPlanState(node)); + ExecSetTupleBound(compute_tuples_needed(node), outerPlanState(node)); } /* @@ -433,6 +438,9 @@ ExecInitLimit(Limit *node, EState *estate, int eflags) */ limitstate->ps.ps_ProjInfo = NULL; + /* + * Initialize the equality evaluation, to detect ties. + */ if (node->limitOption == WITH_TIES) { TupleDesc scanDesc; diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 7e540849b4..62c05c7feb 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2435,6 +2435,9 @@ create_limit_plan(PlannerInfo *root, LimitPath *best_path, int flags) { Limit *plan; Plan *subplan; + int numsortkeys = 0; + AttrNumber *sortColIdx = NULL; + Oid *sortOperators = NULL; /* Limit doesn't project, so tlist requirements pass through */ subplan = create_plan_recurse(root, best_path->subpath, flags); @@ -2442,9 +2445,6 @@ create_limit_plan(PlannerInfo *root, LimitPath *best_path, int flags) if (best_path->limitOption == WITH_TIES) { Query *parse = root->parse; - int numsortkeys; - AttrNumber *sortColIdx; - Oid *sortOperators; ListCell *l; numsortkeys = list_length(parse->sortClause); @@ -2461,19 +2461,14 @@ create_limit_plan(PlannerInfo *root, LimitPath *best_path, int flags) sortOperators[numsortkeys] = sortcl->eqop; numsortkeys++; } - plan = make_limit(subplan, - best_path->limitOffset, - best_path->limitCount, - best_path->limitOption, - numsortkeys, sortColIdx, - sortOperators); } - else - plan = make_limit(subplan, - best_path->limitOffset, - best_path->limitCount, - best_path->limitOption, - 0, NULL, NULL); + + plan = make_limit(subplan, + best_path->limitOffset, + best_path->limitCount, + best_path->limitOption, + numsortkeys, sortColIdx, + sortOperators); copy_generic_path_info(&plan->plan, (Path *) best_path); diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 212820d886..6f15ce8ea4 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -2242,7 +2242,7 @@ typedef struct LimitState PlanState ps; /* its first field is NodeTag */ ExprState *limitOffset; /* OFFSET parameter, or NULL if none */ ExprState *limitCount; /* COUNT parameter, or NULL if none */ - LimitOption limitOption; /* limit specification type */ + LimitOption limitOption; /* limit specification type */ int64 offset; /* current OFFSET value */ int64 count; /* current COUNT, if any */ bool noCount; /* if true, ignore count */ @@ -2250,7 +2250,7 @@ typedef struct LimitState int64 position; /* 1-based index of last tuple returned */ TupleTableSlot *subSlot; /* tuple last obtained from subplan */ ExprState *eqfunction; /* tuple equality qual in case of WITH TIES OPTION */ - TupleTableSlot *last_slot; + TupleTableSlot *last_slot; /* slot for evaluation of ties */ } LimitState; #endif /* EXECNODES_H */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index ce10b55dcf..ce07a1a02d 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -159,7 +159,7 @@ typedef struct Query Node *limitOffset; /* # of result tuples to skip (int8 expr) */ Node *limitCount; /* # of result tuples to return (int8 expr) */ - LimitOption limitOption; /* limit type */ + LimitOption limitOption; /* LIMIT type [WITH TIES | WITH ONLY] */ List *rowMarks; /* a list of RowMarkClause's */ @@ -1574,7 +1574,7 @@ typedef struct SelectStmt List *sortClause; /* sort clause (a list of SortBy's) */ Node *limitOffset; /* # of result tuples to skip */ Node *limitCount; /* # of result tuples to return */ - LimitOption limitOption; /* limit type */ + LimitOption limitOption; /* limit type */ List *lockingClause; /* FOR UPDATE (list of LockingClause's) */ WithClause *withClause; /* WITH clause */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 24b7e51a91..5e961a8f7b 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -945,7 +945,7 @@ typedef struct Limit Plan plan; Node *limitOffset; /* OFFSET parameter, or NULL if none */ Node *limitCount; /* COUNT parameter, or NULL if none */ - LimitOption limitOption; /* LIMIT with ties or exact number */ + LimitOption limitOption; /* LIMIT with ties or exact number */ int numCols; /* number of columns to check for Similarity */ AttrNumber *uniqColIdx; /* their indexes in the target list */ Oid *uniqOperators; /* equality operators to compare with */ diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h index 2c7f4c71ea..f107d4f773 100644 --- a/src/include/nodes/relation.h +++ b/src/include/nodes/relation.h @@ -1737,7 +1737,7 @@ typedef struct LimitPath Path *subpath; /* path representing input source */ Node *limitOffset; /* OFFSET parameter, or NULL if none */ Node *limitCount; /* COUNT parameter, or NULL if none */ - LimitOption limitOption; /* LIMIT with ties or exact number */ + LimitOption limitOption; /* LIMIT with ties or exact number */ } LimitPath; -- 2.13.6