From 773ce509f4c9fb4894b7e5ebfb2692bd87788585 Mon Sep 17 00:00:00 2001 From: Jianghua Yang Date: Sat, 5 Sep 2026 02:09:51 +0800 Subject: [PATCH 1/3] 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 From 657484102a9aad0d7f9e60a531154adc8c7d7ac2 Mon Sep 17 00:00:00 2001 From: Jianghua Yang Date: Tue, 29 Sep 2026 00:06:36 +0800 Subject: [PATCH 2/3] fix(orca): guard missing histograms in disjunctive filter stats as well Follow-up to the null-histogram fallback in MakeHistHashMapConjFilter(): apply the same treatment in MakeHistHashMapDisjFilter() so that a filter column without a histogram, e.g. in WHERE (c2 > 0 OR c1 = 'x'), is estimated with the default selectivity instead of dereferencing null. * A simple predicate whose column has no histogram is skipped before the per-column bookkeeping, like an unsupported predicate, so the previous column's intermediate result is not flushed twice. * A nested conjunction/disjunction on a single column whose histogram is missing from the child result falls back to the multi-column merge path instead of copying a null histogram. * The conjunction path only replaces the column histogram after a single-column disjunction when the disjunction produced one. Addresses review feedback on #1966. --- .../src/statistics/CFilterStatsProcessor.cpp | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp b/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp index ee76fd93216..07892b03005 100644 --- a/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp +++ b/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp @@ -447,9 +447,15 @@ CFilterStatsProcessor::MakeHistHashMapConjFilter( { CHistogram *result_histogram = disjunctive_histograms_after->Find(&colid); - CStatisticsUtils::AddHistogram(mp, colid, result_histogram, - result_histograms, - true /* fReplaceOld */); + // the column may have no histogram at all (see + // MakeHistHashMapDisjFilter()); then there is nothing to + // replace and the scale factor alone carries the estimate + if (nullptr != result_histogram) + { + CStatisticsUtils::AddHistogram(mp, colid, result_histogram, + result_histograms, + true /* fReplaceOld */); + } disjunctive_histograms_after->Release(); last_scale_factor = @@ -534,6 +540,22 @@ CFilterStatsProcessor::MakeHistHashMapDisjFilter( continue; } + BOOL is_pred_simple = + !CStatsPredUtils::IsConjOrDisjPred(child_pred_stats); + CHistogram *histogram = input_histograms->Find(&colid); + if (is_pred_simple && nullptr == histogram) + { + // no histogram for the filter column; estimate the clause with + // the default selectivity instead of dereferencing null. Do this + // before the per-column bookkeeping below, like the unsupported + // predicate case, so the previous column's intermediate result + // is not flushed twice. + scale_factors->Append(GPOS_NEW(mp) CDouble( + 1 / CHistogram::DefaultSelectivity.Get())); + + continue; + } + if (IsNewStatsColumn(colid, previous_colid)) { scale_factors->Append(GPOS_NEW(mp) @@ -545,11 +567,8 @@ CFilterStatsProcessor::MakeHistHashMapDisjFilter( previous_histogram = nullptr; } - CHistogram *histogram = input_histograms->Find(&colid); CHistogram *disjunctive_child_col_histogram = nullptr; - BOOL is_pred_simple = - !CStatsPredUtils::IsConjOrDisjPred(child_pred_stats); BOOL is_colid_present = (gpos::ulong_max != colid); UlongToHistogramMap *child_histograms = nullptr; CDouble child_scale_factor(1.0); @@ -583,8 +602,20 @@ CFilterStatsProcessor::MakeHistHashMapDisjFilter( if (is_colid_present) { // conjunction or disjunction uses only a single column - disjunctive_child_col_histogram = - child_histograms->Find(&colid)->CopyHistogram(); + const CHistogram *child_col_histogram = + child_histograms->Find(&colid); + if (nullptr != child_col_histogram) + { + disjunctive_child_col_histogram = + child_col_histogram->CopyHistogram(); + } + else + { + // the column has no histogram, so the child left none + // behind; treat the child like a multi-column predicate + // and only merge its histogram map and scale factor + is_colid_present = false; + } } } From a1af361792b8b51d0f21007fe61c2555bfc60cb0 Mon Sep 17 00:00:00 2001 From: Jianghua Yang Date: Thu, 22 Oct 2026 04:35:21 +0800 Subject: [PATCH 3/3] fix(orca): flush pending column state when a disjunct child leaves no histogram The null-histogram fallback added for MakeHistHashMapDisjFilter() routes the child to the multi-column merge path but kept the real column id. That path is otherwise only reached with colid == gpos::ulong_max, which makes IsNewStatsColumn() true at the top of the loop, so it assumes the pending per-column state has already been flushed. With a real column id left in place: * a later disjunct on the same column found previous_colid == colid, so the pending scale factor was never appended to scale_factors and was overwritten instead of accumulated, making the OR estimate depend on branch order; * the pending previous_histogram was dropped on the floor by the path's 'previous_histogram = nullptr', losing its contribution to the disjunction and leaking it. Flush the pending histogram and scale factor in the fallback, then reset colid to gpos::ulong_max so the invariant the multi-column path relies on holds again. Reported-by: Alena Rybakina --- .../src/statistics/CFilterStatsProcessor.cpp | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp b/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp index 07892b03005..ff5bfb77c73 100644 --- a/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp +++ b/src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp @@ -613,8 +613,28 @@ CFilterStatsProcessor::MakeHistHashMapDisjFilter( { // the column has no histogram, so the child left none // behind; treat the child like a multi-column predicate - // and only merge its histogram map and scale factor + // and only merge its histogram map and scale factor. + // + // The multi-column path below is otherwise only reached + // with colid == gpos::ulong_max, which makes + // IsNewStatsColumn() true at the top of the loop, so it + // assumes the pending per-column state has already been + // flushed. Flush it here and forget the column id too, + // otherwise the pending histogram would be dropped and + // a later child on the same column would overwrite this + // child's scale factor instead of accumulating it. + if (nullptr != previous_histogram) + { + scale_factors->Append( + GPOS_NEW(mp) CDouble(previous_scale_factor.Get())); + CStatisticsUtils::UpdateDisjStatistics( + mp, non_updatable_cols, input_rows, cumulative_rows, + previous_histogram, disjunctive_result_histograms, + previous_colid); + previous_histogram = nullptr; + } is_colid_present = false; + colid = gpos::ulong_max; } } }