- developers - lists.mariadb.org

[Maria-developers] Some more input on optimizer_trace
by Sergey Petrunia 10 Feb '19

10 Feb '19

Hi Varun, I've did some adjustments MDEV-18489 and pushed the patch into 10.4-optimizer-trace, please check it out. Also I have filed MDEV-18527 and MDEV-18528. Some input on the code: > --- 10.4-optimizer-trace-orig/sql/sql_select.cc > +++ 10.4-optimizer-trace-cl/sql/sql_select.cc > @@ -15983,12 +16250,26 @@ optimize_cond(JOIN *join, COND *conds, > that occurs in a function set a pointer to the multiple equality > predicate. Substitute a constant instead of this field if the > multiple equality contains a constant. > - */ > - DBUG_EXECUTE("where", print_where(conds, "original", QT_ORDINARY);); > + */ > + > + Opt_trace_context *const trace = &thd->opt_trace; > + Json_writer *writer= trace->get_current_json(); > + Json_writer_object trace_wrapper(writer); > + Json_writer_object trace_cond(writer, "condition_processing"); > + trace_cond.add("condition", join->conds == conds ? "WHERE" : "HAVING") > + .add("original_condition", conds); > + > + Json_writer_array trace_steps(writer, "steps"); > + DBUG_EXECUTE("where", print_where(conds, "original", QT_ORDINARY);); Small question: Why was DBUG_EXECUTE shifted right? A bigger question: the code seems unnecesarily verbose: 1. > + Opt_trace_context *const trace = &thd->opt_trace; 2. > + Json_writer *writer= trace->get_current_json(); 3. > + Json_writer_object trace_wrapper(writer); 4. > + Json_writer_object trace_cond(writer, "condition_processing"); Can we save the space by just calling the constructors: Json_writer_object trace_wrapper(thd); Json_writer_object trace_cond(thd, "condition_processing"); ? This applies here and in many other places. Alternative, we could use: Json_writer_object trace_wrapper(thd); Json_writer_object trace_cond(trace_wrapper, "condition_processing"); .. which makes the nesting clearer (and we could also add debug safety check: it is invalid to operate on trace_wrapper until trace_cond hasn't been end()'ed) > --- 10.4-optimizer-trace-orig/sql/opt_range.cc > +++ 10.4-optimizer-trace-cl/sql/opt_range.cc > > +void TRP_ROR_UNION::trace_basic_info(const PARAM *param, > + Json_writer_object *trace_object) const > +{ > + Opt_trace_context *const trace = &param->thd->opt_trace; > + Json_writer* writer= trace->get_current_json(); > + trace_object->add("type", "index_roworder_union"); > + Json_writer_array ota(writer, "union_of"); The name 'ota' makes sense in MySQL codebase (where it is a contraction of Optimizer_trace_array), but is really confusing in MariaDB codebase. Please change everywhere to "smth_trace" or something like that (jwa in the worst case). > @@ -2654,12 +2833,18 @@ int SQL_SELECT::test_quick_select(THD *t > > if (cond) > { > - if ((tree= cond->get_mm_tree(&param, &cond))) > + { > + Json_writer_array trace_range_summary(writer, > + "setup_range_conditions"); > + tree= cond->get_mm_tree(&param, &cond); > + } Does this ever produce anything meaningful, other than empty: "setup_range_conditions": [], In MySQL, the possible contents of this array are: "impossible_condition": { "cause": "comparison_with_null_always_false" } /* impossible_condition */ "impossible_condition": { "cause": "null_field_in_non_null_column" } /* impossible_condition */ But in MariaDB we don't have this (should we?) Also, why_use_underscores_in_value_of_cause? It is a quoted string where spaces are allowed. MySQL seems to have figured this out at some point and have a few cause strings using spaces. > --- 10.4-optimizer-trace-orig/sql/opt_table_elimination.cc > +++ 10.4-optimizer-trace-cl/sql/opt_table_elimination.cc > @@ -522,7 +524,8 @@ eliminate_tables_for_list(JOIN *join, > List<TABLE_LIST> *join_list, > table_map tables_in_list, > Item *on_expr, > - table_map tables_used_elsewhere); > + table_map tables_used_elsewhere, > + Json_writer_array* eliminate_tables); Please change the name to something indicating it's just a trace. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 1

Re: [Maria-developers] b1a4d1e4937: MDEV-16973 Application-time periods: DELETE
by Sergei Golubchik 08 Feb '19

08 Feb '19

Hi, Nikita! On Jan 31, Nikita Malyavin wrote: > revision-id: b1a4d1e4937 (versioning-1.0.7-3-gb1a4d1e4937) > parent(s): 4b01d3aee60 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-01-30 22:54:00 +1000 > message: > > MDEV-16973 Application-time periods: 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 > --- /dev/null > +++ b/mysql-test/suite/period/r/delete.result > @@ -0,0 +1,244 @@ > +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'); > +insert into t values(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; > +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 t1; > +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; > +id log here (and everywhere selecting from log_tbl), better do ORDER BY id it's quite difficult to follow the sequence of events otherwise > +1 >DEL: 1, 1999-01-01, 2018-12-12 > +10 <INS: 1, 1999-01-01, 2000-01-01 > +11 >DEL: 1, 2017-01-01, 2019-01-01 > +12 <DEL: 1, 2017-01-01, 2019-01-01 > +13 >INS: 1, 2018-01-01, 2019-01-01 > +14 <INS: 1, 2018-01-01, 2019-01-01 > +15 >DEL: 2, 1998-01-01, 2018-12-12 > +16 <DEL: 2, 1998-01-01, 2018-12-12 > +17 >INS: 2, 1998-01-01, 2000-01-01 > +18 <INS: 2, 1998-01-01, 2000-01-01 > +19 >INS: 2, 2018-01-01, 2018-12-12 > +2 <DEL: 1, 1999-01-01, 2018-12-12 > +20 <INS: 2, 2018-01-01, 2018-12-12 > +21 >DEL: 3, 1997-01-01, 2015-01-01 > +22 <DEL: 3, 1997-01-01, 2015-01-01 > +23 >INS: 3, 1997-01-01, 2000-01-01 > +24 <INS: 3, 1997-01-01, 2000-01-01 > +25 >DEL: 4, 2016-01-01, 2020-01-01 > +26 <DEL: 4, 2016-01-01, 2020-01-01 > +27 >INS: 4, 2018-01-01, 2020-01-01 > +28 <INS: 4, 2018-01-01, 2020-01-01 > +29 >DEL: 5, 2010-01-01, 2015-01-01 > +3 >INS: 1, 1999-01-01, 2000-01-01 > +30 <DEL: 5, 2010-01-01, 2015-01-01 > +4 <INS: 1, 1999-01-01, 2000-01-01 > +5 >INS: 1, 2018-01-01, 2018-12-12 > +6 <INS: 1, 2018-01-01, 2018-12-12 > +7 >DEL: 1, 1999-01-01, 2017-01-01 > +8 <DEL: 1, 1999-01-01, 2017-01-01 > +9 >INS: 1, 1999-01-01, 2000-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; > +id log > +1 >INS: 1, 1999-01-01, 2000-01-01 > +10 <INS: 2, 1998-01-01, 2000-01-01 > +11 >INS: 2, 2018-01-01, 2018-12-12 > +12 <INS: 2, 2018-01-01, 2018-12-12 > +13 >INS: 3, 1997-01-01, 2000-01-01 > +14 <INS: 3, 1997-01-01, 2000-01-01 > +15 >INS: 4, 2018-01-01, 2020-01-01 > +16 <INS: 4, 2018-01-01, 2020-01-01 > +2 <INS: 1, 1999-01-01, 2000-01-01 > +3 >INS: 1, 2018-01-01, 2018-12-12 > +4 <INS: 1, 2018-01-01, 2018-12-12 > +5 >INS: 1, 1999-01-01, 2000-01-01 > +6 <INS: 1, 1999-01-01, 2000-01-01 > +7 >INS: 1, 2018-01-01, 2019-01-01 > +8 <INS: 1, 2018-01-01, 2019-01-01 > +9 >INS: 2, 1998-01-01, 2000-01-01 > +# multi-table DELETE is not possible > +delete t, t1 from t1, t 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 'for portion of apptime from '2000-01-01' to '2018-01-01'' at line 1 > +delete t for portion of apptime from '2000-01-01' to '2018-01-01', t1 from t, t1; > +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 'for portion of apptime from '2000-01-01' to '2018-01-01', t1 from t, t1' at line 1 > +# Here another check fails before parsing ends > +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1; > +ERROR 42S02: Unknown table 't1' in MULTI DELETE > +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 'for portion of apptime from '2000-01-01' to '2018-01-01'' 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 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 'system_time' at line 1 > +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; > +id str s e > +1 data 1999-01-01 2000-01-01 > +1 data 2018-01-01 2018-12-12 > +1 other data 1999-01-01 2000-01-01 > +1 other data 2018-01-01 2018-12-12 > +drop table t1; > +# SQL16, Part 2, 15.7 <Effect of deleting rows from base tables>, > +# General rules, 8)b)i) > +# 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_increment field is updated > +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'); > +select * from t; > +id s e > +1 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 > +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; > +id log > +1 >DEL: 1999-01-01, 2018-12-12 > +2 <DEL: 1999-01-01, 2018-12-12 > +3 >INS: 1999-01-01, 2000-01-01 > +4 <INS: 1999-01-01, 2000-01-01 > +5 >INS: 2018-01-01, 2018-12-12 > +6 <INS: 2018-01-01, 2018-12-12 > +# auto_increment field overflow > +create or replace table t (id tinyint auto_increment primary key, > +s date, e date, period for apptime(s,e)); > +insert into t values(127, '1999-01-01', '2018-12-12'); > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +ERROR 22003: Out of range value for column 'id' at row 1 add select * from t here, please > +# same for trigger case > +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; > +ERROR 22003: Out of range value for column 'id' at row 1 and here and select from the log_tbl too > +# 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; > +id log > +1 >DEL: 1999-01-01, 2018-12-12 > +2 <DEL: 1999-01-01, 2018-12-12 > +3 >INS: 1999-01-01, 2000-01-01 > +4 <INS: 1999-01-01, 2000-01-01 > +5 >INS: 2018-01-01, 2018-12-12 > +6 <INS: 2018-01-01, 2018-12-12 > +# View can't be used > +create or replace view v as select * from t; > +delete from v for portion of p from '2000-01-01' to '2018-01-01'; > +ERROR 42S02: 'v' is a view > +# 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; > +id log > +1 >DEL: 1999-01-01, 2018-12-12 > +2 <DEL: 1999-01-01, 2018-12-12 > +3 >INS: 1999-01-01, 2000-01-01 > +4 <INS: 1999-01-01, 2000-01-01 > +5 >INS: 2018-01-01, 2018-12-12 > +6 <INS: 2018-01-01, 2018-12-12 > +create or replace database test; > diff --git a/mysql-test/suite/versioning/t/select.test b/mysql-test/suite/versioning/t/select.test > --- a/mysql-test/suite/versioning/t/select.test > +++ b/mysql-test/suite/versioning/t/select.test > @@ -107,6 +107,32 @@ for system_time as of timestamp @t0 as t; > drop table t1; > drop table t2; > > +# Query conditions check > + > +create or replace table t1(x int) with system versioning; > +insert into t1 values (1); > +delete from t1; > +insert into t1 values (2); > +delete from t1; > +insert into t1 values (3); > +delete from t1; > + > +select row_start into @start1 from t1 for system_time all where x = 1; > +select row_end into @end1 from t1 for system_time all where x = 1; > +select row_start into @start2 from t1 for system_time all where x = 2; > +select row_end into @end2 from t1 for system_time all where x = 2; > +select row_start into @start3 from t1 for system_time all where x = 3; > +select row_end into @end3 from t1 for system_time all where x = 3; > + > +select x as ASOF_x from t1 for system_time as of @start2; > +select x as ASOF_x from t1 for system_time as of @end2; > +select x as FROMTO_x from t1 for system_time from @start1 to @end3; > +select x as FROMTO_x from t1 for system_time from @end1 to @start2; > +select x as BETWAND_x from t1 for system_time between @start1 and @end3; > +select x as BETWAND_x from t1 for system_time between @end1 and @start2; > + > +drop table t1; what does that have to do with MDEV-16973? > + > # Wildcard expansion on hidden fields > > create table t1( > @@ -233,9 +259,9 @@ select x from t1 for system_time as of @trx_start; > --echo ### Issue #365, bug 4 (related to #226, optimized fields) > create or replace table t1 (i int, b int) with system versioning; > insert into t1 values (0, 0), (0, 0); > -select min(i) over (partition by b) as f > -from (select i + 0 as i, b from t1) as tt > -order by i; > +#select min(i) over (partition by b) as f > +#from (select i + 0 as i, b from t1) as tt > +#order by i; why is this? > > --echo ### Issue #365, bug 5 (dangling AND) > create or replace table t1 (a int); > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -245,6 +245,48 @@ static bool record_should_be_deleted(THD *thd, TABLE *table, SQL_SELECT *sel, > return false; > } > > +inline > +int TABLE::update_portion_of_time(THD *thd, > + const vers_select_conds_t &period_conds, > + bool *inside_period) I don't understand why you want to keep this very much DELETE-only functionality in the TABLE class which is used everywhere. And what's the point of pretending it's in a common TABLE class, if it can only be used in sql_delete.cc? I find it quite confusing :( > +{ > + 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; > + > + DBUG_ASSERT(!triggers || (!triggers->has_triggers(TRG_EVENT_INSERT, > + TRG_ACTION_BEFORE) > + && !triggers->has_triggers(TRG_EVENT_INSERT, > + TRG_ACTION_AFTER) > + && !triggers->has_delete_triggers())); > + > + 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= update_generated_fields(); > + > + 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(period_conds.end.item, > + field[s->period.start_fieldno]); > + > + return res; > +} > > inline > int TABLE::delete_row() > @@ -672,6 +736,9 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > > table->mark_columns_needed_for_delete(); > > + if (table_list->has_period()) > + table->use_all_columns(); may be even if (table_list->has_period()) table->use_all_columns() else table->mark_columns_needed_for_delete(); > + > if ((table->file->ha_table_flags() & HA_CAN_FORCE_BULK_DELETE) && > !table->prepare_triggers_for_delete_stmt_or_event()) > will_batch= !table->file->start_bulk_delete(); > @@ -727,6 +794,16 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > delete_record= true; > } > > + /* > + From SQL2016, Part 2, 15.7 <Effect of deleting rows from base table>, > + General Rules, 8), we can conclude that DELETE FOR PORTTION OF time performs > + 0-2 INSERTS + DELETE. We can substitute INSERT+DELETE with one UPDATE, but > + only if there are no triggers set. > + It is also meaningless for system-versioned table > + */ > + portion_of_time_through_update= !has_triggers > + && !table->versioned(VERS_TIMESTAMP); I still don't understand why you disable portion_of_time_through_update for VERS_TIMESTAMP, but not for VERS_TRX_ID. > + > THD_STAGE_INFO(thd, stage_updating); > while (likely(!(error=info.read_record())) && likely(!thd->killed) && > likely(!thd->is_error())) > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -3576,6 +3577,20 @@ void LEX::set_trg_event_type_for_tables() > break; > } > > + if (period_conditions.is_set()) > + { > + switch (sql_command) > + { > + case SQLCOM_DELETE: > + case SQLCOM_UPDATE: > + case SQLCOM_REPLACE: > + new_trg_event_map |= static_cast<uint8> > + (1 << static_cast<int>(TRG_EVENT_INSERT)); I've added a helper for this recently, use new_trg_event_map |= trg2bit(TRG_EVENT_INSERT); > + default: > + break; > + } > + } > + > > /* > Do not iterate over sub-selects, only the tables in the outermost > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -9295,6 +9296,22 @@ history_point: > $$= Vers_history_point($1, $2); > } > ; > +opt_for_portion_of_time_clause: > + /* empty */ > + { > + $$= false; > + } > + | FOR_SYM PORTION_SYM OF_SYM ident FROM history_point TO_SYM history_point history_point allows TIMESTAMP '2010-10-10 10:10:10' and TRANSACTION 1234. You don't need any of that, just use a corresponding expression rule. E.g. bit_expr, like history_point does. > + { > + if (unlikely(0 == strcasecmp($4.str, "SYSTEM_TIME"))) > + { > + thd->parse_error(ER_SYNTAX_ERROR, $4.str); no, for the error message to look correct you need to pass the pointer into the query text. It's usually done like this: FOR_SYM PORTION_SYM OF_SYM remember_tok_start ident FROM ... { ... thd->parse_error(ER_SYNTAX_ERROR, $4); > + MYSQL_YYABORT; > + } > + $$= true; > + Lex->period_conditions.init(SYSTEM_TIME_FROM_TO, $6, $8, $4); > + } > + ; > > opt_for_system_time_clause: > /* empty */ Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

[Maria-developers] eq_range_index_dives_limit
by Sergey Petrunia 06 Feb '19

06 Feb '19

Hello, So, MDEV-16934 introduced eq_range_index_dives_limit into 10.2.8 and 10.3.0. The default was set to 0 (which means no limit) in order to not introduce optimizer behavior change into stable versions. The question is: should 10.4 also have 0 by default or we can set it to some finite limit? MySQL's default value is 10. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

3 2

Re: [Maria-developers] d1ccc60: MDEV-14005 Remove need for Partition Key to be part of Primary Key.
by Sergei Golubchik 05 Feb '19

05 Feb '19

Hi, Alexey! On Feb 04, Alexey Botchkov wrote: > Hi, Sergei. > > Below are my reply on your comments. > I think it's better get these agreed before I produce the new patch. > > > > + if ((sav_inited= f->inited) == RND) > > > + f->ha_rnd_end(); > > > > When can this happen? > UPDATE statements do the rnd_init() in order to access through rnd_pos, > and when they need to do the table scan. So, UPDATE does rnd_init(), then rnd_pos(), then update_row(), and at that point you need to look up the key in all partitions, so you need index_read()? Could you, please, add a comment here to explain this sequence of events? > > > +int ha_partition::check_uniques_insert(uchar *buf, > > > ... > > > + { > > > > can you use key_copy() here? > Difficult to use here key_copy() as it is. > Here i need to get the record from the arbitrary buffer, > while key_copy() gets it from the table->record[0]. > It seems weird that the key_copy() actually has the 'from_record > argument, but only uses it to check the null bit. May be, fix it? Meaning, adding 'offset' argument to Field::get_key_image(), I suppose. > > > @@ -283,6 +283,14 @@ class partition_info : public Sql_alloc > .. > > > + uint n_uniques_to_check; > > > + uchar *unique_key_buf[2]; > > > > Is it safe? Isn't partition_info shared between many handlers > > so that every handler can only use it read-only? > > it's not quite clear to me. it seems that every TABLE has its own > > partition_info. But cloned handlers all share the same partition_info. > > You're right, cloned handlers all share the same part_info. I didn't notice > this. > So i guess i should move the uniue_key_buf[2] to the class ha_partition. > Do you know BTW if we ever use cloned handlers to write/update? I don't know, sorry. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] f93af1f93e2: MDEV-12836 Avoid table rebuild when adding or removing of auto_increment settings
by Sergei Golubchik 05 Feb '19

05 Feb '19

Hi, Eugene! On Feb 04, Eugene Kosov wrote: > revision-id: f93af1f93e2 (mariadb-10.3.12-19-gf93af1f93e2) > parent(s): d4144c8e010 > author: Eugene Kosov <claprix(a)yandex.ru> > committer: Eugene Kosov <claprix(a)yandex.ru> > timestamp: 2019-01-23 19:32:02 +0300 > message: > > MDEV-12836 Avoid table rebuild when adding or removing of auto_increment settings > > ALTER_AUTO_INC: this flag indicates that ALTER operation > adds or removes AUTO_INCREMENT from some field > > Field::is_equal(): now sometime returns IS_EQUAL_BUT_AUTO_INC > which means that only AUTO_INCREMENT was changed for that field You enable instant alter for adding auto_inc. This is not what Valerii asked for in MDEV-12836, and for a reason. Consider this test case: create table t1 (a int unique); insert t1 values (NULL),(NULL),(NULL); alter table t1 modify a int auto_increment; here `alter table` modifies the data, actually generating auto-inc values for all NULL columns. I don't think you can do that instantly. You can only remove auto-inc instantly. And for that you don't need any big changes, just modify Field::is_equal() to allow it: diff --git a/sql/field.cc b/sql/field.cc --- a/sql/field.cc +++ b/sql/field.cc @@ -3507,7 +3507,7 @@ uint Field_new_decimal::is_equal(Create_field *new_field) return ((new_field->type_handler() == type_handler()) && ((new_field->flags & UNSIGNED_FLAG) == (uint) (flags & UNSIGNED_FLAG)) && - ((new_field->flags & AUTO_INCREMENT_FLAG) == + ((new_field->flags & AUTO_INCREMENT_FLAG) <= (uint) (flags & AUTO_INCREMENT_FLAG)) && (new_field->length == max_display_length()) && (new_field->decimals == dec)); @@ -9508,7 +9508,7 @@ uint Field_num::is_equal(Create_field *new_field) return ((new_field->type_handler() == type_handler()) && ((new_field->flags & UNSIGNED_FLAG) == (uint) (flags & UNSIGNED_FLAG)) && - ((new_field->flags & AUTO_INCREMENT_FLAG) == + ((new_field->flags & AUTO_INCREMENT_FLAG) <= (uint) (flags & AUTO_INCREMENT_FLAG)) && (new_field->pack_length == pack_length())); } And this should be enough. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] c39f74ce0d9: MDEV-16974 Application period tables: UPDATE
by Sergei Golubchik 05 Feb '19

05 Feb '19

Hi, Nikita! See comments below On Jan 06, Nikita Malyavin wrote: > revision-id: c39f74ce0d9 (versioning-1.0.7-4-gc39f74ce0d9) > parent(s): fd18ed07d65 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-12-03 13:19:18 +1000 > message: > > MDEV-16974 Application period tables: UPDATE > > diff --git a/mysql-test/suite/period/r/delete.result b/mysql-test/suite/period/r/delete.result > index 30c9220d6f9..ccb8663bf72 100644 > --- a/mysql-test/suite/period/r/delete.result > +++ b/mysql-test/suite/period/r/delete.result > @@ -83,6 +83,9 @@ log > <INS: 3, 1997-01-01, 2000-01-01 > >INS: 4, 2018-01-01, 2020-01-01 > <INS: 4, 2018-01-01, 2020-01-01 > +# multi-table DELETE is prohibited > +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1; > +ERROR HY000: PORTION OF time is not allowed here why not ER_PARSE_ERROR? > 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 > delete from t for portion of othertime from '2000-01-01' to '2018-01-01'; > diff --git a/mysql-test/suite/period/r/update.result b/mysql-test/suite/period/r/update.result > new file mode 100644 > index 00000000000..1f59102c131 > --- /dev/null > +++ b/mysql-test/suite/period/r/update.result > @@ -0,0 +1,244 @@ > +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; > +update t for portion of apptime from '2000-01-01' to '2018-01-01' > + set id=id + 5; > +select * from t; same comment about unsorted selects > +id s e > +6 2000-01-01 2018-01-01 > +6 2000-01-01 2017-01-01 > +6 2017-01-01 2018-01-01 > +7 2000-01-01 2018-01-01 > +8 2000-01-01 2015-01-01 > +9 2016-01-01 2018-01-01 > +1 1999-01-01 2000-01-01 > +1 2018-01-01 2018-12-12 > +1 1999-01-01 2000-01-01 > +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 > +# Check triggers > +update t1 for portion of apptime from '2000-01-01' to '2018-01-01' > + set id=id + 5; > +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 > +6 2000-01-01 2017-01-01 > +6 2000-01-01 2018-01-01 > +6 2017-01-01 2018-01-01 > +7 2000-01-01 2018-01-01 > +8 2000-01-01 2015-01-01 > +9 2016-01-01 2018-01-01 > +select * from log_tbl; > +log > +>UPD: 1, 1999-01-01, 2018-12-12 -> 6, 2000-01-01, 2018-01-01 > +<UPD: 1, 1999-01-01, 2018-12-12 -> 6, 2000-01-01, 2018-01-01 > +>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 > +>UPD: 1, 1999-01-01, 2017-01-01 -> 6, 2000-01-01, 2017-01-01 > +<UPD: 1, 1999-01-01, 2017-01-01 -> 6, 2000-01-01, 2017-01-01 > +>INS: 1, 1999-01-01, 2000-01-01 > +<INS: 1, 1999-01-01, 2000-01-01 > +>UPD: 1, 2017-01-01, 2019-01-01 -> 6, 2017-01-01, 2018-01-01 > +<UPD: 1, 2017-01-01, 2019-01-01 -> 6, 2017-01-01, 2018-01-01 > +>INS: 1, 2018-01-01, 2019-01-01 > +<INS: 1, 2018-01-01, 2019-01-01 > +>UPD: 2, 1998-01-01, 2018-12-12 -> 7, 2000-01-01, 2018-01-01 > +<UPD: 2, 1998-01-01, 2018-12-12 -> 7, 2000-01-01, 2018-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 > +>UPD: 3, 1997-01-01, 2015-01-01 -> 8, 2000-01-01, 2015-01-01 > +<UPD: 3, 1997-01-01, 2015-01-01 -> 8, 2000-01-01, 2015-01-01 > +>INS: 3, 1997-01-01, 2000-01-01 > +<INS: 3, 1997-01-01, 2000-01-01 > +>UPD: 4, 2016-01-01, 2020-01-01 -> 9, 2016-01-01, 2018-01-01 > +<UPD: 4, 2016-01-01, 2020-01-01 -> 9, 2016-01-01, 2018-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 tr1upd_t2; > +drop trigger tr2upd_t2; > +update t2 for portion of apptime from '2000-01-01' to '2018-01-01' > + set id=id + 5; > +select * from t2 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 > +6 2000-01-01 2017-01-01 > +6 2000-01-01 2018-01-01 > +6 2017-01-01 2018-01-01 > +7 2000-01-01 2018-01-01 > +8 2000-01-01 2015-01-01 > +9 2016-01-01 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 > +select * from t for portion of apptime from 0 to 1 for system_time all; > +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 should be "right syntax to use near 'for portion of apptime from ...'" > +update t for portion of apptime from 0 to 1 for system_time all set id=1; > +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 'set id=1' at line 1 why not "right syntax to use near 'for system_time all ...'" ? > +# Modifying period start/end fields is forbidden. > +# SQL17: fix the reference, please (SQL:2016, Part 2, section ... paragraph ...) > +# Neither BSTARTCOL nor BENDCOL shall be an explicit <object column> > +# contained in the <set clause list>. > +update t for portion of apptime from '2000-01-01' to '2018-01-01' > + set id= id + 5, s=subdate(s, 5), e=adddate(e, 5); > +ERROR HY000: Column `s` used in period `apptime` specified in update SET list > +# Precision timestamps > +create or replace table t (id int, s timestamp(5), e timestamp(5), > +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'); > +update t for portion of apptime from '2000-01-01 00:00:00.00015' > + to '2018-01-01 12:34:56.31415' > + set id= id + 5; > +select * from t; > +id s e > +6 2000-01-01 00:00:00.00015 2018-01-01 12:34:56.31415 > +6 2000-01-01 00:00:00.00015 2017-01-01 00:00:00.00000 > +1 1999-01-01 00:00:00.00000 2000-01-01 00:00:00.00015 > +1 2018-01-01 12:34:56.31415 2018-12-12 00:00:00.00000 > +1 1999-01-01 00:00:00.00000 2000-01-01 00:00:00.00015 > +# Strings > +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'); > +update t for portion of apptime from '2000-01-01' to '2018-01-01' > + set id= id + 5; > +select * from t; > +id str s e > +6 data 2000-01-01 2018-01-01 > +6 other data 2000-01-01 2018-01-01 > +1 data 1999-01-01 2000-01-01 > +1 data 2018-01-01 2018-12-12 > +1 other data 1999-01-01 2000-01-01 > +1 other data 2018-01-01 2018-12-12 > +# multi-table UPDATE is prohibited > +create or replace table t1(x int); > +update t for portion of apptime from '2000-01-01' to '2018-01-01', t1 > +set t.id= t.id + 5; > +ERROR HY000: PORTION OF time is not allowed here why not ER_PARSE_ERROR? > +update t1 set x= (select id from t for portion of apptime from '2000-01-01' to '2018-01-01'); > +ERROR HY000: PORTION OF time is not allowed here why not ER_PARSE_ERROR? > +# SQL17: fix the reference, please > +# Let FROMVAL be <point in time 1>. FROMVAL shall not generally contain a > +# reference to a column of T or a <routine invocation> > +# whose subject routine is an SQL-invoked routine that > +# is possibly non-deterministic or that possibly modifies SQL-data. > +# ...Same for <point in time 2> (TOVAL) > +update t for portion of apptime from 5*(5+s) to 1 set t.id= t.id + 5; > +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions > +update t for portion of apptime from 1 to e set t.id= t.id + 5; > +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions > +set @s= '2000-01-01'; > +set @e= '2018-01-01'; > +create or replace function f() returns date return @e; > +create or replace function g() returns date not deterministic return @e; > +create or replace function h() returns date deterministic return @e; > +update t for portion of apptime from @s to f() set t.id= t.id + 5; > +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions > +update t for portion of apptime from @s to g() set t.id= t.id + 5; > +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions > +# success > +update t for portion of apptime from @s to h() set t.id= t.id + 5; > +# select value is cached > +update t for portion of apptime from (select s from t2 limit 1) to h() set t.id= t.id + 5; > +# auto_inrement field is updated > +create or replace table t (id int primary key auto_increment, x int, > +s date, e date, period for apptime(s, e)); > +insert into t values (default, 1, '1999-01-01', '2018-12-12'); > +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= x + 5; > +select * from t; > +id x s e > +1 6 2000-01-01 2018-01-01 > +2 1 1999-01-01 2000-01-01 > +3 1 2018-01-01 2018-12-12 > +truncate t; > +insert into t values (default, 1, '1999-01-01', '2018-12-12'); > +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= 1; > +select * from t; > +id x s e > +1 1 2000-01-01 2018-01-01 > +2 1 1999-01-01 2000-01-01 > +3 1 2018-01-01 2018-12-12 > +# generated columns are updated > +create or replace table t (x int, s date, e date, > +xs date as (s) stored, xe date as (e) stored, > +period for apptime(s, e)); > +insert into t values(1, '1999-01-01', '2018-12-12', default, default); > +select * from t; > +x s e xs xe > +1 1999-01-01 2018-12-12 1999-01-01 2018-12-12 > +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= x + 5; > +select *, xs=s and xe=e from t; > +x s e xs xe xs=s and xe=e > +6 2000-01-01 2018-01-01 2000-01-01 2018-01-01 1 > +1 1999-01-01 2000-01-01 1999-01-01 2000-01-01 1 > +1 2018-01-01 2018-12-12 2018-01-01 2018-12-12 1 > +# system_time columns are updated > +create or replace table t (x int, s date, e date, > +row_start SYS_TYPE as row start invisible, > +row_end SYS_TYPE as row end invisible, a bit strange to use invisible in tests for strict standard compliance :) but ok, whatever > +period for apptime(s, e), > +period for system_time(row_start, row_end)) with system versioning; > +insert into t values(1, '1999-01-01', '2018-12-12'), > +(2, '1999-01-01', '1999-12-12'); > +select row_start into @ins_time from t limit 1; > +select * from t order by x, s, e; > +x s e > +1 1999-01-01 2018-12-12 > +2 1999-01-01 1999-12-12 > +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= x + 5; > +select *, if(row_start = @ins_time, "OLD", "NEW"), check_row(row_start, row_end) > +from t for system_time all > +order by x, s, e, row_start; > +x s e if(row_start = @ins_time, "OLD", "NEW") check_row(row_start, row_end) > +1 1999-01-01 2000-01-01 NEW CURRENT ROW > +1 1999-01-01 2018-12-12 OLD HISTORICAL ROW > +1 2018-01-01 2018-12-12 NEW CURRENT ROW > +2 1999-01-01 1999-12-12 OLD CURRENT ROW > +6 2000-01-01 2018-01-01 NEW CURRENT ROW > +create or replace database test; > diff --git a/mysql-test/suite/period/t/delete.test b/mysql-test/suite/period/t/delete.test > index b34de6c37e7..39a68959523 100644 > --- a/mysql-test/suite/period/t/delete.test > +++ b/mysql-test/suite/period/t/delete.test > @@ -34,6 +34,10 @@ drop trigger tr2del_t2; > delete from t2 for portion of APPTIME from '2000-01-01' to '2018-01-01'; > select * from log_tbl; > > +--echo # multi-table DELETE is prohibited > +--error ER_PORTION_OF_TIME_NOT_ALLOWED > +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1; move this into your MDEV-16973 commit is possible, please, > + > --error ER_PARSE_ERROR > delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; > > diff --git a/mysql-test/suite/period/t/update.opt b/mysql-test/suite/period/t/update.opt > new file mode 100644 > index 00000000000..92a881bef67 > --- /dev/null > +++ b/mysql-test/suite/period/t/update.opt > @@ -0,0 +1 @@ > +--explicit_defaults_for_timestamp Why? it's best to avoid non-default command line options, unless you actually test this particular option. If you just need it for convenience - don't. every non-default command line options means a server restart, and they're slow. write DEFAULT NULL explicitly when needed. > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index 7ceff1a215e..c5a359b09e8 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7951,3 +7951,12 @@ ER_PERIOD_NOT_FOUND > > ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED > eng "period SYSTEM_TIME is not allowed here" > + > +ER_PORTION_OF_TIME_NOT_ALLOWED > + eng "PORTION OF time is not allowed here" > + > +ER_PERIOD_PORTION_OF_TIME_CONSTANT > + eng "Values in range FOR PORTION OF %`s should be constant expressions" Reuse ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR please. Like ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR -> ER_NOT_CONSTANT_EXPRESSION eng "Expression in RANGE/LIST VALUES must be constant" -> eng "Expression in %s must be constant" and in mysql.h #define ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR ER_NOT_CONSTANT_EXPRESSION > + > +ER_PERIOD_COLUMNS_UPDATED > + eng "Column %`s used in period %`s specified in update SET list" > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 7a18ac82236..93d2f417a50 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -1343,13 +1343,16 @@ int Lex_input_stream::lex_token(YYSTYPE *yylval, THD *thd) > /* > * Additional look-ahead to resolve doubtful cases like: > * SELECT ... FOR UPDATE > - * SELECT ... FOR SYSTEM_TIME ... . > + * SELECT ... FOR SYSTEM_TIME ... > + * SELECT ... FOR PORTION OF ... . Can you show an example of the statement where you need a second look-ahead to distinguish between FOR PORTION OF and FOR SYSTEM_TIME or FOR UPDATE ? > */ > token= lex_one_token(yylval, thd); > add_digest_token(token, yylval); > switch(token) { > case SYSTEM_TIME_SYM: > return FOR_SYSTEM_TIME_SYM; > + case PORTION: > + return FOR_PORTION_SYM; > default: > /* > Save the token following 'FOR_SYM' > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 220fcf6cc34..44989b20fae 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8121,6 +8121,12 @@ int setup_conds(THD *thd, TABLE_LIST *tables, List<TABLE_LIST> &leaves, > > for (table= tables; table; table= table->next_local) > { > + if (table->has_period() && !thd->lex->portion_of_time_applicable()) > + { > + my_error(ER_PORTION_OF_TIME_NOT_ALLOWED, MYF(0)); > + goto err_no_arena; > + } I don't understand that either, why do you need to check the correct syntax in setup_conds() ? > + > if (select_lex == thd->lex->first_select_lex() && > select_lex->first_cond_optimization && > table->merged_for_insert && > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc > index e76eb729536..4726557d762 100644 > --- a/sql/sql_delete.cc > +++ b/sql/sql_delete.cc > @@ -752,6 +752,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, > delete_record= true; > } > > + /* SQL standard defines DELETE FOR PORTTION OF time as 0-2 INSERTS + DELETE > + * We can substitute INSERT+DELETE with one UPDATE, but only if there are > + * no triggers set. It is also meaningless for system-versioned table > + */ 1. format multi-line comments like everywhere in the code, please 2. every time you refer to the standard, say where the standard says that 3. move this comment in your MDEV-16973 commit, if possible > portion_of_time_through_update= !has_period_triggers > && !table->versioned(VERS_TIMESTAMP); > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 0c368b40836..10535326ce5 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -11910,12 +11911,13 @@ join_table_parens: > > > table_primary_ident: > - table_ident opt_use_partition opt_for_system_time_clause > + table_ident opt_use_partition > + opt_for_portion_of_time_clause opt_for_system_time_clause Oh, no. FOR PORTION OF has very few well defined places where the standard allows it. Don't put it into the generic used-everywhere table_primary_ident. That's why you needed FOR_PORTION_SYM and a second look-ahead. > opt_table_alias_clause opt_key_definition > { > SELECT_LEX *sel= Select; > sel->table_join_options= 0; > - if (!($$= Select->add_table_to_list(thd, $1, $4, > + if (!($$= Select->add_table_to_list(thd, $1, $5, > Select->get_table_join_options(), > YYPS->m_lock_type, > YYPS->m_mdl_type, > diff --git a/sql/table.h b/sql/table.h > index 1b897c1b4c6..f104f4fc99c 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -2420,7 +2423,7 @@ struct TABLE_LIST > > bool has_period() const > { > - return period_conditions.name; > + return period_conditions.is_set(); why? > } > > /** > > diff --git a/sql/table.cc b/sql/table.cc > index 8f1c94dd633..93908a9e2e3 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -6456,7 +6456,7 @@ void TABLE::mark_columns_needed_for_delete(bool with_period) > retrieve the row again. > */ > > -void TABLE::mark_columns_needed_for_update() > +void TABLE::mark_columns_needed_for_update(bool with_period) just like in the delete case, I think this is the caller's problem > { > DBUG_ENTER("TABLE::mark_columns_needed_for_update"); > bool need_signal= false; > @@ -7870,19 +7871,41 @@ static int period_make_insert(TABLE *table, Item *src, Field *dst) > } > > +int TABLE::cut_fields_for_portion_of_time(THD *thd, const vers_select_conds_t &period_conds) looks like something that belongs to sql_update.cc, not TABLE > +{ > + 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); > + > + Field *start_field= field[s->period.start_fieldno]; > + Field *end_field= field[s->period.end_fieldno]; > + > + int res= 0; > + if (lcond && !start_field->has_explicit_value()) > + res= period_conds.start.item->save_in_field(start_field, true); > + > + if (likely(!res) && rcond && !end_field->has_explicit_value()) > + res= period_conds.end.item->save_in_field(end_field, true); > + > + return res; > +} > + > -int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, > +int TABLE::update_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, > bool *inside_period) looks like something that belongs to sql_delete.cc, not TABLE > { > bool lcond= period_conds.field_start->val_datetime_packed(thd) > @@ -7917,7 +7940,8 @@ int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, > return res; > } > > -int TABLE::insert_portion_of_time(THD *thd, vers_select_conds_t &period_conds) > +int TABLE::insert_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, > + ha_rows &rows_inserted) pointers or _constant_ references. Here, rows_inserted must be a pointer. > { > bool lcond= period_conds.field_start->val_datetime_packed(thd) > < period_conds.start.item->val_datetime_packed(thd); > diff --git a/sql/sql_update.cc b/sql/sql_update.cc > index 27bd6f04670..d5240f5524e 100644 > --- a/sql/sql_update.cc > +++ b/sql/sql_update.cc > @@ -177,6 +178,21 @@ static bool check_fields(THD *thd, List<Item> &items, bool update_view) > f->set_has_explicit_value(); > } > } > + > + if (thd->lex->sql_command == SQLCOM_UPDATE && table->has_period()) what else can sql_command be here? SQLCOM_UPDATE_MULTI? SQLCOM_UPDATE_MULTI cannot have table->has_period(). > + { > + for (List_iterator_fast<Item> it(items); (item=it++);) > + { > + Lex_ident name(item->name); > + vers_select_conds_t &period= table->period_conditions; > + if (name.streq(period.field_start->name) > + || name.streq(period.field_end->name)) Don't compare names, compare fields. The field to be updated is, I guess, item->field_for_view_update()->field > + { > + my_error(ER_PERIOD_COLUMNS_UPDATED, MYF(0), name.str, period.name.str); > + return true; > + } > + } > + } > return FALSE; > } > > @@ -323,6 +339,7 @@ int mysql_update(THD *thd, > List<Item> all_fields; > killed_state killed_status= NOT_KILLED; > bool has_triggers, binlog_is_row, do_direct_update= FALSE; > + bool has_period_triggers= false; same as for sql_delete.cc, one has_triggers is enough, second has_period_triggers is quite confusing. > Update_plan query_plan(thd->mem_root); > Explain_update *explain; > TABLE_LIST *update_source_table; > @@ -880,14 +905,25 @@ int mysql_update(THD *thd, > explain->tracker.on_record_after_where(); > store_record(table,record[1]); > > + if (table_list->has_period()) > + table->cut_fields_for_portion_of_time(thd, table_list->period_conditions); 1. this should be done after BEFORE triggers (see SQL:2016, part 2, 15.13 "Effect of replacing rows in base tables") 2. why in cut_fields_for_portion_of_time() you check for has_explicit_value()? fields cannot have explicit values there, you've checked that in check_fields(). > + > if (fill_record_n_invoke_before_triggers(thd, table, fields, values, 0, > TRG_EVENT_UPDATE)) > break; /* purecov: inspected */ > > found++; > > - if (!can_compare_record || compare_record(table)) > + bool record_was_same= false; > + bool need_update= !can_compare_record || compare_record(table) || > + thd->lex->sql_command == SQLCOM_DELETE; why sql_command would be SQLCOM_DELETE here? > + > + if (need_update) > { > + if (table->versioned(VERS_TIMESTAMP) && > + thd->lex->sql_command == SQLCOM_DELETE) > + table->vers_update_end(); > + > if (table->default_field && table->update_default_fields(1, ignore)) > { > error= 1; > @@ -994,6 +1032,13 @@ int mysql_update(THD *thd, > break; > } > > + if (need_update && !record_was_same && table_list->has_period()) I suspect this should happen even if record_was_same. The standard never says "if new values are the same as old values, don't update anything" > + { > + restore_record(table, record[1]); > + table->insert_portion_of_time(thd, table_list->period_conditions, > + rows_inserted); > + } > + > if (!--limit && using_limit) > { > /* Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 4

Re: [Maria-developers] 83444b747f1: MDEV-16975 Application period tables: ALTER TABLE
by Sergei Golubchik 05 Feb '19

05 Feb '19

Hi, Nikita! Please, see my comments below On Dec 28, Nikita Malyavin wrote: > revision-id: 83444b747f1 (versioning-1.0.7-5-g83444b747f1) > parent(s): c39f74ce0d9 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-12-03 13:19:18 +1000 > message: > > MDEV-16975 Application period tables: ALTER TABLE > > * implicit period constraint is hidden and cannot be dropped independently > * create...like and create...select support > > diff --git a/mysql-test/suite/period/r/alter.result b/mysql-test/suite/period/r/alter.result > new file mode 100644 > index 00000000000..6bb9f905358 > --- /dev/null > +++ b/mysql-test/suite/period/r/alter.result > @@ -0,0 +1,101 @@ > +set @s= '1992-01-01'; > +set @e= '1999-12-31'; > +create or replace table t (s date, e date); > +alter table t add period for a(s, e); > +ERROR HY000: Period field `s` cannot be NULL > +alter table t change s s date, add period for a(s, e); > +ERROR HY000: Period field `s` cannot be NULL > +alter table t change s s date, change e e date, add period for a(s, e); > +ERROR HY000: Period field `s` cannot be NULL > +alter table t change s s date not null, change e e date not null, > +add period for a(s, e); > +show create table t; > +Table Create Table > +t CREATE TABLE `t` ( > + `s` date NOT NULL, > + `e` date NOT NULL, > + PERIOD FOR `a` (`s`, `e`) > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +alter table t add id int; > +alter table t drop id; > +insert t values(@e, @s); > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t` Hmm. The standard, indeed, says "there's an implicit constraint". But it does not say "CHECK contraint". UNIQUE is also a constraint, Foreign key is also a constraint. So, I think here it means really just *a* constraint (a limitation, in other words), meaning only that start value must be before the end value. It doesn't mean that a CHECK constraint must be created. In other words, it should not be a CHECK constraint, it should not produce a `CONSTRAINT xxx failed` message, and, of course, it should never ever be shown as a CHECK constraint. > +alter table t drop constraint constraint_1; > +ERROR HY000: Can't DROP CONSTRAINT `CONSTRAINT_1`. Use DROP PERIOD `a` for this > +alter table t drop period for a; > +# Constraint is dropped > +insert t values(@e, @s); > +alter table t drop period for a; > +ERROR 42000: Can't DROP PERIOD `a`; check that it exists > +alter table t add period for a(s, e), drop period for a; > +ERROR 42000: Can't DROP PERIOD `a`; check that it exists > +truncate t; > +alter table t add period for a(s, e); > +insert t values(@e, @s); > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t` > +alter table t add period for a(s, e), drop period for a; > +insert t values(@e, @s); > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t` > +alter table t add s1 date not null, add period for b(s1, e), drop period for a; > +show create table t; > +Table Create Table > +t CREATE TABLE `t` ( > + `s` date NOT NULL, > + `e` date NOT NULL, > + `s1` date NOT NULL, > + PERIOD FOR `b` (`s1`, `e`) > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +insert t(s, s1, e) values(@e, @s, @e); > +insert t(s, s1, e) values(@e, @e, @s); > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t` > +alter table t add constraint check (s < s1 and s1 < e); > +ERROR HY000: CONSTRAINT `(null)` uses fields from PERIOD `b` huh? looks like a null-pointer dereference with a defensive crash-safe printf. and why is it an error? > +create table t1 like t; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `s` date NOT NULL, > + `e` date NOT NULL, > + `s1` date NOT NULL, > + PERIOD FOR `b` (`s1`, `e`) > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +create table t2 (period for b(s,e)) select * from t; > +ERROR 23000: CONSTRAINT `CONSTRAINT_1` failed for `test`.`t2` > +create table t2 (period for b(s1,e)) select * from t; > +# SQL17 p.895 please, fix all the references to the standard to mention the part number, section number and paragraph number instead of the page number. > +# The declared type of BC1 shall be either DATE or a timestamp type > +# and shall be equivalent to the declared type of BC2. > +create or replace table t (s timestamp not null, e timestamp(6) not null); > +alter table t add period for a(s, e); > +ERROR HY000: PERIOD FOR `a` fields types mismatch > +# SQL17 p.895 > +# No column of T shall have a column name that is equivalent to ATPN. > +create or replace table t (a int, s date, e date); > +alter table t add period for a(s, e); > +ERROR HY000: Period and field `a` cannot have same name. > +# SQL17 p.895 > +# Neither BC1 nor BC2 shall be an identity column, a generated column, > +# a system-time period start column, or a system-time period end column. > +create or replace table t (id int primary key, > +s date, > +e date generated always as (s+1)); > +alter table t add period for a(s, e); > +ERROR HY000: Period field `s` cannot be NULL > +create or replace table t (id int primary key, > +s date, > +e date as (s+1) VIRTUAL); > +alter table t add period for a(s, e); > +ERROR HY000: Period field `s` cannot be NULL > +create or replace table t (id int primary key, s timestamp(6), e timestamp(6), > +st timestamp(6) as row start, > +en timestamp(6) as row end, > +period for system_time (st, en)) with system versioning; > +alter table t add period for a(s, en); > +ERROR HY000: Period field `en` cannot be ROW END > +# SQL17 p.895 > +# The table descriptor of T shall not include a period descriptor other > +# than a system-time period descriptor. > +alter table t add period for a(s, e); > +alter table t add period for b(s, e); > +ERROR HY000: cannot specify more than one application-time period > +create or replace database test; > diff --git a/mysql-test/suite/period/r/create.result b/mysql-test/suite/period/r/create.result > index 420eceb2e9a..8c06a0cc134 100644 > --- a/mysql-test/suite/period/r/create.result > +++ b/mysql-test/suite/period/r/create.result > @@ -51,7 +51,7 @@ ERROR HY000: cannot specify more than one application-time period > # No <column name> in any <column definition> shall be equivalent to PN. > create or replace table t (mytime int, s date, e date, > period for mytime(s,e)); > -ERROR HY000: Could not specify name `mytime` for field. It is already used by period. > +ERROR HY000: Period and field `mytime` cannot have same name. This is better. Although I'd just use an existing ER_DUP_FIELDNAME > # CD1 and CD2 both contain either an explicit or an implicit > # <column constraint definition> that specifies > # NOT NULL NOT DEFERRABLE ENFORCED. > diff --git a/sql/field.cc b/sql/field.cc > index d7214687e2d..e065d61e3a5 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -10545,6 +10545,8 @@ Column_definition::Column_definition(THD *thd, Field *old_field, > unireg_check= Field::TMYSQL_COMPRESSED; > compression_method_ptr= zlib_compression_method; > } > + if (orig_field->maybe_null()) > + flags|= EXPLICIT_NULL_FLAG; This shouldn't be needed, as I wrote in a CREATE TABLE review, NOT NULL should be implicit, not an error. > } > else > { > diff --git a/sql/sql_alter.h b/sql/sql_alter.h > index 108b98afdd7..d917793dd90 100644 > --- a/sql/sql_alter.h > +++ b/sql/sql_alter.h > @@ -95,6 +95,8 @@ class Alter_info > CHECK_CONSTRAINT_IF_NOT_EXISTS= 1 > }; > List<Virtual_column_info> check_constraint_list; > + List<Table_period_info> period_list; looks unused (in this patch, at least) > + > // Type of ALTER TABLE operation. > alter_table_operations flags; > ulong partition_flags; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 4189131f801..211969647d4 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6152,6 +6152,11 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) > } > } > } > + else if (drop->type == Alter_drop::PERIOD) > + { > + if (table->s->period.name.streq(drop->name)) > + remove_drop= FALSE; you don't seem to have a single test for DROP PERIOD IF EXISTS > + } > else /* Alter_drop::KEY and Alter_drop::FOREIGN_KEY */ > { > uint n_key; > @@ -8406,6 +8412,34 @@ mysql_prepare_alter_table(THD *thd, TABLE *table, > } > } > > + if (table->s->period.name) > + { > + drop_it.rewind(); > + Alter_drop *drop; > + for (bool found= false; !found && (drop= drop_it++); ) > + { > + found= table->s->period.name.streq(drop->name); > + } > + > + if (drop) > + { > + drop_period= true; > + drop_it.remove(); > + } > + else if (create_info->period_info.is_set() && table->s->period.name) > + { > + my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application"); > + goto err; > + } > + else > + { > + Field *s= table->s->period.start_field(table->s); > + Field *e= table->s->period.end_field(table->s); > + create_info->period_info.set_period(s->field_name, e->field_name); > + create_info->period_info.name= table->s->period.name; why couldn't you remember Field's, why only names? is it because period_info is also used on CREATE TABLE and you don't have Field's there? > + } > + } > + > /* Add all table level constraints which are not in the drop list */ > if (table->s->table_check_constraints) > { > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 10535326ce5..545c8f831b9 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -8190,6 +8195,10 @@ alter_list_item: > { > Lex->alter_info.flags|= ALTER_ADD_PERIOD; > } > + | ADD period_for_application_time where's ADD PERIOD if_not_exists? > + { > + Lex->alter_info.flags|= ALTER_ADD_CHECK_CONSTRAINT; > + } > | add_column '(' create_field_list ')' > { > LEX *lex=Lex; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] ce91482: MDEV-17599 ALTER TABLE DROP CONSTRAINT does not work for foreign keys.
by Sergei Golubchik 04 Feb '19

04 Feb '19

Hi, Alexey! Looks good, one comment below Ok to push On Feb 04, Alexey Botchkov wrote: > revision-id: ce91482df1af6935f81a2142d0ee9391feab2454 (mariadb-10.2.21-45-gce91482) > parent(s): 97930df13c0e403940969ebb47398760b59f753c > committer: Alexey Botchkov > timestamp: 2019-02-04 12:57:57 +0400 > message: > > MDEV-17599 ALTER TABLE DROP CONSTRAINT does not work for foreign keys. > > The list of table constraints doesn't include foreign keys and uniques. > So we replace DROP CONSTRAINT with DROP [FOREIGN] KEY in this case. > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 1b426f8..bd6ab9e 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -8993,6 +8993,64 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name, > > THD_STAGE_INFO(thd, stage_setup); > > + > + if (alter_info->flags & Alter_info::ALTER_DROP_CHECK_CONSTRAINT) > + { > + /* > + ALTER TABLE DROP CONSTRAINT > + should be replaced with ... DROP FOREIGN KEY > + if the constraint is the FOREIGN KEY one. > + */ > + > + List_iterator<Alter_drop> drop_it(alter_info->drop_list); > + Alter_drop *drop; > + alter_info->flags&= ~Alter_info::ALTER_DROP_CHECK_CONSTRAINT; > + > + while ((drop= drop_it++)) > + { > + if (drop->type == Alter_drop::CHECK_CONSTRAINT) > + { > + { > + /* Test if there is a FOREIGN KEY with this name. */ > + List <FOREIGN_KEY_INFO> fk_child_key_list; > + FOREIGN_KEY_INFO *f_key; > + table->file->get_foreign_key_list(thd, &fk_child_key_list); move this out of the loop, perhaps? > + List_iterator<FOREIGN_KEY_INFO> fk_key_it(fk_child_key_list); > + > + while ((f_key= fk_key_it++)) > + { > + if (my_strcasecmp(system_charset_info, f_key->foreign_id->str, > + drop->name) == 0) > + { > + drop->type= Alter_drop::FOREIGN_KEY; > + alter_info->flags|= Alter_info::DROP_FOREIGN_KEY; > + goto do_continue; > + } > + } > + } > + > + { > + /* Test if there is an UNIQUE with this name. */ > + uint n_key; > + > + for (n_key=0; n_key < table->s->keys; n_key++) > + { > + if ((table->key_info[n_key].flags & HA_NOSAME) && > + my_strcasecmp(system_charset_info, > + drop->name, table->key_info[n_key].name) == 0) > + { > + drop->type= Alter_drop::KEY; > + alter_info->flags|= Alter_info::ALTER_DROP_INDEX; > + goto do_continue; > + } > + } > + } > + } > + alter_info->flags|= Alter_info::ALTER_DROP_CHECK_CONSTRAINT; > +do_continue:; > + } > + } > + > handle_if_exists_options(thd, table, alter_info); > > /* > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index b2b9cb4..cc6670b 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1741,7 +1741,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); > IDENT_sys TEXT_STRING_sys TEXT_STRING_literal > NCHAR_STRING opt_component key_cache_name > sp_opt_label BIN_NUM label_ident TEXT_STRING_filesystem ident_or_empty > - opt_constraint constraint opt_ident ident_table_alias > + opt_constraint opt_constraint_no_id constraint opt_ident ident_table_alias > > %type <lex_str_ptr> > opt_table_alias > @@ -6123,6 +6123,11 @@ check_constraint: > } > ; > > +opt_constraint_no_id: > + /* Empty */ {} > + | CONSTRAINT {} > + ; > + > opt_constraint: > /* empty */ { $$= null_lex_str; } > | constraint { $$= $1; } > @@ -7653,7 +7658,7 @@ alter_list_item: > lex->alter_info.drop_list.push_back(ad, thd->mem_root); > lex->alter_info.flags|= Alter_info::DROP_FOREIGN_KEY; > } > - | DROP PRIMARY_SYM KEY_SYM > + | DROP opt_constraint_no_id PRIMARY_SYM KEY_SYM > { > LEX *lex=Lex; > Alter_drop *ad= (new (thd->mem_root) > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 43296f4: MDEV-17599 ALTER TABLE DROP CONSTRAINT does not work for foreign keys.
by Sergei Golubchik 31 Jan '19

31 Jan '19

Hi, Alexey! On Jan 31, Alexey Botchkov wrote: > > can a CHECK constraint have the same name as an existing FK? > > Yes, it can. > So in this case the 'ALTER ... DROP CONSTRAINT c' drops the FOREIGN KEY. On > the second call this ALTER > removes the CHECK. > How do you like that? > I think it's ok as the very idea of same names for constraints seems sick. Let's handle it separately. Could you please report it as a bug? A table shouldn't have two constraints with the same name. > > what about UNIQUE constraint? > > The 'ALTER TABLE ADD CONSTRAINT UNIQUE unique_key' just adds UNIQUE > key. So i'm in doubt how we should handle the 'DROP CONSTRAINT > unique_key' statement. By dropping the unique key, why, is there another option? UNIQUE index is a constraint, not just an optimization helper, like a non-unique index. You already have a loop over all drop constraints, checking for unique shouldn't be a problem. It should drop PRIMARY KEY too, I suppose. We allow ADD CONSTRAINT PRIMARY KEY. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 43296f4: MDEV-17599 ALTER TABLE DROP CONSTRAINT does not work for foreign keys.
by Sergei Golubchik 31 Jan '19

31 Jan '19

Hi, Alexey! Few questions first: * can a CHECK constraint have the same name as an existing FK? * what about UNIQUE constraint? it's also a constraint. The standard says that DROP CONSTRAINT should work for uniques, and ALTER already supports ADD CONSTRAINT UNIQUE (and ADD CONSTRAINT FOREIGN KEY) Otherwise it's fine, a couple of comments see below. On Jan 31, Alexey Botchkov wrote: > revision-id: 43296f4a90cc9281b4ddad498bfab4a4e43131b9 (mariadb-10.2.21-45-g43296f4) > parent(s): 97930df13c0e403940969ebb47398760b59f753c > committer: Alexey Botchkov > timestamp: 2019-01-31 16:23:12 +0400 > message: > > MDEV-17599 ALTER TABLE DROP CONSTRAINT does not work for foreign keys. > > The list of table constraints doesn't include foreign keys. > So we replace DROP CONSTRAINT with DROP FOREIGN KEY in this case. > > diff --git a/mysql-test/t/alter_table.test b/mysql-test/t/alter_table.test > index df077c8..659cae1 100644 > --- a/mysql-test/t/alter_table.test > +++ b/mysql-test/t/alter_table.test > @@ -2013,5 +2013,16 @@ DROP TABLE t1; > > > --echo # > +--echo # MDEV-17599 ALTER TABLE DROP CONSTRAINT does not work for foreign keys. > +--echo # > + > +CREATE TABLE t1(id INT PRIMARY KEY, c1 INT) ENGINE= INNODB; > +CREATE TABLE t2(id INT PRIMARY KEY, c1 INT, CONSTRAINT sid FOREIGN KEY (`c1`) REFERENCES t1 (`id`) ON DELETE NO ACTION ON UPDATE NO ACTION) ENGINE= INNODB; > +SHOW CREATE TABLE t2; > +ALTER TABLE t2 DROP CONSTRAINT sid; please, add a test that drops FK and CHECK constraints in one statement. > +SHOW CREATE TABLE t2; > +DROP TABLE t2, t1; > + > +--echo # > --echo # End of 10.2 tests > --echo # > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 1b426f8..c080e5a 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -8993,6 +8993,44 @@ bool mysql_alter_table(THD *thd,char *new_db, char *new_name, > > THD_STAGE_INFO(thd, stage_setup); > > + > + if (alter_info->flags & Alter_info::ALTER_DROP_CHECK_CONSTRAINT) > + { > + /* > + ALTER TABLE DROP CONSTRAINT > + should be replaced with ... DROP FOREIGN KEY > + if the constraint is the FOREIGN KEY one. > + */ > + > + List_iterator<Alter_drop> drop_it(alter_info->drop_list); > + Alter_drop *drop; > + alter_info->flags&= ~Alter_info::ALTER_DROP_CHECK_CONSTRAINT; > + > + while ((drop= drop_it++)) > + { > + if (drop->type == Alter_drop::CHECK_CONSTRAINT) > + { > + List <FOREIGN_KEY_INFO> fk_child_key_list; > + FOREIGN_KEY_INFO *f_key; > + table->file->get_foreign_key_list(thd, &fk_child_key_list); better do this outside of the loop > + List_iterator<FOREIGN_KEY_INFO> fk_key_it(fk_child_key_list); > + > + while ((f_key= fk_key_it++)) > + { > + if (my_strcasecmp(system_charset_info, f_key->foreign_id->str, > + drop->name) == 0) > + { > + drop->type= Alter_drop::FOREIGN_KEY; > + alter_info->flags|= Alter_info::DROP_FOREIGN_KEY; > + goto do_continue; > + } > + } > + } > + alter_info->flags|= Alter_info::ALTER_DROP_CHECK_CONSTRAINT; > +do_continue:; > + } > + } > + > handle_if_exists_options(thd, table, alter_info); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0