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');