- developers - lists.mariadb.org

Re: [Maria-developers] 9aace068f0c: MDEV-21743 Split up SUPER privilege to smaller privileges
by Sergei Golubchik 02 Mar '20

02 Mar '20

Hi, Alexander! Just a couple of issues that were left open after our slack chat. Everything else looks fine, nothing new below. On Mar 01, Alexander Barkov wrote: > revision-id: 9aace068f0c (mariadb-10.5.0-277-g9aace068f0c) > parent(s): 607960c7722 > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2020-03-01 11:49:25 +0400 > message: > > MDEV-21743 Split up SUPER privilege to smaller privileges > +constexpr uint PRIVILEGE_T_MAX_BIT= 36; > + > +static_assert((privilege_t)(1ULL << PRIVILEGE_T_MAX_BIT) == LAST_CURRENT_ACL, > + "LAST_CURRENT_ACL and PRIVILEGE_T_MAX_BIT do not match"); I'd still prefer to get rid of static_assert here. E.g. with constexpr uint PRIVILEGE_T_MAX_BIT= my_bit_log2(LAST_CURRENT_ACL); (and fixing my_bit_log2 to work with ulonglong). > @@ -7151,6 +7150,18 @@ bool check_global_access(THD *thd, privilege_t want_access, bool no_errors) > { > #ifndef NO_EMBEDDED_ACCESS_CHECKS > char command[128]; > + /* > + The userstat plugin in > + plugin/userstat/client_stats.cc > + plugin/userstat/user_stats.cc > + calls > + check_global_access(SUPER_ACL | PROCESS_ACL) > + when populating CLIENT_STATISTICS and USER_STATISTICS > + INFORMATION_SCHEMA tables. > + We should eventually fix it to use one privilege only and uncomment > + the DBUG_ASSERT below: > + */ > + //DBUG_ASSERT(my_count_bits(want_access) <= 1); I think you can remove SUPER_ACL from the userstat plugin and uncomment the assert above. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 256753e8ae8: Clean up and speed up interfaces for binary row logging
by Sergei Golubchik 01 Mar '20

01 Mar '20

Hi, Michael! Just a few comments, see below On Feb 28, Michael Widenius wrote: > revision-id: 256753e8ae8 (mariadb-10.5.0-269-g256753e8ae8) > parent(s): 9fd961cf687 > author: Michael Widenius <monty(a)mariadb.com> > committer: Michael Widenius <monty(a)mariadb.com> > timestamp: 2020-02-26 16:05:53 +0200 > message: > > Clean up and speed up interfaces for binary row logging ... > diff --git a/mysql-test/suite/rpl/r/create_or_replace_mix.result b/mysql-test/suite/rpl/r/create_or_replace_mix.result > index 661278aa7ef..6c83d27eef9 100644 > --- a/mysql-test/suite/rpl/r/create_or_replace_mix.result > +++ b/mysql-test/suite/rpl/r/create_or_replace_mix.result > @@ -223,26 +226,12 @@ Log_name Pos Event_type Server_id End_log_pos Info > slave-bin.000001 # Gtid # # GTID #-#-# > slave-bin.000001 # Query # # use `test`; create table t1 (a int) > slave-bin.000001 # Gtid # # BEGIN GTID #-#-# > -slave-bin.000001 # Annotate_rows # # insert into t1 values (0),(1),(2) > -slave-bin.000001 # Table_map # # table_id: # (test.t1) > -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F > +slave-bin.000001 # Query # # use `test`; insert into t1 values (0),(1),(2) why is it STATEMENT now? > slave-bin.000001 # Query # # COMMIT > -slave-bin.000001 # Gtid # # BEGIN GTID #-#-# > -slave-bin.000001 # Query # # use `test`; CREATE TABLE `t2` ( > - `a` int(11) DEFAULT NULL > -) ENGINE=MyISAM > -slave-bin.000001 # Annotate_rows # # create table t2 engine=myisam select * from t1 > -slave-bin.000001 # Table_map # # table_id: # (test.t2) > -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F > -slave-bin.000001 # Query # # COMMIT > -slave-bin.000001 # Gtid # # BEGIN GTID #-#-# > -slave-bin.000001 # Query # # use `test`; CREATE OR REPLACE TABLE `t2` ( > - `a` int(11) DEFAULT NULL > -) ENGINE=InnoDB > -slave-bin.000001 # Annotate_rows # # create or replace table t2 engine=innodb select * from t1 > -slave-bin.000001 # Table_map # # table_id: # (test.t2) > -slave-bin.000001 # Write_rows_v1 # # table_id: # flags: STMT_END_F > -slave-bin.000001 # Xid # # COMMIT /* XID */ > +slave-bin.000001 # Gtid # # GTID #-#-# > +slave-bin.000001 # Query # # use `test`; create table t2 engine=myisam select * from t1 > +slave-bin.000001 # Gtid # # GTID #-#-# > +slave-bin.000001 # Query # # use `test`; create or replace table t2 engine=innodb select * from t1 > connection server_1; > drop table t1; > # > diff --git a/mysql-test/suite/rpl/t/rpl_foreign_key.test b/mysql-test/suite/rpl/t/rpl_foreign_key.test > new file mode 100644 > index 00000000000..50be97af24d > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_foreign_key.test > @@ -0,0 +1,18 @@ > +--source include/have_innodb.inc > +--source include/have_binlog_format_row.inc > + > +CREATE TABLE t1 ( > + id INT, > + k INT, > + c CHAR(8), > + KEY (k), > + PRIMARY KEY (id), > + FOREIGN KEY (id) REFERENCES t1 (k) > +) ENGINE=InnoDB; > +LOCK TABLES t1 WRITE; > +SET SESSION FOREIGN_KEY_CHECKS= OFF; > +SET AUTOCOMMIT=OFF; > +INSERT INTO t1 VALUES (1,1,'foo'); > +DROP TABLE t1; > +SET SESSION FOREIGN_KEY_CHECKS= ON; > +SET AUTOCOMMIT=ON; What kind of replication test is it? You don't check binlog events, you don't compare master and slave, you don't even run anything on the slave to check whether your staments were replicated correctly. In fact, you don't have any slave, you have not included master-slave.inc, you only have binlog, so this test should be in the binlog suite, not in the rpl suite - it's a binlog test, not replication test. And even in the binlog suite it would make sense to see what's actually in a binlog. Just as a test that it's ok. > diff --git a/sql/ha_sequence.cc b/sql/ha_sequence.cc > index 6cb9937ebb4..71da208d775 100644 > --- a/sql/ha_sequence.cc > +++ b/sql/ha_sequence.cc > @@ -202,7 +202,11 @@ int ha_sequence::write_row(const uchar *buf) > DBUG_ENTER("ha_sequence::write_row"); > DBUG_ASSERT(table->record[0] == buf); > > - row_already_logged= 0; > + /* > + Log to binary log even if this function has been called before > + (The function ends by setting row_logging to 0) > + */ > + row_logging= row_logging_init; this is a sequence-specific hack, so you should define row_logging_init in ha_sequence class, not in the base handler class > if (unlikely(sequence->initialized == SEQUENCE::SEQ_IN_PREPARE)) > { > /* This calls is from ha_open() as part of create table */ > diff --git a/sql/handler.cc b/sql/handler.cc > index 1e3f987b4e5..4096ae8b90f 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6224,32 +6225,37 @@ bool ha_show_status(THD *thd, handlerton *db_type, enum ha_stat_type stat) > 1 Row needs to be logged > */ > > -bool handler::check_table_binlog_row_based(bool binlog_row) > +bool handler::check_table_binlog_row_based() > { > - if (table->versioned(VERS_TRX_ID)) > - return false; > - if (unlikely((table->in_use->variables.sql_log_bin_off))) > - return 0; /* Called by partitioning engine */ > #ifdef WITH_WSREP > - if (!table->in_use->variables.sql_log_bin && > - wsrep_thd_is_applying(table->in_use)) > - return 0; /* wsrep patch sets sql_log_bin to silence binlogging > - from high priority threads */ > #endif /* WITH_WSREP */ That's an empty #ifdef :) > if (unlikely((!check_table_binlog_row_based_done))) > { > check_table_binlog_row_based_done= 1; > check_table_binlog_row_based_result= > - check_table_binlog_row_based_internal(binlog_row); > + check_table_binlog_row_based_internal(); > } > return check_table_binlog_row_based_result; > } > > -bool handler::check_table_binlog_row_based_internal(bool binlog_row) > +bool handler::check_table_binlog_row_based_internal() > { > THD *thd= table->in_use; > > +#ifdef WITH_WSREP > + if (!thd->variables.sql_log_bin && > + wsrep_thd_is_applying(table->in_use)) > + { > + /* > + wsrep patch sets sql_log_bin to silence binlogging from high > + priority threads > + */ > + return 0; > + } > +#endif > return (table->s->can_do_row_logging && > + !table->versioned(VERS_TRX_ID) && > + !(thd->variables.option_bits & OPTION_BIN_TMP_LOG_OFF) && > thd->is_current_stmt_binlog_format_row() && > /* > Wsrep partially enables binary logging if it have not been > @@ -6769,13 +6718,17 @@ int handler::ha_write_row(const uchar *buf) > { error= write_row(buf); }) > > MYSQL_INSERT_ROW_DONE(error); > - if (likely(!error) && !row_already_logged) > + if (likely(!error)) > { > rows_changed++; > - error= binlog_log_row(table, 0, buf, log_func); > + if (row_logging) > + { > + Log_func *log_func= Write_rows_log_event::binlog_row_logging_function; > + error= binlog_log_row(table, 0, buf, log_func); > + } > #ifdef WITH_WSREP > - if (table_share->tmp_table == NO_TMP_TABLE && > - WSREP(ha_thd()) && (error= wsrep_after_row(ha_thd()))) > + if (WSREP_NNULL(ha_thd()) && table_share->tmp_table == NO_TMP_TABLE && why did you swap tests? NO_TMP_TABLE check is cheaper (same in update and delete) > + !error && (error= wsrep_after_row(ha_thd()))) > { > DBUG_RETURN(error); > } > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 102416ec0a6..9e40c2ae8c8 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10506,10 +10506,10 @@ do_continue:; > No additional logging of query is needed > */ > binlog_done= 1; > + DBUG_ASSERT(new_table->file->row_logging); > new_table->mark_columns_needed_for_insert(); > thd->binlog_start_trans_and_stmt(); > - binlog_write_table_map(thd, new_table, > - thd->variables.binlog_annotate_row_events); > + thd->binlog_write_table_map(new_table, 1); does it mean you force annotations for ALTER TABLE even if they were configured off? And why would ALTER TABLE generate row events anyway? > } > if (copy_data_between_tables(thd, table, new_table, > alter_info->create_list, ignore, Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] GSoC-2020 Query
by Krishna Maheshwari 29 Feb '20

29 Feb '20

Hello, Everyone! I am new here and in open source too. I got to know about MariaDB through GSoC. I have intermediate level of knowledge in sql and have experience working in languages such as c, c++ and java. I did go through the list of tasks mentioned in the mariaDB GSoC 2020, and I was in particularly interested in the tasks listed under MariaDB Server: Optimizer. In that 1) Add FULL OUTER JOIN to MariaDB 2) Recursive CTE support for UPDATE (and DELETE) statements. As I understand the functioning of both of these constructs to some extent, I believe that I would be able to follow the control flow. I would really love to contribute to the code base and be of help but I just don't know where to start from.

1 0

Re: [Maria-developers] 5ae74b4823a: mysqld --help will now load mysqld.options table
by Sergei Golubchik 28 Feb '20

28 Feb '20

Hi, Michael! On Feb 28, Michael Widenius wrote: > revision-id: 5ae74b4823a (mariadb-10.5.0-274-g5ae74b4823a) > parent(s): ee73f2c6e71 > author: Michael Widenius <monty(a)mariadb.com> > committer: Michael Widenius <monty(a)mariadb.com> > timestamp: 2020-02-26 16:05:54 +0200 > message: > > mysqld --help will now load mysqld.options table mysql.plugin > > Changes: > - Initalize Aria early to allow it to load mysql.plugin table with --help > - Don't print 'aborting' when doing --help > - Don't write 'loose' error messages on log_warning < 2 (2 is default) > - Don't write warnings about disabled plugings when doing --help > - Don't write aria_log_control or aria log files when doing --help > - When using --help, open all Aria tables in read only mode (safety) > - If aria_init() fails, do a cleanup(). (Frees used memory) > - If aria_log_control is locked with --help, then don't wait 30 seconds > but instead return at once without initialzing Aria plugin. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index b2f8afca7a6..415a12f4783 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -8511,8 +8511,8 @@ static void option_error_reporter(enum loglevel level, const char *format, ...) > va_start(args, format); > > /* Don't print warnings for --loose options during bootstrap */ > - if (level == ERROR_LEVEL || !opt_bootstrap || > - global_system_variables.log_warnings) > + if (level == ERROR_LEVEL || > + (!opt_bootstrap && global_system_variables.log_warnings > 1)) You've completely suppressed all --loose warnings during bootstrap. Before your patch they were basically always enabled (because of log_warnings==2). I am not sure it's a good idea to disable warnings completely in bootstrap. If fact, I don't see why bootstrap should be special, so I'd simply remove !opt_bootstrap condition completely here. But if you want to keep it you can do something like global_system_variables > opt_bootstrap it'll make bootstrap a bit quieter than normal startup. > { > vprint_msg_to_log(level, format, args); > } > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index d7d7fcca4a2..31de259a218 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1679,7 +1680,22 @@ int plugin_init(int *argc, char **argv, int flags) > global_system_variables.table_plugin = > intern_plugin_lock(NULL, plugin_int_to_ref(plugin_ptr)); > DBUG_SLOW_ASSERT(plugin_ptr->ref_count == 1); > + } > + /* Initialize Aria plugin so that we can load mysql.plugin */ > + plugin_ptr= plugin_find_internal(&Aria, MYSQL_STORAGE_ENGINE_PLUGIN); > + DBUG_ASSERT(plugin_ptr || !mysql_mandatory_plugins[0]); > + if (plugin_ptr) > + { > + DBUG_ASSERT(plugin_ptr->load_option == PLUGIN_FORCE); > > + if (plugin_initialize(&tmp_root, plugin_ptr, argc, argv, false)) > + { > + if (!opt_help) > + goto err_unlock; > + plugin_ptr->state= PLUGIN_IS_DISABLED; > + } > + else > + aria_loaded= 1; > } > mysql_mutex_unlock(&LOCK_plugin); I think this should be done differently. In a completely opposite way. I had unfurtunately hard-coded MyISAM here. A proper fix here could be to remove this special treatment of MyISAM instead of adding another special treatment of Aria. Then plugin_init() could work like: * run dd_frm_type() for the mysql.plugin table - like it's done now * instead of hard-coding MyISAM (and Aria), find this engine name in the plugin_array[] (note, all builtin plugins are already there) * initialize it and (if successful) load mysql.plugin table It only concerns the sql_plugin.cc part of your commit. Your aria part of the commit is still needed, because a good-behaving engine has to be read-only in --help. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 044dfaff670: Replace handler::primary_key_is_clustered() with handler::ha_is_clustered_key()
by Sergei Golubchik 28 Feb '20

28 Feb '20

Hi, Michael! On Feb 28, Michael Widenius wrote: > revision-id: 044dfaff670 (mariadb-10.5.0-275-g044dfaff670) > parent(s): 5ae74b4823a > author: Michael Widenius <monty(a)mariadb.com> > committer: Michael Widenius <monty(a)mariadb.com> > timestamp: 2020-02-26 16:05:55 +0200 > message: > > Replace handler::primary_key_is_clustered() with handler::ha_is_clustered_key() > > This was done to both simplify the code and also to be easier to handle > storage engines that are clustered on some other index than the primary > key. No, I don't get it. Old method meant "Check if the key is a clustered and a reference key". Just "clustered" is marked with HA_CLUSTERED_INDEX. So, 1. You've renamed the method but the name does not match the semantics. It is called is_clustering_key() but it really means "clustered AND reference key" 2. What other engine can be where the reference key isn't a primary key? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-21580: Allow packed sort keys in sort buffer, part #3
by Sergey Petrunia 27 Feb '20

27 Feb '20

Hi Varun, Please more input below: Input on code structure: > SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort... > { > ... > if (!(sort_keys= filesort->make_sortorder(thd, join, first_table_bit))) > DBUG_RETURN(NULL); /* purecov: inspected */ The main effect of this function used to be to create Filesort::sortorder, and so the name 'make_sortorder' used to make sense. Now, it creates a Sort_keys object so the name is counter-intuitive. .. > uint sort_len= sortlength(thd, sort_keys, &multi_byte_charset, > &allow_packing_for_sortkeys); Another counter-intuitive name. Maybe this should be a method of sort_keys object with a different name? And maybe this call and make_sortorder should be moved together since they're logically doing the same thing? ... > Sort_keys* > Filesort::make_sortorder(THD *thd, JOIN *join, table_map first_table_bit) > { ... > if (!sortorder) > sortorder= (SORT_FIELD*) thd->alloc(sizeof(SORT_FIELD) * count); Using "if(!sortorder)" means the sort order can be already present? Is this because we're inside a subquery which we are re-executing? ... > Sort_keys* sort_keys= new Sort_keys(sort_keys_array); But then, we create sort_keys every time, and we do it on a MEM_ROOT which causes a potential O(#rows_examined) memory use. Is my logic correct? I think we should only call make_sortorder() if this hasn't already been done. Any objections to this? > > void Filesort_buffer::sort_buffer(const Sort_param *param, uint count) > { > > ... > qsort2_cmp cmp_func= param->using_packed_sortkeys() ? > get_packed_keys_compare_ptr() : > get_ptr_compare(size); > > my_qsort2(m_sort_keys, count, sizeof(uchar*), cmp_func, > param->using_packed_sortkeys() ? > (void*)param : > (void*) &size); This choose-the-comparison-function logic is duplicated in merge_buffers(). Can we have in one place? I would add appropriate members into class Sort_key (or Sort_param). > if (param->using_packed_addons() || param->using_packed_sortkeys()) > { > /* > The last record read is most likely not complete here. > We need to loop through all the records, reading the length fields, > and then "chop off" the final incomplete record. > */ Why change the identation of this block, from correct to invorrect one? Please move it back two spaces to the left. > static uint make_sortkey(Sort_param *param, uchar *to, uchar *ref_pos) > This function only seems to access Sort_param members that relate to Sort_keys. Did you consider making it accept Sort_keys as an argument? This seems like a sound idea to me: make_sortkey() is only concerned with making sort keys from original records. This is what Sort_keys class should be handling. Sort_param on the other hand covers the entire sorting process: the buffers, total # of rows, etc. Please give it a try. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 4c263b6d30b: MDEV-20632: Recursive CTE cycle detection using CYCLE clause
by Sergei Golubchik 27 Feb '20

27 Feb '20

Hi, Oleksandr! On Feb 26, Oleksandr Byelkin wrote: > On Wed, Feb 26, 2020 at 1:44 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > > Hi, Sanja! > > > > _some_ comments are below. > > > > The main thing, I still don't understand your changes in sql_select.cc. > > > > Why did you create separate counters and code paths with if() for CYCLE? > > I'd think it could be just a generalization of DISTINCT > > DISTINCT makes all fields distinct, except hidden, I need also except come > other (or better say by list of not hidden) Exactly. I thought DISTINCT will be just CYCLE with an empty list of non-distinct columns. Or something like that. Not like if (cycle) { cycle code } else { distinct code } > > > --- a/sql/sql_cte.cc > > > +++ b/sql/sql_cte.cc > > > @@ -982,6 +982,38 @@ With_element::rename_columns_of_derived_unit(THD *thd, > > > > are you sure what you're doing below can still be called > > "rename_columns_of_derived_unit" ? > > I can rename it to prepare_olumns_of_derived_unit, is it OK? anything you like :) > > > +opt_cycle: > > > + /* empty */ > > > + { $$= NULL; } > > > + | > > > + CYCLE_SYM > > > + { > > > + if (!Lex->curr_with_clause->with_recursive) > > > + { > > > + thd->parse_error(ER_SYNTAX_ERROR, $1.pos()); > > > + } > > > + } > > > + '(' with_column_list ')' > > > > Where did you see that the standard requires parentheses here? > > Maybe in ORACLE docs... but without it it creates a lot of conflicts. The standard says no parentheses. What kind of conflicts do you get? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] Lag Free ALTER Table for Slave
by Sachin Setiya 27 Feb '20

27 Feb '20

Hi Everyone! This mail will describe usage and implementation of Lag Free Alter. And this will also answer the question raised by Kristian and Simon J Mudd(in 2016) Desc:- This will split Alter into 2 different commits. START ALTER and COMMIT /ROLLBACK ALTER , Start Alter will be written in binlog as soon as we get the locks for the table, alter will proceeds as usual and at the time of writing binlog if alter is successful we will write COMMIT Alter other wise ROLLBACK Alter. Usage:- Using this feature is quite simple. 1. On master you have to turn on `BINLOG_SPLIT_ALTER` dynamic variable. 2. Slave must be using parallel replication. Advance Usage:- So alter is divided like this. 1. START identifier Actual_alter_stmt 2. COMMIT/ROLLBACK identifier Actual_alter_stmt. OR 2. COMMIT/ROLLBACK identifier ALTER identifier is thread_id. Questions by Simon Mudd. >>* this behaviour should be configurable? Yes. >> - new global variable on the master to allow injection of this changed event stream? Right , `BINLOG_SPLIT_ALTER` >> - a new session variable set in the session where the command is triggered ? Right , `BINLOG_SPLIT_ALTER` >> - on slave a setting to indicate how many INPLACE ALTER TABLEs can run at once? No setting so far , but I am thinking of maximum no of CONCURRENT ALTER = slave_parallel_threads >>* how does a DBA monitor what’s going on? >> - the progress and number of active DDL statements So as we there is 2 part, So progress will go like this 1. Executing of start alter (this will take most of time) 2. Waiting for commit/rollback Signal 3. Commit/ Rollback Alter. Number of active ALTER , these will create new threads so DBA can know using this or I am thinking of adding variable in SHOW SLAVE INFO. which will show active DDL. >> - please consider adding counters/metrics for: >> * number of “asynchronous DDLs” in progress / completed successfully / failed or rolled back Okay, We can have counter for these metrics. If i get time to implement this. >> * sizes of the DDL changes made so far Not sure if we need this. >> * number of threads busy in this state (maybe can be implied from SHOW PROCESSLIST or equivalent but nicer to have an easy to query metric) Show processlist will show the busy threads. Sample Output when concurrent alter is running on slave. slave_parallel_threads= 10 Every 1.0s: mysql -uroot -S /home/sachin/alter/build/mysql-test/var/tmp/mysqld.2.sock -e show processlist sachin-sp52: Wed Jan 22 16:56:00 2020 Id User Host db Command Time State Info Progress 1 system user NULL Daemon NULL InnoDB purge coordinator NULL 0.000 3 system user NULL Daemon NULL InnoDB purge worker NULL 0.000 2 system user NULL Daemon NULL InnoDB purge worker NULL 0.000 4 system user NULL Daemon NULL InnoDB purge worker NULL 0.000 5 system user NULL Daemon NULL InnoDB shutdown handler NULL 0.000 10 root localhost:44478 test Sleep 6 NULL 0.000 11 root localhost:44480 test Sleep 7 NULL 0.000 15 root localhost:44492 test Sleep 6 NULL 0.000 16 root localhost:44494 test Sleep 6 NULL 0.000 17 system user NULL Slave_IO 6 Waiting for master to send event NULL 0.000 19 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 22 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 20 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 23 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 24 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 25 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 26 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 21 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 27 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 28 system user NULL Slave_worker 6 Waiting for work from SQL thread NULL 0.000 18 system user NULL Slave_SQL 6 Slave has read all relay log; waiting for the slave I/O thread to update it NULL 0.000 29 system user test Slave_worker 6 NULL /*!100001 START 19 alter table t3 add column c int, force, algorithm=inplace */ 0.000 30 system user test Slave_worker 6 NULL /*!100001 START 17 alter table t1 add column c int, force, algorithm=inplace */ 0.000 31 system user test Slave_worker 6 NULL /*!100001 START 18 alter table t2 add column c int, force, algorithm=inplace */ 0.000 32 system user test Slave_worker 6 NULL /*!100001 START 20 alter table t4 add column c int, force, algorithm=inplace */ 0.000 33 system user test Slave_worker 6 NULL /*!100001 START 21 alter table t5 add column c int, force, algorithm=inplace */ 0.000 34 root localhost NULL Query 0 Init show processlist 0.000 >>* what happens if the commit/rollback ddl event never arrives? (the statement on the master could be skipped for one of several reasons) So here are 2 options If slave is still running DBA can manually run COMMIT/ROLLBACK identifier alter_stmt/ALTER , where identifier is same as START ALTER identifier. so this will either commit or rollback alter. If slave is not running. Since slave is not running all context is lost , so this will take as much as time a normal alter. >> - I assume that users on the slave see the old structure all the time until it completes Right, we work on copy of alter for myisam and for other case innodb will take care of it. >> - would this have a storage or performance penalty on the slave if the commit/rollback DDL event never arrives? Not performance but yes storage and ram penalty. >> - can a “commit” / “rollback” be manually triggered by the DBA in such circumstances? yes. >>* what happens if the server crashes while this process (or these processes) are ongoing when the server starts up again? So if we crash in START ALTER no issue , we can run it again , since it is transactional(we are working on copy so ..) If we crash after start alter and we receive COMMIT alter then it will be treated as normal alter. ROLLBACK ALTER will be instant. If we crash in COMMIT ALTER this will be same as crash in normal ALTER table , so DBA has to deal with it. Architecture:- Lets look at sample alter perf data for innodb and myisam Myisam + 96.72% 0.00% mysqld mysqld [.] mysql_parse + 96.72% 0.00% mysqld mysqld [.] mysql_execute_command + 96.72% 0.00% mysqld mysqld [.] Sql_cmd_alter_table::execute - 96.72% 0.00% mysqld mysqld [.] mysql_alter_table - mysql_alter_table - 96.63% copy_data_between_tables + 79.64% handler::ha_write_row + 6.24% TABLE::update_default_fields + 5.71% READ_RECORD::read_record + 1.87% do_copy_null 1.05% Field::do_field_int + 0.95% _mcount InnoDB + 41.27% 0.00% mysqld mysqld [.] mysql_parse + 41.27% 0.00% mysqld mysqld [.] mysql_execute_command + 41.27% 0.00% mysqld mysqld [.] Sql_cmd_alter_table::execute - 41.27% 0.00% mysqld mysqld [.] mysql_alter_table - mysql_alter_table - 41.27% mysql_inplace_alter_table - 41.15% handler::ha_inplace_alter_table ha_innobase::inplace_alter_table - row_merge_build_indexes - 41.00% row_merge_read_clustered_index + 22.88% row_merge_insert_index_tuples + 5.89% row_build_w_add_vcol + 5.20% BtrBulk::finish + 3.20% row_merge_buf_add + 0.87% page_cur_move_to_next 0.62% rec_get_offsets_func So as we can see most of the work is done by copy_data_between_tables/ mysql_inplace_alter_table. Master Side:- There is not much change into master side. So after locking table and before executing these functions we write START _id_ alter_stmt into binlog. And at the time of write_bin_log we write COMMIT _id_ alter_stmt into binlog. So alter statement will be divided into 2 , hence 2 gtid. START_ALTER will have special flag FL_START_ALTER_E1(4), this will be used on slave side to create new worker thread for start alter processing. Slave Side:- This require parallel replication on slave side. So in do_event when we get a gtid_event with FL_START_ALTER_E1, ::choose_thread will call rpl_parallel_add_extra_worker, which will create a new worker thread and this will do start alter processing. So the gtid_log_event and next Query_log_event will be scheduled to this new thread. And this thread will be exited as soon as alter is finished. On slave side START ALTER will binlog after successfully getting MDL lock and thd lock. Untill this point we are executed as DDL(so new GCO), but after getting locks we will call finish_event_group. So that new events can proceed in parallel. Then it will continue to execute code in mysql_alter_table until we reach a non transactional part(like renaming table/dropping table in myisam) So before executing NON-Transactional part it will wait signal from other worker thread to either abort or proceed forward. We will add a entry into mi-> start_alter_list, with thread_id(of master) as a key. COMMIT/ROLLBACK Alter is treated as a normal query_log_event so it will be assigned normal worker. This command will take SQLCOM_COMMIT_PREVIOUS path in mysql_execute_command. We will simple search for thread_id into start_alter_list and change status from ::WAITING to ::COMMIT_ALTER and signal the wait condition if we dont find the thread_id we will wait on wait condition, So simple it is just consumer producer between start alter and commit/rollback. Questions by Kristian:- >Can you explain what you mean by "true LOCK=NONE"? It is not clear from your >description. I think you mean something that will allow to run an ALTER >TABLE on the slave in parallel with the same statement on the master? Yes >There will be a number of things to consider with this. Previously, all >transactions in the binlog have been atomic; this introduces related events >that can be arbitrarily separated in the binlog. >For example, suppose the slave stops in the middle between BEGIN_DDL_EVENT >and COMMIT_DDL_EVENT/ROLLBACK_DDL_EVENT. What is then the slave's binlog >position / GTID position? Now we are using 2 gtid for ALTER , So it wont be issue. >Hopefully, the exiting pool of slave worker threads (currently used for >parallel replication) can be used for this as well? Doesn't seem right to >introduce a new kind of thread pool... We are using same(global) thread pool but I am creating new threads for START ALTER. Otherwise we will have a deadlock , Suppose if we have 5 concurrent START ALTER and just 5 slave-worker-threads , all worker will be waiting for COMMIT/ROLLBACK, but we cant execute commit rollback because we dont have any free worker. There can be more case when we have concurrent DML too , so creating a new thread was the safest option. >Won't you need some mechanism to prevent following events to access the >table before ALTER TABLE in a worker thread can acquire the metadata lock? Right, So Untill Locking START ALTER is executed as DDL so no following event will be executed before locking. And one more thing regarding locks , So there will we only one thread will be doing whole work of alter, (we can just assume that there is some sleep on slave) so execution on master and slave will be equivalent. >There are some nasty issues to be aware of with potential deadlocks related >to FLUSH TABLES WITH GLOBAL READ LOCK and such, when multiple threads are >trying to coordinate waiting and locking; there were already some quite hard >issues with this for parallel replication. I have to test for it. Code branch bb-10.5-olter Jira (Mdev-11675(https://jira.mariadb.org/browse/MDEV-11675)) Regards Sachin -- Regards Sachin Setiya Software Engineer at MariaDB

3 4

Re: [Maria-developers] ec1a860bd09: MDEV-21743 Split up SUPER privilege to smaller privileges
by Sergei Golubchik 27 Feb '20

27 Feb '20

Hi, Alexander! Thanks! Looks very straightforward. See few comments below. The main one - I am not sure I like the idea of creating numerous aliases for privileges. This approach looks rather confusing to me. On Feb 26, Alexander Barkov wrote: > revision-id: ec1a860bd09 (mariadb-10.5.0-231-gec1a860bd09) > parent(s): b8b5a6a2f9d > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2020-02-23 22:09:55 +0400 > message: > > MDEV-21743 Split up SUPER privilege to smaller privileges > diff --git a/mysql-test/main/alter_user.test b/mysql-test/main/alter_user.test > index 9ea98615272..60a36499a55 100644 > --- a/mysql-test/main/alter_user.test > +++ b/mysql-test/main/alter_user.test > @@ -30,7 +30,7 @@ alter user foo; > > --echo # Grant super privilege to the user. > connection default; > -grant super on *.* to foo; > +grant READ_ONLY ADMIN on *.* to foo; --echo comments are now wrong > --echo # We now have super privilege. We should be able to run alter user. > connect (b, localhost, foo); > diff --git a/mysql-test/main/grant.result b/mysql-test/main/grant.result > index e83083be4ed..1452ada11f5 100644 > --- a/mysql-test/main/grant.result > +++ b/mysql-test/main/grant.result > @@ -631,6 +634,10 @@ Super Server Admin To use KILL thread, SET GLOBAL, CHANGE MASTER, etc. > Trigger Tables To use triggers > Create tablespace Server Admin To create/alter/drop tablespaces > Update Tables To update existing rows > +Set user Server To create views and stored routines with a different definer > +Federated admin Server To execute the CREATE SERVER, ALTER SERVER, DROP SERVER statements > +Connection admin Server To skip connection related limits tests ^^^ allows KILL too > +Read_only admin Server To perform write operations even if @@read_only=ON > Usage Server Admin No privileges - allow connect only > connect root,localhost,root,,test,$MASTER_MYPORT,$MASTER_MYSOCK; > connection root; > diff --git a/sql/lock.cc b/sql/lock.cc > index 6f86c0a38f6..9a4024606f8 100644 > --- a/sql/lock.cc > +++ b/sql/lock.cc > @@ -114,7 +113,7 @@ lock_tables_check(THD *thd, TABLE **tables, uint count, uint flags) > DBUG_ENTER("lock_tables_check"); > > system_count= 0; > - is_superuser= (thd->security_ctx->master_access & SUPER_ACL) != NO_ACL; > + is_superuser= (thd->security_ctx->master_access & IGNORE_READ_ONLY_ACL) != NO_ACL; may be then s/is_superuser/ignores_read_only/ ? > log_table_write_query= (is_log_table_write_query(thd->lex->sql_command) > || ((flags & MYSQL_LOCK_LOG_TABLE) != 0)); > > diff --git a/sql/privilege.h b/sql/privilege.h > index 5dbc0b6dbdf..f80e726d8d0 100644 > --- a/sql/privilege.h > +++ b/sql/privilege.h > @@ -59,24 +59,60 @@ enum privilege_t: unsigned long long > TRIGGER_ACL = (1UL << 27), > CREATE_TABLESPACE_ACL = (1UL << 28), > DELETE_HISTORY_ACL = (1UL << 29), // Added in 10.3.4 > + SET_USER_ACL = (1UL << 30), // Added in 10.5.2 > + FEDERATED_ADMIN_ACL = (1UL << 31), // Added in 10.5.2 > + CONNECTION_ADMIN_ACL = (1ULL << 32), // Added in 10.5.2 > + READ_ONLY_ADMIN_ACL = (1ULL << 33), // Added in 10.5.2 > + REPL_SLAVE_ADMIN_ACL = (1ULL << 34), // Added in 10.5.2 > + REPL_MASTER_ADMIN_ACL = (1ULL << 35), // Added in 10.5.2 > + BINLOG_ADMIN_ACL = (1ULL << 36) // Added in 10.5.2 > /* > - don't forget to update > - 1. static struct show_privileges_st sys_privileges[] > - 2. static const char *command_array[] and static uint command_lengths[] > - 3. mysql_system_tables.sql and mysql_system_tables_fix.sql > - 4. acl_init() or whatever - to define behaviour for old privilege tables > - 5. sql_yacc.yy - for GRANT/REVOKE to work > - 6. Add a new ALL_KNOWN_ACL_VERSION > - 7. Change ALL_KNOWN_ACL to ALL_KNOWN_ACL_VERSION > - 8. Update User_table_json::get_access() > + don't forget to update: > + In this file: > + - Add a new LAST_version_ACL > + - Fix PRIVILEGE_T_MAX_BIT > + - Add a new ALL_KNOWN_ACL_version > + - Change ALL_KNOWN_ACL to ALL_KNOWN_ACL_version > + - Change GLOBAL_ACLS if needed > + - Change SUPER_ADDED_SINCE_USER_TABLE_ACL if needed > + > + In other files: > + - static struct show_privileges_st sys_privileges[] > + - static const char *command_array[] and static uint command_lengths[] > + - mysql_system_tables.sql and mysql_system_tables_fix.sql > + - acl_init() or whatever - to define behaviour for old privilege tables > + - Update User_table_json::get_access() > + - sql_yacc.yy - for GRANT/REVOKE to work > + > + Important: the enum should contain only single-bit values. > + In this case, debuggers print bit combinations in the readable form: > + (gdb) p (privilege_t) (15) > + $8 = (SELECT_ACL | INSERT_ACL | UPDATE_ACL | DELETE_ACL) > + > + Bit-OR combinations of the above values should be declared outside! > */ > - > - // A combination of all bits defined in 10.3.4 (and earlier) > - ALL_KNOWN_ACL_100304 = (1UL << 30) - 1 > }; > > > -constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100304; > +// Version markers > +constexpr privilege_t LAST_100304_ACL= DELETE_HISTORY_ACL; > +constexpr privilege_t LAST_100502_ACL= BINLOG_ADMIN_ACL; > +constexpr privilege_t LAST_CURRENT_ACL= LAST_100502_ACL; > +constexpr uint PRIVILEGE_T_MAX_BIT= 36; > + > +static_assert((privilege_t)(1ULL << PRIVILEGE_T_MAX_BIT) == LAST_CURRENT_ACL, > + "LAST_CURRENT_ACL and PRIVILEGE_T_MAX_BIT do not match"); why wouldn't you define PRIVILEGE_T_MAX_BIT via LAST_CURRENT_ACL instead? > + > +// A combination of all bits defined in 10.3.4 (and earlier) > +constexpr privilege_t ALL_KNOWN_ACL_100304 = > + (privilege_t) ((LAST_100304_ACL << 1) - 1); > + > +// A combination of all bits defined in 10.5.2 > +constexpr privilege_t ALL_KNOWN_ACL_100502= > + (privilege_t) ((LAST_100502_ACL << 1) - 1); > + > +// A combination of all bits defined as of the current version > +constexpr privilege_t ALL_KNOWN_ACL= ALL_KNOWN_ACL_100502; > > > // Unary operators > @@ -229,6 +280,104 @@ constexpr privilege_t SHOW_CREATE_TABLE_ACLS= > constexpr privilege_t TMP_TABLE_ACLS= > COL_DML_ACLS | ALL_TABLE_DDL_ACLS; > > + > + > +/* > + If a VIEW has a `definer=invoker@host` clause and > + the specified definer does not exists, then > + - The invoker with REVEAL_MISSING_DEFINER_ACL gets: > + ERROR: The user specified as a definer ('definer1'@'localhost') doesn't exist > + - The invoker without MISSING_DEFINER_ACL gets a generic access error, > + without revealing details that the definer does not exists. > + > + TODO: we should eventually test the same privilege when processing > + other objects that have the DEFINER clause (e.g. routines, triggers). > + Currently the missing definer is revealed for non-privileged invokers > + in case of routines, triggers, etc. > +*/ > +constexpr privilege_t REVEAL_MISSING_DEFINER_ACL= SUPER_ACL; 1. why did you create these aliases? I don't think they make the code simpler, on the contrary, now one can never know whether say, IGNORE_READ_ONLY_ACL is a real privilege like in GRANT IGNORE READ_ONLY ON *.* TO user@host or it's just an alias. 2. REVEAL_MISSING_DEFINER_ACL should be SET_USER_ACL, I think > +constexpr privilege_t DES_DECRYPT_ONE_ARG_ACL= SUPER_ACL; > +constexpr privilege_t LOG_BIN_TRUSTED_SP_CREATOR_ACL= SUPER_ACL; this could be SET_USER_ACL too > +constexpr privilege_t DEBUG_ACL= SUPER_ACL; > +constexpr privilege_t SET_GLOBAL_SYSTEM_VARIABLE_ACL= SUPER_ACL; > +constexpr privilege_t SET_RESTRICTED_SESSION_SYSTEM_VARIABLE_ACL= SUPER_ACL; > + > +/* Privileges related to --read-only */ > +constexpr privilege_t IGNORE_READ_ONLY_ACL= READ_ONLY_ADMIN_ACL; > + > +/* Privileges related to connection handling */ > +constexpr privilege_t IGNORE_INIT_CONNECT_ACL= CONNECTION_ADMIN_ACL; > +constexpr privilege_t IGNORE_MAX_USER_CONNECTIONS_ACL= CONNECTION_ADMIN_ACL; > +constexpr privilege_t IGNORE_MAX_CONNECTIONS_ACL= CONNECTION_ADMIN_ACL; > +constexpr privilege_t IGNORE_MAX_PASSWORD_ERRORS_ACL= CONNECTION_ADMIN_ACL; > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t KILL_OTHER_USER_PROCESS_ACL= CONNECTION_ADMIN_ACL; > + > + > +/* > + Binary log related privileges that are checked regardless > + of active replication running. > +*/ > + > +/* > + This command was renamed from "SHOW MASTER STATUS" > + to "SHOW BINLOG STATUS" in 10.5.2. > + Was SUPER_ACL | REPL_CLIENT_ACL prior to 10.5.2 > +*/ > +constexpr privilege_t STMT_SHOW_BINLOG_STATUS_ACL= REPL_CLIENT_ACL; > + > +// Was SUPER_ACL | REPL_CLIENT_ACL prior to 10.5.2 > +constexpr privilege_t STMT_SHOW_BINARY_LOGS_ACL= REPL_CLIENT_ACL; > + > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_PURGE_BINLOG_ACL= BINLOG_ADMIN_ACL; > + > +// Was REPL_SLAVE_ACL prior to 10.5.2 > +constexpr privilege_t STMT_SHOW_BINLOG_EVENTS_ACL= PROCESS_ACL; > + > + > +/* > + Privileges for replication related statements > + that are executed on the master. > +*/ > +constexpr privilege_t COM_REGISTER_SLAVE_ACL= REPL_SLAVE_ACL; > +constexpr privilege_t COM_BINLOG_DUMP_ACL= REPL_SLAVE_ACL; > +// Was REPL_SLAVE_ACL prior to 10.5.2 > +constexpr privilege_t STMT_SHOW_SLAVE_HOSTS_ACL= REPL_MASTER_ADMIN_ACL; > + > + > +/* Privileges for statements that are executed on the slave */ > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_START_SLAVE_ACL= REPL_SLAVE_ADMIN_ACL; > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_STOP_SLAVE_ACL= REPL_SLAVE_ADMIN_ACL; > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_CHANGE_MASTER_ACL= REPL_SLAVE_ADMIN_ACL; > +// Was (SUPER_ACL | REPL_CLIENT_ACL) prior to 10.5.2 > +constexpr privilege_t STMT_SHOW_SLAVE_STATUS_ACL= REPL_SLAVE_ADMIN_ACL; > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_BINLOG_ACL= REPL_SLAVE_ADMIN_ACL; > +// Was REPL_SLAVE_ACL prior to 10.5.2 > +constexpr privilege_t STMT_SHOW_RELAYLOG_EVENTS_ACL= REPL_SLAVE_ADMIN_ACL; > + > + > +/* Privileges for federated database related statements */ > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_CREATE_SERVER_ACL= FEDERATED_ADMIN_ACL; > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_ALTER_SERVER_ACL= FEDERATED_ADMIN_ACL; > +// Was SUPER_ACL prior to 10.5.2 > +constexpr privilege_t STMT_DROP_SERVER_ACL= FEDERATED_ADMIN_ACL; > + > + > +/* Privileges related to processes */ > +constexpr privilege_t COM_PROCESS_INFO_ACL= PROCESS_ACL; > +constexpr privilege_t STMT_SHOW_EXPLAIN_ACL= PROCESS_ACL; > +constexpr privilege_t STMT_SHOW_ENGINE_STATUS_ACL= PROCESS_ACL; > +constexpr privilege_t STMT_SHOW_ENGINE_MUTEX_ACL= PROCESS_ACL; > +constexpr privilege_t STMT_SHOW_PROCESSLIST_ACL= PROCESS_ACL; > + > + > /* > Defines to change the above bits to how things are stored in tables > This is needed as the 'host' and 'db' table is missing a few privileges > diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc > index e2a3c482ae4..b096bfa7a95 100644 > --- a/sql/sql_connect.cc > +++ b/sql/sql_connect.cc > @@ -1246,7 +1245,8 @@ void prepare_new_connection_state(THD* thd) > thd->set_command(COM_SLEEP); > thd->init_for_queries(); > > - if (opt_init_connect.length && !(sctx->master_access & SUPER_ACL)) > + if (opt_init_connect.length && > + (sctx->master_access & IGNORE_INIT_CONNECT_ACL) == NO_ACL) dunno, I kind of like !(access & SOME_ACL) why not to keep privilege_t -> bool? > { > execute_init_command(thd, &opt_init_connect, &LOCK_sys_init_connect); > if (unlikely(thd->is_error())) > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index dac5b025821..7f3a436a4c2 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -7138,8 +7138,7 @@ bool check_some_access(THD *thd, privilege_t want_access, TABLE_LIST *table) > @warning > One gets access right if one has ANY of the rights in want_access. > This is useful as one in most cases only need one global right, > - but in some case we want to check if the user has SUPER or > - REPL_CLIENT_ACL rights. > + but in some case we want to check multiple rights. In what cases? Are there any left? > > @retval > 0 ok Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4c263b6d30b: MDEV-20632: Recursive CTE cycle detection using CYCLE clause
by Sergei Golubchik 26 Feb '20

26 Feb '20

Hi, Sanja! _some_ comments are below. The main thing, I still don't understand your changes in sql_select.cc. Why did you create separate counters and code paths with if() for CYCLE? I'd think it could be just a generalization of DISTINCT On Feb 20, Oleksandr Byelkin wrote: > revision-id: 4c263b6d30b (mariadb-10.5.0-92-g4c263b6d30b) > parent(s): 6f2e2285291 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2020-01-27 21:56:02 +0100 > message: > > MDEV-20632: Recursive CTE cycle detection using CYCLE clause > > Added CYCLE clause to recursive CTE. > > diff --git a/sql/item.h b/sql/item.h > index c1f68a4f942..2cbaf880e00 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -622,6 +622,13 @@ class st_select_lex_unit; > class Item_func_not; > class Item_splocal; > > +/* Item::common_flags */ > +/* Indicates that name of this Item autogenerated or set by user */ > +#define IS_AUTO_GENETATED_NAME 1 typo ^^^ > +/* Inticates that this item is in CYCLE clause of WITH */ typo ^^^ > +#define IS_IN_WITH_CYCLE 2 > + > + > /** > String_copier that sends Item specific warnings. > */ > diff --git a/sql/sql_cte.cc b/sql/sql_cte.cc > index 5b3d3108da5..971844120bc 100644 > --- a/sql/sql_cte.cc > +++ b/sql/sql_cte.cc > @@ -982,6 +982,38 @@ With_element::rename_columns_of_derived_unit(THD *thd, are you sure what you're doing below can still be called "rename_columns_of_derived_unit" ? > else > make_valid_column_names(thd, select->item_list); > > + if (cycle_list) > + { > + List_iterator_fast<Item> it(select->item_list); > + List_iterator_fast<LEX_CSTRING> nm(*cycle_list); > + List_iterator_fast<LEX_CSTRING> nm_check(*cycle_list); > + DBUG_ASSERT(cycle_list->elements != 0); > + while (LEX_CSTRING *name= nm++) > + { > + Item *item; > + // unique check > + LEX_CSTRING *check; > + nm_check.rewind(); > + while ((check= nm_check++) && check != name) > + { > + if (check->length == name->length && > + strncmp(check->str, name->str, name->length) == 0) > + { > + my_error(ER_DUP_FIELDNAME, MYF(0), check->str); > + return true; > + } > + } > + while ((item= it++) && > + (item->name.length != name->length || > + strncmp(item->name.str, name->str, name->length) != 0)); > + if (item == NULL) > + { > + my_error(ER_BAD_FIELD_ERROR, MYF(0), name->str, "CYCLE clause"); > + return true; > + } this pattern is used so often that we might want to have a helper, like, List_iterator_fast<>::find() not in this commit, though. > + item->common_flags|= IS_IN_WITH_CYCLE; > + } > + } > unit->columns_are_renamed= true; > > return false; > @@ -1425,6 +1457,21 @@ void With_clause::print(String *str, enum_query_type query_type) > } > > > +static void list_strlex_print(String *str, List<LEX_CSTRING> *list) > +{ > + List_iterator_fast<LEX_CSTRING> li(*list); > + bool first= TRUE; > + while(LEX_CSTRING *col_name= li++) > + { > + if (first) > + first= FALSE; > + else > + str->append(','); > + str->append(col_name); should be "append_identifier", shouldn't it? add a test where a column name is a reserved word. > + } > +} > + > + > /** > @brief > Print this with element > @@ -1444,22 +1491,20 @@ void With_element::print(String *str, enum_query_type query_type) > { > List_iterator_fast<LEX_CSTRING> li(column_list); > str->append('('); > - for (LEX_CSTRING *col_name= li++; ; ) > - { > - str->append(col_name); > - col_name= li++; > - if (!col_name) > - { > - str->append(')'); > - break; > - } > - str->append(','); > - } > + list_strlex_print(str, &column_list); any other code that could make use of your list_strlex_print ? > + str->append(')'); > } > - str->append(STRING_WITH_LEN(" as ")); > - str->append('('); > + str->append(STRING_WITH_LEN(" as (")); > spec->print(str, query_type); > str->append(')'); > + > + if (cycle_list) > + { > + DBUG_ASSERT(cycle_list->elements != 0); > + str->append(STRING_WITH_LEN(" CYCLE (")); > + list_strlex_print(str, cycle_list); > + str->append(") "); > + } > } > > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index c115d9352aa..0d60ab8579f 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -14704,9 +14704,30 @@ with_list_element: > if (elem->set_unparsed_spec(thd, spec_start, $6.pos(), > spec_start - query_start)) > MYSQL_YYABORT; > + if ($7) > + { > + elem->set_cycle_list($7); > + } > } > ; > > +opt_cycle: > + /* empty */ > + { $$= NULL; } > + | > + CYCLE_SYM > + { > + if (!Lex->curr_with_clause->with_recursive) > + { > + thd->parse_error(ER_SYNTAX_ERROR, $1.pos()); > + } > + } > + '(' with_column_list ')' Where did you see that the standard requires parentheses here? > + { > + $$= $4; > + } > + ; > + > > opt_with_column_list: > /* empty */ Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0