Details
Description
Spider has variables and table parameters, and some variables double as table parameters. Examples include spider_same_server_link, spider_casual_read and spider_read_only_mode. These are the variables we consider in this ticket.
All such variables are of the types of int, longlong or double.
For these variables, the existing overriding mechanism does the following: 1) if the var has value -1, defer to the table parameter value; and 2) if the var value is not -1, it overrides the table param value. The table param value is in turn set to default in spider_set_connect_info_default() if not specified. MDEV-27169 marks 1) as deprecated.
MDEV-27169 also changed the default value of the vars from -1 to the actual default values used in spider_set_connect_info_default(). As a consequence, table params are ignored unless the user sets the variable to -1 (say with the SET command) which would trigger a deprecation warning.
It is caused by functions like the following, which says that if the var is -1, then we use the table param, otherwise we use the var value.
int spider_param_read_only_mode( |
THD *thd,
|
int read_only_mode |
) {
|
DBUG_ENTER("spider_param_read_only_mode"); |
DBUG_RETURN(THDVAR(thd, read_only_mode) == -1 ? // default value is 0 |
read_only_mode : THDVAR(thd, read_only_mode));
|
}
|
Assuming we still want to table params to take effect, this needs fixing. Whether we need to do a rollback or a fixup depends on the answer to the following question: do we want table params to override vars if both are set by the user? If not, then a rollback is sufficient. Otherwise, the existing logic (as shown in the code snippet above) would still be broken.
If we want to do a fixup, one idea would be to remove the assignment of default values in spider_set_connect_info_default() to these table params, so that they remain -1 which was assigned at the beginning of spider_parse_connect_info(). Then in functions like spider_param_read_only_mode(), replace the ternary expression with:
DBUG_RETURN(read_only_mode == -1 ?
|
THDVAR(thd, read_only_mode) : read_only_mode);
|
This way:
- the var is set to actual default value if the user does not set it
- if the user sets the table param but not the var, the table param would be used
- if the user sets the var but not the table param, the var would be used
- if the user sets both, the table param would be used
If we do a rollback, the fixversion should be 10.9+ because that is where MDEV-27169 is in. If we do a fixup, then we need to decide whether to fix it for all versions, or just 10.9+
[update on 2023-07-20]: we did a roll-up for all versions, and as a side-effect, the patches for version 10.4-6 contain changes to the effect of MDEV-27169.
Attachments
Issue Links
- blocks
-
MDEV-31674 Update knowledge base after MDEV-31524 is pushed
-
- Closed
-
- is caused by
-
-
- Closed
-
Activity
Here's a PoC patch that fixes read_only_mode. Will generalise it
to all other table params that are also sysvars.
Expected result: OK/F/F/OK
|
Actual results:
|
10.4: OK/F/F/F: Affected
|
10.5: OK/F/F/F: Affected
|
10.6: OK/F/F/F: Affected
|
10.9: OK/F/OK/F: Affected [10.9 change]
|
10.10: OK/F/OK/F: Affected
|
10.11: OK/F/OK/F: Affected
|
11.0: OK/F/OK/F: Affected
|
11.1: OK/F/OK/F: Affected
|
A 11.0 patch https://github.com/MariaDB/server/commit/f9489283bb0
For lower versions I will need to apply MDEV-27169 first. Each version probably has a different set of table params to worry about.
I cleaned up the commit which is based on 11.0:
https://github.com/MariaDB/server/commit/3c374b218d680bce4f5eb4b1256bdcab525d9a73
It seemed to work fine.
Then I applied it to 10.4, together with the necessary adapatation of the
MDEV-27169 patch[1] which is based on 10.7
[1] https://github.com/MariaDB/server/commit/810ed88c65c
It uncovered some extra changes necessary to similar logics in udfs
spider_direct_sql and spider_copy_tables. So I updated the changes
accordingly[2]. But now ha_part.test is hanging at spider_copy_tables. To
be further investigated.
Hi holyfoot, ptal thanks:
https://github.com/MariaDB/server/commit/a8401b295ca
Note that the above patch is based on 10.4. I also have a patch for 11.0, which is smaller because I don't need to backport MDEV-27169 or worry too much about the udfs because of MDEV-28006:
https://github.com/MariaDB/server/commit/770635d13ba
If you want to review only one patch, please do so with the 10.4 version, thank you.
both patches (for 11.0 and earlier) look ok to push.
I'd suggest to replace _FUN() with _FUNC(), but i'm fine with your decision about it 
Pushed pushed e1d31a10afcca0d8e16f35c8bb4c25c23f94f136 to 10.4
The following commits should be applied to other versions:
10.5: 48faa20db848012e2187a09e05aba832078cb82e
10.6: 51ff9eddf7c0aaf1e022fcb3b48ec36835df7785
10.9: 06a61b8e453126c2de1649073f247d34e85f9702
10.10: 90cd0c156f5bb53fd058d2bbfb83f850ffae6722
10.11+: 124eb662700708f3c4b0fb77968f8b854d6bb4aa
Here's a testcase. With
MDEV-27169(e.g. at "current" 11.13883eb63dc5e663558571c33d086c9fd3aa0cf8f), the 3rd and 4th inserts
fail the test (that is, does not throw error 12518 or succeed as
expected). Without
MDEV-27169(e.g. at its parent commit332c59a27ca798ccf56be14f8c9e4f25d60a47a5), the 4th insert fails.
--echo--echo MDEV-31524 Spider variables that double as table params overriding mechanism is buggy--echo--disable_query_log--disable_result_log--source ../../t/test_init.inc--enable_result_log--enable_query_log# value takes effect.# take effect.--error 12518--error 12518# should take precedence--disable_query_log--disable_result_log--source ../../t/test_deinit.inc--enable_result_log--enable_query_log