|
So, the plan is
- combine select handler and derived handler into one handler that can handle both top level and subquery selects.
- allow the (new universal) select handler to be used for individual selects in a union (and other set operations)
- make the server to try to push the complete select (with union inside) and if that is rejected — try to push individual selects in a union
|
|
During yesterday call with drrtuy we agreed to modify the ColumnStore select handler so it accepts SELECT_LEX_UNIT* instead of SELECT_LEX*:
select_handler* create_columnstore_select_handler(THD* thd, SELECT_LEX* select_lex)
|
to
select_handler* create_columnstore_select_handler(THD* thd, SELECT_LEX_UNIT* select_lex)
|
This single select handler must process all possible cases: unions, single selects, derived tables.
|
|
A note: Spider supports group_by_handler, but not select_handler or derived_handler. So, implementation of this MDEV as we currently see it will not affect Spider. (Although one might wonder whether group_by_handler has some commonality with derived_handler or select_handler that could be factored out in the future).
|
|
psergei GBH is very different to what SH and DH are. It has a certain interface sending down the projected columns list for example. I don't see there is a way to somehow combine these three.
|
|
just an observation: pushdown of union members seems easy to do: psergey-pushdown-union-members.diff (Note: this patch doesn't include the needed changes for federatedx. it still prints the whole query text when pushing down).
|
|
We need to decide which output of EXPLAIN FORMAT=JSON we would like to see. What about something like this?
MariaDB [mdev25080]> explain format=json select a from t1 except select a+2 from t1 union select a-10 from t1;
|
+----------------------------------------------------------------------------------------------------+
|
| EXPLAIN |
|
+----------------------------------------------------------------------------------------------------+
|
| {
|
"query_block": {
|
"union_result": {
|
"pushed down to storage engine": true
|
}
|
}
|
} |
|
|
|
Branch bb-10.11-MDEV-25080 is ready for review.
N.B: don't merge until the ColumnStore implementation is ready.
|
|
Hi Oleg,
First piece of review input:
Looking at this function
static select_handler *create_federatedx_handler(THD *thd, T *sel_lex)
|
I see that it checks all the tables in the query. That is, if one tries to run
select * from federated_table union select * from innodb_table;
|
no pushdown is done.
Please fix this. I would suggest to create a recursive function that walks local tables and calls itself for child subqueries, as we've discussed on the previous calls.
|
|
More review input: https://lists.launchpad.net/maria-developers/msg13214.html
|
|
Looking at joining derived_handler and select_handler together...
print_error implementations look odd.
Both select_handler and derived_handler define them...
void derived_handler::print_error(int error, myf errflag)
|
{
|
my_error(ER_GET_ERRNO, MYF(0), error, hton_name(ht)->str);
|
}
|
|
void select_handler::print_error(int error, myf errflag)
|
{
|
my_error(ER_GET_ERRNO, MYF(0), error, hton_name(ht)->str);
|
}
|
Overloads:
void ha_federatedx_derived_handler::print_error(int, unsigned long)
|
{}
|
|
void ha_federatedx_select_handler::print_error(int error, myf error_flag)
|
{
|
select_handler::print_error(error, error_flag);
|
}
|
void ha_columnstore_derived_handler::print_error(int, unsigned long)
|
{}
|
|
// ha_columnstore_select_handler doesn't overload print_error.
|
|
|
ther difference ins prepare() method:
select_handler defines a virtual prepare():
bool select_handler::prepare()
|
{
|
DBUG_ENTER("select_handler::prepare");
|
/*
|
Some engines (e.g. XPand) initialize "table" on their own.
|
So we need to create a temporary table only if "table" is NULL.
|
*/
|
if (!table && !(table= create_tmp_table(thd)))
|
DBUG_RETURN(true);
|
DBUG_RETURN(table->fill_item_list(&result_columns));
|
}
|
ColumnStore overloads it in a dumb way:
// Copy of select_handler::prepare (see sql/select_handler.cc),
|
// with an added if guard
|
bool ha_columnstore_select_handler::prepare()
|
|
|
I've force-pushed a new revision, please find it in bb-10.11-MDEV-25080. One comment on the cosmetic review:
1.
> 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.
>>> No, it's not a code cleanup. There is
select->join->result->send_eof()
|
against
. The select_handler::result class member is introduced to support both SELECT_LEX and SELECT_LEX_UNIT.
|
|
Note: print_error functions don't have any test coverage:
diff --git a/storage/federatedx/federatedx_pushdown.cc b/storage/federatedx/federatedx_pushdown.cc
|
index 8870aef889d..2bbfaec51b9 100644
|
--- a/storage/federatedx/federatedx_pushdown.cc
|
+++ b/storage/federatedx/federatedx_pushdown.cc
|
@@ -214,6 +214,7 @@ int ha_federatedx_derived_handler::end_scan()
|
|
void ha_federatedx_derived_handler::print_error(int, unsigned long)
|
{
|
+ DBUG_ASSERT(0);
|
}
|
|
|
@@ -363,5 +364,6 @@ int ha_federatedx_select_handler::end_scan()
|
|
void ha_federatedx_select_handler::print_error(int error, myf error_flag)
|
{
|
+ DBUG_ASSERT(0);
|
select_handler::print_error(error, error_flag);
|
}
|
This doesn't fire ever.
|
|
There will be two reviewers from our side, namely tntnatbry and me. AFAIK Gagan will start looking into the updated patch today once again.
|
|
Hi oleg.smirnov, looks like adding paranthesis to the queries is not resulting in the pushdown of the whole union:
Q1:
EXPLAIN (SELECT * FROM federated.t1 UNION
|
SELECT * FROM federated.t2) UNION ALL
|
SELECT * FROM federated.t1;
|
|
id select_type table type possible_keys key key_len ref rows Extra
|
1 PRIMARY <derived2> ALL NULL NULL NULL NULL 8
|
2 PUSHED DERIVED NULL NULL NULL NULL NULL NULL NULL NULL
|
4 PUSHED SELECT NULL NULL NULL NULL NULL NULL NULL NULL
|
Q2:
EXPLAIN (SELECT * FROM federated.t1 UNION
|
SELECT * FROM federated.t2) UNION ALL
|
(SELECT * FROM federated.t1 UNION
|
SELECT * FROM federated.t2);
|
|
id select_type table type possible_keys key key_len ref rows Extra
|
1 PRIMARY <derived2> ALL NULL NULL NULL NULL 8
|
2 PUSHED DERIVED NULL NULL NULL NULL NULL NULL NULL NULL
|
6 UNION <derived4> ALL NULL NULL NULL NULL 8
|
4 PUSHED DERIVED NULL NULL NULL NULL NULL NULL NULL NULL
|
If the paranthesis are removed, the whole query is pushed down:
Q3 (Q1 without paranthesis):
EXPLAIN SELECT * FROM federated.t1 UNION
|
SELECT * FROM federated.t2 UNION ALL
|
SELECT * FROM federated.t1;
|
|
id select_type table type possible_keys key key_len ref rows Extra
|
NULL PUSHED UNION NULL NULL NULL NULL NULL NULL NULL NULL
|
Q4 (Q2 without paranthesis):
EXPLAIN SELECT * FROM federated.t1 UNION
|
SELECT * FROM federated.t2 UNION ALL
|
SELECT * FROM federated.t1 UNION
|
SELECT * FROM federated.t2;
|
|
id select_type table type possible_keys key key_len ref rows Extra
|
NULL PUSHED UNION NULL NULL NULL NULL NULL NULL NULL NULL
|
Here, t1 and t2 are federatedx tables (taken from the federatedx_create_handlers.test MTR test case in preview-10.11-mdev-25080-union-pushdown server branch):
SELECT * FROM federated.t1;
|
abc
|
bcd
|
cde
|
|
SELECT * FROM federated.t2;
|
abc
|
bcd
|
cde
|
def
|
efg
|
|
|
Good points, tntnatbry. I have pushed a quick fix into bb-10.11-mdev-25080-fixes branch, hope this helps. psergei, please review when possible.
|
|
Pushed down UNION on two tables containing two different INT types with different signedness has an incorrect output. In the below, output of Q2 is incorrect. Output of Q1 is correct, however. I have not tested other combinations of numeric types but those could also be in error.
SHOW CREATE TABLE federated.t1;
|
|
Table Create Table
|
t1 CREATE TABLE `t1` (
|
`a` smallint(6) NOT NULL
|
) ENGINE=FEDERATED DEFAULT CHARSET=latin1 CONNECTION='mysql://root@127.0.0.1:16001/federated/t1'
|
|
SHOW CREATE TABLE federated.t2;
|
|
Table Create Table
|
t2 CREATE TABLE `t2` (
|
`a` int(10) unsigned NOT NULL
|
) ENGINE=FEDERATED DEFAULT CHARSET=latin1 CONNECTION='mysql://root@127.0.0.1:16001/federated/t2'
|
|
SELECT * FROM federated.t1;
|
|
a
|
-32768
|
-1
|
0
|
|
SELECT * FROM federated.t2;
|
|
a
|
0
|
1
|
32767
|
Q1:
SELECT a FROM federated.t1 UNION ALL SELECT a FROM federated.t2;
|
|
a
|
-32768
|
-1
|
0
|
0
|
1
|
32767
|
Q2:
SELECT a FROM federated.t2 UNION ALL SELECT a FROM federated.t1;
|
|
a
|
0
|
1
|
32767
|
0
|
0
|
0
|
CREATE TABLE t3 AS SELECT a FROM federated.t1 UNION ALL SELECT a FROM federated.t2;
|
CREATE TABLE t4 AS SELECT a FROM federated.t2 UNION ALL SELECT a FROM federated.t1;
|
Warnings:
|
Warning 1264 Out of range value for column 'a' at row 0
|
Warning 1264 Out of range value for column 'a' at row 0
|
|
|
tntnatbry, I think the problem is the table `t2` is connected to remote table `t1`:
Table Create Table
|
t2 CREATE TABLE `t2` (
|
`a` int(10) unsigned NOT NULL
|
) ENGINE=FEDERATED DEFAULT CHARSET=latin1 CONNECTION='mysql://root@127.0.0.1:16001/federated/t1'
|
As far as I understand, the FederatedX engine may not be able to process such cases when local and remote tables have different field types.
If it's just a typo, please check the queries on a correct test case.
|
|
Hi oleg.smirnov, Sorry for the typo earlier. I fixed the table DDL for t2 in the comment above and reran the test case. I still get the output as above. So this is indeed a bug.
|
|
I agree, the bug really exists. I believe it's not related to the UNION pushdown improvement but to the FederatedX engine. I observed even a more strange result with disabled pushdown option:
MariaDB [mdev25080]> set global federated_pushdown=0;
|
Query OK, 0 rows affected (0.000 sec)
|
|
MariaDB [mdev25080]> SELECT a FROM federated.t2 UNION ALL SELECT a FROM federated.t1;
|
+---+
|
| a |
|
+---+
|
| 2 |
|
| 3 |
|
| 4 |
|
| 5 |
|
| 1 |
|
| 2 |
|
| 3 |
|
+---+
|
7 rows in set (0.003 sec)
|
|
MariaDB [mdev25080]> EXPLAIN SELECT a FROM t12 UNION ALL SELECT a FROM t11;
|
+------+-------------+-------+------+---------------+------+---------+------+------+-------+
|
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
|
+------+-------------+-------+------+---------------+------+---------+------+------+-------+
|
| 1 | PRIMARY | t12 | ALL | NULL | NULL | NULL | NULL | 4 | |
|
| 2 | UNION | t11 | ALL | NULL | NULL | NULL | NULL | 3 | |
|
+------+-------------+-------+------+---------------+------+---------+------+------+-------+
|
2 rows in set (0.002 sec)
|
I'll check this more thoroughly but I believe this shouldn't be an obstacle to test the integration with ColumnStore.
|
|
oleg.smirnov, unfortunately ColumnStore has the same problem. This is a server-side bug, not specific to a storage engine. The temporary table that the server creates for this query to store the results in table->record[0] has incorrect Field attributes.
|
|
Thank you, tntnatbry, this is indeed a bug. The good news it was relatively easy to fix but also there is a case that I haven't fixed yet:
SELECT * FROM t2 UNION SELECT * FROM t1 UNION ALL SELECT 1 ;
|
Partial pushdown of queries including "SELECT <const-value>" produces wrong results. Will keep you updated on the progress.
|
|
There's a memory leak, visible in asan builds in the preview branch. For example here: https://buildbot.askmonty.org/buildbot/builders/kvm-asan/builds/9060/steps/mtr_nm/logs/mysqld.1.err.2 (search for the work "leak")
|
|
serg, after some examination I think this bug is not related to the improvements made for the UNION pushdown. I've filed a separate MDEV-29624 for this.
|
|
tntnatbry, I believe I've addressed the bug you discovered. The fix is pushed to bb-10.11-mdev-25080-fix1.
|
|
According to serg 's message on Slack, it won't happen in 11.0.
|
|
Pushed to 11.1
|
psergey-pushdown-union-members.diff