-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
MDEV-39993 Use CREATE OR REPLACE in sys schema to preserve grants dur… #5244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # | ||
| # MDEV-39993: mariadb-upgrade drops existing EXECUTE grants on sys stored | ||
| # routines during upgrade because sys schema scripts use DROP+CREATE | ||
| # instead of CREATE OR REPLACE | ||
| # | ||
| --source include/have_perfschema.inc | ||
|
|
||
| CREATE USER testuser@localhost; | ||
|
|
||
| GRANT EXECUTE ON FUNCTION sys.quote_identifier TO testuser@localhost; | ||
| GRANT EXECUTE ON PROCEDURE sys.table_exists TO testuser@localhost; | ||
|
|
||
| --echo # Grants BEFORE running upgrade | ||
| --sorted_result | ||
| SHOW GRANTS FOR testuser@localhost; | ||
|
|
||
| --echo # Run mariadb-upgrade (re-installs sys schema) | ||
| --exec $MYSQL_UPGRADE --force 2>&1 | ||
|
|
||
| --echo # Grants AFTER running upgrade (must be preserved) | ||
| --sorted_result | ||
| SHOW GRANTS FOR testuser@localhost; | ||
|
|
||
| --echo # Verify routines still work after replacement | ||
| SELECT sys.quote_identifier('test') AS quoted; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not a valid test IMHO: I'd test it with the actual user that is being created for the test. |
||
|
|
||
| --echo # Verify CREATE OR REPLACE works for fresh routine (simulate new install) | ||
| DROP FUNCTION IF EXISTS sys.quote_identifier; | ||
| --exec $MYSQL_UPGRADE --force 2>&1 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here too. |
||
|
|
||
| --echo # Routine recreated successfully | ||
| SELECT sys.quote_identifier('test') AS quoted; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also not what should be tested. Of course the procedure will be recreated. And this is probably tested someplace else already. I'd suggest dropping the whole test part: it doesn't contribute to more coverage IMHO. Instead you should be testing all of the affected procedures and functions in SYS: there's quite a number of them and you're just testing the one. |
||
|
|
||
| # Cleanup | ||
| --let $MYSQLD_DATADIR= `select @@datadir` | ||
| --remove_file $MYSQLD_DATADIR/mariadb_upgrade_info | ||
| DROP USER testuser@localhost; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging the entire verbose output of
mariadb-upgrademakes the test highly fragile and prone to failure. Any future changes to themysqlorsysschemas (such as adding, removing, or renaming tables/views), or running the test in environments with different storage engine configurations (e.g., with or without InnoDB enabled), will cause result mismatches and test failures.It is highly recommended to wrap the
$MYSQL_UPGRADEexecution with--disable_result_logand--enable_result_logto suppress this output. The test's correctness is already verified by checking the grants and routine functionality before and after the upgrade.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to agree here. it's a good suggestion.