On Mon, Jun 24, 2013 at 8:12 AM, Andres Freund <and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
