Consistently use macro HeapTupleIsValid to check the validity of tuples in tablecmds.c
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?
--
Thanks, Tender Wang
Attachments:
0001-Use-macro-HeapTupleIsValid-to-check-tuple-valid-or-n.patchtext/plain; charset=US-ASCII; name=0001-Use-macro-HeapTupleIsValid-to-check-tuple-valid-or-n.patchDownload
From 13bfbba547df1db9a441eafd9d1aef194ad38a9e Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Wed, 9 Apr 2025 19:25:59 +0800
Subject: [PATCH] Use macro HeapTupleIsValid to check tuple valid or not.
Now, in tablecmds.c, we have three different format to check catalog
tuple be valid or not.
if (tuple != NULL)
...
if (!tuple)
...
if (HeapTupleIsValid(tuple))
...
For more codes readability, using macro HeapTupleIsValid to check the
validity of tuples.
---
src/backend/commands/tablecmds.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 686f1850cab..cd0eaa2efa6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7097,7 +7097,7 @@ find_typed_table_dependencies(Oid typeOid, const char *typeName, DropBehavior be
scan = table_beginscan_catalog(classRel, 1, key);
- while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+ while (HeapTupleIsValid(tuple = heap_getnext(scan, ForwardScanDirection)))
{
Form_pg_class classform = (Form_pg_class) GETSTRUCT(tuple);
@@ -7798,7 +7798,7 @@ ATExecDropNotNull(Relation rel, const char *colName, bool recurse,
* dropconstraint_internal() resets attnotnull.
*/
conTup = findNotNullConstraintAttnum(RelationGetRelid(rel), attnum);
- if (conTup == NULL)
+ if (!HeapTupleIsValid(conTup))
elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation \"%s\"",
colName, RelationGetRelationName(rel));
@@ -9470,7 +9470,7 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
Form_pg_attribute attrForm;
tup = SearchSysCacheAttName(childrelid, strVal(attname));
- if (!tup)
+ if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for attribute %s of relation %u",
strVal(attname), childrelid);
attrForm = (Form_pg_attribute) GETSTRUCT(tup);
@@ -9496,7 +9496,7 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
* valid, though.
*/
tuple = findNotNullConstraint(RelationGetRelid(rel), strVal(column));
- if (tuple != NULL)
+ if (HeapTupleIsValid(tuple))
{
Form_pg_constraint conForm = (Form_pg_constraint) GETSTRUCT(tuple);
@@ -11207,7 +11207,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel)
/* This is a seqscan, as we don't have a usable index ... */
scan = systable_beginscan(pg_constraint, InvalidOid, true,
NULL, 2, key);
- while ((tuple = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(tuple = systable_getnext(scan)))
{
Form_pg_constraint constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
@@ -11878,7 +11878,7 @@ RemoveInheritedConstraint(Relation conrel, Relation trigrel, Oid conoid,
ConstraintRelidTypidNameIndexId,
true, NULL, 1, &key);
objs = new_object_addresses();
- while ((consttup = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(consttup = systable_getnext(scan)))
{
Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(consttup);
@@ -11915,7 +11915,7 @@ RemoveInheritedConstraint(Relation conrel, Relation trigrel, Oid conoid,
ObjectIdGetDatum(conform->oid));
scan2 = systable_beginscan(trigrel, TriggerConstraintIndexId,
true, NULL, 1, &key2);
- while ((trigtup = systable_getnext(scan2)) != NULL)
+ while (HeapTupleIsValid(trigtup = systable_getnext(scan2)))
{
ObjectAddressSet(addr, TriggerRelationId,
((Form_pg_trigger) GETSTRUCT(trigtup))->oid);
@@ -11955,7 +11955,7 @@ DropForeignKeyConstraintTriggers(Relation trigrel, Oid conoid, Oid confrelid,
ObjectIdGetDatum(conoid));
scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true,
NULL, 1, &key);
- while ((trigtup = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
{
Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup);
ObjectAddress trigger;
@@ -12027,7 +12027,7 @@ GetForeignKeyActionTriggers(Relation trigrel,
scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true,
NULL, 1, &key);
- while ((trigtup = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
{
Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup);
@@ -12088,7 +12088,7 @@ GetForeignKeyCheckTriggers(Relation trigrel,
scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true,
NULL, 1, &key);
- while ((trigtup = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
{
Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup);
@@ -12597,7 +12597,7 @@ ATExecAlterConstrInheritability(List **wqueue, ATAlterConstraint *cmdcon,
Form_pg_constraint childcon;
childtup = findNotNullConstraint(childoid, colName);
- if (!childtup)
+ if (!HeapTupleIsValid(childtup))
elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation %u",
colName, childoid);
childcon = (Form_pg_constraint) GETSTRUCT(childtup);
@@ -13189,7 +13189,7 @@ QueueNNConstraintValidation(List **wqueue, Relation conrel, Relation rel,
* column name.
*/
contup = findNotNullConstraint(childoid, colname);
- if (!contup)
+ if (!HeapTupleIsValid(contup))
elog(ERROR, "cache lookup failed for not-null constraint on column \"%s\" of relation \"%s\"",
colname, get_rel_name(childoid));
childcon = (Form_pg_constraint) GETSTRUCT(contup);
@@ -21707,7 +21707,7 @@ validatePartitionedIndex(Relation partedIdx, Relation partedTbl)
ObjectIdGetDatum(RelationGetRelid(partedIdx)));
scan = systable_beginscan(inheritsRel, InheritsParentIndexId, true,
NULL, 1, &key);
- while ((inhTup = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(inhTup = systable_getnext(scan)))
{
Form_pg_inherits inhForm = (Form_pg_inherits) GETSTRUCT(inhTup);
HeapTuple indTup;
@@ -21837,7 +21837,7 @@ GetParentedForeignKeyRefs(Relation partition)
/* XXX This is a seqscan, as we don't have a usable index */
scan = systable_beginscan(pg_constraint, InvalidOid, true, NULL, 2, key);
- while ((tuple = systable_getnext(scan)) != NULL)
+ while (HeapTupleIsValid(tuple = systable_getnext(scan)))
{
Form_pg_constraint constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
--
2.34.1
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.cAny 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
Michael Paquier <michael@paquier.xyz> 于2025年4月10日周四 10:53写道：
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.
OK, makes sense.
--
Thanks, Tender Wang
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.