Subject: [PATCH 2/2] review

From: Tomas Vondra <[email-masked]>
Date: Sun, 28 Oct 2018 18:38:47 +0100

---
 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 } [ <replaceable class="parameter">count</replaceable> ] {
     If <replaceable class="parameter">count</replaceable> is
     omitted in a <literal>FETCH</literal> clause, it defaults to 1.
     <literal>ROW</literal>
-    <literal>WITH TIES</literal> option is Used to return two or more rows 
-    that tie for last place in the limit results set according to <literal>ORDER BY</literal> 
-    clause (<literal>ORDER BY</literal> clause must be specified in this case). 
+    <literal>WITH TIES</literal> option is used to return two or more rows
+    that tie for last place in the limit results set according to <literal>ORDER BY</literal>
+    clause (<literal>ORDER BY</literal> clause must be specified in this case).
     and <literal>ROWS</literal> as well as <literal>FIRST</literal>
     and <literal>NEXT</literal> 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

--------------08FEC5268EA8E45E5EE90A4F
Content-Type: text/x-patch;
 name="0001-rebased-patch.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001-rebased-patch.patch"