Re: "ORDER BY" clause prevents "UPDATE WHERE CURRENT OF"

Gregory Stark <stark@enterprisedb.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Well, it's contrary to SQL spec, which says that sufficiently simple
>> cursors should be updatable even if they don't say FOR UPDATE.
>>
>> However ... the more I think about it, the more I wonder if we shouldn't
>> rip out the current search-the-plan-tree hack and allow WHERE CURRENT OF
>> *only* for cursors that say FOR UPDATE.

> It is tempting since it would make application code which looped updating
> WHERE CURRENT OF semantically equivalent to UPDATE FROM. It seems better to
> have one set of functionality rather than two similar but subtly different
> sets of functionality available depending on the coding style.

After playing around with this for awhile I realized that there really
would be a functional loss if we remove support for WHERE CURRENT OF
with non-FOR-UPDATE cursors.  Namely, that a non-FOR-UPDATE cursor is
insensitive to subsequent updates, which sometimes is handy.  There are
examples of both cases in the portals.sql regression test.

So what I now propose is:

1. If the cursor includes FOR UPDATE/FOR SHARE, use the ExecRowMark
technique to get the target row identity.  In this path we fail if there
is not exactly one FOR UPDATE reference to the UPDATE's target table.
(An interesting property here is that you can update from a self-join,
if you mark only one arm of the join as FOR UPDATE.  See example in
attached regression test additions.)

2. If the cursor doesn't have FOR UPDATE/SHARE, use the existing code.
In this path we fail if the plan is "too complicated".  However, it
shouldn't fail for any case that is simply updatable according to the
SQL spec.

We should revise the documentation to make it very clear that FOR UPDATE
is the preferred way, but that sometimes you might need the other.

Attached is a draft patch that is code-complete but lacks documentation.
The patch is against CVS HEAD, ie, it assumes the SELECT FOR UPDATE
inheritance fixes I made earlier today.

            regards, tom lane

Index: src/backend/executor/execCurrent.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/execCurrent.c,v
retrieving revision 1.7
diff -c -r1.7 execCurrent.c
*** src/backend/executor/execCurrent.c    12 May 2008 00:00:48 -0000    1.7
--- src/backend/executor/execCurrent.c    16 Nov 2008 00:10:32 -0000
***************
*** 46,55 ****
      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)
--- 46,51 ----
***************
*** 79,135 ****
                   errmsg("cursor \"%s\" is not a SELECT query",
                          cursor_name)));
      queryDesc = PortalGetQueryDesc(portal);
!     if (queryDesc == 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.
       */
!     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)));

!     /*
!      * 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)));

!     /* Now OK to return false if we found an inactive scan */
!     if (TupIsNull(scanstate->ss_ScanTupleSlot))
!         return false;

!     /* 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);

!     Assert(tuple_tableoid == table_oid);

!     *current_tid = *tuple_tid;

!     return true;
  }

  /*
--- 75,203 ----
                   errmsg("cursor \"%s\" is not a SELECT query",
                          cursor_name)));
      queryDesc = PortalGetQueryDesc(portal);
!     if (queryDesc == NULL || queryDesc->estate == NULL)
          ereport(ERROR,
                  (errcode(ERRCODE_INVALID_CURSOR_STATE),
                   errmsg("cursor \"%s\" is held from a previous transaction",
                          cursor_name)));

      /*
!      * 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.
       */
!     if (queryDesc->estate->es_rowMarks)
!     {
!         ExecRowMark *erm;
!         ListCell   *lc;

!         /*
!          * 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;
!             }
!         }

!         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)));
!
!         /*
!          * 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)));
!
!         /* Now OK to return false if we found an inactive scan */
!         if (TupIsNull(scanstate->ss_ScanTupleSlot))
!             return false;
!
!         /* 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);
!
!         Assert(tuple_tableoid == table_oid);

!         *current_tid = *tuple_tid;
!
!         return true;
!     }
  }

  /*
Index: src/backend/executor/execMain.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/execMain.c,v
retrieving revision 1.316
diff -c -r1.316 execMain.c
*** src/backend/executor/execMain.c    15 Nov 2008 19:43:45 -0000    1.316
--- src/backend/executor/execMain.c    16 Nov 2008 00:10:32 -0000
***************
*** 609,614 ****
--- 609,615 ----
          /* 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,1423 ****
--- 1419,1425 ----
                          if (tableoid != RelationGetRelid(erm->relation))
                          {
                              /* this child is inactive right now */
+                             ItemPointerSetInvalid(&(erm->curCtid));
                              continue;
                          }
                      }
***************
*** 1481,1486 ****
--- 1483,1491 ----
                              elog(ERROR, "unrecognized heap_lock_tuple status: %u",
                                   test);
                      }
+
+                     /* Remember tuple TID for WHERE CURRENT OF */
+                     erm->curCtid = tuple.t_self;
                  }
              }

Index: src/include/nodes/execnodes.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/execnodes.h,v
retrieving revision 1.195
diff -c -r1.195 execnodes.h
*** src/include/nodes/execnodes.h    15 Nov 2008 19:43:46 -0000    1.195
--- src/include/nodes/execnodes.h    16 Nov 2008 00:10:32 -0000
***************
*** 381,386 ****
--- 381,387 ----
      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: src/test/regress/expected/portals.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/portals.out,v
retrieving revision 1.18
diff -c -r1.18 portals.out
*** src/test/regress/expected/portals.out    15 Nov 2008 19:43:47 -0000    1.18
--- src/test/regress/expected/portals.out    16 Nov 2008 00:10:32 -0000
***************
*** 1154,1159 ****
--- 1154,1200 ----
   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,1171 ****
--- 1207,1217 ----
  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: src/test/regress/sql/portals.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/portals.sql,v
retrieving revision 1.15
diff -c -r1.15 portals.sql
*** src/test/regress/sql/portals.sql    15 Nov 2008 19:43:47 -0000    1.15
--- src/test/regress/sql/portals.sql    16 Nov 2008 00:10:32 -0000
***************
*** 404,409 ****
--- 404,427 ----
  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,419 ****
--- 432,441 ----
  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;