List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:January 20 2011 8:29pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)
View as plain text  
Hello,

Davi Arnaut a écrit, Le 19.01.2011 21:33:
> On 1/19/11 6:14 PM, Guilhem Bichot wrote:
>> Hello,
>>
>> Davi Arnaut a écrit, Le 19.01.2011 20:08:
>>> On 1/19/11 4:58 PM, Guilhem Bichot wrote:
>>>> #At
>>>> file:///home/mysql_src/bzrrepos_new/mysql-next-mr-opt-backporting-wl4800/
> 
>>>>
>>>> based on 
>>>> revid:guilhem.bichot@stripped
>>>>
>>>> 3260 Guilhem Bichot 2011-01-19
>>>> todo question
>>>>
>>>> modified:
>>>> WL4800_TODO.txt
>>>> === modified file 'WL4800_TODO.txt'
>>>> --- a/WL4800_TODO.txt 2011-01-17 20:41:34 +0000
>>>> +++ b/WL4800_TODO.txt 2011-01-19 18:58:53 +0000
>>>> @@ -44,3 +44,7 @@ Make --opt-trace-protocol dump traces to
>>>> can run with it without failing all tests.
>>>>
>>>> Update copyright header of all changed files to reflect changed in 2011
>>>> +
>>>> +should the debug binary really assert(0) if json syntax error? is it a
>>>> +good idea at the customer's? On the other hand, how to make sure a
>>>> +developer notices a syntax error when running tests?
>>>>
>>>
>>> Perhaps it is time for us to have a DEBUG_WARN?
>>
>> What would this macro do?
> 
> Emit a warning! :-)

I guess it would call sql_print_warning() then. fprintf(stderr, 
"Warning") is maybe not guaranteed to reach the log in all platforms and 
all setups.

>> The context is that if a customer/support is troubleshooting a
>> problematic query, using the optimizer trace and a debug binary, it may
>> happen that she/he hits a case where the trace is not JSON-compliant
> 
> I assumed that since you mentioned debug binaries and assert(0) a 
> customer/support using it is the most rare case and actually a bit 
> dangerous. The main and ultimate audience of debug binaries are developers.

I used to think so, when recently I got a support case where the 
customer had installed the debug binary, to inspect a crash in the 
Optimizer, in coordination with Support.
That's why I was wondering whether it was really a good idea that a 
slightly incorrect optimizer trace would cause suicide of the server, 
thus preventing the customer from reading this trace, which may have 
been helpful for troubleshooting the optimizer's problem.

>> (we, optimizer team, cannot guarantee that we have studied *all* code
>> paths which can lead to generation of a trace; there are many more than
>> what is exercised by the full mtr testsuite).
>> In that case, for the customer, the invalid-syntax trace is still worth
>> looking at, informative, and a DBUG_ASSERT(0) is then a pure annoyance.
> 
> Do not worry about customers and debug binaries. We should be asking, is 
> this useful for developers? In any case, even if a customer is running a 
> debug binary, the target of the said test would be to send information 
> to a developer. In this case, a DBUG_ASSERT(0) would be useful to the 
> developer?

DBUG_ASSERT(0) would tell us that the trace is not JSON-compliant. Which 
gives us a bug to fix (we want traces to always be JSON-compliant). On 
the other hand, for the customer's case, the most urgent task is not 
fixing this bug but fixing the first reported optimizer problem which 
led to filing a Support incident (the bad query plan, the crash in 
production binaries, etc).

>> On the other hand, when I, developer, am modifying the optimizer and
>> running the testsuite, I certainly want to be told if my changes lead to
>> an invalid trace, and for this, DBUG_ASSERT(0) is perfect (it makes the
>> test fail).
> 
> I think you have the answer you are looking for :)
> 
>> Maybe we could do a sql_print_warning() instead of DBUG_ASSERT(0); mtr
>> warns when it sees an unexpected warning, it marks the test as failed,
>> prints the warning on stderr, and returns 1 as exit code. I guess this
>> would be good enough?
> 
> I think it depends on the gravity of the occurrence. For example, if the 
> unmet condition can cause significant trouble (i.e. crash) in a 
> production binary, it warrants a assertion. If not, emitting a warning 
> would be more appropriate.

In a production binary, if the trace is not JSON-compliant, it's not 
fatal, we don't generate a crash (we just note it somewhere in the trace 
as an indication).
So it's not a severe issue, it does not mandate a crash.

> Drawing a comparison, the Linux kernel has a pair of macros for those 
> two scenarios, WARN_ON() and BUG_ON(). The latter stops the machine by 
> generating an intentional trap, while the latter just prints a detailed 
> message to system log. BUG_ON() is used when the error is unrecoverable 
> and fatal, while WARN_ON() is used for recoverable errors/bugs that 
> should be addressed somehow.

Right, a non JSON-compliant trace is something to address for us but not 
an emergency.

> Since MTR detects warnings written to the error log, this logic could be 
> applied for MySQL as well. Whether to warn or to panic would depend on 
> the gravity of the failed assumption. Perhaps, also on the amount of 
> information needed to investigate the failed condition, as generating a 
> trap will give us a nice core file.

If I replace DBUG_ASSERT(0) with a warning, and an MTR tests shows this 
warning, I can restart the test with --gdb and put a breakpoint in 
sql_print_warning(). If I am in the capacity of restarting the test 
(which may not be easy, if in pb2).
I need to think about all this :-)
Thanks for the ideas!
Thread
bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260) Guilhem Bichot19 Jan
Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)Davi Arnaut19 Jan
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)Guilhem Bichot19 Jan
    • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)Davi Arnaut19 Jan
      • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)Guilhem Bichot20 Jan
    • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)Bjorn Munch19 Jan
      • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem.bichot:3260)Davi Arnaut19 Jan