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

On Tue, Dec 8, 2015 at 3:12 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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.

That's an alarming possibility. Either way though, it's wrong, and
the fix is fairly clear.

>> 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 agree, obviously.

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

We're on the same page. I just happen to think we might as well put
the check beside the existing special case check for weird before
triggers -- within ExecUpdate()'s HeapTupleSelfUpdated case. That
avoids an extra HeapTupleSatisfiesUpdate() call for every UPSERT
update. We mostly only call that routine from heapam.c as things are.
But I'm hardly going to insist on that.

I actually think that there is an argument to be made for doing
nothing here, and not allowing a cardinality violation at all. It
doesn't seem too inconsistent to follow the old behavior (just accept
a redundant second update) in the case where you update the row within
an UPSERT after the UPSERT locks the row, but before your UPSERT has a
chance to do its UPDATE of that same row (in other words, something
happened to your tuple in between). I do prefer to do what we've
agreed to, though (whether or not it happens in ExecUpdate()), mostly
because it's closer to the other ExecUpdate() HeapTupleSelfUpdated
special case.

Note the similarity between that case, and this new one.

--
Peter Geoghegan