Re: Combine Prune and Freeze records emitted by vacuum

On 09/03/2024 22:41, Melanie Plageman wrote:
> On Wed, Mar 6, 2024 at 7:59 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Does GlobalVisTestIsRemovableXid() handle FrozenTransactionId correctly?
> 
> Okay, so I thought a lot about this, and I don't understand how
> GlobalVisTestIsRemovableXid() would not handle FrozenTransactionId
> correctly.
> 
> vacrel->cutoffs.OldestXmin is computed initially from
> GetOldestNonRemovableTransactionId() which uses ComputeXidHorizons().
> GlobalVisState is updated by ComputeXidHorizons() (when it is
> updated).
> 
> I do see that the comment above GlobalVisTestIsRemovableXid() says
> 
>   * It is crucial that this only gets called for xids from a source that
>   * protects against xid wraparounds (e.g. from a table and thus protected by
>   * relfrozenxid).
> 
> and then in
> 
>       * Convert 32 bit argument to FullTransactionId. We can do so safely
>       * because we know the xid has to, at the very least, be between
>       * [oldestXid, nextXid), i.e. within 2 billion of xid.
> 
> I'm not sure what oldestXid is here.
> It's true that I don't see GlobalVisTestIsRemovableXid() being called
> anywhere else with an xmin as an input. I think that hints that it is
> not safe with FrozenTransactionId. But I don't see what could go
> wrong.
> 
> Maybe it has something to do with converting it to a FullTransactionId?
> 
>     FullTransactionIdFromU64(U64FromFullTransactionId(rel)  + (int32)
> (xid - rel_xid));
> 
> Sorry, I couldn't quite figure it out :(

I just tested it. No, GlobalVisTestIsRemovableXid does not work for 
FrozenTransactionId. I just tested it with state->definitely_needed == 
{0, 4000000000} and xid == FrozenTransactionid, and it incorrectly 
returned 'false'. It treats FrozenTransactionId as if was a regular xid '2'.

>> The XLOG_HEAP2_FREEZE_PAGE record is a little smaller than
>> XLOG_HEAP2_PRUNE. But we could optimize the XLOG_HEAP2_PRUNE format for
>> the case that there's no pruning, just freezing. The record format
>> (xl_heap_prune) looks pretty complex as it is, I think it could be made
>> both more compact and more clear with some refactoring.
> 
> I'm happy to change up xl_heap_prune format. In its current form,
> according to pahole, it has no holes and just 3 bytes of padding at
> the end.
> 
> One way we could make it smaller is by moving the isCatalogRel member
> to directly after snapshotConflictHorizon and then adding a flags
> field and defining flags to indicate whether or not other members
> exist at all. We could set bits for HAS_FREEZE_PLANS, HAS_REDIRECTED,
> HAS_UNUSED, HAS_DEAD. Then I would remove the non-optional uint16
> nredirected, nunused, nplans, ndead and just put the number of
> redirected/unused/etc at the beginning of the arrays containing the
> offsets.

Sounds good.

> Then I could write various macros for accessing them. That
> would make it smaller, but it definitely wouldn't make it less complex
> (IMO).

I don't know, it might turn out not that complex. If you define the 
formats of each of those "sub-record types" as explicit structs, per 
attached sketch, you won't need so many macros. Some care is still 
needed with alignment though.

-- 
Heikki Linnakangas
Neon (https://neon.tech)