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>