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

Roy Lyseng a écrit, Le 21.03.2011 13:35:
> 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 sql_select.cc, 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.cc
>>>> 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 item_subselect.cc, changed several
>> unit->thd
>> to be instead
>> unit->thd(),
>> with
>> st_select_lex_unit::thd() { return m_lex->thd; }.
>> In item_subselect.cc, 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));
>>
>> CREATE VIEW v2 AS
>> 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/sql_lex.cc:2656
>> 2656 DBUG_ASSERT(m_lex == m_lex->thd->lex);
> 
> Based on what I wrote above, I think that this assert is meaningless.

It is not meaningless if we want to replace the existing:
  unit->thd->lex
with getting the lex directly from the unit, i.e.
  unit->m_lex
Which I think was your suggestion (maybe I interpreted wrongly though).

That was the sense of this assertion: check that the replacement is a 
safe thing to do.

>> (gdb) bt
>> #3 0x00000000005fd40f in st_select_lex_unit::lex (this=0x1a8fc20)
>> at /m/bzrrepos_new/59852/sql/sql_lex.cc:2656
>> #4 0x0000000000814c87 in Item_subselect::const_item (this=0x1a90720)
>> at /m/bzrrepos_new/59852/sql/item_subselect.cc:453
>> #5 0x00000000007b59be in Item_in_optimizer::fix_fields (this=0x1a90340,
>> thd=0x19f2a00, ref=0x1a57640)
>> at /m/bzrrepos_new/59852/sql/item_cmpfunc.cc:1767
>> #6 0x00000000008142d0 in Item_subselect::fix_fields (this=0x1a90720,
>> thd=0x19f2a00, ref=0x1a57640)
>> at /m/bzrrepos_new/59852/sql/item_subselect.cc:223
>> #7 0x000000000081990f in Item_in_subselect::fix_fields (this=0x1a90720,
>> thd_arg=0x19f2a00, ref=0x1a57640)
>> at /m/bzrrepos_new/59852/sql/item_subselect.cc:1890
>> #8 0x00000000006c6d04 in TABLE_LIST::prep_where (this=0x1a574e0,
>> thd=0x19f2a00, conds=0x1a79d28, no_where_clause=false)
>> at /m/bzrrepos_new/59852/sql/table.cc:3572
>> #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/sql_base.cc:8288
>> #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/sql_select.cc:488
>> #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/sql_select.cc:570
>> #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 parser).

Dynamic allocation may also allow filling the unit with a call to its 
constructor, instead of having it "default empty" and then decorated 
over time.

> 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.

I tried to update unit->m_lex in mysql_make_view() when we do view 
merging. It seems to work but then I hit the same assert with a derived 
table. I didn't research further.

>> 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.

Done.
Given that the new patch doesn't fix the bug#59852 (as it's only 
cleaning up), I suspect it should be linked to a new bug report having a 
title which clearly says it's only about cleaning up...? My latest 
commit thus has no bug# yet.

Another minor issue: once pushed, this cleanup will make the life of the 
fixer of bug#11886060 more difficult: he won't be able to see a crash 
with the wrong code, as there won't be a "not updated 
subselect_engine::thd" anymore. On the other hand, he could observe that 
we're EXPLAINing an Item_subselect which has 'fixed==false', which is 
another symptom of the problem.
So I imagine I won't have to wait for bug#11886060 to be fixed before 
pushing my cleanup... I also fear that bug#11886060 may not be fixed soon.

> 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.

Isn't THD this runtime context?

>>>> === modified file 'sql/item_subselect.cc'
>>>> --- a/sql/item_subselect.cc 2011-01-18 11:42:09 +0000
>>>> +++ b/sql/item_subselect.cc 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 ;)

sorry, I should have written
thd == unit->m_lex->thd

but if I don't introduce m_lex, I leave
thd == unit->thd

>>
>>>> + // 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.
> 
> Thanks,
> Roy


-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
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