MDEV-39226: Push whole multi-table update/delete down into engines by bsrikanth-mariadb · Pull Request #5529 · MariaDB/server

bsrikanth-mariadb · 2026-08-11T10:54:21Z

Give storage engines a way to take over an entire multi-table
UPDATE/DELETE, the way they can already take over a SELECT. Without it the
join, the row matching and every modification run in the SQL layer even
when an engine could do the whole statement itself in one step; a
single-table UPDATE/DELETE already avoids this via
direct_update_rows()/direct_delete_rows(), but a multi-table statement has
no primary handler object to drive that path.

This adds a generic, engine-agnostic pushdown interface: the SQL layer
offers the statement to the engine, and if the engine accepts it, it
performs the whole thing and reports only the row counts.

Split select_handler into a pushdown_handler base with select_handler
(result set) and a new multi_upddel_handler (runs a whole UPDATE/DELETE,
reports row counts, reported as PUSHED UPDATE/PUSHED DELETE); add
handlerton::create_multi_upddel, looked up in Sql_cmd_dml::execute_inner().
multi_update/multi_delete gain direct_update_delete_done(), which records
the engine's counts so send_eof() binlogs and replies without the
SQL-layer loop; it forces statement-format binlogging so the change still
replicates under binlog_format=ROW, and errors out instead of silently
dropping counts for an unsupported result object.
FederatedX implements the interface as the reference engine used to test
correctness: it prints the statement back and runs it remotely, passes
the engine's error code/SQLSTATE through, reads the matched count from the
remote info string, executes IGNORE locally, and only pushes down when all
tables share one remote server (same as SELECT/derived/unit pushdown).

Test: federated.federatedx_pushdown_upd_del.

spetrunia · 2026-08-14T09:28:13Z

 const char *pushed_select_text= "PUSHED SELECT";
+const char *pushed_update_text= "PUSHED DOWN UPDATE";
+const char *pushed_delete_text= "PUSHED DOWN DELETE";
+


Please follow the pattern: if we use PUSHED SELECT, let's add PUSHED UPDATE , not PUSHED DOWN UPDATE.

Yeah, true. But, felt "PUSHED DOWN XXXX" was sounding better.

Anyways, will keep them consistent.

spetrunia · 2026-08-20T12:04:39Z

+static multi_upddel_handler *
+create_federatedx_multi_upddel_handler(THD *thd, SELECT_LEX *sel_lex)
+{
+  if (!use_pushdown || !is_supported_update_delete(thd->lex->sql_command))


What is this for? Do we get into this function for non-UPDATE/DELETE ?
I have added

DBUG_ASSERT(is_supported_update_delete(thd->lex->sql_command));

and it survived the tests.

hmm. will add the DBUG_ASSERT() instead.

spetrunia · 2026-08-20T12:16:35Z

Please apply this patch: cleanups.patch

spetrunia · 2026-08-20T12:20:59Z

+         str_eq(a->hostname, b->hostname) &&
+         str_eq(a->socket, b->socket) &&
+         str_eq(a->username, b->username) &&
+         str_eq(a->password, b->password);


Even password?
I'm not sure if it's possible to see different data depending on the user... Is password necessary?

sure, password check can be removed.

spetrunia · 2026-08-20T12:22:05Z

+
+ha_federatedx_multi_upddel_handler::~ha_federatedx_multi_upddel_handler()
+  = default;
+


Is this needed?

not really.

spetrunia · 2026-08-20T12:22:59Z

+  The range of the error codes of the client library, CR_MIN_ERROR and
+  CR_MAX_ERROR of errmsg.h. That header cannot be included here because it
+  defines ER, which the server defines differently.
+*/


If ER is the only problem, can one use

#undef ER #include ...

?

it is actually not needed. removed it now.

spetrunia

See above input.

Also, should create_multi_upddel accept SELECT_LEX argument, or LEX would be more meaningful?

bsrikanth-mariadb · 2026-08-21T08:00:36Z

See above input.

Also, should create_multi_upddel accept SELECT_LEX argument, or LEX would be more meaningful?

actually, SELECT_LEX is sufficient.

spetrunia · 2026-08-25T11:12:05Z

WARNINGS-QUESTION:
A question about possible warnings from the backend.

There shouldn't be many cases where that happens because the default logic is "warnings are errors in UPDATE".

Warnings can remain warnings when using UPDATE IGNORE, but we disable pushdown when IGNORE is present.

However, one can also disable warnings by removing STRICT_TRANS_TABLES or STRICT_ALL_TABLES from sql_mode. And ha_federatedx doesn't check for those. @bsrikanth-mariadb , should there be a check for those settings?

Claude also suggests there are warnings which do not turn into errors:

Warnings about optimizer hints
Warnings produced by triggers.
So, should we analyze/pass warnings from the backend? What does federatedx do for PUSHED SELECT or "direct update" ?
(Do not jump to implement please. Let's first gather info and decide. Perhaps, this is a too fine detail?)

spetrunia · 2026-08-25T11:20:29Z

Why does multi_update::direct_update_delete_done use

table->file->has_transactions_and_rollback()

while multi_delete::direct_update_delete_done use

tbl->table->file->has_transactions())

were the checks borrowed from somewhere?

spetrunia · 2026-08-25T11:22:23Z

Please change return type of direct_update_delete_done to void as it can't return errors.

spetrunia · 2026-08-25T11:27:28Z

See above input.
Also, should create_multi_upddel accept SELECT_LEX argument, or LEX would be more meaningful?

actually, SELECT_LEX is sufficient.

It is sufficient but it's not logical. Let's discuss changing it.

spetrunia

Please check the above.

Also please chekc those and add if there are no objections:

commit b89e5c654688c4c9c8db12d98d6d75a097699eb2 (HEAD -> 13.2-MDEV-39226-review-input, origin/13.2-MDEV-39226-review-input, 13.2-MDEV-39226-direct-multi_table-update-delete)
Author: Sergei Petrunia <sergey@mariadb.com>
Date:   Tue Aug 25 13:55:30 2026 +0300

    Review input 3: don't check for is_supported_update_delete()
    
    * we only get into create_federatedx_multi_upddel_handler for UPDATE/DELETE
    
    * Use of is_supported_update_delete() implies that this checks
      for UPDATEs/DELETEs of which only some are supported.
      The function actually returned TRUE for any UPDATE/DELETE.

commit e582992ea4b935b079496a99d1574cc11882beab
Author: Sergei Petrunia <sergey@mariadb.com>
Date:   Tue Aug 25 13:53:12 2026 +0300

    Review input 2: re-word comments

commit 9b4a8958024d205556cdabe398483c08ebb1d4ae
Author: Sergei Petrunia <sergey@mariadb.com>
Date:   Tue Aug 25 13:52:40 2026 +0300

    Review input 1: simplify code in EXPLAIN handling
    
    remove is_pushed_down_select_type(), keep just
    get_pushed_down_select_text().

bsrikanth-mariadb · 2026-08-25T12:25:51Z

Why does multi_update::direct_update_delete_done use
table->file->has_transactions_and_rollback()
while multi_delete::direct_update_delete_done use
tbl->table->file->has_transactions())
were the checks borrowed from somewhere?

Yes, following similar logic from direct_update, and direct_delete

bsrikanth-mariadb · 2026-08-25T12:31:39Z

Please change return type of direct_update_delete_done to void as it can't return errors.

Actually, the default implementation in select_result is to return true; So, only multi_delete, and multi_update can return false.

However, changed the return type to void, as it doesn't throw any real errors.

bsrikanth-mariadb · 2026-08-26T10:53:58Z

WARNINGS-QUESTION: A question about possible warnings from the backend.

There shouldn't be many cases where that happens because the default logic is "warnings are errors in UPDATE".

Warnings can remain warnings when using UPDATE IGNORE, but we disable pushdown when IGNORE is present.

However, one can also disable warnings by removing STRICT_TRANS_TABLES or STRICT_ALL_TABLES from sql_mode. And ha_federatedx doesn't check for those. @bsrikanth-mariadb , should there be a check for those settings?

Claude also suggests there are warnings which do not turn into errors:

Warnings about optimizer hints

Warnings produced by triggers.
So, should we analyze/pass warnings from the backend? What does federatedx do for PUSHED SELECT or "direct update" ?
(Do not jump to implement please. Let's first gather info and decide. Perhaps, this is a too fine detail?)

warnings are swallowed from the remote server. So, nothing gets shown to the client. The behaviour is same for PUSHED SELECT, and single table delete/update. Looks like FederatedX doesn't have a mechanism to handle warnings.

bsrikanth-mariadb · 2026-08-27T06:27:16Z

See above input.
Also, should create_multi_upddel accept SELECT_LEX argument, or LEX would be more meaningful?

actually, SELECT_LEX is sufficient.

It is sufficient but it's not logical. Let's discuss changing it.

Done.

spetrunia · 2026-09-03T11:12:53Z

Please consider the below patch. It does two things.

Adds comments
In multi_delete::direct_update_delete_done, the added comment claims that we do the same as multi_delete's initialize_tables() and send_data() do. But these functions do not call correspondent_table->find_table_for_update(). So it also removes that logic. Tests pass with this.
Please check if we have test coverage for this - DELETE FROM view_over_federated_x, other_table ... . Then let's either adopt this patch or prove that correspondent_table->find_table_for_update() call is necessary (I think it is not needed).

diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
index 3166fa32ecc..130db3df4d0 100644
--- a/sql/sql_delete.cc
+++ b/sql/sql_delete.cc
@@ -1739,9 +1739,9 @@ int multi_delete::rowid_table_deletes(TABLE *table, bool ignore)
 /*
   The engine has performed the whole multi-table DELETE on its own, the
   statement was pushed down through the select_handler interface. Remember
-  the row counts reported by the engine and mark the tables we deleted from,
-  so that send_eof() can binlog the statement and send the OK packet without
-  running the SQL-layer delete loop.
+  the row counts reported by the engine and check whether modified tables
+  were transactional. send_eof() will need this info to binlog the statement
+  correctly.
 */
 
 void multi_delete::direct_update_delete_done(ha_rows found_rows,
@@ -1753,11 +1753,13 @@ void multi_delete::direct_update_delete_done(ha_rows found_rows,
   deleted= affected_rows;
   direct_dml_done= true;
 
-  for (TABLE_LIST *walk= delete_tables; walk; walk= walk->next_local)
+  /*
+    Do the same as multi_delete's initialize_tables() and send_data() do:
+    walk the tables that we delete from and check if they are transactional
+  */
+  for (TABLE_LIST *tbl= delete_tables; tbl; tbl= tbl->next_local)
   {
-    TABLE_LIST *tbl= walk->table ? walk :
-                     walk->correspondent_table->find_table_for_update();
-    if (!tbl || !tbl->table)
+    if (!tbl->table)
       continue;
     if (tbl->table->file->has_transactions())
       transactional_tables= 1;
diff --git a/sql/sql_update.cc b/sql/sql_update.cc
index 9040d2d7a54..c8cc1513fff 100644
--- a/sql/sql_update.cc
+++ b/sql/sql_update.cc
@@ -2934,9 +2934,9 @@ int multi_update::do_updates()
 /*
   The engine has performed the whole multi-table UPDATE on its own, the
   statement was pushed down through the select_handler interface. Remember
-  the row counts reported by the engine and mark the updated tables as
-  modified, so that send_eof() can binlog the statement and send the OK
-  packet without running the SQL-layer update loop.
+  the row counts reported by the engine and check whether modified tables
+  were transactional. send_eof() will need this info to binlog the statement
+  correctly.
 */
 
 void multi_update::direct_update_delete_done(ha_rows found_rows,
@@ -2948,6 +2948,10 @@ void multi_update::direct_update_delete_done(ha_rows found_rows,
   updated= affected_rows;
   direct_dml_done= true;
 
+  /*
+    Do the same as multi_update::do_updates() does:
+    walk the tables that were updated and check if they were transactional
+  */
   for (TABLE_LIST *cur_table= update_tables; cur_table;
        cur_table= cur_table->next_local)
   {

bsrikanth-mariadb · 2026-09-04T04:05:22Z

Please consider the below patch. It does two things.

Adds comments
In multi_delete::direct_update_delete_done, the added comment claims that we do the same as multi_delete's initialize_tables() and send_data() do. But these functions do not call correspondent_table->find_table_for_update(). So it also removes that logic. Tests pass with this.
Please check if we have test coverage for this - DELETE FROM view_over_federated_x, other_table ... . Then let's either adopt this patch or prove that correspondent_table->find_table_for_update() call is necessary (I think it is not needed).

diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
index 3166fa32ecc..130db3df4d0 100644
--- a/sql/sql_delete.cc
+++ b/sql/sql_delete.cc
@@ -1739,9 +1739,9 @@ int multi_delete::rowid_table_deletes(TABLE *table, bool ignore)
 /*
   The engine has performed the whole multi-table DELETE on its own, the
   statement was pushed down through the select_handler interface. Remember
-  the row counts reported by the engine and mark the tables we deleted from,
-  so that send_eof() can binlog the statement and send the OK packet without
-  running the SQL-layer delete loop.
+  the row counts reported by the engine and check whether modified tables
+  were transactional. send_eof() will need this info to binlog the statement
+  correctly.
 */
 
 void multi_delete::direct_update_delete_done(ha_rows found_rows,
@@ -1753,11 +1753,13 @@ void multi_delete::direct_update_delete_done(ha_rows found_rows,
   deleted= affected_rows;
   direct_dml_done= true;
 
-  for (TABLE_LIST *walk= delete_tables; walk; walk= walk->next_local)
+  /*
+    Do the same as multi_delete's initialize_tables() and send_data() do:
+    walk the tables that we delete from and check if they are transactional
+  */
+  for (TABLE_LIST *tbl= delete_tables; tbl; tbl= tbl->next_local)
   {
-    TABLE_LIST *tbl= walk->table ? walk :
-                     walk->correspondent_table->find_table_for_update();
-    if (!tbl || !tbl->table)
+    if (!tbl->table)
       continue;
     if (tbl->table->file->has_transactions())
       transactional_tables= 1;
diff --git a/sql/sql_update.cc b/sql/sql_update.cc
index 9040d2d7a54..c8cc1513fff 100644
--- a/sql/sql_update.cc
+++ b/sql/sql_update.cc
@@ -2934,9 +2934,9 @@ int multi_update::do_updates()
 /*
   The engine has performed the whole multi-table UPDATE on its own, the
   statement was pushed down through the select_handler interface. Remember
-  the row counts reported by the engine and mark the updated tables as
-  modified, so that send_eof() can binlog the statement and send the OK
-  packet without running the SQL-layer update loop.
+  the row counts reported by the engine and check whether modified tables
+  were transactional. send_eof() will need this info to binlog the statement
+  correctly.
 */
 
 void multi_update::direct_update_delete_done(ha_rows found_rows,
@@ -2948,6 +2948,10 @@ void multi_update::direct_update_delete_done(ha_rows found_rows,
   updated= affected_rows;
   direct_dml_done= true;
 
+  /*
+    Do the same as multi_update::do_updates() does:
+    walk the tables that were updated and check if they were transactional
+  */
   for (TABLE_LIST *cur_table= update_tables; cur_table;
        cur_table= cur_table->next_local)
   {

I added view tests, and they confirm that the call to correspondent_table->find_table_for_update() is not really needed. However, initialize_tables() do use correspondent_table->find_table_for_update()

spetrunia · 2026-09-04T08:31:27Z

However, initialize_tables() do use correspondent_table->find_table_for_update()

Agree, it actually does that. Missed it.
It looked odd that send_data() then does NOT use it.
Apparently this is because initialize_tables() "fixes" it here:

    if (!tab->bush_children && tab->table->map & tables_to_delete_from)
    {
      /* We are going to delete from this table */
      TABLE *tbl=walk->table=tab->table;

note that walk->table is assigned tab->table here.

spetrunia · 2026-09-04T08:33:47Z

Note that in this patch federatedx actually disables support for VIEWs.
If I attempt to enable it

diff --git a/storage/federatedx/federatedx_pushdown.cc b/storage/federatedx/federatedx_pushdown.cc
index 0b1f531b724..96f42dd5fdb 100644
--- a/storage/federatedx/federatedx_pushdown.cc
+++ b/storage/federatedx/federatedx_pushdown.cc
@@ -126,7 +126,7 @@ static TABLE *get_fed_table_for_pushdown(SELECT_LEX *sel_lex,
   {
     if (!tbl->table)
       return nullptr;
-    if (tbl->derived)
+    if (tbl->derived || tbl->view)
     {
       /*
         Skip derived table for now as they will be checked

I get view references in the the query that is submitted to the backend:

delete  from `j1`.`t1` using `j1`.`v1` join `j1`.`t2` where `v1`.`pk` = `j1`.`t2`.`pk` and `j1`.`t2`.`a` < 3

spetrunia · 2026-09-04T09:59:48Z

But what if some federated engine DOES support VIEWs...
The result->initialize_tables(this)) is made from JOIN::optimize_stage2(), which is not invoked when Update/Delete is pushed down... So, this change I've mentioned above is not made:

      /* We are going to delete from this table */
      TABLE *tbl=walk->table=tab->table;

Does it mean that multi_update::direct_update_delete_done
should have the

   TABLE_LIST *tbl= walk->table ? walk :
                    walk->correspondent_table->find_table_for_update();

call after all?

spetrunia · 2026-09-04T10:21:03Z

As discussed via voice: it looks like we need to put this back in:

-    TABLE_LIST *tbl= walk->table ? walk :
-                     walk->correspondent_table->find_table_for_update();
-    if (!tbl || !tbl->table)

It should be there but we can't have the test coverage for it currently.

spetrunia

After that, the patch is good to go.

Give storage engines a way to take over an entire multi-table UPDATE/DELETE, the way they can already take over a SELECT. Without it the join, the row matching and every modification run in the SQL layer even when an engine could do the whole statement itself in one step; a single-table UPDATE/DELETE already avoids this via direct_update_rows()/direct_delete_rows(), but a multi-table statement has no primary handler object to drive that path. This adds a generic, engine-agnostic pushdown interface: the SQL layer offers the statement to the engine, and if the engine accepts it, it performs the whole thing and reports only the row counts. - Split select_handler into a pushdown_handler base with select_handler (result set) and a new multi_upddel_handler (runs a whole UPDATE/DELETE, reports row counts, reported as PUSHED UPDATE/PUSHED DELETE); add handlerton::create_multi_upddel, looked up in Sql_cmd_dml::execute_inner(). - multi_update/multi_delete gain direct_update_delete_done(), which records the engine's counts so send_eof() binlogs and replies without the SQL-layer loop; it forces statement-format binlogging so the change still replicates under binlog_format=ROW, and errors out instead of silently dropping counts for an unsupported result object. - FederatedX implements the interface as the reference engine used to test correctness: it prints the statement back and runs it remotely, passes the engine's error code/SQLSTATE through, reads the matched count from the remote info string, executes IGNORE locally, and only pushes down when all tables share one remote server (same as SELECT/derived/unit pushdown). Test: federated.federatedx_pushdown_upd_del.

bsrikanth-mariadb added the MariaDB Corporation label Aug 11, 2026

bsrikanth-mariadb marked this pull request as draft August 11, 2026 10:54

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch 5 times, most recently from f2ce815 to 44765e4 Compare August 14, 2026 08:16

spetrunia reviewed Aug 14, 2026

View reviewed changes

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch 3 times, most recently from b931524 to eb052c3 Compare August 18, 2026 07:04

bsrikanth-mariadb changed the title ~~MDEV-39226: Add multi-table update, delete feature~~ MDEV-39226: Push whole multi-table update/delete down into engines Aug 18, 2026

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch 5 times, most recently from 168a3b1 to f59b0bd Compare August 20, 2026 05:22

bsrikanth-mariadb marked this pull request as ready for review August 20, 2026 05:41

spetrunia reviewed Aug 20, 2026

View reviewed changes

spetrunia requested changes Aug 20, 2026

View reviewed changes

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch from f59b0bd to ed733d2 Compare August 21, 2026 08:08

spetrunia self-requested a review August 25, 2026 11:28

spetrunia requested changes Aug 25, 2026

View reviewed changes

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch from ed733d2 to e2323e5 Compare August 25, 2026 12:52

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch 4 times, most recently from 3deb33d to 5992b3d Compare August 27, 2026 04:48

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch 2 times, most recently from 01b2b18 to 86ccfaa Compare September 4, 2026 06:49

bsrikanth-mariadb closed this Sep 4, 2026

bsrikanth-mariadb reopened this Sep 4, 2026

spetrunia self-requested a review September 4, 2026 10:21

spetrunia approved these changes Sep 4, 2026

View reviewed changes

bsrikanth-mariadb force-pushed the 13.2-MDEV-39226-direct-multi_table-update-delete branch from 86ccfaa to 7d0792a Compare September 4, 2026 12:02


		ha_federatedx_multi_upddel_handler::~ha_federatedx_multi_upddel_handler()
		= default;

Uh oh!

Uh oh!

Conversation

bsrikanth-mariadb commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrunia commented Aug 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb commented Aug 21, 2026

Uh oh!

spetrunia commented Aug 25, 2026

Uh oh!

spetrunia commented Aug 25, 2026

Uh oh!

spetrunia commented Aug 25, 2026

Uh oh!

spetrunia commented Aug 25, 2026

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

bsrikanth-mariadb commented Aug 25, 2026

Uh oh!

bsrikanth-mariadb commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsrikanth-mariadb commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsrikanth-mariadb commented Aug 27, 2026

Uh oh!

spetrunia commented Sep 3, 2026

Uh oh!

bsrikanth-mariadb commented Sep 4, 2026

Uh oh!

spetrunia commented Sep 4, 2026

Uh oh!

spetrunia commented Sep 4, 2026

Uh oh!

spetrunia commented Sep 4, 2026

Uh oh!

spetrunia commented Sep 4, 2026

Uh oh!

spetrunia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

bsrikanth-mariadb commented Aug 11, 2026 •

edited

Loading

bsrikanth-mariadb commented Aug 25, 2026 •

edited

Loading

bsrikanth-mariadb commented Aug 26, 2026 •

edited

Loading