On Mon, Mar 10, 2025 at 10:15:19AM -0500, Nathan Bossart wrote:
> On Sun, Mar 09, 2025 at 11:48:05AM -0400, Tom Lane wrote:
>> Nathan Bossart <nathandboss...@gmail.com> writes:
>>> On Sat, Mar 08, 2025 at 05:17:40PM -0500, Tom Lane wrote:
>>>> ReindexIndex() faces this same problem and solves it with some
>>>> very complex code that manages to get the table's lock first.
>>
>>> I noticed that amcheck's bt_index_check_internal() handles this problem,
>>> ...
>>> stats_lock_check_privileges() does something similar, but it's not as
>>> cautious about the "heapid != IndexGetRelation(indrelid, false)" race
>>> condition.
>>
>> Egad, we've already got three inconsistent implementations of this
>> functionality? I think the first step must be to unify them into
>> a common implementation, if at all possible.
>
> Agreed. I worry that trying to unify each bespoke implementation into a
> single function might result in an unwieldy mess, but I'll give it a
> shot...
I tried to unify these, but each one seems to be just different enough to
make it not worth the trouble. Instead, I took a look at each
implementation:
* amcheck's amcheck_lock_relation_and_check() seems correct to me.
* stats_lock_check_privileges() appears to be missing the second
IndexGetRelation() check after locking the table and index, so I added
that in 0001. Since this code is new to v18, I proposed to back-patch 0001
there.
* RangeVarCallbackForReindexIndex() was checking privileges on the table
before locking it, so I reversed it in 0002. Interestingly, this caused
test errors because LockRelationOid() checks for invalidation messages, so
the pg_class_aclcheck() call started failing with unhelpful errors due to
concurrently dropped relations. To deal with that, I switched to
pg_class_aclcheck_ext() so that we can handle missing relations.
Furthermore, I noticed that this callback seems to assume that as long as
the index does not change between calls, its table won't, either. That's
probably always true in practice, but even if it's completely true, I see
no reason to rely on it. So, I simplified the code to unconditionally
unlock any previously-locked table and to lock whatever IndexGetRelation()
returns. This could probably be back-patched, but in the absence of any
reports or any reproducible bugs, I don't think we should.
* 0003 fixes pg_prewarm's privilege checks by following a similar pattern.
This probably ought to get back-patched to all supported versions.
--
nathan
>From 0e70810f8ef47a0e4e22a70a5c9a2a7776611950 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Wed, 24 Sep 2025 09:24:28 -0500
Subject: [PATCH v3 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 28fa582f8372a6d73adb79f4d28fe997a08a23e0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Wed, 24 Sep 2025 09:24:36 -0500
Subject: [PATCH v3 2/3] fix priv checks in index code
---
src/backend/commands/indexcmds.c | 51 +++++++++++++++-----------------
1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82..e4d05b5f8e6 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
Oid table_oid;
+ AclResult aclresult;
+ bool is_missing = false;
/*
* Lock level here should match table lock in reindex_index() for
@@ -2985,12 +2987,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 +3012,29 @@ 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;
-
- aclresult = pg_class_aclcheck(table_oid, GetUserId(),
ACL_MAINTAIN);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, OBJECT_INDEX,
relation->relname);
- }
+ if (!OidIsValid(table_oid))
+ return;
/* 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;
+
+ /*
+ * Check permissions. Since LockRelationOid() checks for invalidation
+ * messages, the table might go missing here, too. As before, this
isn't
+ * our problem; just return normally.
+ */
+ aclresult = pg_class_aclcheck_ext(table_oid, GetUserId(),
+
ACL_MAINTAIN, &is_missing);
+ if (is_missing)
+ return;
+ else if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
}
/*
--
2.39.5 (Apple Git-154)
>From 84159c5fffd04ea20090e3cae5b9eb783820dd06 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Wed, 24 Sep 2025 09:47:02 -0500
Subject: [PATCH v3 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)
