List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:March 19 2011 2:43pm
Subject:Re: bzr commit into mysql-trunk branch (guilhem.bichot:3335) Bug#59852
View as plain text  
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.

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);
(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.

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.

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

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

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

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