Re: Fix small typo, use InvalidRelFileNumber instead of InvalidOid

On Fri, Nov 8, 2024 at 4:30 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
wrote:

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

Okay I will try that

>
> 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);
>

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.

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