In MDEV-27107 there was an explicit lock for the mariadb-upgrade (where a second mariadb-upgrade could terminate if it doesn't get the lock), but I was thinking of a second mariadb-plugin-init lock exclusively acquired when mariadb does its system tables updates, where it will wait. The intent is that this lock can be acquired by plugins like spider/columnstore for the install plugin / udf that may not have an easy work around.

The other option that is probably also equally valid is ensuring that INSTALL PLUGIN and similar SQL on the system tables acquires metadata locks of the tables correctly (worthy of another task) if found to be deficient.

We are fixing Spider's initialization mechanism in MDEV-27233. Once it is done, we will work on the present issue.

This issue can also cause other unforseen issues. Today I observed this testcase failure outcome in MTR:

mysqltest: At line 8: query 'CREATE TABLE ts ENGINE=Spider COMMENT='WRAPPER "mysql", srv "srv", TABLE "ti"'' failed: ER_NO_SUCH_TABLE (1146): Table 'mysql.spider_tables' doesn't exist

Note that mysql.spider_tables is missing. The testcase being used was simple:

--let $SOCKET=`SELECT @@global.socket`

--source include/have_innodb.inc

INSTALL PLUGIN Spider SONAME 'ha_spider.so';

CREATE USER u@localhost IDENTIFIED BY 'a';

GRANT ALL ON test.* TO u@localhost;

eval CREATE SERVER srv FOREIGN DATA WRAPPER MYSQL OPTIONS (SOCKET "$SOCKET",DATABASE 'test',user 'u',PASSWORD 'a');

CREATE TABLE ti (c INT) ENGINE=InnoDB;

CREATE TABLE ts ENGINE=Spider COMMENT='WRAPPER "mysql", srv "srv", TABLE "ti"';

Rerunning it made the testcase pass. This is thought to be due to the background initialization of Spider.
It would seem that INSTALL PLUGIN Spider SONAME 'ha_spider.so'; should place some form of lock until the initialization is complete, or do the init in the foreground (user) thread, or similar.

Running mysql_upgrade at the same time the initialization of spider is occurring can cause problems as MDEV-27068 shows.

One of the conflicts was

ALTER TABLE proc MODIFY
and spiders' {{ drop procedure}}

danblack Could you provide an example invocation / reproducible testcase showing the conflicts? I skimmed through MDEV-27068 and MDEV-14622 but could not spot any example.

Apart from that, this ticket seems to be mostly about simplifying the init queries or moving the migration part to mysql_upgrade, in order to fix spider init issues like MDEV-27233 and MDEV-22979.

Created an MTR testcase to see if any spider init issues can be seen when running mariadb-upgrade. Did not observe any issues in 1000+ trials.
To repeat, first make this hack in mysql_upgrade_preparation.inc - if you do not have Performance Schema compiled in:

#-- source include/have_perfschema.inc

i.e. add # before the Performance Schema load line in mysql-test/include/mysql_upgrade_preparation.inc.
Then run:

./mtr --repeat=1000 test

With main/test.test containing:

--source include/mysql_upgrade_preparation.inc

--source include/have_working_dns.inc

--source include/have_innodb.inc

--source include/have_partition.inc

--disable_query_log

--disable_result_log

--source $MYSQL_TEST_DIR/plugin/spider/spider/t/test_init.inc

--enable_result_log

--enable_query_log

--let $SOCKET=`SELECT @@global.socket`

DROP SERVER IF EXISTS srv;

eval CREATE SERVER srv FOREIGN DATA WRAPPER MYSQL OPTIONS(SOCKET "$SOCKET",DATABASE 'test',USER 'root',PASSWORD '');

CREATE TABLE t (c INT) ENGINE=InnoDB;

INSERT INTO t VALUES (0);

CREATE TABLE ts (c INT) ENGINE=Spider COMMENT='WRAPPER "mysql", srv "srv", TABLE "t"';

SELECT * FROM ts;

--exec $MYSQL_UPGRADE --force 2>&1

--exec rm $MYSQL_TEST_DIR/var/mysqld.1/data/mariadb_upgrade_info

SELECT * FROM ts;

DROP TABLE t, ts;

--disable_query_log

--disable_result_log

--source $MYSQL_TEST_DIR/plugin/spider/spider/t/test_deinit.inc

--enable_result_log

--enable_query_log

Ok, so here's an idea:

1. separate out the udf creation statements from spider_init_queries to say spider_bg_init_queries
2. "unpack" the procedures into inline statements, using create [column] if not exist etc. remove all queries to information_schema tables (to avoid assertion error of ma_inited), either by using idempotent operations where there were if's or assuming higher spider version. Though item 5 below may fix the assertion in which case we may not need to avoid querying information_schema tables
3. move the execution of spider_init_queries to the main thread, and remove the wait on the background thread in spider_create_handler(), like[1]
4. execute spider_bg_init_queries in a background thread*, pending server start, like how spider_init_queries are executed currently
5. adjust the order of storage engines, so that spider is loaded after aria, including when loaded with --plugin-load-add, so that we can create / alter table with engine=Aria

This way:

1. spider_init_queries does not depend on opening any existing system tables like mysql.proc (MDEV-29904)
2. by the time spider_create_handler() is called, all the spider system tables have been created (so no more ERROR: 12524 Can't open system table mysql.spider_table_sts), so it is ok at init to create spider tables (MDEV-27233) or drop non-existing tables (MDEV-22979)
3. the udf creation that requires opening mysql.func will only take place after the server starts.

The only caveat is that users may not be able to call any of the spider udf's during startup if they have not been created yet. But that seems acceptable.

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

I am marking the present issue as blocking MDEV-27233 and MDEV-22979, and will revise once I have tested this idea.

I have not looked into other related issues like MDEV-27103.

* This would a good opportunity to reduce the number of sts, crd, and init query bg threads to 1 (MDEV-27998).

The issues with the Spider init (MDEV-22979,MDEV-27233,MDEV-29904,MDEV-30370) is more complicated than I thought. We want to fix the former two bugs without causing regression in the latter two.

The spider init queries require:

• Aria engine, for creation of spider system tables (mysql.spider_tables, etc.)
• mysql database, for creation of spider system tables
• acl init etc., for creation of udfs. I have not looked into exactly what is needed for creation of udfs, but acl_init is a necessary condition. In any case, I assume udf creation can wait for the server start.

For Aria engine, we can rearrange the plugin init order so that dynamic plugins like the spider are initialised after builtin plugins like Aria. For udfs, we can separate out these queries to a background thread to wait for server start.

The problem is when we load spider in bootstrap. In mysqld_main() is that it does the following things in order

• initialises all plugins (init_server_components())
• acl_init
• executes bootstrap queries (bootstrap(mysql_stdin))
• executes init file queries (read_init_file(opt_init_file))

The mysql_install_db script passes the creation of mysql database as part of the bootstrap queries. AIUI mysqld_main() assumes at this stage any builtin or dynamic (from plugin-load-add) storage engine either have been initialised to accept these queries, or will be taken care of by the bootstrap queries ("insider help"). For example, innodb system tables mysql.innodb_table_stats and mysql.innodb_index_stats are created using bootstrap queries which guarantees the mysql database exists first.

Spider does not have any "insider help" and cannot assume that mysql database exists when it is being initialised. Worse, it is possible that the spider storage engine is used before the mysql database is creatred: consider the following bootstrap queries:

drop table if exists foo.bar;
create database mysql;

The drop table statement may require all storage engine to be initialised, but spider requires the mysql database for initialisation. If we put spider init queries into a background thread waiting for the creation of the mysql database, it will deadlock on these queries.

I can think of 3 ways to fix this, in the order of increasing amount of work:

1. advise users to not use plugin-load-add=ha_spider, but use init_file with a file including install soname ha_spider; instead. If possible, abort with an error on or ignore plugin-load-add=ha_spider. This also means no spider during bootstrap because --bootstrap causes the server to exit upon execution of the bootstrap queries thus skipping everything that may happen afterwards including init_file. It is tempting to ban plugin-load-add=ha_spider only for bootstrap, but given the complexity introduced by the plugin init reordering and udf creation background thread and the risk of race conditions and deadlocks, it is much simpler to just run all the init queries synchronously and ban plugin-load-add=ha_spider altogether.
2. make bootstrap+spider invoke a init sql script instead ("insider help"). A bit like the good old install_spider.sql, except that 1) now we place it alongside other bootstrapping scripts make it so that it is run only with say an --install_spider flag, 2) the init of spider outside of bootstrapping (e.g. with --init_file or install soname) still follow the current code path of doing it in C++, both coming from a single source of truth.
3. full on resurrection of install_spider.sql. I am not sure what this entails. But will look more into it if needed.

For simplicity, I think the first option is the best for now.

I don't think disallowing plugin-load-add=ha_spider is an option. It breaks the rather basic user expectation, and I'm afraid it's a rather slippery road to start on. Making spider to skip its init queries on bootstrap — that would be fine. We'll still need to solve the dependency problem — the fact that some plugins might need other plugins as prerequisites had always been an issue that needed to be fixed, but we've successfully managed to sidestep it for many years.

moving spider init queries out of ha_spider.so is, basically, MDEV-19850

spider init queries can be simplified a great deal too. No need to query I_S tables all the time. But the dependency on Aria won't go away, because of alter table mysql.spider_link_failed_log engine=Aria etc.

Thanks for the comment serg. As with the discussions at #spider, after refining on the first idea above, I think it is worth trying to re-interpret plugin-load-add=ha_spider as a "internal" file containing install soname ha_spider;. Here is a PoC: https://github.com/MariaDB/server/commit/603a4c218b7

This way we can get rid of the bg thread completely. It also means we can decouple the present issue from MDEV-22979 and MDEV-27233 as by the time of executing the init queries in the internal file it will have all it needs. A clean-up of the queries is still useful though.

Hi holyfoot, PTAL thanks:

• https://github.com/MariaDB/server/commit/27f33965ef0
• https://github.com/MariaDB/server/commit/cb0e0c915f6

The commit to fix the init bugs MDEV-22979 MDEV-27233 will be based on these ones

I wrote a testcase with plugin-load-add=ha_spider in a .cnf file, and each time the server starts, spider gets loaded fine:

# ~/source/mariadb-server/mdev-22979/src/storage/spider/mysql-test/spider/bugfix/t/plugin_load_add_repeatedly.test

--echo #

--echo # plugin-load-add=ha_spider

--echo #

--let SEARCH_PATTERN=\\[Note\\] Loading of SPIDER[a-zA-Z0-9_]* from ha_spider(|.so|.dll) is deferred to a temporary init file

--let SEARCH_FILE=`select regexp_replace(@@tmpdir, '^.*/','')`

--let SEARCH_FILE=$MYSQLTEST_VARDIR/log/$SEARCH_FILE.err

--source include/search_pattern_in_file.inc

select * from mysql.plugin;

create table t (c int) Engine=SPIDER;

--source include/restart_mysqld.inc

--source include/search_pattern_in_file.inc

drop table t;

# ~/source/mariadb-server/mdev-22979/src/storage/spider/mysql-test/spider/bugfix/t/plugin_load_add_repeatedly.cnf

[mysqld.1.1]

plugin-load-add=ha_spider.so

# the test result:

#

# plugin-load-add=ha_spider

#

FOUND 3 /\[Note\] Loading of SPIDER[a-zA-Z0-9_]* from ha_spider(|.so|.dll) is deferred to a temporary init file/ in mysqld.1.1.err

select * from mysql.plugin;

name	dl

create table t (c int) Engine=SPIDER;

# restart

FOUND 6 /\[Note\] Loading of SPIDER[a-zA-Z0-9_]* from ha_spider(|.so|.dll) is deferred to a temporary init file/ in mysqld.1.1.err

drop table t;

ok to push.
It seems to me i already reviewed these patches. Hope i reviewed proper ones this time

Thanks holyfoot. Yes we can consider this as part of MDEV-22979 as
some of the patches there are named after this ticket. Marking as
blocked by that ticket.

Updates on 2023-10-06: close as duplicate to MDEV-22979.