Fix UPDATE/DELETE WHERE CURRENT OF to support repeated update and update-

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

@@ -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 */
 }
 
 

src/backend/executor/nodeTidscan.c

@@ -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))
 		{
 			/*

src/backend/optimizer/path/costsize.c

@@ -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))
 	{

src/include/nodes/execnodes.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/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;

src/test/regress/expected/portals.out

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

src/test/regress/sql/portals.sql

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