maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei,

On Thu, Sep 26, 2019 at 4:15 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

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

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, &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
>


-- 
All the best,

Aleksey Midenkov
@midenok