Re: SSI freezing bug

Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On 2013-10-03 21:14:17 -0700, Dan Ports 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.

I totally agree.  It would be safe, and generally seems better, to
omit xmin from the hash tag for heap tuple locks.

> But locks on those still can have meaning, right? From my very
> limited understanding of predicate.c/SSI I don't see why we
> cannot have meaningful conflicts on tuples that are already
> vacuumable. You'd need some curiously interleaved transactions,
> sure, but it seems possible?

No.  We established quite firmly that predicate locks on a tuple do
not need to be carried forward to new versions of that tuple.  It
is only the *version* of the row that was read to create the
predicate lock which needs to be monitored for write conflicts.  If
the row has been vacuumed, or even determined to be eligible for
vacuuming, we know it is not a candidate for update or delete -- so
it is safe to drop the predicate locks.  We do *not* want to do any
other cleanup for the transaction which read that tuple based on
this; just the individual tuple lock should be cleared.  Any
conflict with the lock which occurred earlier will be preserved.

> ISTM we'd need to peg the xmin horizon for vacuum to the oldest
> xmin predicate.c keeps track of.

That would be another alternative, but it seems more problematic
and less effective.

I'm strongly leaning toward the idea that a slightly tweaked
version of the proposed patch is appropriate for the back-branches,
because the fix Heikki is now suggesting seems too invasive to
back-patch.  I think it would make sense to apply it to master,
too, so that the new isolation tests can be immediately added.  We
can then work on the new approach for 9.4 and have the tests to
help confirm we are not breaking anything.  The tweak would be to
preserve the signature of heap_freeze_tuple(), because after the
more invasive fix in 9.4 the new parameters will not be needed.
They are only passed as non-NULL from one of the three callers, so
it seems best to leave those call sites alone rather than change
them back-and-forth.

I will post a new patch today or tomorrow.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company