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)
