When FOR UPDATE/SHARE is used with LIMIT, put the LockRows plan node

underneath the Limit node, not atop it.  This fixes the old problem that such
a query might unexpectedly return fewer rows than the LIMIT says, due to
LockRows discarding updated rows.

There is a related problem that LockRows might destroy the sort ordering
produced by earlier steps; but fixing that by pushing LockRows below Sort
would create serious performance problems that are unjustified in many
real-world applications, as well as potential deadlock problems from locking
many more rows than expected.  Instead, keep the present semantics of applying
FOR UPDATE after ORDER BY within a single query level; but allow the user to
specify the other way by writing FOR UPDATE in a sub-select.  To make that
work, track whether FOR UPDATE appeared explicitly in sub-selects or got
pushed down from the parent, and don't flatten a sub-select that contained an
explicit FOR UPDATE.

2 files changed, 45 insertions, 21 deletions

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 0b396b29bcc..6b24098cdd3 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planner.c,v 1.260 2009/10/26 02:26:33 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planner.c,v 1.261 2009/10/28 14:55:38 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1638,19 +1638,7 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
 	}
 
 	/*
-	 * If there is a LIMIT/OFFSET clause, add the LIMIT node.
-	 */
-	if (parse->limitCount || parse->limitOffset)
-	{
-		result_plan = (Plan *) make_limit(result_plan,
-										  parse->limitOffset,
-										  parse->limitCount,
-										  offset_est,
-										  count_est);
-	}
-
-	/*
-	 * Finally, if there is a FOR UPDATE/SHARE clause, add the LockRows node.
+	 * If there is a FOR UPDATE/SHARE clause, add the LockRows node.
 	 * (Note: we intentionally test parse->rowMarks not root->rowMarks here.
 	 * If there are only non-locking rowmarks, they should be handled by
 	 * the ModifyTable node instead.)
@@ -1660,6 +1648,23 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
 		result_plan = (Plan *) make_lockrows(result_plan,
 											 root->rowMarks,
 											 SS_assign_special_param(root));
+		/*
+		 * The result can no longer be assumed sorted, since locking might
+		 * cause the sort key columns to be replaced with new values.
+		 */
+		current_pathkeys = NIL;
+	}
+
+	/*
+	 * Finally, if there is a LIMIT/OFFSET clause, add the LIMIT node.
+	 */
+	if (parse->limitCount || parse->limitOffset)
+	{
+		result_plan = (Plan *) make_limit(result_plan,
+										  parse->limitOffset,
+										  parse->limitCount,
+										  offset_est,
+										  count_est);
 	}
 
 	/* Compute result-relations list if needed */
@@ -1795,20 +1800,33 @@ preprocess_rowmarks(PlannerInfo *root)
 
 	/*
 	 * Convert RowMarkClauses to PlanRowMark representation.
-	 *
-	 * Note: currently, it is syntactically impossible to have FOR UPDATE
-	 * applied to an update/delete target rel.  If that ever becomes
-	 * possible, we should drop the target from the PlanRowMark list.
 	 */
 	prowmarks = NIL;
 	foreach(l, parse->rowMarks)
 	{
 		RowMarkClause *rc = (RowMarkClause *) lfirst(l);
-		PlanRowMark *newrc = makeNode(PlanRowMark);
+		RangeTblEntry *rte = rt_fetch(rc->rti, parse->rtable);
+		PlanRowMark *newrc;
 
+		/*
+		 * Currently, it is syntactically impossible to have FOR UPDATE
+		 * applied to an update/delete target rel.  If that ever becomes
+		 * possible, we should drop the target from the PlanRowMark list.
+		 */
 		Assert(rc->rti != parse->resultRelation);
+
+		/*
+		 * Ignore RowMarkClauses for subqueries; they aren't real tables
+		 * and can't support true locking.  Subqueries that got flattened
+		 * into the main query should be ignored completely.  Any that didn't
+		 * will get ROW_MARK_COPY items in the next loop.
+		 */
+		if (rte->rtekind != RTE_RELATION)
+			continue;
+
 		rels = bms_del_member(rels, rc->rti);
 
+		newrc = makeNode(PlanRowMark);
 		newrc->rti = newrc->prti = rc->rti;
 		if (rc->forUpdate)
 			newrc->markType = ROW_MARK_EXCLUSIVE;
@@ -1838,7 +1856,6 @@ preprocess_rowmarks(PlannerInfo *root)
 			continue;
 
 		newrc = makeNode(PlanRowMark);
-
 		newrc->rti = newrc->prti = i;
 		/* real tables support REFERENCE, anything else needs COPY */
 		if (rte->rtekind == RTE_RELATION)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index f48bd31151c..7d4d7b217f7 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -16,7 +16,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/prep/prepjointree.c,v 1.68 2009/10/26 02:26:35 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/prep/prepjointree.c,v 1.69 2009/10/28 14:55:38 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1030,6 +1030,12 @@ is_simple_subquery(Query *subquery)
 	/*
 	 * Can't pull up a subquery involving grouping, aggregation, sorting,
 	 * limiting, or WITH.  (XXX WITH could possibly be allowed later)
+	 *
+	 * We also don't pull up a subquery that has explicit FOR UPDATE/SHARE
+	 * clauses, because pullup would cause the locking to occur semantically
+	 * higher than it should.  Implicit FOR UPDATE/SHARE is okay because
+	 * in that case the locking was originally declared in the upper query
+	 * anyway.
 	 */
 	if (subquery->hasAggs ||
 		subquery->hasWindowFuncs ||
@@ -1039,6 +1045,7 @@ is_simple_subquery(Query *subquery)
 		subquery->distinctClause ||
 		subquery->limitOffset ||
 		subquery->limitCount ||
+		subquery->hasForUpdate ||
 		subquery->cteList)
 		return false;