Bug#121124 Ignore unchanged CHECK columns for online DDL - #723
modasserbillah wants to merge 3 commits into
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
Bot
added
the
OCA Required
github-actions
Bot
added
Tests
|
I have signed an OCA which is under review. The CI workflows seem to block the checkout step for PRs from forks. @gopshank please let me know if I need to follow a different procedure for filing the PR. |
Sure, we shall check and get back. |
|
Thank you for signing the OCA. |
oracle-contributor-agreement
Bot
added
OCA Verified
|
@modasserbillah Can you rebase your branch on latest trunk to pick up the fixed Github CI/CD actions ? |
8271e79 to
6f04097
Compare
github-actions
Bot
removed
Build Failed
|
@RidhaOracle done! |
f58daa0 to
6682c32
Compare
github-actions
Bot
added
Build Passed
|
@phulakun @RidhaOracle can you guide me on this please? How can we get this merged? Seems like some checks are failing on unrelated stuff. |
|
The unrelated failures should have been fixed in trunk, which means if you can rebase your work onto the most recent trunk, then tests should pass. |
|
@henrikingo-oracle I think this branch is already on top of the most recent trunk after my rebase last week. The trunk hasn't moved since then. Do you have reasons to believe otherwise? |
|
@henrikingo-oracle if you or someone from Oracle could help me fix these failures that'd be great. These aren't related to the changes in the PR and I'm not sure how I can move this forward 🙏🏽 |
|
Yes I or someone can help :-) I have to apologise for the mess while we are nearing normal GitHub development workflow in a couple months... But it seems there were some fixes pushed to trunk yesterday, which I think are the ones that should be of help to you as well. Can you please rebase once more and then we can see if MTR gets any greener. Again, I really apologize for the trouble here. I thought MTR fixes were pushed already earlier. |
Problem: ======== A column-definition ALTER checks every column referenced by an enforced CHECK constraint for type changes. Unchanged DATETIME columns use different internal type representations in Item_field and Create_field, so an unrelated instant ENUM extension is incorrectly forced to use COPY. Solution: ========= Only compare CHECK-referenced column types for Create_field entries that represent changed columns. Add MTR coverage for INSTANT and INPLACE ENUM extensions with an unrelated CHECK on DATETIME(6). Signed-off-by: Modasser Billah <imtishad@gmail.com>
6682c32 to
706e318
Compare
github-actions
Bot
removed
Build Passed
|
Thanks for helping me out @henrikingo-oracle ! I have rebased it, let's see how it goes this time 🤞🏽 |
|
Ok nice, it looks like the MTR tests are indeed fixed or disabled... Is it possible the format check failure is in code you edited? At least the file is part of your patch? |
|
The format checks are part of the files I touched but not related to my changes. Do you want me to try to fix that in this PR @henrikingo-oracle? |
|
Yeah it looked like the line nr doesn't match with your patch... But I also don't see other PRs having this failure, so maybe you better look into it and see if you get this PR green :-) |
My understanding is that we are here using a different version of clang-format than have been used before, so we may get errors for existing code. The changes to this PR are later in the file than the code that is flagged, so it is very unlikely that it is caused by this PR. That other PRs do not have this failures, is probably because they do not touch sql_table.cc |
github-actions
Bot
removed
Build Passed
|
Looks like the issue was with semicolons on their own lines, I added a small formatting fix. |
github-actions
Bot
added
Build Passed
|
@henrikingo-oracle all green but merging seems to be still blocked 🤔 |
6 participants
What does this change do?
Only compare a CHECK-referenced column's type when its
Create_fieldrepresents a column changed by the ALTER. It also adds MTR coverage for both INSTANT and INPLACE ENUM extensions when an unrelated enforced CHECK referencesDATETIME(6).Why is it needed?
When any
MODIFY COLUMNsetsALTER_CHANGE_COLUMN, the CHECK-constraint evaluation code currently examines unchanged columns too.Item_fieldreports an unchangedDATETIME(6)asMYSQL_TYPE_DATETIME, whileCreate_fieldreports it asMYSQL_TYPE_DATETIME2. MySQL therefore falsely concludes that the checked column changed and requires a table copy for an otherwise metadata-only ENUM extension.Fixes MySQL Bug #121124.
How was it tested?
mysql-test/scripts/ci/mtr.shpasses locallygit diff --checkpasses and the changed C++ code was formatted with the repository configuration. A local Debug build was attempted on macOS, but the host's global Homebrew Protobuf 29.1 headers conflict with MySQL's bundled Protobuf 24.4 before the build reaches the changed code. The upstream Linux CI build and MTR run are therefore the authoritative validation.The initial GitHub build, MTR, and format jobs also fail before checkout because the repository's
pull_request_targetworkflows do not opt in to checking out fork PR code under the currentactions/checkoutsecurity policy. No PR code is compiled, formatted, or tested in those failed jobs.Contributor checklist
scripts/ci/format.sh)AI assistance
OpenAI Codex assisted with root-cause analysis, the code change, regression-test preparation, and this pull request description.
Areas touched
check_constraintsMTR coverage