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-12404: Add assertions about Index Condition Pushdown use #3123

Open
wants to merge 10 commits into
base: 11.5
Choose a base branch
from

Conversation

spetrunia
Copy link
Member

Add assertions about limitations one has when using Index Condition Pushdown:

  • add handler::assert_icp_limitations()
  • call this function from functions that may attempt violations.

Verified that assert_icp_limitations() as well as calls to it are compiled away in release build.

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

Description

TODO: fill description here

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

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 maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

illuusio and others added 9 commits February 19, 2024 18:50
…rdered

Fix Debian .install files to alphabetically ordered from
mostly ordered state.
MariaDB debian control files are slightly in disorder
run wrap-and-sort tool against them shows some minor
changes.
During the build process this is wrapped into plugin_auth.h.
… to fail

This was caused by the patch for
MDEV-32567 Remove thr_alarm from server codebase
This was needed as mtr --embedded connect.alter failed as online is
ignored for embedder server.
This fixes a wrong commit 30c965f
allow RPM upgrades from a different minor version,
if the major version is the same.
let ALL PRIVILEGES to always mean ALL PRIVILEGES over all
upgrades, no matter what new privileges were added in later versions.
Support index condition pushdown within partitioned tables.
- ha_partition will pass the pushed index condition into all of the used
  partitions.
  - We require that all of the partitions to handle the pushed index
    condition in the same way.
- When using ICP, one may read rows (e.g. call h->index_read_map(buf, ...)
  only to buf= table->record[0], for two reasons:
  * Pushed index condition's Item_field objects point into record[0]
  * InnoDB requires this: it calls offset() which assumes record[0].
  So, when using ICP, ha_partition will read partition records to
  table->record[0] and then will copy record away if it needs it to be
  elsewhere.
Add assertions about limitations one has when using Index Condition
Pushdown:
- add handler::assert_icp_limitations()
- call this function from functions that may attempt violations.

Verified that assert_icp_limitations() as well as calls to it are
compiled away in release build.
pushed_idx_cond has Item_field objects that refer to table->record[0].
*/
DBUG_ASSERT(!(pushed_idx_cond && active_index == pushed_idx_cond_keyno) ||
(buf == table->record[0]));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: redundant parentheses

void handler::assert_icp_limitations(uchar *buf)
{
/*
If we are using ICP, we must read the row to table->record[0], as
Copy link
Contributor

Choose a reason for hiding this comment

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

Good docs, explains the need for these assertions well

Base automatically changed from bb-11.4-mdev-12404 to 11.5 April 17, 2024 13:07
@CLAassistant
Copy link

CLAassistant commented Apr 18, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 6 committers have signed the CLA.

✅ illuusio
✅ grooverdan
✅ spetrunia
❌ montywi
❌ vuvova
❌ DaveGosselin-MariaDB
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
8 participants