Re: preserving forensic information when we freeze

On Mon, Jun 24, 2013 at 8:12 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Agreed. And it also improves the status quo when debugging. I wish this
> would have been the representation since the beginning.
>
> Some remarks:
> * I don't really like that HeapTupleHeaderXminFrozen() now is frequently
> performed without checking for FrozenTransactionId. I think the places
> where that's done are currently safe, but it seems likely that we
> introduce bugs due to people writing similar code.
> I think replacing *GetXmin by a wrapper that returns
> FrozenTransactionId if the hint bit tell us so would be a good
> idea. Then add *GetRawXmin for the rest (probably mostly display).
> Seems like it would make the patch actually smaller.

I did think about this approach, but it seemed like it would add
cycles in a significant number of places. For example, consider the
HeapTupleSatisfies() routines, which are perhaps PostgreSQL's finest
example of a place where you DON'T want to add any cycles. All of the
checks on xmin are conditional on HeapTupleHeaderXminCommitted()
having been found already to be false. That implies that the frozen
bits aren't set either, so if HeapTupleHeaderGetXmin() were to recheck
the bits it would be a waste. As I got to the end of the patch I was
a little dismayed by the number of places that did need adjustment to
check both things, but there are still plenty of important places that
don't.

> * The PLs imo should set fn_xmin to FrozenTransactionId if the hint bit
> tell us the tuple is frozen.

Why? I thought about that, but it seemed to me that the purpose of
that caching was to avoid confusing two functions whose pg_proc tuples
ended up at the same TID. So there's a failure mechanism there: the
tuple can get vacuumed away and replaced with a new tuple which then
gets frozen, and everything (bogusly) matches. If the actual
XID-prior-to-freezing has to match, ISTM that the chances of a false
match are far lower. You have to get the new tuple at the same TID
slot *and* the XID counter has to wrap back around to the
exactly-right XID to get a false match on XID, within the lifetime of
a single backend. That's not bloody likely. Remember, the whole
point of the patch is to start freezing things sooner, so the scenario
where both the original and replacement tuples are frozen is going to
become more common.

We also don't particularly *want* the freezing of a pg_proc tuple to
force recompilations in every backend.

> * I think rewrite_heap_dead_tuple needs to check for a frozen xmin and
> store that. We might looking at a chain which partially was done in
> <9.4. Not sure if that's a realistic scenario, but I'd rather be safe.

IIUC, you're talking about the scenario where we have an update chain
X -> Y, where X is dead but not actually removed and Y is
(forensically) frozen. We're examining tuple Y and trying to
determine whether X has been entered in rs_unresolved_tups. If, as I
think you're proposing, we consider the xmin of Y to be
FrozenTransactionId, we will definitely not find it - because the way
it got into the table in the first place was based on the value
returned by HeapTupleHeaderGetUpdateXid(). And that value is certain
not to be FrozenTransactionId, because we never set the xmax of a
tuple to FrozenTransactionId.

There's no possibility of getting confused here; if X is still around
at all, it's xmax is of the same generation as Y's xmin. Otherwise,
we've had an undetected XID wraparound.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company