sql: allow dropping column referenced in partial index predicate#169753
Draft
rafiss wants to merge 2 commits into
Draft
sql: allow dropping column referenced in partial index predicate#169753rafiss wants to merge 2 commits into
rafiss wants to merge 2 commits into
Conversation
…icate The declarative schema changer previously rejected ALTER TABLE ... DROP COLUMN if the column was referenced in a partial index predicate, with a hint to drop the index first. The restriction was added in cockroachdb#97372 as a temporary safeguard for cockroachdb#96924, where the optimizer could need to read a column being dropped to evaluate the predicate when both the column and its dependent partial index were transitioning simultaneously. The dep rule "secondary index partial no longer public before referenced column" (added later) sequences the partial index into DELETE_ONLY before the column reaches WRITE_ONLY. In DELETE_ONLY the index issues blind DELs and does not need to evaluate its predicate, so the original hazard no longer applies for the declarative path. This commit lifts the restriction in the declarative DROP COLUMN path only. The legacy schema changer keeps the restriction (it lacks the dep-rule machinery), and partial unique-without-index predicates remain blocked in both schema changers — those will be addressed separately. ALTER PRIMARY KEY and ALTER COLUMN TYPE also keep the restriction, because their column-drop traversal can encounter element types that the existing walker does not yet handle. The restriction toggle on walkColumnDependencies changes from a bool to a small predicateRefPolicy enum (allow all / disallow UWI only / disallow all) so each caller can pick the policy that matches its actual constraints. The DML-injection test case "drop column with partial index" was re-enabled (previously skipped under cockroachdb#97813). Resolves: cockroachdb#97813 Epic: none Release note (sql change): ALTER TABLE ... DROP COLUMN no longer rejects columns referenced in a partial index predicate. The dependent partial index is dropped automatically as part of the same statement. The previous behavior — requiring the user to drop the index first — is still in effect when the legacy schema changer is in use, and when the column is referenced in a partial UNIQUE WITHOUT INDEX predicate. Co-Authored-By: roachdev-claude <[email protected]>
b672c00 to
a8d2f07
Compare
Contributor
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Member
The previous commit lifted the restriction on ALTER TABLE ... DROP COLUMN when the column is referenced in a partial-index predicate. The existing dep rule "secondary index partial no longer public before referenced column" sequences the partial index into DELETE_ONLY before the column reaches WRITE_ONLY, which is enough to make the kv layer (issuing blind DELs in DELETE_ONLY) safe. But the optimizer side wasn't safe. addPartialIndexPredicatesForTable iterates tab.DeletableIndexCount() and resolves every partial-index predicate against the table's ordinary (public) column scope. Once the referenced column transitions to WRITE_ONLY, resolution panics with `column "X" does not exist`, breaking any concurrent SELECT against the table — not just queries that would have used the partial index. The mutation path (projectPartialIndexColsImpl) hits a similar failure with `column is being backfilled` once the column is renamed to its placeholder name. Strengthening the dep rule to Index(ABSENT) precedes Column(WRITE_ONLY) fixes the symptom but introduces a dep-graph cycle whenever the dropped column is both a key column and a predicate-referenced column of the same partial index (e.g. UNIQUE INDEX (b) WHERE (b > 0)): the index column needs the column to leave PUBLIC before it can become ABSENT, and the index can't become ABSENT until its index column does. So we have to handle this at the optimizer instead. This commit makes the optbuilder tolerant of partial indexes whose predicate can't currently be resolved: - TableMeta.PartialIndexPredicate now returns (nil, false) instead of panicking when an index is partial in the catalog but missing from partialIndexPredicates. The audit of all 16 direct callers and 4 wrapper callers confirms they already handle ok=false correctly (treating it as "no predicate-based reasoning available, skip this index"). - addPartialIndexPredicatesForTable now skips indexes whose predicate resolution panics with pgcode.UndefinedColumn (logged at v=2). Other panic causes — assertion failures, non-immutable-operator errors, unparseable predicate strings — continue to propagate. - mutation_builder_arbiter.partialIndexPredicate now returns (FiltersExpr, ok). The single caller skips an index as an arbiter candidate when no predicate is available, instead of panicking on the nil-typed cast. - projectPartialIndexColsImpl recovers from both UndefinedColumn and InvalidColumnReference (the "column is being backfilled" panic) and projects constant fallback values for indexes whose predicate can't be resolved: PUT=false (don't insert into a partial index that's being dropped) and DEL=true (issue a blind DEL; the kv layer no-ops if the entry doesn't exist). This matches the safe semantics for a DELETE_ONLY-in-drop partial index. The DML injection test case "drop column with partial index" was re-enabled in the previous commit but failed CI. It now passes. Resolves: cockroachdb#97813 Epic: none Release note: None Co-Authored-By: roachdev-claude <[email protected]>
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.
Lifts the temporary safeguard introduced in #97372 that forbids
ALTER TABLE ... DROP COLUMNwhen the column is referenced in apartial index predicate, in the declarative schema changer path. The
legacy schema changer keeps the restriction. Partial UNIQUE WITHOUT
INDEX predicates also remain blocked in both paths and will be
addressed separately.
The change is split into two commits, reviewable independently:
schemachanger:removes thepanicIfColReferencedInPredicatecheck on the drop-column path. The existing dep rule
secondary index partial no longer public before referenced columnalready orders the partial index into
DELETE_ONLYbefore thereferenced column reaches
WRITE_ONLY, so kv-layer writes are safe(delete-only secondary indexes issue blind DELs without consulting
the predicate). The
walkColumnDependenciestoggle changes from abool to a
predicateRefPolicyenum so each caller (drop-column,alter-primary-key, alter-column-type, rename-column) can pick the
policy that matches its actual constraints. Only the drop-column
path opts into the lifted restriction.
opt:addresses a latent issue exposed by the first commit.With the column in
WRITE_ONLYand the partial index inDELETE_ONLY,addPartialIndexPredicatesForTablepanicsresolving the predicate against the public-only column scope —
breaking any concurrent SELECT against the table, not just queries
that would have used the partial index. The mutation builder's
PUT/DEL synthesis hits a parallel "column is being backfilled"
panic. The commit makes
TableMeta.PartialIndexPredicatereturn(nil, false)instead of panicking when an index is partial inthe catalog but missing from metadata, recovers from
UndefinedColumn/InvalidColumnReferencepanics in the tworesolution sites, and projects constant
PUT=false/DEL=truefallbacks in the mutation builder. All existing callers of
PartialIndexPredicatewere audited and already handleok=false; one (mutation_builder_arbiter.partialIndexPredicate)was discarding the flag and is fixed defensively.
The DML-injection test case "drop column with partial index" (which
was skipped under #97813) is re-enabled and passing.
Note
Marked draft while the optbuilder approach gets a sanity-check
— see #97813 (comment).
The recover-and-fall-back pattern works but weakens an invariant
the optimizer otherwise maintains, and there may be a cleaner
shape (e.g. catalog-layer hiding of partial indexes with
unresolvable predicates, or schema-changer-side clearing of the
predicate string at `DELETE_ONLY`) worth exploring before this
merges.
Resolves: #97813
Epic: none
Release note (sql change): ALTER TABLE ... DROP COLUMN no longer
rejects columns referenced in a partial index predicate. The
dependent partial index is dropped automatically as part of the same
statement. The previous behavior — requiring the user to drop the
index first — is still in effect when the legacy schema changer is
in use, and when the column is referenced in a partial UNIQUE WITHOUT
INDEX predicate.