Hi, Vicențiu! On Jan 21, Vicențiu Ciorbaru wrote:
Hi Sergei!
I agree with most review comments except one:
information.
diff --git a/mysql-test/include/switch_to_mysql_user.inc b/mysql- test/include/switch_to_mysql_user.inc index f5801db6114..eff1d98c2df 100644 --- a/mysql-test/include/switch_to_mysql_user.inc +++ b/mysql-test/include/switch_to_mysql_user.inc @@ -45,6 +45,9 @@ CREATE TABLE mysql.user ( plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, authentication_string TEXT NOT NULL, password_expired ENUM('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, + password_last_changed datetime NULL, + password_lifetime int(20) unsigned NULL, + account_locked enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
No-no. mysql.user structure was frozen in time the moment I pushed the commit with the new structure. It doesn't change anymore.
To test 5.7 support, just add ALTER TABLE in one specific test file. See main/system_mysql_db_507.test
If we are to freeze mysql.user and be as "compatible" as possible , I would freeze it with the latest version of 5.7 included, it feels the most complete.
Any other reason that this is a no-no? :)
We aren't to freeze mysql.user to be as "compatible" as possible. We're to freeze it to never deal with it again. And not to break existing tools that check it. This table is dead, it's never used besides some third-party tools that I know almost nothing about. And these tools don't need new columns either (but they do need non-empty password column, we've got complaints when I changed that). And, anyway, the patch doesn't correct upgrade 10.4.1 mysql.user view (without account_locked column) to the new mysql.user view (with account_locked column). But I think that fixing it is a waste of time :)
As for password_xxx columns, they were added at the same time here so as to have the final format in and simplify logic slightly with mysql.user tabular. Your suggestion of handling it via types could work even with the password columns I think.
One could split it into 2 commits, one that just adds columns and minimal changes to keep things working and one that adds account locking logic.
One reason to have password expiration related columns in the password expiration commit it to see that they are those actual columns that the code needs. And that password expiration code (pushed later) won't change the structure once again, because it'll need something different. Regards, Sergei Chief Architect MariaDB and security@mariadb.org