Re: preserving forensic information when we freeze

On Thu, Nov 21, 2013 at 4:51 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 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().

Dunno. It's at least more code generation.

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

Sure, but that doesn't mean they ALL have to.

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

It was intentional enough. :-)

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

I haven't heard anything further from Heikki, so I'm thinking we
should proceed with this approach. It seems to be the path of least
resistance, if not essential, for making CLUSTER freeze everything
automatically, a change almost everyone seems to really want. Even if
we did have Heikki's stuff, making cluster freeze more aggressively is
still a good argument for doing this. The pages can then be marked
all-visible (something Bruce is working on) and never need to be
revisited. Without this, I don't think we can get there. If we also
handle the vacuum-dirtied-it-already case as you propose here, I think
we'd have quite a respectable improvement in vacuum behavior for 9.4,
even without Heikki's stuff.

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