From f511d13c1e841ff63e0a5a25b6c9603964576b64 Mon Sep 17 00:00:00 2001 From: Jianghua Yang Date: Sat, 5 Sep 2026 02:09:51 +0800 Subject: [PATCH] fix(orca): merge colid->attno mapping in CStatistics::AppendStats to avoid extended-stats crash When a memo group's cached statistics lack columns needed by a later derivation request (e.g. a ROLLUP expanded into a CTE whose consumers request different column sets), the missing columns are derived separately and merged via CStatistics::AppendStats(). That merge copied histograms and widths only, so the colid -> attno mapping (added for extended statistics) diverged from the histogram map: a column could have a histogram but no attno entry. ApplyCorrelatedStatsToScaleFactorFilterCalculation() then dereferenced the result of the attno lookup without a null check and crashed the coordinator with SIGSEGV whenever the table had any extended statistics object with stored data and a filter referenced one of the appended columns (e.g. TPC-DS Q67 after CREATE STATISTICS (ndistinct) on item/date_dim). Reproduced on main with a cassert build: colid 11 had a histogram (5 entries) but the attno map only held 4 entries, attnum was NULL at CExtendedStatsProcessor.cpp:280. This is the same class of bug as the CTE remapping fix in 124a048e8c6 (gpdb#16212), which repaired CopyStatsWithRemap() but not AppendStats(). Fix both layers: * CStatistics::AppendStats() now also merges the colid -> attno mapping (copy-on-write, since the mapping is shared by refcount across stats objects, see ScaleStats()) and adopts the input's extended-stats info when none is set, restoring the invariant that every column with a histogram has an attno entry. * CExtendedStatsProcessor now null-checks the attno and histogram lookups and skips clauses it cannot translate, falling back to the independence assumption, matching what ApplyCorrelatedStatsToNDistinctCalculation() already does. Also bail out gracefully if no ndistinct item matches the attribute set. * CExtendedStatsProcessor also skips system columns (attno <= 0): extended statistics never cover them, and feeding a negative attno into CBitSet::ExchangeSet() would convert it to a huge unsigned bit index. The backend rejects these clauses too (dependency_is_compatible_clause()). Add a regression test with the minimized reproduction: a ROLLUP over a join where the dimension table has ndistinct extended statistics and the filter column is appended to the scan group's stats after the initial derivation. Verified the gporca test passes with both optimizer=on and optimizer=off answer files. Ported from warehouse-pg/warehouse-pg#251. Co-authored-by: Junfeng Yang --- .../naucrates/statistics/CStatistics.h | 4 + .../statistics/CExtendedStatsProcessor.cpp | 76 ++++++++++++++++++- .../src/statistics/CFilterStatsProcessor.cpp | 11 ++- .../src/statistics/CStatistics.cpp | 48 ++++++++++++ src/test/regress/expected/gporca.out | 26 +++++++ .../regress/expected/gporca_optimizer.out | 26 +++++++ src/test/regress/sql/gporca.sql | 22 ++++++ 7 files changed, 210 insertions(+), 3 deletions(-) diff --git a/src/backend/gporca/libnaucrates/include/naucrates/statistics/CStatistics.h b/src/backend/gporca/libnaucrates/include/naucrates/statistics/CStatistics.h index 3dea38b58d7..50ef54a0a47 100644 --- a/src/backend/gporca/libnaucrates/include/naucrates/statistics/CStatistics.h +++ b/src/backend/gporca/libnaucrates/include/naucrates/statistics/CStatistics.h @@ -153,6 +153,10 @@ class CStatistics : public IStatistics, public DbgPrintMixin UlongToColRefMap *colref_mapping, BOOL must_exist); + // helper method to add attno information without remapping + static void AddAttnoInfo(CMemoryPool *mp, UlongToIntMap *src_attno, + UlongToIntMap *dest_attno); + public: CStatistics &operator=(CStatistics &) = delete; diff --git a/src/backend/gporca/libnaucrates/src/statistics/CExtendedStatsProcessor.cpp b/src/backend/gporca/libnaucrates/src/statistics/CExtendedStatsProcessor.cpp index 97bcfc1ed7b..554bc06c28b 100644 --- a/src/backend/gporca/libnaucrates/src/statistics/CExtendedStatsProcessor.cpp +++ b/src/backend/gporca/libnaucrates/src/statistics/CExtendedStatsProcessor.cpp @@ -34,9 +34,31 @@ using namespace gpopt; static BOOL IsDependencyCapablePredicate(CStatsPred *child_pred GPOS_UNUSED) { + /* + * TODO: EsptPoint covers all constant comparisons (Eq/G/GEq/L/LEq, see + * CStatsPredUtils::StatsCmpType()), so range predicates are let into + * functional-dependency estimation where they get equality semantics. + * The backend analogue, dependency_is_compatible_clause(), accepts + * equality-to-pseudoconstant only; this should gate on the point + * predicate's comparison type being EstatscmptEq. + */ return child_pred->GetPredStatsType() == CStatsPred::EsptPoint; } +/* + * A colid -> attno mapping entry is usable for extended-statistics estimation + * only if it exists and refers to a user column. Unlike the backend, ORCA has + * to translate its column ids back to attnos and the mapping is not + * guaranteed to cover every column. System columns (attno <= 0) are never + * covered by extended statistics and cannot be represented in a CBitSet; the + * backend rejects them too (dependency_is_compatible_clause()). + */ +static BOOL +FUsableAttno(const INT *attnum) +{ + return nullptr != attnum && 0 < *attnum; +} + /* * choose_best_statistics * Look for and return statistics with the specified 'requiredkind' which @@ -277,6 +299,14 @@ CExtendedStatsProcessor::ApplyCorrelatedStatsToScaleFactorFilterCalculation( { ULONG colid = child_pred->GetColId(); INT *attnum = colid_to_attno_mapping->Find(&colid); + if (!FUsableAttno(attnum)) + { + /* + * Skip such clauses; they fall back to the independence + * assumption. + */ + continue; + } clauses_attnums->ExchangeSet(*attnum); } } @@ -353,6 +383,12 @@ CExtendedStatsProcessor::ApplyCorrelatedStatsToScaleFactorFilterCalculation( ULONG colid = child_pred->GetColId(); INT *attnum = colid_to_attno_mapping->Find(&colid); + if (!FUsableAttno(attnum)) + { + /* no usable attno for this clause; it was never collected + * into clauses_attnums by the pre-processing loop above */ + continue; + } /* * Technically we could find more than one clause for a given @@ -365,7 +401,24 @@ CExtendedStatsProcessor::ApplyCorrelatedStatsToScaleFactorFilterCalculation( */ if (dependency_implies_attribute(dependency, *attnum)) { - s2 = 1 / result_histograms->Find(&colid)->GetFrequency().Get(); + const CHistogram *histogram = result_histograms->Find(&colid); + if (nullptr == histogram) + { + /* + * No histogram to estimate the implied clause with; give + * up on the dependency for this attribute. The attnum + * bit must still be cleared: the outer loop terminates + * only once no dependency is fully matched by the + * remaining attnums, so leaving the bit set would make + * find_strongest_dependency() return the same dependency + * forever. The clause stays unestimated here and is later + * given the default selectivity by the regular per-column + * path (see MakeHistHashMapConjFilter()). + */ + clauses_attnums->ExchangeClear(*attnum); + continue; + } + s2 = 1 / histogram->GetFrequency().Get(); /* mark this one as done, so we don't touch it again. */ child_pred->SetEstimated(); @@ -428,11 +481,22 @@ CExtendedStatsProcessor::ApplyCorrelatedStatsToNDistinctCalculation( ULONG colid = *(*src_grouping_cols)[ul]; INT *attnum = colid_to_attno_mapping->Find(&colid); - if (!attnum) + if (nullptr == attnum) { + /* no colid -> attno mapping; extended stats are unusable */ attnums->Release(); return false; } + if (0 >= *attnum) + { + /* + * System column: extended statistics never cover it and CBitSet + * cannot represent it. Skip just this column; it never enters + * 'attnums', so it stays unmatched below and is kept for the + * regular per-column ndistinct path. + */ + continue; + } attnums->ExchangeSet(*attnum); } @@ -514,6 +578,14 @@ CExtendedStatsProcessor::ApplyCorrelatedStatsToNDistinctCalculation( } } + if (nullptr == item) + { + /* there should be an item for every attribute combination */ + matched->Release(); + attnums->Release(); + return false; + } + /* Form the output varinfo list, keeping only unmatched ones */ ULongPtrArray *new_src_grouping_cols = GPOS_NEW(mp) ULongPtrArray(mp); for (ULONG ul = 0; ul < src_grouping_cols->Size(); ul++) diff --git a/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp b/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp index a7e458ec353..ee76fd93216 100644 --- a/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp +++ b/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp @@ -375,7 +375,16 @@ CFilterStatsProcessor::MakeHistHashMapConjFilter( if (CStatsPred::EsptDisj != child_pred_stats->GetPredStatsType()) { GPOS_ASSERT(gpos::ulong_max != colid); - hist_before = result_histograms->Find(&colid)->CopyHistogram(); + const CHistogram *base_histogram = result_histograms->Find(&colid); + if (nullptr == base_histogram) + { + // no histogram for the filter column; estimate the clause + // with the default selectivity instead of dereferencing null + scale_factors->Append(GPOS_NEW(mp) CDouble( + 1 / CHistogram::DefaultSelectivity.Get())); + continue; + } + hist_before = base_histogram->CopyHistogram(); GPOS_ASSERT(nullptr != hist_before); CHistogram *result_histogram = nullptr; diff --git a/src/backend/gporca/libnaucrates/src/statistics/CStatistics.cpp b/src/backend/gporca/libnaucrates/src/statistics/CStatistics.cpp index 80d3b289497..3f40e8a073e 100644 --- a/src/backend/gporca/libnaucrates/src/statistics/CStatistics.cpp +++ b/src/backend/gporca/libnaucrates/src/statistics/CStatistics.cpp @@ -552,6 +552,33 @@ CStatistics::AppendStats(CMemoryPool *mp, IStatistics *input_stats) CStatisticsUtils::AddWidthInfo(mp, stats->m_colid_width_mapping, m_colid_width_mapping); GPOS_CHECK_ABORT; + + // Also merge the colid -> attno mapping so that every column that has a + // histogram keeps an attno entry; extended-stats estimation relies on + // this invariant. The mapping may be shared with other stats objects + // (see ScaleStats()), so merge copy-on-write instead of mutating it. + if (0 < stats->m_colid_to_attno_mapping->Size()) + { + UlongToIntMap *attnos_new = GPOS_NEW(mp) UlongToIntMap(mp); + AddAttnoInfo(mp, m_colid_to_attno_mapping, attnos_new); + AddAttnoInfo(mp, stats->m_colid_to_attno_mapping, attnos_new); + m_colid_to_attno_mapping->Release(); + m_colid_to_attno_mapping = attnos_new; + } + + // Attnos are only meaningful relative to m_ext_stats' base relation, so + // merging is only valid for stats objects describing the same relation; + // all callers merge stats of the same memo group / expression today. + GPOS_ASSERT(nullptr == m_ext_stats || nullptr == stats->m_ext_stats || + m_ext_stats == stats->m_ext_stats); + + if (nullptr == m_ext_stats) + { + // non-owning pointer, kept alive by the MD cache (never released by + // ~CStatistics and never AddRef'd) + m_ext_stats = stats->m_ext_stats; + } + GPOS_CHECK_ABORT; } // copy statistics object @@ -776,6 +803,27 @@ CStatistics::AddAttnoInfoWithRemap(CMemoryPool *mp, UlongToIntMap *src_attno, } } +// add attno information without remapping +void +CStatistics::AddAttnoInfo(CMemoryPool *mp, UlongToIntMap *src_attno, + UlongToIntMap *dest_attno) +{ + UlongToIntMapIter col_attno_map_iterator(src_attno); + while (col_attno_map_iterator.Advance()) + { + ULONG colid = *(col_attno_map_iterator.Key()); + + if (nullptr == dest_attno->Find(&colid)) + { + const INT *attno = col_attno_map_iterator.Value(); + INT *attno_copy = GPOS_NEW(mp) INT(*attno); + BOOL result GPOS_ASSERTS_ONLY = + dest_attno->Insert(GPOS_NEW(mp) ULONG(colid), attno_copy); + GPOS_ASSERT(result); + } + } +} + // return the index of the array of upper bound ndvs to which column reference belongs ULONG CStatistics::GetIndexUpperBoundNDVs(const CColRef *colref) diff --git a/src/test/regress/expected/gporca.out b/src/test/regress/expected/gporca.out index f77b935cb9a..a8d2736b323 100644 --- a/src/test/regress/expected/gporca.out +++ b/src/test/regress/expected/gporca.out @@ -15050,6 +15050,32 @@ select * from cte2 inner join cte_test3 on cte2.c = cte_test3.a; drop table cte_test1; drop table cte_test2; drop table cte_test3; +-- Filter estimation with extended statistics must not crash when the memo +-- group's cached stats were extended incrementally (histograms appended for +-- columns that the colid -> attno mapping initially lacked). The ROLLUP is +-- expanded into a CTE whose consumers request different column sets, so the +-- stats of the dimension table's scan group get appended with the filter +-- column after the initial derivation. +create table extstats_dim (d_sk int, d_filter int, d_a int, d_b int, d_c int) distributed by (d_sk); +create table extstats_fact (f_sk int, f_val int) distributed by (f_sk); +insert into extstats_dim select g, g % 20, g % 3, g % 4, g % 5 from generate_series(1, 100) g; +insert into extstats_fact select g % 100 + 1, g from generate_series(1, 1000) g; +create statistics extstats_dim_nd (ndistinct) on d_a, d_b, d_c from extstats_dim; +analyze extstats_dim; +analyze extstats_fact; +set optimizer = on; +select count(*) from ( + select d_a, d_b, d_c, sum(f_val) s + from extstats_fact, extstats_dim + where f_sk = d_sk and d_filter between 5 and 10 + group by rollup(d_a, d_b, d_c)) x; + count +------- + 34 +(1 row) + +reset optimizer; +drop table extstats_fact, extstats_dim; -- start_ignore DROP SCHEMA orca CASCADE; NOTICE: drop cascades to 190 other objects diff --git a/src/test/regress/expected/gporca_optimizer.out b/src/test/regress/expected/gporca_optimizer.out index b8b83e01ab2..11c6111ec2d 100644 --- a/src/test/regress/expected/gporca_optimizer.out +++ b/src/test/regress/expected/gporca_optimizer.out @@ -15085,6 +15085,32 @@ select * from cte2 inner join cte_test3 on cte2.c = cte_test3.a; drop table cte_test1; drop table cte_test2; drop table cte_test3; +-- Filter estimation with extended statistics must not crash when the memo +-- group's cached stats were extended incrementally (histograms appended for +-- columns that the colid -> attno mapping initially lacked). The ROLLUP is +-- expanded into a CTE whose consumers request different column sets, so the +-- stats of the dimension table's scan group get appended with the filter +-- column after the initial derivation. +create table extstats_dim (d_sk int, d_filter int, d_a int, d_b int, d_c int) distributed by (d_sk); +create table extstats_fact (f_sk int, f_val int) distributed by (f_sk); +insert into extstats_dim select g, g % 20, g % 3, g % 4, g % 5 from generate_series(1, 100) g; +insert into extstats_fact select g % 100 + 1, g from generate_series(1, 1000) g; +create statistics extstats_dim_nd (ndistinct) on d_a, d_b, d_c from extstats_dim; +analyze extstats_dim; +analyze extstats_fact; +set optimizer = on; +select count(*) from ( + select d_a, d_b, d_c, sum(f_val) s + from extstats_fact, extstats_dim + where f_sk = d_sk and d_filter between 5 and 10 + group by rollup(d_a, d_b, d_c)) x; + count +------- + 34 +(1 row) + +reset optimizer; +drop table extstats_fact, extstats_dim; -- start_ignore DROP SCHEMA orca CASCADE; NOTICE: drop cascades to 190 other objects diff --git a/src/test/regress/sql/gporca.sql b/src/test/regress/sql/gporca.sql index 29481d859dc..df61a968664 100644 --- a/src/test/regress/sql/gporca.sql +++ b/src/test/regress/sql/gporca.sql @@ -3762,6 +3762,28 @@ drop table cte_test1; drop table cte_test2; drop table cte_test3; +-- Filter estimation with extended statistics must not crash when the memo +-- group's cached stats were extended incrementally (histograms appended for +-- columns that the colid -> attno mapping initially lacked). The ROLLUP is +-- expanded into a CTE whose consumers request different column sets, so the +-- stats of the dimension table's scan group get appended with the filter +-- column after the initial derivation. +create table extstats_dim (d_sk int, d_filter int, d_a int, d_b int, d_c int) distributed by (d_sk); +create table extstats_fact (f_sk int, f_val int) distributed by (f_sk); +insert into extstats_dim select g, g % 20, g % 3, g % 4, g % 5 from generate_series(1, 100) g; +insert into extstats_fact select g % 100 + 1, g from generate_series(1, 1000) g; +create statistics extstats_dim_nd (ndistinct) on d_a, d_b, d_c from extstats_dim; +analyze extstats_dim; +analyze extstats_fact; +set optimizer = on; +select count(*) from ( + select d_a, d_b, d_c, sum(f_val) s + from extstats_fact, extstats_dim + where f_sk = d_sk and d_filter between 5 and 10 + group by rollup(d_a, d_b, d_c)) x; +reset optimizer; +drop table extstats_fact, extstats_dim; + -- start_ignore DROP SCHEMA orca CASCADE; -- end_ignore