Re: Consistently use the XLogRecPtrIsInvalid() macro

Hi,

On Fri, Nov 07, 2025 at 03:03:03PM +0000, Bertrand Drouvot wrote:
> I'm currently working on the RegProcedureIsValid() and OidIsValid() cases,
> will share once done.

here they are, I'm not creating a new thread for those as this is the same
kind of ideas (but for other types) but can create a dedicated one if you prefer.

That's a lot of changes so it is split in multiple patches to ease the review:

0001: makes use of RegProcedureIsValid() instead of direct comparisons with
InvalidOid or literal 0. That leads to some implicit boolean conversion to be
replaced by RegProcedureIsValid() checks. There are no literal 0 assignments on
RegProcedure type.

0002: makes use of OidIsValid() instead of direct comparisons with InvalidOid
or literal 0. That leads to some implicit boolean conversion to be replaced by
OidIsValid() checks. It covers the majority of cases.

0003: same as 0002 but for pointers/array.

0004: same as 0002 but for CATALOG (FormData_xxx) structs.

0005: same as 0002 but for functions returning Oid.

0006: same as 0002 but for remaining edge cases that have been done manually.

0007: replace ternary operators with !OidIsValid() rnd negated OidIsValid
equality to use positive logic.

0008: replace literal 0 with InvalidOid for Oid assignments.

Remarks:

- those patches (except 0006) have been generated using the .cocci scripts in
[1] (note that for the CATALOG patch, the macro_file.txt has to be used, see
the script header).

- comments are not changed, so there is still a few that contains "== InvalidOid"
while the underlying code is now using OidIsValid(). We may want to change
the comments too. I don't have a strong opinion on it just a small preference
to change the comments too. Thoughts?

Please note that, once the patch is applied, there is one InvalidOid direct
comparison done that is not on "Oid" or "RegProcedure" types:

$ git grep "!= InvalidOid"
src/backend/storage/lmgr/lock.c:        (locktag)->locktag_field1 != InvalidOid && \

I think this one should be replaced by != 0 instead because:

- locktag_field1 is uint32 (not RegProcedure or Oid)
- it can be assigned non Oid values (like xid, procNumber)

"
src/include/storage/lock.h:     uint32          locktag_field1; /* a 32-bit ID field */
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (xid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (vxid).procNumber, \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (xid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (id1), \
src/include/storage/lock.h:     ((locktag).locktag_field1 = (dboid), \
"

While the direct comparison to InvalidOid is technically correct, it is
confusing semantically because the field doesn't always contain an OID value.

I'll now look at the TransactionIdIsValid() cases.

[1]: https://github.com/bdrouvot/coccinelle_on_pg/tree/main/InvalidOid

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com