On 2013-07-02 16:29:56 -0400, Robert Haas wrote: > 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.
Well, nothing would prevent using the HeapTupleHeaderGetRawXmin() in those places. Exactly the number of callsites is what makes me think that somebody will get this wrong in the future. > > * 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. > [good reasoning] Man. I hate this hack. I wonder what happens if somebody does a VACUUM FULL of pg_class and there are a bunch of functions created in the same transaction... > > * 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. I am thinking of something slightly different. rewrite_heap_dead_tuple() now removes tuples/xids from the unresolved table that could be from a different xid epoch since it unconditionally does a HASH_REMOVE if it finds an entry doing a lookup using the *preserved* xid. Earlier that was harmless since for frozen tuples it only ever used FrozenTransactionId which obviously cannot be part of a valid chain and couldn't even get entered into unresolved_tups. I am not sure at all if that actually can be harmful but there isn't any reason we would need to do the delete so I wouldn't. There can be complex enough situation where later parts of a ctid chain are dead and earlier ones are recently dead and such that I would rather be cautious. > 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. Another issue I thought about is what we will return for SELECT xmin FROM blarg; Some people use that in their applications (IIRC skytools/pqg/londiste does so) and they might get confused if we suddently return xids from the future. On the other hand, not returning the original xid would be a shame as well... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
