August 2021 - developers - lists.mariadb.org

Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
by Sergei Golubchik 24 Aug '22

24 Aug '22

Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-03-01 17:43:34 +0300 > message: > > MDEV-16546 System versioning setting to allow history modification > > 1. force_fields_visible session variable makes system-invisible and > user-invisible fields visible on next table open. FLUSH TABLES > required for already opened tables. * system_versioning_insert_history * allows pseudocolumns ROW_START and ROW_END be specified in INSERT * doesn't affect invisible columns * FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better > 2. If secure_timestamp allows to modify timestamp variable then > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > specify row_start and row_end system field values. when system_versioning_insert_history=1 > 3. Cleaned up select_insert::send_data() from setting vers_write as > this parameter is now set on TABLE initialization. I don't see where it's set. This commit removes two table->vers_write= table->versioned(); and adds one from_field->table->vers_write= false; in Item_field::fix_fields. But where is vers_write set to true? Was it done in some other commit? > diff --git a/mysql-test/suite/versioning/r/insert.result b/mysql-test/suite/versioning/r/insert.result > index 2645d0184e8..00d1bb705c4 100644 > --- a/mysql-test/suite/versioning/r/insert.result > +++ b/mysql-test/suite/versioning/r/insert.result > @@ -112,3 +112,95 @@ x y > 9 9001 > drop table t1; > drop table t2; > +# > +# MDEV-16546 System versioning setting to allow history modification > +# > +# restart: --secure-timestamp=NO > +set @@session.time_zone='+00:00'; > +create table t1(x int) with system versioning; > +insert into t1(x) values (1); > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); > +ERROR 42S22: Unknown column 'row_start' in 'field list' > +set session force_fields_visible= 1; > +flush tables; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL, > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, shouldn't be visible here, only in insert and load. > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); please, add tests for insert into t1 values (4) this should work, row_start/row_end must be mentioned explicitly. Also insert into t1 set x=5, row_start=..., row_end=... this should work too. And when only one row_start or row_end is mentioned as well. But update t1 set row_start=... insert into t1 (x, row_start, row_end) values (...) on duplicate key update row_start=... this should fail. While replace load data ... replace should work, but as DELETE+INSERT (I mean that DELETE in versioned tables doesn't actually delete history). If that's too difficult, then an error too. > +update t1 set x= x + 1; > +select x, check_row_ts(row_start, row_end) from t1 for system_time all order by x; ... > diff --git a/mysql-test/suite/versioning/t/insert.opt b/mysql-test/suite/versioning/t/insert.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes why is this? > -- source suite/versioning/common_finish.inc > diff --git a/mysql-test/suite/versioning/t/insert2.opt b/mysql-test/suite/versioning/t/insert2.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert2.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes and this? > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d23b190b2c5..93f8a8434e4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid) > #endif > } > > +bool THD::vers_modify_sys_field() const don't call it vers_modify_sys_field, your previous name vers_modify_history was better. There may be other sys fields that will never be directly writable (like ROWID, for example). > +{ > + if (lex->sql_command != SQLCOM_INSERT && > + lex->sql_command != SQLCOM_INSERT_SELECT && > + lex->sql_command != SQLCOM_LOAD) > + return false; I don't think this is enough to reject insert ... on duplicate key update row_start=xxx > + if (opt_secure_timestamp >= SECTIME_REPL || this is fixed in a later commit, so ok > + (opt_secure_timestamp == SECTIME_SUPER && > + !(security_ctx->master_access & SUPER_ACL))) > + return false; > + return true; > +} > + > + > void > wait_for_commit::reinit() > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2913a4a8350..520ea1d63bf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -820,6 +820,7 @@ typedef struct system_variables > > vers_asof_timestamp_t vers_asof_timestamp; > ulong vers_alter_history; > + my_bool force_fields_visible; system_versioning_insert_history > } SV; > > /** > @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */ > Item *sp_prepare_func_item(Item **it_addr, uint cols= 1); > bool sp_eval_expr(Field *result_field, Item **expr_item_ptr); > > + bool vers_modify_sys_field() const; vers_modify_history > }; > > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index f0b84ab126f..84c2ae4ab36 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) does it even make sense? I mean direct insertion of transaction ids. > { > bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > table->file->column_bitmaps_signal(); > @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > info->deleted++; > if (!table->file->has_transactions()) > thd->transaction->stmt.modified_non_trans_table= TRUE; > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes. > { > store_record(table, record[2]); > error= vers_insert_history_row(table); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index f817f9086a9..4429308a097 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size( > SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024), > BLOCK_SIZE(1)); > + > +static Sys_var_mybool Sys_force_fields_visible( > + "force_fields_visible", "Make invisible fields visible on next table open", "system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns" > + SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE)); > diff --git a/sql/table.cc b/sql/table.cc > index f4bdbdeac5a..520f3738c9e 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share, > { > if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam))) > goto err; > + if (thd->variables.force_fields_visible && > + (*field_ptr)->invisible <= INVISIBLE_SYSTEM) > + (*field_ptr)->invisible= VISIBLE; Not really, system_versioning_insert_history should not affect visibility of anything besides ROW_START and ROW_END pseudocolumns in the INSERT and LOAD DATA statements. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] 6768e3a2830: MDEV-22166 MIGRATE PARTITION: move out partition into a table
by Sergei Golubchik 10 Sep '21

10 Sep '21

Hi, Aleksey! It's great that you made it atomic. Frankly, I didn't think of it, MDEV defines the feature as being exactly equivalent to the 3-command sequence, so non-atomic. But, of course, making it atomic makes perfect sense. See other comments below. On Aug 23, Aleksey Midenkov wrote: > commit 6768e3a2830 > Author: Aleksey Midenkov <midenok(a)gmail.com> > Date: Thu Jul 29 22:54:12 2021 +0300 > > diff --git a/sql/lex.h b/sql/lex.h > index cbf9d9d51b2..55eefc39a7e 100644 > --- a/sql/lex.h > +++ b/sql/lex.h > @@ -240,6 +240,7 @@ SYMBOL symbols[] = { > { "EXPLAIN", SYM(DESCRIBE)}, > { "EXTENDED", SYM(EXTENDED_SYM)}, > { "EXTENT_SIZE", SYM(EXTENT_SIZE_SYM)}, > + { "EXTRACT", SYM(EXTRACT_SYM)}, So, you did EXTRACT after all, not MIGRATE? ADD/EXTRACT aren't opposites in SQL. It would be rather consistent to have ALTER ... ADD PARTITION (...) FROM table_name and ALTER ... DROP PARTITION ... TO table_name except that DROP...TO looks kind of strange, and ADD...FROM does not convey the meaning that the table disappears. The second issue also applies to IMPORT/EXPORT. Another option, also kind of strange, but also consistent and makes it clear that the table/partition disappears: ALTER ... MOVE TABLE table_name TO PARTITION (...) ALTER ... MOVE PARTITION ... TO TABLE table_name or the same with CHANGE instead of MOVE to reuse an existing keyword. I'm not saying anything of the above is perfect, and better variants are very much welcome. But pretty much anything is better than ADD/EXTRACT. > { "FALSE", SYM(FALSE_SYM)}, > { "FAST", SYM(FAST_SYM)}, > { "FAULTS", SYM(FAULTS_SYM)}, > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index dc2eb4425f0..a41ea7d3e4a 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -7605,6 +7605,25 @@ alter_commands: > if (Lex->stmt_alter_table_exchange_partition($6)) > MYSQL_YYABORT; > } > + | EXTRACT_SYM PARTITION_SYM alt_part_name_item > + AS TABLE_SYM table_ident have_partitioning > + { > + LEX *lex= Lex; > + lex->first_select_lex()->db= $6->db; > + if (lex->first_select_lex()->db.str == NULL && > + lex->copy_db_to(&lex->first_select_lex()->db)) > + MYSQL_YYABORT; > + if (unlikely(check_table_name($6->table.str,$6->table.length, > + FALSE)) || > + ($6->db.str && unlikely(check_db_name((LEX_STRING*) &$6->db)))) > + my_yyabort_error((ER_WRONG_TABLE_NAME, MYF(0), $6->table.str)); good point, LEX::stmt_alter_table_exchange_partition() doesn't seem to do it > + lex->name= $6->table; > + > + lex->m_sql_cmd= new (thd->mem_root) Sql_cmd_alter_table(); > + if (unlikely(lex->m_sql_cmd == NULL)) > + MYSQL_YYABORT; > + lex->alter_info.partition_flags|= ALTER_PARTITION_EXTRACT; why wouldn't you combine the common code from here and EXCHANGE PARTITION? less code duplication and as a bonus EXCHANGE will get proper table/db name checks. > + } > ; > > remove_partitioning: > diff --git a/sql/sql_alter.h b/sql/sql_alter.h > index a0f89c28d2a..6aa65d232ca 100644 > --- a/sql/sql_alter.h > +++ b/sql/sql_alter.h > @@ -345,6 +348,8 @@ class Alter_table_ctx > /** Indicates that we are altering temporary table */ > bool tmp_table; > > + DDL_LOG_STATE *ddl_log_state; What do you need this for? > + > private: > char new_filename[FN_REFLEN + 1]; > char new_alias_buff[NAME_LEN + 1]; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 779d125af70..033d77a827d 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10016,7 +10146,7 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db, > } > > // In-place execution of ALTER TABLE for partitioning. > - DBUG_RETURN(fast_alter_partition_table(thd, table, alter_info, > + DBUG_RETURN(fast_alter_partition_table(thd, table, alter_info, &alter_ctx, > create_info, table_list, > &alter_ctx.db, > &alter_ctx.table_name)); you no longer need to pass alter_ctx.db and alter_ctx.table_name, if you pass the whole alter_ctx down. > diff --git a/include/my_dbug.h b/include/my_dbug.h > index e25bfcf28a7..cf969ce5750 100644 > --- a/include/my_dbug.h > +++ b/include/my_dbug.h > @@ -104,6 +104,8 @@ extern int (*dbug_sanity)(void); > (_db_keyword_(0,(keyword), 0) ? (a1) : (a2)) > #define DBUG_EVALUATE_IF(keyword,a1,a2) \ > (_db_keyword_(0,(keyword), 1) ? (a1) : (a2)) > +#define DBUG_TRUE_IF(keyword) DBUG_EVALUATE_IF(keyword, 1, 0) > +#define DBUG_FALSE_IF(keyword) DBUG_EVALUATE_IF(keyword, 0, 1) hmm. DBUG_FALSE_IF is not used anywhere, DBUG_TRUE_IF is used once as if (DBUG_TRUE_IF("error_extract_partition_00") || unlikely(error= file->ha_rename_table(from_name, to_name))) and this is typically written as if (DBUG_EVALUATE_IF("error_extract_partition_00", 1, error= file->ha_rename_table(from_name, to_name))) so I see no need for either DBUG_TRUE_IF or DBUG_FALSE_IF, please, remove both > #define DBUG_PRINT(keyword,arglist) \ > do if (_db_pargs_(__LINE__,keyword)) _db_doprnt_ arglist; while(0) > > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 85d880cbbb4..4172a61812e 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -5432,7 +5432,7 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info, > } while (++part_count < tab_part_info->num_parts); > if (num_parts_found != num_parts_dropped) > { > - my_error(ER_DROP_PARTITION_NON_EXISTENT, MYF(0), "DROP"); > + my_error(ER_DROP_PARTITION_NON_EXISTENT, MYF(0), cmd); Not part of your patch, I know, but the error message here is "Error in list of partitions to %-.64s" shouldn't it say "Partition does not exist" or something? > goto err; > } > if (table->file->is_fk_defined_on_table_or_index(MAX_KEY)) > @@ -7268,6 +7420,48 @@ uint fast_alter_partition_table(THD *thd, TABLE *table, > if (alter_partition_lock_handling(lpt)) > goto err; > } > + else if (alter_info->partition_flags & ALTER_PARTITION_EXTRACT) > + { > + if (mysql_write_frm(lpt, WFRM_WRITE_EXTRACTED) || > + write_log_drop_shadow_frm(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_1") || > + ERROR_INJECT_ERROR("fail_extract_partition_1") || > + mysql_write_frm(lpt, WFRM_WRITE_SHADOW) || > + ERROR_INJECT_CRASH("crash_extract_partition_2") || > + ERROR_INJECT_ERROR("fail_extract_partition_2") || > + wait_while_table_is_used(thd, table, HA_EXTRA_NOT_USED) || > + ERROR_INJECT_CRASH("crash_extract_partition_3") || > + ERROR_INJECT_ERROR("fail_extract_partition_3") || > + write_log_extract_partition(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_4") || > + ERROR_INJECT_ERROR("fail_extract_partition_4") || > + alter_close_table(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_5") || > + ERROR_INJECT_ERROR("fail_extract_partition_5") || > + alter_partition_extract(lpt) || > + ERROR_INJECT_CRASH("crash_extract_partition_7") || > + ERROR_INJECT_ERROR("fail_extract_partition_7") || > + (frm_install= TRUE, FALSE) || > + mysql_write_frm(lpt, WFRM_INSTALL_SHADOW|WFRM_BACKUP_ORIGINAL) || > + log_partition_alter_to_ddl_log(lpt) || > + (frm_install= FALSE, FALSE) || > + ERROR_INJECT_CRASH("crash_extract_partition_8") || > + ERROR_INJECT_ERROR("fail_extract_partition_8") || > + (ddl_log_complete(lpt->part_info), false) || as far as I understand this ^^^ should be done *after* binlogging. I suspect that if you crash here, blnlog won't get this ALTER TABLE. > + ((!thd->lex->no_write_to_binlog) && > + (write_bin_log(thd, FALSE, > + thd->query(), thd->query_length()), FALSE)) || if you crash here, what will roll forward on recovery and drop the backup? The log is already closed here, isn't it? I don't understand why you need a backup. Why not to install shadow after the binlog was written? > + mysql_write_frm(lpt, WFRM_DROP_BACKUP) || > + ERROR_INJECT_CRASH("crash_extract_partition_9") || > + ERROR_INJECT_ERROR("fail_extract_partition_9")) > + { > + (void) ddl_log_revert(thd, lpt->part_info); > + handle_alter_part_error(lpt, true, true, frm_install); > + goto err; > + } > + if (alter_partition_lock_handling(lpt)) > + goto err; > + } > else if ((alter_info->partition_flags & ALTER_PARTITION_ADD) && > (part_info->part_type == RANGE_PARTITION || > part_info->part_type == LIST_PARTITION)) > diff --git a/mysql-test/include/lcase_names.inc b/mysql-test/include/lcase_names.inc > new file mode 100644 > index 00000000000..e69de29bb2d better put here some comment, like in other dummy *.inc files see e.g. platform.inc or protocol.inc > diff --git a/mysql-test/suite/atomic/alter_partition,aria.rdiff b/mysql-test/suite/atomic/alter_partition,aria.rdiff > new file mode 100644 > index 00000000000..20c0510c5ed > --- /dev/null > +++ b/mysql-test/suite/atomic/alter_partition,aria.rdiff > @@ -0,0 +1,219 @@ > +--- alter_partition.result > ++++ alter_partition,aria.reject > +@@ -1,4 +1,4 @@ > +-set @@default_storage_engine=MyISAM; > ++set @@default_storage_engine=Aria; > + # Crash recovery > + create or replace procedure prepare_table() > + begin > +@@ -13,12 +13,12 @@ > + end $ > + # QUERY: ALTER TABLE t1 EXTRACT PARTITION p1 AS TABLE tp1 > + # CRASH POINT: ddl_log_create_before_create_frm > +-t1#P#p0.MYD > +-t1#P#p0.MYI > +-t1#P#p1.MYD may be better `replace_result MAI MYI MAD MYD` ? if that's the only difference. To avoid big rdiff files. > +-t1#P#p1.MYI > +-t1#P#pn.MYD > +-t1#P#pn.MYI > ++t1#P#p0.MAD > ++t1#P#p0.MAI Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 20

Re: [Maria-developers] f14cfd22115: Vanilla refactoring: binlog query in DDL_LOG_RENAME_ACTION
by Aleksey Midenkov 24 Aug '21

24 Aug '21

Hi Sergei! Really, this is not pure refactoring. I've been planning to remove it if it will not be useful for MDEV-22165 (that task should be based on this branch). Meanwhile, can you please review other commits? On Mon, Aug 23, 2021 at 7:35 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On Aug 23, Aleksey Midenkov wrote: > > revision-id: f14cfd22115 (mariadb-10.6.1-63-gf14cfd22115) > > parent(s): 03225474d60 > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > timestamp: 2021-08-17 20:01:48 +0300 > > message: > > > > Vanilla refactoring: binlog query in DDL_LOG_RENAME_ACTION > > > > This turned out to be not needed for MDEV-22166. But it still might be > > useful for MDEV-22165 or later. > > There are three changes here. > Adding 'locked' to ddl_log_store_query, creating > st_ddl_recovery::binlog_query(), adding 'case RENAME_PHASE_BINLOG' > > st_ddl_recovery::binlog_query() is ok, if you want to keep it. > But the other two aren't a refactoring, it's creating new functionality > which isn't used in this patch, dead code basically. And your commit > comment seems to imply that this new functionality isn't needed > in the following feature patch either. > > While your other refactoring patches are indeed pure refactoring, this > one isn't, so, please, remove it from the patchset if it isn't used > there. You can keep st_ddl_recovery::binlog_query() if you'd like - it > is indeed pure refactoring. > > Regards, > Sergei > VP of MariaDB Server Engineering > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok

2 1

Re: [Maria-developers] bc55e3c438f: MDEV-4742 - provide function to sort string in "natural" order
by Sergei Golubchik 20 Aug '21

20 Aug '21

Hi, Vladislav! On Aug 20, Vladislav Vaintroub wrote: > commit bc55e3c438f > Author: Vladislav Vaintroub <wlad(a)mariadb.com> > Date: Thu Jul 29 13:28:11 2021 +0200 > > MDEV-4742 - provide function to sort string in "natural" order > > The numbers should be compared as numbers, while the rest should be compared > as string. > > Introduce natural_sort_key() function that transforms original string > so that the lexicographic order of such keys is suitable for > natural sort. This comment doesn't mention the second optional argument. Neither does Jira. > diff --git a/mysql-test/main/natural_sort_key.test b/mysql-test/main/natural_sort_key.test > new file mode 100644 > index 00000000000..bee0d8e76bd > --- /dev/null > +++ b/mysql-test/main/natural_sort_key.test > @@ -0,0 +1,64 @@ > +--source include/have_innodb.inc > +--source include/have_sequence.inc > + > +SET NAMES utf8mb4; > +SELECT NATURAL_SORT_KEY(NULL); > + > +# Sort without tables > +SELECT '' c WHERE 0 UNION VALUES('a10'),('a9'),('a1000'), ('a0'),('b'),('b0') ORDER BY NATURAL_SORT_KEY(c); > + > +#Test that max packet overflow produces NULL plus warning > +SELECT NATURAL_SORT_KEY(repeat('a1',@@max_allowed_packet/2-1)); > + > +#Test with virtual(index only) key > +CREATE TABLE t1( > + c VARCHAR(30) CHARACTER SET latin1 COLLATE latin1_bin, > + k VARCHAR(45) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci AS (NATURAL_SORT_KEY(CONVERT(c USING utf8mb4))) INVISIBLE, > + KEY(k,c)) ENGINE=InnoDB; > +INSERT INTO t1 values > + ('A1'),('a1'),('A100'),('a100'),('A2'),('ä2'),('a2'),('A99'), > + ('äb'),('B1'),('B100'),('B9'),('C'),('100'); > +EXPLAIN SELECT c FROM t1 ORDER BY k,c; > +-- echo #Natural sort order. > +# We sort by 2 colums, for stable sort,as we do not currenly have a case and accent insensitive Unicode collation. > +SELECT c FROM t1 ORDER BY k,c; > +-- echo #Unnatural but unicode aware) sort order > +SELECT c FROM t1 ORDER BY CONVERT(c USING utf8mb4) COLLATE utf8mb4_unicode_ci,c; > +# CREATE TABLE AS SELECT, to see that length of the column is correct. > +CREATE TABLE t2 AS SELECT c, NATURAL_SORT_KEY(c) FROM t1 WHERE 0; you could've simply used --enable_metadata > +SHOW CREATE TABLE t2; > +DROP TABLE t1,t2; > + > +#Show encoding of numbers, including fractions, and leading whitespace. > +SELECT RPAD(val,21,' ') value , RPAD(NATURAL_SORT_KEY(val),26,' ') sortkey , LENGTH(NATURAL_SORT_KEY(val)) - LENGTH(val) encoding_overhead > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.0'),('0.1'), ('0,15'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1'),('01'),('0001') > +UNION SELECT CONCAT('1',repeat('0',seq)) FROM seq_1_to_20 > +) AS numbers ORDER BY sortkey; > + > +--echo # Disable fractions handling by passing NULL as second parameter to NATURAL_SORT_KEY > +SELECT val value > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.1'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1'), ('0,1'),('1,001'),('1,002'),('1,010'),('1,02'),('1,1'),('1,3'),('1')) > +AS numbers ORDER BY NATURAL_SORT_KEY(val, NULL); > + > +--echo # Use ',' as decimal separator for NATURAL_SORT_KEY > +SELECT val value, NATURAL_SORT_KEY(val,',') sortkey > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0,1'),('1,001'),('1,002'),('1,010'),('1,02'),('1,1'),('1,3'),('1')) > +AS numbers ORDER BY sortkey; > + > +--echo # Use '.' as decimal separator for NATURAL_SORT_KEY > +SELECT val value,NATURAL_SORT_KEY(val,'.') sortkey > +FROM > +( > +SELECT 0 val > +UNION VALUES ('0.1'),('1.001'),('1.002'),('1.010'),('1.02'),('1.1'),('1.3'),('1')) > +AS numbers ORDER BY sortkey; > +SET NAMES DEFAULT; > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 7e8ff667e75..a2efd414750 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -5293,6 +5293,232 @@ String *Item_temptable_rowid::val_str(String *str) > return &str_value; > } > > +/** > + Here is how we generate a key suitable for lexicographic comparison > + from a numeric string (representing positive number). > + > + We prepend variable length prefix, that only encodes string length > + The longer the string, the lexicographically larger is the prefix. > + > + Rules for generating prefix is following. > + > + 1. Small length (1 to 9) > + Prefix is single char '0' + length -1 > + This gives the range '0' - '8' > + 2. Medium length(10 to 18) > + Prefix is 2 chars - concat('9', '0'+length-10)' > + This gives range '90'-'98' > + 3. Large length(19) > + Prefix is "990" > + 4. Huge length (20 to 2^32-1) > + Prefix stars with '99', then log10 of the length is added as char > + then the number itself is added > + The range is '99120' - '9994294967295' please also explain here how you handle leading zeros > +*/ > + > +#define NATSORT_BUFSIZE 16 > +static size_t natsort_num2str(size_t n, char *s) > +{ > + if (n < 10) > + { > + s[0]= '0' + (char)n -1; > + return 1; > + } > + > + if (n < 19) > + { > + s[0]= '9'; > + s[1]= '0'+ (char)n- 10; > + return 2; > + } > + > + if (n == 19) > + { > + s[0]= '9'; > + s[1]= '9'; > + s[2]= '0'; > + return 3; > + } > + > + /* Here, n > 19*/ > + s[0]= '9'; > + s[1]= '9'; > + size_t log10n= 0; > + for (size_t tmp= n/10; tmp; tmp /= 10) > + log10n++; > + DBUG_ASSERT(log10n && log10n < 10); first, I don't see where the caller ensures that log10n < 10 (in other words, that n < 9999999999) second, please, never write assert(a && b), always do two separate asserts, so that when it fires, it'd be clear what condition was false > + s[2]='0' + (char)log10n; > + return ll2str(n,s+3,10,0) - s; > +} > + > +/** > + Convert a string to natural sort key. > + @param[in] in - input string > + @param[out] s - output string > + > + We assume that memory is preallocated for the output > + string, so that appends do not fail. > + > + The lead zeros count, if positive, is stored after > + the numeric string. > + > + Fractional parts are stored and compared as strings. > + Even if they have numbers in them, they should just > + be compared as strings. > +*/ > +static void to_natsort_key(const String *in, String *s, my_wc_t decimal_sep) > +{ > + CHARSET_INFO *cs= in->charset(); > + uchar *pos= (uchar *) in->ptr(); > + uchar *end= pos + in->length(); > + size_t num_len= 0; > + size_t leading_zeros= 0; > + uchar *num_start= nullptr; > + bool in_fraction= false; > + size_t fraction_len=0; > + > + for (;;) > + { > + my_wc_t c= 0; > + int charlen= cs->mb_wc(&c, pos, end); > + bool is_digit= charlen >= 1 && (c >= '0' && c <= '9'); what do you mean `>= 1` ? how can it be `> 1` and still satisfy the secon part of the condition? > + > + if (!is_digit && (num_start || leading_zeros)) > + { > + /* > + Handle end of digits run. > + > + Write prefix, natsort_num2str(length) is without leading zeros > + Write numeric value, without leading zeros > + If there were leading zeros, write variable length suffix > + */ > + char buf[NATSORT_BUFSIZE]; > + if (!num_len) > + { > + // number was zeros only > + leading_zeros--; > + num_len= 1; > + } > + > + s->append(buf, natsort_num2str(num_len, buf)); > + if (!num_start) > + s->append('0'); > + else > + s->append((const char *) num_start, pos - num_start, cs); > + > + if (leading_zeros) > + s->append(buf, natsort_num2str(leading_zeros, buf)); hmm. How would it sort "1;", "001;", "1a" ? > + > + /* Reset state.*/ > + num_len= 0; > + num_start= nullptr; > + leading_zeros= 0; > + in_fraction= c == decimal_sep; I don't really like this idea. First, see https://natsort.readthedocs.io/en/master/index.html there are many ways one can potentially want to customize natsort. (of course, none of them have to be in this MDEV) One of those customizations, indeed, is treating floating point numbers as floating point numbers. But comma vs dot is locale thing, and a one string argument it's not how you should tell the function what decimal separator the current locale uses. So. Simple approach - remove the second argument, no floating point support. Complex approach - have a second argument that enables floating point support, but that can be used in the future to specify more customizations, not that is hard-wired to specify decimal separator duplicating and bypassing the locale. > + fraction_len= 0; > + } > + > + if (charlen < 1) > + break; > + > + if (is_digit && !in_fraction) > + { > + /* Do not count leading zeros in num_len, and num_pos */ > + if (c != '0' || num_len) > + { > + num_len++; > + if (!num_start) > + num_start= pos; > + } > + else > + leading_zeros++; > + } > + else > + { > + /* Append a non-digit, or fractional part of the number (handling the same as string) */ > + s->append((const char *) pos, charlen); > + if (in_fraction) > + { > + if (!is_digit && fraction_len) > + in_fraction= false; > + fraction_len++; > + } > + } > + pos+= charlen; > + } > +} > + > + > +String *Item_func_natural_sort_key::val_str(String *s) > +{ > + if (args[0]->is_null()) > + { > + null_value= true; > + return nullptr; > + } > + > + String *in= args[0]->val_str(); > + CHARSET_INFO *cs= in->charset(); > + ulong max_allowed_packet= current_thd->variables.max_allowed_packet; > + size_t reserve_length= (size_t) in->length() + in->length() / 2 + 2; > + if (s->alloc((uint32)reserve_length)) > + goto error_exit; > + s->length(0); > + s->set_charset(cs); > + > + to_natsort_key(in, s, decimal_separator()); > + > + DBUG_ASSERT(s->length() < reserve_length); > + > + if (s->length() > max_allowed_packet) better check this inside to_natsort_key(). max_allowed_packet is supposed to prevent the string from growing too large, instead of just letting it grow and erroring out afterwards. > + { > + push_warning_printf(current_thd, Sql_condition::WARN_LEVEL_WARN, > + ER_WARN_ALLOWED_PACKET_OVERFLOWED, > + ER(ER_WARN_ALLOWED_PACKET_OVERFLOWED), func_name(), > + max_allowed_packet); > + goto error_exit; > + } > + return s; > + > +error_exit: > + null_value=true; > + return nullptr; > +} > + > + > +my_wc_t Item_func_natural_sort_key::decimal_separator() > +{ > + if (m_decimal_separator != DECIMAL_SEP_UNDEFINED) > + return m_decimal_separator; > + > + DBUG_ASSERT(arg_count > 1); > + String *s= args[1]->val_str(); > + if (s && s->length()) > + { > + my_wc_t c; > + if (s->charset()->mb_wc(&c, (const uchar *) s->ptr(), > + (const uchar *) s->end())) > + return c; > + } > + return DECIMAL_SEP_NONE; > +} > + > +bool Item_func_natural_sort_key::fix_length_and_dec(void) > +{ > + if (agg_arg_charsets_for_string_result(collation, args, 1)) > + return true; > + DBUG_ASSERT(collation.collation != NULL); > + uint32 max_clen= args[0]->max_char_length(); > + fix_char_length(max_clen + max_clen/2 + ((max_clen <= 3)?1:0)); > + set_maybe_null(); better, only set maybe_null if the first argument maybe_null or the first argument maybe_too_long > + > + if (arg_count> 1 && args[1]->can_eval_in_optimize()) > + { > + /* Initialize decimal separator */ > + m_decimal_separator= decimal_separator(); > + } > + return false; > +} > + > #ifdef WITH_WSREP > > #include "wsrep_mysqld.h" Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] a19495728de: MDEV-16355 Add option for mysqldump to read data as of specific timestamp from system-versioned tables
by Sergei Golubchik 15 Aug '21

15 Aug '21

Hi, Aleksey! I'm reviewing it separately from other commits in the branch, because it looks to be functionally independent from, say, MDEV-16029 mysqldump: dump and restore historical data and doesn't have to wait for the latter to be pushed first. I think I was able to guess what the patch would look like, if you'd apply it to vanilla 10.7 without MDEV-16029. But just to be sure, please, let me look it over again after the cherry-pick. Otherwise no comments code-wise. A couple of comments related to naming, see below. On Aug 15, Aleksey Midenkov wrote: > revision-id: a19495728de (versioning-1.0.5-45-ga19495728de) > parent(s): be4605ae086 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2018-07-19 14:47:35 +0300 > message: > > MDEV-16355 Add option for mysqldump to read data as of specific timestamp from system-versioned tables > > mysqldump changes: > > * --asof-timestamp option specifies historical point; "asof" isn't a word. I'd suggest --as-of-timestamp or simply --as-of > * with --dump-history it dumps all prior history, without it dumps only single point in time; > * query forging protection for --asof-timestamp parameter. > > diff --git a/client/mysqldump.c b/client/mysqldump.c > index eee12555271..f426e8c58ef 100644 > --- a/client/mysqldump.c > +++ b/client/mysqldump.c > @@ -1099,6 +1102,13 @@ static int get_options(int *argc, char ***argv) > my_progname_short); > return(EX_USAGE); > } > + if (opt_asof_timestamp && strchr(opt_asof_timestamp, '\'')) > + { > + fprintf(stderr, > + "%s: --asof-timestamp wrong argument!\n", > + my_progname_short); May be, use the wording that is closer to the error one would get from any other invalid character, like, if one would do --asof-timestamp='!@#$%' That is, instead of "wrong argument", something like fprintf(stderr, "%s: Incorrect DATETIME value: '%s'\n", opt_asof_timestamp, my_progname_short); Because MariaDB [test]> select timestamp'2010!@#$%'; ERROR 1525 (HY000): Incorrect DATETIME value: '2010!@#$%' > + return(EX_USAGE); > + } > if (strcmp(default_charset, charset_info->csname) && > !(charset_info= get_charset_by_csname(default_charset, > MY_CS_PRIMARY, MYF(MY_WME)))) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] cdb6db8ce38: MDEV-25444 mysql --binary-mode is not able to replay some mysqlbinlog outputs
by Sergei Golubchik 14 Aug '21

14 Aug '21

Hi, Sachin! On Jul 12, Sachin Kumar wrote: > revision-id: cdb6db8ce38 (mariadb-10.3.26-155-gcdb6db8ce38) > parent(s): 72753d2b65f > author: Sachin Kumar <sachin.setiya(a)mariadb.com> > committer: Sachin Kumar <sachin.setiya(a)mariadb.com> > timestamp: 2021-05-19 15:46:57 +0100 > message: > > MDEV-25444 mysql --binary-mode is not able to replay some mysqlbinlog outputs > > Problem:- Some binary data is inserted into the table using Jconnector. When > binlog dump of the data is applied using mysql cleint it gives syntax error. > > Reason:- > After investigating it turns out to be a issue of mysql client not able to properly > handle \\\0 <0 in binary>. In all binary files where mysql client fails to insert > these 2 bytes are commom (0x5c00) > > Solution:- > I have changed mysql.cc to include for the possibility that binary string can > have \\\0 in it. > > --- > client/mysql.cc | 8 ++++++-- > mysql-test/main/binary_zero_insert.result | 6 ++++++ > mysql-test/main/binary_zero_insert.test | 15 +++++++++++++++ > mysql-test/std_data/binary_zero_insert.bin | Bin 0 -> 87 bytes > 4 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/client/mysql.cc b/client/mysql.cc > index 2a7c4eaf3e5..8530c105820 100644 > --- a/client/mysql.cc > +++ b/client/mysql.cc > @@ -2319,8 +2319,12 @@ static bool add_line(String &buffer, char *line, size_t line_length, > { > // Found possbile one character command like \c > > + inchar = (uchar) *++pos; > + // In Binary mode , when in_string is not null \0 should not be treated as > + // end statement. This can happen when we are in middle of binary data which > + // can contain \0 and its quoted with ' '. > + if (!real_binary_mode && !*in_string && !inchar) Two comments here. 1. if you check the history where this "real_binary_mode" came from, you'd arrive at commit 2db4392bf4cb and the test file mysql_binary_mode.test So, I'd say it make sense to add your test there too. And in the same style, wthout a separate binary file. in my experimening I did it as simple as --- a/mysql-test/main/mysql_binary_mode.test +++ b/mysql-test/main/mysql_binary_mode.test @@ -16,14 +16,14 @@ let $table_name_right= `SELECT 0x410D0A42`; let $table_name_wrong= `SELECT 0x410A42`; # 0x410042 => 'A\0B' -let $char0= `SELECT 0x410042`; +let $char0= `SELECT 0x4100425C0043`; (there already was a test for a \0 byte, but not for an escaped \\\0) but if you'd like you can add a separate test with a header and everything. > - if (!(inchar = (uchar) *++pos)) > - break; // readline adds one '\' 2. What does the old code mean? Why returning if inchar==0 ? What does the comment mean? It's the code from the last millenium, does it even apply now? For example, there can be no \0 bytes in the string, because of if (!real_binary_mode && strlen(line) != line_length) ... in other words, shouldn't we just remove the if? I tried, at least the main suite didn't break. > + break; // readline adds one '\' > if (*in_string || inchar == 'N') // \N is short for NULL > { // Don't allow commands in string > *out++='\\'; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

[Maria-developers] Pushing down aggregation functions
by Berrocal, Eduardo 04 Aug '21

04 Aug '21

Hello!, Continuing in my series of emails about engine condition pushdown... I wonder if it is possible to push down aggregation functions such as SUM(), MIN(), MAX() or AVG() to the storage engine. Currently, and as far as I can see (I may be wrong here), it is not possible. However, this is something that can be beneficial if the storage engine can perform some of these basic computations itself, like suming over a whole column and returning just the result. The storage engine can even be smart enough to use some hardware advantages like accelerators or computational storage. If you guys think this is indeed possible without drastic code changes, it would be amazing to be pointed at where this is. At first, I would only push down functions if they operate over just one column of the same table. Things can be made more complicated in the future, of course. Thanks a lot!, Eduardo.

2 1