On 21.03.11 22.26, Guilhem Bichot wrote:
>>> (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:
> with getting the lex directly from the unit, i.e.
> 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.
>> 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.
Hm, Ok. I did not know that derived tables needed a separate LEX.
>>> So if you approve the current refactoring patch, I wouldn't push the part
>>> re-enables EXPLAIN; the patch would be pure cleanup.
>> Yes, I will. Please commit a new patch with only the refactoring.
> 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.
Please do what you feel is the right think do you. I'm beginning to loose track
of the consequences ;)
>> 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?
Currently it is the only runtime context. But I do imagine that if plan objects
were made reentrant, we would need more runtime context. It could be a set of
TABLE pointers for the actual tables to use with query, and a pointer to a
memory object that contains the state for the execution (ie current join state,
current field buffers, current item expressions, etc).
And if we were to implement parallel execution of queries, the THD might be used
for multiple threads executing concurrently, so we might need to report errors
on a thread-specific error handler, etc.
Besides, the THD is a very coarse object that makes it difficult to do unit
testing. We might replace it with individual objects, e.g. an error object for
error reporting, a mem_root pointer for runtime memory allocation, a variable
object for session/system variables, etc.
And I think you agree with me that passing this stuff in a context object is
better than assigning it to the parse tree/query execution plan.