Skip to content

Commit

Permalink
MDEV-33533: Crash at execution of DELETE when trying to use rowid filter
Browse files Browse the repository at this point in the history 
(Based on original patch by Oleksandr Byelkin)

Multi-table DELETE can execute via "buffered" mode: at phase #1 it collects
rowids of rows to be deleted, then at phase #2 in multi_delete::do_deletes()
it calls handler->rnd_pos() to read rows to be deleted and deletes them.

The problem occurred when phase #1 used Rowid Filter on the table that
phase #2 would be deleting from.
In InnoDB, h->rnd_init(scan=false) and h->rnd_pos() is an index scan over PK
under the hood. So, at phase #2 ha_innobase::rnd_init() would try to use the
rowid filter an assertion inside ha_innobase::rnd_init().

Note that multi-table UPDATE works similarly but was not affected, because
patch for MDEV-7487 added code to disable rowid filter for phase #2 in
multi_update::do_updates().

This patch changes the approach:
- It makes InnoDB not use Rowid Filter in rnd_pos() scans: it is disabled in
  ha_innobase::rnd_init() and enabled back in ha_innobase::rnd_end().
- multi_update::do_updates() no longer disables rowid filter for phase#2 as
  it is no longer necessary.
  • Loading branch information
@spetrunia
spetrunia committed May 6, 2024
1 parent 683fbce commit 2a3a1ba
Show file tree
Hide file tree
Showing 7 changed files with 860 additions and 9 deletions.
Loading

2 comments on commit 2a3a1ba

@mariadb-DebarunBanerjee
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Sergei,
Thanks for solving the issue which could be a critical problem for user. I understand what we are fixing here but I could not agree with the design and adding storage engine specific code to enable/disable row ID filtering. It should be driven by the layer above.

A. About the design. I think we should clearly state the design objectives.

  1. The current design is based on handler level flag to enable/disable the feature. Ideally SQL should be diligent enough to keep the state correct for any scan. IMHO, It is very hard to create and maintain sub-criteria and conditions embedded in different places in code to disable the optimization,
  2. If not (1) and we do decide that some handler interfaces (like rnd_pos) should never use row ID filtering optimization, it should be clearly documented in handler.h for such interfaces. Once documented we could decide where to have such checks. It doesn't look good to force the storage engine to implement such checks. Keeping it at ha_* interface level seems a good idea to keep it storage engine agnostic.
  • Note: The issue is actually generic and not specific to Innodb. We should not try to base our design based on how the interfaces are implemented and there by restricting Storage Engine implementation. Any storage supporting cluster index would have same issue. Only heap tables could be immune to it but making such assumption is likely not a good idea.

B. Version to be fixed: The issue should be there 10.5 too. The only issue could be generating the right plan. We should fix it in all versions otherwise it would stay like a hidden bomb fired in customer place with a very complex scenario.

Regards,
Deb

@mariadb-DebarunBanerjee
Copy link
Contributor

Choose a reason for hiding this comment

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

I am done with the review and the patch looks functionally correct.
The expectation from SE looks like an exception and I would love to see this dependency eliminated in future. More comments in MDEV.

Please sign in to comment.