Re: 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)