From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Corey Huinker <corey.huinker@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Ayush Vatsa <ayushvatsa1810@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Clarification on Role Access Rights to Table Indexes
Date: Tue, 14 Oct 2025 16:32:55 -0500
Message-ID: <aO7BhwICvCWm0bZ4@nathan> (raw)
In-Reply-To: <857a4633aea6ef90bde4156ae351c49794b34732.camel@j-davis.com>
References: <8af53c6e8992aa706e63aafe60a3bcf100b524d1.camel@j-davis.com>
<7b0e2774cdcc8f522ac82f64a8d7266f353a5094.camel@j-davis.com>
<aOfXNAFkj_EFm-8q@nathan>
<aOgmi6avE6qMw_6t@nathan>
<aOkzoH-pXdBr0ewf@nathan>
<31a67adbb10b85ff7cddeafe75b9f6505c902e57.camel@j-davis.com>
<aO1TaPd0YesHy5Sn@nathan>
<bd0826223bff21bd2ad3d0b1520ee8429568c526.camel@j-davis.com>
<aO50zOmoRFnB9_IX@nathan>
<857a4633aea6ef90bde4156ae351c49794b34732.camel@j-davis.com>
I've committed 0004. Here is an updated patch set.
--
nathan
From b19e661d5244582752a5fa61b2cc9e60a7825fc1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Oct 2025 14:42:37 -0500
Subject: [PATCH v7 1/3] Fix lookups in
pg_{clear,restore}_{attribute,relation}_stats().
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/Z8zwVmGzXyDdkAXj%40nathan
Backpatch-through: 18
---
src/backend/statistics/attribute_stats.c | 16 ++-
src/backend/statistics/relation_stats.c | 8 +-
src/backend/statistics/stat_utils.c | 152 +++++++++++----------
src/include/statistics/stat_utils.h | 8 +-
src/test/regress/expected/stats_import.out | 6 +-
5 files changed, 103 insertions(+), 87 deletions(-)
diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.cindex 1db6a7f784c..c5df83282e0 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 = 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);
/* 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 = 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);
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.cindex a59f0c519a4..174da7d93a5 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 = 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);
if (!PG_ARGISNULL(RELPAGES_ARG))
{
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.cindex ef7e5168bed..5fd49e26132 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,84 @@ 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_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_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_oid, ShareUpdateExclusiveLock);+ *locked_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. */+ 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 extremely+ * unlikely scenarios we need to handle.+ */+ if (relId == oldRelId)+ {+ /*+ * If a previous lookup found an index, but the current lookup did+ * not, the index was dropped and the OID was reused for something+ * else between lookups. In theory, we could simply drop our lock on+ * the index's relation and proceed, but in the interest of avoiding+ * complexity, we just error.+ */+ if (table_oid == relId && OidIsValid(*locked_oid))+ ereport(ERROR,+ (errcode(ERRCODE_UNDEFINED_OBJECT),+ errmsg("index \"%s\" was concurrently dropped",+ relation->relname)));++ /*+ * If the current lookup found an index but a previous lookup either+ * did not find an index or found one with a different parent+ * relation, the relation was dropped and the OID was reused for an+ * index between lookups. RangeVarGetRelidExtended() will have+ * already locked the index at this point, so we can't just lock the+ * newly discovered parent table OID without risking deadlock. As+ * above, we just error in this case.+ */+ if (table_oid != relId && table_oid != *locked_oid)+ ereport(ERROR,+ (errcode(ERRCODE_UNDEFINED_OBJECT),+ errmsg("index \"%s\" was concurrently created",+ 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,62 +210,36 @@ 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)));+ NameStr(form->relname)),+ errdetail_relkind_not_supported(form->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);- }-- 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_oid = table_oid;+ }
}
diff --git a/src/include/statistics/stat_utils.h b/src/include/statistics/stat_utils.hindex 512eb776e0e..f41b181d4d3 100644--- a/src/include/statistics/stat_utils.h+++ b/src/include/statistics/stat_utils.h@@ -15,6 +15,9 @@
#include "fmgr.h"
+/* avoid including primnodes.h here */+typedef struct RangeVar RangeVar;+
struct StatsArgInfo
{
const char *argname;
@@ -30,9 +33,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.outindex 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 28c556c4d2356f18b35fc81fd9210675a09f204f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Oct 2025 15:55:35 -0500
Subject: [PATCH v7 2/3] Fix lookup code for REINDEX INDEX.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/Z8zwVmGzXyDdkAXj%40nathan
Backpatch-through: 13
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++----------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex ca2bde62e82..5712fac3697 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
@@ -3000,43 +3001,42 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
if (!OidIsValid(relId))
return;
- /*- * If the relation does exist, check whether it's an index. But note that- * the relation might have been dropped between the time we did the name- * lookup and now. In that case, there's nothing to do.- */+ /* If the relation does exist, check whether it's an index. */
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 2b5004e14d5802fda3f51cbeaa0a41a84c633f62 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Oct 2025 16:22:22 -0500
Subject: [PATCH v7 3/3] Fix privilege checks for pg_prewarm() on indexes.
Author: Ayush Vatsa <ayushvatsa1810@gmail.com>
Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/CACX%2BKaMz2ZoOojh0nQ6QNBYx8Ak1Dkoko%3DD4FSb80BYW%2Bo8CHQ%40mail.gmail.com
Backpatch-through: 13
---
contrib/pg_prewarm/pg_prewarm.c | 47 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 ++++++++++++++++++-
2 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.cindex b968933ea8b..5b519a2c854 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))
@@ -106,9 +110,43 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkString = text_to_cstring(forkName);
forkNumber = forkname_to_number(forkString);
- /* Open relation and check privileges. */+ /*+ * Open relation and check privileges. If the relation is an index, we+ * must check the privileges on its parent table instead.+ */+ relkind = get_rel_relkind(relOid);+ if (relkind == RELKIND_INDEX ||+ relkind == RELKIND_PARTITIONED_INDEX)+ {+ privOid = IndexGetRelation(relOid, true);++ /* Lock table before index to avoid deadlock. */+ if (OidIsValid(privOid))+ LockRelationOid(privOid, AccessShareLock);+ }+ else+ privOid = relOid;+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);++ /*+ * It's possible that the relation with OID "privOid" was dropped and the+ * OID was reused before we locked it. If that happens, we could be left+ * with the wrong parent table OID, in which case we must ERROR. It's+ * possible that such a race would change the outcome of+ * get_rel_relkind(), too, but the worst case scenario there is that we'll+ * check privileges on the 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 +271,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.plindex 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] v7-0001-Fix-lookups-in-pg_-clear-restore-_-attribute-rela.patch (13.0K, ../aO7BhwICvCWm0bZ4@nathan/2-v7-0001-Fix-lookups-in-pg_-clear-restore-_-attribute-rela.patch)
download | inline diff:
From b19e661d5244582752a5fa61b2cc9e60a7825fc1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Oct 2025 14:42:37 -0500
Subject: [PATCH v7 1/3] Fix lookups in
pg_{clear,restore}_{attribute,relation}_stats().
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/Z8zwVmGzXyDdkAXj%40nathan
Backpatch-through: 18
---
src/backend/statistics/attribute_stats.c | 16 ++-
src/backend/statistics/relation_stats.c | 8 +-
src/backend/statistics/stat_utils.c | 152 +++++++++++----------
src/include/statistics/stat_utils.h | 8 +-
src/test/regress/expected/stats_import.out | 6 +-
5 files changed, 103 insertions(+), 87 deletions(-)
diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.cindex 1db6a7f784c..c5df83282e0 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 = 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);
/* 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 = 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);
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.cindex a59f0c519a4..174da7d93a5 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 = 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);
if (!PG_ARGISNULL(RELPAGES_ARG))
{
diff --git a/src/backend/statistics/stat_utils.c b/src/backend/statistics/stat_utils.cindex ef7e5168bed..5fd49e26132 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,84 @@ 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_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_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_oid, ShareUpdateExclusiveLock);+ *locked_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. */+ 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 extremely+ * unlikely scenarios we need to handle.+ */+ if (relId == oldRelId)+ {+ /*+ * If a previous lookup found an index, but the current lookup did+ * not, the index was dropped and the OID was reused for something+ * else between lookups. In theory, we could simply drop our lock on+ * the index's relation and proceed, but in the interest of avoiding+ * complexity, we just error.+ */+ if (table_oid == relId && OidIsValid(*locked_oid))+ ereport(ERROR,+ (errcode(ERRCODE_UNDEFINED_OBJECT),+ errmsg("index \"%s\" was concurrently dropped",+ relation->relname)));++ /*+ * If the current lookup found an index but a previous lookup either+ * did not find an index or found one with a different parent+ * relation, the relation was dropped and the OID was reused for an+ * index between lookups. RangeVarGetRelidExtended() will have+ * already locked the index at this point, so we can't just lock the+ * newly discovered parent table OID without risking deadlock. As+ * above, we just error in this case.+ */+ if (table_oid != relId && table_oid != *locked_oid)+ ereport(ERROR,+ (errcode(ERRCODE_UNDEFINED_OBJECT),+ errmsg("index \"%s\" was concurrently created",+ 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,62 +210,36 @@ 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)));+ NameStr(form->relname)),+ errdetail_relkind_not_supported(form->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);- }-- 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_oid = table_oid;+ }
}
diff --git a/src/include/statistics/stat_utils.h b/src/include/statistics/stat_utils.hindex 512eb776e0e..f41b181d4d3 100644--- a/src/include/statistics/stat_utils.h+++ b/src/include/statistics/stat_utils.h@@ -15,6 +15,9 @@
#include "fmgr.h"
+/* avoid including primnodes.h here */+typedef struct RangeVar RangeVar;+
struct StatsArgInfo
{
const char *argname;
@@ -30,9 +33,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.outindex 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] v7-0002-Fix-lookup-code-for-REINDEX-INDEX.patch (3.3K, ../aO7BhwICvCWm0bZ4@nathan/3-v7-0002-Fix-lookup-code-for-REINDEX-INDEX.patch)
download | inline diff:
From 28c556c4d2356f18b35fc81fd9210675a09f204f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Oct 2025 15:55:35 -0500
Subject: [PATCH v7 2/3] Fix lookup code for REINDEX INDEX.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/Z8zwVmGzXyDdkAXj%40nathan
Backpatch-through: 13
---
src/backend/commands/indexcmds.c | 50 ++++++++++++++++----------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex ca2bde62e82..5712fac3697 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
@@ -3000,43 +3001,42 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
if (!OidIsValid(relId))
return;
- /*- * If the relation does exist, check whether it's an index. But note that- * the relation might have been dropped between the time we did the name- * lookup and now. In that case, there's nothing to do.- */+ /* If the relation does exist, check whether it's an index. */
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] v7-0003-Fix-privilege-checks-for-pg_prewarm-on-indexes.patch (5.3K, ../aO7BhwICvCWm0bZ4@nathan/4-v7-0003-Fix-privilege-checks-for-pg_prewarm-on-indexes.patch)
download | inline diff:
From 2b5004e14d5802fda3f51cbeaa0a41a84c633f62 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Oct 2025 16:22:22 -0500
Subject: [PATCH v7 3/3] Fix privilege checks for pg_prewarm() on indexes.
Author: Ayush Vatsa <ayushvatsa1810@gmail.com>
Co-authored-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Jeff Davis <pgsql@j-davis.com>
Discussion: https://postgr.es/m/CACX%2BKaMz2ZoOojh0nQ6QNBYx8Ak1Dkoko%3DD4FSb80BYW%2Bo8CHQ%40mail.gmail.com
Backpatch-through: 13
---
contrib/pg_prewarm/pg_prewarm.c | 47 +++++++++++++++++++++++++++++--
contrib/pg_prewarm/t/001_basic.pl | 29 ++++++++++++++++++-
2 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.cindex b968933ea8b..5b519a2c854 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))
@@ -106,9 +110,43 @@ pg_prewarm(PG_FUNCTION_ARGS)
forkString = text_to_cstring(forkName);
forkNumber = forkname_to_number(forkString);
- /* Open relation and check privileges. */+ /*+ * Open relation and check privileges. If the relation is an index, we+ * must check the privileges on its parent table instead.+ */+ relkind = get_rel_relkind(relOid);+ if (relkind == RELKIND_INDEX ||+ relkind == RELKIND_PARTITIONED_INDEX)+ {+ privOid = IndexGetRelation(relOid, true);++ /* Lock table before index to avoid deadlock. */+ if (OidIsValid(privOid))+ LockRelationOid(privOid, AccessShareLock);+ }+ else+ privOid = relOid;+
rel = relation_open(relOid, AccessShareLock);
- aclresult = pg_class_aclcheck(relOid, GetUserId(), ACL_SELECT);++ /*+ * It's possible that the relation with OID "privOid" was dropped and the+ * OID was reused before we locked it. If that happens, we could be left+ * with the wrong parent table OID, in which case we must ERROR. It's+ * possible that such a race would change the outcome of+ * get_rel_relkind(), too, but the worst case scenario there is that we'll+ * check privileges on the 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 +271,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.plindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, pgsql@j-davis.com, corey.huinker@gmail.com, tgl@sss.pgh.pa.us, ayushvatsa1810@gmail.com, robertmhaas@gmail.com, david.g.johnston@gmail.com
Subject: Re: Clarification on Role Access Rights to Table Indexes
In-Reply-To: <aO7BhwICvCWm0bZ4@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox