MDEV-34902: debian-start erroneously reports issues

[grooverdan]

• The Jira issue number for this PR is: MDEV-34902

Description

Due to table names not being escaped properly.

Also the CHECK TABLES on all tables was excessive. This was original there for MyISAM only tables that required repair. Aria and InnoDB will auto-recover.

Rather than script, take the input via the shell and attempt to push that back into SQL keeping all the quoting correclty, just use a stored procedure that is temporarly created.

Release Notes

Change Debian's on service start check script to only CHECK TABLE tblname FAST on MyISAM tables.

How can this PR be tested?

$ "$MARIADB" -S /tmp/build-mariadb-server-10.11.sock --skip-column-names --silent --batch --force -e '
    DELIMITER //
   CREATE OR REPLACE PROCEDURE mysql.check_myisam()
  BEGIN
  DECLARE done INT DEFAULT FALSE;
  DECLARE db TYPE OF information_schema.TABLES.TABLE_SCHEMA;
  DECLARE tbl TYPE OF information_schema.TABLES.TABLE_NAME;
  DECLARE cur CURSOR FOR SELECT TABLE_SCHEMA,TABLE_NAME FROM tlist; 
  DECLARE CONTINUE HANDLER FOR NOT FOUND SET done=TRUE;
  CREATE TEMPORARY TABLE tlist AS SELECT TABLE_SCHEMA,TABLE_NAME FROM information_schema.TABLES WHERE ENGINE="MyISAM";
    OPEN cur;
    read_loop: LOOP
      FETCH cur INTO db, tbl;
      IF done THEN
        LEAVE read_loop;
      END IF;
      EXECUTE IMMEDIATE CONCAT("CHECK TABLE `", db, "`.`", REPLACE(tbl, "`", "``"), "` FAST");
    END LOOP;
    CLOSE cur;
    DROP TEMPORARY TABLE tlist;
  END;
  //
  DELIMITER ;
  CALL mysql.check_myisam;
  DROP PROCEDURE mysql.check_myisam;
      '
test.table@name!	check	warning	1 client is using or hasn't closed the table properly
test.table@name!	check	status	OK
test.UserTable	check	warning	1 client is using or hasn't closed the table properly
test.UserTable	check	status	OK
test.123table	check	warning	1 client is using or hasn't closed the table properly
test.123table	check	status	OK
test.user-data	check	warning	1 client is using or hasn't closed the table properly
test.user-data	check	status	OK
test.user table	check	warning	1 client is using or hasn't closed the table properly
test.user table	check	status	OK
test.select	check	warning	1 client is using or hasn't closed the table properly
test.select	check	status	OK

Basing the PR against the correct MariaDB version

• This is a new feature or a refactoring, and the PR is based against the main branch.
• This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[vuvova]

Looks good, much simpler.

1. Why did you replace "tool args" with (tool args) ?
2. you missed a bit where I wrote "so let's also add support for FAST to InnoDB"

I looked at ha_innobase::check() implementation, it has a bunch of O(1) checks, like, various values from the header, and few O(N) checks, that scan the data. If I'm not mistaken QUICK disables almost all scanning checks, but may be one was left to work even in QUICK. I'm not sure about it. But if it's the case, please, make FAST to disable it too. Keep all O(1) checks, they can run even with FAST (because they are fast).

Or just confirm that I was wrong and QUICK in InnoDB doesn't do any O(N) checks. Then FAST can simply do the same.

[grooverdan]

Looks good, much simpler.

1. Why did you replace "tool args" with (tool args) ?

Being a better role model in how to execute programs with distinct arguments in bash.

2. you missed a bit where I wrote "so let's also add support for FAST to InnoDB"

I looked at ha_innobase::check() implementation, it has a bunch of O(1) checks, like, various values from the header, and few O(N) checks, that scan the data.

I saw it, but that would be an enhancement over the existing MyISAM/Aria only checks. I didn't think header corruption was a common problem in InnoDB, By using mariadb-check we're getting to the point all tables are opened (but the information_schema query previously probably did that also).

In what is probably a mistake, FAST would trigger the CHECK EXTENDED in InnoDB. QUICK avoids a btr_search_validate and the extended search (in what probably should check against T_EXTEND vs !T_QUICK).

If I'm not mistaken QUICK disables almost all scanning checks, but may be one was left to work even in QUICK. I'm not sure about it.

Quick seems to skip anything except a crashed table (mi_is_crashed / maria_is_crashed) in Aria/MyISAM which I was assuming was the main point of having this check in the start up script.

But if it's the case, please, make FAST to disable it too. Keep all O(1) checks, they can run even with FAST (because they are fast).

FAST will, in the case of uncrashed tables, skip further checking if ((param->testflag & T_FAST) && (share->state.open_count == (uint) (share->global_changed ? 1 :0)" (MyISAM/Aria).

Or just confirm that I was wrong and QUICK in InnoDB doesn't do any O(N) checks. Then FAST can simply do the same.

I can't tell if row_check_index or row_count_rtree_recs run under QUICK are O(N) checks.

• QUICK - does no checking non-crashed Aria/MyISAM tables. It avoids an extended InnoDB check.
• FAST - does checks if tables are open/changed on Aria/MyISAM. An extended INNODB check and a AHI will be checked.

[vuvova]

I saw it, but that would be an enhancement over the existing MyISAM/Aria only checks

Unfortunately, mariadb-check doesn't have a mode "only check MyISAM/Aria", so our options are

• don't check anything
• check everything

you've apparently prefer the second, with

MARIADBCHECK=(/usr/bin/mariadb-check --defaults-extra-file=/etc/mysql/debian.cnf --fast --auto-repair --silent)
MARIADBCHECK_ALL=( "${MARIADBCHECK[@]}" --all-databases)

in that case we need to make sure that it will be fast inside InnoDB too. Otherwise it'll be a nasty surprise for all Debian users.

[grooverdan]

I saw it, but that would be an enhancement over the existing MyISAM/Aria only checks

Unfortunately, mariadb-check doesn't have a mode "only check MyISAM/Aria", so our options are

* don't check anything

* check everything

you've apparently prefer the second, with

MARIADBCHECK=(/usr/bin/mariadb-check --defaults-extra-file=/etc/mysql/debian.cnf --fast --auto-repair --silent)
MARIADBCHECK_ALL=( "${MARIADBCHECK[@]}" --all-databases)

in that case we need to make sure that it will be fast inside InnoDB too. Otherwise it'll be a nasty surprise for all Debian users.

The --quick option was pretty good for avoiding InnoDB EXTENDED checks and included checking the current read view which is pretty useless on a startup script, and for the current view of mariadb-check.

So with the rebase/push I'm going option a), don't check anything. With Aria and InnoDB being crash safe we've got no need to be doing checks.