Fix small typo, use InvalidRelFileNumber instead of InvalidOid
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.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Fix-typo-use-InvalidRelFileNumber-instead-of-Invalid.patchapplication/octet-stream; name=0001-Fix-typo-use-InvalidRelFileNumber-instead-of-Invalid.patchDownload
From ef893379d00b98cea78379d121bf391ccafed9ad Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.kumar@enterprisedb.com>
Date: Fri, 8 Nov 2024 15:09:54 +0530
Subject: [PATCH] Fix typo, use InvalidRelFileNumber instead of InvalidOid
while passing input for RelFileNumber.
---
src/backend/catalog/toasting.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index ad3082c62a..b6b16bc9fe 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -319,7 +319,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
coloptions[1] = 0;
index_create(toast_rel, toast_idxname, toastIndexOid, InvalidOid,
- InvalidOid, InvalidOid,
+ InvalidOid, InvalidRelFileNumber,
indexInfo,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
--
2.39.2 (Apple Git-143)
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 wecan
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
Attachments:
remove_using_oid_for_relfilenumber.patchapplication/octet-stream; name=remove_using_oid_for_relfilenumber.patchDownload
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 275615877e..e2a73d8840 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 73a7592fb7..2f7f06a126 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/Catalog.pm b/src/backend/catalog/Catalog.pm
index 8e709524cb..74d74ec1f8 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -32,6 +32,7 @@ sub ParseHeader
'int32' => 'int4',
'int64' => 'int8',
'Oid' => 'oid',
+ 'RelFileNumber' => 'oid',
'NameData' => 'name',
'TransactionId' => 'xid',
'XLogRecPtr' => 'pg_lsn');
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index c54a543c53..340faeeab7 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -346,7 +346,7 @@ heap_create(const char *relname,
* with oid same as relid.
*/
if (!RelFileNumberIsValid(relfilenumber))
- relfilenumber = relid;
+ relfilenumber = (RelFileNumber) relid;
}
/*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f9bb721c5f..53e489be44 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3740,7 +3740,8 @@ 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/catalog/storage.c b/src/backend/catalog/storage.c
index f56b3cc0f2..316d393b84 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -612,7 +612,8 @@ RestorePendingSyncs(char *startAddress)
RelFileLocator *rlocator;
Assert(pendingSyncHash == NULL);
- for (rlocator = (RelFileLocator *) startAddress; rlocator->relNumber != 0;
+ for (rlocator = (RelFileLocator *) startAddress;
+ rlocator->relNumber != InvalidRelFileNumber;
rlocator++)
AddPendingSync(rlocator);
}
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index ad3082c62a..b6b16bc9fe 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -319,7 +319,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
coloptions[1] = 0;
index_create(toast_rel, toast_idxname, toastIndexOid, InvalidOid,
- InvalidOid, InvalidOid,
+ InvalidOid, InvalidRelFileNumber,
indexInfo,
list_make2("chunk_id", "chunk_seq"),
BTREE_AM_OID,
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index ae0863d9a2..0ae521163b 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1072,6 +1072,7 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
RelFileNumber relfilenumber1,
relfilenumber2;
RelFileNumber swaptemp;
+ Oid swaptempoid;
char swptmpchr;
Oid relam1,
relam2;
@@ -1107,13 +1108,13 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
relform1->relfilenode = relform2->relfilenode;
relform2->relfilenode = swaptemp;
- swaptemp = relform1->reltablespace;
+ swaptempoid = relform1->reltablespace;
relform1->reltablespace = relform2->reltablespace;
- relform2->reltablespace = swaptemp;
+ relform2->reltablespace = swaptempoid;
- swaptemp = relform1->relam;
+ swaptempoid = relform1->relam;
relform1->relam = relform2->relam;
- relform2->relam = swaptemp;
+ relform2->relam = swaptempoid;
swptmpchr = relform1->relpersistence;
relform1->relpersistence = relform2->relpersistence;
@@ -1122,9 +1123,9 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
/* Also swap toast links, if we're swapping by links */
if (!swap_toast_by_content)
{
- swaptemp = relform1->reltoastrelid;
+ swaptempoid = relform1->reltoastrelid;
relform1->reltoastrelid = relform2->reltoastrelid;
- relform2->reltoastrelid = swaptemp;
+ relform2->reltoastrelid = swaptempoid;
}
}
else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eaa8142427..8505cc8c1b 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/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c
index 48350bec52..4ca9b52b04 100644
--- a/src/backend/postmaster/walsummarizer.c
+++ b/src/backend/postmaster/walsummarizer.c
@@ -1277,7 +1277,7 @@ SummarizeDbaseRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
(xl_dbase_create_file_copy_rec *) XLogRecGetData(xlogreader);
rlocator.spcOid = xlrec->tablespace_id;
rlocator.dbOid = xlrec->db_id;
- rlocator.relNumber = 0;
+ rlocator.relNumber = InvalidRelFileNumber;
BlockRefTableSetLimitBlock(brtab, &rlocator, MAIN_FORKNUM, 0);
}
else if (info == XLOG_DBASE_CREATE_WAL_LOG)
@@ -1288,7 +1288,7 @@ SummarizeDbaseRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
xlrec = (xl_dbase_create_wal_log_rec *) XLogRecGetData(xlogreader);
rlocator.spcOid = xlrec->tablespace_id;
rlocator.dbOid = xlrec->db_id;
- rlocator.relNumber = 0;
+ rlocator.relNumber = InvalidRelFileNumber;
BlockRefTableSetLimitBlock(brtab, &rlocator, MAIN_FORKNUM, 0);
}
else if (info == XLOG_DBASE_DROP)
@@ -1299,7 +1299,7 @@ SummarizeDbaseRecord(XLogReaderState *xlogreader, BlockRefTable *brtab)
xlrec = (xl_dbase_drop_rec *) XLogRecGetData(xlogreader);
rlocator.dbOid = xlrec->db_id;
- rlocator.relNumber = 0;
+ rlocator.relNumber = InvalidRelFileNumber;
for (i = 0; i < xlrec->ntablespaces; ++i)
{
rlocator.spcOid = xlrec->tablespace_ids[i];
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index cc8a80ee96..a6721189ad 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1453,7 +1453,7 @@ ForgetDatabaseSyncRequests(Oid dbid)
rlocator.dbOid = dbid;
rlocator.spcOid = 0;
- rlocator.relNumber = 0;
+ rlocator.relNumber = InvalidRelFileNumber;
INIT_MD_FILETAG(tag, rlocator, InvalidForkNumber, InvalidBlockNumber);
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index e63e99c141..ac12459361 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -890,7 +890,7 @@ pg_relation_filenode(PG_FUNCTION_ARGS)
if (RELKIND_HAS_STORAGE(relform->relkind))
{
- if (relform->relfilenode)
+ if (RelFileNumberIsValid(relform->relfilenode))
result = relform->relfilenode;
else /* Consult the relation mapper */
result = RelationMapOidToFilenumber(relid,
@@ -973,7 +973,7 @@ pg_relation_filepath(PG_FUNCTION_ARGS)
rlocator.dbOid = InvalidOid;
else
rlocator.dbOid = MyDatabaseId;
- if (relform->relfilenode)
+ if (RelFileNumberIsValid(relform->relfilenode))
rlocator.relNumber = relform->relfilenode;
else /* Consult the relation mapper */
rlocator.relNumber = RelationMapOidToFilenumber(relid,
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 5bbb654a5d..63c8055135 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3746,23 +3746,23 @@ RelationSetNewRelfilenumber(Relation relation, char persistence)
}
else if (relation->rd_rel->relkind == RELKIND_INDEX)
{
- if (!OidIsValid(binary_upgrade_next_index_pg_class_relfilenumber))
+ if (!RelFileNumberIsValid(binary_upgrade_next_index_pg_class_relfilenumber))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("index relfilenumber value not set when in binary upgrade mode")));
newrelfilenumber = binary_upgrade_next_index_pg_class_relfilenumber;
- binary_upgrade_next_index_pg_class_relfilenumber = InvalidOid;
+ binary_upgrade_next_index_pg_class_relfilenumber = InvalidRelFileNumber;
}
else if (relation->rd_rel->relkind == RELKIND_RELATION)
{
- if (!OidIsValid(binary_upgrade_next_heap_pg_class_relfilenumber))
+ if (!RelFileNumberIsValid(binary_upgrade_next_heap_pg_class_relfilenumber))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("heap relfilenumber value not set when in binary upgrade mode")));
newrelfilenumber = binary_upgrade_next_heap_pg_class_relfilenumber;
- binary_upgrade_next_heap_pg_class_relfilenumber = InvalidOid;
+ binary_upgrade_next_heap_pg_class_relfilenumber = InvalidRelFileNumber;
}
else
ereport(ERROR,
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 0fc2c093b0..48ebbc4b0d 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);
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 2dabbe01ec..82f33c5b87 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -22,7 +22,6 @@
/*
* RelFileNumber data type identifies the specific relation file name.
*/
-typedef Oid RelFileNumber;
#define InvalidRelFileNumber ((RelFileNumber) InvalidOid)
#define RelFileNumberIsValid(relnumber) \
((bool) ((relnumber) != InvalidRelFileNumber))
diff --git a/src/include/postgres_ext.h b/src/include/postgres_ext.h
index 240ad4e93b..ef6f26fd47 100644
--- a/src/include/postgres_ext.h
+++ b/src/include/postgres_ext.h
@@ -30,6 +30,8 @@
*/
typedef unsigned int Oid;
+typedef Oid RelFileNumber;
+
#ifdef __cplusplus
#define InvalidOid (Oid(0))
#else
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