Andres Freund wrote:
> On 2014-06-02 13:27:17 -0400, Alvaro Herrera wrote:
> > 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.
>
> It essentially is. If xmax aborts you still have to deal with an
> 'INSERT_IN_PROGRESS' tuple because, as you say, xmin is still
> running. That's essentially my point. INSERT_IN_PROGRESS isn't a wrong
> answer.
Uh. Actually it strikes me that DELETE_IN_PROGRESS is a wrong answer in
the XidIsInProgress(xmin) block anyhow. If the tuple is being inserted
remotely, there is no way we can return DELETE_IN_PROGRESS, is there?
We can never be sure that the deleting subxact is not going to abort.
The only way for this transaction to say that the tuple is
DELETE_IN_PROGRESS is if the *inserting* subtransaction is an aborted
subxact; but we already tell that case apart. So DELETE_IN_PROGRESS is
only a valid return in the new block, XidIsCurrentXact(xmin).
Another thing I wanted to comment on originally is that I'm not sure
about the CHECK_FOR_INTERRUPTS() additions. Aren't those only to serve
the case of the current bug, or bugs of similar ilk? There is no way
that a valid, non-bug situation is going to cause us to loop forever
there. That said, there isn't much harm in having them there either,
but I think I'd put them before the xact wait, not after.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services