Re: EvalPlanQual() doesn't follow LockTuple() pattern

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