Hi Sergei, On Thu, Sep 26, 2019 at 4:15 PM Sergei Golubchik wrote:

Hi, Aleksey!

On Sep 26, Aleksey Midenkov wrote:

...

revision-id: f6269a85ad7 (mariadb-10.4.7-38-gf6269a85ad7) parent(s): a9ca752f1a9 author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2019-08-19 11:58:56 +0300 message:

MDEV-17553 Enable setting start datetime for interval partitioned history of system versioned tables

* Interactive STARTS syntax

I wouldn't use the word "interactive" here, better say "Explicit STARTS syntax"

Fixed.

...

* SHOW CREATE * Default STARTS rounding depending on INTERVAL type

This is questionable. I don't see why it's much better than the old behavior. It kind of makes sense, but that's all. If it would've worked that way from the start, it would be fine. But now, I'm not sure it offers such important benefits to break the compatibility for it.

Well, I'd like to take my chance on that as it is more user friendly IMO. F.ex. if you rotate by DAY it is expected to rotate on 00:00 each day, not at some strange 13:49:52 or whatever the time was when the command was issued. The latter just doesn't make sense to me. The improvement doesn't break compatibility with existing tables as they store STARTS value explicitly.

...

* Warn when STARTS timestamp is further than INTERVAL

[Closes tempesta-tech/mariadb#574]

=== Dependency hints (auto-detected by git-deps) === 336c0139a89 MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables ^^^ remove these dependency hints before pushing, please

Of course I remove them always.

diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result

...

index 9e532824414..3606c4e407f 100644 --- a/mysql-test/suite/versioning/r/partition.result +++ b/mysql-test/suite/versioning/r/partition.result @@ -1,3 +1,4 @@ +call mtr.add_suppression("need more HISTORY partitions");

why these warnings suddenly started to show up in the error log?

Actually, they were shown in the error log before: 2019-10-31 11:51:37 4 [Warning] mysqld: Versioned table `test`.`t1`: partition `p1` is full, add more HISTORY partitions 2019-10-31 11:51:37 4 [Warning] mysqld: Versioned table `test`.`t1`: partition `p1` is full, add more HISTORY partitions But I couldn't find how they were suppressed. Neither in original commit e851c140f4a4.

...

set system_versioning_alter_history=keep; # Check conventional partitioning on temporal tables create or replace table t1 ( @@ -266,11 +267,11 @@ x 6 insert into t1 values (7), (8); Warnings: -Warning 4114 Versioned table `test`.`t1`: partition `p1` is full, add more HISTORY partitions +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions

Hmm, "is out of LIMIT" sounds strange, I liked "is full" more. Is it that important to say LIMIT or INTERVAL here?

I like this message more because it is clear what clause causes the warning. OTOH just "full" characteristic is vague and is less helpful of why it is full. Moreover when it is out of INTERVAL, "full" just sounds strange.

...

### warn about full partition delete from t1; Warnings: -Warning 4114 Versioned table `test`.`t1`: partition `p1` is full, add more HISTORY partitions +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions select * from t1 partition (p1) order by x; x 4 diff --git a/mysql-test/suite/versioning/r/partition_rotation.result b/mysql-test/suite/versioning/r/partition_rotation.result index 69b30a56bd6..f6db36b117b 100644 --- a/mysql-test/suite/versioning/r/partition_rotation.result +++ b/mysql-test/suite/versioning/r/partition_rotation.result @@ -55,4 +57,265 @@ i explain partitions select * from t1 for system_time all where row_end = @ts; id select_type table partitions type possible_keys key key_len ref rows Extra 1 SIMPLE t1 p1_p1sp0,p1_p1sp1 # NULL NULL NULL NULL # # -drop table t1; +## INTERVAL ... STARTS +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts 'a' +(partition p0 history, partition pn current); +ERROR HY000: Wrong parameters for partitioned `t1`: wrong value for 'STARTS' +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts '00:00:00' +(partition p0 history, partition pn current); +ERROR HY000: Wrong parameters for partitioned `t1`: wrong value for 'STARTS'

There are clear and well defined rules how to cast TIME to TIMESTAMP, so we can allow that. But it might be confusing, so if you'd like, let's keep it your way and lift this restriction later if needed.

Ok.

...

+create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts '2000-00-01 00:00:00' +(partition p0 history, partition pn current); +ERROR HY000: Wrong parameters for partitioned `t1`: wrong value for 'STARTS' +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts '2000-01-01 00:00:00' +(partition p0 history, partition pn current); +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `i` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 DAY STARTS TIMESTAMP'2000-01-01 00:00:00' +(PARTITION `p0` HISTORY ENGINE = MyISAM, + PARTITION `pn` CURRENT ENGINE = MyISAM) +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts 946684800

Now, this is very questionable. There is a well defined rule how to cast numbers to temporal values, and this isn't it. If you want to allow numbers, it has to be

partition by system_time interval 1 day starts 20000101000000

Whatever the server is doing internally when parsing frms isn't really relevant for the end user, is it?

I really don't like different grammar for internal/non-internal. But for now I'm disabling numeric type for the external.

...

+(partition p0 history, partition pn current); +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `i` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 DAY STARTS TIMESTAMP'2000-01-01 00:00:00' +(PARTITION `p0` HISTORY ENGINE = MyISAM, + PARTITION `pn` CURRENT ENGINE = MyISAM) +# Test STARTS warning +set timestamp= unix_timestamp('2000-01-01 00:00:00'); +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day +(partition p0 history, partition pn current); +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `i` int(11) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING + PARTITION BY SYSTEM_TIME INTERVAL 1 DAY STARTS TIMESTAMP'2000-01-01 00:00:00' +(PARTITION `p0` HISTORY ENGINE = MyISAM, + PARTITION `pn` CURRENT ENGINE = MyISAM) +# no warning +create or replace table t1 (i int) with system versioning +partition by system_time interval 1 day starts '2000-01-02 00:00:00'

Why no warning here? If I insert and delete a row right now, it'll happen at 2000-01-01 00:00:00. So, it'll be before STARTS, what partition it should go into?

Of course, everything that goes before STARTS gets into first partition and this will lead to first partition overflow. Fixed the warning message.

As far as I understand, STARTS normally must be before NOW() not before NOW()+INTERVAL

True.

...

diff --git a/sql/partition_info.cc b/sql/partition_info.cc index ba6fc8a49ec..119c00dfce6 100644 --- a/sql/partition_info.cc +++ b/sql/partition_info.cc @@ -2381,6 +2383,111 @@ static bool strcmp_null(const char *a, const char *b) return true; }

+/** + Assign INTERVAL and STARTS for SYSTEM_TIME partitions. + + @return true on error +*/ + +bool partition_info::vers_set_interval(THD* thd, Item* interval, + interval_type int_type, Item* starts, + const char *table_name)

why do you pass the table name as an argument now?

When we have no TABLE object (mysql_parse()) we need anyway to return error and warning messages.

+{

...

+ DBUG_ASSERT(part_type == VERSIONING_PARTITION); + + const bool interactive= !table;

what does that mean?

* first, SQL is not interactive, using this adjective is confusing, * second you didn't write if(!table) because you wanted to use a self-documenting variable name, I presume. This name isn't helping, it confuses even more :(

Well, I agree 'interactive' is not very clear term without comment. "inter-" means "between systems" or "between the system and the outside environment". The term "interactive" can be applied to anything depending on what you decide to be "the system" and what to be "the environment".

please, either use a really self-documenting name, or just write if(!table) with a comment.

... okay, now I see what you mean. Better use a comment:

if (!table) { /* called from mysql_unpack_partition() not from mysql_parse() */

Done.

...

+ MYSQL_TIME ltime; + uint err; + vers_info->interval.type= int_type; + + /* 1. assign INTERVAL to interval.step */ + if (interval->fix_fields_if_needed_for_scalar(thd, &interval)) + return true; + bool error= get_interval_value(thd, interval, int_type, &vers_info->interval.step) || + vers_info->interval.step.neg || vers_info->interval.step.second_part || + !(vers_info->interval.step.year || vers_info->interval.step.month || + vers_info->interval.step.day || vers_info->interval.step.hour || + vers_info->interval.step.minute || vers_info->interval.step.second); + if (error) {

indentation {

Fixed.

+ my_error(ER_PART_WRONG_VALUE, MYF(0), table_name, "INTERVAL");

...

+ return true; + } + + /* 2. assign STARTS to interval.start */ + if (starts) + {

I've already commented on the following, but here it is again:

...

+ if (starts->fix_fields_if_needed_for_scalar(thd, &starts)) + return true; + switch (starts->result_type()) + { + case INT_RESULT: + case DECIMAL_RESULT: + case REAL_RESULT: + if (starts->val_int() > TIMESTAMP_MAX_VALUE) + goto interval_starts_error; + vers_info->interval.start= (time_t) starts->val_int(); + break;

wrong conversion to temporal

...

+ case STRING_RESULT: + case TIME_RESULT: + { + Datetime::Options opt(TIME_NO_ZERO_DATE | TIME_NO_ZERO_IN_DATE, thd); + starts->get_date(thd, <ime, opt); + vers_info->interval.start= TIME_to_timestamp(thd, <ime, &err); + if (err) + goto interval_starts_error; + break; + } + case ROW_RESULT: + default: + goto interval_starts_error; + } + if (interactive) + { + my_tz_OFFSET0->gmt_sec_to_TIME(<ime, thd->query_start()); + if (date_add_interval(thd, <ime, int_type, vers_info->interval.step)) + return true;

I think this isn't needed

...

+ my_time_t boundary= my_tz_OFFSET0->TIME_to_gmt_sec(<ime, &err); + if (vers_info->interval.start > boundary) { + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, + ER_PART_STARTS_BEYOND_INTERVAL, + ER_THD(thd, ER_PART_STARTS_BEYOND_INTERVAL), + table_name); + } + } + } + else // calculate default STARTS depending on INTERVAL + { + thd->variables.time_zone->gmt_sec_to_TIME(<ime, thd->query_start()); + if (vers_info->interval.step.second) + goto interval_set_starts; + ltime.second= 0;

I think this isn't needed

...

+ if (vers_info->interval.step.minute) + goto interval_set_starts; + ltime.minute= 0; + if (vers_info->interval.step.hour) + goto interval_set_starts; + ltime.hour= 0; + if (vers_info->interval.step.day) + goto interval_set_starts; + ltime.day= 1; + if (vers_info->interval.step.month) + goto interval_set_starts; + ltime.month= 1; + DBUG_ASSERT(vers_info->interval.step.year); + +interval_set_starts: + vers_info->interval.start= TIME_to_timestamp(thd, <ime, &err); + if (err) + goto interval_starts_error; + } + + return false; + +interval_starts_error: + my_error(ER_PART_WRONG_VALUE, MYF(0), table_name, "STARTS"); + return true; +} +

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org

-- All the best, Aleksey Midenkov @midenok