On Fri, Aug 29, 2025 at 8:48 PM Álvaro Herrera <alvhe...@kurilemu.de> wrote: > > > > Alternatively, maybe the right fix is to move the parse-analysis > > > work into CreateStatistics altogether. I think there is not a > > > very good argument for ProcessUtilitySlow doing all that stuff. > > > It's supposed to mainly just be a dispatching switch(), IMO. > > > > seems doable. > > transformStatsStmt, CreateStatistics both used only twice, refactoring > > arguments should be fine. > > please check the attached POC, regress tests also added. > > Yeah, I like how this turned out. I found out this was introduced in > commit a4d75c86bf15.
Previously, CreateStatistics and ProcessUtilitySlow opened the same relation twice with a ShareUpdateExclusiveLock. This refactor ensures the relation is opened with ShareUpdateExclusiveLock only once and also reduces redundant error checks. The logic is now more intuitive: we first error checking CreateStatsStmt->relations and then call transformStatsStmt to parse analysis CreateStatsStmt->exprs. please check the attached refactor CreateStatsStmt.
From f11f1c97ef4a18a31d5442e4e745a530caa02d07 Mon Sep 17 00:00:00 2001 From: jian he <jian.universal...@gmail.com> Date: Thu, 4 Sep 2025 12:52:13 +0800 Subject: [PATCH v3 1/1] refactor CreateStatsStmt move all CreateStatsStmt logic into CreateStatistics. Previously, CreateStatistics and ProcessUtilitySlow opened the same relation twice with a ShareUpdateExclusiveLock. This refactor ensures the relation is opened with ShareUpdateExclusiveLock only once and also reduces redundant error checks. The logic is now more intuitive: we first error checking CreateStatsStmt->relations and then call transformStatsStmt to parse analysis CreateStatsStmt->exprs. discussion: https://postgr.es/m/CACJufxFcvo=fM6J9RC88n1U=es9+5qdftwunqmu5h7ogjmq...@mail.gmail.com --- src/backend/commands/statscmds.c | 17 ++++++++++++++--- src/backend/commands/tablecmds.c | 2 +- src/backend/tcop/utility.c | 30 +----------------------------- src/include/commands/defrem.h | 2 +- 4 files changed, 17 insertions(+), 34 deletions(-) diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c index e24d540cd45..100eb82db64 100644 --- a/src/backend/commands/statscmds.c +++ b/src/backend/commands/statscmds.c @@ -29,6 +29,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" @@ -57,9 +58,11 @@ compare_int16(const void *a, const void *b) /* * CREATE STATISTICS + * Ensure queryString is not NULL when we need do parse analysis for stmt! + * stmt changed on the go, see transformStatsStmt. */ ObjectAddress -CreateStatistics(CreateStatsStmt *stmt) +CreateStatistics(CreateStatsStmt *stmt, const char *queryString) { int16 attnums[STATS_MAX_DIMENSIONS]; int nattnums = 0; @@ -111,8 +114,9 @@ CreateStatistics(CreateStatsStmt *stmt) if (!IsA(rln, RangeVar)) ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("only a single relation is allowed in CREATE STATISTICS"))); + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot create statistics on the specified relation"), + errdetail("CREATE STATISTICS only supports tables, foreign tables and materialized views.")); /* * CREATE STATISTICS will influence future execution plans but does @@ -150,6 +154,13 @@ CreateStatistics(CreateStatsStmt *stmt) Assert(rel); relid = RelationGetRelid(rel); + if (queryString != NULL) + { + /* Run parse analysis ... */ + (void) transformStatsStmt(relid, stmt, queryString); + } + Assert(stmt->transformed); + /* * If the node has a name, split it up and determine creation namespace. * If not, put the object in the same namespace as the relation, and cons diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 082a3575d62..b8dd9306b93 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -9654,7 +9654,7 @@ ATExecAddStatistics(AlteredTableInfo *tab, Relation rel, /* The CreateStatsStmt has already been through transformStatsStmt */ Assert(stmt->transformed); - address = CreateStatistics(stmt); + address = CreateStatistics(stmt, NULL); return address; } diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 5f442bc3bd4..027356f6e77 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1866,35 +1866,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("cannot create statistics on the specified relation"), - errdetail("CREATE STATISTICS only supports tables, foreign tables and materialized views."))); - - /* - * 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); - } + address = CreateStatistics((CreateStatsStmt *) parsetree, queryString); break; case T_AlterStatsStmt: diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index dd22b5efdfd..684f62109fd 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); +extern ObjectAddress CreateStatistics(CreateStatsStmt *stmt, const char *queryString); extern ObjectAddress AlterStatistics(AlterStatsStmt *stmt); extern void RemoveStatisticsById(Oid statsOid); extern void RemoveStatisticsDataById(Oid statsOid, bool inh); -- 2.34.1
