public inbox for [email protected]
help / color / mirror / Atom feedRe: Clarification on Role Access Rights to Table Indexes
12+ messages / 2 participants
[nested] [flat]
* Re: Clarification on Role Access Rights to Table Indexes
@ 2025-09-24 16:52 Nathan Bossart <[email protected]>
2025-10-02 17:37 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Nathan Bossart @ 2025-09-24 16:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Sep 24, 2025 at 12:13:34PM -0400, Tom Lane wrote:
> Nathan Bossart <[email protected]> writes:
>> * RangeVarCallbackForReindexIndex() was checking privileges on the table
>> before locking it, so I reversed it in 0002.
>
> Don't we do that intentionally, to make sure someone can't cause DOS
> on a table they have no privileges on?
Ah, right. I switched it back in v4.
--
nathan
From 9cf6e507808cbd4a5c8b93422881ec078ce66f60 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v4 1/3] fix priv checks in stats code
---
src/backend/statistics/stat_utils.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..8b8203a58e3 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -189,6 +189,17 @@ stats_lock_check_privileges(Oid reloid)
Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+ /*
+ * Since we did the IndexGetRelation() call above without any lock,
+ * it's barely possible that a race against an index drop/recreation
+ * could have netted us the wrong table.
+ */
+ if (table_oid != IndexGetRelation(index_oid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(index))));
+
/* retain lock on index */
relation_close(index, NoLock);
}
--
2.39.5 (Apple Git-154)
From 3f6d161c46456b95dfae949ecbe44bd028b6a37d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v4 2/3] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 41 ++++++++++++--------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..2a6e58eb0de 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2986,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
table_lockmode = (state->params.options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock;
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
+ /* Unlock any previously locked heap. */
+ if (OidIsValid(state->locked_table_oid))
{
UnlockRelationOid(state->locked_table_oid, table_lockmode);
state->locked_table_oid = InvalidOid;
@@ -3014,30 +3011,22 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
+ /*
+ * If the OID isn't valid, it means the index was concurrently dropped,
+ * which is not a problem for us; just return normally.
+ */
table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
+ if (!OidIsValid(table_oid))
+ return;
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ /* Check permissions */
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
- if (relId != oldRelId)
- {
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
}
/*
--
2.39.5 (Apple Git-154)
From 2c1da2d3db13603da4b2cded4a2dcf0d86614ca0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v4 3/3] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 34 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 +++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..810a291204b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,31 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, false);
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table.
+ */
+ if (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +260,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] v4-0001-fix-priv-checks-in-stats-code.patch (1.1K, 2-v4-0001-fix-priv-checks-in-stats-code.patch)
download | inline diff:
From 9cf6e507808cbd4a5c8b93422881ec078ce66f60 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v4 1/3] fix priv checks in stats code
---
src/backend/statistics/stat_utils.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..8b8203a58e3 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -189,6 +189,17 @@ stats_lock_check_privileges(Oid reloid)
Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+ /*
+ * Since we did the IndexGetRelation() call above without any lock,
+ * it's barely possible that a race against an index drop/recreation
+ * could have netted us the wrong table.
+ */
+ if (table_oid != IndexGetRelation(index_oid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(index))));
+
/* retain lock on index */
relation_close(index, NoLock);
}
--
2.39.5 (Apple Git-154)
[text/plain] v4-0002-fix-priv-checks-in-index-code.patch (2.7K, 3-v4-0002-fix-priv-checks-in-index-code.patch)
download | inline diff:
From 3f6d161c46456b95dfae949ecbe44bd028b6a37d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v4 2/3] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 41 ++++++++++++--------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..2a6e58eb0de 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2986,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
table_lockmode = (state->params.options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock;
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
+ /* Unlock any previously locked heap. */
+ if (OidIsValid(state->locked_table_oid))
{
UnlockRelationOid(state->locked_table_oid, table_lockmode);
state->locked_table_oid = InvalidOid;
@@ -3014,30 +3011,22 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
+ /*
+ * If the OID isn't valid, it means the index was concurrently dropped,
+ * which is not a problem for us; just return normally.
+ */
table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
+ if (!OidIsValid(table_oid))
+ return;
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ /* Check permissions */
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
- if (relId != oldRelId)
- {
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
}
/*
--
2.39.5 (Apple Git-154)
[text/plain] v4-0003-fix-priv-checks-in-pg_prewarm.patch (4.4K, 4-v4-0003-fix-priv-checks-in-pg_prewarm.patch)
download | inline diff:
From 2c1da2d3db13603da4b2cded4a2dcf0d86614ca0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v4 3/3] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 34 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 +++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..810a291204b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,31 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, false);
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table.
+ */
+ if (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +260,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-02 17:37 ` Nathan Bossart <[email protected]>
1 sibling, 0 replies; 12+ messages in thread
From: Nathan Bossart @ 2025-10-02 17:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Sep 24, 2025 at 11:52:09AM -0500, Nathan Bossart wrote:
> Ah, right. I switched it back in v4.
Unless more feedback materializes, I'm planning to commit these sometime
next week.
--
nathan
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-09 03:06 ` Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Jeff Davis @ 2025-10-09 03:06 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, 2025-09-24 at 11:52 -0500, Nathan Bossart wrote:
> On Wed, Sep 24, 2025 at 12:13:34PM -0400, Tom Lane wrote:
> > Nathan Bossart <[email protected]> writes:
> > > * RangeVarCallbackForReindexIndex() was checking privileges on
> > > the table
> > > before locking it, so I reversed it in 0002.
> >
> > Don't we do that intentionally, to make sure someone can't cause
> > DOS
> > on a table they have no privileges on?
>
> Ah, right. I switched it back in v4.
v4-0001 looks good to me.
Just to make sure I understand: the actual problem would only happen
with OID wraparound, right?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
@ 2025-10-09 03:28 ` Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Davis @ 2025-10-09 03:28 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; +Cc: Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, 2025-10-08 at 20:06 -0700, Jeff Davis wrote:
> On Wed, 2025-09-24 at 11:52 -0500, Nathan Bossart wrote:
> > On Wed, Sep 24, 2025 at 12:13:34PM -0400, Tom Lane wrote:
> > > Nathan Bossart <[email protected]> writes:
> > > > * RangeVarCallbackForReindexIndex() was checking privileges on
> > > > the table
> > > > before locking it, so I reversed it in 0002.
> > >
> > > Don't we do that intentionally, to make sure someone can't cause
> > > DOS
> > > on a table they have no privileges on?
> >
> > Ah, right. I switched it back in v4.
>
> v4-0001 looks good to me.
Actually, now I'm unsure. v4-0001 is taking a lock on the table before
checking privileges, whereas v4-0002 is going to some effort to avoid
that. Is that because the latter is taking a ShareLock?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
@ 2025-10-09 15:39 ` Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2025-10-09 15:39 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Oct 08, 2025 at 08:28:01PM -0700, Jeff Davis wrote:
> Actually, now I'm unsure. v4-0001 is taking a lock on the table before
> checking privileges, whereas v4-0002 is going to some effort to avoid
> that. Is that because the latter is taking a ShareLock?
I was confused by this, too. We seem to go to great lengths to avoid
taking a lock before checking permissions in RangeVarGetRelidExtended(),
but in pg_prewarm() and this stats code, we are taking the lock first.
pg_prewarm() can't use RangeVarGetRelid because you give it the OID, but
I'm not seeing why stat_utils.c can't use it. We should probably fix this.
I wouldn't be surprised if there are other examples.
--
nathan
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-09 21:18 ` Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2025-10-09 21:18 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Oct 09, 2025 at 10:39:32AM -0500, Nathan Bossart wrote:
> On Wed, Oct 08, 2025 at 08:28:01PM -0700, Jeff Davis wrote:
>> Actually, now I'm unsure. v4-0001 is taking a lock on the table before
>> checking privileges, whereas v4-0002 is going to some effort to avoid
>> that. Is that because the latter is taking a ShareLock?
>
> I was confused by this, too. We seem to go to great lengths to avoid
> taking a lock before checking permissions in RangeVarGetRelidExtended(),
> but in pg_prewarm() and this stats code, we are taking the lock first.
> pg_prewarm() can't use RangeVarGetRelid because you give it the OID, but
> I'm not seeing why stat_utils.c can't use it. We should probably fix this.
> I wouldn't be surprised if there are other examples.
I spent some time trying to change pg_prewarm() to check permissions before
locking and came up with the attached. There are certainly issues with the
patch, but this at least demonstrates the complexity required. I'm tempted
to say that this is more trouble than it's worth, but it does feel a little
weird to leave it as-is.
There's a similar pattern in get_rel_from_relname() in dblink.c, which also
seems to only be used with an AccessShareLock (like pg_prewarm). My best
guess from reading lots of code, commit messages, and old e-mails in the
archives is that the original check-privileges-before-locking work was
never completed.
I'm currently leaning towards continuing with v4 of the patch set. 0001
and 0003 are a little weird in that a concurrent change could lead to a
"could not find parent table" ERROR, but IIUC that is an extremely remote
possibility.
--
nathan
From df9b604c0d8c3c90df2026406e6ec0302f22454c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Thu, 9 Oct 2025 15:49:18 -0500
Subject: [PATCH 1/1] pg_prewarm privilege test
---
contrib/pg_prewarm/pg_prewarm.c | 83 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 36 +++++++++++++-
2 files changed, 113 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..57b720cfbed 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,13 +16,17 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
+#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
@@ -42,6 +46,74 @@ typedef enum
static PGIOAlignedBlock blockbuffer;
+static void
+check_privs_and_lock_rel(Oid relid, Oid *heaprel)
+{
+ uint64 inval_count;
+ Oid privOid = InvalidOid;
+
+ do
+ {
+ AclResult aclresult;
+ bool is_missing = false;
+ char relkind;
+
+ /*
+ * Remember this value so that, after locking the relation(s), we can
+ * check whether any invalidation messages have been processed that
+ * might require a do-over.
+ */
+ inval_count = SharedInvalidMessageCounter;
+
+ /*
+ * Unlock any previously-locked relations. We could try to hold onto
+ * these through iterations, but it's simpler to just start fresh each
+ * time.
+ */
+ if (OidIsValid(privOid))
+ {
+ if (privOid != relid)
+ UnlockRelationOid(privOid, AccessShareLock);
+ UnlockRelationOid(relid, AccessShareLock);
+ }
+
+ /*
+ * For indexes, privilege checks must happen on the heap relation.
+ */
+ relkind = get_rel_relkind(relid);
+ if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
+ privOid = IndexGetRelation(relid, false);
+ else
+ privOid = relid;
+
+ /*
+ * Check privileges. If the relation has gone missing, there's
+ * nothing for us to do. We'll either retry in the next loop
+ * iteration, or the caller will fail when it tries to open the
+ * relation.
+ */
+ aclresult = pg_class_aclcheck_ext(privOid, GetUserId(),
+ ACL_SELECT, &is_missing);
+ if (!is_missing && aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(relkind),
+ get_rel_name(relid));
+
+ /*
+ * Lock the relation(s). If relid is an index, make sure we lock its
+ * heap first. Note that we rely on LockRelationOid() to call
+ * AcceptInvalidationMessages().
+ */
+ if (privOid != relid)
+ LockRelationOid(privOid, AccessShareLock);
+ LockRelationOid(relid, AccessShareLock);
+
+ } while (inval_count != SharedInvalidMessageCounter);
+
+ if (privOid != relid)
+ *heaprel = privOid;
+}
+
/*
* pg_prewarm(regclass, mode text, fork text,
* first_block int8, last_block int8)
@@ -70,7 +142,7 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *forkString;
char *ttype;
PrewarmType ptype;
- AclResult aclresult;
+ Oid heaprel = InvalidOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,10 +179,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
- rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ check_privs_and_lock_rel(relOid, &heaprel);
+ rel = relation_open(relOid, NoLock);
/* Check that the relation has storage. */
if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
@@ -236,5 +306,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
/* Close relation, release lock. */
relation_close(rel, AccessShareLock);
+ if (OidIsValid(heaprel))
+ UnlockRelationOid(heaprel, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..5d7010eb44e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,38 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
+# prewarm fails for nonexistent table
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm(0);",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /could not open relation with OID 0/, 'pg_prewarm failed as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] 0001-pg_prewarm-privilege-test.patch (6.3K, 2-0001-pg_prewarm-privilege-test.patch)
download | inline diff:
From df9b604c0d8c3c90df2026406e6ec0302f22454c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Thu, 9 Oct 2025 15:49:18 -0500
Subject: [PATCH 1/1] pg_prewarm privilege test
---
contrib/pg_prewarm/pg_prewarm.c | 83 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 36 +++++++++++++-
2 files changed, 113 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..57b720cfbed 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,13 +16,17 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
+#include "storage/sinval.h"
#include "storage/smgr.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
@@ -42,6 +46,74 @@ typedef enum
static PGIOAlignedBlock blockbuffer;
+static void
+check_privs_and_lock_rel(Oid relid, Oid *heaprel)
+{
+ uint64 inval_count;
+ Oid privOid = InvalidOid;
+
+ do
+ {
+ AclResult aclresult;
+ bool is_missing = false;
+ char relkind;
+
+ /*
+ * Remember this value so that, after locking the relation(s), we can
+ * check whether any invalidation messages have been processed that
+ * might require a do-over.
+ */
+ inval_count = SharedInvalidMessageCounter;
+
+ /*
+ * Unlock any previously-locked relations. We could try to hold onto
+ * these through iterations, but it's simpler to just start fresh each
+ * time.
+ */
+ if (OidIsValid(privOid))
+ {
+ if (privOid != relid)
+ UnlockRelationOid(privOid, AccessShareLock);
+ UnlockRelationOid(relid, AccessShareLock);
+ }
+
+ /*
+ * For indexes, privilege checks must happen on the heap relation.
+ */
+ relkind = get_rel_relkind(relid);
+ if (relkind == RELKIND_INDEX || relkind == RELKIND_PARTITIONED_INDEX)
+ privOid = IndexGetRelation(relid, false);
+ else
+ privOid = relid;
+
+ /*
+ * Check privileges. If the relation has gone missing, there's
+ * nothing for us to do. We'll either retry in the next loop
+ * iteration, or the caller will fail when it tries to open the
+ * relation.
+ */
+ aclresult = pg_class_aclcheck_ext(privOid, GetUserId(),
+ ACL_SELECT, &is_missing);
+ if (!is_missing && aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(relkind),
+ get_rel_name(relid));
+
+ /*
+ * Lock the relation(s). If relid is an index, make sure we lock its
+ * heap first. Note that we rely on LockRelationOid() to call
+ * AcceptInvalidationMessages().
+ */
+ if (privOid != relid)
+ LockRelationOid(privOid, AccessShareLock);
+ LockRelationOid(relid, AccessShareLock);
+
+ } while (inval_count != SharedInvalidMessageCounter);
+
+ if (privOid != relid)
+ *heaprel = privOid;
+}
+
/*
* pg_prewarm(regclass, mode text, fork text,
* first_block int8, last_block int8)
@@ -70,7 +142,7 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *forkString;
char *ttype;
PrewarmType ptype;
- AclResult aclresult;
+ Oid heaprel = InvalidOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,10 +179,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
- rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ check_privs_and_lock_rel(relOid, &heaprel);
+ rel = relation_open(relOid, NoLock);
/* Check that the relation has storage. */
if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
@@ -236,5 +306,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
/* Close relation, release lock. */
relation_close(rel, AccessShareLock);
+ if (OidIsValid(heaprel))
+ UnlockRelationOid(heaprel, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..5d7010eb44e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,38 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
+# prewarm fails for nonexistent table
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm(0);",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /could not open relation with OID 0/, 'pg_prewarm failed as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-10 16:26 ` Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2025-10-10 16:26 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Oct 09, 2025 at 04:18:03PM -0500, Nathan Bossart wrote:
> There's a similar pattern in get_rel_from_relname() in dblink.c, which also
> seems to only be used with an AccessShareLock (like pg_prewarm). My best
> guess from reading lots of code, commit messages, and old e-mails in the
> archives is that the original check-privileges-before-locking work was
> never completed.
I added an 0004 that changes dblink to use RangeVarGetRelidExtended().
> I'm currently leaning towards continuing with v4 of the patch set. 0001
> and 0003 are a little weird in that a concurrent change could lead to a
> "could not find parent table" ERROR, but IIUC that is an extremely remote
> possibility.
After sleeping on it, I still think this is the right call. In any case,
I've spent way too much time on this stuff, so I plan to commit the
attached soon.
--
nathan
From 252991d120be8b50c5c3898deee8a723e3c02c02 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v5 1/4] fix priv checks in stats code
---
src/backend/statistics/stat_utils.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..8b8203a58e3 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -189,6 +189,17 @@ stats_lock_check_privileges(Oid reloid)
Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+ /*
+ * Since we did the IndexGetRelation() call above without any lock,
+ * it's barely possible that a race against an index drop/recreation
+ * could have netted us the wrong table.
+ */
+ if (table_oid != IndexGetRelation(index_oid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(index))));
+
/* retain lock on index */
relation_close(index, NoLock);
}
--
2.39.5 (Apple Git-154)
From 700ef73e8c373c57ab1b502d04176b4445442fb1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v5 2/4] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 41 ++++++++++++--------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..2a6e58eb0de 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2986,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
table_lockmode = (state->params.options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock;
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
+ /* Unlock any previously locked heap. */
+ if (OidIsValid(state->locked_table_oid))
{
UnlockRelationOid(state->locked_table_oid, table_lockmode);
state->locked_table_oid = InvalidOid;
@@ -3014,30 +3011,22 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
+ /*
+ * If the OID isn't valid, it means the index was concurrently dropped,
+ * which is not a problem for us; just return normally.
+ */
table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
+ if (!OidIsValid(table_oid))
+ return;
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ /* Check permissions */
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
- if (relId != oldRelId)
- {
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
}
/*
--
2.39.5 (Apple Git-154)
From d197dec65e1a41c3f0a74b256eb7f9a2ed7c00c1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v5 3/4] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 34 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 +++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..810a291204b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,31 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, false);
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table.
+ */
+ if (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +260,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
From 8a096e9bf7154df9393f6920579996d7dfe8958e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 Oct 2025 09:37:51 -0500
Subject: [PATCH v5 4/4] avoid locking before privilege checks in dblink
---
contrib/dblink/dblink.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0cf4c27f2e9..1e7696beb50 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2460,6 +2460,21 @@ get_tuple_of_interest(Relation rel, int *pkattnums, int pknumatts, char **src_pk
return NULL;
}
+static void
+RangeVarCallbackForDblink(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
+{
+ AclResult aclresult;
+
+ if (!OidIsValid(relId))
+ return;
+
+ aclresult = pg_class_aclcheck(relId, GetUserId(), *((AclMode *) arg));
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relId)),
+ relation->relname);
+}
+
/*
* Open the relation named by relname_text, acquire specified type of lock,
* verify we have specified permissions.
@@ -2469,19 +2484,13 @@ static Relation
get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode)
{
RangeVar *relvar;
- Relation rel;
- AclResult aclresult;
+ Oid relid;
relvar = makeRangeVarFromNameList(textToQualifiedNameList(relname_text));
- rel = table_openrv(relvar, lockmode);
-
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
- aclmode);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
+ relid = RangeVarGetRelidExtended(relvar, lockmode, 0,
+ RangeVarCallbackForDblink, &aclmode);
- return rel;
+ return table_open(relid, NoLock);
}
/*
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] v5-0001-fix-priv-checks-in-stats-code.patch (1.1K, 2-v5-0001-fix-priv-checks-in-stats-code.patch)
download | inline diff:
From 252991d120be8b50c5c3898deee8a723e3c02c02 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v5 1/4] fix priv checks in stats code
---
src/backend/statistics/stat_utils.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..8b8203a58e3 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -189,6 +189,17 @@ stats_lock_check_privileges(Oid reloid)
Assert(index->rd_index && index->rd_index->indrelid == table_oid);
+ /*
+ * Since we did the IndexGetRelation() call above without any lock,
+ * it's barely possible that a race against an index drop/recreation
+ * could have netted us the wrong table.
+ */
+ if (table_oid != IndexGetRelation(index_oid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(index))));
+
/* retain lock on index */
relation_close(index, NoLock);
}
--
2.39.5 (Apple Git-154)
[text/plain] v5-0002-fix-priv-checks-in-index-code.patch (2.7K, 3-v5-0002-fix-priv-checks-in-index-code.patch)
download | inline diff:
From 700ef73e8c373c57ab1b502d04176b4445442fb1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v5 2/4] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 41 ++++++++++++--------------------
1 file changed, 15 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..2a6e58eb0de 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2986,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
table_lockmode = (state->params.options & REINDEXOPT_CONCURRENTLY) != 0 ?
ShareUpdateExclusiveLock : ShareLock;
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
+ /* Unlock any previously locked heap. */
+ if (OidIsValid(state->locked_table_oid))
{
UnlockRelationOid(state->locked_table_oid, table_lockmode);
state->locked_table_oid = InvalidOid;
@@ -3014,30 +3011,22 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
+ /*
+ * If the OID isn't valid, it means the index was concurrently dropped,
+ * which is not a problem for us; just return normally.
+ */
table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
+ if (!OidIsValid(table_oid))
+ return;
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ /* Check permissions */
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
- if (relId != oldRelId)
- {
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
}
/*
--
2.39.5 (Apple Git-154)
[text/plain] v5-0003-fix-priv-checks-in-pg_prewarm.patch (4.4K, 4-v5-0003-fix-priv-checks-in-pg_prewarm.patch)
download | inline diff:
From d197dec65e1a41c3f0a74b256eb7f9a2ed7c00c1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v5 3/4] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 34 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 +++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..810a291204b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,31 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, false);
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table.
+ */
+ if (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +260,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
[text/plain] v5-0004-avoid-locking-before-privilege-checks-in-dblink.patch (1.8K, 5-v5-0004-avoid-locking-before-privilege-checks-in-dblink.patch)
download | inline diff:
From 8a096e9bf7154df9393f6920579996d7dfe8958e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 Oct 2025 09:37:51 -0500
Subject: [PATCH v5 4/4] avoid locking before privilege checks in dblink
---
contrib/dblink/dblink.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0cf4c27f2e9..1e7696beb50 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2460,6 +2460,21 @@ get_tuple_of_interest(Relation rel, int *pkattnums, int pknumatts, char **src_pk
return NULL;
}
+static void
+RangeVarCallbackForDblink(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
+{
+ AclResult aclresult;
+
+ if (!OidIsValid(relId))
+ return;
+
+ aclresult = pg_class_aclcheck(relId, GetUserId(), *((AclMode *) arg));
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relId)),
+ relation->relname);
+}
+
/*
* Open the relation named by relname_text, acquire specified type of lock,
* verify we have specified permissions.
@@ -2469,19 +2484,13 @@ static Relation
get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode)
{
RangeVar *relvar;
- Relation rel;
- AclResult aclresult;
+ Oid relid;
relvar = makeRangeVarFromNameList(textToQualifiedNameList(relname_text));
- rel = table_openrv(relvar, lockmode);
-
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
- aclmode);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
+ relid = RangeVarGetRelidExtended(relvar, lockmode, 0,
+ RangeVarCallbackForDblink, &aclmode);
- return rel;
+ return table_open(relid, NoLock);
}
/*
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-10 18:31 ` Jeff Davis <[email protected]>
2025-10-13 19:30 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Davis @ 2025-10-10 18:31 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, 2025-10-10 at 11:26 -0500, Nathan Bossart wrote:
> On Thu, Oct 09, 2025 at 04:18:03PM -0500, Nathan Bossart wrote:
> > There's a similar pattern in get_rel_from_relname() in dblink.c,
> > which also
> > seems to only be used with an AccessShareLock (like pg_prewarm).
> > My best
> > guess from reading lots of code, commit messages, and old e-mails
> > in the
> > archives is that the original check-privileges-before-locking work
> > was
> > never completed.
Interesting, thank you for the analysis.
> > I'm currently leaning towards continuing with v4 of the patch set.
> > 0001
> > and 0003 are a little weird in that a concurrent change could lead
> > to a
> > "could not find parent table" ERROR, but IIUC that is an extremely
> > remote
> > possibility.
>
> After sleeping on it, I still think this is the right call. In any
> case,
> I've spent way too much time on this stuff, so I plan to commit the
> attached soon.
I'm OK with that. v5-0001 is an improvement over the current situation.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
@ 2025-10-13 19:30 ` Nathan Bossart <[email protected]>
2025-10-14 02:23 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2025-10-13 19:30 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Oct 10, 2025 at 11:31:03AM -0700, Jeff Davis wrote:
> On Fri, 2025-10-10 at 11:26 -0500, Nathan Bossart wrote:
>> After sleeping on it, I still think this is the right call. In any
>> case, I've spent way too much time on this stuff, so I plan to commit
>> the attached soon.
>
> I'm OK with that. v5-0001 is an improvement over the current situation.
Okay, I lied. I spent even more time on these patches and came up with the
attached. Here's a summary of what's going on:
* 0001 moves the code for stats clearing/setting to use
RangeVarGetRelidExtended(). The existing code looks up the relation, locks
it, and then checks permissions. There's no guarantee that the relation
you looked up didn't concurrently change before locking, and locking before
privilege checks is troublesome from a DOS perspective. One downside of
using RangeVarGetRelidExtended() is that we can't use AccessShareLock for
regular indexes, but I'm not sure that's really a problem since we're
already using ShareUpdateExclusiveLock for everything else. The
RangeVarGetRelidExtended() callback is similar to the one modified by 0002.
This should be back-patched to v18.
* 0002 fixes the RangeVarGetRelidExtended() callback for REINDEX INDEX to
handle unlikely scenarios involving OID reuse (e.g., lookup returns the
same index OID for a different table). I did confirm there was a bug here
by concurrently re-creating an index with the same OID for a heap with a
different OID (via the pg_upgrade support functions). In previous versions
of this patch, I tried to fix this by unconditionally unlocking the heap at
the beginning of the callback, but upon further inspection, I noticed that
creates deadlock hazards because we might've already locked the index. (We
need to lock the heap first.) In v6, I've just added an ERROR for these
extremely unlikely scenarios. I've also replaced all early returns in this
function with ERRORs (except for the invalid relId case). AFAICT the extra
checks are unecessary, and even if they were necessary, I think they break
some of the code related to heap locking in subtle ways. Some callbacks do
these extra checks, and others do not, and AFAIK there haven't been any
reported problems either way. 0002 should be back-patched to v13, but it
will look a little different on v16 and newer, i.e., before MAINTAIN was
added.
* 0003 fixes the privilege checks in pg_prewarm by using a similar approach
to amcheck_lock_relation_and_check(). This seems correct to me, but it
does add more locking. This should be back-patched to v13.
* 0004 is a small patch to teach dblink to use RangeVarGetRelidExtended().
I believe this code predates that function. I don't intend to back-patch
this one.
--
nathan
From a198750398236642f724a012ad52cdf6b29c9e6d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 Oct 2025 15:50:06 -0500
Subject: [PATCH v6 1/4] fix priv checks in stats code
---
src/backend/statistics/attribute_stats.c | 16 ++-
src/backend/statistics/relation_stats.c | 8 +-
src/backend/statistics/stat_utils.c | 154 +++++++++++----------
src/include/statistics/stat_utils.h | 5 +-
src/test/regress/expected/stats_import.out | 6 +-
5 files changed, 101 insertions(+), 88 deletions(-)
diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.c
index 1db6a7f784c..d827c9b29d7 100644
--- a/src/backend/statistics/attribute_stats.c
+++ b/src/backend/statistics/attribute_stats.c
@@ -19,8 +19,10 @@
#include "access/heapam.h"
#include "catalog/indexing.h"
+#include "catalog/namespace.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_operator.h"
+#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "statistics/statistics.h"
#include "statistics/stat_utils.h"
@@ -143,6 +145,7 @@ attribute_statistics_update(FunctionCallInfo fcinfo)
char *attname;
AttrNumber attnum;
bool inherited;
+ Oid locked_table_oid = InvalidOid;
Relation starel;
HeapTuple statup;
@@ -182,8 +185,6 @@ attribute_statistics_update(FunctionCallInfo fcinfo)
nspname = TextDatumGetCString(PG_GETARG_DATUM(ATTRELSCHEMA_ARG));
relname = TextDatumGetCString(PG_GETARG_DATUM(ATTRELNAME_ARG));
- reloid = stats_lookup_relid(nspname, relname);
-
if (RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
@@ -191,7 +192,9 @@ attribute_statistics_update(FunctionCallInfo fcinfo)
errhint("Statistics cannot be modified during recovery.")));
/* lock before looking up attribute */
- stats_lock_check_privileges(reloid);
+ reloid = RangeVarGetRelidExtended(makeRangeVar(nspname, relname, -1),
+ ShareUpdateExclusiveLock, 0,
+ RangeVarCallbackForStats, &locked_table_oid);
/* user can specify either attname or attnum, but not both */
if (!PG_ARGISNULL(ATTNAME_ARG))
@@ -917,6 +920,7 @@ pg_clear_attribute_stats(PG_FUNCTION_ARGS)
char *attname;
AttrNumber attnum;
bool inherited;
+ Oid locked_table_oid = InvalidOid;
stats_check_required_arg(fcinfo, cleararginfo, C_ATTRELSCHEMA_ARG);
stats_check_required_arg(fcinfo, cleararginfo, C_ATTRELNAME_ARG);
@@ -926,15 +930,15 @@ pg_clear_attribute_stats(PG_FUNCTION_ARGS)
nspname = TextDatumGetCString(PG_GETARG_DATUM(C_ATTRELSCHEMA_ARG));
relname = TextDatumGetCString(PG_GETARG_DATUM(C_ATTRELNAME_ARG));
- reloid = stats_lookup_relid(nspname, relname);
-
if (RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("recovery is in progress"),
errhint("Statistics cannot be modified during recovery.")));
- stats_lock_check_privileges(reloid);
+ reloid = RangeVarGetRelidExtended(makeRangeVar(nspname, relname, -1),
+ ShareUpdateExclusiveLock, 0,
+ RangeVarCallbackForStats, &locked_table_oid);
attname = TextDatumGetCString(PG_GETARG_DATUM(C_ATTNAME_ARG));
attnum = get_attnum(reloid, attname);
diff --git a/src/backend/statistics/relation_stats.c b/src/backend/statistics/relation_stats.c
index a59f0c519a4..95226dcd1e1 100644
--- a/src/backend/statistics/relation_stats.c
+++ b/src/backend/statistics/relation_stats.c
@@ -20,6 +20,7 @@
#include "access/heapam.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "nodes/makefuncs.h"
#include "statistics/stat_utils.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
@@ -82,6 +83,7 @@ relation_statistics_update(FunctionCallInfo fcinfo)
Datum values[4] = {0};
bool nulls[4] = {0};
int nreplaces = 0;
+ Oid locked_table_oid = InvalidOid;
stats_check_required_arg(fcinfo, relarginfo, RELSCHEMA_ARG);
stats_check_required_arg(fcinfo, relarginfo, RELNAME_ARG);
@@ -89,15 +91,15 @@ relation_statistics_update(FunctionCallInfo fcinfo)
nspname = TextDatumGetCString(PG_GETARG_DATUM(RELSCHEMA_ARG));
relname = TextDatumGetCString(PG_GETARG_DATUM(RELNAME_ARG));
- reloid = stats_lookup_relid(nspname, relname);
-
if (RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("recovery is in progress"),
errhint("Statistics cannot be modified during recovery.")));
- stats_lock_check_privileges(reloid);
+ reloid = RangeVarGetRelidExtended(makeRangeVar(nspname, relname, -1),
+ ShareUpdateExclusiveLock, 0,
+ RangeVarCallbackForStats, &locked_table_oid);
if (!PG_ARGISNULL(RELPAGES_ARG))
{
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..71e2f0f4f02 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -16,9 +16,11 @@
#include "postgres.h"
+#include "access/htup_details.h"
#include "access/relation.h"
#include "catalog/index.h"
#include "catalog/namespace.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_database.h"
#include "funcapi.h"
#include "miscadmin.h"
@@ -29,6 +31,7 @@
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/syscache.h"
/*
* Ensure that a given argument is not null.
@@ -119,53 +122,85 @@ stats_check_arg_pair(FunctionCallInfo fcinfo,
}
/*
- * Lock relation in ShareUpdateExclusive mode, check privileges, and close the
- * relation (but retain the lock).
- *
* A role has privileges to set statistics on the relation if any of the
* following are true:
* - the role owns the current database and the relation is not shared
* - the role has the MAINTAIN privilege on the relation
*/
void
-stats_lock_check_privileges(Oid reloid)
+RangeVarCallbackForStats(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
{
- Relation table;
- Oid table_oid = reloid;
- Oid index_oid = InvalidOid;
- LOCKMODE index_lockmode = NoLock;
+ Oid *locked_table_oid = (Oid *) arg;
+ Oid table_oid = relId;
+ HeapTuple tuple;
+ Form_pg_class form;
+ char relkind;
/*
- * For indexes, we follow the locking behavior in do_analyze_rel() and
- * check_lock_if_inplace_updateable_rel(), which is to lock the table
- * first in ShareUpdateExclusive mode and then the index in AccessShare
- * mode.
- *
- * Partitioned indexes are treated differently than normal indexes in
- * check_lock_if_inplace_updateable_rel(), so we take a
- * ShareUpdateExclusive lock on both the partitioned table and the
- * partitioned index.
+ * If we previously locked some other index's heap, and the name we're
+ * looking up no longer refers to that relation, release the now-useless
+ * lock.
*/
- switch (get_rel_relkind(reloid))
+ if (relId != oldRelId && OidIsValid(*locked_table_oid))
{
- case RELKIND_INDEX:
- index_oid = reloid;
- table_oid = IndexGetRelation(index_oid, false);
- index_lockmode = AccessShareLock;
- break;
- case RELKIND_PARTITIONED_INDEX:
- index_oid = reloid;
- table_oid = IndexGetRelation(index_oid, false);
- index_lockmode = ShareUpdateExclusiveLock;
- break;
- default:
- break;
+ UnlockRelationOid(*locked_table_oid, ShareUpdateExclusiveLock);
+ *locked_table_oid = InvalidOid;
+ }
+
+ /* If the relation does not exist, there's nothing more to do. */
+ if (!OidIsValid(relId))
+ return;
+
+ /*
+ * If the relation does exist, check whether it's an index. But note that
+ * the relation might have been dropped betewen the time we did the name
+ * lookup and now. In that case, there's nothing to do.
+ */
+ relkind = get_rel_relkind(relId);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ table_oid = IndexGetRelation(relId, false);
+
+ /*
+ * If retrying yields the same OID, there are a couple of unlikely
+ * scenarios we need to handle.
+ */
+ if (relId == oldRelId)
+ {
+ /*
+ * If a previous lookup found an index, but the current lookup did
+ * not, release the lock on the index's table.
+ */
+ if (table_oid == relId && OidIsValid(*locked_table_oid))
+ {
+ UnlockRelationOid(*locked_table_oid, ShareUpdateExclusiveLock);
+ *locked_table_oid = InvalidOid;
+ }
+
+ /*
+ * If the current lookup found an index but we did not previously lock
+ * the right table (or any table at all), fail.
+ * RangeVarGetRelidExtended() will have already locked the index in
+ * this case, and it won't retry again, so we can't lock the newly
+ * discovered table OID without risking deadlock. Also, while this
+ * corner case is indeed possible, it is extremely unlikely to happen
+ * in practice, so it's probably not worth any more effort than this.
+ */
+ if (table_oid != relId && table_oid != *locked_table_oid)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("index \"%s\" was concurrently dropped",
+ relation->relname)));
}
- table = relation_open(table_oid, ShareUpdateExclusiveLock);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for OID %u", table_oid);
+ form = (Form_pg_class) GETSTRUCT(tuple);
/* the relkinds that can be used with ANALYZE */
- switch (table->rd_rel->relkind)
+ switch (form->relkind)
{
case RELKIND_RELATION:
case RELKIND_MATVIEW:
@@ -176,65 +211,38 @@ stats_lock_check_privileges(Oid reloid)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot modify statistics for relation \"%s\"",
- RelationGetRelationName(table)),
- errdetail_relkind_not_supported(table->rd_rel->relkind)));
- }
-
- if (OidIsValid(index_oid))
- {
- Relation index;
-
- Assert(index_lockmode != NoLock);
- index = relation_open(index_oid, index_lockmode);
-
- Assert(index->rd_index && index->rd_index->indrelid == table_oid);
-
- /* retain lock on index */
- relation_close(index, NoLock);
+ NameStr(form->relname)),
+ errdetail_relkind_not_supported(form->relkind)));
}
- if (table->rd_rel->relisshared)
+ if (form->relisshared)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot modify statistics for shared relation")));
+ /* Check permissions */
if (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()))
{
- AclResult aclresult = pg_class_aclcheck(RelationGetRelid(table),
+ AclResult aclresult = pg_class_aclcheck(table_oid,
GetUserId(),
ACL_MAINTAIN);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult,
- get_relkind_objtype(table->rd_rel->relkind),
- NameStr(table->rd_rel->relname));
+ get_relkind_objtype(form->relkind),
+ NameStr(form->relname));
}
- /* retain lock on table */
- relation_close(table, NoLock);
-}
+ ReleaseSysCache(tuple);
-/*
- * Lookup relation oid from schema and relation name.
- */
-Oid
-stats_lookup_relid(const char *nspname, const char *relname)
-{
- Oid nspoid;
- Oid reloid;
-
- nspoid = LookupExplicitNamespace(nspname, false);
- reloid = get_relname_relid(relname, nspoid);
- if (!OidIsValid(reloid))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_TABLE),
- errmsg("relation \"%s.%s\" does not exist",
- nspname, relname)));
-
- return reloid;
+ /* Lock heap before index to avoid deadlock. */
+ if (relId != oldRelId && table_oid != relId)
+ {
+ LockRelationOid(table_oid, ShareUpdateExclusiveLock);
+ *locked_table_oid = table_oid;
+ }
}
-
/*
* Find the argument number for the given argument name, returning -1 if not
* found.
diff --git a/src/include/statistics/stat_utils.h b/src/include/statistics/stat_utils.h
index 512eb776e0e..a8789407e53 100644
--- a/src/include/statistics/stat_utils.h
+++ b/src/include/statistics/stat_utils.h
@@ -30,9 +30,8 @@ extern bool stats_check_arg_pair(FunctionCallInfo fcinfo,
struct StatsArgInfo *arginfo,
int argnum1, int argnum2);
-extern void stats_lock_check_privileges(Oid reloid);
-
-extern Oid stats_lookup_relid(const char *nspname, const char *relname);
+extern void RangeVarCallbackForStats(const RangeVar *relation,
+ Oid relId, Oid oldRelid, void *arg);
extern bool stats_fill_fcinfo_from_arg_pairs(FunctionCallInfo pairs_fcinfo,
FunctionCallInfo positional_fcinfo,
diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out
index 9e615ccd0af..98ce7dc2841 100644
--- a/src/test/regress/expected/stats_import.out
+++ b/src/test/regress/expected/stats_import.out
@@ -120,9 +120,9 @@ WHERE relation = 'stats_import.test'::regclass AND
SELECT mode FROM pg_locks
WHERE relation = 'stats_import.test_i'::regclass AND
pid = pg_backend_pid() AND granted;
- mode
------------------
- AccessShareLock
+ mode
+--------------------------
+ ShareUpdateExclusiveLock
(1 row)
COMMIT;
--
2.39.5 (Apple Git-154)
From 6915f1ec62b184974d3f1fdbf9ec9ef65636347e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Mon, 13 Oct 2025 11:12:50 -0500
Subject: [PATCH v6 2/4] fix reindex index rangevar callback
---
src/backend/commands/indexcmds.c | 44 +++++++++++++++++---------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..5a42be0cabe 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
/*
* Lock level here should match table lock in reindex_index() for
@@ -3006,37 +3007,40 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* lookup and now. In that case, there's nothing to do.
*/
relkind = get_rel_relkind(relId);
- if (!relkind)
- return;
if (relkind != RELKIND_INDEX &&
relkind != RELKIND_PARTITIONED_INDEX)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
- table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
+ /* Look up the index's table. */
+ table_oid = IndexGetRelation(relId, false);
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ /*
+ * In the unlikely event that, upon retry, we get the same index OID with
+ * a different table OID, fail. RangeVarGetRelidExtended() will have
+ * already locked the index in this case, and it won't retry again, so we
+ * can't lock the newly discovered table OID without risking deadlock.
+ * Also, while this corner case is indeed possible, it is extremely
+ * unlikely to happen in practice, so it's probably not worth any more
+ * effort than this.
+ */
+ if (relId == oldRelId && table_oid != state->locked_table_oid)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("index \"%s\" was concurrently dropped",
+ relation->relname)));
+
+ /* Check permissions */
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
if (relId != oldRelId)
{
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
}
}
--
2.39.5 (Apple Git-154)
From 7b499fdddc29114fc2eb6065ab8f5912c3f4c5d0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v6 3/4] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 39 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 ++++++++++++++++++++++-
2 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..eff1f40bfd0 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,36 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, true);
+ if (OidIsValid(privOid))
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table. It's possible that such a race would change the outcome of
+ * get_rel_relkind(), too, but the worst case scenario is that we'll check
+ * privileges on an index instead of its parent table, which isn't too
+ * terrible.
+ */
+ if (!OidIsValid(privOid) ||
+ (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true)))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +265,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
From 5127522b53a9fa90af18bcd93365c25ebdab211a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 Oct 2025 09:37:51 -0500
Subject: [PATCH v6 4/4] avoid locking before privilege checks in dblink
---
contrib/dblink/dblink.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0cf4c27f2e9..1e7696beb50 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2460,6 +2460,21 @@ get_tuple_of_interest(Relation rel, int *pkattnums, int pknumatts, char **src_pk
return NULL;
}
+static void
+RangeVarCallbackForDblink(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
+{
+ AclResult aclresult;
+
+ if (!OidIsValid(relId))
+ return;
+
+ aclresult = pg_class_aclcheck(relId, GetUserId(), *((AclMode *) arg));
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relId)),
+ relation->relname);
+}
+
/*
* Open the relation named by relname_text, acquire specified type of lock,
* verify we have specified permissions.
@@ -2469,19 +2484,13 @@ static Relation
get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode)
{
RangeVar *relvar;
- Relation rel;
- AclResult aclresult;
+ Oid relid;
relvar = makeRangeVarFromNameList(textToQualifiedNameList(relname_text));
- rel = table_openrv(relvar, lockmode);
-
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
- aclmode);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
+ relid = RangeVarGetRelidExtended(relvar, lockmode, 0,
+ RangeVarCallbackForDblink, &aclmode);
- return rel;
+ return table_open(relid, NoLock);
}
/*
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] v6-0001-fix-priv-checks-in-stats-code.patch (12.8K, 2-v6-0001-fix-priv-checks-in-stats-code.patch)
download | inline diff:
From a198750398236642f724a012ad52cdf6b29c9e6d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 Oct 2025 15:50:06 -0500
Subject: [PATCH v6 1/4] fix priv checks in stats code
---
src/backend/statistics/attribute_stats.c | 16 ++-
src/backend/statistics/relation_stats.c | 8 +-
src/backend/statistics/stat_utils.c | 154 +++++++++++----------
src/include/statistics/stat_utils.h | 5 +-
src/test/regress/expected/stats_import.out | 6 +-
5 files changed, 101 insertions(+), 88 deletions(-)
diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.c
index 1db6a7f784c..d827c9b29d7 100644
--- a/src/backend/statistics/attribute_stats.c
+++ b/src/backend/statistics/attribute_stats.c
@@ -19,8 +19,10 @@
#include "access/heapam.h"
#include "catalog/indexing.h"
+#include "catalog/namespace.h"
#include "catalog/pg_collation.h"
#include "catalog/pg_operator.h"
+#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
#include "statistics/statistics.h"
#include "statistics/stat_utils.h"
@@ -143,6 +145,7 @@ attribute_statistics_update(FunctionCallInfo fcinfo)
char *attname;
AttrNumber attnum;
bool inherited;
+ Oid locked_table_oid = InvalidOid;
Relation starel;
HeapTuple statup;
@@ -182,8 +185,6 @@ attribute_statistics_update(FunctionCallInfo fcinfo)
nspname = TextDatumGetCString(PG_GETARG_DATUM(ATTRELSCHEMA_ARG));
relname = TextDatumGetCString(PG_GETARG_DATUM(ATTRELNAME_ARG));
- reloid = stats_lookup_relid(nspname, relname);
-
if (RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
@@ -191,7 +192,9 @@ attribute_statistics_update(FunctionCallInfo fcinfo)
errhint("Statistics cannot be modified during recovery.")));
/* lock before looking up attribute */
- stats_lock_check_privileges(reloid);
+ reloid = RangeVarGetRelidExtended(makeRangeVar(nspname, relname, -1),
+ ShareUpdateExclusiveLock, 0,
+ RangeVarCallbackForStats, &locked_table_oid);
/* user can specify either attname or attnum, but not both */
if (!PG_ARGISNULL(ATTNAME_ARG))
@@ -917,6 +920,7 @@ pg_clear_attribute_stats(PG_FUNCTION_ARGS)
char *attname;
AttrNumber attnum;
bool inherited;
+ Oid locked_table_oid = InvalidOid;
stats_check_required_arg(fcinfo, cleararginfo, C_ATTRELSCHEMA_ARG);
stats_check_required_arg(fcinfo, cleararginfo, C_ATTRELNAME_ARG);
@@ -926,15 +930,15 @@ pg_clear_attribute_stats(PG_FUNCTION_ARGS)
nspname = TextDatumGetCString(PG_GETARG_DATUM(C_ATTRELSCHEMA_ARG));
relname = TextDatumGetCString(PG_GETARG_DATUM(C_ATTRELNAME_ARG));
- reloid = stats_lookup_relid(nspname, relname);
-
if (RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("recovery is in progress"),
errhint("Statistics cannot be modified during recovery.")));
- stats_lock_check_privileges(reloid);
+ reloid = RangeVarGetRelidExtended(makeRangeVar(nspname, relname, -1),
+ ShareUpdateExclusiveLock, 0,
+ RangeVarCallbackForStats, &locked_table_oid);
attname = TextDatumGetCString(PG_GETARG_DATUM(C_ATTNAME_ARG));
attnum = get_attnum(reloid, attname);
diff --git a/src/backend/statistics/relation_stats.c b/src/backend/statistics/relation_stats.c
index a59f0c519a4..95226dcd1e1 100644
--- a/src/backend/statistics/relation_stats.c
+++ b/src/backend/statistics/relation_stats.c
@@ -20,6 +20,7 @@
#include "access/heapam.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
+#include "nodes/makefuncs.h"
#include "statistics/stat_utils.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
@@ -82,6 +83,7 @@ relation_statistics_update(FunctionCallInfo fcinfo)
Datum values[4] = {0};
bool nulls[4] = {0};
int nreplaces = 0;
+ Oid locked_table_oid = InvalidOid;
stats_check_required_arg(fcinfo, relarginfo, RELSCHEMA_ARG);
stats_check_required_arg(fcinfo, relarginfo, RELNAME_ARG);
@@ -89,15 +91,15 @@ relation_statistics_update(FunctionCallInfo fcinfo)
nspname = TextDatumGetCString(PG_GETARG_DATUM(RELSCHEMA_ARG));
relname = TextDatumGetCString(PG_GETARG_DATUM(RELNAME_ARG));
- reloid = stats_lookup_relid(nspname, relname);
-
if (RecoveryInProgress())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("recovery is in progress"),
errhint("Statistics cannot be modified during recovery.")));
- stats_lock_check_privileges(reloid);
+ reloid = RangeVarGetRelidExtended(makeRangeVar(nspname, relname, -1),
+ ShareUpdateExclusiveLock, 0,
+ RangeVarCallbackForStats, &locked_table_oid);
if (!PG_ARGISNULL(RELPAGES_ARG))
{
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.c
index ef7e5168bed..71e2f0f4f02 100644
--- a/src/backend/statistics/stat_utils.c
+++ b/src/backend/statistics/stat_utils.c
@@ -16,9 +16,11 @@
#include "postgres.h"
+#include "access/htup_details.h"
#include "access/relation.h"
#include "catalog/index.h"
#include "catalog/namespace.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_database.h"
#include "funcapi.h"
#include "miscadmin.h"
@@ -29,6 +31,7 @@
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
+#include "utils/syscache.h"
/*
* Ensure that a given argument is not null.
@@ -119,53 +122,85 @@ stats_check_arg_pair(FunctionCallInfo fcinfo,
}
/*
- * Lock relation in ShareUpdateExclusive mode, check privileges, and close the
- * relation (but retain the lock).
- *
* A role has privileges to set statistics on the relation if any of the
* following are true:
* - the role owns the current database and the relation is not shared
* - the role has the MAINTAIN privilege on the relation
*/
void
-stats_lock_check_privileges(Oid reloid)
+RangeVarCallbackForStats(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
{
- Relation table;
- Oid table_oid = reloid;
- Oid index_oid = InvalidOid;
- LOCKMODE index_lockmode = NoLock;
+ Oid *locked_table_oid = (Oid *) arg;
+ Oid table_oid = relId;
+ HeapTuple tuple;
+ Form_pg_class form;
+ char relkind;
/*
- * For indexes, we follow the locking behavior in do_analyze_rel() and
- * check_lock_if_inplace_updateable_rel(), which is to lock the table
- * first in ShareUpdateExclusive mode and then the index in AccessShare
- * mode.
- *
- * Partitioned indexes are treated differently than normal indexes in
- * check_lock_if_inplace_updateable_rel(), so we take a
- * ShareUpdateExclusive lock on both the partitioned table and the
- * partitioned index.
+ * If we previously locked some other index's heap, and the name we're
+ * looking up no longer refers to that relation, release the now-useless
+ * lock.
*/
- switch (get_rel_relkind(reloid))
+ if (relId != oldRelId && OidIsValid(*locked_table_oid))
{
- case RELKIND_INDEX:
- index_oid = reloid;
- table_oid = IndexGetRelation(index_oid, false);
- index_lockmode = AccessShareLock;
- break;
- case RELKIND_PARTITIONED_INDEX:
- index_oid = reloid;
- table_oid = IndexGetRelation(index_oid, false);
- index_lockmode = ShareUpdateExclusiveLock;
- break;
- default:
- break;
+ UnlockRelationOid(*locked_table_oid, ShareUpdateExclusiveLock);
+ *locked_table_oid = InvalidOid;
+ }
+
+ /* If the relation does not exist, there's nothing more to do. */
+ if (!OidIsValid(relId))
+ return;
+
+ /*
+ * If the relation does exist, check whether it's an index. But note that
+ * the relation might have been dropped betewen the time we did the name
+ * lookup and now. In that case, there's nothing to do.
+ */
+ relkind = get_rel_relkind(relId);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ table_oid = IndexGetRelation(relId, false);
+
+ /*
+ * If retrying yields the same OID, there are a couple of unlikely
+ * scenarios we need to handle.
+ */
+ if (relId == oldRelId)
+ {
+ /*
+ * If a previous lookup found an index, but the current lookup did
+ * not, release the lock on the index's table.
+ */
+ if (table_oid == relId && OidIsValid(*locked_table_oid))
+ {
+ UnlockRelationOid(*locked_table_oid, ShareUpdateExclusiveLock);
+ *locked_table_oid = InvalidOid;
+ }
+
+ /*
+ * If the current lookup found an index but we did not previously lock
+ * the right table (or any table at all), fail.
+ * RangeVarGetRelidExtended() will have already locked the index in
+ * this case, and it won't retry again, so we can't lock the newly
+ * discovered table OID without risking deadlock. Also, while this
+ * corner case is indeed possible, it is extremely unlikely to happen
+ * in practice, so it's probably not worth any more effort than this.
+ */
+ if (table_oid != relId && table_oid != *locked_table_oid)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("index \"%s\" was concurrently dropped",
+ relation->relname)));
}
- table = relation_open(table_oid, ShareUpdateExclusiveLock);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for OID %u", table_oid);
+ form = (Form_pg_class) GETSTRUCT(tuple);
/* the relkinds that can be used with ANALYZE */
- switch (table->rd_rel->relkind)
+ switch (form->relkind)
{
case RELKIND_RELATION:
case RELKIND_MATVIEW:
@@ -176,65 +211,38 @@ stats_lock_check_privileges(Oid reloid)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot modify statistics for relation \"%s\"",
- RelationGetRelationName(table)),
- errdetail_relkind_not_supported(table->rd_rel->relkind)));
- }
-
- if (OidIsValid(index_oid))
- {
- Relation index;
-
- Assert(index_lockmode != NoLock);
- index = relation_open(index_oid, index_lockmode);
-
- Assert(index->rd_index && index->rd_index->indrelid == table_oid);
-
- /* retain lock on index */
- relation_close(index, NoLock);
+ NameStr(form->relname)),
+ errdetail_relkind_not_supported(form->relkind)));
}
- if (table->rd_rel->relisshared)
+ if (form->relisshared)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot modify statistics for shared relation")));
+ /* Check permissions */
if (!object_ownercheck(DatabaseRelationId, MyDatabaseId, GetUserId()))
{
- AclResult aclresult = pg_class_aclcheck(RelationGetRelid(table),
+ AclResult aclresult = pg_class_aclcheck(table_oid,
GetUserId(),
ACL_MAINTAIN);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult,
- get_relkind_objtype(table->rd_rel->relkind),
- NameStr(table->rd_rel->relname));
+ get_relkind_objtype(form->relkind),
+ NameStr(form->relname));
}
- /* retain lock on table */
- relation_close(table, NoLock);
-}
+ ReleaseSysCache(tuple);
-/*
- * Lookup relation oid from schema and relation name.
- */
-Oid
-stats_lookup_relid(const char *nspname, const char *relname)
-{
- Oid nspoid;
- Oid reloid;
-
- nspoid = LookupExplicitNamespace(nspname, false);
- reloid = get_relname_relid(relname, nspoid);
- if (!OidIsValid(reloid))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_TABLE),
- errmsg("relation \"%s.%s\" does not exist",
- nspname, relname)));
-
- return reloid;
+ /* Lock heap before index to avoid deadlock. */
+ if (relId != oldRelId && table_oid != relId)
+ {
+ LockRelationOid(table_oid, ShareUpdateExclusiveLock);
+ *locked_table_oid = table_oid;
+ }
}
-
/*
* Find the argument number for the given argument name, returning -1 if not
* found.
diff --git a/src/include/statistics/stat_utils.h b/src/include/statistics/stat_utils.h
index 512eb776e0e..a8789407e53 100644
--- a/src/include/statistics/stat_utils.h
+++ b/src/include/statistics/stat_utils.h
@@ -30,9 +30,8 @@ extern bool stats_check_arg_pair(FunctionCallInfo fcinfo,
struct StatsArgInfo *arginfo,
int argnum1, int argnum2);
-extern void stats_lock_check_privileges(Oid reloid);
-
-extern Oid stats_lookup_relid(const char *nspname, const char *relname);
+extern void RangeVarCallbackForStats(const RangeVar *relation,
+ Oid relId, Oid oldRelid, void *arg);
extern bool stats_fill_fcinfo_from_arg_pairs(FunctionCallInfo pairs_fcinfo,
FunctionCallInfo positional_fcinfo,
diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out
index 9e615ccd0af..98ce7dc2841 100644
--- a/src/test/regress/expected/stats_import.out
+++ b/src/test/regress/expected/stats_import.out
@@ -120,9 +120,9 @@ WHERE relation = 'stats_import.test'::regclass AND
SELECT mode FROM pg_locks
WHERE relation = 'stats_import.test_i'::regclass AND
pid = pg_backend_pid() AND granted;
- mode
------------------
- AccessShareLock
+ mode
+--------------------------
+ ShareUpdateExclusiveLock
(1 row)
COMMIT;
--
2.39.5 (Apple Git-154)
[text/plain] v6-0002-fix-reindex-index-rangevar-callback.patch (2.9K, 3-v6-0002-fix-reindex-index-rangevar-callback.patch)
download | inline diff:
From 6915f1ec62b184974d3f1fdbf9ec9ef65636347e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Mon, 13 Oct 2025 11:12:50 -0500
Subject: [PATCH v6 2/4] fix reindex index rangevar callback
---
src/backend/commands/indexcmds.c | 44 +++++++++++++++++---------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..5a42be0cabe 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
/*
* Lock level here should match table lock in reindex_index() for
@@ -3006,37 +3007,40 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* lookup and now. In that case, there's nothing to do.
*/
relkind = get_rel_relkind(relId);
- if (!relkind)
- return;
if (relkind != RELKIND_INDEX &&
relkind != RELKIND_PARTITIONED_INDEX)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not an index", relation->relname)));
- /* Check permissions */
- table_oid = IndexGetRelation(relId, true);
- if (OidIsValid(table_oid))
- {
- AclResult aclresult;
+ /* Look up the index's table. */
+ table_oid = IndexGetRelation(relId, false);
- aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
- }
+ /*
+ * In the unlikely event that, upon retry, we get the same index OID with
+ * a different table OID, fail. RangeVarGetRelidExtended() will have
+ * already locked the index in this case, and it won't retry again, so we
+ * can't lock the newly discovered table OID without risking deadlock.
+ * Also, while this corner case is indeed possible, it is extremely
+ * unlikely to happen in practice, so it's probably not worth any more
+ * effort than this.
+ */
+ if (relId == oldRelId && table_oid != state->locked_table_oid)
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("index \"%s\" was concurrently dropped",
+ relation->relname)));
+
+ /* Check permissions */
+ aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
/* Lock heap before index to avoid deadlock. */
if (relId != oldRelId)
{
- /*
- * If the OID isn't valid, it means the index was concurrently
- * dropped, which is not a problem for us; just return normally.
- */
- if (OidIsValid(table_oid))
- {
- LockRelationOid(table_oid, table_lockmode);
- state->locked_table_oid = table_oid;
- }
+ LockRelationOid(table_oid, table_lockmode);
+ state->locked_table_oid = table_oid;
}
}
--
2.39.5 (Apple Git-154)
[text/plain] v6-0003-fix-priv-checks-in-pg_prewarm.patch (4.7K, 4-v6-0003-fix-priv-checks-in-pg_prewarm.patch)
download | inline diff:
From 7b499fdddc29114fc2eb6065ab8f5912c3f4c5d0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v6 3/4] fix priv checks in pg_prewarm
---
contrib/pg_prewarm/pg_prewarm.c | 39 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 ++++++++++++++++++++++-
2 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index b968933ea8b..eff1f40bfd0 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,9 +16,11 @@
#include <unistd.h>
#include "access/relation.h"
+#include "catalog/index.h"
#include "fmgr.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/lmgr.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
#include "utils/acl.h"
@@ -71,6 +73,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
char *ttype;
PrewarmType ptype;
AclResult aclresult;
+ char relkind;
+ Oid privOid;
/* Basic sanity checking. */
if (PG_ARGISNULL(0))
@@ -107,8 +111,36 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkNumber = forkname_to_number(forkString);
/* Open relation and check privileges. */
+ relkind = get_rel_relkind(relOid);
+ if (relkind == RELKIND_INDEX ||
+ relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ privOid = IndexGetRelation(relOid, true);
+ if (OidIsValid(privOid))
+ LockRelationOid(privOid, AccessShareLock);
+ }
+ else
+ privOid = relOid;
+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);
+
+ /*
+ * If we did the IndexGetRelation() call above, it's barely possible that
+ * a race against an index drop/recreation could have netted us the wrong
+ * table. It's possible that such a race would change the outcome of
+ * get_rel_relkind(), too, but the worst case scenario is that we'll check
+ * privileges on an index instead of its parent table, which isn't too
+ * terrible.
+ */
+ if (!OidIsValid(privOid) ||
+ (privOid != relOid &&
+ privOid != IndexGetRelation(relOid, true)))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("could not find parent table of index \"%s\"",
+ RelationGetRelationName(rel))));
+
+ aclresult = pg_class_aclcheck(privOid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
@@ -233,8 +265,11 @@ pg_prewarm(PG_FUNCTION_ARGS)
read_stream_end(stream);
}
- /* Close relation, release lock. */
+ /* Close relation, release locks. */
relation_close(rel, AccessShareLock);
+ if (privOid != relOid)
+ UnlockRelationOid(privOid, AccessShareLock);
+
PG_RETURN_INT64(blocks_done);
}
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..a77ab67d29e 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,9 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE INDEX test_idx ON test(c1);\n"
+ . "CREATE ROLE test_user LOGIN;");
# test read mode
my $result =
@@ -42,6 +44,31 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test_user should be unable to prewarm table/index without privileges
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for table test/, 'pg_prewarm failed as expected');
+($cmdret, $stdout, $stderr) =
+ $node->psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+ok($stderr =~ /permission denied for index test_idx/, 'pg_prewarm failed as expected');
+
+# test_user should be able to prewarm table/index with privileges
+$node->safe_psql("postgres", "GRANT SELECT ON test TO test_user;");
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+$result =
+ $node->safe_psql(
+ "postgres", "SELECT pg_prewarm('test_idx');",
+ extra_params => [ '--username' => 'test_user' ]);
+like($result, qr/^[1-9][0-9]*$/, 'pg_prewarm succeeded as expected');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.39.5 (Apple Git-154)
[text/plain] v6-0004-avoid-locking-before-privilege-checks-in-dblink.patch (1.8K, 5-v6-0004-avoid-locking-before-privilege-checks-in-dblink.patch)
download | inline diff:
From 5127522b53a9fa90af18bcd93365c25ebdab211a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Fri, 10 Oct 2025 09:37:51 -0500
Subject: [PATCH v6 4/4] avoid locking before privilege checks in dblink
---
contrib/dblink/dblink.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0cf4c27f2e9..1e7696beb50 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2460,6 +2460,21 @@ get_tuple_of_interest(Relation rel, int *pkattnums, int pknumatts, char **src_pk
return NULL;
}
+static void
+RangeVarCallbackForDblink(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
+{
+ AclResult aclresult;
+
+ if (!OidIsValid(relId))
+ return;
+
+ aclresult = pg_class_aclcheck(relId, GetUserId(), *((AclMode *) arg));
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(relId)),
+ relation->relname);
+}
+
/*
* Open the relation named by relname_text, acquire specified type of lock,
* verify we have specified permissions.
@@ -2469,19 +2484,13 @@ static Relation
get_rel_from_relname(text *relname_text, LOCKMODE lockmode, AclMode aclmode)
{
RangeVar *relvar;
- Relation rel;
- AclResult aclresult;
+ Oid relid;
relvar = makeRangeVarFromNameList(textToQualifiedNameList(relname_text));
- rel = table_openrv(relvar, lockmode);
-
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
- aclmode);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
+ relid = RangeVarGetRelidExtended(relvar, lockmode, 0,
+ RangeVarCallbackForDblink, &aclmode);
- return rel;
+ return table_open(relid, NoLock);
}
/*
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-13 19:30 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-14 02:23 ` Jeff Davis <[email protected]>
2025-10-14 16:05 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Davis @ 2025-10-14 02:23 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; Corey Huinker <[email protected]>; +Cc: Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, 2025-10-13 at 14:30 -0500, Nathan Bossart wrote:
> * 0001 moves the code for stats clearing/setting to use
> RangeVarGetRelidExtended(). The existing code looks up the relation,
> locks
> it, and then checks permissions. There's no guarantee that the
> relation
> you looked up didn't concurrently change before locking, and locking
> before
> privilege checks is troublesome from a DOS perspective. One downside
> of
> using RangeVarGetRelidExtended() is that we can't use AccessShareLock
> for
> regular indexes, but I'm not sure that's really a problem since we're
> already using ShareUpdateExclusiveLock for everything else. The
> RangeVarGetRelidExtended() callback is similar to the one modified by
> 0002.
> This should be back-patched to v18.
We tried to match the locking behavior for analyze. Originally, that's
because we were using in-place updates, which required those specific
kinds of locks. Now that the in-place code is gone, then I think it's
OK to use ShareUpdateExclusiveLock for indexes, too, but it is a
notable difference in behavior.
Including Corey in case he has comments.
As for the patch itself, it looks good to me. Stylistically I might
have kept the "index_oid" variable, which makes some of the tests a bit
clearer, but I don't have a strong opinion.
The unlikely scenarios are a bit confusing. I'd probably error for
either case. Also, the error message on the second scenario is wrong if
the previous lookup was a table, I think.
> * 0002 fixes the RangeVarGetRelidExtended() callback for REINDEX
> INDEX to
> handle unlikely scenarios involving OID reuse (e.g., lookup returns
> the
> same index OID for a different table). I did confirm there was a bug
> here
> by concurrently re-creating an index with the same OID for a heap
> with a
> different OID (via the pg_upgrade support functions). In previous
> versions
> of this patch, I tried to fix this by unconditionally unlocking the
> heap at
> the beginning of the callback, but upon further inspection, I noticed
> that
> creates deadlock hazards because we might've already locked the
> index. (We
> need to lock the heap first.) In v6, I've just added an ERROR for
> these
> extremely unlikely scenarios. I've also replaced all early returns
> in this
> function with ERRORs (except for the invalid relId case).
+1 for throwing errors when we have race conditions combined with name
reuse. Looks fine to me.
>
> * 0003 fixes the privilege checks in pg_prewarm by using a similar
> approach
> to amcheck_lock_relation_and_check(). This seems correct to me, but
> it
> does add more locking. This should be back-patched to v13.
IIUC this is locking before the privilege check. Is there a reason why
we think this is OK here (and in amcheck_lock_relation_and_check()) but
not for the stats?
> * 0004 is a small patch to teach dblink to use
> RangeVarGetRelidExtended().
> I believe this code predates that function. I don't intend to back-
> patch
> this one.
Looks good.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-13 19:30 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-14 02:23 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
@ 2025-10-14 16:05 ` Nathan Bossart <[email protected]>
2025-10-14 17:01 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Nathan Bossart @ 2025-10-14 16:05 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
Thanks for reviewing.
On Mon, Oct 13, 2025 at 07:23:36PM -0700, Jeff Davis wrote:
> The unlikely scenarios are a bit confusing. I'd probably error for
> either case. Also, the error message on the second scenario is wrong if
> the previous lookup was a table, I think.
Yeah, I think that's a better idea.
> IIUC this is locking before the privilege check. Is there a reason why
> we think this is OK here (and in amcheck_lock_relation_and_check()) but
> not for the stats?
For amcheck, AFAICT there aren't actually any ACL checks within the code
because the function is restricted to superuser by default. For
pg_prewarm, I don't know. You do have to install the extension before
using it, but once installed, it's available to everyone by default. My
guess is that it just hasn't been a problem in the field.
Regardless, fixing the lock-before-privilege-checks behavior doesn't strike
me as a bug, so I think we ought to proceed with something like 0003 for
back-patching purposes and then to rework it further for v19. Does that
sound okay to you?
>> * 0004 is a small patch to teach dblink to use
>> RangeVarGetRelidExtended(). I believe this code predates that
>> function. I don't intend to back-patch this one.
>
> Looks good.
I'm going to go commit this one now to get it out of the way.
--
nathan
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Clarification on Role Access Rights to Table Indexes
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 03:28 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-09 15:39 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-13 19:30 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-14 02:23 ` Re: Clarification on Role Access Rights to Table Indexes Jeff Davis <[email protected]>
2025-10-14 16:05 ` Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
@ 2025-10-14 17:01 ` Jeff Davis <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Jeff Davis @ 2025-10-14 17:01 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Corey Huinker <[email protected]>; Tom Lane <[email protected]>; Ayush Vatsa <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; PostgreSQL Hackers <[email protected]>
On Tue, 2025-10-14 at 11:05 -0500, Nathan Bossart wrote:
> For
> pg_prewarm, I don't know. You do have to install the extension
> before
> using it, but once installed, it's available to everyone by default.
> My
> guess is that it just hasn't been a problem in the field.
If we start with an OID, what's the right way to do these kinds of
checks? Could we do an ACL check, then lock it, then do an ACL check
again to catch OID wraparound?
Last-minute suggestions on 0003:
* Add a comment around the privOid check to explain that, if the
object is an index, we must check the privileges on the table instead.
* Clarify in the comment that the race against index drop/recreation
involves OID wraparound.
+1 to the patch and backpatch.
As a separate thought, I'm wondering if we should do more to enforce
the idea that we check the privileges and owner of an index's table,
and never the index itself. That's for another discussion, though.
> Regardless, fixing the lock-before-privilege-checks behavior doesn't
> strike
> me as a bug, so I think we ought to proceed with something like 0003
> for
> back-patching purposes and then to rework it further for v19. Does
> that
> sound okay to you?
According to the current rules[1], it does seem to technically be a
bug, but as far as I can tell, not one of much consequence.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/[email protected]
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2025-10-14 17:01 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-09-24 16:52 Re: Clarification on Role Access Rights to Table Indexes Nathan Bossart <[email protected]>
2025-10-02 17:37 ` Nathan Bossart <[email protected]>
2025-10-09 03:06 ` Jeff Davis <[email protected]>
2025-10-09 03:28 ` Jeff Davis <[email protected]>
2025-10-09 15:39 ` Nathan Bossart <[email protected]>
2025-10-09 21:18 ` Nathan Bossart <[email protected]>
2025-10-10 16:26 ` Nathan Bossart <[email protected]>
2025-10-10 18:31 ` Jeff Davis <[email protected]>
2025-10-13 19:30 ` Nathan Bossart <[email protected]>
2025-10-14 02:23 ` Jeff Davis <[email protected]>
2025-10-14 16:05 ` Nathan Bossart <[email protected]>
2025-10-14 17:01 ` Jeff Davis <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox