List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:March 21 2011 12:35pm
Subject:Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852
View as plain text  
Hi Guilhem,

I am CC'ing Magne, because he might become involved...

On 19.03.11 15.43, Guilhem Bichot wrote:
> Hello Roy,
> Roy Lyseng a écrit, Le 09.02.2011 12:51:
>> Hi Guilhem,
>> thank you for this nice refactoring of the optimizer data structs.
> Always a pleasure to delete member variables :-)
>> But I think that more work is needed before this patch can be approved.
>> On 31.01.11 22.51, Guilhem Bichot wrote:
>>> #At file:///home/mysql_src/bzrrepos_new/59852/ based on
>>> revid:roy.lyseng@stripped
>>> 3335 Guilhem Bichot 2011-01-31
>>> Fix for BUG#59852 "EXPLAIN EXTENDED + UNION + 'ORDER BY x IN (subquery)'
> does
>>> not show IN->EXISTS transformation":
>>> make EXPLAIN show it, and fix the resulting crash by getting rid of a
>>> redundant, bad THD pointer.
>>> @ mysql-test/r/union.result
>>> result. Without the change to, the last EXPLAIN would not show
>>> the IN->EXISTS transformation (would just show the original query).
>>> @ mysql-test/t/union.test
>>> test for bug
>>> @ sql/
>>> Item_subselect::thd and subselect_engine::thd are deleted.
>>> The first is replaced with the existing Item_subselect::unit::thd.
>>> The second one is replaced with the existing
> subselect_engine::item::unit::thd
>>> (same object as for the first one, actually: item points to the containing
>>> Item_subselect); for subselect_union_engine it's additionally possible to
> use
>>> subselect_union_engine::unit::thd.
>> This is a good change in the code base. You may also consider removing the thd
>> pointers from the st_select_lex_unit object. AFAIK, an st_select_lex_unit
>> object cannot exist without belonging to a LEX object, hence you may replace
>> the thd pointer with a LEX pointer and access the LEX's thd pointer through
>> this one instead. It will also mean that data in the LEX object can be
>> accessed without going through the THD pointer.
>> This change will cause far fewer updates to THD pointers to occur.
> I have explored this yesterday and today.
> I think you meant something like the experiment below.
> Added a "LEX *m_lex" member to st_select_lex_unit.
> Changed st_select_lex_unit::init_query() to take "LEX* parent"
> parameter, and do "m_lex= parent".
> In, changed several
> unit->thd
> to be instead
> unit->thd(),
> with
> st_select_lex_unit::thd() { return m_lex->thd; }.
> In, changed
> unit->thd->lex
> to
> unit->m_lex.
> But: unit->m_lex is not always == unit->m_lex->thd->lex.

thd->lex is just the "current lex" for the thd, so this can only be guaranteed 
if unit->m_lex is the only lex for the current thd.
> Here is a case study:
> CREATE TABLE t2 ( t2field integer, primary key (t2field));
> SELECT t2field as v2field
> FROM t2 A
> WHERE A.t2field IN (SELECT t2field FROM t2 );
> INSERT INTO t2 VALUES (2),(3),(4);
> SELECT v2field FROM v2;
> In this last SELECT, there is a unit, for which m_lex != m_lex->thd->lex:
> (gdb) f
> #3 0x00000000005fd40f in st_select_lex_unit::lex (this=0x1a8fc20)
> at /m/bzrrepos_new/59852/sql/
> 2656 DBUG_ASSERT(m_lex == m_lex->thd->lex);

Based on what I wrote above, I think that this assert is meaningless.

> (gdb) bt
> #3 0x00000000005fd40f in st_select_lex_unit::lex (this=0x1a8fc20)
> at /m/bzrrepos_new/59852/sql/
> #4 0x0000000000814c87 in Item_subselect::const_item (this=0x1a90720)
> at /m/bzrrepos_new/59852/sql/
> #5 0x00000000007b59be in Item_in_optimizer::fix_fields (this=0x1a90340,
> thd=0x19f2a00, ref=0x1a57640)
> at /m/bzrrepos_new/59852/sql/
> #6 0x00000000008142d0 in Item_subselect::fix_fields (this=0x1a90720,
> thd=0x19f2a00, ref=0x1a57640)
> at /m/bzrrepos_new/59852/sql/
> #7 0x000000000081990f in Item_in_subselect::fix_fields (this=0x1a90720,
> thd_arg=0x19f2a00, ref=0x1a57640)
> at /m/bzrrepos_new/59852/sql/
> #8 0x00000000006c6d04 in TABLE_LIST::prep_where (this=0x1a574e0,
> thd=0x19f2a00, conds=0x1a79d28, no_where_clause=false)
> at /m/bzrrepos_new/59852/sql/
> #9 0x00000000005be20c in TABLE_LIST::prepare_where (this=0x1a574e0,
> thd=0x19f2a00, conds=0x1a79d28, no_where_clause=false)
> at /m/bzrrepos_new/59852/sql/table.h:1745
> #10 0x00000000005bbea8 in setup_conds (thd=0x19f2a00, tables=0x1a574e0,
> leaves=0x1a91600, conds=0x1a79d28)
> at /m/bzrrepos_new/59852/sql/
> #11 0x000000000066c3d9 in setup_without_group (thd=0x19f2a00,
> ref_pointer_array=0x1ab1ff0, tables=0x1a574e0, leaves=0x1a91600,
> fields=..., all_fields=..., conds=0x1a79d28, order=0x0, group=0x0,
> hidden_group_fields=0x1a79be7)
> at /m/bzrrepos_new/59852/sql/
> #12 0x000000000062dbb3 in JOIN::prepare (this=0x1a79950,
> rref_pointer_array=0x19f4f70, tables_init=0x1a574e0, wild_num=0,
> conds_init=0x0, og_num=0, order_init=0x0, group_init=0x0, having_init=0x0,
> proc_param_init=0x0, select_lex_arg=0x19f4d58, unit_arg=0x19f4710)
> at /m/bzrrepos_new/59852/sql/
> #13 0x0000000000636951 in mysql_select (thd=0x19f2a00,
> This unit was created in mysql_make_view() (when the SELECT opened the view). In
> that function, we use a dedicated LEX. The view has a subquery inside, which
> means a new unit is created, and this unit gets the dedicated LEX as "m_lex"
> member. At the end of mysql_make_view(), we lex_end() this dedicated LEX.
> When we later SELECT from the view, we're using a different LEX (thd->lex).
> So unit->m_lex is not usable at this point.

There is also an "embedded" unit in the LEX object. It would be good if this 
unit would also be dynamically allocated. I'm discussing with Magne how to do 
that. (We both came up with the same idea, and it is a prerequisite if we want 
to have a proper interface for generating a parse tree and having a bottom-up 

The problem here is view merging. When the "units" of the referenced view(s) are 
merged into the original query, the LEX pointers will also need updating. AFAIK, 
this is the only place where units are moving.

> So while I agree that in theory, a unit belongs to a LEX, and that link should
> always persist unaltered, it seems that in the current MySQL code, LEXs come and
> go, and units get created in one LEX and used in another...
> So I don't think that it is wise to introduce select_lex_unit::m_lex.
> select_lex_unit::thd, on the other hand, is reliable. In the above scenario,
> there is a single THD, no problem. In scenarios where a unit is created by one
> THD and used by another THD (triggers in shared cache), code takes care to call
> reinit_stmt_before_use() which fixes select_lex_unit::thd. Looks like a working
> machinery.

The change would be to call a similar function when merging views, and then 
change the LEX pointers instead of THD pointers.
>>> Before this change, Item_subselect::thd and subselect_engine::thd
>>> were set to NULL in constructors, and Item_subselect::fix_fields()
>>> corrected them. As Item_subselect::thd is used in subquery
>>> transformations (in Item_singlerow_subselect::select_transformer()
>>> called by JOIN::prepare()), it was necessary that
>>> Item_subselect::fix_fields() was called before JOIN::prepare()
>>> for the subquery's JOIN. And often it works indeed this way,
>>> as Item_subselect::fix_fields() calls JOIN::prepare().
>>> But in
>>> "EXPLAIN select1 UNION select2 ORDER BY select3"
>>> (case of BUG 49734), we have four SELECT_LEX:
>>> select1-2-3 and the "fake UNION SELECT".
>>> EXPLAIN EXTENDED (mysql_explain_union()) calls JOIN::prepare()
>>> for select1, and select2.
>>> select3 is not in the ORDER BY clause of select2, so
>>> JOIN::prepare(select2) does not call Item_subselect::fix_fields().
>>> Then mysql_explain_union() calls JOIN::exec(select2) which itself
>>> calls select_describe(select2). That function calls
>>> mysql_explain_union() on "inner units" of select2, and select3 is
>>> among them. That is weird, by the way: select3 is not in ORDER BY of
>>> select2 (ok) but it's a inner unit of select2 (weird); is this a
>>> parsing artifact? Shouldn't it be a inner unit of the fake SELECT?
>>> So we go into JOIN::prepare(select3), which calls resolve_subquery()
>>> to transform, and crashes on a still-NULL Item_subselect::thd pointer.
>>> If the crash is "leaped over" in gdb, then finally
>>> JOIN::prepare() is called for the fake UNION SELECT, which owns the ORDER BY
>>> clause, so it's only at this late moment, precisely in setup_order(), that
>>> Item_subselect::fix_fields() is called which sets Item_subselect::thd
>>> correctly. Too late.
>>> See also the comment at start of st_select_lex::print()
>>> about thd being NULL.
>>> The advantage of using SELECT_LEX_UNIT::thd pointers is that they are
>>> always kept correct by reinit_stmt_before_use(). The NULL thd pointer
>>> is eliminated, we now use a correct one, which avoids the crash.
>>> One may wonder whether, even with no crash thanks to this patch, it
>>> is still a logical problem to have:
>>> - JOIN::prepare() called before Item_subselect::fix_fields().
>>> - select3 attached to select2 as inner unit but attached to fake
>>> SELECT as ORDER BY clause.
>>> Those oddities are a specificity of EXPLAIN UNION; without a UNION, in
>>> "EXPLAIN select 1 ORDER BY select3",
>>> select1 owns the ORDER BY clause, so setup_order() is called
>>> from JOIN::prepare(select1), which calls Item_subselect::fix_fields(),
>>> which calls JOIN::prepare() (JOIN of select3).
>>> It would be interesting to discuss this with reviewers.
>> I think that you have a good point here. This looks like another problem that
>> is worth looking into. It means that the "refactoring" part of your patch
>> (which I think is good) will actually hide the original problem.
> Right. As I posted in Bug#11766685:
> <quote>
> The wrong inner unit is filed as bug#11886060.
> The root cause of the present bug (11766685) is 11886060. 11886060 has
> the consequence that the ORDER BY subquery is not ready when we EXPLAIN it.
> The patch above makes the subquery have "less internal state", so it is
> in better shape when EXPLAIN happens (no crash), but it is still
> formally not ready. So might still cause still-unknown issues.
> So the patch above should be split in two pieces:
> 1) the cleanup which eliminates THD pointers
> 2) the enabling of subquery transformations in EXPLAIN.
> (1) is good per se, is a kind of small refactoring.
> (2) should not be done until 11886060 is fixed.
> I'll explore what further improvements can be done to (1) to have it
> approved.
> </quote>
> So if you approve the current refactoring patch, I wouldn't push the part which
> re-enables EXPLAIN; the patch would be pure cleanup.

Yes, I will. Please commit a new patch with only the refactoring.

Your suggestion is good. I think my suggestion (removing THD pointers) is even 
better, but long-term the LEX objects will disappear (yes they will!) and be 
replaced by subclasses of Sql_cmd, so replacing THD pointers with LEX pointers 
may not be such a good idea after all.

A separate issue is that we should avoid attaching THD pointers all over the 
place, and instead place it into a "runtime context" object which can provide 
the needed context for execution.
>>> === modified file 'sql/'
>>> --- a/sql/ 2011-01-18 11:42:09 +0000
>>> +++ b/sql/ 2011-01-31 21:51:42 +0000
>>> @@ -45,7 +45,7 @@ inline Item * and_items(Item* cond, Item
>>> }
>>> Item_subselect::Item_subselect():
>>> - Item_result_field(), value_assigned(0), thd(0), substitution(0),
>>> + Item_result_field(), value_assigned(0), substitution(0),
>>> engine(0), old_engine(0), used_tables_cache(0), have_to_be_excluded(0),
>>> const_item_cache(1), engine_changed(0), changed(0),
>>> is_correlated(FALSE)
>>> @@ -169,14 +169,25 @@ Item_subselect::select_transformer(JOIN
>>> }
>>> -bool Item_subselect::fix_fields(THD *thd_param, Item **ref)
>>> +bool Item_subselect::fix_fields(THD *thd, Item **ref)
>>> {
>>> - char const *save_where= thd_param->where;
>>> + char const *save_where= thd->where;
>>> uint8 uncacheable;
>>> bool res;
>>> DBUG_ASSERT(fixed == 0);
>>> - engine->set_thd((thd= thd_param));
>>> + /*
>>> + Pointers to THD must match. unit::thd may vary over the lifetime of the
>>> + item (for example triggers, and thus their Item-s, are in a cache shared
>>> + by all connections), but reinit_stmt_before_use() keeps it up-to-date,
>>> + which we check here. subselect_union_engine functions also do sanity
>>> + checks.
>>> + */
>>> + DBUG_ASSERT(thd == unit->thd);
>> Not needed if unit->thd goes away :)
> we can still assert that
> thd == unit->parent_lex->thd
> I find this is a good idea, for triggers which are stored in a shared cache.

It's st_select_lex that has parent_lex member, not st_select_lex_unit ;)

>>> + // Engine accesses THD via its 'item' pointer, check it:
>>> + engine->assert_item(this);
>> I am not sure that this is needed. Once set up, the optimizer structures
>> should be fairly rigid.
> I thought this too, so wanted to declare subselect_engine::item as:
> Item_subselect * const item;
> and set it once for all in the constructor.
> But alas, this 'item' can change later:
> bool subselect_single_select_engine::change_result(Item_subselect *si,
> select_result_interceptor *res)
> {
> item= si;
> Item_subselect::init() is using this change_result(). It seems to steal the
> engine from another Item_subselect.
> So the engine<->item relationship is not so rigid after all. Which is why I
> added the assertion, in case we get in problems.

Hm, OK.
>>> +
>>> + engine->set_thd_for_result();
>>> if (check_stack_overrun(thd, STACK_MIN_SIZE, (uchar*)&res))
>>> return TRUE;
>>> @@ -285,6 +296,7 @@ bool Item_subselect::exec()
>>> Do not execute subselect in case of a fatal error
>>> or if the query has been killed.
>>> */
>>> + THD * const thd= unit->thd;
>>> if (thd->is_error() || thd->killed)
>>> return 1;
>>> @@ -436,7 +448,7 @@ table_map Item_subselect::used_tables()
>>> bool Item_subselect::const_item() const
>>> {
>>> - return thd->lex->context_analysis_only ? FALSE : const_item_cache;
>>> + return unit->thd->lex->context_analysis_only ? FALSE :
> const_item_cache;
>>> }
>> Deleting unit->thd and adding unit->lex would actually simplify this
> sentence
>> (and others).
> Yes, but we have the unit->lex unreliability problem.
>>> Item *Item_subselect::get_tmp_table_item(THD *thd_arg)
> Please let me know whether the approach of this patch is now acceptable, given
> the experiments I made. I think that this patch does only good compared to the
> existing trunk: it deletes two "THD" pointers, and nowhere does it make code
> more obscure.

Yes, it is acceptable.

bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852Guilhem Bichot31 Jan
  • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852Roy Lyseng9 Feb
    • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852Guilhem Bichot19 Mar
      • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852Roy Lyseng21 Mar
        • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852Guilhem Bichot21 Mar
          • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852Roy Lyseng22 Mar