Re: Incorrect UPDATE trigger invocation in the UPDATE clause of an UPSERT statement.

On Tue, Dec 8, 2015 at 6:44 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> It sounds to me like the real fix would be to just use
> &tuple.t_self. That'll "break" the above regression test. But the reason
> for that seems to be entirely independent: Namely that in this case the
> tuple is only modified after the heap_lock_tuple(), in the course of the
> ExecProject() computing the new tuple version - the SELECT FROM
> upsert_cte...

I think you're right. I had a feeling that there was some unanswered
question about that particular regression test.

FWIW, t_ctid could not really point to arbitrary things, because
heap_lock_tuple() is not asked to follow the update chain, and we
start from the first phase at the first sign of a conflict within
ExecOnConflictUpdate() (it succeeds only when it locks the
*definitive* conflict tuple, with no future tuple version). As you say
t_ctid could still point to a dead tuple, unfortunately, because that
doesn't count as a new version, which would result in raising an
error. Maybe we should revisit the idea of making jjanes_upsert do
fault injection. After all, the whole origin of that tool is Jeff's
crash recovery tester, which artificially simulated torn pages, went
through recovery, and reconciled the state of the database after
recovery with a known-good state that the tool kept track of.

> ISTM, that if we really want to protect against UpdatedSelf we need to
> to do so after ExecQual(), ExecWCE(), and ExecProject(). Each of those
> can trigger such an update.

Hmm. You mean having changed things to pass &tuple.t_self to
ExecUpdate() instead?

One tricky point here is that things are different between
ExecOnConflictUpdate() and ExecUpdate(). The return value
HeapTupleSelfUpdated is a "can't happen" state within
ExecOnConflictUpdate(), for reasons that I think can be well isolated,
and understood relatively easily (see comments), whereas
HeapTupleSelfUpdated is the "I guess we'll just ignore a second
attempt to update tuple" state within ExecUpdate().

I think you might be confusing ExecUpdate()'s use of
HeapTupleSelfUpdated with ExecOnConflictUpdate()'s similar use of
HeapTupleInvisible (where it's almost the same to the user -- it's the
"I guess we'll throw a cardinality violation error to reject a second
attempt at updating the tuple" state there). And, contrariwise,
ExecUpdate() has HeapTupleInvisible as its own "can't happen" error.
Of course, these differences are due to the different types of
snapshots used in each case (dirty snapshot for
ExecOnConflictUpdate(), MVCC snapshot for ExecUpdate() -- at least
prior to 9.5/this bug).

The reason that the with.sql regression test fails when you change the
code to pass &tuple.t_self to ExecUpdate() is that, as you say, we get
the historic ExecUpdate()/plain update tolerance of would-be
"cardinality violations" (the ExecUpdate() HeapTupleSelfUpdated case).

What I don't see is why you suggest we need to worry about each case
(e.g. ExecQual(), ExecWCE(), and ExecProject()) specifically. Could we
just instruct ExecUpdate() that the caller is ExecOnConflictUpdate(),
and so it's appropriate to throw a cardinality violation for its
HeapTupleSelfUpdated case? After all, that case already discriminates
against updates that are not from the same command in the xact (e.g.
due to weird before triggers) by throwing an error [1]. I don't think
we need to throw a cardinality violation unless an UPSERT attempts to
update a tuple a second time, but not if that occurs within a command
that happens to contain an UPSERT not directly doing the updating.

[1] Commit 6868ed74
--
Peter Geoghegan