RowMarks versus child tables with varying column sets

I've been looking into Gordon Shannon's crash report here:
http://archives.postgresql.org/pgsql-general/2010-12/msg01030.php
After some groveling around in the core dump (thanks to Gordon for
making that available), I figured out the cause of the problem.

The missing piece of information that prevented anyone from reproducing
the crash was that Gordon's master table, and many of its inheritance
children, had a dropped column --- but the most-recently-added children
did not. This meant that the child plans of the Update node didn't all
have the same targetlists, which is expected. But then the resjunk
columns added to track row identities for sharelocking didn't occur at
the same column numbers in every subplan. The PlanRowMark representation
built by the planner fails to account for this possibility, since it sends
predetermined column numbers (ctidAttNo, toidAttNo, wholeAttNo) to the
executor in the ModifyTable's rowMarks list. This would fail not only in
the case of dropped columns, but any scenario where the child tables don't
all have identical column sets. The problem only manifests when
EvalPlanQual is run, though, and that requires concurrent updates to the
same row in READ COMMITTED mode. (Which makes it impractical to test in
the current regression test framework :-()

I can see two basic ways of attacking this. Clearly we need to track the
resjunk column numbers separately for each subplan. We could convert
ModifyTable's rowMarks list into a list-of-lists of PlanRowMark, one per
child plan. I'm thinking though that determining those column numbers at
plan time might have been an overly cute idea. It might be better to
remove the column numbers from PlanRowMark, restoring it to a form that's
not dependent on the particular subplan. Then ModifyTable would need to
build a list of ExecRowMark nodes for each child plan from the PlanRowMark
representation, doing the lookup for resjunk column names during plan
startup. This would take a few more cycles than the current way but it
seems more robust. (Another possible objection is that we'd have to put
back into the executor knowledge of the resjunk column naming
conventions, something I'd been trying to decouple by adding these attno
fields. But if it doesn't work, it doesn't work...)

Another point that has to be considered is that currently the executor
maintains a global list of ExecRowMarks (estate->es_rowMarks). In
SELECT FOR UPDATE/SHARE that list is needed so that execCurrent.c (WHERE
CURRENT OF) can pull out the current locked TID of a SELECT FOR UPDATE
cursor. However, we don't support any such thing for ModifyTable. The
other use of the global list is to keep track of which relations have
been opened with a lock type higher than AccessShareLock. On the whole
it seems that it might be best to split the data structure into two
types of structs:

* a global list carrying the same fields as current ExecRowMark except
for the attno fields.

* private lists in LockRows and ModifyTable nodes carrying attribute
numbers plus pointers to the corresponding global-list entry. In
ModifyTable we'd need such a list per child plan.

Comments?

regards, tom lane