I've committed 0004. Here is an updated patch set. -- nathan
>From b19e661d5244582752a5fa61b2cc9e60a7825fc1 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> 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 <[email protected]> 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.c index 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.c index 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.c index 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.h index 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.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 28c556c4d2356f18b35fc81fd9210675a09f204f Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Tue, 14 Oct 2025 15:55:35 -0500 Subject: [PATCH v7 2/3] Fix lookup code for REINDEX INDEX. Reviewed-by: Tom Lane <[email protected]> Reviewed-by: Jeff Davis <[email protected]> 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.c index 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 <[email protected]> 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 <[email protected]> Co-authored-by: Nathan Bossart <[email protected]> Reviewed-by: Tom Lane <[email protected]> Reviewed-by: Jeff Davis <[email protected]> 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.c index 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.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)
