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

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 15:26:07 +0300

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)

---

From: Tom Lane <[email-masked]>
Date: Wed, 09 Apr 2025 10:23:00 -0400

> HeapTupleIsValid() just for compatibility, with a comment to discourage 

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

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 10 Apr 2025 14:37:36 +0200

> 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.