Re: [Maria-developers] 2467eb2d314: MDEV-27743 Remove Lex::charset

2022-03-16 Thread Sergei Golubchik
Hi, Alexander,

I'd suggest to rename it the way you want in the server,
but keep it in libmariadb as is.

It's apparently part of the API and may be (just may be) some clients
use it.

On Mar 15, Alexander Barkov wrote:
> >> +
> >> +  void set_lex_collation(const Lex_collation_st &lc)
> >> +  {
> >> +charset= lc.collation();
> >> +if (lc.is_contextually_typed_collation())
> >> +  flags|= BINCMP_FLAG;
> >> +else
> >> +  flags&= ~BINCMP_FLAG;
> > 
> > Isn't COLLATE DEFAULT also contextually typed?
> 
> "COLLATE DEFAULT" and "COLLATE uca1400_as_ci" also
> reuse this flag. It should be renamed somehow.
> 
> What about CONTEXTUAL_COLLATION_FLAG ?
> 
> Note, it's defined in two places:
> 
> In the server:
> 
> include/mysql_com.h
> #define BINCMP_FLAG   131072U /* Intern: Used by sql_yacc */
> 
> In the client library:
> 
> libmariadb/include/mariadb_com.h
> #define BINCMP_FLAG   131072
> 
> The client library defines, but does not actually use it.
> Should we rename it in the client library? Or remove it?
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org

___
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


Re: [Maria-developers] 2467eb2d314: MDEV-27743 Remove Lex::charset

2022-03-13 Thread Sergei Golubchik
Hi, Alexander,

I had only a couple of questions/comments, see below:

On Mar 11, Alexander Barkov wrote:
> revision-id: 2467eb2d314 (mariadb-10.6.1-330-g2467eb2d314)
> parent(s): 61b72ed3dde
> author: Alexander Barkov
> committer: Alexander Barkov
> timestamp: 2022-02-10 10:27:55 +0400
> message:
> 
> MDEV-27743 Remove Lex::charset
> 
> This patch also fixes:
> MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column 
> definition

how do you plan to fix it in 10.2-10.8 ?

> 
> diff --git a/mysql-test/main/ctype_collate_column.result 
> b/mysql-test/main/ctype_collate_column.result
> new file mode 100644
> index 000..b20be5d3a48
> --- /dev/null
> +++ b/mysql-test/main/ctype_collate_column.result
> @@ -0,0 +1,450 @@
> +#
> +# Start of 10.9 tests
> +#
> +#
> +# MDEV-27743 Remove Lex::charset

1. Jira doesn't explain what user-visible changes one can expect.
If there are none, it's not clear why you needed this big test case

2. the test is totally unreadable. Could you do a normal test
instead? Like, put `select` instead of `execute immediate`
and convert the output to a .test file?

> +#
> +CREATE TABLE cs (cs VARCHAR(64) NOT NULL);
> +INSERT INTO cs (cs) VALUES
> +(''),
> +('CHARACTER SET latin1'),
> +('CHARACTER SET utf8mb4');
> +CREATE TABLE cl0 (cl0 VARCHAR(64) NOT NULL);
> +INSERT INTO cl0 (cl0) VALUES
> +(''),
> +('BINARY'),
> +('COLLATE DEFAULT'),
> +('COLLATE latin1_bin'),
> +('COLLATE latin1_swedish_ci'),
> +('COLLATE utf8mb4_bin'),
> +('COLLATE utf8mb4_general_ci');
> +CREATE TABLE cl1 (cl1 VARCHAR(64) NOT NULL);
> +INSERT INTO cl1 (cl1) VALUES
> +(''),
> +('COLLATE DEFAULT'),
> +('COLLATE latin1_bin'),
> +('COLLATE latin1_swedish_ci'),
> +('COLLATE utf8mb4_bin'),
> +('COLLATE utf8mb4_general_ci');
...
> diff --git a/mysql-test/main/ctype_latin1.result 
> b/mysql-test/main/ctype_latin1.result
> index 0b16d1cf6f2..70577f2c22c 100644
> --- a/mysql-test/main/ctype_latin1.result
> +++ b/mysql-test/main/ctype_latin1.result
> @@ -8889,6 +8889,38 @@ a  b
>  111  111
>  DROP TABLE t1;
>  #
> +# MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column 
> definition
> +#
> +CREATE TABLE t1 (a CHAR(10) CHARACTER SET latin1 COLLATE DEFAULT);
> +SHOW CREATE TABLE t1;
> +TableCreate Table
> +t1   CREATE TABLE `t1` (
> +  `a` char(10) DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;
> +SELECT CAST('a' AS CHAR(10) CHARACTER SET latin1 COLLATE DEFAULT);
> +CAST('a' AS CHAR(10) CHARACTER SET latin1 COLLATE DEFAULT)
> +a
> +CREATE TABLE t1 AS
> +SELECT CAST('a' AS CHAR(10) CHARACTER SET latin1 COLLATE DEFAULT) AS c1;
> +SHOW CREATE TABLE t1;
> +TableCreate Table
> +t1   CREATE TABLE `t1` (
> +  `c1` varchar(10) DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;
> +SELECT COLUMN_GET(COLUMN_CREATE(0, 'string'),0 AS CHAR CHARACTER SET latin1 
> COLLATE DEFAULT) AS c1;
> +c1
> +string
> +CREATE TABLE t1 AS
> +SELECT COLUMN_GET(COLUMN_CREATE(0, 'string'),0 AS CHAR CHARACTER SET latin1 
> COLLATE DEFAULT) AS c1;
> +SHOW CREATE TABLE t1;
> +TableCreate Table
> +t1   CREATE TABLE `t1` (
> +  `c1` longtext DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;

1. why is it in "10.2 tests" ?
2. shouldn't here be at least one test that COLLATE DEFAULT is not a no-op?
Like CREATE TABLE (... COLLATE DEFAULT ...) COLLATE _non_default ?
Or is it in your generated ctype_collate_column.test?

> +#
>  # End of 10.2 tests
>  #
>  #
> diff --git 
> a/mysql-test/suite/roles/create_and_drop_role_invalid_user_table.test 
> b/mysql-test/suite/roles/create_and_drop_role_invalid_user_table.test
> index 9cf0d7b4aff..dac6eab21e9 100644
> --- a/mysql-test/suite/roles/create_and_drop_role_invalid_user_table.test
> +++ b/mysql-test/suite/roles/create_and_drop_role_invalid_user_table.test
> @@ -36,7 +36,7 @@ set default role test_role for root@localhost;
>  drop role test_role;
>  drop user test_user@localhost;
>  
> -alter table user add column default_role char(80) binary default '' not null
> +alter table user add column default_role char(80) default '' not null
> COLLATE utf8_general_ci

Heh, interesting that it worked before
good catch

>  after is_role;
>  alter table user add max_statement_time decimal(12,6) default 0 not null
> diff --git a/sql/structs.h b/sql/structs.h
> index 9a5006e8b47..db065c49931 100644
> --- a/sql/structs.h
> +++ b/sql/structs.h
> @@ -599,11 +599,140 @@ class DDL_options: public DDL_options_st
...
> +class Lex_collation: public Lex_collation_st
> +{
> +public:
> +  Lex_collation(CHARSET_INFO *collation, Type type)
> +  {
> +m_collation= collation;
> +m_type= type;
> +  }
> +  static Lex_collation national(bool bin_mod)
> +  {
> +if (bin_mod)
> +  return Lex_collation(&my_charset_utf8mb3_bin, TYPE_EXPLICIT);
> +return Lex_collation(&my_charset_utf8mb3_general_ci, TYPE_IMPLICIT);

utf8mb3? not utf8?

> +  }