[Maria-developers] Review for: MDEV-26938 Support descending indexes internally in InnoDB (server part)
Hi Serg, There is some non-trivial input too, after all. Please find below.
commit da2903f03de039693354176fda836e6642d6d0f0 Author: Sergei Golubchik <serg@mariadb.org> 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
Hi, Sergey! All fixed, thanks! On Dec 07, Sergey Petrunia wrote:
Hi Serg,
There is some non-trivial input too, after all. Please find below.
commit da2903f03de039693354176fda836e6642d6d0f0 Author: Sergei Golubchik <serg@mariadb.org> 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.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On Tue, Dec 07, 2021 at 07:01:45PM +0300, Sergey Petrunia wrote:
== Issue #1: ordered index scan is not handled in ha_partition ==
I've hit this again when trying to get range optmizer to work. Please find the patch below. It follows MySQL-8's approach.
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)
diff --git a/sql/key.cc b/sql/key.cc index f2cebfe6d82..c43d3c36d5d 100644 --- a/sql/key.cc +++ b/sql/key.cc @@ -495,6 +495,7 @@ int key_cmp(KEY_PART_INFO *key_part, const uchar *key, uint key_length) { int cmp; store_length= key_part->store_length; + int sort_order = (key_part->key_part_flag & HA_REVERSE_SORT) ? -1 : 1; if (key_part->null_bit) { /* This key part allows null values; NULL is lower than everything */ @@ -503,19 +504,19 @@ int key_cmp(KEY_PART_INFO *key_part, const uchar *key, uint key_length) { /* the range is expecting a null value */ if (!field_is_null) - return 1; // Found key is > range + return sort_order; // Found key is > range /* null -- exact match, go to next key part */ continue; } else if (field_is_null) - return -1; // NULL is less than any value + return -sort_order; // NULL is less than any value key++; // Skip null byte store_length--; } if ((cmp=key_part->field->key_cmp(key, key_part->length)) < 0) - return -1; + return -sort_order; if (cmp > 0) - return 1; + return sort_order; } return 0; // Keys are equal } @@ -574,6 +575,7 @@ int key_rec_cmp(void *key_p, uchar *first_rec, uchar *second_rec) do { field= key_part->field; + int sort_order = (key_part->key_part_flag & HA_REVERSE_SORT) ? -1 : 1; if (key_part->null_bit) { @@ -593,12 +595,12 @@ int key_rec_cmp(void *key_p, uchar *first_rec, uchar *second_rec) ; /* Fall through, no NULL fields */ else { - DBUG_RETURN(+1); + DBUG_RETURN(sort_order); } } else if (!sec_is_null) { - DBUG_RETURN(-1); + DBUG_RETURN(-sort_order); } else goto next_loop; /* Both were NULL */ @@ -612,7 +614,7 @@ int key_rec_cmp(void *key_p, uchar *first_rec, uchar *second_rec) */ if ((result= field->cmp_prefix(field->ptr+first_diff, field->ptr+sec_diff, key_part->length))) - DBUG_RETURN(result); + DBUG_RETURN(result * sort_order); next_loop: key_part++; key_part_num++; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
Hi, Sergey! Yes, that's basically how I fixed it. But only in key_rec_cmp(), key_cmp() changes somehow weren't needed. Perhaps they'll be needed for range optimizer though. Please, use preview-10.8-MDEV-26938-desc-indexes for your range optimizer work. On Dec 11, Sergey Petrunia wrote:
On Tue, Dec 07, 2021 at 07:01:45PM +0300, Sergey Petrunia wrote:
== Issue #1: ordered index scan is not handled in ha_partition ==
I've hit this again when trying to get range optmizer to work. Please find the patch below. It follows MySQL-8's approach.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Sergei Golubchik -
Sergey Petrunia