From: Luís Soares Date: April 29 2010 3:44pm Subject: Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3356) Bug#46166 List-Archive: http://lists.mysql.com/commits/106962 Message-Id: <1272555848.9398.220.camel@sunlucid.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Hi Alfranio, On Thu, 2010-04-29 at 12:44 +0100, Alfranio Correia wrote: > Hi Luis, > > Nice work. Patch not approved yet. Thanks... > See some requests in what follows: > > > 1 - Check why the rpl_temporary_errors are failing as follow: > > #0 0x006b7422 in __kernel_vsyscall () > #1 0x004a1e93 in __pthread_kill (threadid=3077912256, signo=6) at ../nptl/sysdeps/unix/sysv/linux/pthread_kill.c:64 > #2 0x0871f299 in my_write_core (sig=6) at stacktrace.c:329 > #3 0x082bd594 in handle_segfault (sig=6) at mysqld.cc:2571 > #4 > #5 0x006b7422 in __kernel_vsyscall () > #6 0x0073d4d1 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 > #7 0x00740932 in *__GI_abort () at abort.c:92 > #8 0x00773ee5 in __libc_message (do_abort=2, fmt=0x837578 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:189 > #9 0x0077dff1 in malloc_printerr (action=, str=0x6
, ptr=0xa4e6c48) at malloc.c:6217 > #10 0x0077f6f2 in _int_free (av=, p=) at malloc.c:4750 > #11 0x007827cd in *__GI___libc_free (mem=0xa4e6c48) at malloc.c:3716 > #12 0x0872ee3e in FreeList (linkp=0x0) at dbug.c:1736 > #13 0x0872e946 in FreeState (cs=0xa4e43f0, state=0x8a49020, free_state=0) at dbug.c:1475 > #14 0x0872ebcb in _db_end_ () at dbug.c:1542 > #15 0x086fd4cb in my_end (infoflag=3) at my_init.c:220 > #16 0x082c0d28 in main (argc=10, argv=0xbfa7c364) at mysqld.cc:4579 > > Notice that I did not check if this is due to your patch. It's not due to my patch. This is a known issue and is tracked in: BUG#46165 and BUG#52629... > 2 - Please, fix the function find_uniq_filename(char *name) > which is not checking if anything bad is happening. Done. > > 3 - I did not follow which functions can fail in the > find_uniq_filename(char *name) and how they fail. So, verify > if it is necessary to introduce an error in the diagnostic area. > > DBUG_EXECUTE_IF("error_unique_log_filename", DBUG_RETURN(1);); Done. I see two functions that return errors (and are now handled): - my_dir - sprintf The others don't handle errors. > > 4 - Please, remember to remove the following code from >= 5.5 > because it is not true after WL#2687: > > + if (error) > + { > + /** > + Be conservative... There are possible lost events (eg, > + failing to log the Execute_load_query_log_event > + on a LOAD DATA while using a non-transactional > + table)! > + */ > + Incident incident= INCIDENT_LOST_EVENTS; > + Incident_log_event ev(current_thd, incident); > + /** > + We give it a shot and try to write an incident event anyway > + to the current log. > + */ > + (void) ev.write(&log_file); > + > + goto end; > + } > Hmm... Ok. Although it isn't me, who is going to merge this to trunk (I will for 6.0-codebase though). > 6 - Please, check the error code of the following function and augment the > test case to take this error into account: > > purge_logs_before_date(purge_time); Hmm... purge_logs_before_date pushes warnings not errors to the diagnostics area... If we force it to return errors then we will face an assertion later... Also, I'm not sure if such behavioral change is acceptable for 5.1! If you still want it, I suggest we open a bug for this and track it there (then we could ask for a different target version - a non GA). > 7 - Add doxygen Done. Added where I felt it made sense. > 8 - Have you checked with the runtime team if they are ok with the following change: > > +++ sql/handler.cc 2010-04-28 22:22:25 +0000 > @@ -1065,6 +1065,8 @@ > 1 transaction was rolled back > @retval > 2 error during commit, data may be inconsistent > + @retval > + 3 error during unlog Waiting for someone to answer my request for feedback. > 9 - Please, print the content of the index file for the following changes: > > --- mysql-test/suite/binlog/t/binlog_index.test 2009-12-08 16:03:19 +0000 > +++ mysql-test/suite/binlog/t/binlog_index.test 2010-04-28 22:22:25 +0000 > @@ -205,11 +205,11 @@ > --echo # This should put the server in unsafe state and stop > --echo # accepting any command. If we inject a fault at this > --echo # point and continue the execution the server crashes. > ---echo # Besides the flush command does not report an error. > --echo # > > --echo # fault_injection_registering_index > > ----> Print it here Done. > > SET SESSION debug="+d,fault_injection_registering_index"; > +-- error ER_CANT_OPEN_FILE > flush logs; > > ----> Print it here Done. > --source include/restart_mysqld.inc > > > @@ -221,6 +221,7 @@ > > --echo # fault_injection_updating_index > > ----> Print it here Not needed ^. > SET SESSION debug="+d,fault_injection_updating_index"; > +-- error ER_CANT_OPEN_FILE > flush logs; > > ----> Print it here Done. > > --source include/restart_mysqld.inc > > > 10 - I did not carefully check your test case to see if it covers > all the changes. I will do so in the next round. However, why don't > you combine it with binlog_index.test or vice-versa? Because of (at least): 1. This new test uses a master and a slave 2. This new test forces (in -master.opt) the max_binlog_size to be 4096, and this precondition requirement is rendered void in binlog_index tests context. I am testing the patch atm, please check if it meets your needs: - http://lists.mysql.com/commits/106961 I'll wait for Zhenxing's review and runtime feedback before formally committing a new one. Regards, Luís