Details
-
Task
-
Status: Closed (View Workflow)
-
Critical
-
Resolution: Fixed
Description
Implement the engine-defined options corresponding to useful table
options, including (options marked as ++ are included only because
of the "keep" decision on the corresponding spider system variables
in MDEV-27228):
- auto_increment_mode
- bgs_mode
- bulk_size
- bulk_update_size
- connect_timeout
- multi_split_read
- net_read_timeout
- net_write_timeout
- priority
- query_cache
- query_cache_sync
- read_only_mode
- skip_parallel_search++
- host
- password
- port
- username
- wrapper
- default_file
- default_group
- driver
- dsn
- filedsn
- socket
- ssl_capath
- ssl_ca
- ssl_cert
- ssl_cipher
- ssl_key
- ssl_vscs
- use_pushdown_udf
- force_bulk_delete
- force_bulk_update
- table_count_mode
- delete_all_rows_type
- idx
Spider table parameters are documented (incompletely) at
https://mariadb.com/kb/en/spider-table-parameters/
Many of these table params have a twin system variable. Examples
include read_only_mode, net_read_timeout etc. The natural
approach to these params is to use HA_TOPTION_SYSVAR macro,
which takes care of the value overriding. This is also a natural
continuation of spider sysvar tickets like MDEV-27169 and
MDEV-31524. However, table options can not fall back dynamically to
the current sysvar value, so we have to keep the overriding/fallback
mechanism and use the string type (HA_TOPTION_STRING) for these
values, where we can encode unspecified as the null string.
There's also a temporary problem with unspecified non-string table
options. Given that we will keep connection string parsing by
COMMENT or CONNECTION until MDEV-31146, It makes sense as a part of
the changes for this ticket, to ignore COMMENT/CONNECTION parsing
when any table option is used. However, it is not possible to tell
whether a non-string type table option has been specified. We fix
this by making these options also string type (so all spider options
are of the string type) and if any of them is non-null (i.e.
specified), we ignore COMMENT/CONNECTION for parsing. An alternative
fix also considered was to make the COMMENT string parsing override
these table options. However, this is an inferior choice given
that 1. this increases complexity in overriding; 2. only six
relatively uncommon table options are not sysvars and not strings
(priority, query_cache, query_cache_sync, force_bulk_delete,
force_bulk_update, table_count_mode); 3. all params are already
supplied as strings (e.g. read_only_mode "1") in the existing
comment string parsing so no surprise to users if they remain
strings, and in create table statements strings do not need to be
quoted anyway; 4. users might want to use comments for comments.
Even though after this task there are MDEV-28861 and MDEV-31146 that
duplicate and remove connection info encoded in comments, there will
still be connection string parser in the spider codebase, for spider
udfs like spider_direct_sql and spider_copy_tables.
Regarding the choice of the table options in the list above, some
undocumented parameters have unclear purpose. It is not in the scope
of this ticket to try to document every one of them. Examples
include force_bulk_update, force_bulk_delete,
table_count_mode. Also worth noting is the parameter
delete_all_rows_type, which is buggy at 11.2 (MDEV-31996), but
on face value its purpose is clear, so we keep it for now.
Attachments
Issue Links
- blocks
-
-
- Closed
-
- causes
-
MDEV-32486 Assertion `!trx->alloc_line_no[id] || trx->alloc_line_no[id] == line_no' failed in spider_alloc_mem_calc
-
- Closed
-
-
-
- Closed
-
- includes
-
-
- Closed
-
-
-
- Closed
-
- is blocked by
-
-
- Closed
-
-
-
- Closed
-
- relates to
-
-
- Stalled
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Confirmed
-
Activity
As some background, there are roughly three kinds of spider
parameters:
- system variables that are also table params, e.g.
read_only_mode and use_table_charset - system variables that are not table params, e.g.
spider_support_xa and supder_conn_recycle_mode - table params that are not system variables, e.g. all
spider_share fields prefixed with tgt_ like tgt_dbs, as
well as others like priority.
All spider system variables can be found in spd_params.cc, and
are documented at
<https://mariadb.com/kb/en/spider-server-system-variables/>, whereas
all table params can be found in the big switch statement in
spd_table.cc mentioned in a previous comment, and are documented
at <https://mariadb.com/kb/en/spider-table-parameters/>. The table
params documentation is rather incomplete, updating which is a
separate task.
I have been going through the table params one by one. The params
monitoring_* seem to be only used with the function
spider_ping_table_mon_from_table(), and they seem to be only
useful with the spider high availability feature, which has been
deprecated in MDEV-28479 and will be removed in MDEV-28862. When a
spider connection string involves multiple remotes (HA), and if a
query sent to a remote fails, spider_ping_table_mon_from_table()
will update the status of that remote to SPIDER_LINK_STATUS_NG, so
that if the query is retried it will not use this remote.
This can be iullstrated from the logs of one of almost identical
looking the ha tests, spider/bg.ha.
|
spider |
--connection master_1
|
eval $MASTER_1_CHECK_HA_STATUS;
|
# query to dropped remote table fails |
--error 12511
|
INSERT INTO ta_l (a, b, c) VALUES |
(6, 'e', '2011-05-05 20:04:05'); |
# some inconsequential queries |
...
|
# query to healthy remote table works |
INSERT INTO ta_l (a, b, c) VALUES |
(6, 'e', '2011-05-05 20:04:05'); |
Some supporting evidence to the above claim: only four spider tests
fail when we put an abort(); at the beginning of
spider_ping_table_mon_from_table(), and only these tests use the
table param mkd (short for monitoring_kind), and all these
tests are HA tests.
Without HA, i.e. when each connection string has only one remote
table, there is no point in calling
spider_ping_table_mon_from_table(), as we don't need to update the
status of various remotes, as there's no remote to fall back to.
In any case, I created a minimal test that exercises
spider_ping_table_mon_from_table() without HA. As expected, the
use is trivial and redundant:
https://github.com/MariaDB/server/commit/322a0862082
Based on this, I conclude that none the monitoring_* table params
should be converted to table options.
Some other HA related table params that should not be converted to
table options include access_balances, active_link_count and
link_status.
Some more conclusions. As before, items marked with + means
keep/implement as table options, and those marked with - means
deprecated/do not implement as table options.
- access_balances-: part of the deprecated HA feature
- active_link_count-: part of the deprecated HA feature
- auto_increment_mode+: used non-trivially in
ha_spider::write_row() - bgs_mode+: whether to run queries on background threads, covered
in tests under the spider/bg suite - bulk_size+: accessed for spider->bulk_size, which is further
used in spider_db_bulk_insert() when inserting - bulk_update_size+: accessed for result_list.bulk_update_size,
which is further used in spider_db_bulk_update() - connect_timeouts+: passed in mysql_option to data node when
connecting - delete_all_rows_type+: whether to do fast delete or direct delete
when deleting all rows of a spider table. See alsoMDEV-31996 - force_bulk_delete+: used to set a spider specific table flag
HA_CAN_FORCE_BULK_DELETE (added in 2017), which are further used
in Sql_cmd_update::update_single_table(). Pending documentation. - force_bulk_update+: same as force_bulk_delete
- key_hint+: the idx000 as documented in
<https://mariadb.com/kb/en/spider-table-parameters/#idx000>, for
sending force/use/ignore index to data node. The 000 is supposed
to be the index of the index, i.e. 000 means the 0th index, 001
means the first index and so on. But it may be redundant and not
needed, see also MDEV-32000 and MDEV-32001. - link_statuses-: part of HA
- monitoring_*-: see my previous comment
- multi_split_read+: not clear, but looks useful. See also
<https://mariadb.com/kb/en/spider-server-system-variables/#spider_multi_split_read>. - net_read_timeouts+: same as connect_timeouts
- net_write_timeouts+: same as connect_timeouts
- priority+: used in commit and rollback, to determine which
connection (e.g. partition) to commit/rollback first. Funnily
enough, smaller means higher priority - query_cache+: works as documented, i.e. whether to pass
[no_]query_cache in queries to data nodes - query_cache_sync+: whether to pass the [no_]query_cache in
queries from spider nodes to data nodes. We can probably combine
query_cache and query_cache_sync into one bitmap. - read_only_mode+: whether the spider table should be read only
and reject writes - read_rate-: unused in the code
- scan_rate-: same as read_rate
- skip_parallel_search+: Not clear, but related to bgs mode and
accessed for ha_spider::use_pre_call which is further used in
the background thread. Looks useful. Pending further investigation
in a separate task. See also
<https://mariadb.com/kb/en/spider-server-system-variables/#spider_skip_parallel_search>. - static_key_cardinality-: unused
- static_link_ids-: used trivially (passing values around, system
table read-write etc.) in spider_get_ping_table_mon_list() and
friends - static_mean_rec_length-: passed onto
ha_statistics::mean_rec_length when not -1, but itself is
always -1 with no real default value - static_records_for_status-: unused
- table_count_mode+: for setting table flags
HA_STATS_RECORDS_IS_EXACT and HA_HAS_RECORDS - tgt_default_files+: same as connect_timeouts
- tgt_default_groups+: same as connect_timeouts
- tgt_drivers+: spider odbc
- tgt_dsns+: spider odbc
- tgt_filedsns+: spider odbc
- tgt_hosts+: used in mysql_real_connect() for connecting to
data nodes - tgt_passwords+: same as tgt_hosts
- tgt_pk_names-: a key name to check the key name in an
HA_ERR_FOUND_DUPP_KEY warning when inserting with
ignore/replace/update on duplication, in some very specific
circumstances. If the key name match, we get a more helpful
warning ER_DUP_ENTRY (1062), otherwise we get a more generic one
ER_DUP_KEY (1022). However the usefulness is limited, as it is
only used when the dup key is primary, in which case the key name
is always "PRIMARY" - tgt_ports+: same as tgt_hosts
- tgt_sequence_names-: unused. previously used with some spider
oracle support which has now been deprecated - tgt_sockets+: same as tgt_hosts
- tgt_ssl_*+: same as connect_timeouts
- tgt_usernames+: same as tgt_hosts
- tgt_wrappers+: the wrapper
- use_pushdown_udf+: self-explanatory - whether to pushdown udf to
data nodes
I have a PoC implementation with read_only_mode at
https://github.com/MariaDB/server/commit/c81002c0595.
I will clean up the commits, and if nothing new, I will generalise it
to all chosen params.
One thing to note is that we cannot determine if any options have been
specified because of how options associated with sysvars work. They
are added to option_list when the sysvar value is non-default.
This makes it impossible to determine whether the an option comes from
a SET statement (session level), or specified for the table (table
level). Given we are delaying deprecating the comment parsing to after
MDEV-28856, the only solution is to let comment parsing override
option values.
Consider this case:
set session spider_read_only_mode = 1; |
create table t1 (c int) ENGINE=Spider COMMENT='read_only_mode "0", WRAPPER "mysql", srv "srv",TABLE "t2"'; |
The correct thing to do is to use the table level value 0 specified in
the comment. However, during the create table query, there no way to
tell whether option_struct->read_only (== 1) is set with a
SET SESSION spider_read_only_mode = 1
or as a create table option
SPIDER_READ_ONLY=1
See https://github.com/MariaDB/server/commit/42d8eb49eba with an
attempt to determine if any options have been specified.
The only way to guarantee this works is to let comment parsing
override table options, which is the approach taken in
https://github.com/MariaDB/server/commit/c81002c0595.
Another issue I encountered is the apparent lack of namespace in
keywords parsing. It appears to me that engine-defined option keywords
cannot clash with existing server keyword. I learned it the hard way
when trying to understand the parsing error with READ_ONLY=1 (see the
testcase in [1]). The tokenizer checks existing keywords and does not
return IDENT when it finds READ_ONLY is an existing keyword in this
case. Either we need to prefix all spider engine-defined options with
say SPIDER_ to avoid polluting the "global namespace" with spider
table options, or update the parser so that engine defined options are
excluded from the check. In [2] I fixed it with a SPIDER_ prefix.
[1]
https://github.com/MariaDB/server/commit/6e7fabe5365#diff-9b8650c503ba8306771bd012fc3a7ae062aa04d769995545989401093416b0ad
[2] https://github.com/MariaDB/server/commit/8aeb70e8e40
Actually, we can't use the sysvar type (HA_TOPTION_SYSVAR) for
table options associated with a sysvar. This is because in Spider
these options, if not specified, should fallback to a dynamic
default, which is the current value of the sysvar. Whereas table
options are designed to be static, i.e. table option value should
stay the same regardless of changes in the sysvar.
Consider the following example (adapted and extracted from
sysvar_params.test from [1])
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"'; |
show create table t1; |
Table Create Table |
t1 CREATE TABLE `t1` ( |
`c` int(11) DEFAULT NULL |
) ENGINE=SPIDER DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci COMMENT='WRAPPER "mysql", srv "srv",TABLE "t2"' |
set session spider_read_only_mode = 1; |
show create table t1; |
Table Create Table |
t1 CREATE TABLE `t1` ( |
`c` int(11) DEFAULT NULL |
) ENGINE=SPIDER DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci COMMENT='WRAPPER "mysql", srv "srv",TABLE "t2"' |
/* 1 */ insert into t1 values (42); |
drop table t1, t2; |
Both show create tables omit the SPIDER_READ_ONLY table options
because they have the default sysvar value 0. However the "dynamic""
value should be 1 in the second case, and the insertion should fail
accordingly.
The same problem occurs in the other case too:
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"'; |
show create table t1; |
Table Create Table |
t1 CREATE TABLE `t1` ( |
`c` int(11) DEFAULT NULL |
) ENGINE=SPIDER DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci COMMENT='read_only_mode "0", WRAPPER "mysql", srv "srv",TABLE "t2"' `SPIDER_READ_ONLY`='ON' |
set session spider_read_only_mode = 0; |
show create table t1; |
Table Create Table |
t1 CREATE TABLE `t1` ( |
`c` int(11) DEFAULT NULL |
) ENGINE=SPIDER DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci COMMENT='read_only_mode "0", WRAPPER "mysql", srv "srv",TABLE "t2"' `SPIDER_READ_ONLY`='ON' |
/* 5 */ insert into t1 values (42); |
drop table t1, t2; |
(here we are able to insert because table options are ignored in this
case and only comment used)
This case also displays a related issue, that except for the string
type, table options do not support "not specified".
So we need to
1. Keep the dynamic fallback for spider table options that are also
sysvars
2. Either make all such options string type so that NULL means not
specified and to fallback to the sysvar, or update spider table
options design to allow "not specified". Given spider is probably
the only engine that requires such dynamic fallback, I'd go with
the "either" option rather than the "or" option.
Here's the latest PoC patch with all the previous issues (keyword
namespace, unspecified options, show create table, multi-level
overriding, dynamic fallback to sysvar etc.) resolved:
https://github.com/MariaDB/server/commit/eb97777b4ce
Will add other options if nothing new.
Here's the current patch implementing all the options specified in the
description.
Here's a funny case where we cannot detect easily mixed use of
comment and table options:
CREATE TABLE ... COMMENT='tbl "tbl_a"' |
PARTITION BY RANGE (a) ( |
PARTITION p1 VALUES LESS THAN (3) COMMENT='srv "s_2_1"', |
PARTITION p2 VALUES LESS THAN MAXVALUE REMOTE_SERVER="s_2_2" |
);
|
In parsing the connection info for p1, the spider parser could only
see comments, notice that none of the options are set, so it parses
the comments. In parsing the connection info for p2, it notices
REMOTE_SERVER is specified, so it ignores COMMENT. This inconsistency
is temporary and will disappear after we remove comment parsing in
MDEV-31146, so rather than trying to detect it, I suggest the solution
is to simply advise in documentation "Don't include connection info in
comment when using options or bad things may happen".
We could also add another boolean spider system variable
spider_ignore_comment that defaults to false, but will default to
true in MDEV-28861.
Hi holyfoot, ptal thanks:
1. https://github.com/MariaDB/server/commit/56d8819a57f
2. https://github.com/MariaDB/server/commit/f4090a25024
3. https://github.com/MariaDB/server/commit/819271b559a
Of these patches, the first and second ones are cleanups, documentations etc., and I intend to push them to the lowest possible version (e.g. 10.4) - please let me know if there are any problems.
The third commit is the actual patch adding the options, targeted for 11.3.
I would like to start work on MDEV-28861 asap, given this ticket has already made the choice of which params to not include in options, thus will be deprecated. But I will have to handle a couple other tasks first.
Thanks for the review holyfoot. Done and pushed the following to
preview-11.3-preview:
56bb5963314 * MDEV-28856 Add remaining Spider table options
|
ee558987043 * MDEV-28856 Spider: Tests, documentation, small fixes and cleanups
|
96db8963441 * MDEV-28856 Spider: drop server in tests
|
44a397feb9f * MDEV-31117 clean up spider connection info parsing
|
bdaff099945 * MDEV-31524 Fixing spider table param / variable overriding
|
The last two commits are needed because patches for respective tickets
had not reached 11.3 yet.
The commits in the current preview-11.3-preview branch are
a4031e4c051 * MDEV-28856 Add remaining Spider table options
|
53c9458c67e * MDEV-28856 Spider: Tests, documentation, small fixes and cleanups
|
ce1dc33e795 * MDEV-28856 Spider: drop server in tests
|
3c2ed2910a0 * MDEV-31117 clean up spider connection info parsing
|
7527a821d1b * MDEV-31524 Fixing spider table param / variable overriding
|
I have noticed an unusually high, continual, system load during testing of this feature (factor x10 approx compared with other runs). The source seems to be the mariadbd's being tested using significantly more CPU resources. ycp does any part of the patch represent a risk for performance degradation, unexpected looping, or similar? Also, the VIRT memory column in htop shows 40T for most processes which is a little concerning though may prove to be harmless.
Update: Issue was found to exist pre-patch also. Mentioned to axel to keep an eye out for it during preview-11-preview performance testing.
Created MDEV-32486 Assertion `!trx->alloc_line_no[id] || trx->alloc_line_no[id] == line_no' failed in spider_alloc_mem_calc
Created MDEV-32492 SIGSEGV in spider_conn_first_link_idx on DELETE
Status update: many new issues were seen during the testing of this feature. Most of these were traced back to 1) missing MDEV-31524 and MDEV-30981 commits in the feature branch, 2) MDEV-32492 (and the iterations thereof, ref bug for details), which turned out to be a legacy bug which asserted itself more readily in the feature branch. The final re-test results of the updated branch of this feature (bb-11.3-mdev-28856-and-fixes) are near complete.
Both MDEV-32558 (marked as duplicate of MDEV-32492 as the underlying issue is the same) as well as the newly created MDEV-32585 are bugs that show in optimized builds only. For MDEV-32558 after discussion we agreed it is acceptable to leave it and fix later in combination with MDEV-32492. MDEV-32585 is a WIP.
Thanks for the testing Roel.
Given the patches depend on the MDEV-31117 commits, the latter having
not yet reached 11.3 (highest version inclusion 11.1), the push is
blocked by the merge of 11.1->11.2->11.3.
squashed the following and pushed to 11.3 as 126157061b4376496c034a809ea4943e863d1465
cc08a83ef42 * MDEV-32486 Fix duplicate spider_bulk_malloc ID
|
99c3003ec78 * MDEV-32234 Fix missing space in Spider variable descriptions
|
80e4b349fc2 * MDEV-28856 fixes test to pass in --ps-protocol
|
a4031e4c051 * MDEV-28856 Add remaining Spider table options
|
From the big switch in spider_parse_connect_info in spd_table.cc
at 11.2[1], I identified 107 table options the user can specify in a
spider connection string, annotated with their statuses:
+: to keep++: to keep (mdev-27228)/: deprecated--: to deprecate (mdev-27228)access_balancesactive_link_countauto_increment_mode++bgs_first_read--bgs_modebgs_second_read--bka_engine--bka_mode--bka_table_name_types/buffer_size/bulk_size++bulk_update_mode--bulk_update_size++casual_read--connect_timeouts++crd_bg_mode--crd_interval--crd_mode/crd_sync/crd_type/crd_weight/delete_all_rows_typedirect_dup_insert--direct_order_limit--error_read_mode--error_write_mode--first_read--force_bulk_deleteforce_bulk_updateinit_sql_alloc_size/internal_limit/internal_offset/internal_optimize--internal_optimize_local--key_hintlink_statusesload_crd_at_startup--load_sts_at_startup--low_mem_read--max_order--monitoring_bg_flagmonitoring_bg_intervalmonitoring_bg_kindmonitoring_binlog_pos_at_failingmonitoring_flagmonitoring_kindmonitoring_limitmonitoring_sidmulti_split_read++net_read_timeouts++net_write_timeouts++priorityquery_cachequery_cache_syncquick_mode--quick_page_byte--quick_page_size--read_only_mode++read_ratereset_sql_alloc--scan_ratesecond_read--select_column_mode--selupd_lock_mode--semi_split_read--semi_split_read_limit--semi_table_lock/semi_table_lock_conn/server_names+skip_default_condition--skip_parallel_search++split_read--static_key_cardinalitystatic_link_idsstatic_mean_rec_lengthstatic_records_for_statusstore_last_crd--store_last_sts/strict_group_bys--sts_bg_mode--sts_interval--sts_mode/sts_sync/table_count_modetgt_dbs+tgt_default_filestgt_default_groupstgt_driverstgt_dsnstgt_filedsnstgt_hosts+tgt_passwords+tgt_pk_namestgt_ports+tgt_sequence_namestgt_socketstgt_ssl_capathstgt_ssl_castgt_ssl_certstgt_ssl_cipherstgt_ssl_keystgt_ssl_vscstgt_table_names+tgt_usernames+tgt_wrappers+use_pushdown_udfuse_table_charset--Of the annotations:
reasons, like remote table names and ports.
are marked in MDEV-27288 to keep. If the system variables
make sense as table options, then the decision to keep carries over
can ignore them for this task
are marked in MDEV-27288 to deprecate, but have not been deprecated
yet. Assuming the decision is correct, we can ignore these options
for this task too.
Removing the ones marked as deprecated to to be deprecated, we are
left with 58 table options, of which we need to make decisions on 50
of them (i.e. those not marked with a single +), whether to
implement each one of them as an engine defined option:
+: to keep++: to keep (mdev-27228)access_balancesactive_link_countauto_increment_mode++bgs_modebulk_size++bulk_update_size++connect_timeouts++delete_all_rows_typeforce_bulk_deleteforce_bulk_updatekey_hintlink_statusesmonitoring_bg_flagmonitoring_bg_intervalmonitoring_bg_kindmonitoring_binlog_pos_at_failingmonitoring_flagmonitoring_kindmonitoring_limitmonitoring_sidmulti_split_read++net_read_timeouts++net_write_timeouts++priorityquery_cachequery_cache_syncread_only_mode++read_ratescan_rateserver_names+skip_parallel_search++static_key_cardinalitystatic_link_idsstatic_mean_rec_lengthstatic_records_for_statustable_count_modetgt_dbs+tgt_default_filestgt_default_groupstgt_driverstgt_dsnstgt_filedsnstgt_hosts+tgt_passwords+tgt_pk_namestgt_ports+tgt_sequence_namestgt_socketstgt_ssl_capathstgt_ssl_castgt_ssl_certstgt_ssl_cipherstgt_ssl_keystgt_ssl_vscstgt_table_names+tgt_usernames+tgt_wrappers+use_pushdown_udf[1] https://github.com/MariaDB/server/blob/4f39ae75606d0400d7d6a3fc5e2123d7aa244011/storage/spider/spd_table.cc#L2199