From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Peter Geoghegan <pg(at)heroku(dot)com> |
Cc: | Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Kevin Grittner <kgrittn(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> |
Subject: | Re: On conflict update & hint bits |
Date: | 2016-10-22 16:38:47 |
Message-ID: | 31342.1477154327@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Peter Geoghegan <pg(at)heroku(dot)com> writes:
> Attached is a revision of Thomas' patch to fix a related issue; it now
> also fixes this no-buffer-lock-held issue.
I think this needs more work.
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?
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.
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.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Ashutosh Bapat | 2016-10-22 16:46:10 | Re: Push down more full joins in postgres_fdw |
Previous Message | Sven R. Kunze | 2016-10-22 16:08:19 | Re: Indirect indexes |