Hi, Alexander! On Dec 26, Alexander Barkov wrote: > Hi Sergei, > > can you please review a patch for MDEV-14603?
I agree with the fix. But I don't like that there are many things to backup/restore (Statement, arena, free_list, and now change_list), they're all saved/restored in different places - it's easy to miss something when making changes. Would it be possible to move all that saving/restoring into dedicated helpers and use them in all three places (prepare, execute, execute immediate)? > diff --git a/mysql-test/r/ps.result b/mysql-test/r/ps.result > index 6857ebc..a56920d 100644 > --- a/mysql-test/r/ps.result > +++ b/mysql-test/r/ps.result > @@ -5074,3 +5074,45 @@ t1 CREATE TABLE `t1` ( > ) ENGINE=MyISAM DEFAULT CHARSET=latin1 > DROP TABLE t1; > DROP PROCEDURE p1; > +# > +# MDEV-14603 signal 11 with short stacktrace > +# > +SET NAMES utf8; > +CREATE TABLE t1(i INT); > +CREATE PROCEDURE p1(tn VARCHAR(32)) > +EXECUTE IMMEDIATE CONCAT('ANALYZE TABLE ',tn); > +CALL p1('t1'); > +Table Op Msg_type Msg_text > +test.t1 analyze status Table is already up to date > +DROP PROCEDURE p1; > +DROP TABLE t1; > +SET NAMES utf8; > +CREATE PROCEDURE p1() > +EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1)); > +CALL p1(); > +DROP PROCEDURE p1; > +SET NAMES utf8; > +CREATE PROCEDURE p1() > +BEGIN > +PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1)); > +EXECUTE stmt; > +DEALLOCATE PREPARE stmt; > +END; > +$$ > +CALL p1(); > +DROP PROCEDURE p1; > +SET NAMES utf8; > +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) > +EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1)); > +CALL p1('x'); > +DROP PROCEDURE p1; > +SET NAMES utf8; > +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) > +BEGIN > +PREPARE stmt FROM 'SELECT ?'; > +EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1)); > +DEALLOCATE PREPARE stmt; > +END; > +$$ > +CALL p1('x'); > +DROP PROCEDURE p1; > diff --git a/mysql-test/t/ps.test b/mysql-test/t/ps.test > index a7683b5..640f479 100644 > --- a/mysql-test/t/ps.test > +++ b/mysql-test/t/ps.test > @@ -4529,3 +4529,62 @@ CREATE TABLE t1 AS SELECT @a AS a, @b AS b; > SHOW CREATE TABLE t1; > DROP TABLE t1; > DROP PROCEDURE p1; > + > + > +--echo # > +--echo # MDEV-14603 signal 11 with short stacktrace > +--echo # > + > +SET NAMES utf8; > +CREATE TABLE t1(i INT); > +CREATE PROCEDURE p1(tn VARCHAR(32)) > + EXECUTE IMMEDIATE CONCAT('ANALYZE TABLE ',tn); > +CALL p1('t1'); > +DROP PROCEDURE p1; > +DROP TABLE t1; > + > +SET NAMES utf8; > +CREATE PROCEDURE p1() > + EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1)); > +--disable_result_log > +CALL p1(); > +--enable_result_log > +DROP PROCEDURE p1; > + > +SET NAMES utf8; > +DELIMITER $$; > +CREATE PROCEDURE p1() > +BEGIN > + PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1)); > + EXECUTE stmt; > + DEALLOCATE PREPARE stmt; > +END; > +$$ > +DELIMITER ;$$ > +--disable_result_log > +CALL p1(); > +--enable_result_log > +DROP PROCEDURE p1; > + > +SET NAMES utf8; > +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) > + EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1)); > +--disable_result_log > +CALL p1('x'); > +--enable_result_log > +DROP PROCEDURE p1; > + > +SET NAMES utf8; > +DELIMITER $$; > +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) > +BEGIN > + PREPARE stmt FROM 'SELECT ?'; > + EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1)); > + DEALLOCATE PREPARE stmt; > +END; > +$$ > +DELIMITER ;$$ > +--disable_result_log > +CALL p1('x'); > +--enable_result_log > +DROP PROCEDURE p1; > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 19c714c..d63c42c 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -2830,6 +2830,25 @@ void mysql_sql_stmt_prepare(THD *thd) > DBUG_VOID_RETURN; > } > > + /* > + Make sure we call Prepared_statement::prepare() with an empty > + THD::change_list. It can be non-empty as the above > + LEX::get_dynamic_sql_string() calls fix_fields() for the Item > + containing the PS source, e.g. on character set conversion: > + > + SET NAMES utf8; > + DELIMITER $$ > + CREATE PROCEDURE p1() > + BEGIN > + PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1)); > + EXECUTE stmt; > + END; > + $$ > + DELIMITER ; > + CALL p1(); > + */ > + Item_change_list save_change_list; > + thd->change_list.move_elements_to(&save_change_list); > if (stmt->prepare(query.str, (uint) query.length)) > { > /* Statement map deletes the statement on erase */ > @@ -2840,6 +2859,7 @@ void mysql_sql_stmt_prepare(THD *thd) > SESSION_TRACKER_CHANGED(thd, SESSION_STATE_CHANGE_TRACKER, NULL); > my_ok(thd, 0L, 0L, "Statement prepared"); > } > + save_change_list.move_elements_to(&thd->change_list); > > DBUG_VOID_RETURN; > } > @@ -2871,7 +2891,30 @@ void mysql_sql_stmt_execute_immediate(THD *thd) > // See comments on thd->free_list in mysql_sql_stmt_execute() > Item *free_list_backup= thd->free_list; > thd->free_list= NULL; > + /* > + Make sure that we're call Prepared_statement::execute_immediate() > + with an empty THD::change_list. It can be non empty as the above > + LEX::prepared_stmt_params_fix_fields() and LEX::get_dynamic_str_string() > + call fix_fields() for the PS source and PS parameter Items and > + can do Item tree changes, e.g. on character set conversion: > + > + - Example #1: Item tree changes in get_dynamic_str_string() > + SET NAMES utf8; > + CREATE PROCEDURE p1() > + EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1)); > + CALL p1(); > + > + - Example #2: Item tree changes in prepared_stmt_param_fix_fields(): > + SET NAMES utf8; > + CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) > + EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING > latin1)); > + CALL p1('x'); > + */ > + Item_change_list save_change_list; > + thd->change_list.move_elements_to(&save_change_list); > (void) stmt->execute_immediate(query.str, (uint) query.length); > + thd->rollback_item_tree_changes(); > + save_change_list.move_elements_to(&thd->change_list); > thd->free_items(); > thd->free_list= free_list_backup; > > @@ -3269,7 +3312,29 @@ void mysql_sql_stmt_execute(THD *thd) > */ > Item *free_list_backup= thd->free_list; > thd->free_list= NULL; // Hide the external (e.g. "SET STATEMENT") Items > + /* > + Make sure we call Prepared_statement::execute_loop() with an empty > + THD::change_list. It can be non-empty because the above > + LEX::prepared_stmt_params_fix_fields() calls fix_fields() for > + the PS parameter Items and can do some Item tree changes, > + e.g. on character set conversion: > + > + SET NAMES utf8; > + DELIMITER $$ > + CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) > + BEGIN > + PREPARE stmt FROM 'SELECT ?'; > + EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1)); > + END; > + $$ > + DELIMITER ; > + CALL p1('x'); > + */ > + Item_change_list save_change_list; > + thd->change_list.move_elements_to(&save_change_list); > (void) stmt->execute_loop(&expanded_query, FALSE, NULL, NULL); > + thd->rollback_item_tree_changes(); > + save_change_list.move_elements_to(&thd->change_list); > thd->free_items(); // Free items created by execute_loop() > /* > Now restore the "external" (e.g. "SET STATEMENT") Item list. Regards, Sergei Chief Architect MariaDB and secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp