Re: [Maria-developers] f6269a85ad7: MDEV-17553 Enable setting start datetime for interval partitioned history of system versioned tables
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 4114Versioned table `test`.`t1`: partition `p1` is
> full, add more HISTORY partitions
> > +Warning 4114Versioned 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 4114Versioned table `test`.`t1`: partition `p1` is
> full, add more HISTORY partitions
> > +Warning 4114Versioned 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 typepossible_keys
> key key_len ref rowsExtra
> > 1SIMPLE t1 p1_p1sp0,p1_p1sp1 # NULLNULL
> NULLNULL# #
> > -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
>
Re: [Maria-developers] f6269a85ad7: MDEV-17553 Enable setting start datetime for interval partitioned history of system versioned tables
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"
> * 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.
> * 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
> 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?
> 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 4114Versioned table `test`.`t1`: partition `p1` is full,
> add more HISTORY partitions
> +Warning 4114Versioned 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?
> ### warn about full partition
> delete from t1;
> Warnings:
> -Warning 4114Versioned table `test`.`t1`: partition `p1` is full,
> add more HISTORY partitions
> +Warning 4114Versioned 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 typepossible_keys key
> key_len ref rowsExtra
> 1SIMPLE t1 p1_p1sp0,p1_p1sp1 # NULLNULLNULL
> 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.
> +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;
> +TableCreate 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
