fix(orca): never build index conditions on INCLUDE-only index columns - #1957
Open
yjhjstz wants to merge 1 commit into
Open
fix(orca): never build index conditions on INCLUDE-only index columns#1957yjhjstz wants to merge 1 commit into
yjhjstz wants to merge 1 commit into
Conversation
Since 37224a3 CXformUtils::PdrgpcrIndexKeys() has been a thin wrapper around PdrgpcrIndexColumns(), which appends the index's INCLUDE columns after its key columns. CPredicateUtils::ExtractIndexPredicates() therefore treated a predicate on an INCLUDE-only column as indexable. Ordinary comparisons were still rejected later by CMDIndexGPDB::IsCompatible(), but the shortcuts for boolean column references (col -> col = true, NOT col -> col = false) and for IS [NOT] NULL bypass that check and went straight into the index condition list. The DXL-to-plan translator then maps the column through GetKeyPos(), which returns ulong_max for a non-key column, so the Var's varattno wrapped to 0 and execution failed with ERROR: bogus index qualification (nodeIndexscan.c:1250) or, for IS [NOT] NULL, ERROR: btree index keys must be ordered by attribute (nbtutils.c:777) Restore the key-only semantics: reintroduce the EIndexCols selector (EicKey / EicKeyAndIncluded) on PdrgpcrIndexColumns()/PcrsIndexColumns() and make PdrgpcrIndexKeys()/PcrsIndexKeys() request key columns only. The cost model keeps asking for key + INCLUDE columns so that ComputeUnusedIndexWeight() costing is unchanged. Index-only-scan coverage checks are unaffected: they already use PcrsIndexReturnableColumns(). Add regression coverage to gp_covering_index. Fixes apache#1948
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core ORCA index predicate extraction and costing behavior and should receive a final domain-expert review despite the added regression coverage.
Pull request overview
This PR fixes an ORCA planning bug where predicates on INCLUDE-only index columns could incorrectly become index quals, leading to executor failures (e.g., “bogus index qualification”), and adds regression coverage for the affected predicate forms.
Changes:
- Reintroduce a key-only vs key+INCLUDE column selector in
CXformUtilsso index-qual construction only considers key columns. - Keep cost model behavior unchanged by explicitly requesting key+INCLUDE columns for unused-index-column weighting.
- Add regression tests (and expected outputs) covering boolean column refs and
IS [NOT] NULLpredicates on INCLUDE-only columns.
File summaries
| File | Description |
|---|---|
| src/backend/gporca/libgpopt/src/xforms/CXformUtils.cpp | Make index-key APIs key-only and add selectable index column retrieval (key vs key+INCLUDE). |
| src/backend/gporca/libgpopt/include/gpopt/xforms/CXformUtils.h | Add EIndexCols selector and update function signatures/docs accordingly. |
| src/backend/gporca/libgpdbcost/src/CCostModelGPDB.cpp | Request key+INCLUDE columns for costing to preserve prior cost behavior. |
| src/test/regress/sql/gp_covering_index.sql | Add regression queries ensuring INCLUDE-only predicates do not become index conditions. |
| src/test/regress/expected/gp_covering_index.out | Update expected output for Postgres planner variant. |
| src/test/regress/expected/gp_covering_index_optimizer.out | Update expected output for ORCA variant. |
Review details
Suppressed comments (1)
src/backend/gporca/libgpopt/src/xforms/CXformUtils.cpp:1845
- The comment for PcrsIndexReturnableColumns contains a typo: "retunable" should be "returnable".
// CXformUtils::PcrsIndexReturnableColumns
//
// @doc:
// Return the set of columns from the given array of columns which are
// retunable through the index (to determine index-only scan capable)
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since 37224a3 CXformUtils::PdrgpcrIndexKeys() has been a thin wrapper around PdrgpcrIndexColumns(), which appends the index's INCLUDE columns after its key columns. CPredicateUtils::ExtractIndexPredicates() therefore treated a predicate on an INCLUDE-only column as indexable. Ordinary comparisons were still rejected later by CMDIndexGPDB::IsCompatible(), but the shortcuts for boolean column references (col -> col = true, NOT col -> col = false) and for IS [NOT] NULL bypass that check and went straight into the index condition list.
The DXL-to-plan translator then maps the column through GetKeyPos(), which returns ulong_max for a non-key column, so the Var's varattno wrapped to 0 and execution failed with
ERROR: bogus index qualification (nodeIndexscan.c:1250)
or, for IS [NOT] NULL,
ERROR: btree index keys must be ordered by attribute (nbtutils.c:777)
Restore the key-only semantics: reintroduce the EIndexCols selector (EicKey / EicKeyAndIncluded) on PdrgpcrIndexColumns()/PcrsIndexColumns() and make PdrgpcrIndexKeys()/PcrsIndexKeys() request key columns only. The cost model keeps asking for key + INCLUDE columns so that ComputeUnusedIndexWeight() costing is unchanged.
Index-only-scan coverage checks are unaffected: they already use PcrsIndexReturnableColumns().
Add regression coverage to gp_covering_index.
Fixes #1948
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions