Hi,
New patch is ready. Most of your requests satisfied, need some more
info, though.
Dmitry Lenev skrev:
> Hello Martin!
>
> ...
>
>> - This extends the use for a Table_triggers_list and we have to document
>> it carefully in the code. Preferrably there should be a flag for each
>> trigger whether it contains parse errors or not, and any other operation
>> than DROP TRIGGER should be refused with an obvious error.
>>
>
> Agree. Actually having one such flag for the whole Table_triggers_list,
> in my opinion, should be enough (Something like m_has_broken_triggers).
> If this flag is set we should disallow any DML which can invoke triggers
> and DDL which works with information about triggers except DROP TRIGGER
> and probably DROP TABLE (e.g. CREATE TRIGGER should be prohibited since
> we won't be able to determine if trigger is being created has same event
> type and time as one of triggers which we have failed to parse).
> Statements which don't invoke triggers (e.g. SELECT) can be allowed.
>
Done.
> Also I think it makes sense to mention name of un-parseable trigger in
> error message which will be emitted in these cases (to do this we can
> add names of such triggers to special list in Table_triggers_list).
>
This seems nice, but we'd have to invent another error message and I
have no idea what the correct procedure is for that. Seems outside the
scope of a bug fix IMHO. The bug reporter wishes to be able to drop
triggers that use deprecated syntax and that is possible now.
> Another issue which you have not mentioned but which I think is fairly
> important is the question "How can user get names of triggers which we
> have failed to parse and which it should drop?". As minimum we need to
> mention these names (or at least one of them) in an error message which
> will be emitted when one tries to execute DML on table with such
> triggers. Also, I think, it would be nice if SHOW TRIGGERS and
> SELECT ... FROM I_S.TRIGGERS produced warnings mentioning these names
> as well (they probably can't be included into normal output of these
> statements as we are missing too much information for such triggers).
>
Yes, that would be nice, but again we would have to invent a new
message. Unless you know some existing message that fits of course, but
I can't think of any.
> ...
> To make this test more portable it is better to use for the above check
> mysqltest's --list_files command (see mysqltest.test for example of its
> usage).
>
done.
>
>> +
>> +DROP TABLE t1, t2;
>>
>
> I think it is a good idea to add test for situation when table
> has several triggers defined on it some of which are parseable
> and some not.
>
done.
> Also we need to add test coverage for attempts to execute DML
> and DDL on tables with unparseable triggers once we add an
> appropriate check and start emitting error in such cases.
>
done.
> ...
>
>
>>
>
> Hmm... What will happen if we add incompatibility between versions which
> will cause different error to be produced (e.g. ER_SP_BADSTATEMENT)?
>
I don't know this error. It sounds like it has to do with SP's. But that
doesn't collide with trigger parsing, or does it?
> (Or may be we already have such incompatibilities?)
> Probably it makes sense to make this code more future-proof by trying
> to ignore all non-fatal errors (i.e. check for "! thd->is_fatal_error"
> instead of particular error code).
>
Not a good idea. If you do that you a get parse error for SHOW TRIGGERS.
> ...
>> - if (parse_sql(thd, & parser_state, creation_ctx))
>> - {
>> - /* Currently sphead is always deleted in case of a parse error */
>> - DBUG_ASSERT(lex.sphead == 0);
>> - goto err_with_lex_cleanup;
>> - }
>> + Old_syntax_trigger_handler error_handler;
>> + thd->push_internal_handler(&error_handler);
>> + bool parse_error= parse_sql(thd, & parser_state, creation_ctx);
>> + thd->pop_internal_handler();
>> +
>> /*
>> Not strictly necessary to invoke this method here, since we know
>> that we've parsed CREATE TRIGGER and not an
>> @@ -1317,6 +1347,36 @@ bool Table_triggers_list::check_n_load(T
>> */
>> lex.set_trg_event_type_for_tables();
>>
>> + if (parse_error)
>> + {
>> + /* Currently sphead is always deleted in case of a parse error */
>> + DBUG_ASSERT(lex.sphead == 0);
>> + if (error_handler.got_trigger_name())
>> + {
>> + if
> (triggers->names_list.push_back(&error_handler.trigger_name,
>> + &table->mem_root))
>> + goto err_with_lex_cleanup;
>> + else
>> + {
>> + /*
>> + The Table_triggers_list is not constructed as a list of
>> + trigger objects as one would expect, but rather of lists of
>> + properties of equal length. Thus, even if we don't get the
>> + trigger name, we still fill all in all the lists with
>> + placeholders as we might otherwise create a skew in the
>> + lists. Obviously, this has to be refactored.
>> + */
>> + LEX_STRING empty= thd->alloc_lex_string(NULL, "", 0, TRUE);
>> + if (triggers->names_list.push_back(empty,
> &table->mem_root) ||
>> + triggers->on_table_names_list.push_back(empty,
>> +
> &table->mem_root))
>> + goto err_with_lex_cleanup;
>>
>
> Ouch... I think something is wrong about the above code.
> This branch is executed only if error_handler.got_trigger_name()
> returns TRUE. But the comment above seems to assume otherwise.
>
Okay, strictly speaking it's not deleted, but set = NULL. Comment fixed.
Best Regards
Martin
