public inbox for [email protected]
help / color / mirror / Atom feedFrom: Melanie Plageman <[email protected]>
To: Chao Li <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)
Date: Tue, 25 Nov 2025 16:43:58 -0500
Message-ID: <CAAKRu_YQ2P0HdL4+JXhM2EN6gTXf=6ma7ZaALrsAY0=q3QgaGQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAAKRu_YX0NP_yhXvPnvDRjVxxprsRBM-_MZzAJskfMydMQ=ETA@mail.gmail.com>
<CA+TgmoZef8XqRujP1NN=wJdV4SxOtu7rxRozsyAtaEvuVMZhEw@mail.gmail.com>
<CAAKRu_YxD3UC3BXxS55jPjBC_yj_vn3FVoLvBMwQuHXGDXacGg@mail.gmail.com>
<CA+TgmobYY2URHKBMh1NHo1zF3Z28TiS_+0aSyRYyBfvauCPZzA@mail.gmail.com>
<CAAKRu_YOJ3VTKo4Z9vB2hGeTnwVWsL39gXH09vyBUQ7bGtDnKA@mail.gmail.com>
<yn4zp35kkdsjx6wf47zcfmxgexxt4h2og47pvnw2x5ifyrs3qc@7uw6jyyxuyf7>
<CAAKRu_ZiuR+YcUc7=TrgANbRakpzCu8X9zqR=Tf0fE6uDbfP1g@mail.gmail.com>
<CA+TgmoYgCs=SEsohP6Z6R3KKsGaqFqvqxH8vQ_-nY4t+7rK8jg@mail.gmail.com>
<CAAKRu_ZP-3=SaZykpwDBMJOdUKyW3Wm5JZfPFRR3L5Ac8ouq4w@mail.gmail.com>
<CAAKRu_bgkOQqu3K5n4YLRsNBZqJ9Rjg80ROqgKSr2UGz4b5hUg@mail.gmail.com>
<2wk7jo4m4qwh5sn33pfgerdjfujebbccsmmlownybddbh6nawl@mdyyqpqzxjek>
<CAAKRu_YR-COJ9aGnMUQqt5yoWmUBjikqrd4jGNZYouHaXpis9g@mail.gmail.com>
<CALdSSPhiCwJwWwgJP1NmqRmnp9RS2tGOBY0gQrfLCbB+OS5_KQ@mail.gmail.com>
<CAAKRu_YS+Ocm=OzMaZnG4egFiE9v4VYfZ25DXd6jbwegqmGYbQ@mail.gmail.com>
<CAAKRu_ZGZSqhGt-RcmmfiSheC+1fjQdxy6_+oM-1jMn8hyVptQ@mail.gmail.com>
<CALdSSPg+B8RTzTXhJvCcKJBqgzhPZkq0E2oqxQdv74ZNZOMVzg@mail.gmail.com>
<CAAKRu_Zha7HcdQBv8tTtQrcry5332J6kHnOc1X=TT03LzUXDow@mail.gmail.com>
<CAAKRu_amF00f2T_H8N6pbbe75C22EeX1OqA=svpj8LNO1sdUuw@mail.gmail.com>
<[email protected]>
Thanks for the review!
On Thu, Nov 20, 2025 at 8:10 PM Chao Li <[email protected]> wrote:
>
> * new_relfrozen_xid and new_relmin_mxid must provided by the caller if the
> - * HEAP_PRUNE_FREEZE option is set. On entry, they contain the oldest XID and
> - * multi-XID seen on the relation so far. They will be updated with oldest
> - * values present on the page after pruning. After processing the whole
> - * relation, VACUUM can use these values as the new relfrozenxid/relminmxid
> - * for the relation.
> + * HEAP_PAGE_PRUNE_FREEZE option is set in params. On entry, they contain the
> + * oldest XID and multi-XID seen on the relation so far. They will be updated
> + * with oldest values present on the page after pruning. After processing the
> + * whole relation, VACUUM can use these values as the new
> + * relfrozenxid/relminmxid for the relation.
> */
> void
> -heap_page_prune_and_freeze(Relation relation, Buffer buffer,
> - GlobalVisState *vistest,
> - int options,
> - struct VacuumCutoffs *cutoffs,
> +heap_page_prune_and_freeze(PruneFreezeParams *params,
> PruneFreezeResult *presult,
> - PruneReason reason,
> OffsetNumber *off_loc,
> TransactionId *new_relfrozen_xid,
> MultiXactId *new_relmin_mxid)
> {
> ```
>
> For this function interface change, I got a concern. The old function comment says "cutoffs contains the freeze cutoffs …. Required if HEAP_PRUNE_FREEZE option is set.”, meaning that cutoffs is only useful and must be set when HEAP_PRUNE_FREEZE is set. But the new comment seems to have lost this indication.
I did move that comment into the PruneFreezeParams struct definition.
> And in the old function interface, cutoffs sat right next to options, readers are easy to notice:
>
> * when options is 0, cutoffs is null
> ```
> heap_page_prune_and_freeze(relation, buffer, vistest, 0,
> NULL, &presult, PRUNE_ON_ACCESS, &dummy_off_loc, NULL, NULL);
> ```
>
> * when options has HEAP_PAGE_PRUNE_FREEZE, cutoffs is passed in
> ```
> prune_options = HEAP_PAGE_PRUNE_FREEZE;
> if (vacrel->nindexes == 0)
> prune_options |= HEAP_PAGE_PRUNE_MARK_UNUSED_NOW;
>
> heap_page_prune_and_freeze(rel, buf, vacrel->vistest, prune_options,
> &vacrel->cutoffs, &presult, PRUNE_VACUUM_SCAN,
> &vacrel->offnum,
> &vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid);
> ```
>
> So, the change doesn’t break anything, but makes code a little bit harder to read. So, my suggestion is to add an assert in heap_page_prune_and_freeze, something like:
>
> Assert(!(params->options & HEAP_PAGE_PRUNE_FREEZE) || params->cutoffs != NULL);
That's fair. I've gone ahead and pushed a commit with your suggested assert.
> 2 - pushed 0001
> ```
> + PruneFreezeParams params = {.relation = rel,.buffer = buf,
> + .reason = PRUNE_VACUUM_SCAN,.options = HEAP_PAGE_PRUNE_FREEZE,
> + .cutoffs = &vacrel->cutoffs,.vistest = vacrel->vistest
> + };
> ```
>
> Using a designated initializer is not wrong, but makes future maintenance harder, because when a new field is added, this initializer will leave the new field uninitiated. From my impression, I don’t remember I ever see a designated initializer in PG code. I only remember 3 ways I have seen:
>
> * use an initialize function to set every fields individually
> * palloc0 to set all 0, then set non-zero fields individually
> * {0} to set all 0, then set non-zero fields individually
Well, the main reason you don't see them much in the code is that a
lot of the code is old and we didn't require a c99-compliant compiler
until fairly recently (okay like 2018/2019) -- and thus couldn't use
designated initializers.
I agree that they are rare for structs (they are quite commonly used
with arrays), but they are there -- for example these bufmgr init
macros
#define BMR_REL(p_rel) \
((BufferManagerRelation){.rel = p_rel})
#define BMR_SMGR(p_smgr, p_relpersistence) \
((BufferManagerRelation){.smgr = p_smgr, .relpersistence =
p_relpersistence})
#define BMR_GET_SMGR(bmr) \
(RelationIsValid((bmr).rel) ? RelationGetSmgr((bmr).rel) : (bmr).smgr)
I don't see how it would be harder to remember to initialize a field
with a designated initializer vs if you have to just remember to add a
line initializing that field in the code. And using a designated
initializer ensures all unspecified fields will be zeroed out.
In general, I have seen threads [1] encouraging the use of designated
initializers, so I'm inclined to leave it as is since it is committed,
and I haven't heard other pushback.
> 3 - pushed 0002
> ```
> prstate->all_visible = false;
> + prstate->all_frozen = false;
> ```
>
> Nit: Now setting the both fields to false repeat in 6 places. Maybe add a static inline function, say PruneClearVisibilityFlags(), may improve maintainability.
I see your point. However, I don't think it would necessarily be an
improvement. This function already has a lot of helpers you have to
jump to to understand what's going on. And in the place where they are
cleared most often, heap_prune_record_unchanged_lp_normal(), we set
other fields of the prstate directly, so it is nice visual symmetry in
my opinion to set them inline.
I did want to use chained assignment (all_visible = all_frozen =
false), but I have had people complain about that in my code before
more than once, so I refrained.
> 4 - pushed 0003
> ```
> + * opporunistically freeze, to indicate if the VM bits can be set. They are
> ```
>
> Typo: opporunistically, missed a “t”.
Fixed in same commit that added the assert.
- Melanie
[1] https://www.postgresql.org/message-id/flat/5B873BED.9080501%40anastigmatix.net#4a067c1314783f0e171b4...
view thread (143+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)
In-Reply-To: <CAAKRu_YQ2P0HdL4+JXhM2EN6gTXf=6ma7ZaALrsAY0=q3QgaGQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox