On Sat, Oct 22, 2016 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 1. Why are you replacing ExecOnConflictUpdate's ExecCheckHeapTupleVisible
> call with ExecCheckTIDVisible? That appears to demand a re-fetch of the
> tuple ExecOnConflictUpdate already has its hands on. Wouldn't it be
> better to just put a buffer lock/unlock around the existing code?
I thought that that was less than idiomatic within nodeModifyTable.c
-- executor modules rarely directly acquire buffer locks. More
importantly, while the lighter weight ExecCheckHeapTupleVisible()
variant seemed to make sense when there was no buffer lock
acquisition, now that it's clear that a buffer lock acquisition is
necessary, I am skeptical. I now doubt that the added complexity pays
for itself, which is why I proposed to remove
ExecCheckHeapTupleVisible(). Besides, ON CONFLICT seems like a feature
that is significantly less compelling at higher isolation levels; that
must be why it took this long to hear a problem report like
Konstantin's.
> 2. Your proposed coding
>
> + if (!heap_fetch(rel, estate->es_snapshot, &tuple, &buffer, true, NULL))
> + ...
> + if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple.t_data)))
>
> will dump core in the case where heap_fetch returns false with
> tuple.t_data set to null. If that case is impossible here,
> it's not apparent why, and it'd surely deserve at least a comment,
> maybe an Assert. But I'd rather just assume it's possible.
You could say the same thing about at least one other existing place
that more or less assumes a conflictTid heap_fetch() or similar cannot
allow that to happen, I think. Maybe this is just as good a place to
talk about it as any other, though. I defer to you.
(It's safe because our snapshot is a kind of interlock against vacuum
killing the conflictTid tuple.)
> I'm not taking a position on whether this is OK at the higher level
> of whether the SSI behavior could be better. However, unless Kevin
> has objections to committing something like this now, I think we
> should fix the above-mentioned problems and push it. The existing
> behavior is clearly bad.
I don't have any strong feelings on it myself just yet; I trust that
Kevin's judgement that we should do better under SSI is sound. I
should have mentioned that Kevin encouraged me to do this much first
in my description of the patch. I'm pretty confident that he will have
no objection to doing something like this for now.
--
Peter Geoghegan
