- developers - lists.mariadb.org

Re: [Maria-developers] cd41d376d41: MDEV-16697: Fix difference between 32bit/windows and 64bit systems in allowed select nest level
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Oleksandr! Why did you do that? You wrote "Fix difference between 32bit/windows and 64bit systems", but to fix just that it would've been enough to change nesting_map to ulonglong. Using Bitmap<> you're basically preparing for any arbitrary >64 nesting depth. Is this your goal? What feature do you need it for? On Nov 06, Oleksandr Byelkin wrote: > revision-id: cd41d376d413d066f3d497a67d039515fd385a1c (versioning-1.0.5-36-gcd41d376d41) > parent(s): 1748a31ae8d69e4939336f644f884e9de3039e7f > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2018-07-05 17:49:44 +0200 > message: > > MDEV-16697: Fix difference between 32bit/windows and 64bit systems in allowed select nest level > > diff --git a/include/my_global.h b/include/my_global.h > index 11d17ad146a..26078114464 100644 > --- a/include/my_global.h > +++ b/include/my_global.h > @@ -984,7 +984,6 @@ typedef unsigned long my_off_t; > TODO Convert these to use Bitmap class. > */ > typedef ulonglong table_map; /* Used for table bits in join */ > -typedef ulong nesting_map; /* Used for flags of nesting constructs */ > > /* often used type names - opaque declarations */ > typedef const struct charset_info_st CHARSET_INFO; > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -33,6 +33,10 @@ > #include "sql_tvc.h" > #include "item.h" > > +/* Used for flags of nesting constructs */ > +#define SELECT_NESTING_MAP_SIZE 64 > +typedef Bitmap<SELECT_NESTING_MAP_SIZE> nesting_map; > + > /* YACC and LEX Definitions */ Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] fa8879bc800: MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Aleksey! Looking at the patch and the test, I get the impression you're fixing the wrong issue. The problem in MDEV-16370 happened when replicating from non-system-versioned table on the master to a system-versioned table on the slave. You don't have a test case for that. Your fix doesn't change the behavior for this use case. So, I don't understand what you've actually fixed. On Nov 06, Aleksey Midenkov wrote: > revision-id: fa8879bc800d2babf48d082ce8c50ffe520e12cc (versioning-1.0.5-140-gfa8879bc800) > parent(s): 28ae79650d3057ffcd849d2ef28dcd80282c85d2 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2018-09-20 10:04:57 +0300 > message: > > MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables > > Rows_log_event::write_row() always overwrite historical row. > > diff --git a/mysql-test/suite/versioning/t/rpl.test b/mysql-test/suite/versioning/t/rpl.test > index a9e3af45af8..1985b17bf6d 100644 > --- a/mysql-test/suite/versioning/t/rpl.test > +++ b/mysql-test/suite/versioning/t/rpl.test > @@ -133,4 +133,24 @@ sync_slave_with_master; > connection master; > drop table t1; > > +--echo # MDEV-16370 row-based binlog events (updates by primary key) > +--echo # can not be applied multiple times to system versioned tables > +create or replace table t1 (pk int primary key, x int) with system versioning; > +insert into t1 values (1, 1); > +--sync_slave_with_master > +flush logs; > +--connection master > +update t1 set x= 2; > +--sync_slave_with_master > +select * from t1; > +--let $datadir= `select @@datadir` > +--let $f2= query_get_value(SHOW SLAVE STATUS, Relay_Log_File, 1) > +--echo # Replaying binlog... > +--exec $MYSQL_BINLOG $datadir/$f2| $MYSQL -S $SLAVE_MYSOCK test > +select * from t1; > +--connection master > + > +drop database test; > +create database test; > + > --source include/rpl_end.inc > diff --git a/sql/log_event.cc b/sql/log_event.cc > index c550adea26b..c4c04743a30 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -13139,13 +13139,26 @@ Rows_log_event::write_row(rpl_group_info *rgi, > } > > // Handle INSERT. > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned()) > { > - ulong sec_part; > - bitmap_set_bit(table->read_set, table->vers_start_field()->field_index); > - // Check whether a row came from unversioned table and fix vers fields. > - if (table->vers_start_field()->get_timestamp(&sec_part) == 0 && sec_part == 0) > - table->vers_update_fields(); > + if (table->versioned(VERS_TIMESTAMP)) > + { > + // Set vers fields when replicating from not system-versioned table. > + ulong sec_part; > + bitmap_set_bit(table->read_set, table->vers_start_field()->field_index); > + // Check whether a row came from unversioned table and fix vers fields. > + if (table->vers_start_field()->get_timestamp(&sec_part) == 0 && sec_part == 0) > + table->vers_update_fields(); > + else > + goto overwrite_historical_row; > + } > + else > + { > +overwrite_historical_row: > + bitmap_set_bit(table->read_set, table->vers_end_field()->field_index); > + if (!overwrite && !table->vers_end_field()->is_max()) > + overwrite= true; > + } > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 503e494ec6e: MDEV-16241 Assertion `inited==RND' failed in handler::ha_rnd_end()
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Aleksey! On Nov 06, Aleksey Midenkov wrote: > revision-id: 503e494ec6e103b23dc55f8fc8ae68ecec8b8316 (versioning-1.0.6-1-g503e494ec6e) > parent(s): 6120ae4acafb23badecc623e33797949e3f6fbbb > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2018-10-22 11:50:10 +0300 > message: > > MDEV-16241 Assertion `inited==RND' failed in handler::ha_rnd_end() > > Discrepancy in open indexes due to overwritten `read_partitions` upon > `ha_open()` in `ha_partition::clone()`. > > [Fixes tempesta-tech/mariadb#551] > > diff --git a/mysql-test/main/partition_innodb.result b/mysql-test/main/partition_innodb.result > index d27abc07984..c63cbc4601d 100644 > --- a/mysql-test/main/partition_innodb.result > +++ b/mysql-test/main/partition_innodb.result > @@ -925,3 +925,14 @@ test_jfg test_jfg11 > test_jfg test_jfg12#P#p1000 > test_jfg test_jfg12#P#pmax > DROP DATABASE test_jfg; > +# > +# MDEV-16241 Assertion `inited==RND' failed in handler::ha_rnd_end() > +# > +CREATE TABLE t1 (pk INT PRIMARY KEY, x INT, y INT, z INT, KEY (x), KEY (y, z)) > +WITH SYSTEM VERSIONING > +PARTITION BY SYSTEM_TIME (PARTITION p1 HISTORY, PARTITION pn CURRENT); > +INSERT INTO t1 VALUES (1, 7, 8, 9), (2, NULL, NULL, NULL), (3, NULL, NULL, NULL); > +SELECT COUNT(*) FROM t1 WHERE x IS NULL AND y IS NULL AND z IS NULL; > +COUNT(*) > +2 > +DROP TABLE t1; please, add a non-system-versioning test too. It's in one of the comments in the bug report. > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 40380ffeec5..901f812a0e6 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -3725,6 +3725,9 @@ void ha_partition::rebind_psi() > handler *ha_partition::clone(const char *name, MEM_ROOT *mem_root) > { > ha_partition *new_handler; > + MY_BITMAP read_partitions; > + MY_BITMAP lock_partitions; > + int error; > > DBUG_ENTER("ha_partition::clone"); > new_handler= new (mem_root) ha_partition(ht, table_share, m_part_info, > @@ -3748,9 +3751,21 @@ handler *ha_partition::clone(const char *name, MEM_ROOT *mem_root) > ALIGN_SIZE(m_ref_length)*2))) > goto err; > > - if (new_handler->ha_open(table, name, > + DBUG_ASSERT(m_tot_parts == m_part_info->read_partitions.n_bits); > + DBUG_ASSERT(m_tot_parts == m_part_info->lock_partitions.n_bits); > + my_bitmap_init(&read_partitions, NULL, m_tot_parts, FALSE); > + my_bitmap_init(&lock_partitions, NULL, m_tot_parts, FALSE); > + bitmap_copy(&read_partitions, &m_part_info->read_partitions); > + bitmap_copy(&lock_partitions, &m_part_info->lock_partitions); > + error= new_handler->ha_open(table, name, > table->db_stat, > - HA_OPEN_IGNORE_IF_LOCKED | HA_OPEN_NO_PSI_CALL)) > + HA_OPEN_IGNORE_IF_LOCKED | HA_OPEN_NO_PSI_CALL); > + bitmap_copy(&m_part_info->read_partitions, &read_partitions); > + bitmap_copy(&m_part_info->lock_partitions, &lock_partitions); > + my_bitmap_free(&read_partitions); > + my_bitmap_free(&lock_partitions); > + > + if (error) I don't think this is needed. ha_partition::open() is aware of clones, it has `if (m_is_clone_of)` to do some stuff only for clones and some only for fresh opens. Perhaps you just need to move the bitmap-resetting code to be done only for fresh opens? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] b0d21b7: MDEV-14567 Include full name of object in message about incorrect value for column.
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Alexey! On Nov 06, Alexey Botchkov wrote: > > That's why I asked what temporary tables have in table->s->db and > > in table->s->table_name. What do they have there? > > 'Persistent' temporary tables (those created by CREATE TEMPORARY > TABLE) have their database/name there. > Auxiliary temporary tables created to execute queries like SELECT have > 'normal' db value and soumething like #sql_1234 as a table name. But that's fine to show, right? No NULL, no garbage. No need to treat them specially. By the way, can this error ever happen for internal temporary tables? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] b0d21b7: MDEV-14567 Include full name of object in message about incorrect value for column.
by Sergei Golubchik 06 Nov '18

06 Nov '18

Hi, Alexey! On Nov 06, Alexey Botchkov wrote: > > >> + long_field_name(buf, sizeof(buf), table->s, field_name), > >Better change the error message to be > > Incorrect decimal value: '%s' for column %`s.%`s.%`s at row %d > > and use like > > - field_name, > > + table->s->db.str, table->s->table_name.str, field_name, > > Don't think it is better. > There are temporary tables and just Item's that launch that error. So > the 'db' and 'table' do not always defined properly. > The long_field_name() i added handles the temporary table case. Yes, I've noticed you had a specia case for temporary tables. That's why I asked what temporary tables have in table->s->db and in table->s->table_name. What do they have there? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] dc9f113111e: MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX
by Sergei Golubchik 05 Nov '18

05 Nov '18

Hi, Thirunarayanan! On Nov 05, Thirunarayanan Balathandayuthapani wrote: > revision-id: dc9f113111ee4daf3964db938ef3ddd82c777328 (mariadb-10.0.37-6-gdc9f113111e) > parent(s): 38b3e52c3c40b35339b1d18c09053be416a93420 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2018-11-01 21:24:03 +0530 > message: > > MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX > if unique index nominated as PK Subject line in git commits is always one line. Try to prefer the format as of MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK that is, MDEV-number, one space, MDEV summary verbatim, all on one line. This way it'll look good in `git log --oneline` and one can search for a bug description (and new lines won't cause the search to fail). > - Server sets the flag ADD_PK_INDEX, DROP_PK_INDEX if the unique key > nominated as Primary key. this isn't a very helpful comment, just a rephrased commit title :) Up to you, of course, it doesn't hurt. But generally, if you feel the fix it kind of obvious and you cannot come up with anything to put into commit comment body, just don't put there anything at all. > > diff --git a/mysql-test/suite/innodb/t/alter_candidate_key.test b/mysql-test/suite/innodb/t/alter_candidate_key.test > --- /dev/null > +++ b/mysql-test/suite/innodb/t/alter_candidate_key.test > @@ -0,0 +1,47 @@ > +--source include/have_innodb.inc > +--source include/have_debug.inc > +--source include/have_debug_sync.inc > + > +CREATE TABLE t1 (f1 INT NOT NULL, f2 INT NOT NULL, > + UNIQUE KEY uidx2(f1,f2), > + UNIQUE KEY uidx1(f2) > +) ENGINE=InnoDB; > + > +SET DEBUG_SYNC = 'innodb_inplace_alter_table_enter SIGNAL conc_dml WAIT_FOR go_ahead'; > +INSERT INTO t1 VALUES(1,1); > +--send > +alter table t1 change column f1 f11 int, algorithm=inplace; > +connect (con1,localhost,root,,); > +SET DEBUG_SYNC = 'now WAIT_FOR conc_dml'; > +DELETE FROM t1; > +SET DEBUG_SYNC = 'now SIGNAL go_ahead'; > +connection default; > +reap; > +CHECK TABLE t1; > +DROP TABLE t1; Could you explain the test case, please? What's going on here? I don't understand what you're fixing and what I'm reviewing, sorry :( Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 5403baabc7e: MDEV-16849 Extending indexed VARCHAR column should be instantaneous
by Sergei Golubchik 05 Nov '18

05 Nov '18

Hi, Thirunarayanan! On Nov 05, Thirunarayanan Balathandayuthapani wrote: > revision-id: 5403baabc7ef87e301964434e3670ded06af0802 (mariadb-10.2.18-40-g5403baabc7e) > parent(s): ab1ce2204e959bea596817494e932754ab5cbe88 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2018-10-26 16:49:08 +0530 > message: > > MDEV-16849 Extending indexed VARCHAR column should be instantaneous > > Analysis: > ======== > Increasing the length of the indexed varchar column is not an instant operation for > innodb. > > Fix: > === > - Introduce the new handler flag 'Alter_inplace_info::ALTER_COLUMN_INDEX_LENGTH' to > indicate the index length differs due to change of column length changes. > > - InnoDB makes the ALTER_COLUMN_INDEX_LENGTH flag as instant operation. > > This is a port of Mysql fix. > > diff --git a/sql/handler.cc b/sql/handler.cc > index da41daf2440..b1fa53fb145 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -4253,7 +4253,8 @@ handler::check_if_supported_inplace_alter(TABLE *altered_table, > Alter_inplace_info::CHANGE_CREATE_OPTION | > Alter_inplace_info::ALTER_PARTITIONED | > Alter_inplace_info::ALTER_VIRTUAL_GCOL_EXPR | > - Alter_inplace_info::ALTER_RENAME; > + Alter_inplace_info::ALTER_RENAME | > + Alter_inplace_info::ALTER_COLUMN_INDEX_LENGTH; You sure? It means that any engine by default can do ALTER_COLUMN_INDEX_LENGTH inplace, unless some other check in the engine disallows it. I'd rather not enable it for all engine, so better remove it. InnoDB has his own check inside ha_innobase::check_if_supported_inplace_alter() anyway. > /* Is there at least one operation that requires copy algorithm? */ > if (ha_alter_info->handler_flags & ~inplace_offline_operations) > diff --git a/sql/handler.h b/sql/handler.h > index ed2ef822c88..f2250699cfb 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -1994,6 +1994,11 @@ class Alter_inplace_info > > static const HA_ALTER_FLAGS ALTER_DROP_CHECK_CONSTRAINT= 1ULL << 40; > > + /** > + Change in index length such that it doesn't require index rebuild. > + */ > + static const HA_ALTER_FLAGS ALTER_COLUMN_INDEX_LENGTH= 1ULL << 41; > + > /** > Create options (like MAX_ROWS) for the new version of table. > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 02780e7d1e0..94d35c99d0a 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6687,18 +6687,27 @@ static bool fill_alter_inplace_info(THD *thd, > key_part < end; > key_part++, new_part++) > { > + new_field= get_field_by_index(alter_info, new_part->fieldnr); > /* > + If there is a change in index length due to column expansion > + like varchar(X) changed to varchar(X + N) and has a compatible > + packed data representation, we mark it for fast/INPLACE change > + in index definition. InnoDB supports INPLACE for this cases > + > Key definition has changed if we are using a different field or > - if the used key part length is different. It makes sense to > - check lengths first as in case when fields differ it is likely > - that lengths differ too and checking fields is more expensive > - in general case. > + if the user key part length is different. > */ > - if (key_part->length != new_part->length) > + if (key_part->length != new_part->length && > + (ha_alter_info->alter_info->flags & Alter_info::ALTER_CHANGE_COLUMN) && why is that? Can it happen that ALTER_CHANGE_COLUMN flag is not set? > + (key_part->field->is_equal((Create_field*) new_field) > + == IS_EQUAL_PACK_LENGTH)) > + { > + ha_alter_info->handler_flags |= > + Alter_inplace_info::ALTER_COLUMN_INDEX_LENGTH; > + } > + else if (key_part->length != new_part->length) > goto index_changed; > > - new_field= get_field_by_index(alter_info, new_part->fieldnr); > - > /* > For prefix keys KEY_PART_INFO::field points to cloned Field > object with adjusted length. So below we have to check field Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] b0d21b7: MDEV-14567 Include full name of object in message about incorrect value for column.
by Sergei Golubchik 05 Nov '18

05 Nov '18

Hi, Alexey! On Nov 04, Alexey Botchkov wrote: > revision-id: b0d21b7832cea92522d0f8d280ceb8b13b335cbf (mariadb-10.2.18-56-gb0d21b7) > parent(s): 1c6b982e02eeaa75bb6c2f2a3c2b64491dd6d3c8 > committer: Alexey Botchkov > timestamp: 2018-11-04 09:14:40 +0400 > message: > > MDEV-14567 Include full name of object in message about incorrect value for column. Wrong MDEV number again. It should be MDEV-14576 > > Put the full field name in the ER_TRUNCATED_WRONG_VALUE_FOR_FIELD > error message. > > +static char *long_field_name(char *buf, size_t buf_len, > + const TABLE_SHARE *s, const char *field_name) > +{ > + bool is_temp= !s|| > + (tmp_file_prefix_length < s->table_name.length && > + memcmp(s->table_name.str, > + tmp_file_prefix, tmp_file_prefix_length) == 0); > + > + if (is_temp) > + strxnmov(buf, buf_len, "{temporary}.", field_name, NullS); > + else > + strxnmov(buf, buf_len, > + s->db.str, ".", s->table_name.str, ".", field_name, NullS); > + > + return buf; > +} > + > + > int Field_geom::store(const char *from, uint length, CHARSET_INFO *cs) > { > if (!length) > @@ -8620,10 +8638,11 @@ int Field_geom::store(const char *from, uint length, CHARSET_INFO *cs) > geom_type != Field::GEOM_GEOMETRYCOLLECTION && > (uint32) geom_type != wkb_type) > { > + char buf[193]; > my_error(ER_TRUNCATED_WRONG_VALUE_FOR_FIELD, MYF(0), > Geometry::ci_collection[geom_type]->m_name.str, > Geometry::ci_collection[wkb_type]->m_name.str, > - field_name, > + long_field_name(buf, sizeof(buf), table->s, field_name), > (ulong) table->in_use->get_stmt_da()-> > current_row_for_warning()); > goto err_exit; Better change the error message to be Incorrect decimal value: '%s' for column %`s.%`s.%`s at row %d and use like my_error(ER_TRUNCATED_WRONG_VALUE_FOR_FIELD, MYF(0), Geometry::ci_collection[geom_type]->m_name.str, Geometry::ci_collection[wkb_type]->m_name.str, - field_name, + table->s->db.str, table->s->table_name.str, field_name, (ulong) table->in_use->get_stmt_da()-> current_row_for_warning()); What would that do for temporary tables? Anyway, the correct quoting is still `db`.`table`.`column`, not 'db.table.column'. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-15740 (InnoDB lost Durability due to incorrect fix of MDEV-11937)
by Marko Mäkelä 02 Nov '18

02 Nov '18

Hi Kristian, Could you please take a look at this InnoDB regression in MariaDB 10.2 and later: https://jira.mariadb.org/browse/MDEV-15740 Because of this bug, we can no longer write tests that would ensure that certain changes are present in the redo log, before killing and restarting the server. This already hurt me a bit when testing MDEV-15562 (instant DROP COLUMN) in 10.4. I had to add extra sleeps to test cases in order to actually test redo log roll-forward followed by rollback. -- Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

2 3

Re: [Maria-developers] [Commits] 737de39: Issue #885: Can't locate lib/mtr_process.pl in @INC on Ubuntu 18.04.1
by Kristian Nielsen 02 Nov '18

02 Nov '18

Hi Sergey, psergey(a)askmonty.org (Sergei Petrunia) writes: > committer: Sergei Petrunia > branch nick: mysql-5.6-rocksdb-spetrunia > Issue #885: Can't locate lib/mtr_process.pl in @INC on Ubuntu 18.04.1 > > Add "." to @INC array like upstream does. Just want to make sure you are aware of this: https://github.com/MariaDB/server/commit/d185f1d68bb1f37bea10d8ac6188e5a04f… I see the branch nick is mysql-5.6-<something>, so maybe this is like this for legacy reasons, which is fine. But this is basically the wrong fix. Clearly sometimes in ancient history, someone did not understand Perl `require` semantics, and ended up with a horrible mess. For example, if someone ever finds they need `require mtr_misc.pl` in a third file, they will find it will not work. Anyway, just a pointer in case you were not aware of a small land-mine hidden under your feet ;-) - Kristian.

2 1