Re: SSI freezing bug

On Thu, Oct 03, 2013 at 06:19:49AM -0700, Kevin Grittner wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> wrote:
> > IMHO it would be better to remove xmin from the lock key, and vacuum
> > away the old predicate locks when the corresponding tuple is vacuumed.
> > The xmin field is only required to handle the case that a tuple is
> > vacuumed, and a new unrelated tuple is inserted to the same slot.
> > Removing the lock when the tuple is removed fixes that.

This seems definitely safe: we need the predicate locks to determine if
someone is modifying a tuple we read, and certainly if it's eligible
for vacuum nobody's going to be modifying that tuple anymore.

> > In fact, I cannot even come up with a situation where you would have a
> > problem if we just removed xmin from the key, even if we didn't vacuum
> > away old locks. I don't think the old lock can conflict with anything
> > that would see the new tuple that gets inserted in the same slot. I have
> > a feeling that you could probably prove that if you stare long enough at
> > the diagram of a dangerous structure and the properties required for a
> > conflict.

This would also be safe, in the sense that it's OK to flag a
conflict even if one doesn't exist. I'm not convinced that it isn't
possible to have false positives this way. I think it's possible for a
tuple to be vacuumed away and the ctid reused before the predicate
locks on it are eligible for cleanup. (In fact, isn't this what was
happening in the thread Kevin linked?)

> You are the one who suggested adding xmin to the key:
>
> http://www.postgresql.org/message-id/4D5A36FC.6010203@enterprisedb.com
>
> I will review that thread in light of your recent comments, but the
> fact is that xmin was not originally in the lock key, testing
> uncovered bugs, and adding xmin fixed those bugs.  I know I tried
> some other approach first, which turned out to be complex and quite
> messy -- it may have been similar to what you are proposing now.

At the time, we thought it was necessary for a predicate lock to lock
*all future versions* of a tuple, and so we had a bunch of code to
maintain a version chain. That was fraught with bugs, and turned out
not to be necessary (IIRC, we worked that out at the pub at PGcon).
That made it critical to distinguish different tuples that had the same
ctid because they could wind up in the wrong chain or cause a cycle.
With that code ripped out, that's no longer an issue.

But all this is an exceptionally subtle part of what was an
exceptionally complex patch, so a lot of careful thought is needed
here...

> It seems to me that a change such as you are now suggesting is
> likely to be too invasive to back-patch.  Do you agree that it
> would make sense to apply the patch I have proposed, back to 9.1,
> and then consider any alternative as 9.4 material?

I agree with this.

Dan

--
Dan R. K. Ports UW CSE http://drkp.net/