Modify UPDATE/DELETE WHERE CURRENT OF to use the FOR UPDATE infrastructure to
authorTom Lane
Sun, 16 Nov 2008 17:34:28 +0000 (17:34 +0000)
committerTom Lane
Sun, 16 Nov 2008 17:34:28 +0000 (17:34 +0000)
locate the target row, if the cursor was declared with FOR UPDATE or FOR
SHARE.  This approach is more flexible and reliable than digging through the
plan tree; for instance it can cope with join cursors.  But we still provide
the old code for use with non-FOR-UPDATE cursors.  Per gripe from Robert Haas.

doc/src/sgml/plpgsql.sgml
doc/src/sgml/ref/declare.sgml
doc/src/sgml/ref/delete.sgml
doc/src/sgml/ref/update.sgml
src/backend/executor/execCurrent.c
src/backend/executor/execMain.c
src/include/nodes/execnodes.h
src/test/regress/expected/portals.out
src/test/regress/sql/portals.sql

index 2a0894bbf27b145aa49d014c94eaac18fd54b660..b8dfe01f5cdd2b487d4d8da460b3e09a7ea78210 100644 (file)
@@ -1,4 +1,4 @@
-
+
 
 
   <application>PL/pgSQL</application> - <acronym>SQL</acronym> Procedural Language
@@ -2674,9 +2674,10 @@ DELETE FROM table WHERE CURRENT OF curso
 
        
         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 FOR UPDATE in the
+        cursor.  For additional information see the
         
         reference page.
        
index ea9b080816ff97e8d701e11f7c3da3104b5b61a0..373ef39be7939a56429c5fc628868a74c190144b 100644 (file)
@@ -1,5 +1,5 @@
 
 
@@ -213,6 +213,12 @@ DECLARE name [ BINARY ] [ INSENSITI
     specified, then backward fetches are disallowed in any case.
    
 
+   
+    Backward fetches are also disallowed when the query
+    includes FOR UPDATE or FOR SHARE; therefore
+    SCROLL may not be specified in this case.
+   
+
    
     If the cursor's query includes FOR UPDATE or FOR
     SHARE, then returned rows are locked at the time they are first
@@ -221,19 +227,40 @@ DECLARE name [ 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 sensitive cursor.  It is often wise to use FOR
-    UPDATE if the cursor is intended to be used with UPDATE
-    ... WHERE CURRENT OF or 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
-    FOR UPDATE, a subsequent WHERE CURRENT OF command
-    will have no effect if the row was changed meanwhile.
+    calls a sensitive cursor.  (Specifying INSENSITIVE
+    together with FOR UPDATE or FOR SHARE is an error.)
    
 
-   
-    SCROLL may not be specified when the query
-    includes FOR UPDATE or FOR SHARE.
-   
+   
+    
+     It is generally recommended to use FOR UPDATE if the cursor
+     is intended to be used with UPDATE ... WHERE CURRENT OF or
+     DELETE ... WHERE CURRENT OF.  Using FOR UPDATE
+     prevents other sessions from changing the rows between the time they are
+     fetched and the time they are updated.  Without FOR UPDATE,
+     a subsequent WHERE CURRENT OF command will have no effect if
+     the row was changed since the cursor was created.
+    
+
+    
+     Another reason to use FOR UPDATE is that without it, a
+     subsequent WHERE CURRENT OF might fail if the cursor query
+     does not meet the SQL standard's rules for being simply
+     updatable (in particular, the cursor must reference just one table
+     and not use grouping or 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.
+    
+
+    
+     The main reason not to use FOR UPDATE with 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.
+    
+   
 
    
     The SQL standard only makes provisions for cursors in embedded
index 6c9fdac5033fb16183542c6f2aedaab2747003d4..62e4555614f07175d63b90b23dd2dc4ea86b6636 100644 (file)
@@ -1,5 +1,5 @@
 
 
@@ -148,10 +148,13 @@ DELETE FROM [ ONLY ] table [ [ AS ]
      
       The name of the cursor to use in a 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 DELETE's target table.
       Note that WHERE CURRENT OF cannot be
-      specified together with a Boolean condition.
+      specified together with a Boolean condition.  See
+      
+      for more information about using cursors with
+      WHERE CURRENT OF.
      
     
    
@@ -244,14 +247,14 @@ DELETE FROM films WHERE kind <> 'Musical';
    Clear the table films:
 
 DELETE FROM films;
-      
+
   
 
   
    Delete completed tasks, returning full details of the deleted rows:
 
 DELETE FROM tasks WHERE status = 'DONE' RETURNING *;
-      
+
   
 
    
@@ -259,7 +262,7 @@ DELETE FROM tasks WHERE status = 'DONE' RETURNING *;
    c_tasks is currently positioned:
 
 DELETE FROM tasks WHERE CURRENT OF c_tasks;
-      
+
   
  
 
index c1996d2d931e8f049a9951b23ba4483379ff9b29..2464bf16f93f7df83584860042916c58903ab7b4 100644 (file)
@@ -1,5 +1,5 @@
 
 
@@ -167,10 +167,13 @@ UPDATE [ ONLY ] table [ [ AS ] 
      
       The name of the cursor to use in a 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 UPDATE's target table.
       Note that WHERE CURRENT OF cannot be
-      specified together with a Boolean condition.
+      specified together with a Boolean condition.  See
+      
+      for more information about using cursors with
+      WHERE CURRENT OF.
      
     
    
@@ -331,7 +334,7 @@ COMMIT;
    c_films is currently positioned:
 
 UPDATE films SET kind = 'Dramatic' WHERE CURRENT OF c_films;
-      
+
   
  
 
index 5ae90c68f40a6e3b385990927f4b375489c7d74b..2f2270cfab3994151cd193a666e06c073a37f381 100644 (file)
@@ -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;
+   }
 }
 
 /*
index 634ca69b4d13e795df992cc2153f45ba2dac9fcd..6d389319fcc87f911a7160ecdbf6736368362034 100644 (file)
@@ -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;
                }
            }
 
index 8c8742e286efa7bc33f0b6bd7ace4bdabf188ec9..9aae040019b5483bd3f26703916344812b7c1656 100644 (file)
@@ -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;
 
 
index 66563615d88cefcf7a5087de79dfc19525042276..95dcea5a1d9ebbd2c24c7d13563dfc7c68a4aaf7 100644 (file)
@@ -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"
index b53eaac786a1cb138f6fb2a9ef26857a7c8a5df6..4265aaa43cf110bf9efb49c78103f248982b0e84 100644 (file)
@@ -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;