From 130a39ff1888f24dd38dc15f227f7160474369f2 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Wed, 3 Nov 2021 20:08:42 -0500 Subject: [PATCH v3 5/6] Maybe better way than looping twice. Change partitioned tables to use only "inherited" stats_ext. --- src/backend/commands/statscmds.c | 63 +++++--- src/backend/optimizer/util/plancat.c | 227 ++++++++++++++------------- 2 files changed, 151 insertions(+), 139 deletions(-) diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c index 4395d878c7..89e9ad5843 100644 --- a/src/backend/commands/statscmds.c +++ b/src/backend/commands/statscmds.c @@ -45,6 +45,7 @@ static char *ChooseExtendedStatisticName(const char *name1, const char *name2, const char *label, Oid namespaceid); static char *ChooseExtendedStatisticNameAddition(List *exprs); +static void RemoveStatisticsByIdWorker(Relation pg_statistic_ext, Oid statsOid, bool inh); /* qsort comparator for the attnums in CreateStatistics */ @@ -524,8 +525,9 @@ CreateStatistics(CreateStatsStmt *stmt) datavalues[Anum_pg_statistic_ext_data_stxoid - 1] = ObjectIdGetDatum(statoid); - /* create only the "stxdinherit=false", because that always exists */ - datavalues[Anum_pg_statistic_ext_data_stxdinherit - 1] = ObjectIdGetDatum(false); + /* create "stxdinherit=false", because that always exists, except for + * partitioned tables, for which that's meaningless */ + datavalues[Anum_pg_statistic_ext_data_stxdinherit - 1] = BoolGetDatum(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); /* no statistics built yet */ datanulls[Anum_pg_statistic_ext_data_stxdndistinct - 1] = true; @@ -729,30 +731,7 @@ RemoveStatisticsById(Oid statsOid) HeapTuple tup; Form_pg_statistic_ext statext; Oid relid; - int inh; - - /* - * First delete the pg_statistic_ext_data tuple holding the actual - * statistical data. - */ - relation = table_open(StatisticExtDataRelationId, RowExclusiveLock); - - /* hack to delete both stxdinherit = true/false */ - for (inh = 0; inh <= 1; inh++) - { - tup = SearchSysCache2(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid), - BoolGetDatum(inh)); - - if (!HeapTupleIsValid(tup)) /* should not happen */ - // elog(ERROR, "cache lookup failed for statistics data %u", statsOid); - continue; - - CatalogTupleDelete(relation, &tup->t_self); - - ReleaseSysCache(tup); - } - - table_close(relation, RowExclusiveLock); + char relkind; /* * Delete the pg_statistic_ext tuple. Also send out a cache inval on the @@ -767,6 +746,7 @@ RemoveStatisticsById(Oid statsOid) statext = (Form_pg_statistic_ext) GETSTRUCT(tup); relid = statext->stxrelid; + relkind = get_rel_relkind(relid); CacheInvalidateRelcacheByRelid(relid); @@ -775,6 +755,37 @@ RemoveStatisticsById(Oid statsOid) ReleaseSysCache(tup); table_close(relation, RowExclusiveLock); + + /* + * Delete the pg_statistic_ext_data tuples holding the actual + * statistical data. + */ + relation = table_open(StatisticExtDataRelationId, RowExclusiveLock); + + if (relkind != RELKIND_PARTITIONED_TABLE) + RemoveStatisticsByIdWorker(relation, statsOid, false); + RemoveStatisticsByIdWorker(relation, statsOid, true); + + table_close(relation, RowExclusiveLock); +} + +void +RemoveStatisticsByIdWorker(Relation pg_statistic_ext, Oid statsOid, bool inh) +{ + HeapTuple tup; + tup = SearchSysCache2(STATEXTDATASTXOID, ObjectIdGetDatum(statsOid), + BoolGetDatum(inh)); + + /* should not happen, .. */ + if (!HeapTupleIsValid(tup)) + { + if (!inh) + elog(ERROR, "cache lookup failed for statistics data %u inh %d", statsOid, inh); + return; + } + + CatalogTupleDelete(pg_statistic_ext, &tup->t_self); + ReleaseSysCache(tup); } /* diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 154d48a330..b1d8847529 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -81,6 +81,9 @@ static void set_baserel_partition_key_exprs(Relation relation, static void set_baserel_partition_constraint(Relation relation, RelOptInfo *rel); +static void get_relation_statistics_worker(RelOptInfo *rel, List **stainfos, + Oid statOid, HeapTuple htup, Form_pg_statistic_ext staForm, Index varno, + bool inh); /* * get_relation_info - @@ -1301,7 +1304,6 @@ get_relation_constraints(PlannerInfo *root, static List * get_relation_statistics(RelOptInfo *rel, Relation relation) { - Index varno = rel->relid; List *statoidlist; List *stainfos = NIL; ListCell *l; @@ -1312,150 +1314,149 @@ get_relation_statistics(RelOptInfo *rel, Relation relation) { Oid statOid = lfirst_oid(l); Form_pg_statistic_ext staForm; - Form_pg_statistic_ext_data dataForm; HeapTuple htup; - HeapTuple dtup; - Bitmapset *keys = NULL; - List *exprs = NIL; - int i; - int inh; htup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(statOid)); if (!HeapTupleIsValid(htup)) elog(ERROR, "cache lookup failed for statistics object %u", statOid); staForm = (Form_pg_statistic_ext) GETSTRUCT(htup); - /* - * Hack to load stats with stxdinherit true/false - there should be - * a better way to do this, I guess. - */ - for (inh = 0; inh <= 1; inh++) - { - dtup = SearchSysCache2(STATEXTDATASTXOID, - ObjectIdGetDatum(statOid), BoolGetDatum((bool) inh)); - if (!HeapTupleIsValid(dtup)) - continue; + get_relation_statistics_worker(rel, &stainfos, statOid, htup, staForm, rel->relid, false); + get_relation_statistics_worker(rel, &stainfos, statOid, htup, staForm, rel->relid, true); - dataForm = (Form_pg_statistic_ext_data) GETSTRUCT(dtup); + ReleaseSysCache(htup); + } - /* - * First, build the array of columns covered. This is ultimately - * wasted if no stats within the object have actually been built, but - * it doesn't seem worth troubling over that case. - */ - for (i = 0; i < staForm->stxkeys.dim1; i++) - keys = bms_add_member(keys, staForm->stxkeys.values[i]); + list_free(statoidlist); - /* - * Preprocess expressions (if any). We read the expressions, run them - * through eval_const_expressions, and fix the varnos. - */ - { - bool isnull; - Datum datum; + return stainfos; +} - /* decode expression (if any) */ - datum = SysCacheGetAttr(STATEXTOID, htup, - Anum_pg_statistic_ext_stxexprs, &isnull); +static void +get_relation_statistics_worker(RelOptInfo *rel, List **stainfos, Oid statOid, HeapTuple htup, Form_pg_statistic_ext staForm, Index varno, bool inh) +{ + Form_pg_statistic_ext_data dataForm; + HeapTuple dtup; + Bitmapset *keys = NULL; + List *exprs = NIL; + + dtup = SearchSysCache2(STATEXTDATASTXOID, + ObjectIdGetDatum(statOid), BoolGetDatum(inh)); + if (!HeapTupleIsValid(dtup)) + return; - if (!isnull) - { - char *exprsString; + dataForm = (Form_pg_statistic_ext_data) GETSTRUCT(dtup); - exprsString = TextDatumGetCString(datum); - exprs = (List *) stringToNode(exprsString); - pfree(exprsString); + /* + * First, build the array of columns covered. This is ultimately + * wasted if no stats within the object have actually been built, but + * it doesn't seem worth troubling over that case. + */ + for (int i = 0; i < staForm->stxkeys.dim1; i++) + keys = bms_add_member(keys, staForm->stxkeys.values[i]); - /* - * Run the expressions through eval_const_expressions. This is - * not just an optimization, but is necessary, because the - * planner will be comparing them to similarly-processed qual - * clauses, and may fail to detect valid matches without this. - * We must not use canonicalize_qual, however, since these - * aren't qual expressions. - */ - exprs = (List *) eval_const_expressions(NULL, (Node *) exprs); + /* + * Preprocess expressions (if any). We read the expressions, run them + * through eval_const_expressions, and fix the varnos. + */ + { + bool isnull; + Datum datum; - /* May as well fix opfuncids too */ - fix_opfuncids((Node *) exprs); + /* decode expression (if any) */ + datum = SysCacheGetAttr(STATEXTOID, htup, + Anum_pg_statistic_ext_stxexprs, &isnull); - /* - * Modify the copies we obtain from the relcache to have the - * correct varno for the parent relation, so that they match - * up correctly against qual clauses. - */ - if (varno != 1) - ChangeVarNodes((Node *) exprs, 1, varno, 0); - } - } - - /* add one StatisticExtInfo for each kind built */ - if (statext_is_kind_built(dtup, STATS_EXT_NDISTINCT)) - { - StatisticExtInfo *info = makeNode(StatisticExtInfo); + if (!isnull) + { + char *exprsString; - info->statOid = statOid; - info->inherit = dataForm->stxdinherit; - info->rel = rel; - info->kind = STATS_EXT_NDISTINCT; - info->keys = bms_copy(keys); - info->exprs = exprs; + exprsString = TextDatumGetCString(datum); + exprs = (List *) stringToNode(exprsString); + pfree(exprsString); - stainfos = lappend(stainfos, info); - } + /* + * Run the expressions through eval_const_expressions. This is + * not just an optimization, but is necessary, because the + * planner will be comparing them to similarly-processed qual + * clauses, and may fail to detect valid matches without this. + * We must not use canonicalize_qual, however, since these + * aren't qual expressions. + */ + exprs = (List *) eval_const_expressions(NULL, (Node *) exprs); - if (statext_is_kind_built(dtup, STATS_EXT_DEPENDENCIES)) - { - StatisticExtInfo *info = makeNode(StatisticExtInfo); + /* May as well fix opfuncids too */ + fix_opfuncids((Node *) exprs); - info->statOid = statOid; - info->inherit = dataForm->stxdinherit; - info->rel = rel; - info->kind = STATS_EXT_DEPENDENCIES; - info->keys = bms_copy(keys); - info->exprs = exprs; + /* + * Modify the copies we obtain from the relcache to have the + * correct varno for the parent relation, so that they match + * up correctly against qual clauses. + */ + if (varno != 1) + ChangeVarNodes((Node *) exprs, 1, varno, 0); + } + } - stainfos = lappend(stainfos, info); - } + /* add one StatisticExtInfo for each kind built */ + if (statext_is_kind_built(dtup, STATS_EXT_NDISTINCT)) + { + StatisticExtInfo *info = makeNode(StatisticExtInfo); - if (statext_is_kind_built(dtup, STATS_EXT_MCV)) - { - StatisticExtInfo *info = makeNode(StatisticExtInfo); + info->statOid = statOid; + info->inherit = dataForm->stxdinherit; + info->rel = rel; + info->kind = STATS_EXT_NDISTINCT; + info->keys = bms_copy(keys); + info->exprs = exprs; - info->statOid = statOid; - info->inherit = dataForm->stxdinherit; - info->rel = rel; - info->kind = STATS_EXT_MCV; - info->keys = bms_copy(keys); - info->exprs = exprs; + *stainfos = lappend(*stainfos, info); + } - stainfos = lappend(stainfos, info); - } + if (statext_is_kind_built(dtup, STATS_EXT_DEPENDENCIES)) + { + StatisticExtInfo *info = makeNode(StatisticExtInfo); - if (statext_is_kind_built(dtup, STATS_EXT_EXPRESSIONS)) - { - StatisticExtInfo *info = makeNode(StatisticExtInfo); + info->statOid = statOid; + info->inherit = dataForm->stxdinherit; + info->rel = rel; + info->kind = STATS_EXT_DEPENDENCIES; + info->keys = bms_copy(keys); + info->exprs = exprs; - info->statOid = statOid; - info->inherit = dataForm->stxdinherit; - info->rel = rel; - info->kind = STATS_EXT_EXPRESSIONS; - info->keys = bms_copy(keys); - info->exprs = exprs; + *stainfos = lappend(*stainfos, info); + } - stainfos = lappend(stainfos, info); - } + if (statext_is_kind_built(dtup, STATS_EXT_MCV)) + { + StatisticExtInfo *info = makeNode(StatisticExtInfo); - ReleaseSysCache(dtup); - } + info->statOid = statOid; + info->inherit = dataForm->stxdinherit; + info->rel = rel; + info->kind = STATS_EXT_MCV; + info->keys = bms_copy(keys); + info->exprs = exprs; - ReleaseSysCache(htup); - bms_free(keys); + *stainfos = lappend(*stainfos, info); } - list_free(statoidlist); + if (statext_is_kind_built(dtup, STATS_EXT_EXPRESSIONS)) + { + StatisticExtInfo *info = makeNode(StatisticExtInfo); - return stainfos; + info->statOid = statOid; + info->inherit = dataForm->stxdinherit; + info->rel = rel; + info->kind = STATS_EXT_EXPRESSIONS; + info->keys = bms_copy(keys); + info->exprs = exprs; + + *stainfos = lappend(*stainfos, info); + } + + ReleaseSysCache(dtup); + bms_free(keys); } /* -- 2.17.0