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

On 2015-12-08 13:17:15 -0800, Peter Geoghegan wrote:
> 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.

I'm not 100% sure, but I do think it could point to arbitrary tuples:
Consider what happens if a previous update to the same tuple failed and
the new version of the tuple is pruned away, and that space is then
reused for something else. Bang. That's probably only possible if the
xmax is a multixact, as heap_lock_tuple() otherwise would clear out
t_ctid - but that's perfectly possible.

> > > 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?

Yes, after that. Passing t_data->t_ctid is just plain wrong.

> 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).

I don't think so. Check the attached, very rough, WIP patch.

> What I don't see is why you suggest we need to worry about each case
> (e.g. ExecQual(), ExecWCE(), and ExecProject()) specifically.

I don't mean individually, just after all of those, but before calling ExecUpdate()

> 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.

I'm inclined to do the check in ExecOnConflictUpdate() instead - istm
that's easier to understand.

Greetings,

Andres Freund