- developers - lists.mariadb.org

[Maria-developers] Review for follow up fix of MDEV-17323: Backport INFORMATION_SCHEMA.CHECK_CONSTRAINTS to 10.2
by Vicențiu Ciorbaru 31 Jan '19

31 Jan '19

Hi Anel! This is not a terribly bad patch, however you have forgotten the following things: You removed the misspelled is_check_constraint.test file, but forgot to remove the is_check_constraint.result file. Now we would be left with a dangling result file. Please be more careful next time. You modified the logic of is_check_constraints.test file from the one in 10.3 I suggest you use git cherry-pick next time when backporting a patch to get the correct changes. This also helps with merges later, as a change in 10.2 will be easily reconcileable in 10.3. Moving onwards with the patch, you will find other comments inline. > > > commit c94f53a34e0b6bf313ecb36a13d5d138150a8a8d > Author: Anel Husakovic <anel(a)mariadb.org> > Date: Thu Jan 24 03:06:56 2019 -0800 > > diff --git a/mysql-test/suite/funcs_1/t/is_check_constraint.test b/mysql-test/suite/funcs_1/t/is_check_constraint.test > deleted file mode 100644 > index 30a72d02b34..00000000000 > --- a/mysql-test/suite/funcs_1/t/is_check_constraint.test > +++ /dev/null > @@ -1,92 +0,0 @@ > ---source include/have_innodb.inc > ---source include/not_embedded.inc > ---echo # > ---echo # MDEV-17323: Backport INFORMATION_SCHEMA.CHECK_CONSTRAINTS to 10.2 > ---echo # > -CREATE user boo1; > -GRANT select,create,alter,drop on foo.* to boo1; > -SHOW GRANTS for boo1; > -CREATE user boo2; > -create database foo; > -# Connect with user boo1 > -CONNECT(con1,localhost, boo1,, foo); > - > -SET check_constraint_checks=1; > -CREATE TABLE t0 > -( > - t int, check (t>32) # table constraint > -) ENGINE=myisam; > ---sorted_result > -SELECT * from information_schema.check_constraints; > - > -ALTER TABLE t0 > -ADD CONSTRAINT CHK_t0_t CHECK(t<100); > ---sorted_result > -SELECT * from information_schema.check_constraints; > - > -ALTER TABLE t0 > -DROP CONSTRAINT CHK_t0_t; > ---sorted_result > -SELECT * from information_schema.check_constraints; > - > -ALTER TABLE t0 > -ADD CONSTRAINT CHECK(t<50); > ---sorted_result > -SELECT * from information_schema.check_constraints; > - > -CREATE TABLE t1 > -( t int CHECK(t>2), # field constraint > - tt int, > - CONSTRAINT CHECK (tt > 32), CONSTRAINT CHECK (tt <50),# autogenerated names table constraints > - CONSTRAINT CHK_tt CHECK(tt<100) # named table constraint > -) ENGINE=InnoDB; > - --sorted_result > -SELECT * from information_schema.check_constraints; > - > -ALTER TABLE t1 > -DROP CONSTRAINT CHK_tt; > ---sorted_result > -SELECT * from information_schema.check_constraints; > - > -CREATE TABLE t2 > -( > -name VARCHAR(30) CHECK(CHAR_LENGTH(name)>2), #field constraint > -start_date DATE, > -end_date DATE, > -CONSTRAINT CHK_dates CHECK(start_date IS NULL) #table constraint > -)ENGINE=Innodb; > - --sorted_result > -SELECT * from information_schema.check_constraints; > - > -ALTER TABLE t1 > -ADD CONSTRAINT CHK_new_ CHECK(t>tt); > ---sorted_result > -SELECT * from information_schema.check_constraints; > - > -# Create table with same field and table check constraint name > -CREATE TABLE t3 > -( > -a int, > -b int check (b>0), # field constraint named 'b' > -CONSTRAINT b check (b>10) # table constraint > -) ENGINE=InnoDB; > - --sorted_result > -SELECT * from information_schema.check_constraints; > - > -DISCONNECT con1; > -CONNECT(con2, localhost, boo2,, test); > - --sorted_result > -SELECT * from information_schema.check_constraints; > - > -DISCONNECT con2; > -CONNECT(con1, localhost, boo1,,foo); > -DROP TABLE t0; > -DROP TABLE t1; > -DROP TABLE t2; > -DROP TABLE t3; > -DROP DATABASE foo; > - > -DISCONNECT con1; > ---CONNECTION default > -DROP USER boo1; > -DROP USER boo2; This updated version of a test covers less functionality than the previous version, I don't like that. Please keep the old version as is, just rename it properly. Serg has already added the include to skip embedded tests in the most up-to-date 10.2. Please rebase on top of that version before you perform the rename. > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 39763451dfb..e951c69d009 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6474,33 +6474,35 @@ static int get_check_constraints_record(THD *thd, TABLE_LIST *tables, > LEX_STRING *table_name) > { > DBUG_ENTER("get_check_constraints_record"); > - if(res) > + if (res) Nice catch with the missing whitespace. > { > if (thd->is_error()) > push_warning(thd, Sql_condition::WARN_LEVEL_WARN, > thd->get_stmt_da()->sql_errno(), > thd->get_stmt_da()->message()); > - thd->clear_error(); > - DBUG_RETURN(0); > + thd->clear_error(); > + DBUG_RETURN(0); Great change, making sure the code is indented appropriately. > } > - if(!tables->view) > + else if (!tables->view) Again, nice catch with the missing whitespace, but there is no need for the else statement here. The DBUG_RETURN from above covers it. > { > - StringBuffer<MAX_FIELD_WIDTH> str(system_charset_info); > - for (uint i= 0; i < tables->table->s->table_check_constraints; i++) > + if (tables->table->s->table_check_constraints) There is no need for the if statement here. The for loop would already catch the condition and not execute any bit. There is also a subtle difference with initialising the StringBuffer object only once, instead of once every loop. "Smart" compilers might be good enough to realise that the object should be reused, but I don't think that is the general case. Lets make the compiler's job easier and do things properly. Keep the StringBuffer initialisation before the for loop and just reset it's length. If you think that str.length(0) is confusing (I agree that the method name reflects a getter, not a setter), put a comment after it saying something like: "Clear the string." Somehow I vaguely remember we discussed this before, but somehow the wrong version ended up in 10.3. We will change that too in a subsequent patch. Also, ouch, I missed during the review that you switched the columns around compared to 10.3. The current version in 10.2 seems like the correct one for me. Which one is the correct version? I remember we discussed this but I can't find the logs. Furthermore, when we do such changes, please explain *everything you modify* in the commit message, don't make me hunt for differences. > > { > - Virtual_column_info *check= tables->table->check_constraints[i]; > - table->field[0]->store(STRING_WITH_LEN("def"), system_charset_info); > - table->field[3]->store(check->name.str, check->name.length, > - system_charset_info); > - str.length(0); > - check->print(&str); > - table->field[4]->store(str.ptr(), str.length(), system_charset_info); > - if (schema_table_store_record(thd, table)) > - DBUG_RETURN(1); > + for (uint i= 0; i < tables->table->s->table_check_constraints; i++) > + { > + StringBuffer<MAX_FIELD_WIDTH> str(system_charset_info); > + Virtual_column_info *check= tables->table->check_constraints[i]; > + restore_record(table, s->default_values); > + table->field[0]->store(STRING_WITH_LEN("def"), system_charset_info); > + table->field[1]->store(db_name->str, db_name->length, system_charset_info); > + table->field[2]->store(check->name.str, check->name.length, system_charset_info); > + table->field[3]->store(table_name->str, table_name->length, system_charset_info); > + check->print(&str); > + table->field[4]->store(str.ptr(), str.length(), system_charset_info); > + schema_table_store_record(thd, table); > + } > } > } > - > - DBUG_RETURN(0); > + DBUG_RETURN(res); > } > > static int get_schema_constraints_record(THD *thd, TABLE_LIST *tables, > @@ -9342,8 +9346,8 @@ ST_SCHEMA_TABLE schema_tables[]= > fill_schema_applicable_roles, 0, 0, -1, -1, 0, 0}, > {"CHARACTER_SETS", charsets_fields_info, 0, > fill_schema_charsets, make_character_sets_old_format, 0, -1, -1, 0, 0}, > - {"CHECK_CONSTRAINTS", check_constraints_fields_info, 0, get_all_tables, 0, > - get_check_constraints_record, 1, 2, 0, OPTIMIZE_I_S_TABLE|OPEN_TABLE_ONLY}, > + {"CHECK_CONSTRAINTS", check_constraints_fields_info, 0, > + get_all_tables, 0, get_check_constraints_record, 1, 2, 0, OPTIMIZE_I_S_TABLE|OPEN_TABLE_ONLY}, I don't think this particular change was necessary. > {"COLLATIONS", collation_fields_info, 0, > fill_schema_collation, make_old_format, 0, -1, -1, 0, 0}, > {"COLLATION_CHARACTER_SET_APPLICABILITY", coll_charset_app_fields_info, Vicențiu

2 1

Re: [Maria-developers] ce4f8955d8d: MDEV-17082 Application period tables: CREATE
by Sergei Golubchik 30 Jan '19

30 Jan '19

Hi, Nikita! On Jan 18, Nikita Malyavin wrote: > > > 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. In a similar case I used something like #define SIZE 2 #define uintSIZEkorr(X) uint2korr(X) This way there are no magically-looking numbers. You can even write `uint ## SIZE ## korr`, if you're feeling adventurous. > 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? You cannot have parts of an English phrase as a parameter. And "VIRTUAL" is just a subset of "GENERATED" anyway. So, just write my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, "GENERATED"); or, better, my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, "GENERATED ALWAYS AS"); > > > + 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 No, it's illogical. You have columns, like a TIMESTAMP(6) GENERATED ALWAYS AS (NOW() + INTERVAL 1 DAY), b TIMESTAMP(6) GENERATED ALWAYS AS ROW START for the first column the error is Period field %`s cannot be GENERATED for the second column the error is Period field %`s cannot be ROW START Why? They're both GENERATED. You don't say Period field %`s cannot be NOW() + INTERVAL 1 DAY See? So just say in both cases that the period cannot be "GENERATED ALWAYS AS". This is quite sufficient. > > > +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" The standard clearly says there can be at most one application-time period. So "Application-time period already defined" is just as fine. When the new standard will allow for many application-time periods, we can rewrite the error message. But don't hold your breath. And, again, you cannot have "application" as a parameter. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 81768c1: MDEV-5313 Improving audit plugin API.
by Sergei Golubchik 30 Jan '19

30 Jan '19

Hi, Alexey! On Jan 20, Alexey Botchkov wrote: > +typedef struct st_mysql MYSQL; > +typedef struct st_mysql_res MYSQL_RES; > +typedef char **MYSQL_ROW; > + > +extern struct sql_service_st { > + MYSQL *(*sqls_init)(MYSQL *mysql); > + void (*sqls_close)(MYSQL *mysql); > + int (*sqls_real_query)(MYSQL *mysql, const char *q, > + unsigned long length); > + unsigned long (*sqls_affected_rows)(MYSQL *mysql); > + uint (*sqls_errno)(MYSQL *mysql); > + const char *(*sqls_error)(MYSQL *mysql); > + MYSQL_RES *(*sqls_store_result)(MYSQL *mysql); > + void (*sqls_free_result)(MYSQL_RES *result); > + unsigned long (*sqls_num_rows)(MYSQL_RES *res); > + unsigned int (*sqls_num_fields)(MYSQL_RES *res); > + MYSQL_ROW (*sqls_fetch_row)(MYSQL_RES *result); > + unsigned long * (*sqls_fetch_lengths)(MYSQL_RES *result); > +} *sql_service; I'd say, if you're emulating client C API, you don't need all these sqls* functions. You basically only need one function, let's call it mysql_internal_connect() which is a variant of mysql_real_connect() for running SQL internally in the server. And it'll do mysql->methods= &internal_sql_methods; which will be your callback table, you won't need a second one. And after that mysql_* API functions will work normally. At least those that you decide to support. Alternatively you don't even need mysql_internal_connect(), you can integrate it into CLI_MYSQL_REAL_CONNECT (just like you've done with libmysqld.c:mysql_real_connect() calling cli_mysql_real_connect()). Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

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

29 Jan '19

Hi, Thirunarayanan! On Jan 29, Thirunarayanan Balathandayuthapani wrote: > > > +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? > > > If we shorten the column length then whole table must be rebuild. So there > is no need for index comparison over there. Oh, I see. Ok to push, then. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] fd18ed07d65: MDEV-16973 Application period tables: DELETE
by Sergei Golubchik 29 Jan '19

29 Jan '19

Hi, Nikita! Thanks! Please, see review comments below On Jan 03, Nikita Malyavin wrote: > revision-id: fd18ed07d65 (versioning-1.0.7-3-gfd18ed07d65) > parent(s): a643ca815af > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-12-03 13:19:18 +1000 > message: > > MDEV-16973 Application period tables: DELETE > > * inject portion of time updates into mysql_delete main loop > * triggered case emits delete+insert, no updates > * PORTION OF `SYSTEM_TIME` is forbidden > * `DELETE HISTORY .. FOR PORTION OF ...` is forbidden as well > > diff --git a/mysql-test/suite/period/r/delete.result b/mysql-test/suite/period/r/delete.result > new file mode 100644 > index 00000000000..30c9220d6f9 > --- /dev/null > +++ b/mysql-test/suite/period/r/delete.result > @@ -0,0 +1,216 @@ > +create or replace table t (id int, s date, e date, period for apptime(s,e)); > +insert into t values(1, '1999-01-01', '2018-12-12'); > +insert into t values(1, '1999-01-01', '2017-01-01'); > +insert into t values(1, '2017-01-01', '2019-01-01'); > +insert into t values(2, '1998-01-01', '2018-12-12'); > +insert into t values(3, '1997-01-01', '2015-01-01'); > +insert into t values(4, '2016-01-01', '2020-01-01'); I'd insert also, say, (5, '2010-01-01', '2015-01-01'). > +create or replace table t1 (id int, s date, e date, period for apptime(s,e)); > +insert t1 select * from t; > +create or replace table t2 (id int, s date, e date, period for apptime(s,e)); > +insert t2 select * from t; > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +delete from t1 for portion of APPTIME from '2000-01-01' to '2018-01-01'; > +select * from t; try to avoid unsorted selects, the row order is undefined in these cases use --sorted_results command before the select or the order by. --sorted_results is generally better, because it's done on the client, so it doesn't change the execution plan on the server. > +id s e > +1 1999-01-01 2000-01-01 > +1 1999-01-01 2000-01-01 > +1 2018-01-01 2019-01-01 > +2 1998-01-01 2000-01-01 > +3 1997-01-01 2000-01-01 > +4 2018-01-01 2020-01-01 > +1 2018-01-01 2018-12-12 > +2 2018-01-01 2018-12-12 > +select * from t1 order by id, s, e; > +id s e > +1 1999-01-01 2000-01-01 > +1 1999-01-01 2000-01-01 > +1 2018-01-01 2018-12-12 > +1 2018-01-01 2019-01-01 > +2 1998-01-01 2000-01-01 > +2 2018-01-01 2018-12-12 > +3 1997-01-01 2000-01-01 > +4 2018-01-01 2020-01-01 > +select * from log_tbl; > +log > +>DEL: 1, 1999-01-01, 2018-12-12 > +<DEL: 1, 1999-01-01, 2018-12-12 > +>INS: 1, 1999-01-01, 2000-01-01 > +<INS: 1, 1999-01-01, 2000-01-01 > +>INS: 1, 2018-01-01, 2018-12-12 > +<INS: 1, 2018-01-01, 2018-12-12 > +>DEL: 1, 1999-01-01, 2017-01-01 > +<DEL: 1, 1999-01-01, 2017-01-01 > +>INS: 1, 1999-01-01, 2000-01-01 > +<INS: 1, 1999-01-01, 2000-01-01 > +>DEL: 1, 2017-01-01, 2019-01-01 > +<DEL: 1, 2017-01-01, 2019-01-01 > +>INS: 1, 2018-01-01, 2019-01-01 > +<INS: 1, 2018-01-01, 2019-01-01 > +>DEL: 2, 1998-01-01, 2018-12-12 > +<DEL: 2, 1998-01-01, 2018-12-12 > +>INS: 2, 1998-01-01, 2000-01-01 > +<INS: 2, 1998-01-01, 2000-01-01 > +>INS: 2, 2018-01-01, 2018-12-12 > +<INS: 2, 2018-01-01, 2018-12-12 > +>DEL: 3, 1997-01-01, 2015-01-01 > +<DEL: 3, 1997-01-01, 2015-01-01 > +>INS: 3, 1997-01-01, 2000-01-01 > +<INS: 3, 1997-01-01, 2000-01-01 > +>DEL: 4, 2016-01-01, 2020-01-01 > +<DEL: 4, 2016-01-01, 2020-01-01 > +>INS: 4, 2018-01-01, 2020-01-01 > +<INS: 4, 2018-01-01, 2020-01-01 > +# INSERT trigger only also works > +drop trigger tr1del_t2; > +drop trigger tr2del_t2; > +delete from t2 for portion of APPTIME from '2000-01-01' to '2018-01-01'; > +select * from log_tbl; > +log > +>INS: 1, 1999-01-01, 2000-01-01 > +<INS: 1, 1999-01-01, 2000-01-01 > +>INS: 1, 2018-01-01, 2018-12-12 > +<INS: 1, 2018-01-01, 2018-12-12 > +>INS: 1, 1999-01-01, 2000-01-01 > +<INS: 1, 1999-01-01, 2000-01-01 > +>INS: 1, 2018-01-01, 2019-01-01 > +<INS: 1, 2018-01-01, 2019-01-01 > +>INS: 2, 1998-01-01, 2000-01-01 > +<INS: 2, 1998-01-01, 2000-01-01 > +>INS: 2, 2018-01-01, 2018-12-12 > +<INS: 2, 2018-01-01, 2018-12-12 > +>INS: 3, 1997-01-01, 2000-01-01 > +<INS: 3, 1997-01-01, 2000-01-01 > +>INS: 4, 2018-01-01, 2020-01-01 > +<INS: 4, 2018-01-01, 2020-01-01 > +delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; > +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1 The error is good, ER_PARSE_ERROR is very appropriate here. But it should say "right syntax to use near 'for portion of' at line 1" > +delete from t for portion of othertime from '2000-01-01' to '2018-01-01'; > +ERROR HY000: period `othertime` is not found in table > +delete from t for portion of system_time from '2000-01-01' to '2018-01-01'; > +ERROR HY000: period SYSTEM_TIME is not allowed here could be just ER_PARSE_ERROR too, I suppose > +create or replace table t (id int, str text, s date, e date, > +period for apptime(s,e)); > +insert into t values(1, 'data', '1999-01-01', '2018-12-12'); > +insert into t values(1, 'other data', '1999-01-01', '2018-12-12'); > +insert into t values(1, 'deleted', '2000-01-01', '2018-01-01'); > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +show warnings; > +Level Code Message > +select * from t; --sorted_results or order by > +id str s e > +1 data 1999-01-01 2000-01-01 > +1 other data 1999-01-01 2000-01-01 > +1 data 2018-01-01 2018-12-12 > +1 other data 2018-01-01 2018-12-12 > +drop table t1; > +# SQL17 Case 15.7-8.b.i good! you forgot "part 2" and "general rules", but even without them it only takes a couple of seconds to find this quote. > +# If the column descriptor that corresponds to the i-th field of BR > +# describes an identity column, a generated column, a system-time period > +# start column, or a system-time period end column, then let V i be > +# DEFAULT. > +# auto_inrement field is updated "increment" > +create or replace table t (id int primary key auto_increment, s date, e date, > +period for apptime(s, e)); > +insert into t values (default, '1999-01-01', '2018-12-12'); please, add a select * here. > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +select * from t; > +id s e > +2 1999-01-01 2000-01-01 > +3 2018-01-01 2018-12-12 > +truncate t; > +# same for trigger case > +insert into t values (default, '1999-01-01', '2018-12-12'); > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +select * from t; > +id s e > +2 1999-01-01 2000-01-01 > +3 2018-01-01 2018-12-12 > +select * from log_tbl; > +log > +>DEL: 1999-01-01, 2018-12-12 > +<DEL: 1999-01-01, 2018-12-12 > +>INS: 1999-01-01, 2000-01-01 > +<INS: 1999-01-01, 2000-01-01 > +>INS: 2018-01-01, 2018-12-12 > +<INS: 2018-01-01, 2018-12-12 > +# generated columns are updated > +create or replace table t (s date, e date, > +xs date as (s) stored, xe date as (e) stored, > +period for apptime(s, e)); > +insert into t values('1999-01-01', '2018-12-12', default, default); > +select * from t; > +s e xs xe > +1999-01-01 2018-12-12 1999-01-01 2018-12-12 > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +select * from t; > +s e xs xe > +1999-01-01 2000-01-01 1999-01-01 2000-01-01 > +2018-01-01 2018-12-12 2018-01-01 2018-12-12 > +truncate t; > +# same for trigger case > +insert into t values('1999-01-01', '2018-12-12', default, default); > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +select * from t; > +s e xs xe > +1999-01-01 2000-01-01 1999-01-01 2000-01-01 > +2018-01-01 2018-12-12 2018-01-01 2018-12-12 > +select * from log_tbl; > +log > +>DEL: 1999-01-01, 2018-12-12 > +<DEL: 1999-01-01, 2018-12-12 > +>INS: 1999-01-01, 2000-01-01 > +<INS: 1999-01-01, 2000-01-01 > +>INS: 2018-01-01, 2018-12-12 > +<INS: 2018-01-01, 2018-12-12 > +# system_time columns are updated > +create or replace table t ( > +s date, e date, > +row_start SYS_TYPE as row start invisible, > +row_end SYS_TYPE as row end invisible, > +period for apptime(s, e), > +period for system_time (row_start, row_end)) with system versioning; > +insert into t values('1999-01-01', '2018-12-12'), > +('1999-01-01', '1999-12-12'); > +select row_start into @ins_time from t limit 1; > +select * from t order by s, e; > +s e > +1999-01-01 1999-12-12 > +1999-01-01 2018-12-12 > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +select *, if(row_start = @ins_time, "OLD", "NEW"), check_row(row_start, row_end) > +from t for system_time all > +order by s, e, row_start; > +s e if(row_start = @ins_time, "OLD", "NEW") check_row(row_start, row_end) > +1999-01-01 1999-12-12 OLD CURRENT ROW > +1999-01-01 2000-01-01 NEW CURRENT ROW > +1999-01-01 2018-12-12 OLD HISTORICAL ROW > +2018-01-01 2018-12-12 NEW CURRENT ROW > +# same for trigger case > +delete from t; > +delete history from t; > +insert into t values('1999-01-01', '2018-12-12'), > +('1999-01-01', '1999-12-12'); > +select row_start into @ins_time from t limit 1; > +select * from t order by s, e; > +s e > +1999-01-01 1999-12-12 > +1999-01-01 2018-12-12 > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +select *, if(row_start = @ins_time, "OLD", "NEW"), check_row(row_start, row_end) > +from t for system_time all > +order by s, e, row_start; > +s e if(row_start = @ins_time, "OLD", "NEW") check_row(row_start, row_end) > +1999-01-01 1999-12-12 OLD CURRENT ROW > +1999-01-01 2000-01-01 NEW CURRENT ROW > +1999-01-01 2018-12-12 OLD HISTORICAL ROW > +2018-01-01 2018-12-12 NEW CURRENT ROW > +select * from log_tbl; > +log > +>DEL: 1999-01-01, 2018-12-12 > +<DEL: 1999-01-01, 2018-12-12 > +>INS: 1999-01-01, 2000-01-01 > +<INS: 1999-01-01, 2000-01-01 > +>INS: 2018-01-01, 2018-12-12 > +<INS: 2018-01-01, 2018-12-12 > +create or replace database test; > diff --git a/sql/lex.h b/sql/lex.h > index d336c273a18..883316ab988 100644 > --- a/sql/lex.h > +++ b/sql/lex.h > @@ -476,6 +476,7 @@ static SYMBOL symbols[] = { > { "POINT", SYM(POINT_SYM)}, > { "POLYGON", SYM(POLYGON)}, > { "PORT", SYM(PORT_SYM)}, > + { "PORTION", SYM(PORTION)}, PORTION_SYM, please > { "PRECEDES", SYM(PRECEDES_SYM)}, > { "PRECEDING", SYM(PRECEDING_SYM)}, > { "PRECISION", SYM(PRECISION)}, > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 4f2c3fe7611..0c368b40836 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1033,6 +1033,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > %token PERCENT_RANK_SYM > %token PERCENTILE_CONT_SYM > %token PERCENTILE_DISC_SYM > +%token PORTION add /* SQL-2016-R */ comment and it seems to be 2016 standard, not 2017 standard, https://en.wikipedia.org/wiki/SQL please change it everywhere, it's too confusing otherwise > %token POSITION_SYM /* SQL-2003-N */ > %token PRECISION /* SQL-2003-R */ > %token PRIMARY_SYM /* SQL-2003-R */ > @@ -13437,23 +13449,26 @@ delete_part2: > opt_delete_options single_multi {} > | HISTORY_SYM delete_single_table opt_delete_system_time > { > + MYSQL_YYABORT_UNLESS(!Lex->last_table()->has_period()); > Lex->last_table()->vers_conditions= Lex->vers_conditions; > Lex->pop_select(); //main select > } > ; > > delete_single_table: > - FROM table_ident opt_use_partition > + FROM table_ident opt_for_portion_of_time_clause opt_use_partition better put opt_for_portion_of_time_clause after opt_use_partition, I think it's more logical that way. FOR PORTION OF is logical specification, while opt_use_partition is physical one, just like the table name, so it makes sense to keep them together > { > if (unlikely(!Select-> > add_table_to_list(thd, $2, NULL, TL_OPTION_UPDATING, > YYPS->m_lock_type, > YYPS->m_mdl_type, > NULL, > - $3))) > + $4))) > MYSQL_YYABORT; > YYPS->m_lock_type= TL_READ_DEFAULT; > YYPS->m_mdl_type= MDL_SHARED_READ; > + if ($3) > + Lex->last_table()->period_conditions= Lex->period_conditions; > } > ; > > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index ceb3a58a427..7ceff1a215e 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7945,3 +7945,9 @@ ER_PERIOD_FIELD_WRONG_ATTRIBUTES > > ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD > eng "Field %`s is specified for more than one period" > + > +ER_PERIOD_NOT_FOUND > + eng "period %`s is not found in table" first letter capitalized, please > + > +ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED > + eng "period SYSTEM_TIME is not allowed here" same here, or, better, use ER_PARSE_ERROR > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > index 3ae113a4595..e76eb729536 100644 > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -287,7 +287,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > bool return_error= 0; > ha_rows deleted= 0; > bool reverse= FALSE; > - bool has_triggers; > + bool has_triggers= false; > + bool has_period_triggers= false; I don't see why you need has_triggers and has_period_triggers, I find it confusing. just make `has_triggers=true` if there's `FOR PORTION OF` and there are insert triggers. In other words, has_triggers should be true if there are any triggers that this statement might need to fire. > ORDER *order= (ORDER *) ((order_list && order_list->elements) ? > order_list->first : NULL); > SELECT_LEX *select_lex= thd->lex->first_select_lex(); > @@ -334,6 +338,21 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > table_list->on_expr= NULL; > } > } > + if (table_list->has_period()) > + { > + if (table_list->is_view_or_derived()) > + { > + my_error(ER_IT_IS_A_VIEW, MYF(0), table_list->table_name.str); > + DBUG_RETURN(true); > + } > + > + int err= select_lex->period_setup_conds(thd, table_list, conds); > + if (err) > + DBUG_RETURN(true); > + > + conds= table_list->on_expr; > + table_list->on_expr= NULL; Why did you define SELECT_LEX::period_setup_conds to put the condition in TABLE_LIST::on_expr only to hack it up from there? Just make it to return the new condition or NULL in case of an error. > + } > > if (mysql_handle_list_of_derived(thd->lex, table_list, DT_MERGE_FOR_INSERT)) > DBUG_RETURN(TRUE); > @@ -727,6 +752,9 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > delete_record= true; > } > > + portion_of_time_through_update= !has_period_triggers > + && !table->versioned(VERS_TIMESTAMP); I suspect that with VERS_TRX_ID you'd also need to use delete/insert. Add a test, please. > + > THD_STAGE_INFO(thd, stage_updating); > while (likely(!(error=info.read_record())) && likely(!thd->killed) && > likely(!thd->is_error())) > @@ -969,7 +1013,8 @@ int mysql_prepare_delete(THD *thd, TABLE_LIST *table_list, > DBUG_RETURN(TRUE); > } > > - if (unique_table(thd, table_list, table_list->next_global, 0)) > + if (table_list->has_period() Really? Why? > + || unique_table(thd, table_list, table_list->next_global, 0)) > *delete_while_scanning= false; > > if (select_lex->inner_refs_list.elements && > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 5f76f36983b..de8ed2f14b3 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -8302,6 +8302,7 @@ TABLE_LIST *st_select_lex::add_table_to_list(THD *thd, > ptr->ignore_leaves= MY_TEST(table_options & TL_OPTION_IGNORE_LEAVES); > ptr->sequence= MY_TEST(table_options & TL_OPTION_SEQUENCE); > ptr->derived= table->sel; > + ptr->vers_conditions.name= "SYSTEM_TIME"; why? > if (!ptr->derived && is_infoschema_db(&ptr->db)) > { > ST_SCHEMA_TABLE *schema_table; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 19cea4d6a15..ea1c7834f57 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -717,12 +717,152 @@ void vers_select_conds_t::print(String *str, enum_query_type query_type) const > } > } > > +void period_update_condition(THD *thd, TABLE_LIST *table, SELECT_LEX *select, static > + vers_select_conds_t &conds, bool timestamp) please, either use pointers or _const_ references. in other words, `conds` here should be a pointer > +{ > + DBUG_ASSERT(table); > + DBUG_ASSERT(table->table); > +#define newx new (thd->mem_root) > + TABLE_SHARE *share= table->table->s; > + const TABLE_SHARE::period_info_t *period= conds.period; > + > + const LEX_CSTRING &fstart= period->start_field(share)->field_name; > + const LEX_CSTRING &fend= period->end_field(share)->field_name; > + > + conds.field_start= newx Item_field(thd, &select->context, > + table->db.str, table->alias.str, > + thd->make_clex_string(fstart)); > + conds.field_end= newx Item_field(thd, &select->context, > + table->db.str, table->alias.str, > + thd->make_clex_string(fend)); > + > + Item *cond1= NULL, *cond2= NULL, *cond3= NULL, *curr= NULL; > + if (timestamp) > + { > + MYSQL_TIME max_time; > + switch (conds.type) > + { > + case SYSTEM_TIME_UNSPECIFIED: > + thd->variables.time_zone->gmt_sec_to_TIME(&max_time, TIMESTAMP_MAX_VALUE); > + max_time.second_part= TIME_MAX_SECOND_PART; > + curr= newx Item_datetime_literal(thd, &max_time, TIME_SECOND_PART_DIGITS); > + cond1= newx Item_func_eq(thd, conds.field_end, curr); > + break; > + case SYSTEM_TIME_AS_OF: > + cond1= newx Item_func_le(thd, conds.field_start, conds.start.item); > + cond2= newx Item_func_gt(thd, conds.field_end, conds.start.item); > + break; > + case SYSTEM_TIME_FROM_TO: > + cond1= newx Item_func_lt(thd, conds.field_start, conds.end.item); > + cond2= newx Item_func_gt(thd, conds.field_end, conds.start.item); > + cond3= newx Item_func_lt(thd, conds.start.item, conds.end.item); > + break; > + case SYSTEM_TIME_BETWEEN: > + cond1= newx Item_func_le(thd, conds.field_start, conds.end.item); > + cond2= newx Item_func_gt(thd, conds.field_end, conds.start.item); > + cond3= newx Item_func_le(thd, conds.start.item, conds.end.item); > + break; > + case SYSTEM_TIME_BEFORE: > + cond1= newx Item_func_lt(thd, conds.field_end, conds.start.item); > + break; > + default: > + DBUG_ASSERT(0); > + } > + } > + else > + { > + DBUG_ASSERT(table->table->s && table->table->s->db_plugin); > + > + Item *trx_id0= conds.start.item; > + Item *trx_id1= conds.end.item; > + if (conds.start.item && conds.start.unit == VERS_TIMESTAMP) > + { > + bool backwards= conds.type != SYSTEM_TIME_AS_OF; > + trx_id0= newx Item_func_trt_id(thd, conds.start.item, > + TR_table::FLD_TRX_ID, backwards); > + } > + if (conds.end.item && conds.end.unit == VERS_TIMESTAMP) > + { > + trx_id1= newx Item_func_trt_id(thd, conds.end.item, > + TR_table::FLD_TRX_ID, false); > + } > + > + switch (conds.type) > + { > + case SYSTEM_TIME_UNSPECIFIED: > + curr= newx Item_int(thd, ULONGLONG_MAX); > + cond1= newx Item_func_eq(thd, conds.field_end, curr); add here DBUG_ASSERT(!conds.start.item); DBUG_ASSERT(!conds.end.item); > + break; > + case SYSTEM_TIME_AS_OF: > + cond1= newx Item_func_trt_trx_sees_eq(thd, trx_id0, conds.field_start); > + cond2= newx Item_func_trt_trx_sees(thd, conds.field_end, trx_id0); DBUG_ASSERT(!conds.end.item); > + break; > + case SYSTEM_TIME_FROM_TO: > + cond1= newx Item_func_trt_trx_sees(thd, trx_id1, conds.field_start); > + cond3= newx Item_func_lt(thd, conds.start.item, conds.end.item); > + break; hmm, you don't use trx_id0 here at all. did you forget cond2= newx Item_func_trt_trx_sees_eq(thd, conds.field_end, trx_id0); > + case SYSTEM_TIME_BETWEEN: > + cond1= newx Item_func_trt_trx_sees_eq(thd, trx_id1, conds.field_start); > + cond2= newx Item_func_trt_trx_sees_eq(thd, conds.field_end, trx_id0); > + cond3= newx Item_func_le(thd, conds.start.item, conds.end.item); > + break; > + case SYSTEM_TIME_BEFORE: > + cond1= newx Item_func_trt_trx_sees(thd, trx_id0, conds.field_end); > + break; > + default: > + DBUG_ASSERT(0); > + } > + } > + > + if (cond1) > + { > + cond1= and_items(thd, cond2, cond1); > + cond1= and_items(thd, cond3, cond1); > + table->on_expr= and_items(thd, table->on_expr, cond1); > + } > +#undef newx > +} > + > +int SELECT_LEX::period_setup_conds(THD *thd, TABLE_LIST *tables, Item *where) > +{ > + DBUG_ENTER("SELECT_LEX::period_setup_conds"); > + > + if (!thd->stmt_arena->is_conventional() && > + !thd->stmt_arena->is_stmt_prepare_or_first_sp_execute()) > + { > + // statement is already prepared > + DBUG_RETURN(0); > + } > + > + if (thd->lex->is_view_context_analysis()) > + DBUG_RETURN(0); I see that you've copied these two if's from SELECT_LEX::vers_setup_conds. I suspect that the first if() is not quite correct here, and I plan to take a closer look at it. so, it'd be great if you could come up with some way to avoid copying these two if()'s. So that there will be only one place to fix. May be, extract them into a function or something, I don't know. > + > + for (TABLE_LIST *table= tables; table; table= table->next_local) 1. can here be more than one table? you only allow FOR PORTION OF in delete_single_table parser rule. 2. if no, add an assert here that there's only one table in the list 3. and please add a test for DELETE FROM view FOR PORTION OF > + { > + if (!table->table) > + continue; > + vers_select_conds_t &conds= table->period_conditions; > + if (0 == strcasecmp(conds.name.str, "SYSTEM_TIME")) why not to check it in the parser? > + { > + my_error(ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED, MYF(0)); > + DBUG_RETURN(-1); > + } > + if (!table->table->s->period.name.streq(conds.name)) > + { > + my_error(ER_PERIOD_NOT_FOUND, MYF(0), conds.name.str); > + DBUG_RETURN(-1); > + } > + > + conds.period= &table->table->s->period; > + period_update_condition(thd, table, this, conds, true); > + table->on_expr= and_items(thd, where, table->on_expr); > + } > + DBUG_RETURN(0); > +} > + > int SELECT_LEX::vers_setup_conds(THD *thd, TABLE_LIST *tables) > { > DBUG_ENTER("SELECT_LEX::vers_setup_cond"); > -#define newx new (thd->mem_root) > - > - TABLE_LIST *table; > > if (!thd->stmt_arena->is_conventional() && > !thd->stmt_arena->is_stmt_prepare_or_first_sp_execute()) > diff --git a/sql/table.cc b/sql/table.cc > index 87fa1ba4d61..8f1c94dd633 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -6426,9 +6426,14 @@ void TABLE::mark_columns_needed_for_delete() > > if (s->versioned) > { > - bitmap_set_bit(read_set, s->vers.start_field(s)->field_index); > - bitmap_set_bit(read_set, s->vers.end_field(s)->field_index); > - bitmap_set_bit(write_set, s->vers.end_field(s)->field_index); > + bitmap_set_bit(read_set, s->vers.start_fieldno); > + bitmap_set_bit(read_set, s->vers.end_fieldno); > + bitmap_set_bit(write_set, s->vers.end_fieldno); > + } > + > + if (with_period) > + { > + use_all_columns(); > } Nope, I think this is conceptually wrong. TABLE::mark_columns_needed_for_delete marks columns that are needed to *delete* a row. The fact that you might need to *insert* if there's a FOR PORTION OF, is irrelevant here, it's something the caller should bother about. > } > > @@ -7834,6 +7839,101 @@ int TABLE::update_default_fields(bool update_command, bool ignore_errors) > DBUG_RETURN(res); > } > > +static int table_update_generated_fields(TABLE *table) > +{ > + int res= 0; > + if (table->found_next_number_field) > + { > + table->next_number_field= table->found_next_number_field; > + res= table->found_next_number_field->set_default(); > + table->file->update_auto_increment(); > + } > + > + if (table->default_field) > + res= table->update_default_fields(0, false); why is that? > + > + if (likely(!res) && table->vfield) > + res= table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE); > + if (likely(!res) && table->versioned()) > + table->vers_update_fields(); I believe you need to verify CHECK constraints too. and, please, add a test where they fail here. > + return res; > +} > + > +static int period_make_insert(TABLE *table, Item *src, Field *dst) > +{ > + THD *thd= table->in_use; > + > + store_record(table, record[1]); > + int res= src->save_in_field(dst, true); > + > + if (likely(!res)) > + table_update_generated_fields(table); perhaps, move store_record() and save_in_field() into table_update_generated_fields() ? less duplicated code. But the function will need to be renamed, like prepare_for_period_insert or something > + > + if (likely(!res) && table->triggers) > + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, TRG_ACTION_BEFORE, true); > + > + if (likely(!res)) > + res = table->file->ha_write_row(table->record[0]); > + > + if (likely(!res) && table->triggers) > + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, TRG_ACTION_AFTER, true); > + > + restore_record(table, record[1]); > + return res; > +} > + > +int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, > + bool *inside_period) is it used for UPDATE too? or only for DELETE? > +{ > + bool lcond= period_conds.field_start->val_datetime_packed(thd) > + < period_conds.start.item->val_datetime_packed(thd); > + bool rcond= period_conds.field_end->val_datetime_packed(thd) > + > period_conds.end.item->val_datetime_packed(thd); > + > + *inside_period= !lcond && !rcond; > + if (*inside_period) > + return 0; > + > + int res= 0; > + Item *src= lcond ? period_conds.start.item : period_conds.end.item; > + uint dst_fieldno= lcond ? s->period.end_fieldno : s->period.start_fieldno; > + > + store_record(this, record[1]); > + if (likely(!res)) > + res= src->save_in_field(field[dst_fieldno], true); > + > + if (likely(!res)) > + res= table_update_generated_fields(this); > + may be, an assert that there're no delete/insert triggers in a table? > + if(likely(!res)) > + res= file->ha_update_row(record[1], record[0]); > + > + restore_record(this, record[1]); > + > + if (likely(!res) && lcond && rcond) > + res= period_make_insert(this, period_conds.end.item, > + field[s->period.start_fieldno]); > + > + return res; > +} > + > +int TABLE::insert_portion_of_time(THD *thd, vers_select_conds_t &period_conds) > +{ > + bool lcond= period_conds.field_start->val_datetime_packed(thd) > + < period_conds.start.item->val_datetime_packed(thd); > + bool rcond= period_conds.field_end->val_datetime_packed(thd) > + > period_conds.end.item->val_datetime_packed(thd); > + > + int res= 0; > + if (lcond) > + res= period_make_insert(this, period_conds.start.item, > + field[s->period.end_fieldno]); > + if (likely(!res) && rcond) > + res= period_make_insert(this, period_conds.end.item, > + field[s->period.start_fieldno]); > + > + return res; > +} > > void TABLE::vers_update_fields() > { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 5

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

29 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ä 25 Jan '19

25 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 24 Jan '19

24 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