On 20/03/2024 03:36, Melanie Plageman wrote:
> On Mon, Mar 18, 2024 at 01:15:21AM +0200, Heikki Linnakangas wrote:
>> On 15/03/2024 02:56, Melanie Plageman wrote:
>>> Okay, so I was going to start using xl_heap_prune for vacuum here too,
>>> but I realized it would be bigger because of the
>>> snapshotConflictHorizon. Do you think there is a non-terrible way to
>>> make the snapshotConflictHorizon optional? Like with a flag?
>>
>> Yeah, another flag would do the trick.
>
> Okay, I've done this in attached v4 (including removing
> XLOG_HEAP2_VACUUM). I had to put the snapshot conflict horizon in the
> "main chunk" of data available at replay regardless of whether or not
> the record ended up including an FPI.
>
> I made it its own sub-record (xlhp_conflict_horizon) less to help with
> alignment (though we can use all the help we can get there) and more to
> keep it from getting lost. When you look at heapam_xlog.h, you can see
> what a XLOG_HEAP2_PRUNE record will contain starting with the
> xl_heap_prune struct and then all the sub-record types.
Ok, now that I look at this, I wonder if we're being overly cautious
about the WAL size. We probably could just always include the snapshot
field, and set it to InvalidTransactionId and waste 4 bytes when it's
not needed. For the sake of simplicity. I don't feel strongly either way
though, the flag is pretty simple too.
> xl_heap_prune->flags is a uint8, but we are already using 7 of the bits.
> Should we make it a unit16?
It doesn't matter much either way. We can also make it larger when we
need more bits, there's no need make room for them them beforehand.
> Eventually, I would like to avoid emitting a separate XLOG_HEAP2_VISIBLE
> record for vacuum's first and second passes and just include the VM
> update flags in the xl_heap_prune record. xl_heap_visible->flags is a
> uint8. If we made xl_heap_prune->flags uint16, we could probably combine
> them (though maybe we want other bits available). Also vacuum's second
> pass doesn't set a snapshotConflictHorizon, so if we combined
> xl_heap_visible and xl_heap_prune for vacuum we would end up saving even
> more space (since vacuum sets xl_heap_visible->snapshotConflictHorizon
> to InvalidXLogRecPtr).
Makes sense.
> A note on sub-record naming: I kept xl_heap_freeze_plan's name but
> prefixed the other sub-records with xlhp. Do you think it is worth
> renaming it (to xlhp_freeze_plan)?
Yeah, perhaps.
> Also, should I change xlhp_freeze to xlhp_freeze_page?
I renamed it to xlhp_freeze_plans, for some consistency with
xlhp_prune_items.
I realized that the WAL record format changes are pretty independent
from the rest of the patches. They could be applied before the rest.
Without the rest of the changes, we'll still write two WAL records per
page in vacuum, one to prune and another one to freeze, but it's another
meaningful incremental step. So I reshuffled the patches, so that the
WAL format is changed first, before the rest of the changes.
0001-0008: These are the WAL format changes. There's some comment
cleanup needed, but as far as the code goes, I think these are pretty
much ready to be squashed & committed.
0009-: The rest of the v4 patches, rebased over the WAL format changes.
I also added a few small commits for little cleanups that caught my eye,
let me know if you disagree with those.
--
Heikki Linnakangas
Neon (https://neon.tech)