Re: preserving forensic information when we freeze

On 2013-11-21 15:59:35 -0500, Robert Haas wrote:
> > * 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.

The patch as sent shouldn't really do that in any of paths I know to be
hot - it uses *RawXmin() there.

> 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.

But in which cases would that actually be slower? There'll be no
additional code executed if the hint bits for frozen are set, and in
case not it will usually safe us an external function call to
TransactionIdPrecedes().

> 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.

That's a fair argument. Although there's several HeapTupleHeader* macros
that muck with stuff besides infomask.

> > * 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.

I don't have a clear preference either, I just noticed the inconsistency
and wasn't sure whether it was intentional.

> > * 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.

Yes, that's my reasoning as well - and why I think we should check for
new-style frozen xids. Either via my version of GetXmin() or
HeapTupleHeaderXminFrozen() if we go with yours.

> > 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.

Definitely separate yes. And I agree, it's partially moot if Heikki's
patch gets in, but I am not sure it will make it into 9.4. There seems
to be quite some work left.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services