On Tue, Nov 29, 2016 at 6:26 AM, Simon Riggs <si...@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) &&
>= estate->es_output_cid)
                                return NULL;

Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to