Hello Alik,
thank you for working on this.
* Alexander Nozdrin <alik@stripped> [07/08/22 14:51]:
Please push the comments, they are all fine.
See below the review for the patch.
> ChangeSet@stripped, 2007-08-22 14:33:23+04:00, anozdrin@stripped +7 -0
> Fix for BUG#25843: changing default database between PREPARE and EXECUTE
> of statement breaks binlog.
>
> There were two problems discovered by this bug:
>
> 1. Default (current) database is not fixed at the creation time.
> That leads to wrong output of DATABASE() function.
>
> 2. Database attributes (@@collation_database) are not fixed
> at the creation time. That leads to wrong resultset.
>
> Binlog breakage and Query Cache wrong output happened because of the first
> problem.
>
> The fix is to remember the current database at the PREPARE-time and set it
> each time at EXECUTE.
>
> mysql-test/r/ps-qc.result@stripped, 2007-08-22 14:33:13+04:00, anozdrin@stripped +174
> -0
> Result file.
>
> mysql-test/r/ps-qc.result@stripped, 2007-08-22 14:33:13+04:00, anozdrin@stripped +0 -0
>
> mysql-test/t/ps-qc.opt@stripped, 2007-08-22 14:33:13+04:00, anozdrin@stripped +1 -0
> ps-qc.test requires that the Query Cache is operational.
There is already such a test, query_cache_ps*
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/r/ps-qc.result 2007-08-22 14:33:13 +04:00
> @@ -0,0 +1,174 @@
> +########################################################################
> +#
> +# BUG#25843: Changing default database between PREPARE and EXECUTE of
> +# statement breaks binlog.
> +#
> +########################################################################
> +
> +#
> +EXECUTE stmt_d_1;
> +
> +FLUSH LOGS;
> +
> +SHOW BINLOG EVENTS FROM 106;
> +Log_name Pos Event_type Server_id End_log_pos Info
> +master-bin.000001 106 Query 1 193 use `test`; INSERT INTO t1 VALUES(1)
> +master-bin.000001 193 Query 1 280 use `test`; INSERT INTO t1 VALUES(1)
> +master-bin.000001 280 Rotate 1 324 master-bin.000002;pos=4
> +
> +DROP DATABASE mysqltest1;
This piece of the test should go to the replication test suite.
> @@ -988,7 +992,7 @@ exit:
> it to 0.
> */
> if (thd->db && !strcmp(thd->db, db))
> - thd->set_db(NULL, 0);
> + mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);
OK.
> + TODO: actually, new_db_name and new_db_name->str seem to be always
> + non-NULL. In case of stored program, new_db_name->str == "" and
> + new_db_name->length == 0.
The invariant is different.
It's either a valid database, or a NULL pointer.
Per-Erik broke it first, and then the code started to get
replicated.
sp_use_new_db shouldn't set an empty db string.
> + /* Report an error an free new_db_file_name. */
> +
an -> and
> my_error(ER_BAD_DB_ERROR, MYF(0), new_db_file_name.str);
> my_free(new_db_file_name.str, MYF(0));
> +
> + /* The operation failed. */
> +
> DBUG_RETURN(TRUE);
> }
> }
> @@ -1820,7 +1863,7 @@ bool mysql_rename_db(THD *thd, LEX_STRIN
>
> /* Step9: Let's do "use newdb" if we renamed the current database */
> if (change_to_newdb)
> - error|= mysql_change_db(thd, new_db, 0);
> + error|= mysql_change_db(thd, new_db, FALSE);
> +
> + /**
> + Name of the database that was current at the preparation of the
> + statement.
> + */
> + LEX_STRING db_name;
> };
Would it be possible to simply move THD::{db,db_length} from THD to Statement?
Then you could save the old db in stmt_backup.
> + /* Switch the current database (if needed). */
> +
> + if (thd->db && !db_name.str ||
> + !thd->db && db_name.str ||
> + strcmp(thd->db, db_name.str) != 0)
> + {
> + saved_db_name.str= thd->strmake(thd->db, thd->db_length);
> + saved_db_name.length= thd->db_length;
> +
> + cur_db_changed= TRUE;
> +
> + if (mysql_change_db(thd, &db_name, TRUE))
> + goto error;
> + }
Why weren't you able to use sp_use_new_db here?
--
-- Konstantin Osipov Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com The best DATABASE COMPANY in the GALAXY
