Thread: Re: Fix small typo, use InvalidRelFileNumber instead of InvalidOid

On 2024-Nov-08, Dilip Kumar wrote:

> It appears that we are passing InvalidOid instead of InvalidRelFileNumber
> when calling index_create(). While this isn’t technically a bug, since
> InvalidRelFileNumber is defined as InvalidOid, it’s preferable to use the
> correct identifier for clarity and consistency.

Oh ugh, this is quite a widespread problem actually, and it's just
because RelFileNumber is a typedef to Oid that the compiler doesn't
complain.  In a very quick desultory scan, I found a bunch of similar
issues elsewhere, such as below (also the assignment to relNumber in
GetNewRelFileNumber).  This isn't exhaustive by any means nor did I try
to compile it ... are you motivated to search for this more
exhaustively?  You could try (temporarily) making RelFileNumber a
typedef that tricks the compiler into creating a warning or error for
every mischief, such as a struct, fix all the warnings/errors, then
revert the temporary change.

diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 275615877eb..e2a73d88408 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -740,7 +740,7 @@ GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path,
      */
     rlocator.spcOid = spcoid;
     rlocator.dbOid = dboid;
-    rlocator.relNumber = 0;
+    rlocator.relNumber = InvalidRelFileNumber;
     if (BlockRefTableGetEntry(ib->brtab, &rlocator, MAIN_FORKNUM,
                               &limit_block) != NULL)
     {
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index 73a7592fb71..2f7f06a126f 100644
--- a/src/backend/bootstrap/bootparse.y
+++ b/src/backend/bootstrap/bootparse.y
@@ -206,7 +206,7 @@ Boot_CreateStmt:
                                                    PG_CATALOG_NAMESPACE,
                                                    shared_relation ? GLOBALTABLESPACE_OID : 0,
                                                    $3,
-                                                   InvalidOid,
+                                                   InvalidRelFileNumber,
                                                    HEAP_TABLE_AM_OID,
                                                    tupdesc,
                                                    RELKIND_RELATION,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f9bb721c5fe..f8d4824a3f8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3740,7 +3740,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
     if (set_tablespace)
     {
         /* Update its pg_class row */
-        SetRelationTableSpace(iRel, params->tablespaceOid, InvalidOid);
+        SetRelationTableSpace(iRel, params->tablespaceOid, InvalidRelFileNumber);
 
         /*
          * Schedule unlinking of the old index storage at transaction commit.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eaa81424270..8505cc8c1b5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15441,7 +15441,7 @@ ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
     }
 
     /* Update can be done, so change reltablespace */
-    SetRelationTableSpace(rel, newTableSpace, InvalidOid);
+    SetRelationTableSpace(rel, newTableSpace, InvalidRelFileNumber);
 
     InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
 
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 0fc2c093b0d..bffbb98779e 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -54,7 +54,7 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
 
     /* identifier of physical storage file */
     /* relfilenode == 0 means it is a "mapped" relation, see relmapper.c */
-    Oid            relfilenode BKI_DEFAULT(0);
+    RelFileNumber relfilenode BKI_DEFAULT(0);
 
     /* identifier of table space for relation (0 means default for database) */
     Oid            reltablespace BKI_DEFAULT(0) BKI_LOOKUP_OPT(pg_tablespace);


-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Dilip Kumar <dilipbalaut@gmail.com> writes:
> IIRC, In catalog we intentionally left it as Oid because RelFileNumber is
> an internal typedef bug, it is not an exposed datatype, so probably we can
> not use it in catalog.

We could declare it as RelFileNumber so that that is what C code sees,
and teach Catalog.pm to translate that to OID in the emitted catalog
contents.  I think you'd have to do that to avoid a ton of false
positives when you make RelFileNumber into a struct, and we might as
well keep the result.

            regards, tom lane

On Sat, Nov 9, 2024 at 12:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Nov 8, 2024 at 8:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Dilip Kumar <dilipbalaut@gmail.com> writes:
>> > IIRC, In catalog we intentionally left it as Oid because RelFileNumber is
>> > an internal typedef bug, it is not an exposed datatype, so probably we can
>> > not use it in catalog.
>>
>> We could declare it as RelFileNumber so that that is what C code sees,
>> and teach Catalog.pm to translate that to OID in the emitted catalog
>> contents.
>
>
> Yeah that make sense and yes we can actually keep this change
>
>>
>>   I think you'd have to do that to avoid a ton of false
>> positives when you make RelFileNumber into a struct, and we might as
>> well keep the result.
>
>
> Even after this, there were tons of false positives, whenever using any comparison operator on relfilenumbers[1] and
thereare tons of those, or using them in log messages with %u [2].  Anyway, I have gone through those manually and
afterignoring all false positives here is what I got, PFA patch (odd 25 places where I have fixed usage of Oid instead
ofRelFileNumbers). 
>
>
> [1]
> ../../../../src/include/storage/buf_internals.h:157:20: error: invalid operands to binary expression ('const
RelFileNumber'(aka 'const struct RelFileNumber') and 'const RelFileNumber') 
>                 (tag1->relNumber == tag2->relNumber)
>
> [2]
> fsmpage.c:277:47: warning: format specifies type 'unsigned int' but the argument has type 'RelFileNumber' (aka
'structRelFileNumber') [-Wformat] 
>                                  blknum, rlocator.spcOid, rlocator.dbOid, rlocator.relNumber);
>

Other than the changes, I have made in patch in my previous email
there are a couple of more items we can consider for this change, but
I am not sure so listing down here

1. We are using PG_RETURN_OID() for returning the relfilenumber,
should we introduce something like PG_RETURN_RELFILENUMBER() ? e.g
pg_relation_filenode()
2. We are using PG_GETARG_OID() for getting the relfilenumber as an
external function argument, shall we introduce something like
PG_GETARG_RELFILENUMBER() ? e.g.
binary_upgrade_set_next_heap_relfilenode()
3. info.c:611:23: error: assigning to 'RelFileNumber' (aka 'struct
RelFileNumber') from incompatible type 'Oid' (aka 'unsigned int')
                curr->relfilenumber = atooid(PQgetvalue(res, relnum,
i_relfilenumber));
4. Also using ObjectIdGetDatum() and DatumGetObjectId() for
relfilenumber so shall we introduce a new function i.e
RelFileNumberGetDatum() and DatumGetRelFileNumber()


--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com