Fix small typo, use InvalidRelFileNumber instead of InvalidOid

On Fri, Nov 8, 2024 at 3:17 PM Dilip Kumar <dilipbalaut@gmail.com> 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.

Agreed. We already use InvalidRelFileNumber at one of the other
callers in index_concurrently_create_copy(). Your patch looks good to
me.

--
With Regards,
Amit Kapila.

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/

On Fri, Nov 8, 2024 at 4:30 PM Alvaro Herrera <alvherre@alvh.no-ip.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

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 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]../../../../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) and there are tons of those, or
using them in log messages with %u [2]fsmpage.c:277:47: warning: format specifies type 'unsigned int' but the argument has type 'RelFileNumber' (aka 'struct RelFileNumber') [-Wformat] blknum, rlocator.spcOid, rlocator.dbOid, rlocator.relNumber);. Anyway, I have gone through those
manually and after ignoring all false positives here is what I got, PFA
patch (odd 25 places where I have fixed usage of Oid instead of
RelFileNumbers).

[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)
../../../../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 'struct RelFileNumber') [-Wformat] blknum, rlocator.spcOid, rlocator.dbOid, rlocator.relNumber);
fsmpage.c:277:47: warning: format specifies type 'unsigned int' but the
argument has type 'RelFileNumber' (aka 'struct RelFileNumber') [-Wformat]
blknum, rlocator.spcOid, rlocator.dbOid,
rlocator.relNumber);

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

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 there are tons of those, or using them in log messages with %u [2]. Anyway, I have gone through those manually and after ignoring all false positives here is what I got, PFA patch (odd 25 places where I have fixed usage of Oid instead of RelFileNumbers).

[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 'struct RelFileNumber') [-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