Repository navigation
fix: the DDL waiter parsed IF NOT EXISTS as the column name - #1408
Merged
Merged
Conversation
The schema-change waiter extracted the wrong column name from every guarded ALTER, so it waited for a column that can never exist and never waited for the real one -- reinstating the exact race (#1222) it was written to close. ALTER TABLE employees.ship_class ADD COLUMN IF NOT EXISTS ship_spec Nullable(String) first, ADD COLUMN IF NOT EXISTS somecol Nullable(Int32) after start_build Waiting for 2 column(s) to appear in employees.ship_class: [IF, IF] ADD_COLUMN_PATTERN captured the first token after ADD COLUMN. That was correct until the generated DDL became guarded; the pattern predates the guards and was never updated, so it now returns the keyword IF. DROP_COLUMN_PATTERN has the same defect with IF EXISTS. Consequences, all observed in CI: * the waiter polls system.columns for a column named IF for the full 30s timeout, ~600 polls per table, then gives up; * the real column is never waited for, so the caller reads column metadata that may predate the ALTER -- the stale-column-list race the class exists to prevent; * the test then dereferences a column map that was never populated and dies with a NullPointerException far from the cause. Fix: consume the optional guard in the prefix instead of capturing it. A column legitimately named 'if' still extracts correctly, since the guard only matches when IF introduces the EXISTS clause. Also names the misparse in the timeout warning. A wrong column name degraded silently for the whole timeout and then surfaced as an unrelated NPE; the warning now says so where the operator will see it. WHY THIS PASSED CI BEFORE: every test in the class re-declared the regex inline rather than using the shipped constant, so the copies stayed correct while the real pattern broke. The patterns are now package-private and the new tests assert against them directly. Tests: DDLSchemaChangeWaiterRaceConditionTest 25 (4 new) -- the verbatim failing-CI DDL, the guarded drop form, the unguarded form still working, and a column actually named 'if'. Failing-first verified. Reverting the two patterns fails exactly the new tests: addColumnIfNotExistsIsNotTheColumnName:147 expected: <[ship_spec, somecol]> but was: <[IF, IF]> dropColumnIfExistsIsNotTheColumnName:159 expected: <[extra]> but was: <[IF]> Verified against the integration tests that were failing at aa00431 (run 32834092832, job java-lightweight), all now green: AlterTableAddColumnIT 1/1 AlterTableAddColumnRaceIT 1/1 AlterTableDropColumnCacheIT 1/1 MySQLDemoIT 6 run, 0 failures, 0 errors (was 2 failures + 3 errors) Destructive-Op-Check: sites=2; result=pass
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.
Fixes the
java-lightweightfailures in run 32834092832 and theJUnit Test Reportjob that reports them.The schema-change waiter extracted the wrong column name from every guarded ALTER, so it waited for a column that can never exist and never waited for the real one — reinstating the exact race (#1222) it was written to close.
ADD_COLUMN_PATTERNcaptured the first token afterADD COLUMN. That was correct until the generated DDL became guarded; the pattern predates the guards and was never updated, so it now returns the keywordIF.DROP_COLUMN_PATTERNhas the same defect withIF EXISTS.Consequences, all observed in CI
system.columnsfor a column namedIFfor the full 30 s timeout — ~600 polls per table — then gives up;NullPointerExceptionfar from the cause.Fix
Consume the optional guard in the prefix instead of capturing it. A column legitimately named
`if`still extracts correctly, since the guard only matches whenIFintroduces theEXISTSclause.The timeout warning now names the misparse too. A wrong column name degraded silently for the whole timeout and then surfaced as an unrelated NPE; the warning says so where the operator will see it.
Why this passed CI before
Every test in the class re-declared the regex inline rather than using the shipped constant, so the copies stayed correct while the real pattern broke. The patterns are now package-private and the new tests assert against them directly.
Tests — 25 in the class, 4 new
The verbatim failing-CI DDL, the guarded drop form, the unguarded form still working, and a column actually named
`if`.Failing-first verified — reverting the two patterns fails exactly the new tests:
Integration tests
Run locally against the suites that were failing at
aa00431a, all now green:Base branch
2.10.0.