On Tue, Nov 29, 2016 at 6:26 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> src/backend/access/heap/README.tuplock says we do this...
>
> LockTuple()
> XactLockTableWait()
> mark tuple as locked by me
> UnlockTuple()
>
> only problem is we don't... because EvalPlanQualFetch() does this
>
> XactLockTableWait()
> LockTuple()
Hmm. Yeah. Actually, it doesn't do LockTuple() directly but just
calls heap_lock_tuple(), which calls heap_acquire_tuplock(), which
calls LockTupleTuplock(), which calls LockTuple(). The words "lock"
and "tuple" are overloaded to the point of total confusion here, which
may account for the oversight you spotted.
> If README.tuplock's reasons for the stated lock order is correct then
> it implies that EvalPlanQual updates could be starved indefinitely,
> which is probably bad.
I suspect that it's pretty hard to hit the starvation case in
practice, because EvalPlanQual updates are pretty rare. It's hard to
imagine a stream of updates all hitting the same tuple for long enough
to cause a serious problem. Eventually EvalPlanQualFetch would win
the race, I think.
> It might also be a bug of more serious nature, though no bug seen.
> This was found while debugging why wait_event not set correctly.
>
> In any case, I can't really see any reason for this coding in
> EvalPlanQual and it isn't explained in comments. Simply removing the
> wait allows the access pattern to follow the documented lock order,
> and allows regression tests and isolation tests to pass without
> problem. Perhaps I have made an error there.
That might cause a problem because of this intervening test, for the
reasons explained in the comment:
/* * If tuple was inserted by our own
transaction, we have to check * cmin against es_output_cid: cmin >= current
CID means our * command cannot see the tuple, so we should
ignore it. Otherwise * heap_lock_tuple() will throw an error, and
so would any later * attempt to update or delete the tuple. (We
need not check cmax * because HeapTupleSatisfiesDirty will
consider a tuple deleted * by our transaction dead, regardless of
cmax.) We just checked * that priorXmax == xmin, so we can test that
variable instead of * doing HeapTupleHeaderGetXmin again. */
if (TransactionIdIsCurrentTransactionId(priorXmax) &&
HeapTupleHeaderGetCmin(tuple.t_data)
>= estate->es_output_cid) { ReleaseBuffer(buffer);
return NULL; }
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company