MDEV-35767: Prevent MTR from overwriting list-like options in test-specific .cnf files#4792
MDEV-35767: Prevent MTR from overwriting list-like options in test-specific .cnf files#4792MicheleFilisina wants to merge 4 commits intoMariaDB:mainfrom
Conversation
|
|
gkodinov
added
the
External Contribution
gkodinov
changed the title
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
Please have a single commit.
And have it contain a commit message that complies to CODING_STANDARDS.md
Also, please target 10.11 : this is a bug fix.
| sub insert { | ||
| my ($self, $option_name, $value, $if_not_exist)= @_; | ||
| my $option= $self->option($option_name); | ||
| if (defined($option) and !$if_not_exist) { |
There was a problem hiding this comment.
I do not think you should be touching this function at all. mtr already supports multi-value variables.
mysql-test/lib/My/Config.pm
Outdated
| return $group->options(); | ||
| } | ||
|
|
||
| # |
| croak "group '$group_name' does not exist" | ||
| unless defined($group); | ||
|
|
||
| my $option= $group->option($option_name); |
| @@ -0,0 +1,3 @@ | |||
| SHOW VARIABLES LIKE 'replicate_do_table'; | |||
| Variable_name Value | |||
| replicate_do_table | |||
There was a problem hiding this comment.
Your test should have printed the 3 tables. and it doesn't.
mysql-test/main/list_parsing.result
Outdated
| @@ -0,0 +1,3 @@ | |||
| SHOW VARIABLES LIKE 'replicate_do_table'; | |||
There was a problem hiding this comment.
disable the test for embedded.
| ( | ||
| "plugin-load-add" => 1, | ||
| "optimizer-switch" => 1, | ||
| "replicate-do-table" => 1, |
There was a problem hiding this comment.
This is the only addition that should be kept. Also, please add a unit tests for this in mariadb-test-run.pl!
MicheleFilisina
force-pushed
the
MDEV-35767-fix
branch
from
61eb286 to
2186a4f
Compare
…nning in embedded mode.
|
There are a few more multi-value filter names: |
mysql-test/main/list_parsing.result
Outdated
| @@ -0,0 +1,3 @@ | |||
| SHOW VARIABLES LIKE 'replicate_do_table'; | |||
| Variable_name Value | |||
| replicate_do_table test.t3,test.t1,test.t2 | |||
There was a problem hiding this comment.
is the order of these predictable in the server?
Maybe if it isn't, the they can be ordered with this test:
SELECT tbl
FROM JSON_TABLE(
CONCAT('[', @@replicate_do_table, ']'),]
'$[*]' COLUMNS (
tbl VARCHAR(16) PATH '$'
)
) AS jt
ORDER BY tbl;
There was a problem hiding this comment.
i had the same suspicion
There was a problem hiding this comment.
i tried your query and it gave me syntax error.
wouldn't this one be better?
SELECT GROUP_CONCAT(tbl ORDER BY tbl) AS sorted_replicate_do_table
FROM JSON_TABLE(
CONCAT('["', REPLACE(@@replicate_do_table, ',', '","'), '"]'),
'$[*]' COLUMNS (
tbl VARCHAR(255) PATH '$'
)
) AS jt;
Change list_parsing.test so that the output is always sorted, add multi-value filter names to Config.pm
MicheleFilisina
force-pushed
the
MDEV-35767-fix
branch
from
f7be74a to
221ee43
Compare
Change list_parsing.test so that the output is always sorted, add multi-value filter names to Config.pm
|
i didn't add a test for each multi-value filter name, but i tested them locally, also, should i open a new pull request to target 10.11? |
5 participants
Currently, MTR fails to properly concatenate list-like options (such as replicate_do_table) when they are declared on multiple lines inside a '.cnf' file. Because the MTR parser reads .cnf files into a dictionary/hash structure, so duplicate keys simply overwrite the previous ones.
This patch updates 'mysql-test/lib/My/Config.pm' to normalize incoming option names (stripping leading dashes and exchanging underscores for dashes) prior to checking if they require multipart concatenation. If the option is a known list property and already exists, instead of overwriting the previous value MTR will concatenate the new value onto the existing one separated by a comma.
Added a new MTR test suite scenario with multiple 'replicate-do-table' declarations to prove the property correctly via SHOW VARIABLES.
Also i'm a new contributor (and a student) so i'm kinda newbie. Thus even, though i made my best, i'm not sure to have followed all the guidelines and this code might be a bit 'buggy'. So i'd appreciate a thorough review on the code.