MDEV-32732 Support DESC indexes in loose scan optimization

[mariadb-YuchenPei]

Extend loose index scan to support descending indexes.

This is achieved by removing a block skipping creating loose index
scan plan for descending index, as well as generalising the execution
of such plans.

The generalisation applies to all levels looking for min/max in loose
index scan. In the highest level (get_next), generalise min and max to
first and last, so that it still proceeds in the direction agreeing
with the index parity. In the lower levels, combine next_min and
next_max methods into next_min_max, and combine next_min_in_range and
next_max_in_range into next_min_max_in_range. This retains existing
logic of these functions and reduces code duplication, while allowing
handling of all four combinations (min, max) x (asc index, desc
index).

---

• The Jira issue number for this PR is: MDEV-32732

Description

See the commit message.

Addressing comments in the first round of review:

• Please look at this patch fixing the coding style
  [^mdev32732-fix-coding-style.diff] and apply if there are no
  objections

Applied.

• The commit comment needs to be more verbose. It should be a
  description of what the patch does and how.

Done.

• QUICK_GROUP_MIN_MAX_SELECT::skip_nulls() is misleading: it also does the part which has nothing to do with NULLs:
  " Apply the constant equality conditions to the non-group select fields"
  This needs to be either moved or of the function or the function
  needs to be renamed.

Done. Moved the call out into next_min_max(). Also added some tests
covering these index lookups before the call to skip_nulls().

• I do not quite understand what QUICK_GROUP_MIN_MAX_SELECT::next_min_max_in_range() tries to do with NULL_RANGE-s.
  AFAIU it shouldn't have gotten them, if it got them, it should ignore them - do not even look for rows in them.
  Currently it produces wrong results:

Fixed the handling of NULL_RANGE's, especially when looking for a MIN,
which the logic in next_min_in_range before the patch carries over.
More specifically, in case of MIN we consider ranges from the left to
the right, if the leftmost range is NULL_RANGE and a NULL is found,
then the NULL needs to be saved:

• if a key is found in the remaining ranges, then we use that key
• if no key is found in the remaining ranges, then we use the saved
  NULL

How can this PR be tested?

See tests in the patch

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch
• This is a bug fix and the PR is based against the earliest branch in which the bug can be reproduced

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[spetrunia]

This is the last piece of input I have. Will be ok to push after the testcases are fixed.

[spetrunia]

t doesn't look like a good idea to use this approach:

• We may not get test coverage in release builds.
• The .test file is made to produce the same result regardless of whether the optimization is used or not. If it gets disabled for some reason (in both release and debug builds), we will never know.

I might have said earlier that one can use DBUG-based code to force the optimizer to make a desired choice. I think it's still true when one or more of the following holds:

• it is impossible (or very difficult) to construct a testcase
• the testcase needs to be very large, too large for MTR.

Note that here we have neither:

• I have tried changing SELECTs to EXPLAINs and removing the SET DEBUG - it looks like all tests will pick loose scan anyway?
• If something doesn't pick loose scan, I bet one can easily get the loose scan to be picked by adding several big GROUP BY groups into the table and adding them into WHERE so that non-loose-scan becomes much more expensive while loose scan becomes just a bit more expensive.

Take away: like some other tests, please do

let $q=SELECT ....;
eval  $q;
eval EXPLAIN $q;

and remove the set debug="+d,force_group_by"; .
I don't expect it will cause a lot of issues.

AFAIU the tests are not using InnoDB. If they do, please use innodb_stable_estimates.inc and maybe add innodb_max_purge_lag_wait=0 there...

[mariadb-YuchenPei]

Thanks, done.

[mariadb-YuchenPei]

Patch sent for testing.

[spetrunia]

Now, looks good.