Re: bk commit into 5.0 tree (davi:1.2516) BUG#21975

[Konstantin Osipov]

Hello Davi,

* Davi Arnaut <davi@stripped> [07/08/21 00:27]:

> ChangeSet@stripped, 2007-08-20 16:58:03-03:00, davi@stripped +6 -0
>   Bug#21975 Grant and revoke statements are non-transactional
>   Bug#17244 GRANT gives strange error message when used in a stored function

Thank you for looking into this.

Please see my comments on this patch below.
>   
>   All statements which commit (implicit or explicitly) or rollbacks a transaction are
> prohibited inside of stored functions or triggers.
>   
>   Patch contributed by Vladimir Shebordaev

The patch should go into 5.1 tree, since it contains an
incompatible change.

Since this is an incompatible change, the changeset comment should
mention that the manual needs to be updated.

Generally, the changeset comment should describe the problem, the
solution, incompatible changes, if any.

This patch fixes the following bugs:

  21975 Grant and revoke statements are non-transactional
  21422 GRANT/REVOKE possible inside stored function, probably in a trigger
  17244 GRANT gives strange error message when used in a stored function

Please mention all of them in the changeset comment. You have
already added test cases for all of them.

>   mysql-test/r/grant-binlog.result@stripped, 2007-08-20 16:58:00-03:00, davi@stripped
> +52 -0
>     Add test result for Bug#21975 transaction
> 
>   sql/sql_parse.cc@stripped, 2007-08-20 16:58:00-03:00, davi@stripped +4 -0
>     End active transaction, effectively prohibiting commit in stored functions or
> triggers.

COMMIT or ROLLBACK are already prohibited in stored functions and
triggers. 

Please change the comment to:

End active transaction in SQLCOM_GRANT and SQLCOM_REVOKE,
and thus effectively prohibit these statements in stored functions
and triggers. An implicit commit also fixes a bug in replication,
when GRANT or REVOKE would disappear from the binary log in case
of a subsequent ROLLBACK, since they were considered transactional
statements.


> diff -Nrup a/mysql-test/r/grant-binlog.result b/mysql-test/r/grant-binlog.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/t/grant-binlog.test	2007-08-20 16:58:00 -03:00
> @@ -0,0 +1,39 @@

First of all, replication tests should be prefixed - so the name
should be rpl_binlog_grant or binlog_grant.

Secondly, I think there is already an existing test case you could
use:

rpl_ddl.test

> +-- source include/have_innodb.inc
> +-- source include/not_embedded.inc
> +
> +let $VERSION=`select version()`;
> +
> +# Bug #21975: grant/revoke statements in transaction
> +# used to disappear from binlog upon rallback.
> +# Now GRANT/REVOKE do implicitly commit
> +# transaction
> +
> +--disable-warnings
^^^
this is a typo.

Should be --disable_warnings

You could check in the result file - it still has the warnings.

> +drop database if exists d1;
> +create database d1;
> +use d1;
> +create table t (s1 int) engine=innodb;
> +set @@autocommit=0;
> +start transaction;
> +insert into t values (1);
> +grant select on t to x@y;
> +#
> +# There is no active transaction here
> +#
> +rollback;
> +show grants for x@y;
> +--replace_result $VERSION VERSION
> +show binlog events;
> +start transaction;
> +insert into t values (2);
> +revoke select on t from x@y;
> +#
> +# There is no active transaction here
> +#
> +commit;
> +select * from t;
> +show grants for x@y;
> +--replace_result $VERSION VERSION
> +show binlog events;
> +drop user x@y;
> +drop database d1;
> diff -Nrup a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test
> --- a/sql/sp_head.cc	2007-07-31 09:23:23 -03:00
> +++ b/sql/sp_head.cc	2007-08-20 16:57:59 -03:00
> @@ -242,11 +242,14 @@ sp_get_flags_for_command(LEX *lex)
>    case SQLCOM_CREATE_TRIGGER:
>    case SQLCOM_CREATE_USER:
>    case SQLCOM_ALTER_TABLE:
> +  case SQLCOM_GRANT:
> +  case SQLCOM_REVOKE:
>    case SQLCOM_BEGIN:
>    case SQLCOM_RENAME_TABLE:
>    case SQLCOM_RENAME_USER:
>    case SQLCOM_DROP_INDEX:
>    case SQLCOM_DROP_DB:
> +  case SQLCOM_REVOKE_ALL:
>    case SQLCOM_DROP_USER:
>    case SQLCOM_DROP_VIEW:
>    case SQLCOM_DROP_TRIGGER:

OK.

> --- a/sql/sql_parse.cc	2007-08-02 06:51:00 -03:00
> +++ b/sql/sql_parse.cc	2007-08-20 16:58:00 -03:00
> @@ -4061,6 +4061,8 @@ end_with_restore_list:
>      if (check_access(thd, UPDATE_ACL, "mysql", 0, 1, 1, 0) &&
>          check_global_access(thd,CREATE_USER_ACL))
>        break;
> +    if (end_active_trans(thd))
> +      goto error;

An implicit commit should be issued regardless of the success or
failure of privileges check.

If a statement has a side effect, which is evil already, at least
let's have it in all cases.

Therefore, 

if (end_active_trans(thd))
  goto error;

Should go before check_access.

Please add a comment for that.
      
>      /* Conditionally writes to binlog */
>      if (!(res = mysql_revoke_all(thd, lex->users_list)))
>        send_ok(thd);
> @@ -4077,6 +4079,8 @@ end_with_restore_list:
>                       select_lex->db ? is_schema_db(select_lex->db) : 0))
>        goto error;
>  
> +    if (end_active_trans(thd))
> +      goto error;

Same here.

>      if (thd->security_ctx->user)              // If not replication
>      {
>        LEX_USER *user, *tmp_user;

Thank you for looking into this,

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY