- developers - lists.mariadb.org

Re: [Maria-developers] 322301e2434: MDEV-16849 Extending indexed VARCHAR column should be instantaneous
by Sergei Golubchik 28 Jan '19

28 Jan '19

Hi, Thirunarayanan! On Jan 28, Thirunarayanan Balathandayuthapani wrote: > revision-id: 322301e2434 (mariadb-10.2.21-2-g322301e2434) > parent(s): fe4c65db1dc > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2019-01-01 11:01:03 +0530 > message: > > MDEV-16849 Extending indexed VARCHAR column should be instantaneous > > - Addressed Serg's review comments. > > --- > diff --git a/mysql-test/suite/innodb/r/alter_varchar_change.result b/mysql-test/suite/innodb/r/alter_varchar_change.result > index 5d2c6403aab..2f412aecbf1 100644 > --- a/mysql-test/suite/innodb/r/alter_varchar_change.result > +++ b/mysql-test/suite/innodb/r/alter_varchar_change.result > @@ -316,6 +316,137 @@ t1 CREATE TABLE `t1` ( > DROP TABLE t1; > CREATE TABLE t1(f1 INT NOT NULL, > f2 VARCHAR(100), > +INDEX idx(f2(10)), > +INDEX idx1(f1))ENGINE=InnoDB; > +CALL get_table_id("test/t1", @tbl_id); > +ALTER TABLE t1 MODIFY f2 VARCHAR(50), DROP INDEX idx1; > +CALL get_table_id("test/t1", @tbl1_id); > +SELECT @tbl1_id = @tbl_id; > +@tbl1_id = @tbl_id > +0 > +SELECT @idx1_id = @idx_id; > +@idx1_id = @idx_id > +0 Why `DROP INDEX idx1` ? As far as I remember, the point was "make VARCHAR shorter, but still longer than a prefix". In this case there is no need to rebuild the index, why was it rebuilt? > +SHOW CREATE TABLE t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `f1` int(11) NOT NULL, > + `f2` varchar(50) DEFAULT NULL, > + KEY `idx` (`f2`(10)) > +) ENGINE=InnoDB DEFAULT CHARSET=latin1 > +DROP TABLE t1; > +CREATE TABLE t1(f1 INT NOT NULL, > +f2 VARCHAR(100), > +INDEX idx(f2(10)), > +INDEX idx1(f1))ENGINE=InnoDB; > +CALL get_table_id("test/t1", @tbl_id); > +ALTER TABLE t1 MODIFY f2 VARCHAR(5), DROP INDEX idx1; > +CALL get_table_id("test/t1", @tbl1_id); > +SELECT @tbl1_id = @tbl_id; > +@tbl1_id = @tbl_id > +0 > +SELECT @idx1_id = @idx_id; > +@idx1_id = @idx_id > +0 Correct. "VARCHAR is changed to be shorter than a prefix" means that the index must be rebuilt. > +SHOW CREATE TABLE t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `f1` int(11) NOT NULL, > + `f2` varchar(5) DEFAULT NULL, > + KEY `idx` (`f2`) > +) ENGINE=InnoDB DEFAULT CHARSET=latin1 > +DROP TABLE t1; ... > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2864474a8e8..7eca33812b9 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6629,6 +6629,7 @@ static bool fill_alter_inplace_info(THD *thd, > Go through keys and check if the original ones are compatible > with new table. > */ > + uint old_field_len= 0; > KEY *table_key; > KEY *table_key_end= table->key_info + table->s->keys; > KEY *new_key; > @@ -6698,8 +6699,16 @@ static bool fill_alter_inplace_info(THD *thd, > Key definition has changed if we are using a different field or > if the user key part length is different. > */ > - if (key_part->length <= new_part->length && > - old_field->pack_length() < new_field->pack_length && > + old_field_len= old_field->pack_length(); > + > + if (old_field->type() == MYSQL_TYPE_VARCHAR) > + { > + old_field_len= (old_field->pack_length() > + - ((Field_varstring*) old_field)->length_bytes); > + } > + > + if (key_part->length == old_field_len && > + key_part->length <= new_part->length && Why <= here? Doesn't it mean that if key_part->length == new_part->length then you'll still set ALTER_COLUMN_INDEX_LENGTH even if _nothing_ has changed? > (key_part->field->is_equal((Create_field*) new_field) > == IS_EQUAL_PACK_LENGTH)) > { > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 54411ba: MDEV-17148 DROP DATABASE throw "Directory not empty" after changed lower_case_table_names.
by Sergei Golubchik 28 Jan '19

28 Jan '19

Hi, Alexey! We've already discussed the solution, so ok to push. But it's a bit weird to have "mdev17148" in the std_data, could you rename it to mdev17148.frm? Or, even better, don't check in a binary file, but do instead create table t1 (a int); copy_file $datadir/test/t1.frm $datadir/db1/T1.frm; std_data is for files that cannot be easily created in the test. Like a .frm file from mysql-4.0. On Dec 24, Alexey Botchkov wrote: > revision-id: 54411ba5caee35d0a64e727c117c222b6eef8812 (mariadb-10.2.19-98-g54411ba) > parent(s): 975f4a1295f2b678c5ecea2cf4cfc69e177d88f5 > committer: Alexey Botchkov > timestamp: 2018-12-24 16:41:16 +0400 > message: > > MDEV-17148 DROP DATABASE throw "Directory not empty" after changed lower_case_table_names. > > No need to lowercase table names on case-sensitive file systems, as the > cache won't contain the 'lowercased' table anyway. And it prevents the > UPPERCASE.frm from being deleted. > > mysql-test/r/lowercase_table.result | 2 ++ > mysql-test/std_data/mdev17148 | Bin 0 -> 433 bytes > mysql-test/t/lowercase_table.test | 10 ++++++++++ > sql/sql_db.cc | 8 ++++++-- > 4 files changed, 18 insertions(+), 2 deletions(-) > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 3262afc6c50: 5.6.42-84.2
by Marko Mäkelä 24 Jan '19

24 Jan '19

Hi Sanja, The patch is adding many changes from MySQL for things that we have already fixed in a better way in both InnoDB and XtraDB when merging from MySQL 5.6.42. Most of it should not be applied. I checked the following output: cd percona-server git diff Percona-Server-5.6.41-84.1..Percona-Server-5.6.42-84.2 storage/innobase/| diff -I^@@ - <(cd ../mysql-server;git diff mysql-5.6.41..mysql-5.6.42 storage/innobase/) It identified the following files that have useful changes from Percona: storage/xtradb/include/univ.i storage/xtradb/log/log0online.cc storage/xtradb/row/row0mysql.cc Let me identify the acceptable changes below. Please do not apply anything else to storage/xtradb. > storage/xtradb/handler/ha_innodb.cc | 5 ++++ > storage/xtradb/handler/handler0alter.cc | 35 ++++++++++++++++++++++------ > storage/xtradb/include/os0file.h | 6 ++++- > storage/xtradb/row/row0merge.cc | 17 ++++++++++++-- > storage/xtradb/row/row0sel.cc | 4 ++-- Please, no changes to these files. > storage/xtradb/include/univ.i | 2 +- > storage/xtradb/log/log0online.cc | 41 ++++++++++++++++++++++++++++++++- These are OK as such. > storage/xtradb/row/row0mysql.cc | 15 +++--------- This contains too many changes. Let me quote the only acceptable change: > diff --git a/storage/xtradb/row/row0mysql.cc b/storage/xtradb/row/row0mysql.cc > index ddde8b8f730..9fc5c669ba0 100644 > --- a/storage/xtradb/row/row0mysql.cc > +++ b/storage/xtradb/row/row0mysql.cc > @@ -5500,18 +5500,6 @@ row_rename_table_for_mysql( > goto funct_exit; > } > > - /* Wait for background fts sync to finish */ > - for (retry = 1; dict_fts_index_syncing(table); ++retry) { > - DICT_BG_YIELD(trx); > - if (retry % 100 == 0) { > - ib_logf(IB_LOG_LEVEL_INFO, > - "Unable to rename table %s to new name" > - " %s because FTS sync is running on table." > - " Retrying\n", > - old_name, new_name); > - } > - } > - > /* We use the private SQL parser of Innobase to generate the query > graphs needed in updating the dictionary data from system tables. */ > The above is http://github.com/percona/percona-server/commit/a7f9ecef3ed61468eb5a37af4f1… which is removing a Percona workaround for a MySQL bug that was fixed both in MySQL 5.6.42 and in MariaDB. Best regards, Marko

1 0

Re: [Maria-developers] 1a4031d9c8f: MDEV-17297: stats.records=0 for a table of Archive engine when it has rows, when we run ANALYZE command
by Sergei Golubchik 23 Jan '19

23 Jan '19

Hi, Sergei! On Nov 07, Sergei Petrunia wrote: > revision-id: 1a4031d9c8f314896daf8f7c19d6172b9bba313b (mariadb-10.3.10-49-g1a4031d9c8f) > parent(s): e058a251c10350f3727ca1df022dc5786933535b > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2018-11-07 11:58:30 +0300 > message: > > MDEV-17297: stats.records=0 for a table of Archive engine when it has rows, when we run ANALYZE command ... > diff --git a/storage/archive/ha_archive.cc b/storage/archive/ha_archive.cc > index 487c0038239..9d7f7e66b28 100644 > --- a/storage/archive/ha_archive.cc > +++ b/storage/archive/ha_archive.cc > @@ -1737,6 +1721,38 @@ int ha_archive::info(uint flag) > } > > > +int ha_archive::external_lock(THD *thd, int lock_type) > +{ > + if (lock_type == F_RDLCK) > + { > + // We are going to read from the table. Flush any pending writes that we > + // may have > + flush_and_clear_pending_writes(); > + } > + return 0; > +} This is somewhat unexpected. Normally buffers are flushed at the end of a statement, on F_UNLCK. For example, you'll have a better chance of surviving a crash (or kill -9). Otherwise I don't see much of a difference, so do it as you like. Shouldn't it go into 5.5, as the earliest affected version? > +void ha_archive::flush_and_clear_pending_writes() > +{ > + mysql_mutex_lock(&share->mutex); > + if (share->dirty) > + { > + DBUG_PRINT("ha_archive", ("archive flushing out rows for scan")); > + DBUG_ASSERT(share->archive_write_open); > + azflush(&(share->archive_write), Z_SYNC_FLUSH); > + share->dirty= FALSE; > + } > + > + /* > + This should be an accurate number now, though bulk and delayed inserts can > + cause the number to be inaccurate. > + */ > + stats.records= share->rows_recorded; > + mysql_mutex_unlock(&share->mutex); > +} Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] 1eb364f8b3d: MDEV-17421: mtr does not restart the server whose parameters were changed
by Sergei Golubchik 23 Jan '19

23 Jan '19

Hi, Julius! Looks ok, I'll merge it. But without mtr_restart_t1/mtr_restart_t2 tests. You cannot rely on any specific order of test execution, mtr reorders them as it sees fit. On Jan 23, Julius Goryavsky wrote: > revision-id: 1eb364f8b3d (mariadb-10.1.35-84-g1eb364f8b3d) > parent(s): 3c3c4ae2254 > author: Julius Goryavsky <sysprg(a)gmail.com> > committer: Julius Goryavsky <sysprg(a)gmail.com> > timestamp: 2018-10-10 17:16:34 +0200 > message: > > MDEV-17421: mtr does not restart the server whose parameters were changed > > If a mtr test runs multiple servers and only some of them get > restarted on whatever reason with new command-line parameters, > then subsequent mtr test may fail, because no cleanup is performed. > Replication and Galera test suites are affected. > > In the mtr script, there is a server_need_restart function > that decides whether we need to start a new mysqld process before > the new (next) test. If the mysqld parameters were changed in the > previous test - not necessarily the parameters of the primary mysqld > server, maybe even the secondary server parameters - this function > decides to start a new mysqld process. But since it does not remove > the old (changed) parameters, the new process starts with the > parameters changed by the *previous* test. > > To correct this error, we must delete the modified process > parameters after checking that they have been changed during > the previous test. > > This patch also simplifies and makes more stable the > galera_drop_database test, during debugging of which this > problem was detected. > > https://jira.mariadb.org/browse/MDEV-17421 > > --- > mysql-test/mysql-test-run.pl | 1 + > mysql-test/suite/galera/disabled.def | 1 - > .../suite/galera/r/galera_mtr_restart_t1.result | 1 + > .../suite/galera/r/galera_mtr_restart_t2.result | 1 + > .../suite/galera/t/galera_drop_database.test | 26 +++------------- > .../suite/galera/t/galera_mtr_restart_t1.test | 35 ++++++++++++++++++++++ > .../suite/galera/t/galera_mtr_restart_t2.test | 13 ++++++++ > mysql-test/suite/rpl/r/mtr_restart_t1.result | 5 ++++ > mysql-test/suite/rpl/r/mtr_restart_t2.result | 4 +++ > mysql-test/suite/rpl/t/mtr_restart_t1.test | 31 +++++++++++++++++++ > mysql-test/suite/rpl/t/mtr_restart_t2.test | 18 +++++++++++ > 11 files changed, 113 insertions(+), 23 deletions(-) > > diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl > index d969d7bf9f6..d3ffd16987d 100755 > --- a/mysql-test/mysql-test-run.pl > +++ b/mysql-test/mysql-test-run.pl > @@ -5282,6 +5282,7 @@ sub server_need_restart { > exists $server->{'restart_opts'}) > { > my $use_dynamic_option_switch= 0; > + delete $server->{'restart_opts'}; > if (!$use_dynamic_option_switch) > { > mtr_verbose_restart($server, "running with different options '" . > diff --git a/mysql-test/suite/galera/disabled.def b/mysql-test/suite/galera/disabled.def > index 603031f52b7..464ed6444f9 100644 > --- a/mysql-test/suite/galera/disabled.def > +++ b/mysql-test/suite/galera/disabled.def > @@ -34,4 +34,3 @@ partition : MDEV-13881 galera.partition failed in buildbot with wrong result > galera_as_slave_replication_budle : MDEV-15785 Test case galera_as_slave_replication_bundle caused debug assertion > galera_wan : MDEV-17259: Test failure on galera.galera_wan > galera_pc_ignore_sb : MDEV-17357 Test failure on galera.galera_pc_ignore_sb > -galera_drop_database : test > diff --git a/mysql-test/suite/galera/r/galera_mtr_restart_t1.result b/mysql-test/suite/galera/r/galera_mtr_restart_t1.result > new file mode 100644 > index 00000000000..c628a99f1e8 > --- /dev/null > +++ b/mysql-test/suite/galera/r/galera_mtr_restart_t1.result > @@ -0,0 +1 @@ > +weight=111 > diff --git a/mysql-test/suite/galera/r/galera_mtr_restart_t2.result b/mysql-test/suite/galera/r/galera_mtr_restart_t2.result > new file mode 100644 > index 00000000000..0d488f3d174 > --- /dev/null > +++ b/mysql-test/suite/galera/r/galera_mtr_restart_t2.result > @@ -0,0 +1 @@ > +weight=1 > diff --git a/mysql-test/suite/galera/t/galera_drop_database.test b/mysql-test/suite/galera/t/galera_drop_database.test > index 47fe8315198..12d9efea2f9 100644 > --- a/mysql-test/suite/galera/t/galera_drop_database.test > +++ b/mysql-test/suite/galera/t/galera_drop_database.test > @@ -9,6 +9,7 @@ > --let $node_2=node_2 > --source include/auto_increment_offset_save.inc > > +# Create test database with two sets of the FTS indexes: > CREATE DATABASE fts; > USE fts; > CREATE TABLE fts_t1 (f1 INT PRIMARY KEY AUTO_INCREMENT, f2 VARCHAR(100), FULLTEXT (f2)) ENGINE=InnoDB; > @@ -23,34 +24,19 @@ DROP TABLE ten; > UPDATE fts_t1 SET f2 = 'abcd'; > UPDATE fts_t2 SET f2 = 'efjh'; > > +# Restart the second node: > --connection node_2 > -let $wsrep_cluster_address = `SELECT @@global.wsrep_node_incoming_address`; > --source include/restart_mysqld.inc > > --connection node_1 > --let $wait_condition = SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; > --source include/wait_condition.inc > > ---let $galera_connection_name = node_2a > ---let $galera_server_number = 2 > ---source include/galera_connect.inc > ---connection node_2a > +--connection node_2 > --source include/wait_until_ready.inc > > +# Drop the tables and database after nodes restarted: > --connection node_1 > ---let $restart_parameters = --wsrep-cluster-address=gcomm://$wsrep_cluster_address > ---source include/restart_mysqld.inc > - > ---connection node_2a > ---let $wait_condition = SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; > ---source include/wait_condition.inc > - > ---let $galera_connection_name = node_1a > ---let $galera_server_number = 1 > ---source include/galera_connect.inc > ---connection node_1a > ---source include/wait_until_ready.inc > - > USE fts; > DROP TABLE fts_t1; > DROP TABLE fts_t2; > @@ -58,8 +44,4 @@ SHOW TABLES; > DROP DATABASE fts; > > # Restore original auto_increment_offset values. > ---let $node_1=node_1a > ---let $node_2=node_2a > --source include/auto_increment_offset_restore.inc > - > ---source include/galera_end.inc > diff --git a/mysql-test/suite/galera/t/galera_mtr_restart_t1.test b/mysql-test/suite/galera/t/galera_mtr_restart_t1.test > new file mode 100644 > index 00000000000..563d199625a > --- /dev/null > +++ b/mysql-test/suite/galera/t/galera_mtr_restart_t1.test > @@ -0,0 +1,35 @@ > +# This test verifies that mtr will restart the mysqld process, > +# whose parameters were changed during the test. The verification > +# itself is carried out in the following galera_mtr_restart_t2 > +# test. If mtr restart the mysqld process, then the pc.weight > +# value will be reset to the default ("1"), but if there is no > +# restart, then we will see the changed value ("111") during > +# the next test. > +# > +--source include/galera_cluster.inc > +--source include/have_innodb.inc > + > +# Save original auto_increment_offset values. > +--let $node_1=node_1 > +--let $node_2=node_2 > +--source include/auto_increment_offset_save.inc > + > +--connection node_2 > +--let $restart_parameters = --wsrep_provider_options=pc.weight=111;repl.causal_read_timeout=PT90S;evs.suspect_timeout=PT10S;evs.inactive_timeout=PT30S;evs.install_timeout=PT15S > +--source include/restart_mysqld.inc > + > +--connection node_1 > +--let $wait_condition = SELECT VARIABLE_VALUE = 2 FROM INFORMATION_SCHEMA.GLOBAL_STATUS WHERE VARIABLE_NAME = 'wsrep_cluster_size'; > +--source include/wait_condition.inc > + > +--connection node_2 > +--source include/wait_until_ready.inc > + > +# Check that the parameter value really changed: > +--let $gp = `SELECT SUBSTR(@@wsrep_provider_options, LOCATE('pc.weight =', @@wsrep_provider_options) + LENGTH('pc.weight = '))` > +--let $weight = `SELECT SUBSTR('$gp', 1, LOCATE(';', '$gp') - 1)` > +--echo weight=$weight > + > +# Restore original auto_increment_offset values. > +--connection node_1 > +--source include/auto_increment_offset_restore.inc > diff --git a/mysql-test/suite/galera/t/galera_mtr_restart_t2.test b/mysql-test/suite/galera/t/galera_mtr_restart_t2.test > new file mode 100644 > index 00000000000..fcc1d0515a6 > --- /dev/null > +++ b/mysql-test/suite/galera/t/galera_mtr_restart_t2.test > @@ -0,0 +1,13 @@ > +# This test verifies that mtr will restart the mysqld process, > +# whose parameters were changed during the previous test. If mtr > +# restart the mysqld process, then the pc.weight value will be > +# reset to the default ("1"), but if there is no restart, then > +# we will see the changed value ("111") in this test. > +# > +--source include/galera_cluster.inc > +--source include/have_innodb.inc > + > +--connection node_2 > +--let $gp = `SELECT SUBSTR(@@wsrep_provider_options, LOCATE('pc.weight =', @@wsrep_provider_options) + LENGTH('pc.weight = '))` > +--let $weight = `SELECT SUBSTR('$gp', 1, LOCATE(';', '$gp') - 1)` > +--echo weight=$weight > diff --git a/mysql-test/suite/rpl/r/mtr_restart_t1.result b/mysql-test/suite/rpl/r/mtr_restart_t1.result > new file mode 100644 > index 00000000000..56b64a2fc70 > --- /dev/null > +++ b/mysql-test/suite/rpl/r/mtr_restart_t1.result > @@ -0,0 +1,5 @@ > +include/master-slave.inc > +[connection master] > +include/rpl_stop_server.inc [server_number=1] > +new auto_increment_offset=111 > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/r/mtr_restart_t2.result b/mysql-test/suite/rpl/r/mtr_restart_t2.result > new file mode 100644 > index 00000000000..3c8fe59d607 > --- /dev/null > +++ b/mysql-test/suite/rpl/r/mtr_restart_t2.result > @@ -0,0 +1,4 @@ > +include/master-slave.inc > +[connection master] > +auto_increment_offset=1 > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/mtr_restart_t1.test b/mysql-test/suite/rpl/t/mtr_restart_t1.test > new file mode 100644 > index 00000000000..3003a49c424 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/mtr_restart_t1.test > @@ -0,0 +1,31 @@ > +# This test verifies that mtr will restart the mysqld process, > +# whose parameters were changed during the test. The verification > +# itself is carried out in the following mtr_restart_t2 test. > +# If mtr restart the mysqld process, then the auto_increment_offset > +# parameter value will be reset to the default ("1"), but if there > +# is no restart, then we will see the changed value ("111") during > +# the next test. > +# > +--source include/have_binlog_format_row.inc > +--source include/master-slave.inc > + > +connection master; > + > +let $auto_increment_offset = `SELECT @@global.auto_increment_offset`; > + > +--let $rpl_server_number=1 > +source include/rpl_stop_server.inc; > +--let $rpl_server_parameters=--auto-increment-offset=111 > +--let $keep_include_silent=1 > +source include/rpl_start_server.inc; > +--let $keep_include_silent=0 > + > +let $auto_increment_offset_new = `SELECT @@global.auto_increment_offset`; > +--echo new auto_increment_offset=$auto_increment_offset_new > + > +--disable_query_log > +--eval SET @@global.auto_increment_offset = $auto_increment_offset; > +--enable_query_log > + > +--connection master > +--source include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/mtr_restart_t2.test b/mysql-test/suite/rpl/t/mtr_restart_t2.test > new file mode 100644 > index 00000000000..ab246af6b44 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/mtr_restart_t2.test > @@ -0,0 +1,18 @@ > +# This test verifies that mtr will restart the mysqld process, > +# whose parameters were changed during the previous test. If mtr > +# restart the mysqld process, then the auto_increment_offsert > +# parameter value will be reset to the default ("1"), but if there > +# is no restart, then we will see the changed value ("111") in > +# this test. > +# > +--source include/have_binlog_format_row.inc > +--source include/master-slave.inc > + > +connection master; > + > +let $auto_increment_offset = `SELECT @@global.auto_increment_offset`; > + > +--echo auto_increment_offset=$auto_increment_offset > + > +--connection master > +--source include/rpl_end.inc > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] c8248ca9a22: MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK
by Sergei Golubchik 23 Jan '19

23 Jan '19

Hi, Thiru! Thanks! This is, indeed, much easier to follow. Nicely done. Ok to push. On Jan 23, Thirunarayanan Balathandayuthapani wrote: > revision-id: c8248ca9a22 (mariadb-10.0.37-45-gc8248ca9a22) > parent(s): 12f362c3338 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2019-01-18 12:01:18 +0530 > message: > > MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2302026b18b..645746a7ab0 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6513,6 +6519,17 @@ static bool fill_alter_inplace_info(THD *thd, > new_field->field->field_index != key_part->fieldnr - 1) > goto index_changed; > } > + > + /* > + Rebuild the index if following condition get satisfied: > + > + (i) Old table doesn't have primary key, new table has it and vice-versa > + (ii) Primary key changed to another existing index > + */ > + if ((new_key == new_pk) != > + (table_key == &table->key_info[table->s->primary_key])) I would suggest to rewrite the condition as table_key - table->key_info == table->s->primary_key it means the same thing, but doesn't confuse the reader with a question "what's with a buffer overrun if table->s->primary_key == MAX_KEY?" > + goto index_changed; > + > continue; > > index_changed: > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] c3ef531aaff: MDEV-10963 Fragmented BINLOG query
by Sergei Golubchik 22 Jan '19

22 Jan '19

Hi, Andrei! Thanks! We're almost there. just a couple of minor comments re. format strings, see below. On Dec 18, andrei.elkin(a)pp.inet.fi wrote: > revision-id: c3ef531aaff39d3262882213157d36a545b600dd (mariadb-10.1.37-31-gc3ef531aaff) > parent(s): 541500295abdba7fa619065069291ac9c1dd6e83 > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2018-12-18 18:28:59 +0200 > message: > > MDEV-10963 Fragmented BINLOG query ... > diff --git a/sql/log_event.cc b/sql/log_event.cc > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -10491,12 +10505,128 @@ void Rows_log_event::pack_info(Protocol *protocol) > #endif > > #ifdef MYSQL_CLIENT > +/** > + Print an event "body" cache to @c file possibly in two fragments. > + Each fragement is optionally per @c do_wrap to produce an SQL statement. > + > + @param file a file to print to > + @param body the "body" IO_CACHE of event > + @param do_wrap whether to wrap base64-encoded strings with > + SQL cover. > + @param delimiter delimiter string > + > + The function signals on any error through setting @c body->error to -1. > +*/ > +void copy_cache_to_file_wrapped(FILE *file, > + IO_CACHE *body, > + bool do_wrap, > + const char *delimiter) > +{ > + const char str_binlog[]= "\nBINLOG '\n"; > + const char fmt_delim[]= "'%s\n"; > + const char fmt_n_delim[]= "\n'%s\n"; > + const my_off_t cache_size= my_b_tell(body); > + > + if (reinit_io_cache(body, READ_CACHE, 0L, FALSE, FALSE)) > + { > + body->error= -1; > + goto end; > + } > + > + if (!do_wrap) > + { > + my_b_copy_to_file(body, file, SIZE_T_MAX); > + } > + else if (4 + sizeof(str_binlog) + cache_size + sizeof(fmt_delim) > > + opt_binlog_rows_event_max_encoded_size) > + { > + /* > + 2 fragments can always represent near 1GB row-based > + base64-encoded event as two strings each of size less than > + max(max_allowed_packet). Greater number of fragments does not > + save from potential need to tweak (increase) @@max_allowed_packet > + before to process the fragments. So 2 is safe and enough. > + > + Split the big query when its packet size's estimation exceeds a > + limit. The estimate includes the maximum packet header > + contribution of non-compressed packet. > + */ > + const char fmt_frag[]= "%sSET @binlog_fragment_%d ='\n"; > + > + my_fprintf(file, fmt_frag, "\n", 0); > + if (my_b_copy_to_file(body, file, cache_size/2 + 1)) > + { > + body->error= -1; > + goto end; > + } > + my_fprintf(file, fmt_n_delim, delimiter); see, you use fmt_n_delim only once. here. but you end it with \n. And it forced you to start fmt_frag with %s. please, remove \n from the end of fmt_n_delim and add it to the beginning of fmt_frag > + > + my_fprintf(file, fmt_frag, "", 1); > + if (my_b_copy_to_file(body, file, SIZE_T_MAX)) > + { > + body->error= -1; > + goto end; > + } > + my_fprintf(file, fmt_delim, delimiter); and here - no \n before ' ? I've got lost in all these new lines and format constants. can you please show what the binlog dump will look like? > + > + my_fprintf(file, "BINLOG @binlog_fragment_%d, @binlog_fragment_%d%s\n", > + 0, 1, delimiter); don't pass 0 and 1 as parameters, it's not like they can ever change :) > + } > + else > + { > + my_fprintf(file, str_binlog); > + if (my_b_copy_to_file(body, file, SIZE_T_MAX)) > + { > + body->error= -1; > + goto end; > + } > + my_fprintf(file, fmt_delim, delimiter); > + } > + reinit_io_cache(body, WRITE_CACHE, 0, FALSE, TRUE); > + > +end: > + return; > +} > + > +/** > + The function invokes base64 encoder to run on the current > + event string and store the result into two caches. > + When the event ends the current statement the caches are is copied into > + the argument file. > + Copying is also concerned how to wrap the event, specifically to produce > + a valid SQL syntax. > + When the encoded data size is within max(MAX_ALLOWED_PACKET) > + a regular BINLOG query is composed. Otherwise it is build as fragmented > + > + SET @binlog_fragment_0='...'; > + SET @binlog_fragment_1='...'; > + BINLOG @binlog_fragment_0, @binlog_fragment_1; > + > + where fragments are represented by a pair of indexed user > + "one shot" variables. > + > + NOTE. still @note > + If any changes made don't forget to duplicate them to > + Old_rows_log_event as long as it's supported. > + > + @param file pointer to IO_CACHE > + @param print_event_info pointer to print_event_info specializing > + what out of and how to print the event > + @param name the name of a table that the event operates on > + > + The function signals on any error of cache access through setting > + that cache's @c error to -1. > +*/ > void Rows_log_event::print_helper(FILE *file, > PRINT_EVENT_INFO *print_event_info, > char const *const name) > { > IO_CACHE *const head= &print_event_info->head_cache; > IO_CACHE *const body= &print_event_info->body_cache; > + bool do_print_encoded= > + print_event_info->base64_output_mode != BASE64_OUTPUT_DECODE_ROWS && > + !print_event_info->short_form; > + > if (!print_event_info->short_form) > { > bool const last_stmt_event= get_flags(STMT_END_F); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02587f61f40: MDEV-13095 Implement User Account locking
by Sergei Golubchik 21 Jan '19

21 Jan '19

Hi, Robert! On Jan 21, Robert Bindar wrote: > Hi Sergei! Thanks for reviewing this :-) > > > > You could simply add accessors like > > > > longlong get_password_lifetime() const > > { > > Field *f= get_field(end_priv_columns + 11, MYSQL_TYPE_LONG); > > return f ? f->val_int() : 0; > > } > > > > without changing anything else in User_table_tabular. > > Correct me if I'm mistaken. Let's assume we index like that, > > then get_is_role will become: > > bool get_is_role() const > { > Field *f = get_field(end_priv_columns + 14, MYSQL_TYPE_ENUM); > return f ? f->val_int() - 1 : 0; > } > > If we'll start the server with a 10.4.1 datadir for instance, > get_is_role will go beyond num_fields() and thus will always return > false. From my understanding so far, this means we won't be able to > support previous MariaDB datadirs without upgrading. Is this the > intended behavior? You've missed that I also suggested _not_ to add any more columns to the mysql.user table :) So you can never have is_role and password_lifetime in the same table. It's either 5.7 structure or 10.3 structure. And get_is_role() stays as it was: bool get_is_role() const { Field *f = get_field(end_priv_columns + 11, MYSQL_TYPE_ENUM); return f ? f->val_int() - 1 : 0; } That's exactly what I meant by "without changing anything else in User_table_tabular" Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02587f61f40: MDEV-13095 Implement User Account locking
by Sergei Golubchik 21 Jan '19

21 Jan '19

Hi, Robert! On Jan 20, Robert Bindar wrote: > revision-id: 02587f61f40 (mariadb-10.4.1-100-g02587f61f40) > parent(s): 937c90ce2d3 > author: Robert Bindar <robert(a)mariadb.org> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2019-01-20 00:33:44 +0100 > message: > > MDEV-13095 Implement User Account locking > > Add server support for user account locking. > This patch adds two new statements for denying a user's subsequent login > attempts: > LOCK USER[S] > user_name [, user2_name ] ... > UNLOCK USER[S] > user_name [, user2_name ] ... This doesn't seem to be correct anymore :) > The SHOW GRANTS command was updated to support locking state > information. > > diff --git a/mysql-test/include/switch_to_mysql_user.inc b/mysql-test/include/switch_to_mysql_user.inc > index f5801db6114..eff1d98c2df 100644 > --- a/mysql-test/include/switch_to_mysql_user.inc > +++ b/mysql-test/include/switch_to_mysql_user.inc > @@ -45,6 +45,9 @@ CREATE TABLE mysql.user ( > plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, > authentication_string TEXT NOT NULL, > password_expired ENUM('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, > + password_last_changed datetime NULL, > + password_lifetime int(20) unsigned NULL, > + account_locked enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, No-no. mysql.user structure was frozen in time the moment I pushed the commit with the new structure. It doesn't change anymore. To test 5.7 support, just add ALTER TABLE in one specific test file. See main/system_mysql_db_507.test > is_role enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, > default_role char(80) binary DEFAULT '' NOT NULL, > max_statement_time decimal(12,6) DEFAULT 0 NOT NULL, > diff --git a/mysql-test/main/lock_user.result b/mysql-test/main/lock_user.result > --- /dev/null > +++ b/mysql-test/main/lock_user.result > @@ -0,0 +1,132 @@ > +connect con2,localhost,user2; > +disconnect con2; > +connection default; > +alter user user1@localhost account unlock; > +# > +# A user shouldn't be able to lock itself out "lock himself out" And why not? A user can drop himself or change the password to some garbage. It's the same as locking or worse. > +# > +create user newuser@localhost; > +grant CREATE USER on *.* to newuser@localhost; > +connect con1,localhost,newuser; > +connection con1; > +alter user newuser@localhost account lock; > +ERROR HY000: Operation ALTER USER failed for 'newuser'@'localhost' > +disconnect con1; > +connection default; > +drop user newuser@localhost; ... > +alter user user1@localhost account lock; > +show grants for user1@localhost; > +Grants for user1@localhost > +GRANT USAGE ON *.* TO 'user1'@'localhost' > +ALTER USER 'user1'@'localhost' ACCOUNT LOCK No, I don't think so. It's MySQL compatibility feature, make it MySQL compatible. And it's just not very logical to put ALTER USER in SHOW GRANTS. So, SHOW GRANTS should not show ACCOUNT LOCK, but SHOW CREATE USER should. > diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql > index f788f5d67d5..01d686ac345 100644 > --- a/scripts/mysql_system_tables.sql > +++ b/scripts/mysql_system_tables.sql > @@ -79,7 +79,10 @@ CREATE DEFINER=root@localhost SQL SECURITY DEFINER VIEW IF NOT EXISTS user AS SE > CAST(IFNULL(JSON_VALUE(Priv, '$.max_user_connections'), 0) AS SIGNED) AS max_user_connections, > IFNULL(JSON_VALUE(Priv, '$.plugin'), '') AS plugin, > IFNULL(JSON_VALUE(Priv, '$.authentication_string'), '') AS authentication_string, > - 'N' AS password_expired, > + ELT(IFNULL(JSON_VALUE(Priv, '$.password_expired'), 0) + 1, 'N', 'Y') AS password_expired, > + CAST(JSON_UNQUOTE(JSON_EXTRACT(Priv, '$.password_last_changed')) AS DATETIME) AS password_last_changed, > + CAST(JSON_VALUE(Priv, '$.password_lifetime') AS UNSIGNED) AS password_lifetime, > + ELT(IFNULL(JSON_VALUE(Priv, '$.account_locked'), 0) + 1, 'N', 'Y') AS account_locked, No-no. mysql.user is frozen, it doesn't change anymore. It stays always as it was in 10.3. This was the whole point of moving to JSON, to never ever add new columns to mysql.user table. > ELT(IFNULL(JSON_VALUE(Priv, '$.is_role'), 0) + 1, 'N', 'Y') AS is_role, > IFNULL(JSON_VALUE(Priv, '$.default_role'), '') AS default_role, > CAST(IFNULL(JSON_VALUE(Priv, '$.max_statement_time'), 0.0) AS DECIMAL(12,6)) AS max_statement_time > diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql > index 82ec2faa12d..5442e5fee73 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -643,12 +643,16 @@ ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, > ALTER TABLE user MODIFY plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, > MODIFY authentication_string TEXT NOT NULL; > ALTER TABLE user ADD password_expired ENUM('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; > +ALTER TABLE user ADD password_last_changed datetime NULL after password_expired; > +ALTER TABLE user ADD password_lifetime int(20) unsigned NULL after password_last_changed; > +ALTER TABLE user ADD account_locked enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL after password_lifetime; But that's okay, because otherwise CREATE ... SELECT below won't work. > ALTER TABLE user ADD is_role enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; > ALTER TABLE user ADD default_role char(80) binary DEFAULT '' NOT NULL; > ALTER TABLE user ADD max_statement_time decimal(12,6) DEFAULT 0 NOT NULL; > -- Somewhere above, we ran ALTER TABLE user .... CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin. > -- we want password_expired column to have collation utf8_general_ci. > ALTER TABLE user MODIFY password_expired ENUM('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; > +ALTER TABLE user MODIFY account_locked enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; I don't think it's needed, we're dropping the table anyway. > -- Checking for any duplicate hostname and username combination are exists. > @@ -804,6 +808,10 @@ IF 'BASE TABLE' = (select table_type from information_schema.tables where table_ > 'max_statement_time', max_statement_time, > 'plugin', if(plugin>'',plugin,if(length(password)=16,'mysql_old_password','mysql_native_password')), > 'authentication_string', if(plugin>'',authentication_string,password), > + 'password_expired', 'Y'=password_expired, > + 'password_last_changed', password_last_changed, > + 'password_lifetime', password_lifetime, It would be better to put password expiration changes in the password expiration commit. > + 'account_locked', 'Y'=account_locked, > 'default_role', default_role, > 'is_role', 'Y'=is_role)) as Priv > FROM user; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index fe6fc9148bd..e4fc96b61cd 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -798,12 +806,25 @@ class Grant_table_base > else if (start_priv_columns) > break; > } > + > + end_security_columns= end_priv_columns; > + for (; end_security_columns < num_fields(); end_security_columns++) > + { > + Field *field= m_table->field[end_security_columns]; > + const char *colname= field->field_name.str; > + if (field->real_type() == MYSQL_TYPE_ENUM && > + !strcmp(colname, "is_role")) > + break; > + } Are you trying to make it super-generic, like with privileges, for an arbitrary number of security columns? Don't bother, 5.7 was the last MySQL version with privilege tables, they won't be adding more "security columns" to it. You could simply add accessors like longlong get_password_lifetime() const { Field *f= get_field(end_priv_columns + 11, MYSQL_TYPE_LONG); return f ? f->val_int() : 0; } without changing anything else in User_table_tabular. > } > > /* The index at which privilege columns start. */ > uint start_priv_columns; > /* The index after the last privilege column */ > uint end_priv_columns; > + /* The index after the last security column, i.e. index of is_role. This index > + is equal to num_fields if the is_role column does not exist */ > + uint end_security_columns; > > TABLE *m_table; > }; > @@ -8758,7 +8838,6 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_CSTRING *u, > return (ROLE_GRANT_PAIR *) > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > - > static bool show_role_grants(THD *thd, const char *username, restore the empty line please > const char *hostname, ACL_USER_BASE *acl_entry, > char *buff, size_t buffsize) > @@ -10555,6 +10634,52 @@ int mysql_alter_user(THD* thd, List<LEX_USER> &users_list) > DBUG_RETURN(result); > } > > +/* > + Mark an user account as locked or unlocked. > + > + SYNOPSIS Doxygen comments. This style that you've copied is from (iirc) before 2005:) > + lock_user_account() > + thd The current thread. > + user_table A TL_WRITE opened User_table > + lex_user The user to lock/unlock. > + acl_user The acl user to be updated > + > + RETURN > + > 0 Error. Error message already sent. > + 0 OK. > +*/ > +static int lock_user_account(THD* thd, const User_table &user_table, > + LEX_USER *lex_user, ACL_USER *acl_user) > +{ > + bool update_account_locking= thd->lex->account_options.update_account_locking; > + bool account_locked_value= thd->lex->account_options.account_locked_value; > + > + DBUG_ENTER("lock_user_account"); > + > + if (!update_account_locking) > + DBUG_RETURN(0); > + > + mysql_mutex_assert_owner(&acl_cache->lock); > + > + /* > + Do not allow the current user to lock itself out. > + */ > + ACL_USER *current_acl_user= find_user_wild(thd->security_ctx->priv_host, > + thd->security_ctx->priv_user); > + if (!current_acl_user || > + (!strcmp(lex_user->user.str, current_acl_user->user.str) && > + !my_strcasecmp(system_charset_info, lex_user->host.str, > + current_acl_user->host.hostname))) > + { > + DBUG_RETURN(1); > + } > + > + acl_user->account_locked= account_locked_value; > + > + user_table.set_account_locked(account_locked_value); > + > + DBUG_RETURN(0); > +} > > static bool > mysql_revoke_sp_privs(THD *thd, Grant_tables *tables, const Sp_handler *sph, > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 8f62dca4aec..6faa5966d29 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1148,6 +1148,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > Non-reserved keywords > */ > > +%token <kwd> ACCOUNT_SYM comment /* MYSQL */ > %token <kwd> ACTION /* SQL-2003-N */ > %token <kwd> ADMIN_SYM /* SQL-2003-N */ > %token <kwd> ADDDATE_SYM /* MYSQL-FUNC */ > +opt_account_option: > + ACCOUNT_SYM LOCK_SYM > + { > + Lex->account_options.update_account_locking= true; > + Lex->account_options.account_locked_value= true; > + } > + | ACCOUNT_SYM UNLOCK_SYM > + { > + Lex->account_options.update_account_locking= true; > + Lex->account_options.account_locked_value= false; Why wouldn't you make it a 3-state enum { NOT_SPECIFIED, LOCKED, UNLOCKED } ? looks more natural here. > + } > + ; > + > ev_alter_on_schedule_completion: > /* empty */ { $$= 0;} Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] Fwd: ce4f8955d8d: MDEV-17082 Application period tables: CREATE
by Nikita Malyavin 17 Jan '19

17 Jan '19

---------- Forwarded message --------- From: Nikita Malyavin <nikitamalyavin(a)gmail.com> Date: пт, 18 янв. 2019 г. в 01:46 Subject: Re: ce4f8955d8d: MDEV-17082 Application period tables: CREATE To: Sergei Golubchik <serg(a)mariadb.org> Hi, Sergei! > > +# SQL17 p.832 > > +# Let IDCN be an implementation-dependent <constraint name> that is not > > +# equivalent to the <constraint name> of any table constraint descriptor > > +# included in S. > > Not enough context. It was quite difficult to find this quote. > > When you're quoting the standard, please, specify the part name and the > section name. And there's no need to specify a page. For example: > > SQL2016, Part 2, 11.27 <add table period definition>, General Rules, 2)a) > Fixed, as well as all the rest cites > > + Table_period_info("SYSTEM_TIME", sizeof "SYSTEM_TIME" - 1), > > We have a macro for that, use > > Table_period_info(STRING_WITH_LEN("SYSTEM_TIME")) > ✅ > > + then no promition is done. > > promotion ✅ > > + Table_period_info& get_table_period_info() > > + { > > + return create_info.period_info; > > + } > > really? :) you've introduced a new method get_table_period_info > only to use it two lines below and nowhere else? > Not really, of course. But agree, it's not worth to have such a shortcut. > > + > > + int add_period(Lex_ident name, Lex_ident_sys_st start, Lex_ident_sys_st end) > > + { > > + Table_period_info &info= get_table_period_info(); > > + if (info.is_set()) > > + { > > + my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application"); > > + return 1; > > + } > > + info.set_period(start, end); > > + info.name= name; > > + > > + Virtual_column_info *constr= new Virtual_column_info(); > > + constr->expr= lt_creator.create(thd, create_item_ident_nosp(thd, &start), > > + create_item_ident_nosp(thd, &end)); > > + add_constraint(&null_clex_str, constr, false); > > + return 0; > > + } > > + > > sp_package *get_sp_package() const; > > > > /** > > diff --git a/sql/table.cc b/sql/table.cc > > index ab1a04ccae4..87fa1ba4d61 100644 > > --- a/sql/table.cc > > +++ b/sql/table.cc > > @@ -1228,6 +1228,14 @@ static const Type_handler *old_frm_type_handler(uint pack_flag, > > } > > > > > > +bool TABLE_SHARE::init_period_from_extra2(period_info_t &period, > > + const uchar *data) > > +{ > > + period.start_fieldno= uint2korr(data); > > + period.end_fieldno= uint2korr(data + sizeof(uint16)); > > not "sizeof(uint16)" but 2. I don't think it's guaranteed that > sizeof(uint16) == 2, but as you do uint2korr, you only read 2 bytes, > so data + 2 is always correct. > Yes, it's correct, and I've fixed it, but it wasn't about coorrectness -- but readability. Now what 4, or what is 8 -- is the question that the reader could сonsider. This numbers look magical. With using sizeof(uint16) the one could at least understand, that this is about several double-byte words read. The another problem is that sizeof(uint16) is already used in system versioning, few lines below, so the resulting code will look patchy here. > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > > + f->field_name.str, "VIRTUAL or GENERATED"); > > just use "GENERATED", no English in the error message parameters, please. > VIRTUAL/GENERATED? What'd You say? > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > > + f->field_name.str, "NULL"); > > why is that? it's unrelated and inconsistent with cases like > > create table t1 (a int null, primary key (a)); > > where a column is silently created NOT NULL. > So, you should ignore explicit NULL and create columns NOT NULL. > > If you want to change this, it should be consistent in all cases. > create an MDEV, suggest there that the statement above should be an error. > but I don't think it's worth spending time on now. > Ok, I understand. Fixed. > > + else if (f->flags & VERS_SYSTEM_FIELD) > > + { > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, > > + f->flags & VERS_SYS_START_FLAG ? "ROW START" : "ROW END"); > > No need to have a special case for that. Use the same error as for vcol_info: > > if (f->vcol_info || f->flags & VERS_SYSTEM_FIELD) > { > my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > f->field_name.str, "GENERATED ALWAYS AS"); > Looks not very much user-friendly I think. Let's leave the verbosity as it was > What does this error have to do with MDEV-16976? > SQL standard clearly says (part 2, 11.3 <table definition>, syntax rules, 2)d) > > d) No <column name> in any <column definition> shall be equivalent to PN. > > (here PN is "period name"). So MDEV-16976 or not, a period name should > never match a column name. > I was thinking to implement it through virtual column. Then this check will become redundant, and we'll get duplicate field error > > + eng "Temporary tables with application period not allowed" > > "application time period" > ✅ > > + > > +ER_PERIOD_FIELD_NOT_FOUND > > + eng "Field %`s in PERIOD FOR %`s is not found in table" > > I don't think this needs a new error message, just use ER_BAD_FIELD_ERROR > ✅ > > + > > +ER_PERIOD_TYPES_MISMATCH > > + eng "PERIOD FOR %`s fields types mismatch" > > + > > +ER_PERIOD_MAX_COUNT_EXCEEDED > > better say ER_MORE_THAN_ONE_PERIOD or something... > "max count exceeded" suggests there is some not-trivial > (and possibly variable) max count > > Or ER_PERIOD_ALREADY_DEFINED or ER_DUPLICATE_PERIOD etc. > "already defined" looks like a good one. > > And I've already commented (in another patch) on using English > words as arguments - it will look like a bug when the error message is > translated > I like ER_MORE_THAN_ONE_PERIOD more. IMO application-time periods designed in the way to support a possibility of several periods definition, so i like to treat it as "N periods max supported, but now N=1" > > + eng "cannot specify more than one %s-time period" > > + > > +ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD > > + eng "Could not specify name %`s for field. It is already used by period." > > I don't think this needs a new error message, just use ER_DUP_FIELDNAME > ✅ > > + > > +ER_PERIOD_FIELD_WRONG_ATTRIBUTES > > + eng "Period field %`s cannot be %s" > > + > > +ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD > > + eng "Field %`s is specified for more than one period" > > This looks unused ✅ Sincerely Nikita Malyavin

1 0