Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM
 on-access)
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAAKRu_ZCjHoRPfQ8AbMrFY8TOMCPAvZ0_m9SX7yg0edfTk45-g@mail.gmail.com>
Date: Tue, 23 Dec 2025 08:00:57 +0800
Cc: Xuneng Zhou <xunengzhou@gmail.com>,
 Andres Freund <andres@anarazel.de>,
 Kirill Reshke <reshkekirill@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Andrey Borodin <x4mmm@yandex-team.ru>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Heikki Linnakangas <hlinnaka@iki.fi>
Content-Transfer-Encoding: quoted-printable
Message-Id: <7F5BCD7A-764D-4D8D-8E27-6F2CAAEA1CEE@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>
 <6BC5DBAB-6084-4BB8-8450-52E9648AB021@gmail.com>
 <CAAKRu_ZCjHoRPfQ8AbMrFY8TOMCPAvZ0_m9SX7yg0edfTk45-g@mail.gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/7F5BCD7A-764D-4D8D-8E27-6F2CAAEA1CEE%40gmail.com>
Precedence: bulk


> On Dec 23, 2025, at 01:57, Melanie Plageman =
<melanieplageman@gmail.com> wrote:
>=20
> On Mon, Dec 22, 2025 at 2:20=E2=80=AFAM Chao Li =
<li.evan.chao@gmail.com> wrote:
>>=20
>> A few more comments on v29:
>=20
> Thanks for the continued review! I've attached v30.
>=20
>> 1 - 0002 - Looks like since 0002, visibilitymap_set()=E2=80=99s =
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 =E2=80=9C(void) =
function_with_a_return_value()=E2=80=9D.
>=20
> 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.
>=20
> I have changed the function signature as you suggested in v30.
>=20
> 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.
>=20
> 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.

=46rom a previous patch, I learned from Peter Eisentraut that =E2=80=9CWe =
don't care about ABI changes in major releases.=E2=80=9D, 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
>>=20
>> Nit: a tailing dot is needed in the end of the comment line.
>=20
> 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.   <=3D=3D=3D 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 <=3D=3D=3D Not a tailing period
 	 *
 	 * HEAP_PAGE_PRUNE_UPDATE_VM indicates that we will set the =
page's status
 	 * in the VM.                                 <=3D=3D=3D Has a =
tailing period
```

>=20
>> 9 - 0006
>>=20
>> @@ -3537,6 +3537,7 @@ heap_page_would_be_all_visible(Relation rel, =
Buffer buf,
>>        {
>>                ItemId          itemid;
>>                HeapTupleData tuple;
>> +               TransactionId dead_after =3D InvalidTransactionId;
>> ```
>>=20
>> This initialization seems to not needed, as =
HeapTupleSatisfiesVacuumHorizon() will always set a value to it.
>=20
> 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/