pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed"unexpected duplicate for tablespace" problem in logical replication
25+ messages / 9 participants
[nested] [flat]
* "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-06 02:14 wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
0 siblings, 1 reply; 25+ messages in thread
From: wangsh.fnst@fujitsu.com @ 2022-04-06 02:14 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Hi,
I met a problem while using logical replication in PG11 and I think all
the PG version have this problem.
The log looks like:
> ERROR: unexpected duplicate for tablespace 0, relfilenode xxxxxxx
Someone also reported this problem in [1], but no one has responded to it.
I did some investigation, and found a way to reproduce this problem. The
steps are:
1. create a table (call it tableX) and truncate it.
2. cycle through 2^32 OIDs.
3. restart the database to clear all the cache.
4. create a temp table which make the temp table's OID equals to the
tableX's relfilenode and insert any data into tableX.
The attachment(run.sh) can reproduce this problem in PG10 and PG11with
the help of option 'WITH OIDS'. I don't find any way to cycle the OIDs
quickly in branch master, but I use the gdb to reproduce this problem too.
Now, function GetNewRelFileNode() only checks:
1. duplicated OIDs in pg_class.
2. relpath(rnode) is exists in disk.
However, the result of relpath(temp table) and relpath(non-temp table)
are different, temp table's relpath() has a prefix "t%d". That means, if
there is a table that value of relfilenode is 20000(but the value of oid
isn't 20000), it's possible to create a temp table that value of
relfilenode is also 20000.
I think function GetNewRelFileNode() should always check the duplicated
relfilenode, see the patch(a simple to way to fix this problem is master
branch).
Any comment?
Regards,
Shenhao Wang
[1]
https://www.postgresql.org/message-id/flat/CAM5YvKTPxmMT%3DS7iPcu5SgmaOv4S4nhE1HZRO_sdFX9cXeXXOQ%40m...
Attachments:
[application/x-shellscript] run.sh (1.4K, ../../bbaaf9f9-ebb2-645f-54bb-34d6efc7ac42@fujitsu.com/2-run.sh)
download
[text/x-patch] 0001-Check-the-duplicated-relfilenode-when-create-table-o.patch (4.4K, ../../bbaaf9f9-ebb2-645f-54bb-34d6efc7ac42@fujitsu.com/3-0001-Check-the-duplicated-relfilenode-when-create-table-o.patch)
download | inline diff:
From bc3a756e71bef744f1c9cf652c0eb0e4933c965b Mon Sep 17 00:00:00 2001
From: Shenhao Wang <wangsh.fnst@fujitsu.com>
Date: Sat, 2 Apr 2022 11:03:44 +0800
Subject: [PATCH] Check the duplicated relfilenode when create table or
truncate table.
---
src/backend/catalog/catalog.c | 102 ++++++++++++++++++++++++++--------
1 file changed, 78 insertions(+), 24 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index dfd5fb669e..7f51445573 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -55,6 +55,8 @@
#define GETNEWOID_LOG_THRESHOLD 1000000
#define GETNEWOID_LOG_MAX_INTERVAL 128000000
+static bool isRelNodeCollision(RelFileNodeBackend rnode, Relation relation);
+
/*
* IsSystemRelation
* True iff the relation is either a system catalog or a toast table.
@@ -490,8 +492,7 @@ Oid
GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
{
RelFileNodeBackend rnode;
- char *rpath;
- bool collides;
+ bool usedAsOID;
BackendId backend;
/*
@@ -515,6 +516,16 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
return InvalidOid; /* placate compiler */
}
+ if (pg_class != NULL)
+ {
+ usedAsOID = true;
+ }
+ else
+ {
+ pg_class = table_open(RelationRelationId, AccessShareLock);
+ usedAsOID = false;
+ }
+
/* This logic should match RelationInitPhysicalAddr */
rnode.node.spcNode = reltablespace ? reltablespace : MyDatabaseTableSpace;
rnode.node.dbNode = (rnode.node.spcNode == GLOBALTABLESPACE_OID) ? InvalidOid : MyDatabaseId;
@@ -531,34 +542,16 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
CHECK_FOR_INTERRUPTS();
/* Generate the OID */
- if (pg_class)
+ if (usedAsOID)
rnode.node.relNode = GetNewOidWithIndex(pg_class, ClassOidIndexId,
Anum_pg_class_oid);
else
rnode.node.relNode = GetNewObjectId();
- /* Check for existing file of same name */
- rpath = relpath(rnode, MAIN_FORKNUM);
+ } while (isRelNodeCollision(rnode, pg_class));
- if (access(rpath, F_OK) == 0)
- {
- /* definite collision */
- collides = true;
- }
- else
- {
- /*
- * Here we have a little bit of a dilemma: if errno is something
- * other than ENOENT, should we declare a collision and loop? In
- * practice it seems best to go ahead regardless of the errno. If
- * there is a colliding file we will get an smgr failure when we
- * attempt to create the new relation file.
- */
- collides = false;
- }
-
- pfree(rpath);
- } while (collides);
+ if (!usedAsOID)
+ table_close(pg_class, AccessShareLock);
return rnode.node.relNode;
}
@@ -666,3 +659,64 @@ pg_stop_making_pinned_objects(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+
+/*
+ * isRelNodeCollision
+ * Checking the rnode collides with existing one.
+ */
+static bool
+isRelNodeCollision(RelFileNodeBackend rnode, Relation relation)
+{
+ char *rpath;
+ bool collides;
+ SysScanDesc scandesc;
+ static ScanKeyData skey[2];
+
+ /* Check for existing file of same name */
+ rpath = relpath(rnode, MAIN_FORKNUM);
+
+ if (access(rpath, F_OK) == 0)
+ {
+ /* definite collision */
+ collides = true;
+ }
+ else
+ {
+ ScanKeyInit(&skey[0],
+ Anum_pg_class_reltablespace,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(
+ rnode.node.spcNode == MyDatabaseTableSpace ?
+ InvalidOid : MyDatabaseTableSpace));
+ ScanKeyInit(&skey[1],
+ Anum_pg_class_relfilenode,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(rnode.node.relNode));
+
+ scandesc = systable_beginscan(relation,
+ ClassTblspcRelfilenodeIndexId,
+ true, NULL, 2, skey);
+
+ if (HeapTupleIsValid(systable_getnext(scandesc)))
+ {
+ /* duplicate tablespace and relfilenode in pg_class */
+ collides = true;
+ }
+ else
+ {
+ /*
+ * Here we have a little bit of a dilemma: if errno is something
+ * other than ENOENT, should we declare a collision and loop? In
+ * practice it seems best to go ahead regardless of the errno. If
+ * there is a colliding file we will get an smgr failure when we
+ * attempt to create the new relation file.
+ */
+ collides = false;
+ }
+
+ systable_endscan(scandesc);
+ }
+
+ pfree(rpath);
+ return collides;
+}
--
2.32.0
^ permalink raw reply [nested|flat] 25+ messages in thread
* RE: "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-08 09:44 osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
parent: wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
0 siblings, 1 reply; 25+ messages in thread
From: osumi.takamichi@fujitsu.com @ 2022-04-08 09:44 UTC (permalink / raw)
To: wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Wednesday, April 6, 2022 11:14 AM wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com> wrote:
> I met a problem while using logical replication in PG11 and I think all the PG
> version have this problem.
>
>
> The log looks like:
> > ERROR: unexpected duplicate for tablespace 0, relfilenode xxxxxxx
> Someone also reported this problem in [1], but no one has responded to it.
>
>
>
> I did some investigation, and found a way to reproduce this problem. The
> steps are:
>
>
> 1. create a table (call it tableX) and truncate it.
>
>
> 2. cycle through 2^32 OIDs.
>
>
> 3. restart the database to clear all the cache.
>
>
> 4. create a temp table which make the temp table's OID equals to the
> tableX's relfilenode and insert any data into tableX.
>
>
> The attachment(run.sh) can reproduce this problem in PG10 and PG11with
> the help of option 'WITH OIDS'. I don't find any way to cycle the OIDs
> quickly in branch master, but I use the gdb to reproduce this problem too.
>
>
>
> Now, function GetNewRelFileNode() only checks:
>
>
> 1. duplicated OIDs in pg_class.
>
>
> 2. relpath(rnode) is exists in disk.
>
>
> However, the result of relpath(temp table) and relpath(non-temp table)
> are different, temp table's relpath() has a prefix "t%d". That means, if
> there is a table that value of relfilenode is 20000(but the value of oid
> isn't 20000), it's possible to create a temp table that value of
> relfilenode is also 20000.
>
>
> I think function GetNewRelFileNode() should always check the duplicated
> relfilenode, see the patch(a simple to way to fix this problem is master
> branch).
>
>
> Any comment?
Hi, thank you for your report.
It seems correct that there's room that wraparounded oid can be used for temp table,
and we get duplicate result when we retrieve it and face the error.
I reproduced your issue with HEAD and gdb, by replacing rnode.node.relNode with
an existing relfilenode in GetNewRelFileNode(), immediately before the call of relpath().
At the same time, I also confirmed that with your patch, I had another iteration
call of GetNewOidWithIndex() and got different oid, even if I inserted an existing
relfilenode in the same way. I didn't get the same error with your patch.
Below are my several review comments on your patch.
(1) Name of isRelNodeCollision
How about changing it to "IsCollidedRelNode" ?
When I see around of this code, there are functions
named as "IsReservedName", "IsSharedRelation" or "IsPinnedObject".
So, my suggestion would be more aligned.
(2) Remove curly brackets for one sentence
+ if (pg_class != NULL)
+ {
+ usedAsOID = true;
+ }
+ else
+ {
+ pg_class = table_open(RelationRelationId, AccessShareLock);
+ usedAsOID = false;
+ }
+
Please remove curly brackets for one sentence.
Also, I suggest you add a comment something like
"Ensure we have opened pg_class in this function"
at the top of this check for clarification.
(3) variables of isRelNodeCollision
+ SysScanDesc scandesc;
+ static ScanKeyData skey[2];
We can move those declaration into the inside of
the conditional branch for scankey.
(4) Having Assert in isRelNodeCollision
How about add an Assert to check the argument
relation is not NULL ?
(5) argument name of isRelNodeCollision
I suggest you change it to pg_class.
Best Regards,
Takamichi Osumi
^ permalink raw reply [nested|flat] 25+ messages in thread
* RE: "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-11 07:40 osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
parent: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
0 siblings, 1 reply; 25+ messages in thread
From: osumi.takamichi@fujitsu.com @ 2022-04-11 07:40 UTC (permalink / raw)
To: wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Friday, April 8, 2022 6:44 PM I wrote:
> On Wednesday, April 6, 2022 11:14 AM wangsh.fnst@fujitsu.com
> <wangsh.fnst@fujitsu.com> wrote:
> > I met a problem while using logical replication in PG11 and I think
> > all the PG version have this problem.
> >
> >
> > The log looks like:
> > > ERROR: unexpected duplicate for tablespace 0, relfilenode xxxxxxx
> > Someone also reported this problem in [1], but no one has responded to it.
> >
> >
> >
> > I did some investigation, and found a way to reproduce this problem.
> > The steps are:
> >
> >
> > 1. create a table (call it tableX) and truncate it.
> >
> >
> > 2. cycle through 2^32 OIDs.
> >
> >
> > 3. restart the database to clear all the cache.
> >
> >
> > 4. create a temp table which make the temp table's OID equals to the
> > tableX's relfilenode and insert any data into tableX.
> >
> >
> > The attachment(run.sh) can reproduce this problem in PG10 and PG11with
> > the help of option 'WITH OIDS'. I don't find any way to cycle the OIDs
> > quickly in branch master, but I use the gdb to reproduce this problem too.
> >
> >
> >
> > Now, function GetNewRelFileNode() only checks:
> >
> >
> > 1. duplicated OIDs in pg_class.
> >
> >
> > 2. relpath(rnode) is exists in disk.
> >
> >
> > However, the result of relpath(temp table) and relpath(non-temp table)
> > are different, temp table's relpath() has a prefix "t%d". That means,
> > if there is a table that value of relfilenode is 20000(but the value
> > of oid isn't 20000), it's possible to create a temp table that value
> > of relfilenode is also 20000.
> >
> >
> > I think function GetNewRelFileNode() should always check the
> > duplicated relfilenode, see the patch(a simple to way to fix this
> > problem is master branch).
> >
> >
> > Any comment?
> Hi, thank you for your report.
>
>
> It seems correct that there's room that wraparounded oid can be used for temp
> table, and we get duplicate result when we retrieve it and face the error.
>
> I reproduced your issue with HEAD and gdb, by replacing rnode.node.relNode
> with an existing relfilenode in GetNewRelFileNode(), immediately before the
> call of relpath().
One thing I forgot to note is that this bug is not unique to the logical replication.
There is other path to hit it for example, pg_filenode_relation
in the same procedures with gdb.
In the below output, I created tempa table with the same filenode with gdb
without having a pair of logical replication and got the same error you reported.
postgres=# select oid, relname, relfilenode, reltablespace from pg_class where relname in ('c', 'tempa');
oid | relname | relfilenode | reltablespace
-------+---------+-------------+---------------
16387 | c | 16390 | 0
16390 | tempa | 16390 | 0
(2 rows)
postgres=# select pg_filenode_relation(0, 16390);
ERROR: unexpected duplicate for tablespace 0, relfilenode 16390
Best Regards,
Takamichi Osumi
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-11 10:24 wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
parent: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
0 siblings, 1 reply; 25+ messages in thread
From: wangsh.fnst@fujitsu.com @ 2022-04-11 10:24 UTC (permalink / raw)
To: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Hi, Osumi-san, Thank you for your comment.
On 4/11/22 15:40, Osumi, Takamichi/大墨 昂道 wrote:
> (1) Name of isRelNodeCollision
changed.
> (2) Remove curly brackets for one sentence
changed.
> (3) variables of isRelNodeCollision
changed.
> (4) Having Assert in isRelNodeCollision
added.
> (5) argument name of isRelNodeCollision
changed.
BTW, I also change the parameter from null to SnapshotAny
scandesc = systable_beginscan(pg_class,
ClassTblspcRelfilenodeIndexId,
- true, NULL, 2, skey);
+ true, SnapshotAny, 2, skey);
I'm not sure, but I think SnapshotAny is more suitable after reading the
source(comment) in function GetNewOidWithIndex().
Please see the attachment
Regards,
Shenhao Wang
Attachments:
[text/x-patch] v2-0001-Check-the-duplicated-relfilenode-when-create-tabl.patch (4.1K, ../../dff776f7-fd73-6b14-a6c0-fba8c3858735@fujitsu.com/2-v2-0001-Check-the-duplicated-relfilenode-when-create-tabl.patch)
download | inline diff:
From a7c872e45e04f0079a150b844b7827305de77d6c Mon Sep 17 00:00:00 2001
From: Shenhao Wang <wangsh.fnst@fujitsu.com>
Date: Mon, 11 Apr 2022 18:12:34 +0800
Subject: [PATCH v2] Check the duplicated relfilenode when create table or
truncate table.
---
src/backend/catalog/catalog.c | 95 ++++++++++++++++++++++++++---------
1 file changed, 71 insertions(+), 24 deletions(-)
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 520f77971b..4bf3a181e6 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -56,6 +56,8 @@
#define GETNEWOID_LOG_THRESHOLD 1000000
#define GETNEWOID_LOG_MAX_INTERVAL 128000000
+static bool IsCollidedRelNode(RelFileNodeBackend rnode, Relation relation);
+
/*
* IsSystemRelation
* True iff the relation is either a system catalog or a toast table.
@@ -496,9 +498,8 @@ Oid
GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
{
RelFileNodeBackend rnode;
- char *rpath;
- bool collides;
BackendId backend;
+ bool isCreateTable = (pg_class != NULL);
/*
* If we ever get here during pg_upgrade, there's something wrong; all
@@ -532,39 +533,28 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
*/
rnode.backend = backend;
+ /*
+ * We should check the new relfilenode is duplicated in pg_class, so we
+ * should open pg_class.
+ */
+ if (!isCreateTable)
+ pg_class = table_open(RelationRelationId, AccessShareLock);
+
do
{
CHECK_FOR_INTERRUPTS();
/* Generate the OID */
- if (pg_class)
+ if (isCreateTable)
rnode.node.relNode = GetNewOidWithIndex(pg_class, ClassOidIndexId,
Anum_pg_class_oid);
else
rnode.node.relNode = GetNewObjectId();
- /* Check for existing file of same name */
- rpath = relpath(rnode, MAIN_FORKNUM);
+ } while (IsCollidedRelNode(rnode, pg_class));
- if (access(rpath, F_OK) == 0)
- {
- /* definite collision */
- collides = true;
- }
- else
- {
- /*
- * Here we have a little bit of a dilemma: if errno is something
- * other than ENOENT, should we declare a collision and loop? In
- * practice it seems best to go ahead regardless of the errno. If
- * there is a colliding file we will get an smgr failure when we
- * attempt to create the new relation file.
- */
- collides = false;
- }
-
- pfree(rpath);
- } while (collides);
+ if (!isCreateTable)
+ table_close(pg_class, AccessShareLock);
return rnode.node.relNode;
}
@@ -672,3 +662,60 @@ pg_stop_making_pinned_objects(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+
+/*
+ * IsCollidedRelNode
+ * Checking the rnode collides with existing one.
+ */
+static bool
+IsCollidedRelNode(RelFileNodeBackend rnode, Relation pg_class)
+{
+ char *rpath;
+ bool collides;
+
+ Assert(pg_class);
+
+ /* Check for existing file of same name */
+ rpath = relpath(rnode, MAIN_FORKNUM);
+
+ if (access(rpath, F_OK) == 0)
+ {
+ /* definite collision */
+ collides = true;
+ }
+ else
+ {
+ SysScanDesc scandesc;
+ ScanKeyData skey[2];
+
+ ScanKeyInit(&skey[0],
+ Anum_pg_class_reltablespace,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(
+ rnode.node.spcNode == MyDatabaseTableSpace ?
+ InvalidOid : MyDatabaseTableSpace));
+ ScanKeyInit(&skey[1],
+ Anum_pg_class_relfilenode,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(rnode.node.relNode));
+
+ /* See notes in GetNewOidWithIndex about using SnapshotAny */
+ scandesc = systable_beginscan(pg_class,
+ ClassTblspcRelfilenodeIndexId,
+ true, SnapshotAny, 2, skey);
+
+ /*
+ * Here we have a little bit of a dilemma: if errno is something
+ * other than ENOENT, should we declare a collision and loop? In
+ * practice it seems best to go ahead regardless of the errno. If
+ * there is a colliding file we will get an smgr failure when we
+ * attempt to create the new relation file.
+ */
+ collides = HeapTupleIsValid(systable_getnext(scandesc));
+
+ systable_endscan(scandesc);
+ }
+
+ pfree(rpath);
+ return collides;
+}
--
2.32.0
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-12 02:45 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
0 siblings, 2 replies; 25+ messages in thread
From: Kyotaro Horiguchi @ 2022-04-12 02:45 UTC (permalink / raw)
To: wangsh.fnst@fujitsu.com; +Cc: osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
At Mon, 11 Apr 2022 10:24:26 +0000, "wangsh.fnst@fujitsu.com" <wangsh.fnst@fujitsu.com> wrote in
> Please see the attachment
Sorry for being late, but this seems to be in the wrong direction.
In the problem case, as you know, GetNewRelFileNode does *not* check
oid uniqueness in pg_class. The on-catalog duplicate check is done
only when the result is to be used as the *table oid* at creation. In
other cases it chooses a relfilenode that does not duplicate in
storage file names irrelevantly to the relation oid. As the result
Non-temporary and temporary tables have separate relfilenode oid
spaces, either intentional or not..
Thus, I think we don't want GetNewRelFileNode to check for duplicate
oid on pg_class if pg_class is not passed since that significantly
narrow the oid space for relfilenode. If we did something like that,
we might do check the existence of file names of the both non-temp and
temp in GetNewRelFileNode(), but that also narrows the relfilenode oid
space.
RelidByRelfilenode is called by autoprewarm, logical replication, and
pg_filenode_relation. In the autoprewarm and logical replication
cases, it is called only for non-temprary relations so letting the
function ignore (oid duplicating) temp tables works.
pg_relfilienode_relation is somewhat dubious. It is affected by this
bug. In the attached PoC, to avoid API change (in case for
back-patching), RelidByRelfilenode ignores duplcates of differernt
persistence. Also the PoC prioritize on persistent tables to
temporary ones but I'm not sure it's good decision, but otherwise we
need to change the signature of pg_filenode_relation.
The attached is an PoC of that. The first one is a reproduction-aid
code. With it applied, the following steps create duplicate
relfilenode relations.
(apply the first diff)
$ rm /tmp/hoge
$ (start postgres)
# touch /tmp/hoge
$ psql
=# create table xp (a int);
=# truncate xp;
=# create temp table xt (a int);
=# truncate xt;
=# select oid, relname, relfilenode from pg_class where relname like 'x%';
oid | relname | relfilenode
-------+---------+-------------
16384 | xp | 55555
55558 | xt | 55555
(2 rows)
=# select pg_filenode_relation(0, 55555);
ERROR: unexpected duplicate for tablespace 0, relfilenode 55555
(apply the second diff)
=# create temp table xt (a int);
=# truncate xt;
=# select pg_filenode_relation(0, 55555);
pg_filenode_relation
----------------------
xp
(1 row)
What do you think about this direction?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 43f14c233d..2c668ff190 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3708,6 +3708,11 @@ RelationSetNewRelfilenode(Relation relation, char persistence)
RelFileNode newrnode;
/* Allocate a new relfilenode */
+ {
+ struct stat b;
+ if (stat("/tmp/hoge", &b) == 0)
+ ShmemVariableCache->nextOid = 55555;
+ }
newrelfilenode = GetNewRelFileNode(relation->rd_rel->reltablespace, NULL,
persistence);
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index 70c323c720..5d0b39a243 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -130,7 +130,8 @@ InitializeRelfilenodeMap(void)
/*
* Map a relation's (tablespace, filenode) to a relation's oid and cache the
- * result.
+ * result. If both non-temprary and temprary reltations are found, persistent
+ * one is choosed.
*
* Returns InvalidOid if no relation matching the criteria could be found.
*/
@@ -183,6 +184,8 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
}
else
{
+ bool is_temp = false;
+
/*
* Not a shared table, could either be a plain relation or a
* non-shared, nailed one, like e.g. pg_class.
@@ -212,14 +215,25 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
if (found)
- elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenode %u",
- reltablespace, relfilenode);
+ {
+ /* check if the same entry for the same persistency exists */
+ if (is_temp ?
+ classform->relpersistence == RELPERSISTENCE_TEMP :
+ classform->relpersistence != RELPERSISTENCE_TEMP)
+ elog(ERROR,
+ "unexpected duplicate for tablespace %u, relfilenode %u",
+ reltablespace, relfilenode);
+
+ /* prioritize non-temporary if both are found */
+ if (!is_temp)
+ continue;
+ }
found = true;
Assert(classform->reltablespace == reltablespace);
Assert(classform->relfilenode == relfilenode);
relid = classform->oid;
+ is_temp = (classform->relpersistence == RELPERSISTENCE_TEMP);
}
systable_endscan(scandesc);
Attachments:
[text/plain] generate_same_relfilenode.txt (556B, ../../20220412.114539.151610727144113536.horikyota.ntt@gmail.com/2-generate_same_relfilenode.txt)
download | inline diff:
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 43f14c233d..2c668ff190 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3708,6 +3708,11 @@ RelationSetNewRelfilenode(Relation relation, char persistence)
RelFileNode newrnode;
/* Allocate a new relfilenode */
+ {
+ struct stat b;
+ if (stat("/tmp/hoge", &b) == 0)
+ ShmemVariableCache->nextOid = 55555;
+ }
newrelfilenode = GetNewRelFileNode(relation->rd_rel->reltablespace, NULL,
persistence);
[text/plain] allow_dup_relfilenodeid_PoC.txt (1.7K, ../../20220412.114539.151610727144113536.horikyota.ntt@gmail.com/3-allow_dup_relfilenodeid_PoC.txt)
download | inline diff:
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index 70c323c720..5d0b39a243 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -130,7 +130,8 @@ InitializeRelfilenodeMap(void)
/*
* Map a relation's (tablespace, filenode) to a relation's oid and cache the
- * result.
+ * result. If both non-temprary and temprary reltations are found, persistent
+ * one is choosed.
*
* Returns InvalidOid if no relation matching the criteria could be found.
*/
@@ -183,6 +184,8 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
}
else
{
+ bool is_temp = false;
+
/*
* Not a shared table, could either be a plain relation or a
* non-shared, nailed one, like e.g. pg_class.
@@ -212,14 +215,25 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
if (found)
- elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenode %u",
- reltablespace, relfilenode);
+ {
+ /* check if the same entry for the same persistency exists */
+ if (is_temp ?
+ classform->relpersistence == RELPERSISTENCE_TEMP :
+ classform->relpersistence != RELPERSISTENCE_TEMP)
+ elog(ERROR,
+ "unexpected duplicate for tablespace %u, relfilenode %u",
+ reltablespace, relfilenode);
+
+ /* prioritize non-temporary if both are found */
+ if (!is_temp)
+ continue;
+ }
found = true;
Assert(classform->reltablespace == reltablespace);
Assert(classform->relfilenode == relfilenode);
relid = classform->oid;
+ is_temp = (classform->relpersistence == RELPERSISTENCE_TEMP);
}
systable_endscan(scandesc);
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-12 03:36 wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
1 sibling, 0 replies; 25+ messages in thread
From: wangsh.fnst@fujitsu.com @ 2022-04-12 03:36 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 4/12/22 10:45, Kyotaro Horiguchi wrote:
> What do you think about this direction?
I think this direction is better then mine. This POC looks good to me.
Regards,
Shenhao Wang
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2022-04-19 06:23 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
1 sibling, 1 reply; 25+ messages in thread
From: Masahiko Sawada @ 2022-04-19 06:23 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
On Tue, Apr 12, 2022 at 11:45 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 11 Apr 2022 10:24:26 +0000, "wangsh.fnst@fujitsu.com" <wangsh.fnst@fujitsu.com> wrote in
> > Please see the attachment
>
> Sorry for being late, but this seems to be in the wrong direction.
>
> In the problem case, as you know, GetNewRelFileNode does *not* check
> oid uniqueness in pg_class. The on-catalog duplicate check is done
> only when the result is to be used as the *table oid* at creation. In
> other cases it chooses a relfilenode that does not duplicate in
> storage file names irrelevantly to the relation oid. As the result
> Non-temporary and temporary tables have separate relfilenode oid
> spaces, either intentional or not..
>
> Thus, I think we don't want GetNewRelFileNode to check for duplicate
> oid on pg_class if pg_class is not passed since that significantly
> narrow the oid space for relfilenode. If we did something like that,
> we might do check the existence of file names of the both non-temp and
> temp in GetNewRelFileNode(), but that also narrows the relfilenode oid
> space.
Agreed.
>
> RelidByRelfilenode is called by autoprewarm, logical replication, and
> pg_filenode_relation. In the autoprewarm and logical replication
> cases, it is called only for non-temprary relations so letting the
> function ignore (oid duplicating) temp tables works.
> pg_relfilienode_relation is somewhat dubious. It is affected by this
> bug. In the attached PoC, to avoid API change (in case for
> back-patching), RelidByRelfilenode ignores duplcates of differernt
> persistence. Also the PoC prioritize on persistent tables to
> temporary ones but I'm not sure it's good decision, but otherwise we
> need to change the signature of pg_filenode_relation.
>
> The attached is an PoC of that. The first one is a reproduction-aid
> code. With it applied, the following steps create duplicate
> relfilenode relations.
>
> What do you think about this direction?
Sounds good to me. If we go in this direction, probably we also need
to change the cache checks in RelidByRelfilenode() so that we
prioritize non-temporary relations. Otherwise, we will end up
returning the OID of temporary relation if it's already cached.
Another idea I came up with is that we have RelidByRelfilenode() check
only non-temporary relations by adding relpersistence !=
RELPERSISTENCE_TEMP to the scan key. If we want to support temporary
relations too, I think that, in v16, we can add relpersistence to
RelidByRelfilenode() as a function argument (and a flag to
pg_filenode_relation() accordingly) so that the user can get the name
of the temporary relation by like pg_filenode_relation(0, 16386,
true). On the other hand, in back branches, if an application is using
pg_filenode_relation() to get the name of temporary relations, it
would break it.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-01-08 05:02 vignesh C <vignesh21@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: vignesh C @ 2024-01-08 05:02 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
On Tue, 19 Apr 2022 at 11:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Apr 12, 2022 at 11:45 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Mon, 11 Apr 2022 10:24:26 +0000, "wangsh.fnst@fujitsu.com" <wangsh.fnst@fujitsu.com> wrote in
> > > Please see the attachment
> >
> > Sorry for being late, but this seems to be in the wrong direction.
> >
> > In the problem case, as you know, GetNewRelFileNode does *not* check
> > oid uniqueness in pg_class. The on-catalog duplicate check is done
> > only when the result is to be used as the *table oid* at creation. In
> > other cases it chooses a relfilenode that does not duplicate in
> > storage file names irrelevantly to the relation oid. As the result
> > Non-temporary and temporary tables have separate relfilenode oid
> > spaces, either intentional or not..
> >
> > Thus, I think we don't want GetNewRelFileNode to check for duplicate
> > oid on pg_class if pg_class is not passed since that significantly
> > narrow the oid space for relfilenode. If we did something like that,
> > we might do check the existence of file names of the both non-temp and
> > temp in GetNewRelFileNode(), but that also narrows the relfilenode oid
> > space.
>
> Agreed.
>
> >
> > RelidByRelfilenode is called by autoprewarm, logical replication, and
> > pg_filenode_relation. In the autoprewarm and logical replication
> > cases, it is called only for non-temprary relations so letting the
> > function ignore (oid duplicating) temp tables works.
> > pg_relfilienode_relation is somewhat dubious. It is affected by this
> > bug. In the attached PoC, to avoid API change (in case for
> > back-patching), RelidByRelfilenode ignores duplcates of differernt
> > persistence. Also the PoC prioritize on persistent tables to
> > temporary ones but I'm not sure it's good decision, but otherwise we
> > need to change the signature of pg_filenode_relation.
> >
> > The attached is an PoC of that. The first one is a reproduction-aid
> > code. With it applied, the following steps create duplicate
> > relfilenode relations.
> >
> > What do you think about this direction?
>
> Sounds good to me. If we go in this direction, probably we also need
> to change the cache checks in RelidByRelfilenode() so that we
> prioritize non-temporary relations. Otherwise, we will end up
> returning the OID of temporary relation if it's already cached.
>
> Another idea I came up with is that we have RelidByRelfilenode() check
> only non-temporary relations by adding relpersistence !=
> RELPERSISTENCE_TEMP to the scan key. If we want to support temporary
> relations too, I think that, in v16, we can add relpersistence to
> RelidByRelfilenode() as a function argument (and a flag to
> pg_filenode_relation() accordingly) so that the user can get the name
> of the temporary relation by like pg_filenode_relation(0, 16386,
> true). On the other hand, in back branches, if an application is using
> pg_filenode_relation() to get the name of temporary relations, it
> would break it.
I have changed the status to "Waiting on Author" as the points raised
by Masahiko Sawada-san is pending.
Regards,
Vignesh
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-01-12 08:37 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Kyotaro Horiguchi @ 2024-01-12 08:37 UTC (permalink / raw)
To: vignesh21@gmail.com; +Cc: sawada.mshk@gmail.com; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
At Mon, 8 Jan 2024 10:32:03 +0530, vignesh C <vignesh21@gmail.com> wrote in
> I have changed the status to "Waiting on Author" as the points raised
> by Masahiko Sawada-san is pending.
Oops! This slipped off from my mind. Thank you for the reminder.
> On Tue, 19 Apr 2022 at 11:54, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > What do you think about this direction?
> >
> > Sounds good to me. If we go in this direction, probably we also need
> > to change the cache checks in RelidByRelfilenode() so that we
> > prioritize non-temporary relations. Otherwise, we will end up
> > returning the OID of temporary relation if it's already cached.
Maybe. It might be better for the cache not to register temprary
relations at all.
> > Another idea I came up with is that we have RelidByRelfilenode() check
> > only non-temporary relations by adding relpersistence !=
> > RELPERSISTENCE_TEMP to the scan key.
I'm not sure it is good considering the case pg_relationumber_relation
for a temporary relation.
> > If we want to support temporary
> > relations too, I think that, in v16, we can add relpersistence to
> > RelidByRelfilenode() as a function argument (and a flag to
> > pg_filenode_relation() accordingly) so that the user can get the name
> > of the temporary relation by like pg_filenode_relation(0, 16386,
> > true). On the other hand, in back branches, if an application is using
> > pg_filenode_relation() to get the name of temporary relations, it
> > would break it.
Agreed. So, the attachd files are the patches for the master and older
versions respectively.
If there is a concern with the patch for the master, the parameter
handling of pg_filenode_relation might be not good. If the feature of
searching for relations regardless of their persistence is
unnecessary, it could be simplified. (Anyway it lacks documentation at
all).
Also, while the patch for version 11 is attached, I am unable to build
this version on my system (although I haven't investigated this
deeply, but I'm on Rocky 9 now).
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] RelidByRelfilenumber_prefer_perm_rels_master.patch (6.2K, ../../20240112.173746.1710988914918513917.horikyota.ntt@gmail.com/2-RelidByRelfilenumber_prefer_perm_rels_master.patch)
download | inline diff:
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 9085a409db..67a3f2783d 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -511,7 +511,8 @@ autoprewarm_database_main(Datum main_arg)
Assert(rel == NULL);
StartTransactionCommand();
- reloid = RelidByRelfilenumber(blk->tablespace, blk->filenumber);
+ reloid = RelidByRelfilenumber(blk->tablespace, blk->filenumber,
+ RELPERSISTENCE_PERMANENT);
if (OidIsValid(reloid))
rel = try_relation_open(reloid, AccessShareLock);
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index f315fecf18..73978c7756 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -404,6 +404,11 @@ CREATE OR REPLACE FUNCTION
RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend'
PARALLEL SAFE;
+CREATE OR REPLACE FUNCTION
+ pg_filenode_relation(tablespace oid, relfilenumber oid, persistence "char" DEFAULT NULL)
+ RETURNS regclass VOLATILE LANGUAGE internal AS 'pg_filenode_relation'
+ PARALLEL SAFE;
+
-- legacy definition for compatibility with 9.3
CREATE OR REPLACE FUNCTION
json_populate_record(base anyelement, from_json json, use_json_as_text boolean DEFAULT false)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index d1334ffb55..5a0bcb9f59 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2185,7 +2185,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Assert(snapshot_now);
reloid = RelidByRelfilenumber(change->data.tp.rlocator.spcOid,
- change->data.tp.rlocator.relNumber);
+ change->data.tp.rlocator.relNumber,
+ RELPERSISTENCE_PERMANENT);
/*
* Mapped catalog tuple without data, emitted while
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 2df025d50a..efabffa6e6 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -928,10 +928,26 @@ pg_filenode_relation(PG_FUNCTION_ARGS)
Oid heaprel;
/* test needed so RelidByRelfilenumber doesn't misbehave */
- if (!RelFileNumberIsValid(relfilenumber))
+ if (PG_ARGISNULL(0) || PG_ARGISNULL(1) ||
+ !RelFileNumberIsValid(relfilenumber))
PG_RETURN_NULL();
- heaprel = RelidByRelfilenumber(reltablespace, relfilenumber);
+ if (!PG_ARGISNULL(2))
+ heaprel = RelidByRelfilenumber(reltablespace, relfilenumber,
+ PG_GETARG_CHAR(2));
+ else
+ {
+ /* search for any kind of persistence. for backward compatibility */
+ char persistences[] = {'p', 'u', 't'};
+
+ for (int i = 0 ; i < 3 ; i++)
+ {
+ heaprel = RelidByRelfilenumber(reltablespace, relfilenumber,
+ persistences[i]);
+ if (OidIsValid(heaprel))
+ break;
+ }
+ }
if (!OidIsValid(heaprel))
PG_RETURN_NULL();
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index b7caa84341..3c4b94c434 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -38,6 +38,7 @@ typedef struct
{
Oid reltablespace;
RelFileNumber relfilenumber;
+ char persistence;
} RelfilenumberMapKey;
typedef struct
@@ -135,7 +136,8 @@ InitializeRelfilenumberMap(void)
* Returns InvalidOid if no relation matching the criteria could be found.
*/
Oid
-RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
+RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber,
+ char persistence)
{
RelfilenumberMapKey key;
RelfilenumberMapEntry *entry;
@@ -156,6 +158,7 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
MemSet(&key, 0, sizeof(key));
key.reltablespace = reltablespace;
key.relfilenumber = relfilenumber;
+ key.persistence = persistence;
/*
* Check cache and return entry if one is found. Even if no target
@@ -211,10 +214,16 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
{
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
+ if (classform->relpersistence != persistence)
+ continue;
+
if (found)
+ {
elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenumber %u",
- reltablespace, relfilenumber);
+ "unexpected duplicate for tablespace %u, relfilenumber %u, persistence %c",
+ reltablespace, relfilenumber, persistence);
+
+ }
found = true;
Assert(classform->reltablespace == reltablespace);
@@ -226,7 +235,7 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
table_close(relation, AccessShareLock);
/* check for tables that are mapped but not shared */
- if (!found)
+ if (!found && persistence == RELPERSISTENCE_PERMANENT)
relid = RelationMapFilenumberToOid(relfilenumber, false);
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 7979392776..2cf326185e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7505,8 +7505,8 @@
proname => 'pg_relation_filenode', provolatile => 's', prorettype => 'oid',
proargtypes => 'regclass', prosrc => 'pg_relation_filenode' },
{ oid => '3454', descr => 'relation OID for filenode and tablespace',
- proname => 'pg_filenode_relation', provolatile => 's',
- prorettype => 'regclass', proargtypes => 'oid oid',
+ proname => 'pg_filenode_relation', proisstrict => 'f', provolatile => 'v',
+ prorettype => 'regclass', proargtypes => 'oid oid char',
prosrc => 'pg_filenode_relation' },
{ oid => '3034', descr => 'file path of relation',
proname => 'pg_relation_filepath', provolatile => 's', prorettype => 'text',
diff --git a/src/include/utils/relfilenumbermap.h b/src/include/utils/relfilenumbermap.h
index 0ca89e452a..5b98e02eaa 100644
--- a/src/include/utils/relfilenumbermap.h
+++ b/src/include/utils/relfilenumbermap.h
@@ -16,6 +16,7 @@
#include "common/relpath.h"
extern Oid RelidByRelfilenumber(Oid reltablespace,
- RelFileNumber relfilenumber);
+ RelFileNumber relfilenumber,
+ char persistence);
#endif /* RELFILENUMBERMAP_H */
[text/x-patch] RelidByRelfilenumber_prefer_perm_rels_15-16.patch (2.3K, ../../20240112.173746.1710988914918513917.horikyota.ntt@gmail.com/3-RelidByRelfilenumber_prefer_perm_rels_15-16.patch)
download | inline diff:
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index 220f33d43f..d188f53eda 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -130,7 +130,12 @@ InitializeRelfilenumberMap(void)
/*
* Map a relation's (tablespace, relfilenumber) to a relation's oid and cache
- * the result.
+ * the result. If both permanent and temporary relations are found, persistent
+ * one is chosen.
+ *
+ * XXXX: This behavior is valid for most use cases, except for the SQL function
+ * pg_lation_filenode(), which can handle temporary relations. In all other
+ * cases, this function is intended fo ruse with permanent relations only.
*
* Returns InvalidOid if no relation matching the criteria could be found.
*/
@@ -183,6 +188,8 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
}
else
{
+ bool is_temp = false;
+
/*
* Not a shared table, could either be a plain relation or a
* non-shared, nailed one, like e.g. pg_class.
@@ -212,19 +219,34 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
if (found)
- elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenumber %u",
- reltablespace, relfilenumber);
+ {
+ /* check if the same entry for the same persistency exists */
+ if (is_temp ?
+ classform->relpersistence == RELPERSISTENCE_TEMP :
+ classform->relpersistence != RELPERSISTENCE_TEMP)
+ elog(ERROR,
+ "unexpected duplicate for tablespace %u, relfilenumber %u",
+ reltablespace, relfilenumber);
+
+ /* prioritize non-temporary if both are found */
+ if (!is_temp)
+ continue;
+ }
found = true;
Assert(classform->reltablespace == reltablespace);
Assert(classform->relfilenode == relfilenumber);
relid = classform->oid;
+ is_temp = (classform->relpersistence == RELPERSISTENCE_TEMP);
}
systable_endscan(scandesc);
table_close(relation, AccessShareLock);
+ /* don't cache the relid if it is a temp relation */
+ if (is_temp && found)
+ return relid;
+
/* check for tables that are mapped but not shared */
if (!found)
relid = RelationMapFilenumberToOid(relfilenumber, false);
[text/x-patch] RelidByRelfilenumber_prefer_perm_rels_12-14.patch (2.1K, ../../20240112.173746.1710988914918513917.horikyota.ntt@gmail.com/4-RelidByRelfilenumber_prefer_perm_rels_12-14.patch)
download | inline diff:
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index 56d7c73d33..e9ae548a4b 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -130,7 +130,12 @@ InitializeRelfilenodeMap(void)
/*
* Map a relation's (tablespace, filenode) to a relation's oid and cache the
- * result.
+ * result. If both permanent and temporary relations are found, persistent one
+ * is chosen.
+ *
+ * XXXX: This behavior is valid for most use cases, except for the SQL function
+ * pg_lation_filenode(), which can handle temporary relations. In all other
+ * cases, this function is intended fo ruse with permanent relations only.
*
* Returns InvalidOid if no relation matching the criteria could be found.
*/
@@ -183,6 +188,8 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
}
else
{
+ bool is_temp = false;
+
/*
* Not a shared table, could either be a plain relation or a
* non-shared, nailed one, like e.g. pg_class.
@@ -212,9 +219,19 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
if (found)
- elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenode %u",
- reltablespace, relfilenode);
+ {
+ /* check if the same entry for the same persistency exists */
+ if (is_temp ?
+ classform->relpersistence == RELPERSISTENCE_TEMP :
+ classform->relpersistence != RELPERSISTENCE_TEMP)
+ elog(ERROR,
+ "unexpected duplicate for tablespace %u, relfilenode %u",
+ reltablespace, relfilenode);
+
+ /* prioritize non-temporary if both are found */
+ if (!is_temp)
+ continue;
+ }
found = true;
Assert(classform->reltablespace == reltablespace);
@@ -225,6 +242,10 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
systable_endscan(scandesc);
table_close(relation, AccessShareLock);
+ /* don't cache the relid if it is a temp relation */
+ if (is_temp && found)
+ return relid;
+
/* check for tables that are mapped but not shared */
if (!found)
relid = RelationMapFilenodeToOid(relfilenode, false);
[text/x-patch] RelidByRelfilenumber_prefer_perm_rels_11.patch (2.2K, ../../20240112.173746.1710988914918513917.horikyota.ntt@gmail.com/5-RelidByRelfilenumber_prefer_perm_rels_11.patch)
download | inline diff:
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index db6bb1c1fd..df3e11943c 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -133,7 +133,12 @@ InitializeRelfilenodeMap(void)
/*
* Map a relation's (tablespace, filenode) to a relation's oid and cache the
- * result.
+ * result. If both permanent and temporary relations are found, persistent one
+ * is chosen.
+ *
+ * XXXX: This behavior is valid for most use cases, except for the SQL function
+ * pg_lation_filenode(), which can handle temporary relations. In all other
+ * cases, this function is intended fo ruse with permanent relations only.
*
* Returns InvalidOid if no relation matching the criteria could be found.
*/
@@ -186,6 +191,8 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
}
else
{
+ bool is_temp = false;
+
/*
* Not a shared table, could either be a plain relation or a
* non-shared, nailed one, like e.g. pg_class.
@@ -213,9 +220,21 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
while (HeapTupleIsValid(ntp = systable_getnext(scandesc)))
{
if (found)
- elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenode %u",
- reltablespace, relfilenode);
+ {
+ Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
+
+ /* check if the same entry for the same persistency exists */
+ if (is_temp ?
+ classform->relpersistence == RELPERSISTENCE_TEMP :
+ classform->relpersistence != RELPERSISTENCE_TEMP)
+ elog(ERROR,
+ "unexpected duplicate for tablespace %u, relfilenode %u",
+ reltablespace, relfilenode);
+
+ /* prioritize non-temporary if both are found */
+ if (!is_temp)
+ continue;
+ }
found = true;
#ifdef USE_ASSERT_CHECKING
@@ -240,6 +259,10 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
systable_endscan(scandesc);
heap_close(relation, AccessShareLock);
+ /* don't cache the relid if it is a temp relation */
+ if (is_temp && found)
+ return relid;
+
/* check for tables that are mapped but not shared */
if (!found)
relid = RelationMapFilenodeToOid(relfilenode, false);
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-01-15 21:32 Robert Haas <robertmhaas@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Robert Haas @ 2024-01-15 21:32 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: vignesh21@gmail.com; sawada.mshk@gmail.com; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
On Fri, Jan 12, 2024 at 3:38 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> Maybe. It might be better for the cache not to register temprary
> relations at all.
This point seems worthy of serious consideration to me. Is there any
reason why we need RelidByRelfilenumber() to work with temporary
relations at all? I understand that the current behavior is exposed
via the SQL-callable function, but maybe that's not really
intentional. If there's no other use of RelidByRelfilenumber() that
needs to care about permanent relations intrinsically, I think we
shouldn't hesitate to just cut them out of the mechanism entirely.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-01-16 01:58 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Kyotaro Horiguchi @ 2024-01-16 01:58 UTC (permalink / raw)
To: robertmhaas@gmail.com; +Cc: vignesh21@gmail.com; sawada.mshk@gmail.com; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
At Mon, 15 Jan 2024 16:32:07 -0500, Robert Haas <robertmhaas@gmail.com> wrote in
> On Fri, Jan 12, 2024 at 3:38 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > Maybe. It might be better for the cache not to register temprary
> > relations at all.
>
> This point seems worthy of serious consideration to me. Is there any
> reason why we need RelidByRelfilenumber() to work with temporary
> relations at all? I understand that the current behavior is exposed
> via the SQL-callable function, but maybe that's not really
> intentional. If there's no other use of RelidByRelfilenumber() that
> needs to care about permanent relations intrinsically, I think we
> shouldn't hesitate to just cut them out of the mechanism entirely.
Do you mean that the current behavior of the SQL-callable function is
being treated as a bug and should it be corrected?
Simply doing so will result in the functions pg_relation_filenode()
and pg_filenode_relation() behaving asymmetrically. While there is no
need to purposely change the behavior of the former, it is necessary
to document the behavior of the latter regardless.
The attached patch does the above for the master head. If we apply
this approach to older versions, I can adapt and create similar
patches for them.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] v2_RelidByRelfilenumber_prefer_perm_rels_master.patch (3.0K, ../../20240116.105804.1924548897701268962.horikyota.ntt@gmail.com/2-v2_RelidByRelfilenumber_prefer_perm_rels_master.patch)
download | inline diff:
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 210c7c0b02..ee0b81ca74 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28321,16 +28321,20 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
<indexterm>
<primary>pg_filenode_relation</primary>
</indexterm>
- <function>pg_filenode_relation</function> ( <parameter>tablespace</parameter> <type>oid</type>, <parameter>filenode</parameter> <type>oid</type> )
+ <function>pg_filenode_relation</function>
+ ( <parameter>tablespace</parameter> <type>oid</type>,
+ <parameter>filenode</parameter> <type>oid</type>)
<returnvalue>regclass</returnvalue>
</para>
<para>
Returns a relation's OID given the tablespace OID and filenode it is
- stored under. This is essentially the inverse mapping of
- <function>pg_relation_filepath</function>. For a relation in the
- database's default tablespace, the tablespace can be specified as zero.
- Returns <literal>NULL</literal> if no relation in the current database
- is associated with the given values.
+ stored under, if the given values match a permanent relation. This is
+ essentially the inverse mapping
+ of <function>pg_relation_filepath</function> only regarding permanent
+ relations. For a relation in the database's default tablespace, the
+ tablespace can be specified as zero. Returns <literal>NULL</literal>
+ if no permanent relation in the current database is associated with
+ the given values.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index b7caa84341..87b903360e 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -129,10 +129,11 @@ InitializeRelfilenumberMap(void)
}
/*
- * Map a relation's (tablespace, relfilenumber) to a relation's oid and cache
- * the result.
+ * Map a relation's (tablespace, relfilenumber) to a permanent relation's oid
+ * and cache the result.
*
- * Returns InvalidOid if no relation matching the criteria could be found.
+ * Returns InvalidOid if no permanent relation matching the criteria could be
+ * found.
*/
Oid
RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
@@ -211,6 +212,16 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
{
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
+ /*
+ * Temporary relations should be excluded. This exclusion is
+ * necessary because they can lead to the wrong result; the
+ * relfilenumber space is shared with persistent
+ * relations. Additionally, excluding them is possible since they
+ * are not the focus in this context.
+ */
+ if (classform->relpersistence == RELPERSISTENCE_TEMP)
+ continue;
+
if (found)
elog(ERROR,
"unexpected duplicate for tablespace %u, relfilenumber %u",
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-02-02 15:49 Robert Haas <robertmhaas@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Robert Haas @ 2024-02-02 15:49 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: vignesh21@gmail.com, sawada.mshk@gmail.com, wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Mon, Jan 15, 2024 at 8:58 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> Do you mean that the current behavior of the SQL-callable function is
> being treated as a bug and should it be corrected?
>
> Simply doing so will result in the functions pg_relation_filenode()
> and pg_filenode_relation() behaving asymmetrically. While there is no
> need to purposely change the behavior of the former, it is necessary
> to document the behavior of the latter regardless.
>
> The attached patch does the above for the master head. If we apply
> this approach to older versions, I can adapt and create similar
> patches for them.
Yes, this patch shows the approach I was asking about.
Andres, what do you think about this idea? I wonder if you just
momentarily forgot about temporary relations when coding
RelidByRelfilenumber -- because for that function to give well-defined
answers with temporary relations included, it would need the backend
ID as an additional argument.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-03-18 06:35 Michael Paquier <michael@paquier.xyz>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Michael Paquier @ 2024-03-18 06:35 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; vignesh21@gmail.com, sawada.mshk@gmail.com, wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Fri, Feb 02, 2024 at 10:49:17AM -0500, Robert Haas wrote:
> Andres, what do you think about this idea? I wonder if you just
> momentarily forgot about temporary relations when coding
> RelidByRelfilenumber -- because for that function to give well-defined
> answers with temporary relations included, it would need the backend
> ID as an additional argument.
No idea what Andres thinks, but seeing that pg_filenode_relation()
uses in input a tablespace OID and a filenode OID while ignoring the
prefix that would be used for a temp relation path (with a 't' and the
backend number), it is clear that the current function is not suited
to make the difference between temporary and persistent relations as
we'd need to have a priority order to choose one over the other. And
that may not lead to the correct choice.
Ignoring temporary relations entirely makes sense: one cannot get a
regclass from only a tablespace and a relfilenode, the persistence, as
well as a backend ID would also be required. I've not checked the
patch in details, but it's to say that the idea to cut temporary
relations sounds rather right here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Zffgupps4Gv9BVXL@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2024-07-24 21:51 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 25+ messages in thread
From: Masahiko Sawada @ 2024-07-24 21:51 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; vignesh21@gmail.com, wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Sun, Mar 17, 2024 at 11:35 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Feb 02, 2024 at 10:49:17AM -0500, Robert Haas wrote:
> > Andres, what do you think about this idea? I wonder if you just
> > momentarily forgot about temporary relations when coding
> > RelidByRelfilenumber -- because for that function to give well-defined
> > answers with temporary relations included, it would need the backend
> > ID as an additional argument.
>
>
> Ignoring temporary relations entirely makes sense: one cannot get a
> regclass from only a tablespace and a relfilenode, the persistence, as
> well as a backend ID would also be required. I've not checked the
> patch in details, but it's to say that the idea to cut temporary
> relations sounds rather right here.
That makes sense to me too.
Regarding the patch, filtering by the relpersistence in
systable_getnext() loop seems to be good to me. Alternatively we can
add "relpersistence == RELPERSISTENCE_TEMP" to the scan key. The patch
would need regression tests too.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-03-25 10:01 vignesh C <vignesh21@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: vignesh C @ 2025-03-25 10:01 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Thu, 25 Jul 2024 at 03:21, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sun, Mar 17, 2024 at 11:35 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Fri, Feb 02, 2024 at 10:49:17AM -0500, Robert Haas wrote:
> > > Andres, what do you think about this idea? I wonder if you just
> > > momentarily forgot about temporary relations when coding
> > > RelidByRelfilenumber -- because for that function to give well-defined
> > > answers with temporary relations included, it would need the backend
> > > ID as an additional argument.
> >
> >
> > Ignoring temporary relations entirely makes sense: one cannot get a
> > regclass from only a tablespace and a relfilenode, the persistence, as
> > well as a backend ID would also be required. I've not checked the
> > patch in details, but it's to say that the idea to cut temporary
> > relations sounds rather right here.
>
> That makes sense to me too.
>
> Regarding the patch, filtering by the relpersistence in
> systable_getnext() loop seems to be good to me. Alternatively we can
> add "relpersistence == RELPERSISTENCE_TEMP" to the scan key. The patch
> would need regression tests too.
The attached patch adds a new test and resolves an existing test
failure. However, a downside is that we can no longer verify the
mapping of the temporary tables.
Regards,
Vignesh
Attachments:
[text/x-patch] v3_RelidByRelfilenumber_prefer_perm_rels_master.patch (6.6K, ../../CALDaNm2b3XtaA6GQCD55z-eOyxXR2s_oBifesb7FgpKLJNYR0A@mail.gmail.com/2-v3_RelidByRelfilenumber_prefer_perm_rels_master.patch)
download | inline diff:
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6fa1d6586b8..4f6edf308b5 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -30198,16 +30198,20 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
<indexterm>
<primary>pg_filenode_relation</primary>
</indexterm>
- <function>pg_filenode_relation</function> ( <parameter>tablespace</parameter> <type>oid</type>, <parameter>filenode</parameter> <type>oid</type> )
+ <function>pg_filenode_relation</function>
+ ( <parameter>tablespace</parameter> <type>oid</type>,
+ <parameter>filenode</parameter> <type>oid</type>)
<returnvalue>regclass</returnvalue>
</para>
<para>
Returns a relation's OID given the tablespace OID and filenode it is
- stored under. This is essentially the inverse mapping of
- <function>pg_relation_filepath</function>. For a relation in the
- database's default tablespace, the tablespace can be specified as zero.
- Returns <literal>NULL</literal> if no relation in the current database
- is associated with the given values.
+ stored under, if the given values match a permanent relation. This is
+ essentially the inverse mapping
+ of <function>pg_relation_filepath</function> only regarding permanent
+ relations. For a relation in the database's default tablespace, the
+ tablespace can be specified as zero. Returns <literal>NULL</literal>
+ if no permanent relation in the current database is associated with
+ the given values.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index 8a2f6f8c693..6ffddfe0c5e 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -127,10 +127,11 @@ InitializeRelfilenumberMap(void)
}
/*
- * Map a relation's (tablespace, relfilenumber) to a relation's oid and cache
- * the result.
+ * Map a relation's (tablespace, relfilenumber) to a permanent relation's oid
+ * and cache the result.
*
- * Returns InvalidOid if no relation matching the criteria could be found.
+ * Returns InvalidOid if no permanent relation matching the criteria could be
+ * found.
*/
Oid
RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
@@ -208,6 +209,16 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
{
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
+ /*
+ * Temporary relations should be excluded. This exclusion is
+ * necessary because they can lead to the wrong result; the
+ * relfilenumber space is shared with persistent
+ * relations. Additionally, excluding them is possible since they
+ * are not the focus in this context.
+ */
+ if (classform->relpersistence == RELPERSISTENCE_TEMP)
+ continue;
+
if (found)
elog(ERROR,
"unexpected duplicate for tablespace %u, relfilenumber %u",
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 362f38856d2..9c97d38bb1b 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3501,9 +3501,9 @@ SELECT
oid, mapped_oid, reltablespace, relfilenode, relname
FROM pg_class,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) AS mapped_oid
-WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
+WHERE relkind IN ('r', 'i', 'S', 'm') AND mapped_oid IS DISTINCT FROM oid;
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
-WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
+WHERE (c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL) AND c.relpersistence != 't';
oid | mapped_oid | reltablespace | relfilenode | relname
-----+------------+---------------+-------------+---------
(0 rows)
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 76604705a93..b6e5083ee6b 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -102,6 +102,16 @@ ERROR: tables declared WITH OIDS are not supported
-- but explicitly not adding oids is still supported
CREATE TEMP TABLE withoutoid() WITHOUT OIDS; DROP TABLE withoutoid;
CREATE TEMP TABLE withoutoid() WITH (oids = false); DROP TABLE withoutoid;
+-- verify that temporary tables data are not retrieved by pg_filenode_relation
+CREATE TEMP TABLE filenoderelationcheck(c1 int);
+SELECT pg_filenode_relation (reltablespace, pg_relation_filenode(oid)) FROM pg_class
+WHERE relname = 'filenoderelationcheck';
+ pg_filenode_relation
+----------------------
+
+(1 row)
+
+DROP TABLE filenoderelationcheck;
-- check restriction with default expressions
-- invalid use of column reference in default expressions
CREATE TABLE default_expr_column (id int DEFAULT (id));
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 84e93ef575e..20d50a3a214 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2191,10 +2191,10 @@ SELECT
oid, mapped_oid, reltablespace, relfilenode, relname
FROM pg_class,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) AS mapped_oid
-WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
+WHERE relkind IN ('r', 'i', 'S', 'm') AND mapped_oid IS DISTINCT FROM oid;
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
-WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
+WHERE (c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL) AND c.relpersistence != 't';
-- Checks on creating and manipulation of user defined relations in
-- pg_catalog.
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 37a227148e9..44b0c14902f 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -68,6 +68,12 @@ CREATE TABLE withoid() WITH (oids = true);
CREATE TEMP TABLE withoutoid() WITHOUT OIDS; DROP TABLE withoutoid;
CREATE TEMP TABLE withoutoid() WITH (oids = false); DROP TABLE withoutoid;
+-- verify that temporary tables data are not retrieved by pg_filenode_relation
+CREATE TEMP TABLE filenoderelationcheck(c1 int);
+SELECT pg_filenode_relation (reltablespace, pg_relation_filenode(oid)) FROM pg_class
+WHERE relname = 'filenoderelationcheck';
+DROP TABLE filenoderelationcheck;
+
-- check restriction with default expressions
-- invalid use of column reference in default expressions
CREATE TABLE default_expr_column (id int DEFAULT (id));
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-06-19 08:47 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Ashutosh Bapat @ 2025-06-19 08:47 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Tue, Jun 17, 2025 at 6:29 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 25 Jul 2024 at 03:21, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Sun, Mar 17, 2024 at 11:35 PM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Fri, Feb 02, 2024 at 10:49:17AM -0500, Robert Haas wrote:
> > > > Andres, what do you think about this idea? I wonder if you just
> > > > momentarily forgot about temporary relations when coding
> > > > RelidByRelfilenumber -- because for that function to give well-defined
> > > > answers with temporary relations included, it would need the backend
> > > > ID as an additional argument.
> > >
> > >
> > > Ignoring temporary relations entirely makes sense: one cannot get a
> > > regclass from only a tablespace and a relfilenode, the persistence, as
> > > well as a backend ID would also be required. I've not checked the
> > > patch in details, but it's to say that the idea to cut temporary
> > > relations sounds rather right here.
> >
> > That makes sense to me too.
> >
> > Regarding the patch, filtering by the relpersistence in
> > systable_getnext() loop seems to be good to me. Alternatively we can
> > add "relpersistence == RELPERSISTENCE_TEMP" to the scan key.
Do you mean relpersistence != RELPERSISTENCE_TEMP? I tried that. It's
not possible to add this check to the scankey since indexes do not
work with != conditions. We will need to use a sequential scan to do
so, but that will affect the performance. I think what Vignesh has
done in his patch is good enough.
>
> The attached patch adds a new test and resolves an existing test
> failure. However, a downside is that we can no longer verify the
> mapping of the temporary tables.
Yes, but I think the current way of verification wasn't correct anyway
because it couldn't match the temporary table's relation exactly. We
will need to devise another way to do that, maybe creating a version
of pg_filenode_relation() for temporary tables.
Some more comments, some of which I have applied in the attached
patches. Please review those. Let me know what you think.
I feel that we should mention permanent tables in the prologue of
pg_filenode_relation() for somebody who just looks at
pg_filenode_relation(). Also in its pg_proc.dat description for one
who looks at \df+ output. Attached patch does that.
- * Returns InvalidOid if no relation matching the criteria could be found.
+ * Returns InvalidOid if no permanent relation matching the criteria could be
+ * found.
The relpersistence enum has values
#define RELPERSISTENCE_PERMANENT 'p' /* regular table */
#define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent table */
#define RELPERSISTENCE_TEMP 't' /* temporary table */
The description of UNLOGGED mentions "permanent" so it looks like your
use of "permanent" covers both regular and unlogged tables. However
looking purely at the RELPERSISTENCE_ names, it' s possible that one
will associate the word "permanent" with RELPERSISTENCE_PERMANENT
only, and not with RELPERSISTENCE_UNLOGGED. Should we use
"non-temporary" instead to avoid confusion?
+ /*
+ * Temporary relations should be excluded. This exclusion is
+ * necessary because they can lead to the wrong result; the
+ * relfilenumber space is shared with persistent
+ * relations. Additionally, excluding them is possible since they
+ * are not the focus in this context.
+ */
+ if (classform->relpersistence == RELPERSISTENCE_TEMP)
+ continue;
+
I slightly rephrased the comment and moved it to the function prologue
since it defines the function's scope.
WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
+WHERE relkind IN ('r', 'i', 'S', 'm') AND mapped_oid IS DISTINCT FROM oid;
Why do we need to remove permanent toast tables from the query?
Instead adjustment below
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
-WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
+WHERE (c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL) AND
c.relpersistence != 't';
seems enough. Actually, we shouldn't pass temporary tables to
pg_filenode_relation(), since it doesn't map temporary relations now.
Adjusted that way in the attached patch.
While reviewing the patch, I found something else. The
RelidByRelfilenumber() enters negative cache entries.
RelfilenumberMapInvalidateCallback() treats the negative entries
specifically which indicates that it's intentional. But if somebody
calls pg_filenode_relation() repeatedly with invalid relfilenodes,
that would bloat the cache with "kinda useless entries". It's a way to
make a backend consume a lot of memory quickly and thus perform a DOS.
For example, on my laptop, I could make a backend consume almost 3GB
of memory in 7 minutes.
#SELECT pg_size_pretty(total_bytes) total_bytes,
pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
where name = 'RelfilenumberMap cache';
total_bytes | used_bytes
-------------+------------
(0 rows)
#\timing
Timing is on.
#select r is null, count(*) from (select pg_filenode_relation(i, j) r
from generate_series(1, 1000) i, generate_series(1, 1000) j) q group
by r is null;
?column? | count
----------+---------
t | 1000000
(1 row)
Time: 4705.483 ms (00:04.705)
#SELECT pg_size_pretty(total_bytes) total_bytes,
pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
where name = 'RelfilenumberMap cache';
total_bytes | used_bytes
-------------+------------
40 MB | 39 MB
(1 row)
Time: 2.351 ms
#select r is null, count(*) from (select pg_filenode_relation(i, j) r
from generate_series(1001, 10000) i, generate_series(1001, 10000) j) q
group by r is null;
?column? | count
----------+----------
f | 153215
t | 80846785
(2 rows)
Time: 421998.039 ms (07:01.998)
#SELECT pg_size_pretty(total_bytes) total_bytes,
pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
where name = 'RelfilenumberMap cache';
total_bytes | used_bytes
-------------+------------
3180 MB | 3176 MB
(1 row)
Time: 132.187 ms
Logical replication and autoprewarm may not cause such a large bloat
but there is no limit to passing invalid combinations of reltablespace
and relfilenumber to pg_filenode_relation(). Do we want to prohibit
that case by passing a flag from logical pg_filenode_relation() to not
cache invalid entries?
I have moved the CF entry to the July commitfest.
--
Best Wishes,
Ashutosh Bapat
Attachments:
[text/x-patch] 0001-Ignore-temporary-relations-in-RelidByRelfil-20250619.patch (11.8K, ../../CAExHW5vaHu=s6eopJ2jxyGNRG--uEsJwYk92yxa2L_80+Y=1YA@mail.gmail.com/2-0001-Ignore-temporary-relations-in-RelidByRelfil-20250619.patch)
download | inline diff:
From b9273ac448b46d30ce30cbbb19021d1126684885 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Wed, 18 Jun 2025 16:56:25 +0530
Subject: [PATCH 1/2] Ignore temporary relations in RelidByRelfilenumber()
Temporary relations may have the same relfilenumber as a permanent
relation, or other temporary relations associated with other sessions.
The function may get confused if the given relfilenumber and tablespace
is associated with one or more temporary tables and throw an error. This
will disturb the intended users of this function i.e. logical
replication and autoprewarm, which lookup only permanent relation. Hence
the function only considers only non-temporary relations. The third
caller of this function is pg_filenode_relation(). It throws an error in
such cases. We require proc number of the backend which created the
temporary table along with the tablespace and relfilenumber in order to
uniquely identify a temporary relation. Since any user visible backend
identifier is not an argument to this function, it is doubtful whether
the function was intended to be used to map temporary relations. In
short, considering temporary relations seems to be an oversight in
commit f01d1ae3a104019d6d68aeff85c4816a275130b3. Fix it.
Author: Vignesh C <vignesh21@gmail.com>
Reported-By: Shenhao Wang <wangsh.fnst@fujitsu.com>
Reviewed-By: Michael Paquier <michael@paquier.xyz>,
Reviewed-By: Robert Haas <robertmhaas@gmail.com>,
Reviewed-By: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
Reviewed-By: Takamichi Osumi <osumi.takamichi@fujitsu.com>
Reviewed-By: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/bbaaf9f9-ebb2-645f-54bb-34d6efc7ac42%40fujitsu.com
---
doc/src/sgml/func.sgml | 16 +++++++++-------
src/backend/utils/adt/dbsize.c | 5 +++--
src/backend/utils/cache/relfilenumbermap.c | 20 +++++++++++++++++---
src/include/catalog/pg_proc.dat | 2 +-
src/test/regress/expected/alter_table.out | 9 ++++++---
src/test/regress/expected/create_table.out | 10 ++++++++++
src/test/regress/sql/alter_table.sql | 10 ++++++----
src/test/regress/sql/create_table.sql | 6 ++++++
8 files changed, 58 insertions(+), 20 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8d7d9a2f3e8..3b5d6ebc946 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -30317,16 +30317,18 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
<indexterm>
<primary>pg_filenode_relation</primary>
</indexterm>
- <function>pg_filenode_relation</function> ( <parameter>tablespace</parameter> <type>oid</type>, <parameter>filenode</parameter> <type>oid</type> )
+ <function>pg_filenode_relation</function>
+ ( <parameter>tablespace</parameter> <type>oid</type>,
+ <parameter>filenode</parameter> <type>oid</type>)
<returnvalue>regclass</returnvalue>
</para>
<para>
- Returns a relation's OID given the tablespace OID and filenode it is
- stored under. This is essentially the inverse mapping of
- <function>pg_relation_filepath</function>. For a relation in the
- database's default tablespace, the tablespace can be specified as zero.
- Returns <literal>NULL</literal> if no relation in the current database
- is associated with the given values.
+ Returns a permanent relation's OID given the tablespace OID and filenode
+ it is stored under. This is essentially the inverse mapping of
+ <function>pg_relation_filepath</function> only for permanent relations.
+ For a relation in the database's default tablespace, the tablespace can
+ be specified as zero. Returns <literal>NULL</literal> if no permanent
+ relation in the current database is associated with the given values.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 25865b660ef..334ba6557ac 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -933,8 +933,9 @@ pg_relation_filenode(PG_FUNCTION_ARGS)
*
* This is expected to be used when somebody wants to match an individual file
* on the filesystem back to its table. That's not trivially possible via
- * pg_class, because that doesn't contain the relfilenumbers of shared and nailed
- * tables.
+ * pg_class, because that doesn't contain the relfilenumbers of shared and
+ * nailed tables. This function looks up only permanent relations; see
+ * RelidByRelfilenumber() for reasons.
*
* We don't fail but return NULL if we cannot find a mapping.
*
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index 8a2f6f8c693..14c30276e24 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -127,10 +127,21 @@ InitializeRelfilenumberMap(void)
}
/*
- * Map a relation's (tablespace, relfilenumber) to a relation's oid and cache
- * the result.
+ * Map a relation's (tablespace, relfilenumber) to a relation's oid and cache the
+ * result.
*
- * Returns InvalidOid if no relation matching the criteria could be found.
+ * A temporary relation may share its relfilenumber with a permanent relation,
+ * or other temporary relations. When looking up a permanent relation, this
+ * function may get confused by temporary relations which have same
+ * relfilenumber and throw an error. This behaviour may disturb logical
+ * replication and autoprewarm, both of which lookup permanent relations.
+ * Further in order to uniquely identify a temporary relation we also need the
+ * proc number of the backend which created it. The proc number is not available
+ * to this function. Given the intended use of this function, ignore temporary
+ * relations.
+ *
+ * Returns InvalidOid if no permanent relation matching the criteria could be
+ * found.
*/
Oid
RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
@@ -208,6 +219,9 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
{
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
+ if (classform->relpersistence == RELPERSISTENCE_TEMP)
+ continue;
+
if (found)
elog(ERROR,
"unexpected duplicate for tablespace %u, relfilenumber %u",
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index d3d28a263fa..4df360f76c4 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7775,7 +7775,7 @@
{ oid => '2999', descr => 'filenode identifier of relation',
proname => 'pg_relation_filenode', provolatile => 's', prorettype => 'oid',
proargtypes => 'regclass', prosrc => 'pg_relation_filenode' },
-{ oid => '3454', descr => 'relation OID for filenode and tablespace',
+{ oid => '3454', descr => 'permanent relation OID for filenode and tablespace',
proname => 'pg_filenode_relation', provolatile => 's',
prorettype => 'regclass', proargtypes => 'oid oid',
prosrc => 'pg_filenode_relation' },
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 476266e3f4b..33b3f14615a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3565,14 +3565,17 @@ SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment F
-- Check that we map relation oids to filenodes and back correctly. Only
-- display bad mappings so the test output doesn't change all the time. A
-- filenode function call can return NULL for a relation dropped concurrently
--- with the call's surrounding query, so ignore a NULL mapped_oid for
--- relations that no longer exist after all calls finish.
+-- with the call's surrounding query, so ignore a NULL mapped_oid for relations
+-- that no longer exist after all calls finish. The function maps only permanent
+-- relations, so ignore temporary relations.
CREATE TEMP TABLE filenode_mapping AS
SELECT
oid, mapped_oid, reltablespace, relfilenode, relname
FROM pg_class,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) AS mapped_oid
-WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
+WHERE relkind IN ('r', 'i', 'S', 't', 'm')
+ AND relpersistence != 't'
+ AND mapped_oid IS DISTINCT FROM oid;
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
oid | mapped_oid | reltablespace | relfilenode | relname
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 76604705a93..dcdd59175da 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -102,6 +102,16 @@ ERROR: tables declared WITH OIDS are not supported
-- but explicitly not adding oids is still supported
CREATE TEMP TABLE withoutoid() WITHOUT OIDS; DROP TABLE withoutoid;
CREATE TEMP TABLE withoutoid() WITH (oids = false); DROP TABLE withoutoid;
+-- verify that temporary tables data are not retrieved by pg_filenode_relation
+CREATE TEMP TABLE filenoderelationcheck(c1 int);
+SELECT pg_filenode_relation (reltablespace, pg_relation_filenode(oid)) FROM pg_class
+WHERE relname = 'filenoderelationcheck';
+ pg_filenode_relation
+----------------------
+
+(1 row)
+
+DROP TABLE filenoderelationcheck;
-- check restriction with default expressions
-- invalid use of column reference in default expressions
CREATE TABLE default_expr_column (id int DEFAULT (id));
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 5ce9d1e429f..4931b450c00 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2200,15 +2200,17 @@ SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment F
-- Check that we map relation oids to filenodes and back correctly. Only
-- display bad mappings so the test output doesn't change all the time. A
-- filenode function call can return NULL for a relation dropped concurrently
--- with the call's surrounding query, so ignore a NULL mapped_oid for
--- relations that no longer exist after all calls finish.
+-- with the call's surrounding query, so ignore a NULL mapped_oid for relations
+-- that no longer exist after all calls finish. The function maps only permanent
+-- relations, so ignore temporary relations.
CREATE TEMP TABLE filenode_mapping AS
SELECT
oid, mapped_oid, reltablespace, relfilenode, relname
FROM pg_class,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) AS mapped_oid
-WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
-
+WHERE relkind IN ('r', 'i', 'S', 't', 'm')
+ AND relpersistence != 't'
+ AND mapped_oid IS DISTINCT FROM oid;
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 37a227148e9..d5fe0f861a9 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -68,6 +68,12 @@ CREATE TABLE withoid() WITH (oids = true);
CREATE TEMP TABLE withoutoid() WITHOUT OIDS; DROP TABLE withoutoid;
CREATE TEMP TABLE withoutoid() WITH (oids = false); DROP TABLE withoutoid;
+-- verify that temporary tables data are not retrieved by pg_filenode_relation
+CREATE TEMP TABLE filenoderelationcheck(c1 int);
+SELECT pg_filenode_relation (reltablespace, pg_relation_filenode(oid)) FROM pg_class
+WHERE relname = 'filenoderelationcheck';
+DROP TABLE filenoderelationcheck;
+
-- check restriction with default expressions
-- invalid use of column reference in default expressions
CREATE TABLE default_expr_column (id int DEFAULT (id));
base-commit: 1546e17f9d067e714e066fcdd57d5f56c14f4174
--
2.34.1
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-08-13 09:48 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Ashutosh Bapat @ 2025-08-13 09:48 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Thu, Jun 19, 2025 at 2:17 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Tue, Jun 17, 2025 at 6:29 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, 25 Jul 2024 at 03:21, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Sun, Mar 17, 2024 at 11:35 PM Michael Paquier <michael@paquier.xyz> wrote:
> > > >
> > > > On Fri, Feb 02, 2024 at 10:49:17AM -0500, Robert Haas wrote:
> > > > > Andres, what do you think about this idea? I wonder if you just
> > > > > momentarily forgot about temporary relations when coding
> > > > > RelidByRelfilenumber -- because for that function to give well-defined
> > > > > answers with temporary relations included, it would need the backend
> > > > > ID as an additional argument.
> > > >
> > > >
> > > > Ignoring temporary relations entirely makes sense: one cannot get a
> > > > regclass from only a tablespace and a relfilenode, the persistence, as
> > > > well as a backend ID would also be required. I've not checked the
> > > > patch in details, but it's to say that the idea to cut temporary
> > > > relations sounds rather right here.
> > >
> > > That makes sense to me too.
> > >
> > > Regarding the patch, filtering by the relpersistence in
> > > systable_getnext() loop seems to be good to me. Alternatively we can
> > > add "relpersistence == RELPERSISTENCE_TEMP" to the scan key.
>
> Do you mean relpersistence != RELPERSISTENCE_TEMP? I tried that. It's
> not possible to add this check to the scankey since indexes do not
> work with != conditions. We will need to use a sequential scan to do
> so, but that will affect the performance. I think what Vignesh has
> done in his patch is good enough.
>
> >
> > The attached patch adds a new test and resolves an existing test
> > failure. However, a downside is that we can no longer verify the
> > mapping of the temporary tables.
>
> Yes, but I think the current way of verification wasn't correct anyway
> because it couldn't match the temporary table's relation exactly. We
> will need to devise another way to do that, maybe creating a version
> of pg_filenode_relation() for temporary tables.
>
> Some more comments, some of which I have applied in the attached
> patches. Please review those. Let me know what you think.
>
> I feel that we should mention permanent tables in the prologue of
> pg_filenode_relation() for somebody who just looks at
> pg_filenode_relation(). Also in its pg_proc.dat description for one
> who looks at \df+ output. Attached patch does that.
>
> - * Returns InvalidOid if no relation matching the criteria could be found.
> + * Returns InvalidOid if no permanent relation matching the criteria could be
> + * found.
>
> The relpersistence enum has values
> #define RELPERSISTENCE_PERMANENT 'p' /* regular table */
> #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent table */
> #define RELPERSISTENCE_TEMP 't' /* temporary table */
>
> The description of UNLOGGED mentions "permanent" so it looks like your
> use of "permanent" covers both regular and unlogged tables. However
> looking purely at the RELPERSISTENCE_ names, it' s possible that one
> will associate the word "permanent" with RELPERSISTENCE_PERMANENT
> only, and not with RELPERSISTENCE_UNLOGGED. Should we use
> "non-temporary" instead to avoid confusion?
>
> + /*
> + * Temporary relations should be excluded. This exclusion is
> + * necessary because they can lead to the wrong result; the
> + * relfilenumber space is shared with persistent
> + * relations. Additionally, excluding them is possible since they
> + * are not the focus in this context.
> + */
> + if (classform->relpersistence == RELPERSISTENCE_TEMP)
> + continue;
> +
>
> I slightly rephrased the comment and moved it to the function prologue
> since it defines the function's scope.
>
> WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
> +WHERE relkind IN ('r', 'i', 'S', 'm') AND mapped_oid IS DISTINCT FROM oid;
>
> Why do we need to remove permanent toast tables from the query?
> Instead adjustment below
>
> SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
> -WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
> +WHERE (c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL) AND
> c.relpersistence != 't';
>
> seems enough. Actually, we shouldn't pass temporary tables to
> pg_filenode_relation(), since it doesn't map temporary relations now.
> Adjusted that way in the attached patch.
>
> While reviewing the patch, I found something else. The
> RelidByRelfilenumber() enters negative cache entries.
> RelfilenumberMapInvalidateCallback() treats the negative entries
> specifically which indicates that it's intentional. But if somebody
> calls pg_filenode_relation() repeatedly with invalid relfilenodes,
> that would bloat the cache with "kinda useless entries". It's a way to
> make a backend consume a lot of memory quickly and thus perform a DOS.
> For example, on my laptop, I could make a backend consume almost 3GB
> of memory in 7 minutes.
>
> #SELECT pg_size_pretty(total_bytes) total_bytes,
> pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
> where name = 'RelfilenumberMap cache';
> total_bytes | used_bytes
> -------------+------------
> (0 rows)
>
> #\timing
> Timing is on.
> #select r is null, count(*) from (select pg_filenode_relation(i, j) r
> from generate_series(1, 1000) i, generate_series(1, 1000) j) q group
> by r is null;
> ?column? | count
> ----------+---------
> t | 1000000
> (1 row)
>
> Time: 4705.483 ms (00:04.705)
> #SELECT pg_size_pretty(total_bytes) total_bytes,
> pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
> where name = 'RelfilenumberMap cache';
> total_bytes | used_bytes
> -------------+------------
> 40 MB | 39 MB
> (1 row)
>
> Time: 2.351 ms
> #select r is null, count(*) from (select pg_filenode_relation(i, j) r
> from generate_series(1001, 10000) i, generate_series(1001, 10000) j) q
> group by r is null;
> ?column? | count
> ----------+----------
> f | 153215
> t | 80846785
> (2 rows)
>
> Time: 421998.039 ms (07:01.998)
> #SELECT pg_size_pretty(total_bytes) total_bytes,
> pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
> where name = 'RelfilenumberMap cache';
> total_bytes | used_bytes
> -------------+------------
> 3180 MB | 3176 MB
> (1 row)
>
> Time: 132.187 ms
>
> Logical replication and autoprewarm may not cause such a large bloat
> but there is no limit to passing invalid combinations of reltablespace
> and relfilenumber to pg_filenode_relation(). Do we want to prohibit
> that case by passing a flag from logical pg_filenode_relation() to not
> cache invalid entries?
>
> I have moved the CF entry to the July commitfest.
The patch needed a rebase because of func.sgml refactoring. Here's a
rebased patchset.
--
Best Wishes,
Ashutosh Bapat
Attachments:
[text/x-patch] 0002-Avoid-bloating-RelfilenumberMap-cache-by-ne-20250813.patch (2.8K, ../../CAExHW5vBePcL9iEFasECB-wmYksZ9nUVnjnzOqkHYvtbCn==0A@mail.gmail.com/2-0002-Avoid-bloating-RelfilenumberMap-cache-by-ne-20250813.patch)
download | inline diff:
From 99bf6eceafcde4c0498798d69fc0aae3eab7d297 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Thu, 19 Jun 2025 10:39:35 +0530
Subject: [PATCH 2/2] Avoid bloating RelfilenumberMap cache by negative entries
RelidByRelfilenumber() has three users, logical replication, autoprewarm and
pg_filenode_relation(). The first two seldom cause negative entries in the cache
since they lookup a valid relation using valid tablespace and relfilenode.
Presence of negative entries doesn't help them.
But pg_filenode_relation(), which is SQL callable, may be invoked with
invalid tablespace and relfilenode pair causing negative entries in the
cache. These entries consume memory which is freed only when an
invalidation message is received. This can lead to bloating of the
cache. The function can be used for a denial of service attack since any
user can execute it.
This commit avoids bloating the cache by not adding negative entries.
Reported by: Ashutosh Bapat
Author: Ashutosh Bapat
---
src/backend/utils/cache/relfilenumbermap.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index 14c30276e24..f12f79694ad 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -60,13 +60,15 @@ RelfilenumberMapInvalidateCallback(Datum arg, Oid relid)
hash_seq_init(&status, RelfilenumberMapHash);
while ((entry = (RelfilenumberMapEntry *) hash_seq_search(&status)) != NULL)
{
+ /* Negative cache entries are not expected. */
+ Assert(OidIsValid(entry->relid));
+
/*
* If relid is InvalidOid, signaling a complete reset, we must remove
* all entries, otherwise just remove the specific relation's entry.
* Always remove negative cache entries.
*/
if (relid == InvalidOid || /* complete reset */
- entry->relid == InvalidOid || /* negative cache entry */
entry->relid == relid) /* individual flushed relation */
{
if (hash_search(RelfilenumberMapHash,
@@ -175,7 +177,11 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
entry = hash_search(RelfilenumberMapHash, &key, HASH_FIND, &found);
if (found)
+ {
+ /* Negative cache entries are not expected. */
+ Assert(OidIsValid(entry->relid));
return entry->relid;
+ }
/* ok, no previous cache entry, do it the hard way */
@@ -241,6 +247,10 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
relid = RelationMapFilenumberToOid(relfilenumber, false);
}
+ /* Avoid bloating cache with negative entries. */
+ if (!OidIsValid(relid))
+ return relid;
+
/*
* Only enter entry into cache now, our opening of pg_class could have
* caused cache invalidations to be executed which would have deleted a
--
2.34.1
[text/x-patch] 0001-Ignore-temporary-relations-in-RelidByRelfil-20250813.patch (11.4K, ../../CAExHW5vBePcL9iEFasECB-wmYksZ9nUVnjnzOqkHYvtbCn==0A@mail.gmail.com/3-0001-Ignore-temporary-relations-in-RelidByRelfil-20250813.patch)
download | inline diff:
From 74f52e4a3ecb58a6fb2f351d978576a67535c384 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Wed, 18 Jun 2025 16:56:25 +0530
Subject: [PATCH 1/2] Ignore temporary relations in RelidByRelfilenumber()
Temporary relations may have the same relfilenumber as a permanent
relation, or other temporary relations associated with other sessions.
The function may get confused if the given relfilenumber and tablespace
is associated with one or more temporary tables and throw an error. This
will disturb the intended users of this function i.e. logical
replication and autoprewarm, which lookup only permanent relation. Hence
the function only considers only non-temporary relations. The third
caller of this function is pg_filenode_relation(). It throws an error in
such cases. We require proc number of the backend which created the
temporary table along with the tablespace and relfilenumber in order to
uniquely identify a temporary relation. Since any user visible backend
identifier is not an argument to this function, it is doubtful whether
the function was intended to be used to map temporary relations. In
short, considering temporary relations seems to be an oversight in
commit f01d1ae3a104019d6d68aeff85c4816a275130b3. Fix it.
Author: Vignesh C <vignesh21@gmail.com>
Reported-By: Shenhao Wang <wangsh.fnst@fujitsu.com>
Reviewed-By: Michael Paquier <michael@paquier.xyz>,
Reviewed-By: Robert Haas <robertmhaas@gmail.com>,
Reviewed-By: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
Reviewed-By: Takamichi Osumi <osumi.takamichi@fujitsu.com>
Reviewed-By: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/bbaaf9f9-ebb2-645f-54bb-34d6efc7ac42%40fujitsu.com
---
doc/src/sgml/func/func-admin.sgml | 12 ++++++------
src/backend/utils/adt/dbsize.c | 5 +++--
src/backend/utils/cache/relfilenumbermap.c | 20 +++++++++++++++++---
src/include/catalog/pg_proc.dat | 2 +-
src/test/regress/expected/alter_table.out | 9 ++++++---
src/test/regress/expected/create_table.out | 10 ++++++++++
src/test/regress/sql/alter_table.sql | 10 ++++++----
src/test/regress/sql/create_table.sql | 6 ++++++
8 files changed, 55 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/func/func-admin.sgml b/doc/src/sgml/func/func-admin.sgml
index 446fdfe56f4..b59a3a9c52b 100644
--- a/doc/src/sgml/func/func-admin.sgml
+++ b/doc/src/sgml/func/func-admin.sgml
@@ -1829,12 +1829,12 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
<returnvalue>regclass</returnvalue>
</para>
<para>
- Returns a relation's OID given the tablespace OID and filenode it is
- stored under. This is essentially the inverse mapping of
- <function>pg_relation_filepath</function>. For a relation in the
- database's default tablespace, the tablespace can be specified as zero.
- Returns <literal>NULL</literal> if no relation in the current database
- is associated with the given values.
+ Returns a permanent relation's OID given the tablespace OID and filenode
+ it is stored under. This is essentially the inverse mapping of
+ <function>pg_relation_filepath</function> only for permanent relations.
+ For a relation in the database's default tablespace, the tablespace can
+ be specified as zero. Returns <literal>NULL</literal> if no permanent
+ relation in the current database is associated with the given values.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 25865b660ef..334ba6557ac 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -933,8 +933,9 @@ pg_relation_filenode(PG_FUNCTION_ARGS)
*
* This is expected to be used when somebody wants to match an individual file
* on the filesystem back to its table. That's not trivially possible via
- * pg_class, because that doesn't contain the relfilenumbers of shared and nailed
- * tables.
+ * pg_class, because that doesn't contain the relfilenumbers of shared and
+ * nailed tables. This function looks up only permanent relations; see
+ * RelidByRelfilenumber() for reasons.
*
* We don't fail but return NULL if we cannot find a mapping.
*
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index 8a2f6f8c693..14c30276e24 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -127,10 +127,21 @@ InitializeRelfilenumberMap(void)
}
/*
- * Map a relation's (tablespace, relfilenumber) to a relation's oid and cache
- * the result.
+ * Map a relation's (tablespace, relfilenumber) to a relation's oid and cache the
+ * result.
*
- * Returns InvalidOid if no relation matching the criteria could be found.
+ * A temporary relation may share its relfilenumber with a permanent relation,
+ * or other temporary relations. When looking up a permanent relation, this
+ * function may get confused by temporary relations which have same
+ * relfilenumber and throw an error. This behaviour may disturb logical
+ * replication and autoprewarm, both of which lookup permanent relations.
+ * Further in order to uniquely identify a temporary relation we also need the
+ * proc number of the backend which created it. The proc number is not available
+ * to this function. Given the intended use of this function, ignore temporary
+ * relations.
+ *
+ * Returns InvalidOid if no permanent relation matching the criteria could be
+ * found.
*/
Oid
RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
@@ -208,6 +219,9 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
{
Form_pg_class classform = (Form_pg_class) GETSTRUCT(ntp);
+ if (classform->relpersistence == RELPERSISTENCE_TEMP)
+ continue;
+
if (found)
elog(ERROR,
"unexpected duplicate for tablespace %u, relfilenumber %u",
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 118d6da1ace..ab1405aa0a5 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7787,7 +7787,7 @@
{ oid => '2999', descr => 'filenode identifier of relation',
proname => 'pg_relation_filenode', provolatile => 's', prorettype => 'oid',
proargtypes => 'regclass', prosrc => 'pg_relation_filenode' },
-{ oid => '3454', descr => 'relation OID for filenode and tablespace',
+{ oid => '3454', descr => 'permanent relation OID for filenode and tablespace',
proname => 'pg_filenode_relation', provolatile => 's',
prorettype => 'regclass', proargtypes => 'oid oid',
prosrc => 'pg_filenode_relation' },
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 08984dd98f1..d6d7cae73c9 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3565,14 +3565,17 @@ SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment F
-- Check that we map relation oids to filenodes and back correctly. Only
-- display bad mappings so the test output doesn't change all the time. A
-- filenode function call can return NULL for a relation dropped concurrently
--- with the call's surrounding query, so ignore a NULL mapped_oid for
--- relations that no longer exist after all calls finish.
+-- with the call's surrounding query, so ignore a NULL mapped_oid for relations
+-- that no longer exist after all calls finish. The function maps only permanent
+-- relations, so ignore temporary relations.
CREATE TEMP TABLE filenode_mapping AS
SELECT
oid, mapped_oid, reltablespace, relfilenode, relname
FROM pg_class,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) AS mapped_oid
-WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
+WHERE relkind IN ('r', 'i', 'S', 't', 'm')
+ AND relpersistence != 't'
+ AND mapped_oid IS DISTINCT FROM oid;
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
oid | mapped_oid | reltablespace | relfilenode | relname
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 76604705a93..dcdd59175da 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -102,6 +102,16 @@ ERROR: tables declared WITH OIDS are not supported
-- but explicitly not adding oids is still supported
CREATE TEMP TABLE withoutoid() WITHOUT OIDS; DROP TABLE withoutoid;
CREATE TEMP TABLE withoutoid() WITH (oids = false); DROP TABLE withoutoid;
+-- verify that temporary tables data are not retrieved by pg_filenode_relation
+CREATE TEMP TABLE filenoderelationcheck(c1 int);
+SELECT pg_filenode_relation (reltablespace, pg_relation_filenode(oid)) FROM pg_class
+WHERE relname = 'filenoderelationcheck';
+ pg_filenode_relation
+----------------------
+
+(1 row)
+
+DROP TABLE filenoderelationcheck;
-- check restriction with default expressions
-- invalid use of column reference in default expressions
CREATE TABLE default_expr_column (id int DEFAULT (id));
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index fc6e36d0e78..22b21a51101 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2200,15 +2200,17 @@ SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment F
-- Check that we map relation oids to filenodes and back correctly. Only
-- display bad mappings so the test output doesn't change all the time. A
-- filenode function call can return NULL for a relation dropped concurrently
--- with the call's surrounding query, so ignore a NULL mapped_oid for
--- relations that no longer exist after all calls finish.
+-- with the call's surrounding query, so ignore a NULL mapped_oid for relations
+-- that no longer exist after all calls finish. The function maps only permanent
+-- relations, so ignore temporary relations.
CREATE TEMP TABLE filenode_mapping AS
SELECT
oid, mapped_oid, reltablespace, relfilenode, relname
FROM pg_class,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) AS mapped_oid
-WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
-
+WHERE relkind IN ('r', 'i', 'S', 't', 'm')
+ AND relpersistence != 't'
+ AND mapped_oid IS DISTINCT FROM oid;
SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 37a227148e9..d5fe0f861a9 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -68,6 +68,12 @@ CREATE TABLE withoid() WITH (oids = true);
CREATE TEMP TABLE withoutoid() WITHOUT OIDS; DROP TABLE withoutoid;
CREATE TEMP TABLE withoutoid() WITH (oids = false); DROP TABLE withoutoid;
+-- verify that temporary tables data are not retrieved by pg_filenode_relation
+CREATE TEMP TABLE filenoderelationcheck(c1 int);
+SELECT pg_filenode_relation (reltablespace, pg_relation_filenode(oid)) FROM pg_class
+WHERE relname = 'filenoderelationcheck';
+DROP TABLE filenoderelationcheck;
+
-- check restriction with default expressions
-- invalid use of column reference in default expressions
CREATE TABLE default_expr_column (id int DEFAULT (id));
base-commit: 783cbb6d5e8bdf87d321286f210983c177ead967
--
2.34.1
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-08-13 09:59 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Ashutosh Bapat @ 2025-08-13 09:59 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Wed, Aug 13, 2025 at 3:18 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Jun 19, 2025 at 2:17 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Tue, Jun 17, 2025 at 6:29 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, 25 Jul 2024 at 03:21, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Sun, Mar 17, 2024 at 11:35 PM Michael Paquier <michael@paquier.xyz> wrote:
> > > > >
> > > > > On Fri, Feb 02, 2024 at 10:49:17AM -0500, Robert Haas wrote:
> > > > > > Andres, what do you think about this idea? I wonder if you just
> > > > > > momentarily forgot about temporary relations when coding
> > > > > > RelidByRelfilenumber -- because for that function to give well-defined
> > > > > > answers with temporary relations included, it would need the backend
> > > > > > ID as an additional argument.
> > > > >
> > > > >
> > > > > Ignoring temporary relations entirely makes sense: one cannot get a
> > > > > regclass from only a tablespace and a relfilenode, the persistence, as
> > > > > well as a backend ID would also be required. I've not checked the
> > > > > patch in details, but it's to say that the idea to cut temporary
> > > > > relations sounds rather right here.
> > > >
> > > > That makes sense to me too.
> > > >
> > > > Regarding the patch, filtering by the relpersistence in
> > > > systable_getnext() loop seems to be good to me. Alternatively we can
> > > > add "relpersistence == RELPERSISTENCE_TEMP" to the scan key.
> >
> > Do you mean relpersistence != RELPERSISTENCE_TEMP? I tried that. It's
> > not possible to add this check to the scankey since indexes do not
> > work with != conditions. We will need to use a sequential scan to do
> > so, but that will affect the performance. I think what Vignesh has
> > done in his patch is good enough.
> >
> > >
> > > The attached patch adds a new test and resolves an existing test
> > > failure. However, a downside is that we can no longer verify the
> > > mapping of the temporary tables.
> >
> > Yes, but I think the current way of verification wasn't correct anyway
> > because it couldn't match the temporary table's relation exactly. We
> > will need to devise another way to do that, maybe creating a version
> > of pg_filenode_relation() for temporary tables.
> >
> > Some more comments, some of which I have applied in the attached
> > patches. Please review those. Let me know what you think.
> >
> > I feel that we should mention permanent tables in the prologue of
> > pg_filenode_relation() for somebody who just looks at
> > pg_filenode_relation(). Also in its pg_proc.dat description for one
> > who looks at \df+ output. Attached patch does that.
> >
> > - * Returns InvalidOid if no relation matching the criteria could be found.
> > + * Returns InvalidOid if no permanent relation matching the criteria could be
> > + * found.
> >
> > The relpersistence enum has values
> > #define RELPERSISTENCE_PERMANENT 'p' /* regular table */
> > #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent table */
> > #define RELPERSISTENCE_TEMP 't' /* temporary table */
> >
> > The description of UNLOGGED mentions "permanent" so it looks like your
> > use of "permanent" covers both regular and unlogged tables. However
> > looking purely at the RELPERSISTENCE_ names, it' s possible that one
> > will associate the word "permanent" with RELPERSISTENCE_PERMANENT
> > only, and not with RELPERSISTENCE_UNLOGGED. Should we use
> > "non-temporary" instead to avoid confusion?
> >
> > + /*
> > + * Temporary relations should be excluded. This exclusion is
> > + * necessary because they can lead to the wrong result; the
> > + * relfilenumber space is shared with persistent
> > + * relations. Additionally, excluding them is possible since they
> > + * are not the focus in this context.
> > + */
> > + if (classform->relpersistence == RELPERSISTENCE_TEMP)
> > + continue;
> > +
> >
> > I slightly rephrased the comment and moved it to the function prologue
> > since it defines the function's scope.
> >
> > WHERE relkind IN ('r', 'i', 'S', 't', 'm') AND mapped_oid IS DISTINCT FROM oid;
> > +WHERE relkind IN ('r', 'i', 'S', 'm') AND mapped_oid IS DISTINCT FROM oid;
> >
> > Why do we need to remove permanent toast tables from the query?
> > Instead adjustment below
> >
> > SELECT m.* FROM filenode_mapping m LEFT JOIN pg_class c ON c.oid = m.oid
> > -WHERE c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL;
> > +WHERE (c.oid IS NOT NULL OR m.mapped_oid IS NOT NULL) AND
> > c.relpersistence != 't';
> >
> > seems enough. Actually, we shouldn't pass temporary tables to
> > pg_filenode_relation(), since it doesn't map temporary relations now.
> > Adjusted that way in the attached patch.
> >
> > While reviewing the patch, I found something else. The
> > RelidByRelfilenumber() enters negative cache entries.
> > RelfilenumberMapInvalidateCallback() treats the negative entries
> > specifically which indicates that it's intentional. But if somebody
> > calls pg_filenode_relation() repeatedly with invalid relfilenodes,
> > that would bloat the cache with "kinda useless entries". It's a way to
> > make a backend consume a lot of memory quickly and thus perform a DOS.
> > For example, on my laptop, I could make a backend consume almost 3GB
> > of memory in 7 minutes.
> >
> > #SELECT pg_size_pretty(total_bytes) total_bytes,
> > pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
> > where name = 'RelfilenumberMap cache';
> > total_bytes | used_bytes
> > -------------+------------
> > (0 rows)
> >
> > #\timing
> > Timing is on.
> > #select r is null, count(*) from (select pg_filenode_relation(i, j) r
> > from generate_series(1, 1000) i, generate_series(1, 1000) j) q group
> > by r is null;
> > ?column? | count
> > ----------+---------
> > t | 1000000
> > (1 row)
> >
> > Time: 4705.483 ms (00:04.705)
> > #SELECT pg_size_pretty(total_bytes) total_bytes,
> > pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
> > where name = 'RelfilenumberMap cache';
> > total_bytes | used_bytes
> > -------------+------------
> > 40 MB | 39 MB
> > (1 row)
> >
> > Time: 2.351 ms
> > #select r is null, count(*) from (select pg_filenode_relation(i, j) r
> > from generate_series(1001, 10000) i, generate_series(1001, 10000) j) q
> > group by r is null;
> > ?column? | count
> > ----------+----------
> > f | 153215
> > t | 80846785
> > (2 rows)
> >
> > Time: 421998.039 ms (07:01.998)
> > #SELECT pg_size_pretty(total_bytes) total_bytes,
> > pg_size_pretty(used_bytes) used_bytes FROM pg_backend_memory_contexts
> > where name = 'RelfilenumberMap cache';
> > total_bytes | used_bytes
> > -------------+------------
> > 3180 MB | 3176 MB
> > (1 row)
> >
> > Time: 132.187 ms
> >
> > Logical replication and autoprewarm may not cause such a large bloat
> > but there is no limit to passing invalid combinations of reltablespace
> > and relfilenumber to pg_filenode_relation(). Do we want to prohibit
> > that case by passing a flag from logical pg_filenode_relation() to not
> > cache invalid entries?
> >
> > I have moved the CF entry to the July commitfest.
>
> The patch needed a rebase because of func.sgml refactoring. Here's a
> rebased patchset.
Please ignore the 0002 patch in the earlier patchset. It's an
experimental change related to negative entries bloat in
RelfilenumberMap cache.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-08-21 06:12 Michael Paquier <michael@paquier.xyz>
parent: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
0 siblings, 2 replies; 25+ messages in thread
From: Michael Paquier @ 2025-08-21 06:12 UTC (permalink / raw)
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; +Cc: vignesh C <vignesh21@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Wed, Aug 13, 2025 at 03:29:50PM +0530, Ashutosh Bapat wrote:
Sorry for the delay. This has been on my TODO list for some time.
Back to it now.
> On Wed, Aug 13, 2025 at 3:18 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>> On Thu, Jun 19, 2025 at 2:17 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>> The description of UNLOGGED mentions "permanent" so it looks like your
>>> use of "permanent" covers both regular and unlogged tables. However
>>> looking purely at the RELPERSISTENCE_ names, it' s possible that one
>>> will associate the word "permanent" with RELPERSISTENCE_PERMANENT
>>> only, and not with RELPERSISTENCE_UNLOGGED. Should we use
>>> "non-temporary" instead to avoid confusion?
FWIW, I see a pretty good point in backpatching what we have here, as
active users of logical replication can be impacted by this issue
randomly. Even if this requires a wraparound, for some users it may
be a window that can open in a couple of days, mostly for workloads
with a high temp table exhaustion, I guess. In short, let's not touch
pg_proc.dat, and I'm planning for a backpatch all the way down due to
the possible random intrusions with the logirep paths.
The changes at the top of RelidByRelfilenumber() describing the
problem with temporary tables feels a bit bloated. I see no need to
mention directly autoprewarm and logical replication, two of the three
callers of this routine, with the explanation coming down to the fact
that this is only a temp relation problem because we cannot pinpoint
to an OID without knowing a proc number. So let's cut a few lines
here at the top of RelidByRelfilenumber(), let's add a simpler line in
func-admin.sgml to say directly and only that "temporary relations are
not supported", passing on the "permanent" vs "no permanent" business.
I don't see a direct need to mention that at the top of
pg_relation_filenode(), as long as RelidByRelfilenumber(), but OK by
me to add a simpler "temporary relations are not supported", it looks
like most prefer than based on the contents of the thread.
>>> Logical replication and autoprewarm may not cause such a large bloat
>>> but there is no limit to passing invalid combinations of reltablespace
>>> and relfilenumber to pg_filenode_relation(). Do we want to prohibit
>>> that case by passing a flag from logical pg_filenode_relation() to not
>>> cache invalid entries?
Fun. Yes, I agree that we could do better here. It does not strike
me as an issue as invasive as the original report, direct calls of
pg_filenode_relation() are much rarer than the code paths of
autoprewarm and logical replication touched by RelidByRelfilenumber().
That's a potential HEAD improvement to me.
> Please ignore the 0002 patch in the earlier patchset. It's an
> experimental change related to negative entries bloat in
> RelfilenumberMap cache.
And I was wondering what was going on in the latest patch set.. I'm
ignoring this part.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aKa4wnxX6h2siDIq@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-08-21 14:24 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 25+ messages in thread
From: Ashutosh Bapat @ 2025-08-21 14:24 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; vignesh C <vignesh21@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Thu, Aug 21, 2025 at 11:42 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Aug 13, 2025 at 03:29:50PM +0530, Ashutosh Bapat wrote:
>
> Sorry for the delay. This has been on my TODO list for some time.
> Back to it now.
>
> > On Wed, Aug 13, 2025 at 3:18 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >> On Thu, Jun 19, 2025 at 2:17 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >>> The description of UNLOGGED mentions "permanent" so it looks like your
> >>> use of "permanent" covers both regular and unlogged tables. However
> >>> looking purely at the RELPERSISTENCE_ names, it' s possible that one
> >>> will associate the word "permanent" with RELPERSISTENCE_PERMANENT
> >>> only, and not with RELPERSISTENCE_UNLOGGED. Should we use
> >>> "non-temporary" instead to avoid confusion?
>
> FWIW, I see a pretty good point in backpatching what we have here, as
> active users of logical replication can be impacted by this issue
> randomly. Even if this requires a wraparound, for some users it may
> be a window that can open in a couple of days, mostly for workloads
> with a high temp table exhaustion, I guess. In short, let's not touch
> pg_proc.dat, and I'm planning for a backpatch all the way down due to
> the possible random intrusions with the logirep paths.
The only change in pg_proc.dat is change in the function description.
We can revert it for the benefit of backpatching. Its usefulness is
arguable. +1 for not touching pg_proc.dat.
>
> The changes at the top of RelidByRelfilenumber() describing the
> problem with temporary tables feels a bit bloated. I see no need to
> mention directly autoprewarm and logical replication, two of the three
> callers of this routine, with the explanation coming down to the fact
> that this is only a temp relation problem because we cannot pinpoint
> to an OID without knowing a proc number.
How about the following:
---
Map a relation's (tablespace, relfilenumber) to a relation's oid and
cache the result.
A temporary relation may have the same relfilenumber as a permanent
relation, or as other temporary relations. In order to uniquely
identify a temporary relation we also need the
proc number of the backend which created it. The proc number is not
available to this function. Hence ignore temporary relations.
Returns InvalidOid if no permanent relation matching the criteria
could be found.
---
> So let's cut a few lines
> here at the top of RelidByRelfilenumber(), let's add a simpler line in
> func-admin.sgml to say directly and only that "temporary relations are
> not supported", passing on the "permanent" vs "no permanent" business.
> I don't see a direct need to mention that at the top of
> pg_relation_filenode(), as long as RelidByRelfilenumber(), but OK by
> me to add a simpler "temporary relations are not supported", it looks
> like most prefer than based on the contents of the thread.
If we just say that temporary relations are not supported, the user
may wonder as to what will happen if they pass tablespace and
relfilenode of a temporary relation. Instead it's better to clarify
that behaviour. How about adding following line to the current
description of the function in func-admin.sgml
"Also, returns <literal>NULL</literal> if a temporary relation in the
current database is associated with the given values.".
Said that if you insist on adding just "Temporary relations are not
supported", we can go ahead with it.
>
> >>> Logical replication and autoprewarm may not cause such a large bloat
> >>> but there is no limit to passing invalid combinations of reltablespace
> >>> and relfilenumber to pg_filenode_relation(). Do we want to prohibit
> >>> that case by passing a flag from logical pg_filenode_relation() to not
> >>> cache invalid entries?
>
> Fun. Yes, I agree that we could do better here. It does not strike
> me as an issue as invasive as the original report, direct calls of
> pg_filenode_relation() are much rarer than the code paths of
> autoprewarm and logical replication touched by RelidByRelfilenumber().
> That's a potential HEAD improvement to me.
Ok. I will make a patch to ignore negative entries when
RelidByRelfilenumber() is invoked from pg_filenode_relation(). I am ok
with head-only improvement. If we find reports from the field that
this behaviour creates problems in the previous releases, we will be
able to back patch it. I will try to come up with a back-patchable
patch.
Vignesh, since you are the original author of the patch, do you want
to take care of addressing the comments? Otherwise I can.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-08-21 21:36 Michael Paquier <michael@paquier.xyz>
parent: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
0 siblings, 0 replies; 25+ messages in thread
From: Michael Paquier @ 2025-08-21 21:36 UTC (permalink / raw)
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; +Cc: vignesh C <vignesh21@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Thu, Aug 21, 2025 at 07:54:55PM +0530, Ashutosh Bapat wrote:
> How about the following:
>
> ---
> Map a relation's (tablespace, relfilenumber) to a relation's oid and
> cache the result.
>
> A temporary relation may have the same relfilenumber as a permanent
> relation, or as other temporary relations. In order to uniquely
> identify a temporary relation we also need the
> proc number of the backend which created it. The proc number is not
> available to this function. Hence ignore temporary relations.
>
> Returns InvalidOid if no permanent relation matching the criteria
> could be found.
> ---
That's simpler.
> Said that if you insist on adding just "Temporary relations are not
> supported", we can go ahead with it.
That would be simpler as well, providing the information.
> Vignesh, since you are the original author of the patch, do you want
> to take care of addressing the comments? Otherwise I can.
It's fine. I am going to take care of that.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aKeReH7_GwoMUoLi@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-09-18 12:07 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 25+ messages in thread
From: Ashutosh Bapat @ 2025-09-18 12:07 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: vignesh C <vignesh21@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com, osumi.takamichi@fujitsu.com, pgsql-bugs@lists.postgresql.org, Andres Freund <andres@anarazel.de>
On Thu, Aug 21, 2025 at 11:42 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Aug 13, 2025 at 03:29:50PM +0530, Ashutosh Bapat wrote:
>
> >>> Logical replication and autoprewarm may not cause such a large bloat
> >>> but there is no limit to passing invalid combinations of reltablespace
> >>> and relfilenumber to pg_filenode_relation(). Do we want to prohibit
> >>> that case by passing a flag from logical pg_filenode_relation() to not
> >>> cache invalid entries?
>
> Fun. Yes, I agree that we could do better here. It does not strike
> me as an issue as invasive as the original report, direct calls of
> pg_filenode_relation() are much rarer than the code paths of
> autoprewarm and logical replication touched by RelidByRelfilenumber().
> That's a potential HEAD improvement to me.
Sorry for taking time. Here's a patch to address this. Added to the commitfest.
--
Best Wishes,
Ashutosh Bapat
Attachments:
[text/x-patch] 0001-Negative-RelfilenumberMap-cache-entries-fro-20250918.patch (5.5K, ../../CAExHW5uQ7xE9zuRT6Au1VG_eJXZJtVLe1KP7HRsoRw2iRuQQyw@mail.gmail.com/2-0001-Negative-RelfilenumberMap-cache-entries-fro-20250918.patch)
download | inline diff:
From 6a3562b4ac8917c8b577797e5468416a90cc04f5 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Thu, 18 Sep 2025 17:24:09 +0530
Subject: [PATCH] Negative RelfilenumberMap cache entries from
pg_filenode_relation()
RelidByRelfilenumber() adds negative entries to the cache. It has three
users, logical replication, autoprewarm and pg_filenode_relation(). The
first two need negative entries in the cache in case they happen to
lookup non-existent mapping again and again. However such mappings will
be smaller in number and usually come from some database object e.g. WAL
or autoprewarm metadata.
But pg_filenode_relation(), which is SQL callable, may be invoked many
times with invalid tablespace and relfilenode pairs, causing the cache
to be bloated with negative cache entries. This can be used as a denial
of service attack since any user can execute it. This commit avoids such
a bloat.
Reported by: Ashutosh Bapat
Author: Ashutosh Bapat
---
contrib/pg_prewarm/autoprewarm.c | 6 +++++-
.../replication/logical/reorderbuffer.c | 7 ++++++-
src/backend/utils/adt/dbsize.c | 6 +++++-
src/backend/utils/cache/relfilenumbermap.c | 18 +++++++++++++-----
src/include/utils/relfilenumbermap.h | 3 ++-
5 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 8b68dafc261..9732495eaa4 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -540,7 +540,11 @@ autoprewarm_database_main(Datum main_arg)
StartTransactionCommand();
- reloid = RelidByRelfilenumber(blk.tablespace, blk.filenumber);
+ /*
+ * Cache negative entries in case we end up looking up a non-existent
+ * mapping again and again.
+ */
+ reloid = RelidByRelfilenumber(blk.tablespace, blk.filenumber, true);
if (!OidIsValid(reloid) ||
(rel = try_relation_open(reloid, AccessShareLock)) == NULL)
{
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4736f993c37..b85f426e19f 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2326,8 +2326,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
case REORDER_BUFFER_CHANGE_DELETE:
Assert(snapshot_now);
+ /*
+ * Cache negative entries in case we end up looking up a
+ * non-existent mapping again and again.
+ */
reloid = RelidByRelfilenumber(change->data.tp.rlocator.spcOid,
- change->data.tp.rlocator.relNumber);
+ change->data.tp.rlocator.relNumber,
+ true);
/*
* Mapped catalog tuple without data, emitted while
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 894d226541f..8bf22daa4c2 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -955,7 +955,11 @@ pg_filenode_relation(PG_FUNCTION_ARGS)
if (!RelFileNumberIsValid(relfilenumber))
PG_RETURN_NULL();
- heaprel = RelidByRelfilenumber(reltablespace, relfilenumber);
+ /*
+ * A malicious user invoke the SQL function with negative entries many
+ * times to consume memory as a DoS attach. Avoid that.
+ */
+ heaprel = RelidByRelfilenumber(reltablespace, relfilenumber, false);
if (!OidIsValid(heaprel))
PG_RETURN_NULL();
diff --git a/src/backend/utils/cache/relfilenumbermap.c b/src/backend/utils/cache/relfilenumbermap.c
index 0b6f9cf3fa1..0aed3058350 100644
--- a/src/backend/utils/cache/relfilenumbermap.c
+++ b/src/backend/utils/cache/relfilenumbermap.c
@@ -135,10 +135,13 @@ InitializeRelfilenumberMap(void)
* identify a temporary relation would require a backend's proc number, which
* we do not know about. Hence, this function ignores this case.
*
+ * If cache_negative is true, negative cache entries (mapping to InvalidOid)
+ * will be stored in the cache. If false, they won't be cached.
+ *
* Returns InvalidOid if no relation matching the criteria could be found.
*/
Oid
-RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
+RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber, bool cache_negative)
{
RelfilenumberMapKey key;
RelfilenumberMapEntry *entry;
@@ -239,11 +242,16 @@ RelidByRelfilenumber(Oid reltablespace, RelFileNumber relfilenumber)
* Only enter entry into cache now, our opening of pg_class could have
* caused cache invalidations to be executed which would have deleted a
* new entry if we had entered it above.
+ *
+ * Cache valid entries always, and negative entries only if requested.
*/
- entry = hash_search(RelfilenumberMapHash, &key, HASH_ENTER, &found);
- if (found)
- elog(ERROR, "corrupted hashtable");
- entry->relid = relid;
+ if (OidIsValid(relid) || cache_negative)
+ {
+ entry = hash_search(RelfilenumberMapHash, &key, HASH_ENTER, &found);
+ if (found)
+ elog(ERROR, "corrupted hashtable");
+ entry->relid = relid;
+ }
return relid;
}
diff --git a/src/include/utils/relfilenumbermap.h b/src/include/utils/relfilenumbermap.h
index 86fadefe499..5799a3257d3 100644
--- a/src/include/utils/relfilenumbermap.h
+++ b/src/include/utils/relfilenumbermap.h
@@ -16,6 +16,7 @@
#include "common/relpath.h"
extern Oid RelidByRelfilenumber(Oid reltablespace,
- RelFileNumber relfilenumber);
+ RelFileNumber relfilenumber,
+ bool cache_negative);
#endif /* RELFILENUMBERMAP_H */
base-commit: 2e66cae935c2e0f7ce9bab6b65ddeb7806f4de7c
--
2.34.1
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-09-18 12:17 Andres Freund <andres@anarazel.de>
parent: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
0 siblings, 1 reply; 25+ messages in thread
From: Andres Freund @ 2025-09-18 12:17 UTC (permalink / raw)
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; vignesh C <vignesh21@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2025-09-18 17:37:10 +0530, Ashutosh Bapat wrote:
> From 6a3562b4ac8917c8b577797e5468416a90cc04f5 Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
> Date: Thu, 18 Sep 2025 17:24:09 +0530
> Subject: [PATCH] Negative RelfilenumberMap cache entries from
> pg_filenode_relation()
>
> RelidByRelfilenumber() adds negative entries to the cache. It has three
> users, logical replication, autoprewarm and pg_filenode_relation(). The
> first two need negative entries in the cache in case they happen to
> lookup non-existent mapping again and again. However such mappings will
> be smaller in number and usually come from some database object e.g. WAL
> or autoprewarm metadata.
>
> But pg_filenode_relation(), which is SQL callable, may be invoked many
> times with invalid tablespace and relfilenode pairs, causing the cache
> to be bloated with negative cache entries. This can be used as a denial
> of service attack since any user can execute it. This commit avoids such
> a bloat.
I don't really understand why this is worth fixing for the relfilenode stuff
specifically - isn't this true for just about *all* of our caches? Many, if
not most, can be reached via SQL?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-09-18 12:22 Andres Freund <andres@anarazel.de>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 1 reply; 25+ messages in thread
From: Andres Freund @ 2025-09-18 12:22 UTC (permalink / raw)
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; vignesh C <vignesh21@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2025-09-18 08:17:49 -0400, Andres Freund wrote:
> On 2025-09-18 17:37:10 +0530, Ashutosh Bapat wrote:
> > From 6a3562b4ac8917c8b577797e5468416a90cc04f5 Mon Sep 17 00:00:00 2001
> > From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
> > Date: Thu, 18 Sep 2025 17:24:09 +0530
> > Subject: [PATCH] Negative RelfilenumberMap cache entries from
> > pg_filenode_relation()
> >
> > RelidByRelfilenumber() adds negative entries to the cache. It has three
> > users, logical replication, autoprewarm and pg_filenode_relation(). The
> > first two need negative entries in the cache in case they happen to
> > lookup non-existent mapping again and again. However such mappings will
> > be smaller in number and usually come from some database object e.g. WAL
> > or autoprewarm metadata.
> >
> > But pg_filenode_relation(), which is SQL callable, may be invoked many
> > times with invalid tablespace and relfilenode pairs, causing the cache
> > to be bloated with negative cache entries. This can be used as a denial
> > of service attack since any user can execute it. This commit avoids such
> > a bloat.
>
> I don't really understand why this is worth fixing for the relfilenode stuff
> specifically - isn't this true for just about *all* of our caches? Many, if
> not most, can be reached via SQL?
Example:
postgres[315631][1]=# SELECT count(*), sum(total_bytes) total_bytes, sum(total_nblocks) total_nblocks, sum(free_bytes) free_bytes, sum(free_chunks) free_chunks, sum(used_bytes) used_bytes FROM pg_backend_memory_contexts WHERE path @> (SELECT path FROM pg_backend_memory_contexts WHERE name = 'CacheMemoryContext');
┌───────┬─────────────┬───────────────┬────────────┬─────────────┬────────────┐
│ count │ total_bytes │ total_nblocks │ free_bytes │ free_chunks │ used_bytes │
├───────┼─────────────┼───────────────┼────────────┼─────────────┼────────────┤
│ 89 │ 747200 │ 187 │ 130336 │ 216 │ 616864 │
└───────┴─────────────┴───────────────┴────────────┴─────────────┴────────────┘
(1 row)
Time: 1.540 ms
postgres[315631][1]=# SELECT to_regclass(g.i::text||'.'||g.i::text) is NULL, count(*) FROM generate_series(1, 10000000) g(i) GROUP BY 1;
┌──────────┬──────────┐
│ ?column? │ count │
├──────────┼──────────┤
│ t │ 10000000 │
└──────────┴──────────┘
(1 row)
Time: 23238.662 ms (00:23.239)
postgres[315631][1]=# SELECT count(*), sum(total_bytes) total_bytes, sum(total_nblocks) total_nblocks, sum(free_bytes) free_bytes, sum(free_chunks) free_chunks, sum(used_bytes) used_bytes FROM pg_backend_memory_contexts WHERE path @> (SELECT path FROM pg_backend_memory_contexts WHERE name = 'CacheMemoryContext');
┌───────┬─────────────┬───────────────┬────────────┬─────────────┬────────────┐
│ count │ total_bytes │ total_nblocks │ free_bytes │ free_chunks │ used_bytes │
├───────┼─────────────┼───────────────┼────────────┼─────────────┼────────────┤
│ 89 │ 2223205104 │ 441 │ 8331448 │ 472 │ 2214873656 │
└───────┴─────────────┴───────────────┴────────────┴─────────────┴────────────┘
(1 row)
There are so so so many ways to cause arbitrarily large memory usage,
e.g. just by definining a lot of prepared statements, that I just don't see it
worth adding any complexity to the relfilenode case specifically.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 25+ messages in thread
* Re: "unexpected duplicate for tablespace" problem in logical replication
@ 2025-09-18 15:30 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 25+ messages in thread
From: Ashutosh Bapat @ 2025-09-18 15:30 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Michael Paquier <michael@paquier.xyz>; vignesh C <vignesh21@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; wangsh.fnst@fujitsu.com; osumi.takamichi@fujitsu.com; pgsql-bugs@lists.postgresql.org
On Thu, Sep 18, 2025 at 5:53 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2025-09-18 08:17:49 -0400, Andres Freund wrote:
> > On 2025-09-18 17:37:10 +0530, Ashutosh Bapat wrote:
> > > From 6a3562b4ac8917c8b577797e5468416a90cc04f5 Mon Sep 17 00:00:00 2001
> > > From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
> > > Date: Thu, 18 Sep 2025 17:24:09 +0530
> > > Subject: [PATCH] Negative RelfilenumberMap cache entries from
> > > pg_filenode_relation()
> > >
> > > RelidByRelfilenumber() adds negative entries to the cache. It has three
> > > users, logical replication, autoprewarm and pg_filenode_relation(). The
> > > first two need negative entries in the cache in case they happen to
> > > lookup non-existent mapping again and again. However such mappings will
> > > be smaller in number and usually come from some database object e.g. WAL
> > > or autoprewarm metadata.
> > >
> > > But pg_filenode_relation(), which is SQL callable, may be invoked many
> > > times with invalid tablespace and relfilenode pairs, causing the cache
> > > to be bloated with negative cache entries. This can be used as a denial
> > > of service attack since any user can execute it. This commit avoids such
> > > a bloat.
> >
> > I don't really understand why this is worth fixing for the relfilenode stuff
> > specifically - isn't this true for just about *all* of our caches? Many, if
> > not most, can be reached via SQL?
>
> Example:
>
> postgres[315631][1]=# SELECT count(*), sum(total_bytes) total_bytes, sum(total_nblocks) total_nblocks, sum(free_bytes) free_bytes, sum(free_chunks) free_chunks, sum(used_bytes) used_bytes FROM pg_backend_memory_contexts WHERE path @> (SELECT path FROM pg_backend_memory_contexts WHERE name = 'CacheMemoryContext');
> ┌───────┬─────────────┬───────────────┬────────────┬─────────────┬────────────┐
> │ count │ total_bytes │ total_nblocks │ free_bytes │ free_chunks │ used_bytes │
> ├───────┼─────────────┼───────────────┼────────────┼─────────────┼────────────┤
> │ 89 │ 747200 │ 187 │ 130336 │ 216 │ 616864 │
> └───────┴─────────────┴───────────────┴────────────┴─────────────┴────────────┘
> (1 row)
>
> Time: 1.540 ms
> postgres[315631][1]=# SELECT to_regclass(g.i::text||'.'||g.i::text) is NULL, count(*) FROM generate_series(1, 10000000) g(i) GROUP BY 1;
> ┌──────────┬──────────┐
> │ ?column? │ count │
> ├──────────┼──────────┤
> │ t │ 10000000 │
> └──────────┴──────────┘
> (1 row)
>
Interesting! I didn't know that cat cache could have negative entries
in it. But SearchCatCacheMiss says so explicitly
/*
* Tuple was not found in cache, so we have to try to retrieve it directly
* from the relation. If found, we will add it to the cache; if not
* found, we will add a negative cache entry instead.
*/
That settles it. Use of negative entries spread wider than I thought
and in places where they may not even be useful. Thanks for the
example. I don't see any reason to pursue this patch specifically.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 25+ messages in thread
end of thread, other threads:[~2025-09-18 15:30 UTC | newest]
Thread overview: 25+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-06 02:14 "unexpected duplicate for tablespace" problem in logical replication wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
2022-04-08 09:44 ` osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
2022-04-11 07:40 ` osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
2022-04-11 10:24 ` wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
2022-04-12 02:45 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2022-04-12 03:36 ` wangsh.fnst@fujitsu.com <wangsh.fnst@fujitsu.com>
2022-04-19 06:23 ` Masahiko Sawada <sawada.mshk@gmail.com>
2024-01-08 05:02 ` vignesh C <vignesh21@gmail.com>
2024-01-12 08:37 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2024-01-15 21:32 ` Robert Haas <robertmhaas@gmail.com>
2024-01-16 01:58 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2024-02-02 15:49 ` Robert Haas <robertmhaas@gmail.com>
2024-03-18 06:35 ` Michael Paquier <michael@paquier.xyz>
2024-07-24 21:51 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-03-25 10:01 ` vignesh C <vignesh21@gmail.com>
2025-06-19 08:47 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
2025-08-13 09:48 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
2025-08-13 09:59 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
2025-08-21 06:12 ` Michael Paquier <michael@paquier.xyz>
2025-08-21 14:24 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
2025-08-21 21:36 ` Michael Paquier <michael@paquier.xyz>
2025-09-18 12:07 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
2025-09-18 12:17 ` Andres Freund <andres@anarazel.de>
2025-09-18 12:22 ` Andres Freund <andres@anarazel.de>
2025-09-18 15:30 ` Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox