Fix PlanRowMark/ExecRowMark structures to handle inheritance correctly.

In an inherited UPDATE/DELETE, each target table has its own subplan,
because it might have a column set different from other targets.  This
means that the resjunk columns we add to support EvalPlanQual might be
at different physical column numbers in each subplan.  The EvalPlanQual
rewrite I did for 9.0 failed to account for this, resulting in possible
misbehavior or even crashes during concurrent updates to the same row,
as seen in a recent report from Gordon Shannon.  Revise the data structure
so that we track resjunk column numbers separately for each subplan.

I also chose to move responsibility for identifying the physical column
numbers back to executor startup, instead of assuming that numbers derived
during preprocess_targetlist would stay valid throughout subsequent
massaging of the plan.  That's a bit slower, so we might want to consider
undoing it someday; but it would complicate the patch considerably and
didn't seem justifiable in a bug fix that has to be back-patched to 9.0.

2 files changed, 40 insertions, 15 deletions

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 928cf4c5a55..fda44255b08 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -407,12 +407,10 @@ typedef struct EState
  * When doing UPDATE, DELETE, or SELECT FOR UPDATE/SHARE, we should have an
  * ExecRowMark for each non-target relation in the query (except inheritance
  * parent RTEs, which can be ignored at runtime).  See PlanRowMark for details
- * about most of the fields.
+ * about most of the fields.  In addition to fields directly derived from
+ * PlanRowMark, we store curCtid, which is used by the WHERE CURRENT OF code.
  *
- * es_rowMarks is a list of these structs.	Each LockRows node has its own
- * list, which is the subset of locks that it is supposed to enforce; note
- * that the per-node lists point to the same structs that are in the global
- * list.
+ * EState->es_rowMarks is a list of these structs.
  */
 typedef struct ExecRowMark
 {
@@ -421,11 +419,28 @@ typedef struct ExecRowMark
 	Index		prti;			/* parent range table index, if child */
 	RowMarkType markType;		/* see enum in nodes/plannodes.h */
 	bool		noWait;			/* NOWAIT option */
+	ItemPointerData curCtid;	/* ctid of currently locked tuple, if any */
+} ExecRowMark;
+
+/*
+ * ExecAuxRowMark -
+ *	   additional runtime representation of FOR UPDATE/SHARE clauses
+ *
+ * Each LockRows and ModifyTable node keeps a list of the rowmarks it needs to
+ * deal with.  In addition to a pointer to the related entry in es_rowMarks,
+ * this struct carries the column number(s) of the resjunk columns associated
+ * with the rowmark (see comments for PlanRowMark for more detail).  In the
+ * case of ModifyTable, there has to be a separate ExecAuxRowMark list for
+ * each child plan, because the resjunk columns could be at different physical
+ * column positions in different subplans.
+ */
+typedef struct ExecAuxRowMark
+{
+	ExecRowMark *rowmark;		/* related entry in es_rowMarks */
 	AttrNumber	ctidAttNo;		/* resno of ctid junk attribute, if any */
 	AttrNumber	toidAttNo;		/* resno of tableoid junk attribute, if any */
 	AttrNumber	wholeAttNo;		/* resno of whole-row junk attribute, if any */
-	ItemPointerData curCtid;	/* ctid of currently locked tuple, if any */
-} ExecRowMark;
+} ExecAuxRowMark;
 
 
 /* ----------------------------------------------------------------
@@ -1002,7 +1017,7 @@ typedef struct EPQState
 	PlanState  *planstate;		/* plan state tree ready to be executed */
 	TupleTableSlot *origslot;	/* original output tuple to be rechecked */
 	Plan	   *plan;			/* plan tree to be executed */
-	List	   *rowMarks;		/* ExecRowMarks (non-locking only) */
+	List	   *arowMarks;		/* ExecAuxRowMarks (non-locking only) */
 	int			epqParam;		/* ID of Param to force scan node re-eval */
 } EPQState;
 
@@ -1030,6 +1045,7 @@ typedef struct ModifyTableState
 	PlanState **mt_plans;		/* subplans (one per target rel) */
 	int			mt_nplans;		/* number of plans in the array */
 	int			mt_whichplan;	/* which one is being executed (0..n-1) */
+	List	  **mt_arowmarks;	/* per-subplan ExecAuxRowMark lists */
 	EPQState	mt_epqstate;	/* for evaluating EvalPlanQual rechecks */
 	bool		fireBSTriggers; /* do we need to fire stmt triggers? */
 } ModifyTableState;
@@ -1706,7 +1722,7 @@ typedef struct SetOpState
 typedef struct LockRowsState
 {
 	PlanState	ps;				/* its first field is NodeTag */
-	List	   *lr_rowMarks;	/* List of ExecRowMarks */
+	List	   *lr_arowMarks;	/* List of ExecAuxRowMarks */
 	EPQState	lr_epqstate;	/* for evaluating EvalPlanQual rechecks */
 } LockRowsState;
 
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 79876100d5a..e14257932e6 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -158,6 +158,9 @@ typedef struct Result
  *	 ModifyTable node -
  *		Apply rows produced by subplan(s) to result table(s),
  *		by inserting, updating, or deleting.
+ *
+ * Note that rowMarks and epqParam are presumed to be valid for all the
+ * subplan(s); they can't contain any info that varies across subplans.
  * ----------------
  */
 typedef struct ModifyTable
@@ -690,9 +693,18 @@ typedef enum RowMarkType
  * prti == parent's RT index, and can therefore be recognized as children by
  * the fact that prti != rti.
  *
- * The AttrNumbers are filled in during preprocess_targetlist.	We use
- * different subsets of them for plain relations, inheritance children,
- * and non-table relations.
+ * The planner also adds resjunk output columns to the plan that carry
+ * information sufficient to identify the locked or fetched rows.  For
+ * tables (markType != ROW_MARK_COPY), these columns are named
+ *		tableoid%u			OID of table
+ *		ctid%u				TID of row
+ * The tableoid column is only present for an inheritance hierarchy.
+ * When markType == ROW_MARK_COPY, there is instead a single column named
+ *		wholerow%u			whole-row value of relation
+ * In all three cases, %u represents the parent rangetable index (prti).
+ * Note this means that all tables in an inheritance hierarchy share the
+ * same resjunk column names.  However, in an inherited UPDATE/DELETE the
+ * columns could have different physical column numbers in each subplan.
  */
 typedef struct PlanRowMark
 {
@@ -702,9 +714,6 @@ typedef struct PlanRowMark
 	RowMarkType markType;		/* see enum above */
 	bool		noWait;			/* NOWAIT option */
 	bool		isParent;		/* true if this is a "dummy" parent entry */
-	AttrNumber	ctidAttNo;		/* resno of ctid junk attribute, if any */
-	AttrNumber	toidAttNo;		/* resno of tableoid junk attribute, if any */
-	AttrNumber	wholeAttNo;		/* resno of whole-row junk attribute, if any */
 } PlanRowMark;