Re: preserving forensic information when we freeze

On Thu, Nov 21, 2013 at 9:47 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> As promised I'm currently updating the patch. Some questions arose
> during that:
>
> * Should HeapTupleHeaderXminFrozen also check for FrozenTransactionId?
> It seems quite possible that people think they've delt with frozen
> xmin entirely after checking, but they still might get
> FrozenTransactionId back in a pg_upgraded cluster.

The reason I originally wrote the patch the way I did, rather than the
way that you prefer, is that it minimizes the number of places where
we might perform extra tests that are known not to be needed in
context. These code paths are hot. If you do this sort of thing,
then after macro expansion we may end up with a lot of things like:
(flags & FROZEN) || (rawxid == 2) ? 2 : rawxid. I want to avoid that.
That macros is intended, specifically, to be a test for flag bits,
and I think it should do precisely that. If that's not what you want,
then don't use that macro.

> * Existing htup_details boolean checks contain an 'Is', but
> HeapTupleHeaderXminCommitted, HeapTupleHeaderXminInvalid,
> HeapTupleHeaderXminFrozen don't contain any verb. Not sure.

We could say XminIsComitted, XminIsInvalid, XminIsFrozen, etc. I
don't particularly care for it, but I can see the argument for it.

> * EvalPlanQualFetch() uses priorXmax like logic to find updated versions
> of tuples. I am not 100% sure there aren't cases where that's
> problematic even with the current code, but I think it's better not to
> use the raw xid there - priorXmax can never be FrozenTransactionId, so
> comparing it to the GetXmin() seems better.
> It also has the following wrong comment:
> * .... (Should be safe to examine xmin without getting
> * buffer's content lock, since xmin never changes in an existing
> * tuple.)

Hmm. The new tuple can't be frozen unless it's all-visible, and it
can't be all-visible if our scan can still see its predecessor. That
would imply that if it's frozen, it must be an unrelated tuple. I
think.

> But I don't see that causing any problems.
> * ri_trigger.c did do a TransactionIdIsCurrentTransactionId() on a raw
> xmin value, the consequences are minor though.
>
> The rewrite to introduce HeapTupleHeaderGet[Raw]Xmin() indeed somewhat
> increases the patchsize - but I think it's enough increase in
> expressiveness to be well worth the cost. Indeed it led me to find at
> least one issue (with minor consequences).
>
> I think once we have this we should start opportunistically try to
> freeze tuples during vacuum using OldestXmin instead of FreezeLimit if
> the page is already dirty.

Separate patch, but yeah, something like that. If we have to mark the
page all-visible, we might as well freeze it while we're there. We
should think about how it interacts with Heikki's freeze-without-write
patch though.

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