Hi Davi
If I understand correctly, thd->profiling should be instrumented
properly with:
- thd->profiling.start_new_query();
- thd->profiling.finish_current_query();
at the *outermost* entry points in the server.
There are 2 entry points:
- handle_bootstrap
- do_command
I agree with the change in handle_bootstrap(), as it seems this path was
not working properly.
Now, I would like to understand why moving the instrumentation from
do_command()
down to dispatch_command() fixes the reported bug,
and why narrowing the scope of the instrumented code is considered a
good change.
Could the reported bug be fixed by changing handle_bootstrap() alone ?
Thanks,
Marc
Davi Arnaut wrote:
> # At a local mysql-5.1-bugteam repository of davi
>
> 2696 Davi Arnaut 2008-08-13
> Bug#38560: valgrind warnings on PB due to query profiling
>
> Backport 6.0 fix for this problem:
>
> Fix for a valgrind warning due to a jump on a uninitialized
> variable. The problem was that the sql profile preparation
> function wasn't being called for all possible code paths
> of query execution. The solution is to move the preparation
> to the dispatch_command function and to explicitly call the
> profile preparation function on bootstrap.
> modified:
> sql/sql_parse.cc
>
> per-file messages:
> sql/sql_parse.cc
> Move profile prepare of new query to dispatch_command.
> This is safe because dispatch_command is not reentrant.
> Finish query profiling properly when executing bootstrap
> commands.
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2008-08-07 02:23:58 +0000
> +++ b/sql/sql_parse.cc 2008-08-13 16:13:47 +0000
> @@ -441,6 +441,7 @@ pthread_handler_t handle_bootstrap(void
> thd->query[length] = '\0';
> DBUG_PRINT("query",("%-.4096s",thd->query));
> #if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
> + thd->profiling.start_new_query();
> thd->profiling.set_query_source(thd->query, length);
> #endif
>
> @@ -456,6 +457,10 @@ pthread_handler_t handle_bootstrap(void
> bootstrap_error= thd->is_error();
> net_end_statement(thd);
>
> +#if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
> + thd->profiling.finish_current_query();
> +#endif
> +
> if (bootstrap_error)
> break;
>
> @@ -733,11 +738,7 @@ bool do_command(THD *thd)
>
> net_new_transaction(net);
>
> - packet_length= my_net_read(net);
> -#if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
> - thd->profiling.start_new_query();
> -#endif
> - if (packet_length == packet_error)
> + if ((packet_length= my_net_read(net)) == packet_error)
> {
> DBUG_PRINT("info",("Got error %d reading command from socket %s",
> net->error,
> @@ -794,9 +795,6 @@ bool do_command(THD *thd)
> return_value= dispatch_command(command, thd, packet+1, (uint) (packet_length-1));
>
> out:
> -#if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
> - thd->profiling.finish_current_query();
> -#endif
> DBUG_RETURN(return_value);
> }
> #endif /* EMBEDDED_LIBRARY */
> @@ -898,6 +896,10 @@ bool dispatch_command(enum enum_server_c
> DBUG_ENTER("dispatch_command");
> DBUG_PRINT("info",("packet: '%*.s'; command: %d", packet_length, packet,
> command));
>
> +#if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
> + thd->profiling.start_new_query();
> +#endif
> +
> thd->command=command;
> /*
> Commands which always take a long time are logged into
> @@ -1519,6 +1521,11 @@ bool dispatch_command(enum enum_server_c
> VOID(pthread_mutex_unlock(&LOCK_thread_count));
> thd->packet.shrink(thd->variables.net_buffer_length); // Reclaim some
> memory
> free_root(thd->mem_root,MYF(MY_KEEP_PREALLOC));
> +
> +#if defined(ENABLED_PROFILING) && defined(COMMUNITY_SERVER)
> + thd->profiling.finish_current_query();
> +#endif
> +
> DBUG_RETURN(error);
> }
>
>
>
>
