Fix UPDATE/DELETE WHERE CURRENT OF to support repeated update and update-
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 24 Oct 2007 18:37:09 +0000 (18:37 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 24 Oct 2007 18:37:09 +0000 (18:37 +0000)
then-delete on the current cursor row.  The basic fix is that nodeTidscan.c
has to apply heap_get_latest_tid() to the current-scan-TID obtained from the
cursor query; this ensures we get the latest row version to work with.
However, since that only works if the query plan is a TID scan, we also have
to hack the planner to make sure only that type of plan will be selected.
(Formerly, the planner might decide to apply a seqscan if the table is very
small.  This change is probably a Good Thing anyway, since it's hard to see
how a seqscan could really win.)  That means the execQual.c code to support
CurrentOfExpr as a regular expression type is dead code, so replace it with
just an elog().  Also, add regression tests covering these cases.  Note
that the added tests expose the fact that re-fetching an updated row
misbehaves if the cursor used FOR UPDATE.  That's an independent bug that
should be fixed later.  Per report from Dharmendra Goyal.

src/backend/executor/execQual.c
src/backend/executor/nodeTidscan.c
src/backend/optimizer/path/costsize.c
src/include/nodes/execnodes.h
src/test/regress/expected/portals.out
src/test/regress/sql/portals.sql

index cc165006f5c55808fddb40d4613e2ffac583741b..53ddc6581975d2768a10e8f0294c2eafadf2c85c 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.222 2007/09/06 17:31:58 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.223 2007/10/24 18:37:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -3694,45 +3694,17 @@ ExecEvalArrayCoerceExpr(ArrayCoerceExprState *astate,
 /* ----------------------------------------------------------------
  *     ExecEvalCurrentOfExpr
  *
- * Normally, the planner will convert CURRENT OF into a TidScan qualification,
- * but we have plain execQual support in case it doesn't.
+ * The planner must convert CURRENT OF into a TidScan qualification.
+ * So, we have to be able to do ExecInitExpr on a CurrentOfExpr,
+ * but we shouldn't ever actually execute it.
  * ----------------------------------------------------------------
  */
 static Datum
 ExecEvalCurrentOfExpr(ExprState *exprstate, ExprContext *econtext,
                      bool *isNull, ExprDoneCond *isDone)
 {
-   CurrentOfExpr *cexpr = (CurrentOfExpr *) exprstate->expr;
-   bool    result;
-   bool    lisnull;
-   Oid     tableoid;
-   ItemPointer tuple_tid;
-   ItemPointerData cursor_tid;
-
-   if (isDone)
-       *isDone = ExprSingleResult;
-   *isNull = false;
-
-   Assert(cexpr->cvarno != INNER);
-   Assert(cexpr->cvarno != OUTER);
-   Assert(!TupIsNull(econtext->ecxt_scantuple));
-   /* Use slot_getattr to catch any possible mistakes */
-   tableoid = DatumGetObjectId(slot_getattr(econtext->ecxt_scantuple,
-                                            TableOidAttributeNumber,
-                                            &lisnull));
-   Assert(!lisnull);
-   tuple_tid = (ItemPointer)
-       DatumGetPointer(slot_getattr(econtext->ecxt_scantuple,
-                                    SelfItemPointerAttributeNumber,
-                                    &lisnull));
-   Assert(!lisnull);
-
-   if (execCurrentOf(cexpr, econtext, tableoid, &cursor_tid))
-       result = ItemPointerEquals(&cursor_tid, tuple_tid);
-   else
-       result = false;
-
-   return BoolGetDatum(result);
+   elog(ERROR, "CURRENT OF cannot be executed");
+   return 0;                   /* keep compiler quiet */
 }
 
 
index 31a9828b6a1b22fe130e11a1fcf0d14cf6209b10..8c217a442becdcc1ed468e01100648b1e17a8616 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/executor/nodeTidscan.c,v 1.55 2007/06/11 22:22:40 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/executor/nodeTidscan.c,v 1.56 2007/10/24 18:37:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -68,6 +68,7 @@ TidListCreate(TidScanState *tidstate)
    tidList = (ItemPointerData *)
        palloc(numAllocTids * sizeof(ItemPointerData));
    numTids = 0;
+   tidstate->tss_isCurrentOf = false;
 
    foreach(l, evalList)
    {
@@ -165,6 +166,7 @@ TidListCreate(TidScanState *tidstate)
                                 numAllocTids * sizeof(ItemPointerData));
                }
                tidList[numTids++] = cursor_tid;
+               tidstate->tss_isCurrentOf = true;
            }
        }
        else
@@ -182,6 +184,9 @@ TidListCreate(TidScanState *tidstate)
        int         lastTid;
        int         i;
 
+       /* CurrentOfExpr could never appear OR'd with something else */
+       Assert(!tidstate->tss_isCurrentOf);
+
        qsort((void *) tidList, numTids, sizeof(ItemPointerData),
              itemptr_comparator);
        lastTid = 0;
@@ -269,7 +274,8 @@ TidNext(TidScanState *node)
 
        /*
         * XXX shouldn't we check here to make sure tuple matches TID list? In
-        * runtime-key case this is not certain, is it?
+        * runtime-key case this is not certain, is it?  However, in the
+        * WHERE CURRENT OF case it might not match anyway ...
         */
 
        ExecStoreTuple(estate->es_evTuple[scanrelid - 1],
@@ -319,6 +325,15 @@ TidNext(TidScanState *node)
    while (node->tss_TidPtr >= 0 && node->tss_TidPtr < numTids)
    {
        tuple->t_self = tidList[node->tss_TidPtr];
+
+       /*
+        * For WHERE CURRENT OF, the tuple retrieved from the cursor might
+        * since have been updated; if so, we should fetch the version that
+        * is current according to our snapshot.
+        */
+       if (node->tss_isCurrentOf)
+           heap_get_latest_tid(heapRelation, snapshot, &tuple->t_self);
+
        if (heap_fetch(heapRelation, snapshot, tuple, &buffer, false, NULL))
        {
            /*
index d1eb29691a06c648f38a93633efc6296986dc216..c722070abc8bd5d213d3113e269c542e0c8cb2c7 100644 (file)
@@ -54,7 +54,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/optimizer/path/costsize.c,v 1.186 2007/09/22 21:36:40 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/optimizer/path/costsize.c,v 1.187 2007/10/24 18:37:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -769,6 +769,7 @@ cost_tidscan(Path *path, PlannerInfo *root,
 {
    Cost        startup_cost = 0;
    Cost        run_cost = 0;
+   bool        isCurrentOf = false;
    Cost        cpu_per_tuple;
    QualCost    tid_qual_cost;
    int         ntuples;
@@ -778,9 +779,6 @@ cost_tidscan(Path *path, PlannerInfo *root,
    Assert(baserel->relid > 0);
    Assert(baserel->rtekind == RTE_RELATION);
 
-   if (!enable_tidscan)
-       startup_cost += disable_cost;
-
    /* Count how many tuples we expect to retrieve */
    ntuples = 0;
    foreach(l, tidquals)
@@ -793,6 +791,12 @@ cost_tidscan(Path *path, PlannerInfo *root,
 
            ntuples += estimate_array_length(arraynode);
        }
+       else if (IsA(lfirst(l), CurrentOfExpr))
+       {
+           /* CURRENT OF yields 1 tuple */
+           isCurrentOf = true;
+           ntuples++;
+       }
        else
        {
            /* It's just CTID = something, count 1 tuple */
@@ -800,6 +804,22 @@ cost_tidscan(Path *path, PlannerInfo *root,
        }
    }
 
+   /*
+    * We must force TID scan for WHERE CURRENT OF, because only nodeTidscan.c
+    * understands how to do it correctly.  Therefore, honor enable_tidscan
+    * only when CURRENT OF isn't present.  Also note that cost_qual_eval
+    * counts a CurrentOfExpr as having startup cost disable_cost, which we
+    * subtract off here; that's to prevent other plan types such as seqscan
+    * from winning.
+    */
+   if (isCurrentOf)
+   {
+       Assert(baserel->baserestrictcost.startup >= disable_cost);
+       startup_cost -= disable_cost;
+   }
+   else if (!enable_tidscan)
+       startup_cost += disable_cost;
+
    /*
     * The TID qual expressions will be computed once, any other baserestrict
     * quals once per retrived tuple.
@@ -2002,8 +2022,8 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
    }
    else if (IsA(node, CurrentOfExpr))
    {
-       /* This is noticeably more expensive than a typical operator */
-       context->total.per_tuple += 100 * cpu_operator_cost;
+       /* Report high cost to prevent selection of anything but TID scan */
+       context->total.startup += disable_cost;
    }
    else if (IsA(node, SubLink))
    {
index 82f851eeacc18d81406c197f2161b78acc1baff1..fbb07feae7e6869725fd1d38d3b5fdae389691c0 100644 (file)
@@ -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/execnodes.h,v 1.178 2007/09/20 17:56:32 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/nodes/execnodes.h,v 1.179 2007/10/24 18:37:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1087,6 +1087,7 @@ typedef struct BitmapHeapScanState
 /* ----------------
  *  TidScanState information
  *
+ *     isCurrentOf    scan has a CurrentOfExpr qual
  *     NumTids        number of tids in this scan
  *     TidPtr         index of currently fetched tid
  *     TidList        evaluated item pointers (array of size NumTids)
@@ -1096,6 +1097,7 @@ typedef struct TidScanState
 {
    ScanState   ss;             /* its first field is NodeTag */
    List       *tss_tidquals;   /* list of ExprState nodes */
+   bool        tss_isCurrentOf;
    int         tss_NumTids;
    int         tss_TidPtr;
    int         tss_MarkTidPtr;
index 3638664b1bbc1cc3aca427bb12a6443f4ce6c883..b6673073cdf68da743c248df205f2dae62622827 100644 (file)
@@ -982,6 +982,139 @@ SELECT * FROM uctest;
   8 | one
 (2 rows)
 
+-- Check repeated-update and update-then-delete cases
+BEGIN;
+DECLARE c1 CURSOR FOR SELECT * FROM uctest;
+FETCH c1;
+ f1 |  f2   
+----+-------
+  3 | three
+(1 row)
+
+UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1;
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+  8 | one
+ 13 | three
+(2 rows)
+
+UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1;
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+  8 | one
+ 23 | three
+(2 rows)
+
+-- insensitive cursor should not show effects of updates or deletes
+FETCH RELATIVE 0 FROM c1;
+ f1 |  f2   
+----+-------
+  3 | three
+(1 row)
+
+DELETE FROM uctest WHERE CURRENT OF c1;
+SELECT * FROM uctest;
+ f1 | f2  
+----+-----
+  8 | one
+(1 row)
+
+DELETE FROM uctest WHERE CURRENT OF c1; -- no-op
+SELECT * FROM uctest;
+ f1 | f2  
+----+-----
+  8 | one
+(1 row)
+
+UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- no-op
+SELECT * FROM uctest;
+ f1 | f2  
+----+-----
+  8 | one
+(1 row)
+
+FETCH RELATIVE 0 FROM c1;
+ f1 |  f2   
+----+-------
+  3 | three
+(1 row)
+
+ROLLBACK;
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+  3 | three
+  8 | one
+(2 rows)
+
+BEGIN;
+DECLARE c1 CURSOR FOR SELECT * FROM uctest FOR UPDATE;
+FETCH c1;
+ f1 |  f2   
+----+-------
+  3 | three
+(1 row)
+
+UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1;
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+  8 | one
+ 13 | three
+(2 rows)
+
+UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1;
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+  8 | one
+ 23 | three
+(2 rows)
+
+-- sensitive cursor should show effects of updates or deletes
+-- XXX current behavior is WRONG
+FETCH RELATIVE 0 FROM c1;
+ 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
+(1 row)
+
+UPDATE uctest SET f1 = f1 + 10 WHERE CURRENT OF c1; -- no-op
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+ 23 | three
+(1 row)
+
+FETCH RELATIVE 0 FROM c1;
+ f1 | f2 
+----+----
+(0 rows)
+
+ROLLBACK;
+SELECT * FROM uctest;
+ f1 |  f2   
+----+-------
+  3 | three
+  8 | one
+(2 rows)
+
 -- Check inheritance cases
 CREATE TEMP TABLE ucchild () inherits (uctest);
 INSERT INTO ucchild values(100, 'hundred');
index 382a28c4e30bfa156de6f5c0a0d442c5851a56af..bdf5956d69ca4e68195287f579e5e80a28f7dcaa 100644 (file)
@@ -349,6 +349,46 @@ SELECT * FROM uctest;
 COMMIT;
 SELECT * FROM uctest;
 
+-- Check repeated-update and update-then-delete cases
+BEGIN;
+DECLARE c1 CURSOR FOR SELECT * FROM uctest;
+FETCH c1;
+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;
+-- insensitive cursor should not show effects of updates or deletes
+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;
+FETCH RELATIVE 0 FROM c1;
+ROLLBACK;
+SELECT * FROM uctest;
+
+BEGIN;
+DECLARE c1 CURSOR FOR SELECT * FROM uctest FOR UPDATE;
+FETCH c1;
+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;
+FETCH RELATIVE 0 FROM c1;
+ROLLBACK;
+SELECT * FROM uctest;
+
 -- Check inheritance cases
 CREATE TEMP TABLE ucchild () inherits (uctest);
 INSERT INTO ucchild values(100, 'hundred');