Thread: Consistently use macro HeapTupleIsValid to check the validity of tuples in tablecmds.c

On 09/04/2025 14:51, Tender Wang wrote:
> Hi,
> 
> While working on another patch, I find that tablecmds.c now has three
> ways to check the validity of catalog tuples.
> i.
> if (tuple != NULL)
> 
> ii.
> if (!tuple)
> 
> iii.
> if (HeapTupleIsValid(tuple)
> 
> In tablecmds.c, most checks use macro HeapTupleIsValid. For 
> code readability,
> I changed the first and the second formats to the third one, e.g., using 
> HeapTupleIsValid.
> 
> BTW,  I searched the other files, some files also have different ways to 
> check the validity of tuples.
> But the attached patch only focuses on tablecmds.c
> 
> Any thoughts?

It's a matter of taste, but personally I find 'if (tuple != NULL)' more 
clear than 'if (HeapTupleIsValid(tuple))'. The presence of a macro 
suggests that there might be other kinds of invalid tuples than a NULL 
pointer, which just adds mental load.

Inconsistency is not good either though. I'm not sure it's worth the 
churn, but I could get on board a patch to actually replace all 
HeapTupleIsValid(tuple) calls with plain "tuple != NULL" checks. Keep 
HeapTupleIsValid() just for compatibility, with a comment to discourage 
using it.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Inconsistency is not good either though. I'm not sure it's worth the 
> churn, but I could get on board a patch to actually replace all 
> HeapTupleIsValid(tuple) calls with plain "tuple != NULL" checks. Keep 
> HeapTupleIsValid() just for compatibility, with a comment to discourage 
> using it.

Would you then advocate for also removing macros such as OidIsValid()
and PointerIsValid()?  That gets into a *lot* of code churn, and
subsequent back-patching pain.  We had a discussion about that
just recently IIRC, and decided not to go there.

There's also the perennial issue of whether to write
"if (foo != NULL)" or just "if (foo)".  I'm not sure it's worth
trying to standardize that completely.

            regards, tom lane

On 09/04/2025 17:23, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> Inconsistency is not good either though. I'm not sure it's worth the
>> churn, but I could get on board a patch to actually replace all
>> HeapTupleIsValid(tuple) calls with plain "tuple != NULL" checks. Keep
>> HeapTupleIsValid() just for compatibility, with a comment to discourage
>> using it.
> 
> Would you then advocate for also removing macros such as OidIsValid()
> and PointerIsValid()?  That gets into a *lot* of code churn, and
> subsequent back-patching pain.  We had a discussion about that
> just recently IIRC, and decided not to go there.

PointerIsValid is pretty pointless, I think I'd be in favor of 
eliminating that.

OidIsValid() is a little more sensible. If you write "oid != 
InvalidOid", that reads as a double negative, "is oid not invalid".

But yeah, probably not worth the churn.

> There's also the perennial issue of whether to write
> "if (foo != NULL)" or just "if (foo)".  I'm not sure it's worth
> trying to standardize that completely.

Agreed. I use both, depending on which mood I'm in.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

On Wed, Apr 09, 2025 at 05:43:24PM +0300, Heikki Linnakangas wrote:
> Agreed. I use both, depending on which mood I'm in.

Same here, extended to OidIsValid(), HeapTupleIsValid(), XLogRecPtr,
etc., and I tend to prefer such macros, except if consistency of the
surroundings matter most.  FWIW, I think that living with the current
state of things to limit backpatch pain is fine.  There is no need to
change the existing code as an attempt to apply more standardization
even if one or more code grammar patterns mean the same thing.
--
Michael

On 09.04.25 14:26, Heikki Linnakangas wrote:
> It's a matter of taste, but personally I find 'if (tuple != NULL)' more 
> clear than 'if (HeapTupleIsValid(tuple))'. The presence of a macro 
> suggests that there might be other kinds of invalid tuples than a NULL 
> pointer, which just adds mental load.

agreed

> Inconsistency is not good either though. I'm not sure it's worth the 
> churn, but I could get on board a patch to actually replace all 
> HeapTupleIsValid(tuple) calls with plain "tuple != NULL" checks. Keep 
> HeapTupleIsValid() just for compatibility, with a comment to discourage 
> using it.

I'd be in favor of that.

On 09.04.25 16:23, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> Inconsistency is not good either though. I'm not sure it's worth the
>> churn, but I could get on board a patch to actually replace all
>> HeapTupleIsValid(tuple) calls with plain "tuple != NULL" checks. Keep
>> HeapTupleIsValid() just for compatibility, with a comment to discourage
>> using it.
> 
> Would you then advocate for also removing macros such as OidIsValid()
> and PointerIsValid()?  That gets into a *lot* of code churn, and
> subsequent back-patching pain.  We had a discussion about that
> just recently IIRC, and decided not to go there.

I'd generally be in favor of getting rid of these.  Many of these 
*IsValid macros generally don't actually do anything useful on top of 
plain C code, and they add a level of mystery and confusion.  No one is 
adding new ones like that, so over time, the coding styles diverge.  And 
they distract from macros that actually do something useful like 
AllocSizeIsValid().

In terms of backpatching effort/risk, here are some simple statistics:

git log --oneline REL_13_0..REL_13_20 | wc -l
1920

git log --oneline -G OidIsValid REL_13_0..REL_13_20 | wc -l
25

git log --oneline -G PointerIsValid REL_13_0..REL_13_20 | wc -l
5

So from that it would appear to be a relatively very small problem.