[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:
Blocks
blocks MDEV-31674 Update knowledge base after MDEV-3152... Closed
PartOf
Problem/Incident
is caused by MDEV-27169 Change default values of Spider plugi... Closed

 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.



 Comments   
Comment by Yuchen Pei [ 2023-06-22 ]

Here's a testcase. With MDEV-27169 (e.g. at "current" 11.1
3883eb63dc5e663558571c33d086c9fd3aa0cf8f), 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 commit
332c59a27ca798ccf56be14f8c9e4f25d60a47a5), 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
SET @old_spider_read_only_mode = @@session.spider_read_only_mode;
evalp CREATE SERVER srv FOREIGN DATA WRAPPER MYSQL OPTIONS (SOCKET "$MASTER_1_MYSOCK", DATABASE 'test',user 'root');
 
# when the user does not set var nor the table option, the default
# value takes effect.
set session spider_read_only_mode = default;
create table t2 (c int);
create table t1 (c int) ENGINE=Spider COMMENT='WRAPPER "mysql", srv "srv",TABLE "t2"';
/* 1 */ insert into t1 values (42);
drop table t1, t2;
 
# when the user sets var but not the table option, the var should be
# take effect.
set session spider_read_only_mode = 1;
create table t2 (c int);
create table t1 (c int) ENGINE=Spider COMMENT='WRAPPER "mysql", srv "srv",TABLE "t2"';
--error 12518
/* 2 */ insert into t1 values (42);
drop table t1, t2;
 
# when the user does not set the var, but sets the table option, the
# table option should take effect
SET session spider_read_only_mode = default;
create table t2 (c int);
create table t1 (c int) ENGINE=Spider COMMENT='read_only_mode "1", WRAPPER "mysql", srv "srv",TABLE "t2"';
--error 12518
/* 3 */ insert into t1 values (42);
drop table t1, t2;
 
# when the user sets both var and table option, the table option
# should take precedence
set session spider_read_only_mode = 1;
create table t2 (c int);
create table t1 (c int) ENGINE=Spider COMMENT='read_only_mode "0", WRAPPER "mysql", srv "srv",TABLE "t2"';
/* 4 */ insert into t1 values (42);
drop table t1, t2;
 
SET session spider_read_only_mode = @old_spider_read_only_mode;
--disable_query_log
--disable_result_log
--source ../../t/test_deinit.inc
--enable_result_log
--enable_query_log

Comment by Yuchen Pei [ 2023-06-23 ]

Here's a PoC patch that fixes read_only_mode. Will generalise it
to all other table params that are also sysvars.

https://github.com/MariaDB/server/commit/8f49fbcf2ad

Comment by Roel Van de Paar [ 2023-06-23 ]

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

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 MDEV-27169 first. Each version probably has a different set of table params to worry about.

Comment by Yuchen Pei [ 2023-06-29 ]

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.

[2] https://github.com/MariaDB/server/commit/6bc457d9f8c

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 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.

Comment by Alexey Botchkov [ 2023-07-06 ]

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

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
10.6: 51ff9eddf7c0aaf1e022fcb3b48ec36835df7785
10.9: 06a61b8e453126c2de1649073f247d34e85f9702
10.10: 90cd0c156f5bb53fd058d2bbfb83f850ffae6722
10.11+: 124eb662700708f3c4b0fb77968f8b854d6bb4aa

Generated at Thu Feb 08 10:24:31 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.