maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr,

On Jan 06, Oleksandr Byelkin wrote:
> revision-id: 92ff948d021 (mariadb-10.3.37-53-g92ff948d021)
> parent(s): 180b2bcd538
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2022-12-05 20:42:03 +0100
> message:
> 
> MDEV-29231 View returns wrong value with SQL_MODE 'NO_BACKSLASH_ESCAPES'
> 
> 1. make view printing sql_mode independent
>   1.1. Print always current escape character
>   1.2. Allow escape character usage independent of sql_mode
> 2. Add workaround for parsing "like 3 in (0,1) escape 3" problem
>  (alwaysuse parances in case of printing ESCAPE)
> 
> diff --git a/mysql-test/main/ctype_cp1250_ch.result b/mysql-test/main/ctype_cp1250_ch.result
> index b0aa4cff382..88175ae5013 100644
> --- a/mysql-test/main/ctype_cp1250_ch.result
> +++ b/mysql-test/main/ctype_cp1250_ch.result
> @@ -129,7 +129,7 @@ EXPLAIN EXTENDED SELECT * FROM t1 WHERE CONCAT(c1)='a' AND CONCAT(c1) LIKE 'a ';
>  id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
>  1	SIMPLE	t1	ALL	NULL	NULL	NULL	NULL	2	100.00	Using where
>  Warnings:
> -Note	1003	select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where concat(`test`.`t1`.`c1`) = 'a' and concat(`test`.`t1`.`c1`) like 'a '
> +Note	1003	select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where concat(`test`.`t1`.`c1`) = 'a' and concat(`test`.`t1`.`c1`) like ('a ') escape '\\'

I don't think there's a need to print the escape clause here, it only
makes the output less readable.

> diff --git a/mysql-test/main/default.result b/mysql-test/main/default.result
> index c9ac96367aa..5c72b894bf6 100644
> --- a/mysql-test/main/default.result
> +++ b/mysql-test/main/default.result
> @@ -1936,7 +1936,7 @@ CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-0
>  SHOW CREATE TABLE t1;
>  Table	Create Table
>  t1	CREATE TABLE `t1` (
> -  `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01' and '2012-12-12'))
> +  `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01' and '2012-12-12') escape '\\')

So, there're basically two approaches to a fix and we're inconsistently
using both for different items.

1. one can always print expressions in the environment-independent way
   - create longer less readable text
   - not all items support it, new syntax might be needed
     (cannot emulate NO_BACKSLASH_ESCAPES with the ESCAPE clause)

2. one can set the environment (sql_mode) to the same hard-coded value
   before parsing views and vcols
   - one cannot dump the view/table definition in one sql_mode and load
     in another
     + this isn't normally a problem, as mysqldump also uses a fixed
     sql_mode
   - some item properties (what are they?) might be only set at
     fix_fields, fixing sql_mode during parsing won't help there

Could we try to agree to one approach and use it consistently for all
items from now on?

> diff --git a/mysql-test/main/parser.result b/mysql-test/main/parser.result
> index 0bb4e82c8b8..79ec9f539eb 100644
> --- a/mysql-test/main/parser.result
> +++ b/mysql-test/main/parser.result
> @@ -1325,11 +1325,11 @@ select 1 between 2 in (3,4) and 5 AS `1 between (2 in (3,4)) and 5`
>  create or replace view v1 as select 1 between (2 like 3) and 4;
>  Select view_definition from information_schema.views where table_schema='test' and table_name='v1';
>  view_definition
> -select 1 between 2 like 3 and 4 AS `1 between (2 like 3) and 4`
> +select 1 between 2 like (3) escape '\\' and 4 AS `1 between (2 like 3) and 4`
>  create or replace view v1 as select 1 not between (2 like 3) and 4;
>  Select view_definition from information_schema.views where table_schema='test' and table_name='v1';
>  view_definition
> -select 1 not between 2 like 3 and 4 AS `1 not between (2 like 3) and 4`
> +select 1 not between 2 like (3) escape '\\' and 4 AS `1 not between (2 like 3) and 4`
>  drop view v1;
>  #
>  # Start of 10.2 tests
> diff --git a/mysql-test/main/precedence.result b/mysql-test/main/precedence.result
> index fc6579651b4..ce26ca2acf2 100644
> --- a/mysql-test/main/precedence.result
> +++ b/mysql-test/main/precedence.result
> @@ -113,7 +113,7 @@ NOT 2 != 3	NOT (2 != 3)	(NOT 2) != 3
>  create or replace view v1 as select NOT 2 LIKE 3, NOT (2 LIKE 3), (NOT 2) LIKE 3;
>  Select view_definition from information_schema.views where table_schema='test' and table_name='v1';
>  view_definition
> -select 2  not like 3 AS `NOT 2 LIKE 3`,2  not like 3 AS `NOT (2 LIKE 3)`,!2 like 3 AS `(NOT 2) LIKE 3`
> +select 2  not like (3) escape '\\' AS `NOT 2 LIKE 3`,2  not like (3) escape '\\' AS `NOT (2 LIKE 3)`,!2 like (3) escape '\\' AS `(NOT 2) LIKE 3`
>  select NOT 2 LIKE 3, NOT (2 LIKE 3), (NOT 2) LIKE 3 union select * from v1;
>  NOT 2 LIKE 3	NOT (2 LIKE 3)	(NOT 2) LIKE 3
>  1	1	0
> diff --git a/mysql-test/main/precedence_bugs.result b/mysql-test/main/precedence_bugs.result
> index 723ab823b48..9654e3c31db 100644
> --- a/mysql-test/main/precedence_bugs.result
> +++ b/mysql-test/main/precedence_bugs.result
> @@ -54,7 +54,7 @@ drop table t1;
>  create view v1 as select 1 like (now() between '2000-01-01' and '2012-12-12' );
>  show create view v1;
>  View	v1
> -Create View	CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select 1 like (current_timestamp() between '2000-01-01' and '2012-12-12') AS `1 like (now() between '2000-01-01' and '2012-12-12' )`
> +Create View	CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select 1 like (current_timestamp() between '2000-01-01' and '2012-12-12') escape '\\' AS `1 like (now() between '2000-01-01' and '2012-12-12' )`
>  character_set_client	latin1
>  collation_connection	latin1_swedish_ci
>  drop view v1;

if you'll always print ESCAPE clause, it'll make many tests in
parser.test, precedence.test and precedence_bugs.test meaningless.
Don't just update result files, please, remove tests that no longer make
sense. Or rewrite them, when possible (but in precedence.test it's not).

> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> index b7b0c981c2d..702689a7496 100644
> --- a/sql/item_cmpfunc.cc
> +++ b/sql/item_cmpfunc.cc
> @@ -5262,10 +5262,15 @@ void Item_func_like::print(String *str, enum_query_type query_type)
>      str->append(STRING_WITH_LEN(" not "));
>    str->append(func_name());
>    str->append(' ');
> -  if (escape_used_in_parsing)
> +  if (escape_used_in_parsing || (query_type | QT_VIEW_INTERNAL))
>    {
> -    args[1]->print_parenthesised(str, query_type, precedence());
> -    str->append(STRING_WITH_LEN(" escape "));
> +    /*
> +      Explicit parencies is workaround against parser bug which üprevent
> +      patsing "like 3 in (0,1) escape 3" correctly
> +    */
> +  str->append('(');
> +    args[1]->print(str, query_type);
> +    str->append(STRING_WITH_LEN(") escape "));

no, this is wrong. Don't just force parentheses there. Instead, fix
the precedence, so that the parentheses would be printed automatically
when needed.

>      escape_item->print_parenthesised(str, query_type, higher_precedence());
>    }
>    else
> @@ -5390,10 +5395,7 @@ bool fix_escape_item(THD *thd, Item *escape_item, String *tmp_str,
>      if (escape_str)
>      {
>        const char *escape_str_ptr= escape_str->ptr();
> -      if (escape_used_in_parsing && (
> -             (((thd->variables.sql_mode & MODE_NO_BACKSLASH_ESCAPES) &&
> -                escape_str->numchars() != 1) ||
> -               escape_str->numchars() > 1)))
> +      if (escape_used_in_parsing && escape_str->numchars() > 1)

I still think that ESCAPE '' doesn't mean "no escape character".
I tried and in my tests it didn't.

>        {
>          my_error(ER_WRONG_ARGUMENTS,MYF(0),"ESCAPE");
>          return TRUE;
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx