Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-36094 Row ID filtering for reverse-ordered scans #3846

Open
wants to merge 2 commits into
base: 12.0-mdev-34413-icp-reverse-scan
Choose a base branch
from

Conversation

DaveGosselin-MariaDB
Copy link
Member

@DaveGosselin-MariaDB DaveGosselin-MariaDB commented Feb 19, 2025

This PR has two commits:

Second commit:

The fix for MDEV-34413 added support for Index Condition Pushdown with reverse ordered scans. This makes Rowid filtering work with reverse-ordered scans, too, so enable it. For example, InnoDB can now check the pushed index condition and then check the rowid filter on success, in the ORDER BY ... DESC case.

First commit:

Add tests showing that RowID filtering is not enabled for reverse-ordered scans.

@DaveGosselin-MariaDB DaveGosselin-MariaDB self-assigned this Feb 19, 2025
@DaveGosselin-MariaDB DaveGosselin-MariaDB changed the title 12.0 mdev 36094 rowid reverse scan MDEV-36094 Row ID filtering for reverse-ordered scans Feb 19, 2025
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-34413-icp-reverse-scan branch 2 times, most recently from 027a361 to 6257ec9 Compare February 23, 2025 00:41
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-36094-rowid-reverse-scan branch from 8fbba54 to 53cd5a7 Compare February 24, 2025 18:36
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-34413-icp-reverse-scan branch from 6257ec9 to e7b539b Compare February 25, 2025 15:44
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-36094-rowid-reverse-scan branch from 53cd5a7 to e77efc4 Compare February 25, 2025 17:59
@spetrunia
Copy link
Member

The patch adds Handler_rowid_filter_attempts and ...matches counters. Is there any problem with looking at ANALYZE FORMAT=JSON output, instead?

I'm wondering if these counters would make sense for the end-users (i.e. non-database devs). Globals like "[index] rows read" seem to make sense, but I have a doubt whether global "rowid filter checks" is useful.

@DaveGosselin-MariaDB
Copy link
Member Author

The patch adds Handler_rowid_filter_attempts and ...matches counters. Is there any problem with looking at ANALYZE FORMAT=JSON output, instead?

I'm wondering if these counters would make sense for the end-users (i.e. non-database devs). Globals like "[index] rows read" seem to make sense, but I have a doubt whether global "rowid filter checks" is useful.

I'm not sure what you mean by "The patch adds Handler_rowid_filter_attempts and ...matches counters"; to clarify, the first commit shows that the new counters are zero, then the second shows that they're nonzero, reflecting that the changes to enable rowid filtering for reverse scans is in effect.

I'm happy to remove the counters and look at analyze format=json output using json_extract instead, especially if you think that the counters are not useful. I don't want to add something that you think isn't needed. 😊

@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-34413-icp-reverse-scan branch 2 times, most recently from dbbae24 to 3ebfc68 Compare February 28, 2025 15:40
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-36094-rowid-reverse-scan branch from e77efc4 to b14730f Compare February 28, 2025 18:48
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-34413-icp-reverse-scan branch from 3ebfc68 to 17b715e Compare March 3, 2025 13:50
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-36094-rowid-reverse-scan branch from b14730f to 838f4dc Compare March 3, 2025 21:22
Add tests showing that RowID filtering is not enabled for
reverse-ordered scans.
The fix for MDEV-34413 added support for Index Condition Pushdown with reverse
ordered scans.  This makes Rowid filtering work with reverse-ordered scans, too,
so enable it.  For example, InnoDB can now check the pushed index condition and
then check the rowid filter on success, in the ORDER BY ... DESC case.
@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 12.0-mdev-36094-rowid-reverse-scan branch from 838f4dc to 1ceb42e Compare March 3, 2025 21:27
Copy link
Member

@spetrunia spetrunia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need attion on the above comments.

@@ -3778,16 +3778,30 @@ ANALYZE
"key_length": "5",
"used_key_parts": ["a"],
"ref": ["const"],
"rowid_filter": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that a few lines above, on line 3755, there is a note:

# The following must NOT use Rowid Filter:

It needs to be updated.

"loops": 1,
"r_loops": 1,
"rows": 250,
"r_index_rows": 5251,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this show that the Rowid Filter with ICP has the "walk-off-the-end-of-range" problem?

r_index_rows=5251. This is 20x what the server expected (rows=250).

Just to be sure, I've added to the test the same query with ORDER BY ASC. The output is:
https://gist.github.com/spetrunia/7ae335d01775400bb607672111c202ab

Key place:

          "rowid_filter": {
            "range": {
              "key": "b",
              "used_key_parts": ["b"]
            },
            "rows": 252,
            "selectivity_pct": 4.799085888,
            "r_rows": 250,
            "r_lookups": 250,
            "r_selectivity_pct": 100,
            "r_buffer_size": "REPLACED",
            "r_filling_time_ms": "REPLACED"
          },
          "loops": 1,
          "r_loops": 1,
          "rows": 250,
          "r_index_rows": 250,
          "r_rows": 250,

note r_index_rows=rows= 250, note r_lookups=250.
This is how it should have worked.

Please investigate this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants