Hi Vicentiu,
The patch looks fine. Thanks!
I have only small comments below:
>
> [Commits] 974064cde3c: MDEV-12666: CURRENT_ROLE() and DATABASE() does not
> work in a view
> vicentiu vicentiu at mariadb.org
> Thu Jun 8 10:11:38 EEST 2017
>
> Previous message (by thread): [Commits] 7f2d3c3: MDEV-11084 Select
> statement with partition selection against MyISAM table opens all partitions.
> Next message (by thread): [Commits] d03abc71a49: MDEV-12609: Allow
> suppression of InnoDB log messages about reserving extents
> Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
>
> revision-id: 974064cde3c1cd1be9157bc11ce410604807f68b
> (mariadb-10.0.31-2-g974064cde3c)
> parent(s): 54cd5bc703da08be45894495d7f3e2c8117617cd
> author: Vicențiu Ciorbaru
> committer: Vicențiu Ciorbaru
> timestamp: 2017-06-08 10:10:07 +0300
> message:
>
> MDEV-12666: CURRENT_ROLE() and DATABASE() does not work in a view
>
> The problem lies in how CURRENT_ROLE is defined. The
> Item_func_current_role inherits from Item_func_sysconst, which defines
> a safe_charset_converter to be a const_charset_converter.
>
> During view creation, if there is no role previously set, the current_role()
> function returns NULL.
>
> This is captured on item instantiation and the
> const_charset_converter call subsequently returns an Item_null.
> In turn, the function is replaced with Item_null and the view is
> then created with an Item_null instead of Item_func_current_role.
>
> Without this patch, the first SHOW CREATE VIEW from the testcase would
> have a where clause of WHERE role_name = NULL, while the second SHOW
> CREATE VIEW would show a correctly created view.
>
> The same applies for the DATABASE function, as it can change as well.
>
> There is an additional problem with CURRENT_ROLE() when used in a
> prepared statement. During prepared statement creation we used to set
> the string_value of the function to the current role as well as the
> null_value flag. During execution, if CURRENT_ROLE was not null, the
> null_value flag was never set to not-null during fix_fields.
>
> Item_func_current_user however can never be NULL so it did not show this
> problem in a view before. At the same time, the CURRENT_USER() can not
> be changed between prepared statement execution and creation so the
> implementation where the value is stored during fix_fields is
> sufficient.
>
> Note also that DATABASE() function behaves differently during prepared
> statements. See bug 25843 for details or commit
> 7e0ad09edff587dadc3e9855fc81e1b7de8f2199
>
<cut>
> --- a/sql/item_strfunc.cc
> +++ b/sql/item_strfunc.cc
> @@ -2344,6 +2344,7 @@ String *Item_func_database::val_str(String *str)
> }
> else
> str->copy(thd->db, thd->db_length, system_charset_info);
> + null_value= 0;
> return str;
> }
>
> @@ -2378,6 +2379,30 @@ bool Item_func_user::init(const char *user, const char
> *host)
> }
>
>
> +Item *Item_func_sysconst::safe_charset_converter(CHARSET_INFO *tocs)
> +{
> + /*
> + During view or prepared statement creation, the item should not
> + make use of const_charset_converter as it would imply substitution
> + with constant items which is not correct. Functions can have different
> + values during view creation and view execution based on context.
> +
> + Return the identical item during view creation and prepare.
> + */
> + if (current_thd->lex->context_analysis_only &
> + (CONTEXT_ANALYSIS_ONLY_PREPARE | CONTEXT_ANALYSIS_ONLY_VIEW))
> + return this;
> + return const_charset_converter(tocs, true, fully_qualified_func_name());
I propose to avoid code duplication and do like this:
if (!Item_func_sysconst::const_item())
return this;
return return const_charset_converter(tocs, true,
fully_qualified_func_name());
> +}
> +
> +bool Item_func_sysconst::const_item() const
> +{
> + if (current_thd->lex->context_analysis_only &
> + (CONTEXT_ANALYSIS_ONLY_PREPARE | CONTEXT_ANALYSIS_ONLY_VIEW))
> + return false;
> + return true;
> +}
> +
> bool Item_func_user::fix_fields(THD *thd, Item **ref)
> {
> return (Item_func_sysconst::fix_fields(thd, ref) ||
> @@ -2403,20 +2428,25 @@ bool Item_func_current_role::fix_fields(THD *thd,
> Item **ref)
>
> Security_context *ctx= context->security_ctx
> ? context->security_ctx : thd->security_ctx;
> -
> if (ctx->priv_role[0])
> {
> if (str_value.copy(ctx->priv_role, strlen(ctx->priv_role),
> system_charset_info))
> return 1;
> -
> str_value.mark_as_const();
> + null_value= maybe_null= 0;
> return 0;
> }
> null_value= maybe_null= 1;
> return 0;
> }
>
> +String* Item_func_current_role::val_str(String * str)
> +{
> + DBUG_ASSERT(fixed == 1);
> + return null_value ? NULL : &str_value;
> +}
> +
Why did you move val_str() from *.h to *.cc ?
The method is just two lines, I'd leave it in *.h
<cut>
_______________________________________________
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
