[Maria-developers] Review for: MDEV-26938 Support descending indexes internally in InnoDB (server part)

[Sergey Petrunia]

Hi Serg,

There is some non-trivial input too, after all. Please find below.

> commit da2903f03de039693354176fda836e6642d6d0f0
> Author: Sergei Golubchik 
> Date:   Wed Nov 24 16:50:21 2021 +0100
> 
> MDEV-26938 Support descending indexes internally in InnoDB (server part)
> 
> * preserve DESC index property in the parser
> * store it in the frm (only for HA_KEY_ALG_BTREE)
> * read it from the frm
> * show it in SHOW CREATE
> * skip DESC indexes in opt_range.cc and opt_sum.cc
> * ORDER BY test
> 

== Trivial input ==

> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 2726b9a68c2..d6449eeca4c 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -685,6 +685,14 @@ static uint pack_keys(uchar *keybuff, uint key_count, 
> KEY *keyinfo,
>  DBUG_PRINT("loop", ("flags: %lu  key_parts: %d  key_part: %p",
>  key->flags, key->user_defined_key_parts,
>  key->key_part));
> +/* For SPATIAL, FULLTEXT and HASH indexes (anything other than B-tree),
> +   ignore the ASC/DESC attribute of columns. */
> +const uchar ha_reverse_sort= key->algorithm == HA_KEY_ALG_BTREE
> +  ? HA_REVERSE_SORT :
> +  (key->algorithm == HA_KEY_ALG_UNDEF &&
> +   !(key->flags & (HA_FULLTEXT | HA_SPATIAL)))
> +  ? HA_REVERSE_SORT : 0;

The above is hard to read for me. I think if-statement form would be more
readable:

 const uchar ha_reverse_sort= 0;
 if (key->algorithm == HA_KEY_ALG_BTREE || 
 (key->algorithm == HA_KEY_ALG_UNDEF && 
  !(key->flags & (HA_FULLTEXT | HA_SPATIAL))
   ha_reverse_sort= HA_REVERSE_SORT;


== Issue #1: ordered index scan is not handled in ha_partition ==

Testcase:

create table t10 (a int, b int, key(a desc)) partition by hash(a) partitions 4;
insert into t10 select seq, seq from seq_0_to_999;

MariaDB [test]> select * from t10 order by a limit 3;
+--+--+
| a| b|
+--+--+
|3 |3 |
|7 |7 |
|   11 |   11 |
+--+--+
3 rows in set (0.002 sec)

Correct output:

MariaDB [test]> select * from t10 use index() order by a limit 3;
+--+--+
| a| b|
+--+--+
|0 |0 |
|1 |1 |
|2 |2 |
+--+--+
3 rows in set (0.011 sec)

== Issue #2: extra warning ==

MariaDB [test]> create table t1 (a int, b int, key(a), key(a));
Query OK, 0 rows affected, 1 warning (0.016 sec)

MariaDB [test]> show warnings;
+---+--+--+
| Level | Code | Message
  |
+---+--+--+
| Note  | 1831 | Duplicate index `a_2`. This is deprecated and will be 
disallowed in a future release |
+---+--+--+
   
1 row in set (0.000 sec)
  
 
This is ok.


MariaDB [test]> create table t2 (a int, b int, key(a), key(a desc));
 
Query OK, 0 rows affected, 1 warning (0.004 sec)
  

  
MariaDB [test]> show warnings;
+---+--+--+
   
| Level | Code | Message
  |   
+---+--+--+
   
| Note  | 1831 | Duplicate index `a_2`. This is deprecated and will be 
disallowed in a future release |   
+---+--+--+
   
1 row in set (0.000 sec)
  
 
This is probably not? MySQL doesn't produce this warning in this case.

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT

[Aleksey Midenkov]

Hi, Sergei!

On Mon, Dec 6, 2021 at 11:22 PM Sergei Golubchik  wrote:
>
> Hi, Aleksey!
>
> On Dec 05, Aleksey Midenkov wrote:
> > > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > > > index ac22a63bdba..459edb614f7 100644
> > > > --- a/sql/sql_base.cc
> > > > +++ b/sql/sql_base.cc
> > > > @@ -4203,6 +,15 @@ bool open_tables(THD *thd, const DDL_options_st 
> > > > ,
> > > >  }
> > > >
> > > >thd->current_tablenr= 0;
> > > > +
> > > > +#ifdef WITH_PARTITION_STORAGE_ENGINE
> > > > +  if (!thd->stmt_arena->is_conventional())
> > > > +  {
> > > > +for (tables= *start; tables; tables= tables->next_global)
> > > > +  tables->vers_skip_create= false;
> > > > +  }
> > >
> > > This looks a bit like an overkill, you do it for every prepare,
> > > while auto-adding a partition is a rare event, as you said.
> > >
> > > May be you can do instead in TABLE::vers_switch_partition
> > >
> > >  uint *create_count= !thd->stmt_arena->is_conventional() &&
> > >  table_list->vers_skip_create ?
> >
> > No, this is inside backoff action loop. That will interfere with the
> > normal vers_skip_create algorithm (which also applies to PS, SP). The
> > goal of the above code is to reset vers_skip_create from the previous
> > statement execution.
>
> In that case you can use an auto-resetting value, like,
> if tables->vers_skip_create == thd->query_id it means, "skip".
> That is vers_skip_create must be of query_id_t. You set it with
> tables->vers_skip_create= thd->query_id;
> And on the next statement it automatically expires.
>
> This semantics is a bit more complex than boolean, so it'd need a
> comment, like
>
>   /*
>   Protect single thread from repeating partition auto-create over
>   multiple share instances (as the share is closed on backoff action).
>   Skips auto-create only for one given query id.
>   */
>   query_id_tvers_skip_create;

Done! Thanks for the tip.

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



-- 
@midenok

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp