September 2022 - developers - lists.mariadb.org

[Maria-developers] MDEV-25080 Allow pushdown of queries involving UNIONs in outer select to foreign engines
by Oleg Smirnov 09 Sep '22

09 Sep '22

Hello! The implementation for MDEV-25080 is ready and pushed to the *preview-10.11-mdev-25080-union-pushdown *branch. The FederatedX storage engine is adjusted accordingly. The example of how it works can be found at *mysql-test/federated/federatedx_create_handlers.test:* --echo # Pushdown of the whole UNION SELECT * from federated.t1 UNION SELECT * from federated.t2; EXPLAIN SELECT * from federated.t1 UNION SELECT * from federated.t2; --echo # Pushdown of a part of the UNION SELECT * from federated.t1 UNION SELECT * from t3; EXPLAIN SELECT * from federated.t1 UNION SELECT * from t3; -- Best regards, ------------------------- *Oleg Smirnov* Sr. Software Engineer MariaDB Server Team

1 0

[Maria-developers] Cosmetic input re: MDEV-25080 Allow pushdown of UNIONs to foreign engines
by Sergey Petrunia 06 Sep '22

06 Sep '22

Hi Oleg, Please find below coding style/etc input re the patch for MDEV-25080. (See the MDEV for other comments, more are coming). > diff --git a/sql/select_handler.cc b/sql/select_handler.cc > index 795ed8eb641..99bd2c0cf7f 100644 > --- a/sql/select_handler.cc > +++ b/sql/select_handler.cc > @@ -114,10 +125,7 @@ bool select_handler::send_data() > bool select_handler::send_eof() > { > DBUG_ENTER("select_handler::send_eof"); > - > - if (select->join->result->send_eof()) > - DBUG_RETURN(true); > - DBUG_RETURN(false); > + DBUG_RETURN(result->send_eof()); > } What is this change for? If it's just the code cleanup and we HAVE to do it (I don't see an urgent reason), please do it in a separate commit. Reasons against putting this change into this commit: When one sees this change as part of a meaningful patch, they start to examine the change to see what difference it makes. After one minute, one realizes the old and new code are the same. The time and attention have already been wasted, though. > diff --git a/sql/sql_explain.cc b/sql/sql_explain.cc > index 7c3918bfd20..6726e1b28f1 100644 > --- a/sql/sql_explain.cc > +++ b/sql/sql_explain.cc > @@ -555,6 +575,12 @@ int Explain_union::print_explain(Explain_query *query, > } > > if (!using_tmp) > + /* > + The union operation may not employ a temporary table, for example, > + for UNION ALL, in that case the results of the query are sent directly > + to the output. So there is no actual UNION operation and we don't need > + to print the line in the EXPLAIN output. > + */ > return 0; Please add { } braces as the if-part takes more than one line now. > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 9382625a747..437acbb749c 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -5549,9 +5552,13 @@ void st_select_lex::set_explain_type(bool on_the_fly) > type= "MATERIALIZED UNION"; > else > { > - type= is_uncacheable ? "UNCACHEABLE UNION": "UNION"; > + type= is_uncacheable ? "UNCACHEABLE UNION" > + : "UNION"; > Again: please don't just re-format the code > @@ -5561,15 +5568,14 @@ void st_select_lex::set_explain_type(bool on_the_fly) > { > bool uses_cte= false; > for (JOIN_TAB *tab= first_linear_tab(join, WITHOUT_BUSH_ROOTS, > - WITH_CONST_TABLES); > - tab; > - tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS)) > + WITH_CONST_TABLES); > + tab; tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS)) > { and here. > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 45fed46e375..2310583074b 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1843,6 +1843,8 @@ int JOIN::optimize() > join_optimization_state init_state= optimization_state; > if (select_lex->pushdown_select) > { > + if (optimization_state == JOIN::OPTIMIZATION_DONE) > + return 0; Please add and assert that after this we have optimization_state==JOIN::NOT_OPTIMIZED. > // Do same as JOIN::optimize_inner does: > fields= &select_lex->item_list; > > @@ -4885,7 +4887,6 @@ void JOIN::cleanup_item_list(List<Item> &items) const > DBUG_VOID_RETURN; > } > > - Please put this back, the coding style says to use two empty lines between the functions. > /** > @brief > Look for provision of the select_handler interface by a foreign engine .. > -select_handler *find_select_handler(THD *thd, > - SELECT_LEX* select_lex) > +select_handler *find_select_handler(THD *thd, SELECT_LEX *select_lex) ^ Don't reformat. > { > - if (select_lex->next_select()) > - return 0; > if (select_lex->master_unit()->outer_select()) > return 0; > > @@ -4921,15 +4922,14 @@ select_handler *find_select_handler(THD *thd, > { > tbl= select_lex->join->tables_list; > } > - else if (thd->lex->query_tables && > - thd->lex->query_tables->next_global) > + else if (thd->lex->query_tables && thd->lex->query_tables->next_global) > { > tbl= thd->lex->query_tables->next_global; > } > else > return 0; > > - for (;tbl; tbl= tbl->next_global) > + for (; tbl; tbl= tbl->next_global) > { > if (!tbl->table) > continue; > @@ -4937,12 +4937,14 @@ select_handler *find_select_handler(THD *thd, > if (!ht->create_select) > continue; > select_handler *sh= ht->create_select(thd, select_lex); > - return sh; > + if (sh) > + return sh; > } > return 0; > } Don't reformat. meaningful changes are ok. > > > + Remove the 3rd empty line. > /** > An entry point to single-unit select (a select without UNION). > ... diff --git a/sql/sql_union.cc b/sql/sql_union.cc index e8bdbcba2f2..4cb9f3e025a 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -1294,6 +1294,52 @@ bool init_item_int(THD* thd, Item_int* &item) return true; } > +select_handler *find_unit_handler(THD *thd, > + SELECT_LEX_UNIT *unit) ^^^ coding style. ... > @@ -2053,6 +2106,7 @@ void st_select_lex_unit::optimize_bag_operation(bool is_outer_distinct) > bag_set_op_optimized= true; > } > > +select_handler *find_select_handler(THD *thd, SELECT_LEX *select_lex); Please don't have function declaration in the middle of a file... > @@ -2272,14 +2350,14 @@ bool st_select_lex_unit::exec() > if (union_result->flush()) > { > thd->lex->current_select= lex_select_save; > - DBUG_RETURN(1); > + return true; Please fix identation. > diff --git a/storage/federatedx/federatedx_pushdown.cc b/storage/federatedx/federatedx_pushdown.cc > index 664f0570238..492055773cb 100644 > --- a/storage/federatedx/federatedx_pushdown.cc > +++ b/storage/federatedx/federatedx_pushdown.cc > @@ -163,13 +163,13 @@ void ha_federatedx_derived_handler::print_error(int, unsigned long) > } > > > -static select_handler* > -create_federatedx_select_handler(THD* thd, SELECT_LEX *sel) > +template<typename T> > +static select_handler *create_federatedx_handler(THD *thd, T *sel_lex) > { > if (!use_pushdown) > return 0; > Do we need to generate that much code? What if there was a function like bool check_pushdown_allowed(select_lex_node *node) which could process both SELECT_LEX and SELECT_LEX_UNIT objects. Then, we would need two one-liner functions: create_select_handler(type S) { if (check_pushdown_allowed(S)) return new ha_federatedx_select_handler(S) } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 489a7fba324: MDEV-29322 ASAN heap-use-after-free in Query_log_event::do_apply_event
by Sergei Golubchik 04 Sep '22

04 Sep '22

Hi, Andrei, I still don't understand this. 1. Why rgi->options_to_bin_log is only set for GTID_EVENT? Is there a guarantee that there always be a GTID_EVENT before a QUERY_EVENT? What if gtids aren't enabled? 2. how do you guarantee that all query events for a previous value of options_written_to_bin_log have been applied before updating it in the rgi? 3. and if you do guarantee it, why is it atomic? No query events = nobody reads options_written_to_bin_log concurrently. And why not to do a simple patch, like the one I've attached? (note, it's an intentionally minimal patch to show the fix, practically I'd accompany it with a small cleanup patch) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Sep 02, Andrei Elkin wrote: > > > Query_log_event cannot just randomly use some > > options_written_to_bin_log, it must use options_written_to_bin_log > > from the Format_description_log_event that came from the binlog file > > where the Query_log_event was read from. > > [ Not to engage you into a separate discussion, > still why 'randomly'? In the quoted description p.1 says > the Query_log_event uses `options_written_to_bin_log` from the > immediately preceding-in-binlog FD. ] > > > If rli->relay_log.description_event_for_exec is not that > > Format_description_log_event, and assuming that the correct > > Format_description_log_event is not stored anywhere, the Query_log_event > > has to remember options_written_to_bin_log itself, > > It's an option, but the chosen one avoids such remembering per each > Query event. Instead ... > > > there's not much we can do about it. > > > > Or it can apply the mask in the constructor and store not flags2 but > > the new value of thd->variables.option_bits. > > > > Or it can extend flags2 to cover all OPTIONS_WRITTEN_TO_BINLOG. > > ... of taking care of individual events the patch basically goes along > the base to use a placeholder that > A. lasts (unlike FD instance) the whole slave session time, and > B. the stored `options_written_to_binlog` is guaranteed to be a copy > of the most recent FD before the Query > > To help you to understand the method, here is a sequence diagram, > where on the top line > the slave SQL thread, its Relay-log-info object, a similar to that Worker's instance, > and a Worker thread. > > > SQL thread RLI/RGI Worker RGI Worker > | . . . > --->+ read FD . . i > | . . . > +--------------> > stores options > . > --->+ read Query > | . > +-------------------------->+ > finds Worker RGI > to store in theree a copy FD.options > | . > +-------------------------------------------->+ > schedules Query Worker | > . | > .<----------------+ > reads options | > | applies > ... Query > > It must be clear the A holds. > To prove B let after Query a second FD^2 is read by the SQL thread and > that FD^2 is from a different version server. > Then the SQL thread always have a wait-for-workers-to-become-idle > condition, so execution would go like this: > > --->+ read Query . ... > | . | > +-------------------------->+ | > finds Worker RGI | > to store in theree a copy FD.options | > | | > --->+ read FD^2 | applying of > | | Query > ... | > ... wait for Worker to complete Query | > ... | > +<============================================| > | > +-------------->. > | stores FD^2 > | options > ... > > So FD^2 can't affect queries from earlier binlogs. > > I am all ears to hear about your feedback. > > Cheers, > > Andrei

2 2

Re: [Maria-developers] 489a7fba324: MDEV-29322 ASAN heap-use-after-free in Query_log_event::do_apply_event
by Sergei Golubchik 01 Sep '22

01 Sep '22

Hi, Andrei, On Sep 01, Andrei wrote: > revision-id: 489a7fba324 (mariadb-10.5.17-10-g489a7fba324) > parent(s): 0324bde8469 > author: Andrei > committer: Andrei > timestamp: 2022-09-01 14:15:11 +0300 > message: > > MDEV-29322 ASAN heap-use-after-free in Query_log_event::do_apply_event > > The ASAN report was reported in the parallel slave execution of a Query > event and implicitly involved (so also parallelly run) Format-Description > event. > The Query actually had dependency on a preceding "old" FD whose > instance got destructed, so to cause the ASAN error. > > The case is fixed with copying of the value of > `options_written_to_bin_log` the sensitive member > of FD event into worker's stable placeholders, so FD change > won't affect Queries dependent on "old" instances. > > The method observes the following properties: > 0. each worker gets its private copy; > 1. the copied value gets a part of the execution context for any > Queries that follow the FD in binlog; > 2. any new FD forces a new copy which does not any affect on > the past (in binlog order) Queries which might be still in > execution. Sorry, but this doesn't make any sense. The value of options_written_to_bin_log depends on the version of the server that has written that binlog file. Query_log_event cannot just randomly use some options_written_to_bin_log, it must use options_written_to_bin_log from the Format_description_log_event that came from the binlog file where the Query_log_event was read from. If rli->relay_log.description_event_for_exec is not that Format_description_log_event, and assuming that the correct Format_description_log_event is not stored anywhere, the Query_log_event has to remember options_written_to_bin_log itself, there's not much we can do about it. Or it can apply the mask in the constructor and store not flags2 but the new value of thd->variables.option_bits. Or it can extend flags2 to cover all OPTIONS_WRITTEN_TO_BINLOG. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0