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_balances

active_link_count

auto_increment_mode++

bgs_first_read--

bgs_mode

bgs_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_type

direct_dup_insert--

direct_order_limit--

error_read_mode--

error_write_mode--

first_read--

force_bulk_delete

force_bulk_update

init_sql_alloc_size/

internal_limit/

internal_offset/

internal_optimize--

internal_optimize_local--

key_hint

link_statuses

load_crd_at_startup--

load_sts_at_startup--

low_mem_read--

max_order--

monitoring_bg_flag

monitoring_bg_interval

monitoring_bg_kind

monitoring_binlog_pos_at_failing

monitoring_flag

monitoring_kind

monitoring_limit

monitoring_sid

multi_split_read++

net_read_timeouts++

net_write_timeouts++

priority

query_cache

query_cache_sync

quick_mode--

quick_page_byte--

quick_page_size--

read_only_mode++

read_rate

reset_sql_alloc--

scan_rate

second_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_cardinality

static_link_ids

static_mean_rec_length

static_records_for_status

store_last_crd--

store_last_sts/

strict_group_bys--

sts_bg_mode--

sts_interval--

sts_mode/

sts_sync/

table_count_mode

tgt_dbs+

tgt_default_files

tgt_default_groups

tgt_drivers

tgt_dsns

tgt_filedsns

tgt_hosts+

tgt_passwords+

tgt_pk_names

tgt_ports+

tgt_sequence_names

tgt_sockets

tgt_ssl_capaths

tgt_ssl_cas

tgt_ssl_certs

tgt_ssl_ciphers

tgt_ssl_keys

tgt_ssl_vscs

tgt_table_names+

tgt_usernames+

tgt_wrappers+

use_pushdown_udf

use_table_charset--

Of the annotations:

• a single + marks table options that should be kept for obvious
  reasons, like remote table names and ports.
• a doulbe ++ marks table options whose corresponding system variables
  are marked in MDEV-27288 to keep. If the system variables
  make sense as table options, then the decision to keep carries over
• a / marks table optons that have already been deprecated. So we
  can ignore them for this task
• a double – marks table options whose corresponding system variables
  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_balances

active_link_count

auto_increment_mode++

bgs_mode

bulk_size++

bulk_update_size++

connect_timeouts++

delete_all_rows_type

force_bulk_delete

force_bulk_update

key_hint

link_statuses

monitoring_bg_flag

monitoring_bg_interval

monitoring_bg_kind

monitoring_binlog_pos_at_failing

monitoring_flag

monitoring_kind

monitoring_limit

monitoring_sid

multi_split_read++

net_read_timeouts++

net_write_timeouts++

priority

query_cache

query_cache_sync

read_only_mode++

read_rate

scan_rate

server_names+

skip_parallel_search++

static_key_cardinality

static_link_ids

static_mean_rec_length

static_records_for_status

table_count_mode

tgt_dbs+

tgt_default_files

tgt_default_groups

tgt_drivers

tgt_dsns

tgt_filedsns

tgt_hosts+

tgt_passwords+

tgt_pk_names

tgt_ports+

tgt_sequence_names

tgt_sockets

tgt_ssl_capaths

tgt_ssl_cas

tgt_ssl_certs

tgt_ssl_ciphers

tgt_ssl_keys

tgt_ssl_vscs

tgt_table_names+

tgt_usernames+

tgt_wrappers+

use_pushdown_udf

[1] https://github.com/MariaDB/server/blob/4f39ae75606d0400d7d6a3fc5e2123d7aa244011/storage/spider/spd_table.cc#L2199

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.

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

[1] https://github.com/MariaDB/server/commit/1bbab11f4e5

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.

https://github.com/MariaDB/server/commit/7904d8ea992

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.

ok to push with minor correction in the last patch comment.

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.

OK to push.

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