github / gh-ost

GitHub's Online Schema-migration Tool for MySQL
MIT License
12.42k stars 1.26k forks source link

[Suggestion] Apply simple validation to alter command #1431

Open acb opened 4 months ago

acb commented 4 months ago

tl;dr: It would be nice if the --alter command line arg did some basic validation and threw an error if your SQL starts with ALTER TABLE as that can have unexpected results.

DB: Mariadb 10.6.10 gh-ost: 1.1.5

I ran into an interesting issue today that I think could be prevented with a little input validation in gh-ost. I was adding a modified_time column to an existing table with associated index and used the following flags in my gh-ost command:

gh-ost
    --table="exampletable"
    --alter="ALTER TABLE exampletable ADD COLUMN modified_time TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, ADD INDEX i_modified_time (modified_time);"

The command worked by all appearances: it created the new table, I verified that the column and index was created, and then executed the cutover. Shortly we discovered that the new table didn't have any actual data in its indexes, the Cardinality was 0 for all of them including PRIMARY, which made every query crawl. We ended up having to roll back the cutover, which ended up being fairly painful.

I'm not 100% sure why this happened as we've run gh-ost in production before (including the exact same migration of adding a modified_time column to a different table!) without issue, but it's 100% reproducible on this particular table on our database. Possibly related to https://github.com/github/gh-ost/issues/875 and/or https://jira.mariadb.org/browse/MDEV-28327?page=com.atlassian.jira.plugin.system.issuetabpanels%3Aall-tabpanel.

Running the same gh-ost command but dropping the ALTER TABLE exampletable from the --alter= flag works correctly, the created table has its indexes populated and the cutover works fine.

Ultimately this is user error, but given that it's an easy mistake to make (especially for someone new at gh-ost) and it's easy to catch, adding a check to the --alter to throw an error if you pass ALTER TABLE to it should be simple to implement and might save someone else a headache and some downtime in the future.

meiji163 commented 1 week ago

Sounds like a reasonable check to add, do you want to open a PR?