hi. new patch attached.
Summary of the problem we’re trying to solve: 1. ProcessUtility for T_CreateStatsStmt, there’s a lot of code that doesn’t belong there. 2. The current pattern of repeatedly performing RangeVar lookups isn’t ideal — we should perform the lookup once and then use the corresponding relation OID for subsequent operations. The main part this patch is copied from https://postgr.es/m/CA+TgmobyMXzoEzscCRDCggHRCTp1TW=dm9pehmwoykos43w...@mail.gmail.com Below is a copy of the commit message. ------------------ Previously, a lot of code would be run in the ProcessUtility pipeline, where it is not welcome (that's supposed to be just a giant dispatcher switch). Move the parse analysis code to CreateStatistics() instead, which then means we don't need to open the relation twice; it also allows us to give a better error message when something other than a relation name is given in the FROM clause. Generally we should avoid look up the same less-than-fully-qualified name multiple times, we might get different answers due to concurrent activity, and that might create a security vulnerability, along the lines of CVE-2014-0062. Refactor so that a CreateStatsStmt carries both the untransformed FROM clause and the range table that we get when it's transformed. That way, once the name lookups have been done, we can propagate the results forward to future processing steps and avoid ever repeating the lookup. ------------------ -- jian https://www.enterprisedb.com/
From 1f6c467793843354e492cc7b5cd7e1137822250a Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Thu, 13 Nov 2025 11:19:26 +0800 Subject: [PATCH v5 1/1] Restructure CreateStatsStmt parse-analysis processing Previously, a lot of code would be run in the ProcessUtility pipeline, where it is not welcome (that's supposed to be just a giant dispatcher switch). Move the parse analysis code to CreateStatistics() instead, which then means we don't need to open the relation twice; it also allows us to give a better error message when something other than a relation name is given in the FROM clause. Generally we should avoid look up the same less-than-fully-qualified name multiple times, we might get different answers due to concurrent activity, and that might create a security vulnerability, along the lines of CVE-2014-0062. Refactor so that a CreateStatsStmt carries both the untransformed FROM clause and the range table that we get when it's transformed. That way, once the name lookups have been done, we can propagate the results forward to future processing steps and avoid ever repeating the lookup. discussion: https://postgr.es/m/cacjufxgxlu400qbbgdoboaza0xk58rqqscaprxbamumo0f8...@mail.gmail.com discussion: https://postgr.es/m/CA+TgmobyMXzoEzscCRDCggHRCTp1TW=dm9pehmwoykos43w...@mail.gmail.com commitfest: https://commitfest.postgresql.org/patch/6106 --- src/backend/commands/statscmds.c | 65 ++++------------ src/backend/commands/tablecmds.c | 17 ++++- src/backend/parser/gram.y | 6 +- src/backend/parser/parse_utilcmd.c | 118 +++++++++++++++++++++++++++-- src/backend/tcop/utility.c | 29 +------ src/include/commands/defrem.h | 2 +- src/include/nodes/parsenodes.h | 3 + src/include/parser/parse_utilcmd.h | 3 +- 8 files changed, 150 insertions(+), 93 deletions(-) diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c index 0cf0ea43f19..0e08229de53 100644 --- a/src/backend/commands/statscmds.c +++ b/src/backend/commands/statscmds.c @@ -30,6 +30,7 @@ #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "optimizer/optimizer.h" +#include "parser/parse_utilcmd.h" #include "statistics/statistics.h" #include "utils/acl.h" #include "utils/builtins.h" @@ -60,7 +61,7 @@ compare_int16(const void *a, const void *b) * CREATE STATISTICS */ ObjectAddress -CreateStatistics(CreateStatsStmt *stmt, bool check_rights) +CreateStatistics(CreateStatsStmt *stmt, bool check_rights, const char *queryString) { int16 attnums[STATS_MAX_DIMENSIONS]; int nattnums = 0; @@ -78,6 +79,7 @@ CreateStatistics(CreateStatsStmt *stmt, bool check_rights) Datum exprsDatum; Relation statrel; Relation rel = NULL; + RangeTblEntry *rte; Oid relid; ObjectAddress parentobject, myself; @@ -95,58 +97,23 @@ CreateStatistics(CreateStatsStmt *stmt, bool check_rights) Assert(IsA(stmt, CreateStatsStmt)); + /* Run parse analysis */ + stmt = transformStatsStmt(stmt, queryString); + + /* XXX internal error is better than Assert? error message need polish */ + if (list_length(stmt->rtable) != list_length(stmt->from_clause)) + elog(ERROR, "CreateStatsStmt length of rtable should equal to from_clause"); + /* - * Examine the FROM clause. Currently, we only allow it to be a single - * simple table, but later we'll probably allow multiple tables and JOIN - * syntax. The grammar is already prepared for that, so we have to check - * here that what we got is what we can support. + * Double-check that the range table is of a form we can support. */ - if (list_length(stmt->relations) != 1) + if (list_length(stmt->rtable) != 1) ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("only a single relation is allowed in CREATE STATISTICS"))); + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("only a single relation is allowed in CREATE STATISTICS")); - foreach(cell, stmt->relations) - { - Node *rln = (Node *) lfirst(cell); - - if (!IsA(rln, RangeVar)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("only a single relation is allowed in CREATE STATISTICS"))); - - /* - * CREATE STATISTICS will influence future execution plans but does - * not interfere with currently executing plans. So it should be - * enough to take only ShareUpdateExclusiveLock on relation, - * conflicting with ANALYZE and other DDL that sets statistical - * information, but not with normal queries. - */ - rel = relation_openrv((RangeVar *) rln, ShareUpdateExclusiveLock); - - /* Restrict to allowed relation types */ - if (rel->rd_rel->relkind != RELKIND_RELATION && - rel->rd_rel->relkind != RELKIND_MATVIEW && - rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE && - rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("cannot define statistics for relation \"%s\"", - RelationGetRelationName(rel)), - errdetail_relkind_not_supported(rel->rd_rel->relkind))); - - /* You must own the relation to create stats on it */ - if (!object_ownercheck(RelationRelationId, RelationGetRelid(rel), stxowner)) - aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind), - RelationGetRelationName(rel)); - - /* Creating statistics on system catalogs is not allowed */ - if (!allowSystemTableMods && IsSystemRelation(rel)) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("permission denied: \"%s\" is a system catalog", - RelationGetRelationName(rel)))); - } + rte = linitial_node(RangeTblEntry, stmt->rtable); + rel = relation_open(rte->relid, NoLock); Assert(rel); relid = RelationGetRelid(rel); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 23ebaa3f230..5d778f8d7d6 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -9682,7 +9682,7 @@ ATExecAddStatistics(AlteredTableInfo *tab, Relation rel, /* The CreateStatsStmt has already been through transformStatsStmt */ Assert(stmt->transformed); - address = CreateStatistics(stmt, !is_rebuild); + address = CreateStatistics(stmt, !is_rebuild, NULL); return address; } @@ -15658,10 +15658,19 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, querytree_list = list_concat(querytree_list, afterStmts); } else if (IsA(stmt, CreateStatsStmt)) + { + RangeTblEntry *rte; + CreateStatsStmt *ss = castNode(CreateStatsStmt, stmt); + + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = oldRelId; + rte->rellockmode = ShareUpdateExclusiveLock; + ss->rtable = list_make1(rte); + querytree_list = lappend(querytree_list, - transformStatsStmt(oldRelId, - (CreateStatsStmt *) stmt, - cmd)); + transformStatsStmt(ss, cmd)); + } else querytree_list = lappend(querytree_list, stmt); } diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index c3a0a354a9c..ca3cb446acd 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -4778,7 +4778,8 @@ CreateStatsStmt: n->defnames = $3; n->stat_types = $4; n->exprs = $6; - n->relations = $8; + n->from_clause = $8; + n->rtable = NIL; n->stxcomment = NULL; n->if_not_exists = false; $$ = (Node *) n; @@ -4791,7 +4792,8 @@ CreateStatsStmt: n->defnames = $6; n->stat_types = $7; n->exprs = $9; - n->relations = $11; + n->from_clause = $11; + n->rtable = NIL; n->stxcomment = NULL; n->if_not_exists = true; $$ = (Node *) n; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index e96b38a59d5..410d8e84e87 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -2049,6 +2049,7 @@ generateClonedExtStatsStmt(RangeVar *heapRel, Oid heapRelid, HeapTuple ht_stats; Form_pg_statistic_ext statsrec; CreateStatsStmt *stats; + RangeTblEntry *rte; List *stat_types = NIL; List *def_names = NIL; bool isnull; @@ -2152,7 +2153,15 @@ generateClonedExtStatsStmt(RangeVar *heapRel, Oid heapRelid, stats->defnames = NULL; stats->stat_types = stat_types; stats->exprs = def_names; - stats->relations = list_make1(heapRel); + stats->from_clause = list_make1(heapRel); + + /* Create a range table entry. */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = heapRelid; + rte->rellockmode = ShareUpdateExclusiveLock; + stats->rtable = list_make1(rte); + stats->stxcomment = NULL; stats->transformed = true; /* don't need transformStatsStmt again */ stats->if_not_exists = false; @@ -3136,32 +3145,46 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString) /* * transformStatsStmt - parse analysis for CREATE STATISTICS * - * To avoid race conditions, it's important that this function relies only on - * the passed-in relid (and not on stmt->relation) to determine the target - * relation. + * This is used by CREATE STATISTICS. Changing table column’s data type or + * generated expression will trigger a rebuild of the related table’s + * statistics, during which this will also be used. */ CreateStatsStmt * -transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString) +transformStatsStmt(CreateStatsStmt *stmt, const char *queryString) { ParseState *pstate; ParseNamespaceItem *nsitem; ListCell *l; Relation rel; + RangeTblEntry *rte; /* Nothing to do if statement already transformed. */ if (stmt->transformed) return stmt; + /* Construct range table if not done yet. */ + if (stmt->rtable == NIL) + stmt->rtable = transformStatsFromClause(stmt->from_clause); + + /* + * The range table should have been derived from a FROM clause consisting + * of a single RangeVar, so we expect a single RangeTblEntry. + */ + Assert(list_length(stmt->rtable) == 1); + rte = linitial(stmt->rtable); + Assert(IsA(rte, RangeTblEntry)); + + /* See transformStatsFromClause for notes on lock level. */ + rel = table_open(rte->relid, NoLock); + /* Set up pstate */ pstate = make_parsestate(NULL); pstate->p_sourcetext = queryString; /* * Put the parent table into the rtable so that the expressions can refer - * to its fields without qualification. Caller is responsible for locking - * relation, but we still need to open it. + * to its fields without qualification. */ - rel = relation_open(relid, NoLock); nsitem = addRangeTableEntryForRelation(pstate, rel, AccessShareLock, NULL, false, true); @@ -3205,6 +3228,85 @@ transformStatsStmt(Oid relid, CreateStatsStmt *stmt, const char *queryString) return stmt; } +/* + * Accepts a list of RangeVar objects and returns a corresponding list of + * RangeTblEntry objects. For use while transforming a CreateStatsStmt. + * + * At present, the only case we know how to handle is a FROM clause + * consisting of a single RangeVar. + */ +List * +transformStatsFromClause(List *from_clause) +{ + RangeTblEntry *rte; + ListCell *cell; + Relation rel = NULL; + + /* + * Examine the FROM clause. Currently, we only allow it to be a single + * simple table, but later we'll probably allow multiple tables and JOIN + * syntax. The grammar is already prepared for that, so we have to check + * here that what we got is what we can support. + */ + if (list_length(from_clause) != 1) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("only a single relation is allowed in CREATE STATISTICS")); + + foreach(cell, from_clause) + { + Node *rln = (Node *) lfirst(cell); + + if (!IsA(rln, RangeVar)) + ereport(ERROR, + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("CREATE STATISTICS only supports relation names in the FROM clause")); + + /* + * CREATE STATISTICS will influence future execution plans but does + * not interfere with currently executing plans. So it should be + * enough to take only ShareUpdateExclusiveLock on relation, + * conflicting with ANALYZE and other DDL that sets statistical + * information, but not with normal queries. + */ + rel = relation_openrv((RangeVar *) rln, ShareUpdateExclusiveLock); + + /* Restrict to allowed relation types */ + if (rel->rd_rel->relkind != RELKIND_RELATION && + rel->rd_rel->relkind != RELKIND_MATVIEW && + rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE && + rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE) + ereport(ERROR, + errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot define statistics for relation \"%s\"", + RelationGetRelationName(rel)), + errdetail_relkind_not_supported(rel->rd_rel->relkind)); + + /* You must own the relation to create stats on it */ + if (!object_ownercheck(RelationRelationId, RelationGetRelid(rel), GetUserId())) + aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind), + RelationGetRelationName(rel)); + + /* Creating statistics on system catalogs is not allowed */ + if (!allowSystemTableMods && IsSystemRelation(rel)) + ereport(ERROR, + errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied: \"%s\" is a system catalog", + RelationGetRelationName(rel))); + } + + /* Create a range table entry. */ + rte = makeNode(RangeTblEntry); + rte->rtekind = RTE_RELATION; + rte->relid = RelationGetRelid(rel); + rte->rellockmode = ShareUpdateExclusiveLock; + + /* Close relation */ + relation_close(rel, NoLock); + + return list_make1(rte); +} + /* * transformRuleStmt - diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index d18a3a60a46..0c31113ebb5 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1874,34 +1874,7 @@ ProcessUtilitySlow(ParseState *pstate, break; case T_CreateStatsStmt: - { - Oid relid; - CreateStatsStmt *stmt = (CreateStatsStmt *) parsetree; - RangeVar *rel = (RangeVar *) linitial(stmt->relations); - - if (!IsA(rel, RangeVar)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("CREATE STATISTICS only supports relation names in the FROM clause"))); - - /* - * CREATE STATISTICS will influence future execution plans - * but does not interfere with currently executing plans. - * So it should be enough to take ShareUpdateExclusiveLock - * on relation, conflicting with ANALYZE and other DDL - * that sets statistical information, but not with normal - * queries. - * - * XXX RangeVarCallbackOwnsRelation not needed here, to - * keep the same behavior as before. - */ - relid = RangeVarGetRelid(rel, ShareUpdateExclusiveLock, false); - - /* Run parse analysis ... */ - stmt = transformStatsStmt(relid, stmt, queryString); - - address = CreateStatistics(stmt, true); - } + address = CreateStatistics((CreateStatsStmt *) parsetree, true, queryString); break; case T_AlterStatsStmt: diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index f3432b4b6a1..95397f91c5a 100644 --- a/src/include/commands/defrem.h +++ b/src/include/commands/defrem.h @@ -85,7 +85,7 @@ extern void RemoveOperatorById(Oid operOid); extern ObjectAddress AlterOperator(AlterOperatorStmt *stmt); /* commands/statscmds.c */ -extern ObjectAddress CreateStatistics(CreateStatsStmt *stmt, bool check_rights); +extern ObjectAddress CreateStatistics(CreateStatsStmt *stmt, bool check_rights, const char *queryString); extern ObjectAddress AlterStatistics(AlterStatsStmt *stmt); extern void RemoveStatisticsById(Oid statsOid); extern void RemoveStatisticsDataById(Oid statsOid, bool inh); diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index d14294a4ece..3e9257cc8ae 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3523,6 +3523,9 @@ typedef struct CreateStatsStmt List *stat_types; /* stat types (list of String) */ List *exprs; /* expressions to build statistics on */ List *relations; /* rels to build stats on (list of RangeVar) */ + List *from_clause; /* FROM clause ((list of RangeVar)) */ + List *rtable; /* It’s not derived directly from the parser, instead + * it comes from parse analysis. (list of RangeTblEntry) */ char *stxcomment; /* comment to apply to stats, or NULL */ bool transformed; /* true when transformStatsStmt is finished */ bool if_not_exists; /* do nothing if stats name already exists */ diff --git a/src/include/parser/parse_utilcmd.h b/src/include/parser/parse_utilcmd.h index 4965fac4495..499e9ebef1c 100644 --- a/src/include/parser/parse_utilcmd.h +++ b/src/include/parser/parse_utilcmd.h @@ -26,8 +26,9 @@ extern AlterTableStmt *transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, List **afterStmts); extern IndexStmt *transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString); -extern CreateStatsStmt *transformStatsStmt(Oid relid, CreateStatsStmt *stmt, +extern CreateStatsStmt *transformStatsStmt(CreateStatsStmt *stmt, const char *queryString); +extern List *transformStatsFromClause(List *from_clause); extern void transformRuleStmt(RuleStmt *stmt, const char *queryString, List **actions, Node **whereClause); extern List *transformCreateSchemaStmtElements(List *schemaElts, -- 2.34.1
