Disallow scrolling of FOR UPDATE/FOR SHARE cursors, so as to avoid problems

in corner cases such as re-fetching a just-deleted row.  We may be able to
relax this someday, but let's find out how many people really care before
we invest a lot of work in it.  Per report from Heikki and subsequent
discussion.

While in the neighborhood, make the combination of INSENSITIVE and FOR UPDATE
throw an error, since they are semantically incompatible.  (Up to now we've
accepted but just ignored the INSENSITIVE option of DECLARE CURSOR.)

diff --git a/doc/src/sgml/ref/declare.sgml b/doc/src/sgml/ref/declare.sgml
index f823cf77bbea5a31c1e634871d9bd2953d2aa8ee..269d2c101c4ce9ca18c50df6be5069d613505f60 100644 (file)
--- 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.41 2007/06/11 01:16:21 tgl Exp $
+$PostgreSQL: pgsql/doc/src/sgml/ref/declare.sgml,v 1.42 2007/10/24 23:27:07 tgl Exp $
 PostgreSQL documentation
 -->
 
@@ -172,7 +172,7 @@ DECLARE <replaceable class="parameter">name</replaceable> [ BINARY ] [ INSENSITI
     transaction.  Thus, <command>DECLARE</> without <literal>WITH
     HOLD</literal> is useless outside a transaction block: the cursor would
     survive only to the completion of the statement.  Therefore
-    <productname>PostgreSQL</productname> reports an error if this
+    <productname>PostgreSQL</productname> reports an error if such a
     command is used outside a transaction block.
     Use
     <xref linkend="sql-begin" endterm="sql-begin-title">,
@@ -230,6 +230,11 @@ DECLARE <replaceable class="parameter">name</replaceable> [ BINARY ] [ INSENSITI
     will have no effect if the row was changed meanwhile.
    </para>
 
+   <para>
+    <literal>SCROLL</literal> may not be specified when the query
+    includes <literal>FOR UPDATE</> or <literal>FOR SHARE</>.
+   </para>
+
    <para>
     The SQL standard only makes provisions for cursors in embedded
     <acronym>SQL</acronym>.  The <productname>PostgreSQL</productname>
@@ -265,10 +270,11 @@ DECLARE liahona CURSOR FOR SELECT * FROM films;
   <title>Compatibility</title>
 
   <para>
-   The SQL standard specifies that by default, cursors are sensitive to
-   concurrent updates of the underlying data.  In
+   The SQL standard says that it is implementation-dependent whether cursors
+   are sensitive to concurrent updates of the underlying data by default.  In
    <productname>PostgreSQL</productname>, cursors are insensitive by default,
-   and can be made sensitive by specifying <literal>FOR UPDATE</>.
+   and can be made sensitive by specifying <literal>FOR UPDATE</>.  Other
+   products may work differently.
   </para>
 
   <para>

diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index 939452650d088428b78c3caa2a4a3462d7719a87..e8f21d4f0836027cd1074c58fc3cc316ba082f54 100644 (file)
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -14,7 +14,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.65 2007/04/27 22:05:47 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.66 2007/10/24 23:27:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -102,12 +102,13 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
         *
         * If the user didn't specify a SCROLL type, allow or disallow scrolling
         * based on whether it would require any additional runtime overhead to do
-        * so.
+        * so.  Also, we disallow scrolling for FOR UPDATE cursors.
         */
        portal->cursorOptions = cstmt->options;
        if (!(portal->cursorOptions & (CURSOR_OPT_SCROLL | CURSOR_OPT_NO_SCROLL)))
        {
-               if (ExecSupportsBackwardScan(stmt->planTree))
+               if (stmt->rowMarks == NIL &&
+                       ExecSupportsBackwardScan(stmt->planTree))
                        portal->cursorOptions |= CURSOR_OPT_SCROLL;
                else
                        portal->cursorOptions |= CURSOR_OPT_NO_SCROLL;

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 875e4da2914e1f73ab40cabe12f1eb1c8faa4678..6d59401d0fb963f176dea5d26f21606b88092d3e 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.181 2007/09/20 17:56:31 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.182 2007/10/24 23:27:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -975,12 +975,29 @@ SPI_cursor_open(const char *name, SPIPlanPtr plan,
        {
                if (list_length(stmt_list) == 1 &&
                        IsA((Node *) linitial(stmt_list), PlannedStmt) &&
+                       ((PlannedStmt *) linitial(stmt_list))->rowMarks == NIL &&
                        ExecSupportsBackwardScan(((PlannedStmt *) linitial(stmt_list))->planTree))
                        portal->cursorOptions |= CURSOR_OPT_SCROLL;
                else
                        portal->cursorOptions |= CURSOR_OPT_NO_SCROLL;
        }
 
+       /*
+        * Disallow SCROLL with SELECT FOR UPDATE.  This is not redundant with
+        * the check in transformDeclareCursorStmt because the cursor options
+        * might not have come through there.
+        */
+       if (portal->cursorOptions & CURSOR_OPT_SCROLL)
+       {
+               if (list_length(stmt_list) == 1 &&
+                       IsA((Node *) linitial(stmt_list), PlannedStmt) &&
+                       ((PlannedStmt *) linitial(stmt_list))->rowMarks != NIL)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                        errmsg("DECLARE CURSOR SCROLL ... FOR UPDATE/SHARE is not supported"),
+                                        errdetail("Scrollable cursors must be READ ONLY.")));
+       }
+
        /*
         * If told to be read-only, we'd better check for read-only queries.
         * This can't be done earlier because we need to look at the finished,

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 3135d8524675545d3d0cc5542b9538065fc890db..567130b18db5947bec9678f94bc843ae1b2db437 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -17,7 +17,7 @@
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- *     $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.367 2007/06/23 22:12:51 tgl Exp $
+ *     $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.368 2007/10/24 23:27:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1623,6 +1623,20 @@ transformDeclareCursorStmt(ParseState *pstate, DeclareCursorStmt *stmt)
                          errmsg("DECLARE CURSOR WITH HOLD ... FOR UPDATE/SHARE is not supported"),
                                 errdetail("Holdable cursors must be READ ONLY.")));
 
+       /* FOR UPDATE and SCROLL are not compatible */
+       if (result->rowMarks != NIL && (stmt->options & CURSOR_OPT_SCROLL))
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                         errmsg("DECLARE CURSOR SCROLL ... FOR UPDATE/SHARE is not supported"),
+                                errdetail("Scrollable cursors must be READ ONLY.")));
+
+       /* FOR UPDATE and INSENSITIVE are not compatible */
+       if (result->rowMarks != NIL && (stmt->options & CURSOR_OPT_INSENSITIVE))
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                         errmsg("DECLARE CURSOR INSENSITIVE ... FOR UPDATE/SHARE is not supported"),
+                                errdetail("Insensitive cursors must be READ ONLY.")));
+
        /* We won't need the raw querytree any more */
        stmt->query = NULL;
 

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 412fadac54e94784e8207478a55c19608c859185..e1a6198e01250f264b0d12f3d7bbc0e3b21fee3e 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/nodes/parsenodes.h,v 1.353 2007/09/03 18:46:30 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/parsenodes.h,v 1.354 2007/10/24 23:27:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1479,7 +1479,7 @@ typedef struct CommentStmt
 #define CURSOR_OPT_BINARY              0x0001          /* BINARY */
 #define CURSOR_OPT_SCROLL              0x0002          /* SCROLL explicitly given */
 #define CURSOR_OPT_NO_SCROLL   0x0004          /* NO SCROLL explicitly given */
-#define CURSOR_OPT_INSENSITIVE 0x0008          /* INSENSITIVE (unimplemented) */
+#define CURSOR_OPT_INSENSITIVE 0x0008          /* INSENSITIVE */
 #define CURSOR_OPT_HOLD                        0x0010          /* WITH HOLD */
 #define CURSOR_OPT_FAST_PLAN   0x0020          /* prefer fast-start plan */
 

diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
index b6673073cdf68da743c248df205f2dae62622827..527550eabde67f910d437674ecb114457484fa00 100644 (file)
--- a/src/test/regress/expected/portals.out
+++ b/src/test/regress/expected/portals.out
@@ -1073,40 +1073,31 @@ SELECT * FROM uctest;
  23 | three
 (2 rows)
 
--- sensitive cursor should show effects of updates or deletes
--- XXX current behavior is WRONG
-FETCH RELATIVE 0 FROM c1;
+DELETE FROM uctest WHERE CURRENT OF c1;
+SELECT * FROM uctest;
  f1 | f2  
 ----+-----
   8 | one
 (1 row)
 
-DELETE FROM uctest WHERE CURRENT OF c1;
-SELECT * FROM uctest;
- f1 |  f2   
-----+-------
- 23 | three
-(1 row)
-
 DELETE FROM uctest WHERE CURRENT OF c1; -- no-op
 SELECT * FROM uctest;
- f1 |  f2   
-----+-------
- 23 | three
+ f1 | f2  
+----+-----
+  8 | one
 (1 row)
 
 UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- no-op
 SELECT * FROM uctest;
- f1 |  f2   
-----+-------
- 23 | three
+ f1 | f2  
+----+-----
+  8 | one
 (1 row)
 
+--- sensitive cursors can't currently scroll back, so this is an error:
 FETCH RELATIVE 0 FROM c1;
- f1 | f2 
-----+----
-(0 rows)
-
+ERROR:  cursor can only scan forward
+HINT:  Declare it with SCROLL option to enable backward scan.
 ROLLBACK;
 SELECT * FROM uctest;
  f1 |  f2   

diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
index bdf5956d69ca4e68195287f579e5e80a28f7dcaa..8275ed78c84647ffb780bc71272122eaf43ae9ed 100644 (file)
--- a/src/test/regress/sql/portals.sql
+++ b/src/test/regress/sql/portals.sql
@@ -376,15 +376,13 @@ UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1;
 SELECT * FROM uctest;
 UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1;
 SELECT * FROM uctest;
--- sensitive cursor should show effects of updates or deletes
--- XXX current behavior is WRONG
-FETCH RELATIVE 0 FROM c1;
 DELETE FROM uctest WHERE CURRENT OF c1;
 SELECT * FROM uctest;
 DELETE FROM uctest WHERE CURRENT OF c1; -- no-op
 SELECT * FROM uctest;
 UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- no-op
 SELECT * FROM uctest;
+--- sensitive cursors can't currently scroll back, so this is an error:
 FETCH RELATIVE 0 FROM c1;
 ROLLBACK;
 SELECT * FROM uctest;