From: Nathan Bossart <nathan@postgresql.org>
Subject: [PATCH v6 1/4] fix priv checks in stats code
Date: Fri, 10 Oct 2025 15:50:06 -0500
---
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.cindex 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.cindex 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.cindex 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.hindex 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.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)
--iujXWOSrXz0NaSpd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename=v6-0002-fix-reindex-index-rangevar-callback.patch
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-general@postgresql.org
Cc: nathan@postgresql.org
Subject: Re: [PATCH v6 1/4] fix priv checks in stats code
In-Reply-To: <no-message-id-49383@localhost>
* 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