Message-ID: <b2ceeea0-abc5-49bb-ac0a-acd84940fc5c@iki.fi>
Date: Tue, 28 Oct 2025 13:40:24 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Consistently use the XLogRecPtrIsInvalid() macro
To: Quan Zongliang <quanzongliang@yeah.net>,
 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <aQB7EvGqrbZXrMlg@ip-10-97-1-34.eu-west-3.compute.internal>
 <20213178-92cd-4ab9-9699-c68cd75bae58@yeah.net>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <20213178-92cd-4ab9-9699-c68cd75bae58@yeah.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/b2ceeea0-abc5-49bb-ac0a-acd84940fc5c%40iki.fi>
Precedence: bulk

On 28/10/2025 10:53, Quan Zongliang wrote:
> On 10/28/25 4:13 PM, Bertrand Drouvot wrote:
>> While working on refactoring some code in [1], one of the changes was:
>>
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (initial_restart_lsn !=3D Inv=
alidXLogRecPtr &&
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 initial_=
restart_lsn < oldestLSN)
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 XLogRecPtr=C2=A0 restart_lsn =3D=
 s->data.restart_lsn;
>> +
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (restart_lsn !=3D InvalidXLog=
RecPtr &&
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 restart_=
lsn < oldestLSN)
>>
>> Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here.
>>
>> But this !=3D InvalidXLogRecPtr check was existing code, so why not=20
>> consistently use XLogRecPtrIsInvalid() where we check equality
>> against InvalidXLogRecPtr?
>>=20
>> At the time the current XLogRecPtrIsInvalid() has been introduced=20
>> (0ab9d1c4b316) all the InvalidXLogRecPtr equality checks were done
>> using XLogRecPtrIsInvalid().
>>=20
>> But since, it has changed: I looked at it and this is not the case=20
>> anymore in 20 files.
>>=20
>> PFA, patches to $SUBJECT.  To ease the review, I created one patch
>> per modified file.
>>=20
>> I suspect the same approach could be applied to some other macros=20
>> too.  Let's start with XLogRecPtrIsInvalid() first.
>=20
> Agree. This patch has made the code style more consistent. And using=20
> such macros is also in line with the usual practice. Just like=20
> OidIsValid and TransactionIdIsValid.

Back in the day, XLogRecPtrIsInvalid() was needed because XLogRecPtr was=20
a struct. 'x =3D=3D InvalidXLogRecPtr' simply did not work. I don't see m=
uch=20
need for it nowadays. In fact I wonder if we should go in the other=20
direction and replace XLogRecPtrIsInvalid(x) calls with 'x =3D=3D=20
InvalidXLogRecPtr'.

It's also a bit cumbersome that we have XLogRecPtrIsInvalid() rather=20
than XLogRecPtrIsValid(). That's inconsistent with OidIsValid and=20
TransactionIdInValid, and it leads to an awkward double negative 'if=20
(!XLogRecPtrIsInvalid(x))' if you want to check that 'x' is valid.

Overall I'm inclined to do nothing. But if anything, perhaps introduce=20
XLogRecPtrIsValid(x) and switch to that, or replace=20
XLogRecPtrIsInvalid(x) calls with 'x =3D=3D InvalidXLogRecPtr'

- Heikki