diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index 2a0894bbf27b145aa49d014c94eaac18fd54b660..b8dfe01f5cdd2b487d4d8da460b3e09a7ea78210 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -1,4 +1,4 @@ -<!-- $PostgreSQL: pgsql/doc/src/sgml/plpgsql.sgml,v 1.135 2008/10/28 22:02:05 tgl Exp $ --> +<!-- $PostgreSQL: pgsql/doc/src/sgml/plpgsql.sgml,v 1.136 2008/11/16 17:34:28 tgl Exp $ --> <chapter id="plpgsql"> <title><application>PL/pgSQL</application> - <acronym>SQL</acronym> Procedural Language</title> @@ -2674,9 +2674,10 @@ DELETE FROM <replaceable>table</replaceable> WHERE CURRENT OF <replaceable>curso <para> When a cursor is positioned on a table row, that row can be updated - or deleted using the cursor to identify the row. Note that this - only works for simple (non-join, non-grouping) cursor queries. - For additional information see the + or deleted using the cursor to identify the row. There are + restrictions on what the cursor's query can be (in particular, + no grouping) and it's best to use <literal>FOR UPDATE</> in the + cursor. For additional information see the <xref linkend="sql-declare" endterm="sql-declare-title"> reference page. </para> diff --git a/doc/src/sgml/ref/declare.sgml b/doc/src/sgml/ref/declare.sgml index ea9b080816ff97e8d701e11f7c3da3104b5b61a0..373ef39be7939a56429c5fc628868a74c190144b 100644 --- a/doc/src/sgml/ref/declare.sgml +++ b/doc/src/sgml/ref/declare.sgml @@ -1,5 +1,5 @@ <!-- -$PostgreSQL: pgsql/doc/src/sgml/ref/declare.sgml,v 1.44 2008/11/14 10:22:46 petere Exp $ +$PostgreSQL: pgsql/doc/src/sgml/ref/declare.sgml,v 1.45 2008/11/16 17:34:28 tgl Exp $ PostgreSQL documentation --> @@ -213,6 +213,12 @@ DECLARE <replaceable class="parameter">name</replaceable> [ BINARY ] [ INSENSITI specified, then backward fetches are disallowed in any case. </para> + <para> + Backward fetches are also disallowed when the query + includes <literal>FOR UPDATE</> or <literal>FOR SHARE</>; therefore + <literal>SCROLL</literal> may not be specified in this case. + </para> + <para> If the cursor's query includes <literal>FOR UPDATE</> or <literal>FOR SHARE</>, then returned rows are locked at the time they are first @@ -221,19 +227,40 @@ DECLARE <replaceable class="parameter">name</replaceable> [ BINARY ] [ INSENSITI these options. In addition, the returned rows will be the most up-to-date versions; therefore these options provide the equivalent of what the SQL standard - calls a <quote>sensitive cursor</>. It is often wise to use <literal>FOR - UPDATE</> if the cursor is intended to be used with <command>UPDATE - ... WHERE CURRENT OF</> or <command>DELETE ... WHERE CURRENT OF</>, - since this will prevent other sessions from changing the rows between - the time they are fetched and the time they are updated. Without - <literal>FOR UPDATE</>, a subsequent <literal>WHERE CURRENT OF</> command - will have no effect if the row was changed meanwhile. + calls a <quote>sensitive cursor</>. (Specifying <literal>INSENSITIVE</> + together with <literal>FOR UPDATE</> or <literal>FOR SHARE</> is an error.) </para> - <para> - <literal>SCROLL</literal> may not be specified when the query - includes <literal>FOR UPDATE</> or <literal>FOR SHARE</>. - </para> + <caution> + <para> + It is generally recommended to use <literal>FOR UPDATE</> if the cursor + is intended to be used with <command>UPDATE ... WHERE CURRENT OF</> or + <command>DELETE ... WHERE CURRENT OF</>. Using <literal>FOR UPDATE</> + prevents other sessions from changing the rows between the time they are + fetched and the time they are updated. Without <literal>FOR UPDATE</>, + a subsequent <literal>WHERE CURRENT OF</> command will have no effect if + the row was changed since the cursor was created. + </para> + + <para> + Another reason to use <literal>FOR UPDATE</> is that without it, a + subsequent <literal>WHERE CURRENT OF</> might fail if the cursor query + does not meet the SQL standard's rules for being <quote>simply + updatable</> (in particular, the cursor must reference just one table + and not use grouping or <literal>ORDER BY</>). Cursors + that are not simply updatable might work, or might not, depending on plan + choice details; so in the worst case, an application might work in testing + and then fail in production. + </para> + + <para> + The main reason not to use <literal>FOR UPDATE</> with <literal>WHERE + CURRENT OF</> is if you need the cursor to be scrollable, or to be + insensitive to the subsequent updates (that is, continue to show the old + data). If this is a requirement, pay close heed to the caveats shown + above. + </para> + </caution> <para> The SQL standard only makes provisions for cursors in embedded diff --git a/doc/src/sgml/ref/delete.sgml b/doc/src/sgml/ref/delete.sgml index 6c9fdac5033fb16183542c6f2aedaab2747003d4..62e4555614f07175d63b90b23dd2dc4ea86b6636 100644 --- a/doc/src/sgml/ref/delete.sgml +++ b/doc/src/sgml/ref/delete.sgml @@ -1,5 +1,5 @@ <!-- -$PostgreSQL: pgsql/doc/src/sgml/ref/delete.sgml,v 1.34 2008/11/14 10:22:46 petere Exp $ +$PostgreSQL: pgsql/doc/src/sgml/ref/delete.sgml,v 1.35 2008/11/16 17:34:28 tgl Exp $ PostgreSQL documentation --> @@ -148,10 +148,13 @@ DELETE FROM [ ONLY ] <replaceable class="PARAMETER">table</replaceable> [ [ AS ] <para> The name of the cursor to use in a <literal>WHERE CURRENT OF</> condition. The row to be deleted is the one most recently fetched - from this cursor. The cursor must be a simple (non-join, non-aggregate) + from this cursor. The cursor must be a non-grouping query on the <command>DELETE</>'s target table. Note that <literal>WHERE CURRENT OF</> cannot be - specified together with a Boolean condition. + specified together with a Boolean condition. See + <xref linkend="sql-declare" endterm="sql-declare-title"> + for more information about using cursors with + <literal>WHERE CURRENT OF</>. </para> </listitem> </varlistentry> @@ -244,14 +247,14 @@ DELETE FROM films WHERE kind <> 'Musical'; Clear the table <literal>films</literal>: <programlisting> DELETE FROM films; -</programlisting> +</programlisting> </para> <para> Delete completed tasks, returning full details of the deleted rows: <programlisting> DELETE FROM tasks WHERE status = 'DONE' RETURNING *; -</programlisting> +</programlisting> </para> <para> @@ -259,7 +262,7 @@ DELETE FROM tasks WHERE status = 'DONE' RETURNING *; <literal>c_tasks</> is currently positioned: <programlisting> DELETE FROM tasks WHERE CURRENT OF c_tasks; -</programlisting> +</programlisting> </para> </refsect1> diff --git a/doc/src/sgml/ref/update.sgml b/doc/src/sgml/ref/update.sgml index c1996d2d931e8f049a9951b23ba4483379ff9b29..2464bf16f93f7df83584860042916c58903ab7b4 100644 --- a/doc/src/sgml/ref/update.sgml +++ b/doc/src/sgml/ref/update.sgml @@ -1,5 +1,5 @@ <!-- -$PostgreSQL: pgsql/doc/src/sgml/ref/update.sgml,v 1.47 2008/11/14 10:22:47 petere Exp $ +$PostgreSQL: pgsql/doc/src/sgml/ref/update.sgml,v 1.48 2008/11/16 17:34:28 tgl Exp $ PostgreSQL documentation --> @@ -167,10 +167,13 @@ UPDATE [ ONLY ] <replaceable class="PARAMETER">table</replaceable> [ [ AS ] <rep <para> The name of the cursor to use in a <literal>WHERE CURRENT OF</> condition. The row to be updated is the one most recently fetched - from this cursor. The cursor must be a simple (non-join, non-aggregate) + from this cursor. The cursor must be a non-grouping query on the <command>UPDATE</>'s target table. Note that <literal>WHERE CURRENT OF</> cannot be - specified together with a Boolean condition. + specified together with a Boolean condition. See + <xref linkend="sql-declare" endterm="sql-declare-title"> + for more information about using cursors with + <literal>WHERE CURRENT OF</>. </para> </listitem> </varlistentry> @@ -331,7 +334,7 @@ COMMIT; <literal>c_films</> is currently positioned: <programlisting> UPDATE films SET kind = 'Dramatic' WHERE CURRENT OF c_films; -</programlisting> +</programlisting> </para> </refsect1> diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c index 5ae90c68f40a6e3b385990927f4b375489c7d74b..2f2270cfab3994151cd193a666e06c073a37f381 100644 --- a/src/backend/executor/execCurrent.c +++ b/src/backend/executor/execCurrent.c @@ -6,7 +6,7 @@ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/backend/executor/execCurrent.c,v 1.7 2008/05/12 00:00:48 alvherre Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execCurrent.c,v 1.8 2008/11/16 17:34:28 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -46,10 +46,6 @@ execCurrentOf(CurrentOfExpr *cexpr, char *table_name; Portal portal; QueryDesc *queryDesc; - ScanState *scanstate; - bool lisnull; - Oid tuple_tableoid; - ItemPointer tuple_tid; /* Get the cursor name --- may have to look up a parameter reference */ if (cexpr->cursor_name) @@ -79,57 +75,129 @@ execCurrentOf(CurrentOfExpr *cexpr, errmsg("cursor \"%s\" is not a SELECT query", cursor_name))); queryDesc = PortalGetQueryDesc(portal); - if (queryDesc == NULL) + if (queryDesc == NULL || queryDesc->estate == NULL) ereport(ERROR, (errcode(ERRCODE_INVALID_CURSOR_STATE), errmsg("cursor \"%s\" is held from a previous transaction", cursor_name))); /* - * Dig through the cursor's plan to find the scan node. Fail if it's not - * there or buried underneath aggregation. + * We have two different strategies depending on whether the cursor uses + * FOR UPDATE/SHARE or not. The reason for supporting both is that the + * FOR UPDATE code is able to identify a target table in many cases where + * the other code can't, while the non-FOR-UPDATE case allows use of WHERE + * CURRENT OF with an insensitive cursor. */ - scanstate = search_plan_tree(ExecGetActivePlanTree(queryDesc), - table_oid); - if (!scanstate) - ereport(ERROR, - (errcode(ERRCODE_INVALID_CURSOR_STATE), - errmsg("cursor \"%s\" is not a simply updatable scan of table \"%s\"", - cursor_name, table_name))); + if (queryDesc->estate->es_rowMarks) + { + ExecRowMark *erm; + ListCell *lc; - /* - * The cursor must have a current result row: per the SQL spec, it's an - * error if not. We test this at the top level, rather than at the scan - * node level, because in inheritance cases any one table scan could - * easily not be on a row. We want to return false, not raise error, if - * the passed-in table OID is for one of the inactive scans. - */ - if (portal->atStart || portal->atEnd) - ereport(ERROR, - (errcode(ERRCODE_INVALID_CURSOR_STATE), - errmsg("cursor \"%s\" is not positioned on a row", - cursor_name))); + /* + * Here, the query must have exactly one FOR UPDATE/SHARE reference to + * the target table, and we dig the ctid info out of that. + */ + erm = NULL; + foreach(lc, queryDesc->estate->es_rowMarks) + { + ExecRowMark *thiserm = (ExecRowMark *) lfirst(lc); + + if (RelationGetRelid(thiserm->relation) == table_oid) + { + if (erm) + ereport(ERROR, + (errcode(ERRCODE_INVALID_CURSOR_STATE), + errmsg("cursor \"%s\" has multiple FOR UPDATE/SHARE references to table \"%s\"", + cursor_name, table_name))); + erm = thiserm; + } + } - /* Now OK to return false if we found an inactive scan */ - if (TupIsNull(scanstate->ss_ScanTupleSlot)) + if (erm == NULL) + ereport(ERROR, + (errcode(ERRCODE_INVALID_CURSOR_STATE), + errmsg("cursor \"%s\" does not have a FOR UPDATE/SHARE reference to table \"%s\"", + cursor_name, table_name))); + + /* + * The cursor must have a current result row: per the SQL spec, it's + * an error if not. + */ + if (portal->atStart || portal->atEnd) + ereport(ERROR, + (errcode(ERRCODE_INVALID_CURSOR_STATE), + errmsg("cursor \"%s\" is not positioned on a row", + cursor_name))); + + /* Return the currently scanned TID, if there is one */ + if (ItemPointerIsValid(&(erm->curCtid))) + { + *current_tid = erm->curCtid; + return true; + } + + /* + * This table didn't produce the cursor's current row; some other + * inheritance child of the same parent must have. Signal caller + * to do nothing on this table. + */ return false; + } + else + { + ScanState *scanstate; + bool lisnull; + Oid tuple_tableoid; + ItemPointer tuple_tid; + + /* + * Without FOR UPDATE, we dig through the cursor's plan to find the + * scan node. Fail if it's not there or buried underneath + * aggregation. + */ + scanstate = search_plan_tree(ExecGetActivePlanTree(queryDesc), + table_oid); + if (!scanstate) + ereport(ERROR, + (errcode(ERRCODE_INVALID_CURSOR_STATE), + errmsg("cursor \"%s\" is not a simply updatable scan of table \"%s\"", + cursor_name, table_name))); - /* Use slot_getattr to catch any possible mistakes */ - tuple_tableoid = DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot, - TableOidAttributeNumber, - &lisnull)); - Assert(!lisnull); - tuple_tid = (ItemPointer) - DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot, - SelfItemPointerAttributeNumber, - &lisnull)); - Assert(!lisnull); + /* + * The cursor must have a current result row: per the SQL spec, it's + * an error if not. We test this at the top level, rather than at the + * scan node level, because in inheritance cases any one table scan + * could easily not be on a row. We want to return false, not raise + * error, if the passed-in table OID is for one of the inactive scans. + */ + if (portal->atStart || portal->atEnd) + ereport(ERROR, + (errcode(ERRCODE_INVALID_CURSOR_STATE), + errmsg("cursor \"%s\" is not positioned on a row", + cursor_name))); - Assert(tuple_tableoid == table_oid); + /* Now OK to return false if we found an inactive scan */ + if (TupIsNull(scanstate->ss_ScanTupleSlot)) + return false; - *current_tid = *tuple_tid; + /* Use slot_getattr to catch any possible mistakes */ + tuple_tableoid = + DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot, + TableOidAttributeNumber, + &lisnull)); + Assert(!lisnull); + tuple_tid = (ItemPointer) + DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot, + SelfItemPointerAttributeNumber, + &lisnull)); + Assert(!lisnull); - return true; + Assert(tuple_tableoid == table_oid); + + *current_tid = *tuple_tid; + + return true; + } } /* diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 634ca69b4d13e795df992cc2153f45ba2dac9fcd..6d389319fcc87f911a7160ecdbf6736368362034 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -26,7 +26,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.316 2008/11/15 19:43:45 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.317 2008/11/16 17:34:28 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -609,6 +609,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) /* We'll locate the junk attrs below */ erm->ctidAttNo = InvalidAttrNumber; erm->toidAttNo = InvalidAttrNumber; + ItemPointerSetInvalid(&(erm->curCtid)); estate->es_rowMarks = lappend(estate->es_rowMarks, erm); } @@ -1418,6 +1419,7 @@ lnext: ; if (tableoid != RelationGetRelid(erm->relation)) { /* this child is inactive right now */ + ItemPointerSetInvalid(&(erm->curCtid)); continue; } } @@ -1481,6 +1483,9 @@ lnext: ; elog(ERROR, "unrecognized heap_lock_tuple status: %u", test); } + + /* Remember tuple TID for WHERE CURRENT OF */ + erm->curCtid = tuple.t_self; } } diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 8c8742e286efa7bc33f0b6bd7ace4bdabf188ec9..9aae040019b5483bd3f26703916344812b7c1656 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.195 2008/11/15 19:43:46 tgl Exp $ + * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.196 2008/11/16 17:34:28 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -381,6 +381,7 @@ typedef struct ExecRowMark bool noWait; /* NOWAIT option */ AttrNumber ctidAttNo; /* resno of its ctid junk attribute */ AttrNumber toidAttNo; /* resno of tableoid junk attribute, if any */ + ItemPointerData curCtid; /* ctid of currently locked tuple, if any */ } ExecRowMark; diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out index 66563615d88cefcf7a5087de79dfc19525042276..95dcea5a1d9ebbd2c24c7d13563dfc7c68a4aaf7 100644 --- a/src/test/regress/expected/portals.out +++ b/src/test/regress/expected/portals.out @@ -1154,6 +1154,47 @@ SELECT * FROM uctest; 110 | hundred (3 rows) +-- Can update from a self-join, but only if FOR UPDATE says which to use +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM uctest a, uctest b WHERE a.f1 = b.f1 + 5; +FETCH 1 FROM c1; + f1 | f2 | f1 | f2 +----+-----+----+------- + 18 | one | 13 | three +(1 row) + +UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- fail +ERROR: cursor "c1" is not a simply updatable scan of table "uctest" +ROLLBACK; +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM uctest a, uctest b WHERE a.f1 = b.f1 + 5 FOR UPDATE; +FETCH 1 FROM c1; + f1 | f2 | f1 | f2 +----+-----+----+------- + 18 | one | 13 | three +(1 row) + +UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- fail +ERROR: cursor "c1" has multiple FOR UPDATE/SHARE references to table "uctest" +ROLLBACK; +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM uctest a, uctest b WHERE a.f1 = b.f1 + 5 FOR SHARE OF a; +FETCH 1 FROM c1; + f1 | f2 | f1 | f2 +----+-----+----+------- + 18 | one | 13 | three +(1 row) + +UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; +SELECT * FROM uctest; + f1 | f2 +-----+--------- + 13 | three + 28 | one + 110 | hundred +(3 rows) + +ROLLBACK; -- Check various error cases DELETE FROM uctest WHERE CURRENT OF c1; -- fail, no such cursor ERROR: cursor "c1" does not exist @@ -1166,6 +1207,11 @@ DELETE FROM uctest WHERE CURRENT OF c; -- fail, cursor on wrong table ERROR: cursor "c" is not a simply updatable scan of table "uctest" ROLLBACK; BEGIN; +DECLARE c CURSOR FOR SELECT * FROM tenk2 FOR SHARE; +DELETE FROM uctest WHERE CURRENT OF c; -- fail, cursor on wrong table +ERROR: cursor "c" does not have a FOR UPDATE/SHARE reference to table "uctest" +ROLLBACK; +BEGIN; DECLARE c CURSOR FOR SELECT * FROM tenk1 JOIN tenk2 USING (unique1); DELETE FROM tenk1 WHERE CURRENT OF c; -- fail, cursor is on a join ERROR: cursor "c" is not a simply updatable scan of table "tenk1" diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql index b53eaac786a1cb138f6fb2a9ef26857a7c8a5df6..4265aaa43cf110bf9efb49c78103f248982b0e84 100644 --- a/src/test/regress/sql/portals.sql +++ b/src/test/regress/sql/portals.sql @@ -404,6 +404,24 @@ FETCH 1 FROM c1; COMMIT; SELECT * FROM uctest; +-- Can update from a self-join, but only if FOR UPDATE says which to use +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM uctest a, uctest b WHERE a.f1 = b.f1 + 5; +FETCH 1 FROM c1; +UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- fail +ROLLBACK; +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM uctest a, uctest b WHERE a.f1 = b.f1 + 5 FOR UPDATE; +FETCH 1 FROM c1; +UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- fail +ROLLBACK; +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM uctest a, uctest b WHERE a.f1 = b.f1 + 5 FOR SHARE OF a; +FETCH 1 FROM c1; +UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; +SELECT * FROM uctest; +ROLLBACK; + -- Check various error cases DELETE FROM uctest WHERE CURRENT OF c1; -- fail, no such cursor @@ -414,6 +432,10 @@ DECLARE c CURSOR FOR SELECT * FROM tenk2; DELETE FROM uctest WHERE CURRENT OF c; -- fail, cursor on wrong table ROLLBACK; BEGIN; +DECLARE c CURSOR FOR SELECT * FROM tenk2 FOR SHARE; +DELETE FROM uctest WHERE CURRENT OF c; -- fail, cursor on wrong table +ROLLBACK; +BEGIN; DECLARE c CURSOR FOR SELECT * FROM tenk1 JOIN tenk2 USING (unique1); DELETE FROM tenk1 WHERE CURRENT OF c; -- fail, cursor is on a join ROLLBACK;