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.

Author: tglsfdc

doc/src/sgml/ref/select.sgml

@@ -1,5 +1,5 @@
 <!--
-$PostgreSQL: pgsql/doc/src/sgml/ref/select.sgml,v 1.127 2009/10/27 17:11:18 tgl Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/select.sgml,v 1.128 2009/10/28 14:55:37 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -1092,22 +1092,12 @@ FOR SHARE [ OF <replaceable class="parameter">table_name</replaceable> [, ...] ]
     has already locked a selected row or rows, <command>SELECT FOR
     UPDATE</command> will wait for the other transaction to complete,
     and will then lock and return the updated row (or no row, if the
-    row was deleted).  For further discussion see <xref
+    row was deleted).  Within a <literal>SERIALIZABLE</> transaction,
+    however, an error will be thrown if a row to be locked has changed
+    since the transaction started.  For further discussion see <xref
     linkend="mvcc">.
    </para>
 
-   <para>
-    To prevent the operation from waiting for other transactions to commit,
-    use the <literal>NOWAIT</> option.  <command>SELECT FOR UPDATE
-    NOWAIT</command> reports an error, rather than waiting, if a selected row
-    cannot be locked immediately.  Note that <literal>NOWAIT</> applies only
-    to the row-level lock(s) &mdash; the required <literal>ROW SHARE</literal>
-    table-level lock is still taken in the ordinary way (see
-    <xref linkend="mvcc">).  You can use the <literal>NOWAIT</> option of
-    <xref linkend="sql-lock" endterm="sql-lock-title">
-    if you need to acquire the table-level lock without waiting.
-   </para>
-
    <para>
     <literal>FOR SHARE</literal> behaves similarly, except that it
     acquires a shared rather than exclusive lock on each retrieved
@@ -1117,13 +1107,26 @@ FOR SHARE [ OF <replaceable class="parameter">table_name</replaceable> [, ...] ]
     from performing <command>SELECT FOR SHARE</command>.
    </para>
 
+   <para>
+    To prevent the operation from waiting for other transactions to commit,
+    use the <literal>NOWAIT</> option.  With <literal>NOWAIT</>, the statement
+    reports an error, rather than waiting, if a selected row
+    cannot be locked immediately.  Note that <literal>NOWAIT</> applies only
+    to the row-level lock(s) &mdash; the required <literal>ROW SHARE</literal>
+    table-level lock is still taken in the ordinary way (see
+    <xref linkend="mvcc">).  You can use
+    <xref linkend="sql-lock" endterm="sql-lock-title">
+    with the <literal>NOWAIT</> option first,
+    if you need to acquire the table-level lock without waiting.
+   </para>
+
    <para>
     If specific tables are named in <literal>FOR UPDATE</literal>
     or <literal>FOR SHARE</literal>,
     then only rows coming from those tables are locked; any other
     tables used in the <command>SELECT</command> are simply read as
     usual.  A <literal>FOR UPDATE</literal> or <literal>FOR SHARE</literal>
-    clause without a table list affects all tables used in the command.
+    clause without a table list affects all tables used in the statement.
     If <literal>FOR UPDATE</literal> or <literal>FOR SHARE</literal> is
     applied to a view or sub-query, it affects all tables used in
     the view or sub-query.
@@ -1151,6 +1154,36 @@ FOR SHARE [ OF <replaceable class="parameter">table_name</replaceable> [, ...] ]
     individual table rows; for example they cannot be used with aggregation.
    </para>
 
+   <para>
+    When <literal>FOR UPDATE</literal> or <literal>FOR SHARE</literal>
+    appears at the top level of a <command>SELECT</> query, the rows that
+    are locked are exactly those that are returned by the query; in the
+    case of a join query, the rows locked are those that contribute to
+    returned join rows.  In addition, rows that satisfied the query
+    conditions as of the query snapshot will be locked, although they
+    will not be returned if they have since been updated to not satisfy
+    the query conditions.  If a <literal>LIMIT</> is used, locking stops
+    once enough rows have been returned to satisfy the limit (but note that
+    rows skipped over by <literal>OFFSET</> will get locked).  Similarly,
+    if <literal>FOR UPDATE</literal> or <literal>FOR SHARE</literal>
+    is used in a cursor's query, only rows actually fetched or stepped past
+    by the cursor will be locked.
+   </para>
+
+   <para>
+    When <literal>FOR UPDATE</literal> or <literal>FOR SHARE</literal>
+    appears in a sub-<command>SELECT</>, the rows locked are those
+    returned to the outer query by the sub-query.  This might involve
+    fewer rows than inspection of the sub-query alone would suggest,
+    since conditions from the outer query might be used to optimize
+    execution of the sub-query.  For example,
+<programlisting>
+SELECT * FROM (SELECT * FROM mytable FOR UPDATE) ss WHERE col1 = 5;
+</programlisting>
+    will lock only rows having <literal>col1 = 5</>, even though that
+    condition is not textually within the sub-query.
+   </para>
+
   <caution>
    <para>
     Avoid locking a row and then modifying it within a later savepoint or
@@ -1177,30 +1210,26 @@ ROLLBACK TO s;
 
   <caution>
    <para>
-    It is possible for a <command>SELECT</> command using both
-    <literal>LIMIT</literal> and <literal>FOR UPDATE/SHARE</literal>
-    clauses to return fewer rows than specified by <literal>LIMIT</literal>.
-    This is because <literal>LIMIT</> is applied first.  The command
-    selects the specified number of rows,
-    but might then block trying to obtain a lock on one or more of them.
-    Once the <literal>SELECT</> unblocks, the row might have been deleted
-    or updated so that it does not meet the query <literal>WHERE</> condition
-    anymore, in which case it will not be returned.
-   </para>
-  </caution>
-
-  <caution>
-   <para>
-    Similarly, it is possible for a <command>SELECT</> command
-    using <literal>ORDER BY</literal> and <literal>FOR
-    UPDATE/SHARE</literal> to return rows out of order.  This is
-    because <literal>ORDER BY</> is applied first.  The command
-    orders the result, but might then block trying to obtain a lock
-    on one or more of the rows.  Once the <literal>SELECT</>
-    unblocks, one of the ordered columns might have been modified
-    and be returned out of order.  A workaround is to perform
-    <command>SELECT ... FOR UPDATE/SHARE</> and then <command>SELECT
-    ... ORDER BY</>.
+    It is possible for a <command>SELECT</> command using <literal>ORDER
+    BY</literal> and <literal>FOR UPDATE/SHARE</literal> to return rows out of
+    order.  This is because <literal>ORDER BY</> is applied first.
+    The command sorts the result, but might then block trying to obtain a lock
+    on one or more of the rows.  Once the <literal>SELECT</> unblocks, some
+    of the ordering column values might have been modified, leading to those
+    rows appearing to be out of order (though they are in order in terms
+    of the original column values).  This can be worked around at need by
+    placing the <literal>FOR UPDATE/SHARE</literal> clause in a sub-query,
+    for example
+<programlisting>
+SELECT * FROM (SELECT * FROM mytable FOR UPDATE) ss ORDER BY column1;
+</programlisting>
+    Note that this will result in locking all rows of <structname>mytable</>,
+    whereas <literal>FOR UPDATE</> at the top level would lock only the
+    actually returned rows.  This can make for a significant performance
+    difference, particularly if the <literal>ORDER BY</> is combined with
+    <literal>LIMIT</> or other restrictions.  So this technique is recommended
+    only if concurrent updates of the ordering columns are expected and a
+    strictly sorted result is required.
    </para>
   </caution>
   </refsect2>
@@ -1541,15 +1570,28 @@ SELECT distributors.* WHERE distributors.name = 'Westward';
     used by <productname>MySQL</productname>.  The SQL:2008 standard
     has introduced the clauses <literal>OFFSET ... FETCH {FIRST|NEXT}
     ...</literal> for the same functionality, as shown above
-    in <xref linkend="sql-limit" endterm="sql-limit-title">, and this
+    in <xref linkend="sql-limit" endterm="sql-limit-title">.  This
     syntax is also used by <productname>IBM DB2</productname>.
     (Applications written for <productname>Oracle</productname>
     frequently use a workaround involving the automatically
-    generated <literal>rownum</literal> column, not available in
+    generated <literal>rownum</literal> column, which is not available in
     PostgreSQL, to implement the effects of these clauses.)
    </para>
   </refsect2>
 
+  <refsect2>
+   <title><literal>FOR UPDATE</> and <literal>FOR SHARE</></title>
+
+   <para>
+    Although <literal>FOR UPDATE</> appears in the SQL standard, the
+    standard allows it only as an option of <command>DECLARE CURSOR</>.
+    <productname>PostgreSQL</productname> allows it in any <command>SELECT</>
+    query as well as in sub-<command>SELECT</>s, but this is an extension.
+    The <literal>FOR SHARE</> variant, and the <literal>NOWAIT</> option,
+    do not appear in the standard.
+   </para>
+  </refsect2>
+
   <refsect2>
    <title>Nonstandard Clauses</title>
 

src/backend/nodes/copyfuncs.c

@@ -15,7 +15,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/nodes/copyfuncs.c,v 1.449 2009/10/26 02:26:31 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/nodes/copyfuncs.c,v 1.450 2009/10/28 14:55:38 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1859,6 +1859,7 @@ _copyRowMarkClause(RowMarkClause *from)
 	COPY_SCALAR_FIELD(rti);
 	COPY_SCALAR_FIELD(forUpdate);
 	COPY_SCALAR_FIELD(noWait);
+	COPY_SCALAR_FIELD(pushedDown);
 
 	return newnode;
 }
@@ -2223,6 +2224,7 @@ _copyQuery(Query *from)
 	COPY_SCALAR_FIELD(hasSubLinks);
 	COPY_SCALAR_FIELD(hasDistinctOn);
 	COPY_SCALAR_FIELD(hasRecursive);
+	COPY_SCALAR_FIELD(hasForUpdate);
 	COPY_NODE_FIELD(cteList);
 	COPY_NODE_FIELD(rtable);
 	COPY_NODE_FIELD(jointree);

src/backend/nodes/equalfuncs.c

@@ -22,7 +22,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/nodes/equalfuncs.c,v 1.371 2009/10/26 02:26:31 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/nodes/equalfuncs.c,v 1.372 2009/10/28 14:55:38 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -860,6 +860,7 @@ _equalQuery(Query *a, Query *b)
 	COMPARE_SCALAR_FIELD(hasSubLinks);
 	COMPARE_SCALAR_FIELD(hasDistinctOn);
 	COMPARE_SCALAR_FIELD(hasRecursive);
+	COMPARE_SCALAR_FIELD(hasForUpdate);
 	COMPARE_NODE_FIELD(cteList);
 	COMPARE_NODE_FIELD(rtable);
 	COMPARE_NODE_FIELD(jointree);
@@ -2198,6 +2199,7 @@ _equalRowMarkClause(RowMarkClause *a, RowMarkClause *b)
 	COMPARE_SCALAR_FIELD(rti);
 	COMPARE_SCALAR_FIELD(forUpdate);
 	COMPARE_SCALAR_FIELD(noWait);
+	COMPARE_SCALAR_FIELD(pushedDown);
 
 	return true;
 }

src/backend/nodes/outfuncs.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/nodes/outfuncs.c,v 1.370 2009/10/26 02:26:31 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/nodes/outfuncs.c,v 1.371 2009/10/28 14:55:38 tgl Exp $
  *
  * NOTES
  *	  Every node type that can appear in stored rules' parsetrees *must*
@@ -1987,6 +1987,7 @@ _outQuery(StringInfo str, Query *node)
 	WRITE_BOOL_FIELD(hasSubLinks);
 	WRITE_BOOL_FIELD(hasDistinctOn);
 	WRITE_BOOL_FIELD(hasRecursive);
+	WRITE_BOOL_FIELD(hasForUpdate);
 	WRITE_NODE_FIELD(cteList);
 	WRITE_NODE_FIELD(rtable);
 	WRITE_NODE_FIELD(jointree);
@@ -2036,6 +2037,7 @@ _outRowMarkClause(StringInfo str, RowMarkClause *node)
 	WRITE_UINT_FIELD(rti);
 	WRITE_BOOL_FIELD(forUpdate);
 	WRITE_BOOL_FIELD(noWait);
+	WRITE_BOOL_FIELD(pushedDown);
 }
 
 static void

src/backend/nodes/readfuncs.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/nodes/readfuncs.c,v 1.226 2009/10/26 02:26:32 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/nodes/readfuncs.c,v 1.227 2009/10/28 14:55:38 tgl Exp $
  *
  * NOTES
  *	  Path and Plan nodes do not have any readfuncs support, because we
@@ -203,6 +203,7 @@ _readQuery(void)
 	READ_BOOL_FIELD(hasSubLinks);
 	READ_BOOL_FIELD(hasDistinctOn);
 	READ_BOOL_FIELD(hasRecursive);
+	READ_BOOL_FIELD(hasForUpdate);
 	READ_NODE_FIELD(cteList);
 	READ_NODE_FIELD(rtable);
 	READ_NODE_FIELD(jointree);
@@ -295,6 +296,7 @@ _readRowMarkClause(void)
 	READ_UINT_FIELD(rti);
 	READ_BOOL_FIELD(forUpdate);
 	READ_BOOL_FIELD(noWait);
+	READ_BOOL_FIELD(pushedDown);
 
 	READ_DONE();
 }

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)