From 93671bd11f03367c9a0adaefb6d992de3bfde57a Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Sun, 31 Mar 2019 00:48:37 +0100
Subject: [PATCH 2/2] fixes and comments

---
 doc/src/sgml/ref/select.sgml     |  6 +++---
 src/backend/executor/nodeLimit.c | 24 ++++++++++++++----------
 src/backend/nodes/outfuncs.c     | 29 +++--------------------------
 src/include/nodes/parsenodes.h   |  2 +-
 src/include/optimizer/planmain.h |  2 +-
 5 files changed, 22 insertions(+), 41 deletions(-)

diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index b3b045ea87..e83d309c5b 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -44,7 +44,7 @@ SELECT [ ALL | DISTINCT [ ON ( <replaceable class="parameter">expression</replac
     [ ORDER BY <replaceable class="parameter">expression</replaceable> [ ASC | DESC | USING <replaceable class="parameter">operator</replaceable> ] [ NULLS { FIRST | LAST } ] [, ...] ]
     [ LIMIT { <replaceable class="parameter">count</replaceable> | ALL } ]
     [ OFFSET <replaceable class="parameter">start</replaceable> [ ROW | ROWS ] ]
-    [ FETCH { FIRST | NEXT } [ <replaceable class="parameter">count</replaceable> ] { ROW | ROWS } [ ONLY | WITH TIES ] ]
+    [ FETCH { FIRST | NEXT } [ <replaceable class="parameter">count</replaceable> ] { ROW | ROWS } { ONLY | WITH TIES } ]
     [ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } [ OF <replaceable class="parameter">table_name</replaceable> [, ...] ] [ NOWAIT | SKIP LOCKED ] [...] ]
 
 <phrase>where <replaceable class="parameter">from_item</replaceable> can be one of:</phrase>
@@ -1430,7 +1430,7 @@ OFFSET <replaceable class="parameter">start</replaceable>
     which <productname>PostgreSQL</productname> also supports.  It is:
 <synopsis>
 OFFSET <replaceable class="parameter">start</replaceable> { ROW | ROWS }
-FETCH { FIRST | NEXT } [ <replaceable class="parameter">count</replaceable> ] { ROW | ROWS } [ ONLY | WITH TIES ]
+FETCH { FIRST | NEXT } [ <replaceable class="parameter">count</replaceable> ] { ROW | ROWS } { ONLY | WITH TIES }
 </synopsis>
     In this syntax, the <replaceable class="parameter">start</replaceable>
     or <replaceable class="parameter">count</replaceable> value is required by
@@ -1442,7 +1442,7 @@ FETCH { FIRST | NEXT } [ <replaceable class="parameter">count</replaceable> ] {
     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>
+    that tie for the last place in the result 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
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index e8aed10177..68ade28c75 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -127,9 +127,12 @@ ExecLimit(PlanState *pstate)
 			{
 				/*
 				 * Forwards scan, so check for stepping off end of window. If
-				 * we are at the end of the window, return NULL without
-				 * advancing the subplan or the position variable; but change
-				 * the state machine state to record having done so.
+				 * we are at the end of the window, the behavior depends whether
+				 * ONLY or WITH TIES was specified.  In case of ONLY, we return
+				 * NULL without advancing the subplan or the position variable;
+				 * but change the state machine state to record having done so.
+				 * In the WITH TIES mode, we need to advance the subplan until
+				 * we find the first row with different ORDER BY pathkeys.
 				 */
 				if (!node->noCount &&
 					node->position - node->offset >= node->count &&
@@ -159,6 +162,7 @@ ExecLimit(PlanState *pstate)
 						node->lstate = LIMIT_SUBPLANEOF;
 						return NULL;
 					}
+
 					/*
 					 * Test if the new tuple and the last tuple match.
 					 * If so we return the tuple.
@@ -176,9 +180,9 @@ ExecLimit(PlanState *pstate)
 						node->lstate = LIMIT_WINDOWEND;
 
 						/*
-						* If we know we won't need to back up, we can release
-						* resources at this point.
-						*/
+						 * If we know we won't need to back up, we can release
+						 * resources at this point.
+						 */
 						if (!(node->ps.state->es_top_eflags & EXEC_FLAG_BACKWARD))
 							(void) ExecShutdownNode(outerPlan);
 
@@ -197,12 +201,12 @@ ExecLimit(PlanState *pstate)
 						node->lstate = LIMIT_SUBPLANEOF;
 						return NULL;
 					}
-					if (node->limitOption == WITH_TIES)
-					{
-						ExecCopySlot(node->last_slot, slot);
-					}
 					node->subSlot = slot;
 					node->position++;
+
+					/* XXX We probably only need to do this for the last tuple in the regular window, no? */
+					if (node->limitOption == WITH_TIES)
+						ExecCopySlot(node->last_slot, slot);
 				}
 			}
 			else
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2d2ea588d2..6c1d9e4c25 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -910,32 +910,9 @@ _outLimit(StringInfo str, const Limit *node)
 	WRITE_NODE_FIELD(limitCount);
 	WRITE_ENUM_FIELD(limitOption, LimitOption);
 	WRITE_INT_FIELD(numCols);
-	if (node->numCols > 0)
-	{
-		appendStringInfoString(str, " :uniqColIdx");
-		for (i = 0; i < node->numCols; i++)
-			appendStringInfo(str, " %d", node->uniqColIdx[i]);
-
-		appendStringInfoString(str, " :uniqOperators");
-		for (i = 0; i < node->numCols; i++)
-			appendStringInfo(str, " %u", node->uniqOperators[i]);
-
-		appendStringInfoString(str, " :uniqCollations");
-		for (i = 0; i < node->numCols; i++)
-			appendStringInfo(str, " %u", node->uniqOperators[i]);
-	}
-	else
-	{
-		appendStringInfoString(str, " :uniqColIdx");
-		appendStringInfo(str, " NULL");
-
-		appendStringInfoString(str, " :uniqOperators");
-		appendStringInfo(str, " NULL");
-
-		appendStringInfoString(str, " :uniqCollations");
-		appendStringInfo(str, " NULL");
-	}
-
+	WRITE_ATTRNUMBER_ARRAY(uniqColIdx, node->numCols);
+	WRITE_OID_ARRAY(uniqOperators, node->numCols);
+	WRITE_OID_ARRAY(uniqCollations, node->numCols);
 }
 
 static void
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 4b7357910f..40b9f2eeb8 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 [WITH TIES | WITH ONLY] */
+	LimitOption	limitOption;	/* limit type {ONLY | WITH TIES} */
 
 	List	   *rowMarks;		/* a list of RowMarkClause's */
 
diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h
index 52d86bd6fb..924f82ab25 100644
--- a/src/include/optimizer/planmain.h
+++ b/src/include/optimizer/planmain.h
@@ -57,7 +57,7 @@ extern Agg *make_agg(List *tlist, List *qual,
 		 List *groupingSets, List *chain,
 		 double dNumGroups, Plan *lefttree);
 extern Limit *make_limit(Plan *lefttree, Node *limitOffset, Node *limitCount,
-		 LimitOption limitOption,int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations);
+		 LimitOption limitOption, int ordNumCols, AttrNumber *ordColIdx, Oid *ordOperators, Oid *ordCollations);
 
 /*
  * prototypes for plan/initsplan.c
-- 
2.20.1

