[Commits] 6f1628b: MDEV-25565 Crash on 2-nd execution of SP/PS for query calculating window functions

Vicențiu Ciorbaru vicentiu at mariadb.org
Tue Jul 20 14:11:55 EEST 2021


Hi Igor!

The patch looks good, however I suggest the following changes to make it
smaller and easier to read & maintain.

1. You do not need to have special if statements for save_partition_list
and save_order_list when you do the cleanup.
2. You do not need to have special code that saves the partition list and
the order list *only* when it is overwritten.

You can instead always set save_partition_list to be equal to
partition_list in the Window_spec's constructor. Same for save_order_list
and order_list.

Then, during cleanup, you can always execute
order_list= save_order_list;
partition_list= save_partition_list;

The advantage of this change is that it removes all the references to
save_xxxx_list in sql_window.cc. You then only have a simple cleanup
function sql_union.cc.

3. I would prefer to name the variables "initial_order_list" and
"initial_partition_list".

The patch then becomes:
diff --git a/sql/sql_union.cc b/sql/sql_union.cc
index e5648e6989b..54a33bb7863 100644
--- a/sql/sql_union.cc
+++ b/sql/sql_union.cc
@@ -30,6 +30,7 @@
 #include "filesort.h"                           // filesort_free_buffers
 #include "sql_view.h"
 #include "sql_cte.h"
+#include "item_windowfunc.h"

 bool mysql_union(THD *thd, LEX *lex, select_result *result,
                  SELECT_LEX_UNIT *unit, ulong setup_tables_done_option)
@@ -1551,6 +1552,21 @@ static void cleanup_order(ORDER *order)
 }


+static void cleanup_window_funcs(List<Item_window_func> &win_funcs)
+{
+  List_iterator_fast<Item_window_func> it(win_funcs);
+  Item_window_func *win_func;
+  while ((win_func= it++))
+  {
+    Window_spec *win_spec= win_func->window_spec;
+    if (!win_spec)
+      continue;
+    win_spec->partition_list= win_spec->initial_partition_list;
+    win_spec->order_list= win_spec->initial_order_list;
+  }
+}
+
+
 bool st_select_lex::cleanup()
 {
   bool error= FALSE;
@@ -1559,6 +1575,8 @@ bool st_select_lex::cleanup()
   cleanup_order(order_list.first);
   cleanup_order(group_list.first);

+  cleanup_window_funcs(window_funcs);
+
   if (join)
   {
     List_iterator<TABLE_LIST> ti(leaf_tables);
diff --git a/sql/sql_window.h b/sql/sql_window.h
index e0c1563e5bb..cf84e26c822 100644
--- a/sql/sql_window.h
+++ b/sql/sql_window.h
@@ -99,8 +99,10 @@ class Window_spec : public Sql_alloc
   LEX_STRING *window_ref;

   SQL_I_List<ORDER> *partition_list;
+  SQL_I_List<ORDER> *initial_partition_list;

   SQL_I_List<ORDER> *order_list;
+  SQL_I_List<ORDER> *initial_order_list;

   Window_frame *window_frame;

@@ -111,7 +113,8 @@ class Window_spec : public Sql_alloc
               SQL_I_List<ORDER> *ord_list,
               Window_frame *win_frame)
     : window_names_are_checked(false), window_ref(win_ref),
-      partition_list(part_list), order_list(ord_list),
+      partition_list(part_list), initial_partition_list(part_list),
+      order_list(ord_list), initial_order_list(ord_list),
       window_frame(win_frame), referenced_win_spec(NULL) {}

   virtual char *name() { return NULL; }


On Sat, 10 Jul 2021 at 04:57, IgorBabaev <igor at mariadb.com> wrote:

> revision-id: 6f1628b917d365ecfc9c4c9951011613f4212592
> (mariadb-10.2.31-1048-g6f1628b)
> parent(s): fb0b28932ce82903f2fcfb690a71bff52355507f
> author: Igor Babaev
> committer: Igor Babaev
> timestamp: 2021-07-09 18:56:34 -0700
> message:
>
> MDEV-25565 Crash on 2-nd execution of SP/PS for query calculating window
> functions
>            from view
>
> A crash of the server happened when executing a stored procedure whose the
> only query calculated window functions over a mergeable view specified
> as a select from non-mergeable view. The crash could be reproduced if
> the window specifications of the window functions were identical and both
> contained PARTITION lists and ORDER BY lists. A crash also happened on
> the second execution of the prepared statement created for such query.
> If to use derived tables or CTE instead of views the problem still
> manifests itself crashing the server.
>
> When optimizing the window specifications of a window function the
> server can substitute the partition lists and the order lists for
> the corresponding lists from another window specification in the case
> when the lists are identical. This substitution is not permanent and should
> be rolled back before the second execution. It was not done and this
> ultimately led to a crash when resolving the column names at the second
> execution of SP/PS.
>
> ---
>  mysql-test/r/win.result | 287
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  mysql-test/t/win.test   | 147 +++++++++++++++++++++++++
>  sql/sql_union.cc        |  26 +++++
>  sql/sql_window.cc       |  12 ++
>  sql/sql_window.h        |   5 +-
>  5 files changed, 476 insertions(+), 1 deletion(-)
>
> diff --git a/mysql-test/r/win.result b/mysql-test/r/win.result
> index 8a31dcc..bc017ea 100644
> --- a/mysql-test/r/win.result
> +++ b/mysql-test/r/win.result
> @@ -3911,5 +3911,292 @@ sum(i) over () IN ( SELECT 1 FROM t1 a)
>  0
>  DROP TABLE t1;
>  #
> +# MDEV-25565: 2-nd call of SP with SELECT from view / derived table / CTE
> +#             returning the result  of calculation of 2 window
> +#             functions that use the same window specification
> +#
> +create table t1 (a int);
> +insert into t1 values (3), (7), (1), (7), (1), (1), (3), (1), (5);
> +create view v2 as select a from t1 group by a;
> +create view v1 as select * from v2;
> +create procedure sp1() select v1.a,
> +sum(v1.a) over (partition by v1.a  order by v1.a) as k,
> +avg(v1.a) over (partition by v1.a  order by v1.a) as m
> +from v1;
> +call sp1();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp1();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "select v1.a,
> +sum(v1.a) over (partition by v1.a  order by v1.a) as k,
> +avg(v1.a) over (partition by v1.a  order by v1.a) as m
> +from v1";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +create procedure sp2() select * from
> +( select dt1.a,
> +sum(dt1.a) over (partition by dt1.a  order by dt1.a) as k,
> +avg(dt1.a) over (partition by dt1.a  order by dt1.a) as m
> +from (select * from v2) as dt1
> +) as dt;
> +call sp2();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp2();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "select * from
> +( select dt1.a,
> +sum(dt1.a) over (partition by dt1.a  order by dt1.a) as k,
> +avg(dt1.a) over (partition by dt1.a  order by dt1.a) as m
> +from (select * from v2) as dt1
> +) as dt";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +create procedure sp3() select * from
> +( select dt1.a,
> +sum(dt1.a) over (partition by dt1.a  order by dt1.a) as k,
> +avg(dt1.a) over (partition by dt1.a  order by dt1.a) as m
> +from ( select * from (select * from t1 group by a) as dt2 ) as dt1
> +) as dt;
> +call sp3();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp3();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "select * from
> +( select dt1.a,
> +sum(dt1.a) over (partition by dt1.a  order by dt1.a) as k,
> +avg(dt1.a) over (partition by dt1.a  order by dt1.a) as m
> +from ( select * from (select * from t1 group by a) as dt2 ) as dt1
> +) as dt";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +create procedure sp4() with cte1 as (select * from (select * from t1
> group by a) as dt2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte;
> +call sp4();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp4();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "with cte1 as (select * from (select * from t1 group by
> a) as dt2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +create procedure sp5() with cte1 as (select * from v2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte;
> +call sp5();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp5();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "with cte1 as (select * from v2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +create procedure sp6() with
> +cte1 as (with cte2 as (select * from t1 group by a) select * from cte2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte;
> +call sp6();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp6();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "with
> +cte1 as (with cte2 as (select * from t1 group by a) select * from cte2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +create procedure sp7() with
> +cte2 as (select * from v1),
> +cte1 as (select * from cte2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte;
> +call sp7();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +call sp7();
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +prepare stmt from "with
> +cte2 as (select * from v1),
> +cte1 as (select * from cte2),
> +cte as
> +( select cte1.a,
> +sum(cte1.a) over (partition by cte1.a  order by cte1.a) as k,
> +avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +from cte1 )
> +select * from cte";
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +execute stmt;
> +a      k       m
> +1      1       1.0000
> +3      3       3.0000
> +5      5       5.0000
> +7      7       7.0000
> +deallocate prepare stmt;
> +drop procedure sp1;
> +drop procedure sp2;
> +drop procedure sp3;
> +drop procedure sp4;
> +drop procedure sp5;
> +drop procedure sp6;
> +drop procedure sp7;
> +drop view v1,v2;
> +drop table t1;
> +#
>  # End of 10.2 tests
>  #
> diff --git a/mysql-test/t/win.test b/mysql-test/t/win.test
> index c07a81f..72e789d 100644
> --- a/mysql-test/t/win.test
> +++ b/mysql-test/t/win.test
> @@ -2557,5 +2557,152 @@ SELECT sum(i) over () IN ( SELECT 1 FROM t1 a)
> FROM t1;
>  DROP TABLE t1;
>
>  --echo #
> +--echo # MDEV-25565: 2-nd call of SP with SELECT from view / derived
> table / CTE
> +--echo #             returning the result  of calculation of 2 window
> +--echo #             functions that use the same window specification
> +--echo #
> +
> +create table t1 (a int);
> +insert into t1 values (3), (7), (1), (7), (1), (1), (3), (1), (5);
> +
> +create view v2 as select a from t1 group by a;
> +create view v1 as select * from v2;
> +
> +let $q1=
> +select v1.a,
> +          sum(v1.a) over (partition by v1.a  order by v1.a) as k,
> +          avg(v1.a) over (partition by v1.a  order by v1.a) as m
> +from v1;
> +
> +eval create procedure sp1() $q1;
> +call sp1();
> +call sp1();
> +
> +eval prepare stmt from "$q1";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +let $q2=
> +select * from
> +  ( select dt1.a,
> +           sum(dt1.a) over (partition by dt1.a  order by dt1.a) as k,
> +           avg(dt1.a) over (partition by dt1.a  order by dt1.a) as m
> +    from (select * from v2) as dt1
> +  ) as dt;
> +
> +eval create procedure sp2() $q2;
> +call sp2();
> +call sp2();
> +
> +eval prepare stmt from "$q2";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +let $q3=
> +select * from
> +  ( select dt1.a,
> +           sum(dt1.a) over (partition by dt1.a  order by dt1.a) as k,
> +           avg(dt1.a) over (partition by dt1.a  order by dt1.a) as m
> +    from ( select * from (select * from t1 group by a) as dt2 ) as dt1
> +  ) as dt;
> +
> +eval create procedure sp3() $q3;
> +call sp3();
> +call sp3();
> +
> +eval prepare stmt from "$q3";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +let $q4=
> +with cte1 as (select * from (select * from t1 group by a) as dt2),
> +     cte as
> +     ( select cte1.a,
> +              sum(cte1.a) over (partition by cte1.a  order by cte1.a) as
> k,
> +              avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +       from cte1 )
> +select * from cte;
> +
> +eval create procedure sp4() $q4;
> +call sp4();
> +call sp4();
> +
> +eval prepare stmt from "$q4";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +let $q5=
> +with cte1 as (select * from v2),
> +     cte as
> +     ( select cte1.a,
> +              sum(cte1.a) over (partition by cte1.a  order by cte1.a) as
> k,
> +              avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +       from cte1 )
> +select * from cte;
> +
> +eval create procedure sp5() $q5;
> +call sp5();
> +call sp5();
> +
> +eval prepare stmt from "$q5";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +let $q6=
> +with
> +cte1 as (with cte2 as (select * from t1 group by a) select * from cte2),
> +     cte as
> +     ( select cte1.a,
> +              sum(cte1.a) over (partition by cte1.a  order by cte1.a) as
> k,
> +              avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +       from cte1 )
> +select * from cte;
> +
> +eval create procedure sp6() $q6;
> +call sp6();
> +call sp6();
> +
> +eval prepare stmt from "$q6";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +let $q7=
> +with
> +     cte2 as (select * from v1),
> +     cte1 as (select * from cte2),
> +     cte as
> +     ( select cte1.a,
> +              sum(cte1.a) over (partition by cte1.a  order by cte1.a) as
> k,
> +              avg(cte1.a) over (partition by cte1.a  order by cte1.a) as m
> +       from cte1 )
> +select * from cte;
> +
> +eval create procedure sp7() $q7;
> +call sp7();
> +call sp7();
> +
> +eval prepare stmt from "$q7";
> +execute stmt;
> +execute stmt;
> +deallocate prepare stmt;
> +
> +
> +drop procedure sp1;
> +drop procedure sp2;
> +drop procedure sp3;
> +drop procedure sp4;
> +drop procedure sp5;
> +drop procedure sp6;
> +drop procedure sp7;
> +drop view v1,v2;
> +drop table t1;
> +
> +--echo #
>  --echo # End of 10.2 tests
>  --echo #
> diff --git a/sql/sql_union.cc b/sql/sql_union.cc
> index 7baedfb..f3c90b8 100644
> --- a/sql/sql_union.cc
> +++ b/sql/sql_union.cc
> @@ -30,6 +30,7 @@
>  #include "filesort.h"                           // filesort_free_buffers
>  #include "sql_view.h"
>  #include "sql_cte.h"
> +#include "item_windowfunc.h"
>
>  bool mysql_union(THD *thd, LEX *lex, select_result *result,
>                   SELECT_LEX_UNIT *unit, ulong setup_tables_done_option)
> @@ -1550,6 +1551,29 @@ static void cleanup_order(ORDER *order)
>  }
>
>
> +static void cleanup_window_funcs(List<Item_window_func> &win_funcs)
> +{
> +  List_iterator_fast<Item_window_func> it(win_funcs);
> +  Item_window_func *win_func;
> +  while ((win_func= it++))
> +  {
> +    Window_spec *win_spec= win_func->window_spec;
> +    if (!win_spec)
> +      continue;
> +    if (win_spec->save_partition_list)
> +    {
> +      win_spec->partition_list= win_spec->save_partition_list;
> +      win_spec->save_partition_list= NULL;
> +    }
> +    if (win_spec->save_order_list)
> +    {
> +      win_spec->order_list= win_spec->save_order_list;
> +      win_spec->save_order_list= NULL;
> +    }
> +  }
> +}
> +
> +
>  bool st_select_lex::cleanup()
>  {
>    bool error= FALSE;
> @@ -1558,6 +1582,8 @@ bool st_select_lex::cleanup()
>    cleanup_order(order_list.first);
>    cleanup_order(group_list.first);
>
> +  cleanup_window_funcs(window_funcs);
> +
>    if (join)
>    {
>      List_iterator<TABLE_LIST> ti(leaf_tables);
> diff --git a/sql/sql_window.cc b/sql/sql_window.cc
> index 612c6e6..3ef751b 100644
> --- a/sql/sql_window.cc
> +++ b/sql/sql_window.cc
> @@ -479,9 +479,15 @@ int
> compare_window_funcs_by_window_specs(Item_window_func *win_func1,
>        Let's use only one of the lists.
>      */
>      if (!win_spec1->name() && win_spec2->name())
> +    {
> +      win_spec1->save_partition_list= win_spec1->partition_list;
>        win_spec1->partition_list= win_spec2->partition_list;
> +    }
>      else
> +    {
> +      win_spec2->save_partition_list= win_spec2->partition_list;
>        win_spec2->partition_list= win_spec1->partition_list;
> +    }
>
>      cmp= compare_order_lists(win_spec1->order_list,
>                               win_spec2->order_list);
> @@ -494,9 +500,15 @@ int
> compare_window_funcs_by_window_specs(Item_window_func *win_func1,
>         Let's use only one of the lists.
>      */
>      if (!win_spec1->name() && win_spec2->name())
> +    {
> +      win_spec1->save_order_list= win_spec2->order_list;
>        win_spec1->order_list= win_spec2->order_list;
> +    }
>      else
> +    {
> +      win_spec1->save_order_list= win_spec2->order_list;
>        win_spec2->order_list= win_spec1->order_list;
> +    }
>
>      cmp= compare_window_frames(win_spec1->window_frame,
>                                 win_spec2->window_frame);
> diff --git a/sql/sql_window.h b/sql/sql_window.h
> index e0c1563..417d0bc 100644
> --- a/sql/sql_window.h
> +++ b/sql/sql_window.h
> @@ -99,8 +99,10 @@ class Window_spec : public Sql_alloc
>    LEX_STRING *window_ref;
>
>    SQL_I_List<ORDER> *partition_list;
> +  SQL_I_List<ORDER> *save_partition_list;
>
>    SQL_I_List<ORDER> *order_list;
> +  SQL_I_List<ORDER> *save_order_list;
>
>    Window_frame *window_frame;
>
> @@ -111,7 +113,8 @@ class Window_spec : public Sql_alloc
>                SQL_I_List<ORDER> *ord_list,
>                Window_frame *win_frame)
>      : window_names_are_checked(false), window_ref(win_ref),
> -      partition_list(part_list), order_list(ord_list),
> +      partition_list(part_list), save_partition_list(NULL),
> +      order_list(ord_list), save_order_list(NULL),
>        window_frame(win_frame), referenced_win_spec(NULL) {}
>
>    virtual char *name() { return NULL; }
> _______________________________________________
> commits mailing list
> commits at mariadb.org
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.askmonty.org/pipermail/commits/attachments/20210720/f3a4bcf0/attachment-0001.html>


More information about the commits mailing list