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 1 commit intoMariaDB:10.11from
Conversation
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.
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); |
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.
61eb286 to
2186a4f
Compare
|
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;
There was a problem hiding this comment.
that works. I was rough estimating with a numeric example. Thanks for working it out.
f7be74a to
221ee43
Compare
|
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? |
you can rebase your commits to 10.11, squash them up, git force push to the same branch and this PR will updated. Then edit the title of this PR - there's the target branch listed there, change that to 10.11. |
546f020 to
2a8012d
Compare
gkodinov
left a comment
There was a problem hiding this comment.
Looks much better! Thanks for working on this.
Some polishing suggestions below.
mysql-test/main/list_parsing.test
Outdated
| @@ -0,0 +1,8 @@ | |||
| --source include/not_embedded.inc | |||
| SELECT GROUP_CONCAT(tbl ORDER BY tbl) AS sorted_replicate_do_table | |||
There was a problem hiding this comment.
I do not mind the nice derived table etc, but I'd just do SELECT @@replicate_do_table. The order should be stable.
There was a problem hiding this comment.
Oh, I didn't know the order is deterministic. Thanks for the advice 👍.
| !include include/default_my.cnf | ||
|
|
||
| [mysqld.1] | ||
| replicate_do_table=test.t1 |
There was a problem hiding this comment.
Do you really need to prefix these with "test." ? And does it need to be 3 lines? IMHO 2 would do just fine.
There was a problem hiding this comment.
Also, please try to keep a single commit. the github PR UI has as "rebase" button: use that if you need to update your branch. Or the equivalent on the command line.
There was a problem hiding this comment.
Do you really need to prefix these with "test." ? And does it need to be 3 lines? IMHO 2 would do just fine.
I tested removing the "test." prefix, but i belive it causes the server to crash on startup (it dies after 0.06 seconds). In var/log i get this error: [ERROR] Could not add do table rule 't1'.
It seems the configuration strictly requires the database.table format.
But i'm not sure though, what do you think?
…te Option Add some multi-part options to Config.pm, Add a test case for list parsing in mysqltest_multi_opt_replicate_do
d21e30e to
b71c4c1
Compare
|
I was wondering if it would be better to add tests for other multi-options as well? |
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.