List:Commits« Previous MessageNext Message »
From:Alexander Nozdrin Date:October 31 2007 2:21pm
Subject:Re: bk commit into 5.1 tree (gkodinov:1.2615) BUG#31866
View as plain text  
Hi,

thanks for working on this.

The patch generally is Ok with me, however I have few minor notes.
You can ignore them if you don't like them.

On Tuesday 30 October 2007 18:36, kgeorge@stripped wrote:
> ChangeSet@stripped, 2007-10-30 17:36:09+02:00, gkodinov@stripped +3 -0
>   Bug #31866: MySQL Server crashes on SHOW CREATE TRIGGER statement
>   
>   SHOW CREATE TRIGGER was not checking for detected errors 
>   opening/reading the trigger filer. 

A typo here (s/filer/file).

> diff -Nrup a/mysql-test/t/trigger.test b/mysql-test/t/trigger.test
> --- a/mysql-test/t/trigger.test	2007-08-15 11:18:40 +03:00
> +++ b/mysql-test/t/trigger.test	2007-10-30 17:36:08 +02:00
> @@ -2246,3 +2246,11 @@ select * from table_25411_a;
>  drop table table_25411_a;
>  drop table table_25411_b;
>  
> +#
> +# Bug #31866: MySQL Server crashes on SHOW CREATE TRIGGER statement
> +#
> +
> +--error ER_TRG_DOES_NOT_EXIST
> +SHOW CREATE TRIGGER a;

Could you please use "the trigger standard name" and our usual technique
to check that the trigger does not exist. I'm not sure if it is usable,
but we try to stick to it to be able to test on the user databases.

I mean, something like this:

--disable-warnings
DROP TRIGGER IF EXISTS trg;
--enable-warnings

--error ER_TRG_DOES_NOT_EXIST
SHOW CREATE TRIGGER trg;

> diff -Nrup a/sql/sql_show.cc b/sql/sql_show.cc
> --- a/sql/sql_show.cc	2007-10-23 11:20:49 +03:00
> +++ b/sql/sql_show.cc	2007-10-30 17:36:08 +02:00
> @@ -6860,7 +6860,10 @@ static TABLE_LIST *get_trigger_table(THD
>  
>  bool show_create_trigger(THD *thd, const sp_name *trg_name)
>  {
> -  TABLE_LIST *lst= get_trigger_table(thd, trg_name);
> +  TABLE_LIST *lst;
> +
> +  if (!(lst= get_trigger_table(thd, trg_name)))
> +    return TRUE;

Could you please rewrite this to:

  if (!lst)
     return TRUE;

This way, it would be easier to debug this code (in particular,
to examine the value of 'lst' in a debugger).

Thank you!

-- 
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com
Thread
bk commit into 5.1 tree (gkodinov:1.2615) BUG#31866kgeorge30 Oct
  • Re: bk commit into 5.1 tree (gkodinov:1.2615) BUG#31866Alexander Nozdrin31 Oct
    • Re: bk commit into 5.1 tree (gkodinov:1.2615) BUG#31866Georgi Kodinov1 Nov