"changed" flag in system.settings is calculated incorrectly for settings with multiple values

[MikhailBurdukov]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fixed incorrect "changed" flag evaluation for settings with multiple values.

How to reproduce:

SET join_algorithm = 'auto,direct';
SET join_algorithm = 'auto,direct';
SELECT * FROM system.settings WHERE name = 'join_algorithm';

Result

┌─name───────────┬─value───────┬─changed─┬─description─────────────┬─min──┬─max──┬─readonly─┬─type──────────┬─default─┬─alias_for─┐
│ join_algorithm │ auto,direct │       0 │ Specify join algorithm. │ ᴺᵁᴸᴸ │ ᴺᵁᴸᴸ │        0 │ JoinAlgorithm │ default │           │
└────────────────┴─────────────┴─────────┴─────────────────────────┴──────┴──────┴──────────┴───────────────┴─────────┴───────────┘

Meanwhile other types of settings, works as expected:

SET mysql_max_rows_to_insert = 123123;
SET mysql_max_rows_to_insert = 123123;
SELECT * FROM system.settings WHERE name = 'mysql_max_rows_to_insert';

Result

┌─name─────────────────────┬─value──┬─changed─┬─description─────────────────────────────────────────────────────────────────────┬─min──┬─max──┬─readonly─┬─type───┬─default─┬─alias_for─┐
│ mysql_max_rows_to_insert │ 123123 │       1 │ The maximum number of rows in MySQL batch insertion of the MySQL storage engine │ ᴺᵁᴸᴸ │ ᴺᵁᴸᴸ │        0 │ UInt64 │ 65536   │           │
└──────────────────────────┴────────┴─────────┴─────────────────────────────────────────────────────────────────────────────────┴──────┴──────┴──────────┴────────┴─────────┴───────────┘

[serxa]

Good finding. Looks like this inconsistency existed since SettingFieldMultiEnum type introduction for a couple of MySQL datatypes. Although test failures are now related. Looks like the tests are wrong:

[----------] 6 tests from SettingMySQLDataTypesSupport
[ RUN      ] SettingMySQLDataTypesSupport.WithDECIMAL
[       OK ] SettingMySQLDataTypesSupport.WithDECIMAL (0 ms)
[ RUN      ] SettingMySQLDataTypesSupport.WithDATE
[       OK ] SettingMySQLDataTypesSupport.WithDATE (0 ms)
[ RUN      ] SettingMySQLDataTypesSupport.With1
[       OK ] SettingMySQLDataTypesSupport.With1 (0 ms)
[ RUN      ] SettingMySQLDataTypesSupport.WithMultipleValues
[       OK ] SettingMySQLDataTypesSupport.WithMultipleValues (0 ms)
[ RUN      ] SettingMySQLDataTypesSupport.SetString
src/Core/tests/gtest_settings.cpp:125: Failure
Value of: setting.changed
  Actual: true
Expected: false
[  FAILED  ] SettingMySQLDataTypesSupport.SetString (0 ms)
[ RUN      ] SettingMySQLDataTypesSupport.SetInvalidString
src/Core/tests/gtest_settings.cpp:166: Failure
Value of: setting.changed
  Actual: true
Expected: false
[  FAILED  ] SettingMySQLDataTypesSupport.SetInvalidString (0 ms)
[----------] 6 tests from SettingMySQLDataTypesSupport (0 ms total)

[MikhailBurdukov]

Looks like the tests are wrong:

Do you mean it's ok if we change these tests for new behavior?
I'm actually not very familiar with this part of the project, could these changes broke some integration with mysql?
But, from my point of view, this is fine, because mysql doesn't keeps any flags for settings.

mysql> SELECT * FROM performance_schema.global_variables WHERE Variable_name = 'sort_buffer_size'
    -> ;
+------------------+----------------+
| VARIABLE_NAME    | VARIABLE_VALUE |
+------------------+----------------+
| sort_buffer_size | 262144         |
+------------------+----------------+
1 row in set (0.00 sec)

mysql> SHOW SESSION VARIABLES LIKE 'sort_buffer_size';
+------------------+--------+
| Variable_name    | Value  |
+------------------+--------+
| sort_buffer_size | 262144 |
+------------------+--------+
1 row in set (0.00 sec)

[serxa]

Yes, I think it is totally fine to change the tests

[MikhailBurdukov]

@serxa It looks like the failure of the tests is caused not by my changes. Could we merge PR?

[serxa]

Stateless tests (release) — fail: 1 — 00992_system_parts_race_condition_zookeeper_long #48665