From 32f2fe04e43f57cdff4f0adec0af6d7bdb909e75 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 10 Nov 2026 13:43:27 -0500 Subject: [PATCH v4] Store empty stavalues when needed for expression statistics. Commit 261f89a97 changed the API contract for update_attstats() to store an empty array, not a NULL, if it's passed a nonnull stavalues pointer along with a zero values count. We missed that serialize_expr_stats() needs to be kept in sync with that, so that we now get "unexpected null value" errors if statistics on an expression hit one of the cases where the statistics collection code requests that. Rather than just adjusting serialize_expr_stats() to match, let's avoid having duplicated code at all, by factoring out a routine to build a pg_statistic tuple from a VacAttrStats structure. Bug: #19849 Reported-by: Atish Prakash <125atishprak@gmail.com> Author: Atish Prakash <125atishprak@gmail.com> Co-authored-by: Shihao Zhong Co-authored-by: Tom Lane Discussion: https://postgr.es/m/19849-38b8bd2fea807154@postgresql.org Backpatch-through: 19 --- src/backend/commands/analyze.c | 201 ++++++++++++++---------- src/backend/statistics/extended_stats.c | 84 +--------- src/include/commands/vacuum.h | 4 + 3 files changed, 122 insertions(+), 167 deletions(-) diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 518d7526255..01efa78cf76 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -1713,6 +1713,115 @@ acquire_inherited_sample_rows(Relation onerel, int elevel, } +/* + * build_statistic_tuple() -- build a pg_statistic tuple from a VacAttrStats + * + * relid is the Oid of the table being analyzed, inh indicates if these are + * inheritance-tree or table-only stats, stats contains the collected stats + * including the column number. + * + * tupdesc must be a tuple descriptor for pg_statistic. oldtup is the + * existing pg_statistic tuple we intend to replace, or NULL to build + * a new pg_statistic tuple. + * + * This is exported so that extended_stats.c can use it. (In that usage, + * relid is InvalidOid, so don't trust it for anything.) + */ +HeapTuple +build_statistic_tuple(Oid relid, bool inh, + VacAttrStats *stats, + TupleDesc tupdesc, + HeapTuple oldtup) +{ + HeapTuple stup; + Datum values[Natts_pg_statistic]; + bool nulls[Natts_pg_statistic]; + bool replaces[Natts_pg_statistic]; + int i, + k; + + for (i = 0; i < Natts_pg_statistic; ++i) + { + nulls[i] = false; + replaces[i] = true; + } + + values[Anum_pg_statistic_starelid - 1] = ObjectIdGetDatum(relid); + values[Anum_pg_statistic_staattnum - 1] = Int16GetDatum(stats->tupattnum); + values[Anum_pg_statistic_stainherit - 1] = BoolGetDatum(inh); + values[Anum_pg_statistic_stanullfrac - 1] = Float4GetDatum(stats->stanullfrac); + values[Anum_pg_statistic_stawidth - 1] = Int32GetDatum(stats->stawidth); + values[Anum_pg_statistic_stadistinct - 1] = Float4GetDatum(stats->stadistinct); + i = Anum_pg_statistic_stakind1 - 1; + for (k = 0; k < STATISTIC_NUM_SLOTS; k++) + { + values[i++] = Int16GetDatum(stats->stakind[k]); /* stakindN */ + } + i = Anum_pg_statistic_staop1 - 1; + for (k = 0; k < STATISTIC_NUM_SLOTS; k++) + { + values[i++] = ObjectIdGetDatum(stats->staop[k]); /* staopN */ + } + i = Anum_pg_statistic_stacoll1 - 1; + for (k = 0; k < STATISTIC_NUM_SLOTS; k++) + { + values[i++] = ObjectIdGetDatum(stats->stacoll[k]); /* stacollN */ + } + i = Anum_pg_statistic_stanumbers1 - 1; + for (k = 0; k < STATISTIC_NUM_SLOTS; k++) + { + if (stats->stanumbers[k] != NULL) + { + int nnum = stats->numnumbers[k]; + Datum *numdatums = palloc_array(Datum, nnum); + ArrayType *arry; + + for (int n = 0; n < nnum; n++) + numdatums[n] = Float4GetDatum(stats->stanumbers[k][n]); + arry = construct_array_builtin(numdatums, nnum, FLOAT4OID); + values[i++] = PointerGetDatum(arry); /* stanumbersN */ + } + else + { + nulls[i] = true; + values[i++] = (Datum) 0; + } + } + i = Anum_pg_statistic_stavalues1 - 1; + for (k = 0; k < STATISTIC_NUM_SLOTS; k++) + { + if (stats->stavalues[k] != NULL) + { + ArrayType *arry; + + arry = construct_array(stats->stavalues[k], + stats->numvalues[k], + stats->statypid[k], + stats->statyplen[k], + stats->statypbyval[k], + stats->statypalign[k]); + values[i++] = PointerGetDatum(arry); /* stavaluesN */ + } + else + { + nulls[i] = true; + values[i++] = (Datum) 0; + } + } + + /* Build a replacement or all-new tuple */ + if (HeapTupleIsValid(oldtup)) + stup = heap_modify_tuple(oldtup, + tupdesc, + values, + nulls, + replaces); + else + stup = heap_form_tuple(tupdesc, values, nulls); + + return stup; +} + /* * update_attstats() -- update attribute statistics for one relation * @@ -1752,88 +1861,14 @@ update_attstats(Oid relid, bool inh, int natts, VacAttrStats **vacattrstats) VacAttrStats *stats = vacattrstats[attno]; HeapTuple stup, oldtup; - int i, - k, - n; - Datum values[Natts_pg_statistic]; - bool nulls[Natts_pg_statistic]; - bool replaces[Natts_pg_statistic]; /* Ignore attr if we weren't able to collect stats */ if (!stats->stats_valid) continue; - /* - * Construct a new pg_statistic tuple - */ - for (i = 0; i < Natts_pg_statistic; ++i) - { - nulls[i] = false; - replaces[i] = true; - } - - values[Anum_pg_statistic_starelid - 1] = ObjectIdGetDatum(relid); - values[Anum_pg_statistic_staattnum - 1] = Int16GetDatum(stats->tupattnum); - values[Anum_pg_statistic_stainherit - 1] = BoolGetDatum(inh); - values[Anum_pg_statistic_stanullfrac - 1] = Float4GetDatum(stats->stanullfrac); - values[Anum_pg_statistic_stawidth - 1] = Int32GetDatum(stats->stawidth); - values[Anum_pg_statistic_stadistinct - 1] = Float4GetDatum(stats->stadistinct); - i = Anum_pg_statistic_stakind1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - values[i++] = Int16GetDatum(stats->stakind[k]); /* stakindN */ - } - i = Anum_pg_statistic_staop1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - values[i++] = ObjectIdGetDatum(stats->staop[k]); /* staopN */ - } - i = Anum_pg_statistic_stacoll1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - values[i++] = ObjectIdGetDatum(stats->stacoll[k]); /* stacollN */ - } - i = Anum_pg_statistic_stanumbers1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - if (stats->stanumbers[k] != NULL) - { - int nnum = stats->numnumbers[k]; - Datum *numdatums = palloc_array(Datum, nnum); - ArrayType *arry; - - for (n = 0; n < nnum; n++) - numdatums[n] = Float4GetDatum(stats->stanumbers[k][n]); - arry = construct_array_builtin(numdatums, nnum, FLOAT4OID); - values[i++] = PointerGetDatum(arry); /* stanumbersN */ - } - else - { - nulls[i] = true; - values[i++] = (Datum) 0; - } - } - i = Anum_pg_statistic_stavalues1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - if (stats->stavalues[k] != NULL) - { - ArrayType *arry; - - arry = construct_array(stats->stavalues[k], - stats->numvalues[k], - stats->statypid[k], - stats->statyplen[k], - stats->statypbyval[k], - stats->statypalign[k]); - values[i++] = PointerGetDatum(arry); /* stavaluesN */ - } - else - { - nulls[i] = true; - values[i++] = (Datum) 0; - } - } + /* Open index information when we know we need it */ + if (indstate == NULL) + indstate = CatalogOpenIndexes(sd); /* Is there already a pg_statistic tuple for this attribute? */ oldtup = SearchSysCache3(STATRELATTINH, @@ -1841,25 +1876,19 @@ update_attstats(Oid relid, bool inh, int natts, VacAttrStats **vacattrstats) Int16GetDatum(stats->tupattnum), BoolGetDatum(inh)); - /* Open index information when we know we need it */ - if (indstate == NULL) - indstate = CatalogOpenIndexes(sd); + /* Build the new or replacement tuple */ + stup = build_statistic_tuple(relid, inh, stats, + RelationGetDescr(sd), oldtup); if (HeapTupleIsValid(oldtup)) { /* Yes, replace it */ - stup = heap_modify_tuple(oldtup, - RelationGetDescr(sd), - values, - nulls, - replaces); ReleaseSysCache(oldtup); CatalogTupleUpdateWithInfo(sd, &stup->t_self, stup, indstate); } else { /* No, insert new tuple */ - stup = heap_form_tuple(RelationGetDescr(sd), values, nulls); CatalogTupleInsertWithInfo(sd, stup, indstate); } diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index b66a49a6fb5..da5e7264d25 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -2333,14 +2333,13 @@ build_expr_data(List *exprs, int stattarget) return exprdata; } -/* form an array of pg_statistic rows (per update_attstats) */ +/* form an array of pg_statistic rows, one per expression */ static Datum serialize_expr_stats(AnlExprData *exprdata, int nexprs) { int exprno; Oid typOid; Relation sd; - ArrayBuildState *astate = NULL; sd = table_open(StatisticRelationId, RowExclusiveLock); @@ -2355,12 +2354,7 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs) for (exprno = 0; exprno < nexprs; exprno++) { - int i, - k; VacAttrStats *stats = exprdata[exprno].vacattrstat; - - Datum values[Natts_pg_statistic]; - bool nulls[Natts_pg_statistic]; HeapTuple stup; if (!stats->stats_valid) @@ -2373,80 +2367,8 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs) continue; } - /* - * Construct a new pg_statistic tuple - */ - for (i = 0; i < Natts_pg_statistic; ++i) - { - nulls[i] = false; - } - - values[Anum_pg_statistic_starelid - 1] = ObjectIdGetDatum(InvalidOid); - values[Anum_pg_statistic_staattnum - 1] = Int16GetDatum(InvalidAttrNumber); - values[Anum_pg_statistic_stainherit - 1] = BoolGetDatum(false); - values[Anum_pg_statistic_stanullfrac - 1] = Float4GetDatum(stats->stanullfrac); - values[Anum_pg_statistic_stawidth - 1] = Int32GetDatum(stats->stawidth); - values[Anum_pg_statistic_stadistinct - 1] = Float4GetDatum(stats->stadistinct); - i = Anum_pg_statistic_stakind1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - values[i++] = Int16GetDatum(stats->stakind[k]); /* stakindN */ - } - i = Anum_pg_statistic_staop1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - values[i++] = ObjectIdGetDatum(stats->staop[k]); /* staopN */ - } - i = Anum_pg_statistic_stacoll1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - values[i++] = ObjectIdGetDatum(stats->stacoll[k]); /* stacollN */ - } - i = Anum_pg_statistic_stanumbers1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - int nnum = stats->numnumbers[k]; - - if (nnum > 0) - { - int n; - Datum *numdatums = palloc_array(Datum, nnum); - ArrayType *arry; - - for (n = 0; n < nnum; n++) - numdatums[n] = Float4GetDatum(stats->stanumbers[k][n]); - arry = construct_array_builtin(numdatums, nnum, FLOAT4OID); - values[i++] = PointerGetDatum(arry); /* stanumbersN */ - } - else - { - nulls[i] = true; - values[i++] = (Datum) 0; - } - } - i = Anum_pg_statistic_stavalues1 - 1; - for (k = 0; k < STATISTIC_NUM_SLOTS; k++) - { - if (stats->numvalues[k] > 0) - { - ArrayType *arry; - - arry = construct_array(stats->stavalues[k], - stats->numvalues[k], - stats->statypid[k], - stats->statyplen[k], - stats->statypbyval[k], - stats->statypalign[k]); - values[i++] = PointerGetDatum(arry); /* stavaluesN */ - } - else - { - nulls[i] = true; - values[i++] = (Datum) 0; - } - } - - stup = heap_form_tuple(RelationGetDescr(sd), values, nulls); + stup = build_statistic_tuple(InvalidOid, false, stats, + RelationGetDescr(sd), NULL); astate = accumArrayResult(astate, heap_copy_tuple_as_datum(stup, RelationGetDescr(sd)), diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 89fa1fcd261..3bcb7113762 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -441,6 +441,10 @@ extern void analyze_rel(Oid relid, RangeVar *relation, BufferAccessStrategy bstrategy); extern bool attribute_is_analyzable(Relation onerel, int attnum, Form_pg_attribute attr, int *p_attstattarget); +extern HeapTuple build_statistic_tuple(Oid relid, bool inh, + VacAttrStats *stats, + TupleDesc tupdesc, + HeapTuple oldtup); extern bool std_typanalyze(VacAttrStats *stats); /* in utils/misc/sampling.c --- duplicate of declarations in utils/sampling.h */ -- 2.52.0