public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Melanie Plageman <[email protected]>
Cc: Xuneng Zhou <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Kirill Reshke <[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, 23 Dec 2025 08:00:57 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAAKRu_ZCjHoRPfQ8AbMrFY8TOMCPAvZ0_m9SX7yg0edfTk45-g@mail.gmail.com>
References: <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>
<mhf4vkmh3j57zx7vuxp4jagtdzwhu3573pgfpmnjwqa6i6yj5y@sy4ymcdtdklo>
<CAAKRu_agCLAQ9OjmrTdJe-X=Xr7QnU4d=cfxdQGwc9jNx9w31w@mail.gmail.com>
<CAAKRu_ayWLg=WDGZZfSPWf0KjPM8u=LBb0D6XaEWyx2_YFFwAQ@mail.gmail.com>
<CABPTF7X1X810eT7hjRz9M69sjMv1Vui5gXQEGhqYXmkmywhHCQ@mail.gmail.com>
<CAAKRu_ZcK+ez71W7j+QzMo0gSafTsbGA3TU-fVSptw7yh41BEQ@mail.gmail.com>
<[email protected]>
<CAAKRu_ZCjHoRPfQ8AbMrFY8TOMCPAvZ0_m9SX7yg0edfTk45-g@mail.gmail.com>
> On Dec 23, 2025, at 01:57, Melanie Plageman <[email protected]> wrote:
>
> On Mon, Dec 22, 2025 at 2:20 AM Chao Li <[email protected]> wrote:
>>
>> A few more comments on v29:
>
> Thanks for the continued review! I've attached v30.
>
>> 1 - 0002 - Looks like since 0002, visibilitymap_set()’s return value is no longer used, so do we need to update the function and change return type to void? I remember in some patches, to address Coverity alerts, people had to do “(void) function_with_a_return_value()”.
>
> I was torn about whether or not to change the return value. Coverity
> doesn't always warn about unused return values. Usually it warns if it
> perceives the return value as needed for error checking or if it
> thinks not using the return value is incorrect. It may still warn in
> this case, but it's not obvious to me which way it would go.
>
> I have changed the function signature as you suggested in v30.
>
> My hesitation is that visibilitymap_set() is in a header file and
> could be used by extensions/forks, etc. Adding more information by
> changing a return value from void to non-void doesn't have any
> negative effect on those potential callers. But taking away a return
> value is more likely to affect them in a potentially negative way.
>
> However, I'm significantly changing the signature in this release, so
> everybody that used it will have to change their code completely
> anyway. Also, I just added a return value for visibilitymap_set() in
> the previous release (18). Historically, it returned void. So, I've
> gone with your suggestion.
From a previous patch, I learned from Peter Eisentraut that “We don't care about ABI changes in major releases.”, see:
https://www.postgresql.org/message-id/70913dbd-dadf-4560-9f81-c0df72bf6578%40eisentraut.org
>> - * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples, and
>> - * will return 'all_visible', 'all_frozen' flags to the caller.
>> + * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples
>>
>> Nit: a tailing dot is needed in the end of the comment line.
>
> I've changed it. One interesting thing is that our "policy" for
> periods in comments is that we don't put periods at the end of
> one-line comments and we do put them at the end of mult-line comment
> sentences. This is a one-line comment inside a comment block, so I
> wasn't sure what to do. If you noticed it, and it bothered you, it's
> easy enough to change, though.
If this is a one-line comment, I would have not been caring about the tailing period.
The problem is this is a paragraph of a block comment, and the above and below paragraphs all have tailing periods. So, for consistency, I raised the comment.
```
* HEAP_PAGE_PRUNE_MARK_UNUSED_NOW indicates that dead items can be set
* LP_UNUSED during pruning. <=== Has a tailing period
*
- * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples, and
- * will return 'all_visible', 'all_frozen' flags to the caller.
+ * HEAP_PAGE_PRUNE_FREEZE indicates that we will also freeze tuples <=== Not a tailing period
*
* HEAP_PAGE_PRUNE_UPDATE_VM indicates that we will set the page's status
* in the VM. <=== Has a tailing period
```
>
>> 9 - 0006
>>
>> @@ -3537,6 +3537,7 @@ heap_page_would_be_all_visible(Relation rel, Buffer buf,
>> {
>> ItemId itemid;
>> HeapTupleData tuple;
>> + TransactionId dead_after = InvalidTransactionId;
>> ```
>>
>> This initialization seems to not needed, as HeapTupleSatisfiesVacuumHorizon() will always set a value to it.
>
> I think this is a comment for a later patch in the set (you originally
> said it was from 0006), but I've changed dead_after to not be
> initialized like this.
My bad. This comment was actually for 0009. In v31, I see you have removed the initialization to dead_after.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
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], [email protected]
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)
In-Reply-To: <[email protected]>
* 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