maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Sep 26, Aleksey Midenkov wrote:
> revision-id: f6269a85ad7 (mariadb-10.4.7-38-gf6269a85ad7)
> parent(s): a9ca752f1a9
> author: Aleksey Midenkov <midenok@xxxxxxxxx>
> committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> 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	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?

>  ### 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.

> +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?

> +(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?

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

> 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?

> +{
> +  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 :(

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() */

> +  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 {

> +    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, &ltime, opt);
> +        vers_info->interval.start= TIME_to_timestamp(thd, &ltime, &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(&ltime, thd->query_start());
> +      if (date_add_interval(thd, &ltime, 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(&ltime, &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(&ltime, 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, &ltime, &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@xxxxxxxxxxx