[MDEV-31524] Spider variables that double as table params overriding mechanism is buggy Created: 2023-06-22 Updated: 2023-11-27 Resolved: 2023-07-13 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - Spider |
| Affects Version/s: | 10.4, 10.5, 10.6, 10.9, 10.10, 10.11, 11.0, 11.1 |
| Fix Version/s: | 10.4.31, 10.5.22, 10.6.15, 10.9.8, 10.10.6, 10.11.5, 11.0.3, 11.1.2, 11.2.1 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Yuchen Pei | Assignee: | Yuchen Pei |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Issue Links: |
|
||||||||||||||||||||
| 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.
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.
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:
This way:
If we do a rollback, the fixversion should be 10.9+ because that is where [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 |
| Comments |
| Comment by Yuchen Pei [ 2023-06-22 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Here's a testcase. With
| ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Here's a PoC patch that fixes read_only_mode. Will generalise it | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Roel Van de Paar [ 2023-06-23 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
| ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-27 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
A 11.0 patch https://github.com/MariaDB/server/commit/f9489283bb0 For lower versions I will need to apply | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-29 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I cleaned up the commit which is based on 11.0: It seemed to work fine. Then I applied it to 10.4, together with the necessary adapatation of the [1] https://github.com/MariaDB/server/commit/810ed88c65c It uncovered some extra changes necessary to similar logics in udfs | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-06-30 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Alexey Botchkov [ 2023-07-06 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
both patches (for 11.0 and earlier) look ok to push. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Yuchen Pei [ 2023-07-13 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Pushed pushed e1d31a10afcca0d8e16f35c8bb4c25c23f94f136 to 10.4 The following commits should be applied to other versions: 10.5: 48faa20db848012e2187a09e05aba832078cb82e |