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-30469 Support ORDER BY and LIMIT for multi-table DELETE, index h… #3660

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DaveGosselin-MariaDB
Copy link
Member

@DaveGosselin-MariaDB DaveGosselin-MariaDB commented Nov 25, 2024

…ints for single-table DELETE.

We now allow multitable queries with order by and limit, such as:
delete t1., t2. from t1, t2 order by t1.id desc limit 3;
To predict what rows will be deleted, run the equivalent select:
select t1., t2. from t1, t2 order by t1.id desc limit 3;
Additionally, index hints are now supported with single table delete statements:
delete from t2 use index(xid) order by (id) limit 2;

This approach changes the multi_delete SELECT result interceptor to use a temporary table to collect row ids pertaining to the rows that will be deleted, rather than directly deleting rows from the target table(s). Row ids are collected during send_data, then read during send_eof to delete target rows. In the event that the temporary table created in memory is not big enough for all matching rows, it is converted to an aria table.

This patch includes a number of changes that begin to make the code easier to maintain

  • Renamed SQL_I_List::link_in_list to SQL_I_List::insert. link_in_list was ambiguous as it could refer to a link or it could refer to a node
  • Remove field_name local variable in multi_update::initialize_tables because it is not used when creating the temporary tables
  • multi_update changes:
    • Move temp table callocs to init, a more natural location for them, and moved tables_to_update to const member variable so we don't recompute it.
    • Filter out jtbm tables and tables not in the update map, pushing those that will be updated into an update_targets container. This simplifies checks and loops in initialize_tables.

Other changes:

  • Deleting from a sequence now affects zero rows instead of emitting an error

Limitations:

  • The federated connector does not create implicit row ids, so we to use a key when conditionally deleting. See the change in federated_maybe_16324629.test

@CLAassistant
Copy link

CLA assistant check
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.

sql/sql_delete.cc Outdated Show resolved Hide resolved
sql/sql_delete.cc Outdated Show resolved Hide resolved
sql/sql_delete.cc Outdated Show resolved Hide resolved
sql/sql_update.cc Outdated Show resolved Hide resolved
@spetrunia
Copy link
Member

  • DELETE can send empty results, so don't assert(0) on DA_EMPTY.

Please explain why? I thought its result would be always "N rows deleted" (where N can be 0). What did change with this patch?

@DaveGosselin-MariaDB DaveGosselin-MariaDB force-pushed the 11.8-mdev-30469 branch 2 times, most recently from d1dc3c2 to f228b06 Compare November 26, 2024 22:24
@DaveGosselin-MariaDB
Copy link
Member Author

  • DELETE can send empty results, so don't assert(0) on DA_EMPTY.

Please explain why? I thought its result would be always "N rows deleted" (where N can be 0). What did change with this patch?

My change was incorrect. Sanja explained to me what to do on the MDEV, so I fixed my code accordingly.

…ints for single-table DELETE

This patch includes a few changes to make the code easier to maintain:
  - Renamed SQL_I_List::link_in_list to SQL_I_List::insert.  link_in_list was
  ambiguous as it could refer to a link or it could refer to a node
  - Remove field_name local variable in multi_update::initialize_tables because
  it is not used when creating the temporary tables
  - multi_update changes:
    - Move temp table callocs to init, a more natural location for them, and moved
    tables_to_update to const member variable so we don't recompute it.
    - Filter out jtbm tables and tables not in the update map, pushing those that
    will be updated into an update_targets container.  This simplifies checks and
    loops in initialize_tables.
…ints for single-table DELETE

We now allow multitable queries with order by and limit, such as:
  delete t1.*, t2.* from t1, t2 order by t1.id desc limit 3;
To predict what rows will be deleted, run the equivalent select:
  select t1.*, t2.* from t1, t2 order by t1.id desc limit 3;
Additionally, index hints are now supported with single table delete statements:
  delete from t2 use index(xid) order by (id) limit 2;

This approach changes the multi_delete SELECT result interceptor to use a temporary
table to collect row ids pertaining to the rows that will be deleted, rather than
directly deleting rows from the target table(s).  Row ids are collected during
send_data, then read during send_eof to delete target rows.  In the event that the
temporary table created in memory is not big enough for all matching rows, it is
converted to an aria table.

Limitations:
  - The federated connector does not create implicit row ids, so we to use a key
  when conditionally deleting.  See the change in federated_maybe_16324629.test
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.

4 participants