LiJie20190102 commented on PR #12968: URL: https://github.com/apache/gravitino/pull/12968#issuecomment-5585332616
> Thanks for the fix. I suggest keeping only the PostgreSQL changes in this PR and discussing the MySQL changes separately. > > The MySQL checks can avoid creating the same index again, but other statements, such as ADD COLUMN and DROP INDEX, can still fail when we run the script again. So the extra SQL does not yet solve the upgrade retry problem. Could we first clarify which retry case we want to support? > > For the test, please check that the indexes exist and have the expected columns and uniqueness. If we only want to test running the index statements twice, please make that scope clear. Ignoring errors from other DDL statements does not show that the whole upgrade script can be run again safely. Thanks for the detailed review, @yuqi1129 . I've reverted the MySQL changes from this PR as you suggested. Regarding the MySQL scope, the original issue #12516 only asks for CREATE INDEX idempotency — it does not cover ADD COLUMN, DROP INDEX, or RENAME INDEX. You're right that making only CREATE INDEX idempotent doesn't solve the full upgrade-retry problem for MySQL, since those other DDL statements would still fail on re-run. I see two options: Keep this PR PostgreSQL-only, and open a separate issue for full MySQL upgrade-script idempotency (covering ADD COLUMN, DROP INDEX, RENAME INDEX, etc.). This keeps the scope tight and matches the issue title. Expand this PR to make all DDL in the MySQL upgrade scripts idempotent using stored procedures (AddColumnIfMissing, DropIndexIfExists, RenameIndexIfExists, CreateIndexIfNotExists). This would be a larger change and might be better as its own PR. Which approach do you prefer? If you'd like a separate issue, I'm happy to file one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
