Re: uninterruptable loop: concurrent delete in progress within table

Andres Freund wrote:
> On 2014-06-02 12:48:01 -0400, Alvaro Herrera wrote:
> > Andres Freund wrote:
> >
> > > I do wonder if any of the other existing callers of HTSV are affected. I
> > > don't understand predicate.c well enough to be sure, but it looks to me
> > > like it'd could in theory lead to missed conflicts. Seems fairly
> > > unlikely to matter in practice though.
> >
> > One place where the difference in the new logic would cause a change is
> > the tupleIsAlive bit in IndexBuildHeapScan(), when building unique
> > indexes.  Right now if the tuple is inserted by a remote running
> > transaction, and updated by it, we return DELETE_IN_PROGRESS, so we set
> > tupleIsAlive = false; so we wouldn't block if we see a tuple with that
> > value elsewhere.   If we instead return INSERT_IN_PROGRESS we set
> > tupleIsAlive = true, and we would block until the inserter is done.
>
> Hm. Maybe I am missing something, but if either INSERT or
> DELETE_IN_PROGRESS is returned for a unique index in
> IndexBuildHeapScan() we'll wait for the xmin/xmax respectively and
> recheck the tuple, right? That recheck will then return a non-ephemeral
> status.

Yeah, that's how I interpret that maze of little loops using callbacks.
I don't think waiting on xmin is the same as waiting on xmax, however;
the xmin could stay running for a while, but the xmax could be an
quickly aborted subtransaction, for instance.

> > > I have to say I really hate the amount of repetitive code inside the
> > > individual visibility routines. Obviously it's nothing backpatchable and
> > > may become moot to a certain degree with the CSN work, but those are
> > > pretty close to being unmaintainable.
> >
> > One thing I'm now wondering while reading this code is those cases where
> > XMAX_INVALID is not set, but the value in Xmax is InvalidXid.  We have
> > discussed these scenarios elsewhere and the conclusion seems to be that
> > they are rare but possible and therefore we should cater for them.
>
> Is there really a situation where that combination is possible for
> XMAX_INVALID? I don't see how.

I vaguely remember Robert described how would this work, even though the
precise details evidently escape me because I can't quite trace it now.
I can't remember what thread this was on, either.

> Are you possibly thinking of the case where 9.3 assumed wrongly for a
> while that a set XMAX_INVALID would be sufficient for freezing?

Nope, that's not it IIRC.  It wasn't a bug but valid a corner case.  I
think it involved a crash.

> > So instead of a check like
> >     if (tuple->t_infomask & HEAP_XMAX_INVALID)
> > we would have something like
> >     if (HeapTupleHeaderXmaxIsInvalid(tuple))
> > which checks both things -- and is easier to read, IMHO.
>
> Note that that bit of code isn't new. It just has moved a tiny
> bit. Similar tests are splattered all over tqual.c - so I have a hard
> time thinking this could be broken.

Yeah, I'm not saying this is a new bug; I was only reacting to your
suggestion that we dump all of tqual.c and rewrite it from scratch
(heh).  Also, the corner case that produced this exact scenario is very
low probability IIRC, so even if it's broken today, it might not be all
that obvious that this is the cause.

I agree that the visibility routines are pretty hard to follow; and
evidently given the number of times we've messed up, it's hard to get
all the little corner cases right.  I don't know what a more
maintainable version of them would look like, however, particularly
keeping in mind that they are performance-critical.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services