Hi Oystein,
I think it is a good idea to add the boolean flag to run_service_interface_sql()
and handle warnings inside. This is a convenience function and a perfect place
for placing the code there. Once this is done, I'd remove the copy_warnings()
function which is not really needed - the loop can be executed directly inside
run_service_interface_sql(). I also have a suggestion for improving
documentation of the new parameter.
I do not make it a formal review because it is not clear if you are going to
commit a new patch or not.
Oystein Grovlen wrote:
> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc 2008-12-06 00:02:44 +0000
> +++ b/sql/si_objects.cc 2008-12-17 10:27:06 +0000
> @@ -161,6 +161,25 @@
> ///////////////////////////////////////////////////////////////////////////
>
> /**
> + Update THD with the warnings from the given list.
> +
> + @param[in] thd Thread context.
> + @parampin] src Warning list.
> +*/
> +
> +void copy_warnings(THD *thd, List<MYSQL_ERROR> *src)
> +{
> + List_iterator_fast<MYSQL_ERROR> err_it(*src);
> + MYSQL_ERROR *err;
> +
> + while ((err= err_it++))
> + push_warning(thd, err->level, err->code, err->msg);
This loop could be executed directly inside run_service_interface_sql...
> +}
> +
> +///////////////////////////////////////////////////////////////////////////
> +///////////////////////////////////////////////////////////////////////////
> +
> +/**
> Execute one DML statement in a backup-specific context. Result set and
> warning information are stored in the output parameter. Some session
> attributes are preserved and reset to predefined values before query
> @@ -168,7 +187,8 @@
>
> @param[in] thd Thread context.
> @param[in] query SQL query to be executed.
> - @param[out] ed_result A place to store result and warnings.
> + @param[in] get_warnings If true, update THD with warnings.
"Update THD with warnings" can be unclear. Perhaps "copy warnings to the error
stack" would be better. In either case, warnings and errors are stored in
ed_connection object - perhaps good to mention it too.
> + @param[out] ed_connection A place to store result and warnings.
>
> @return Error status.
> @retval TRUE on error.
> @@ -177,7 +197,7 @@
>
> bool
> run_service_interface_sql(THD *thd, Ed_connection *ed_connection,
> - const LEX_STRING *query)
> + const LEX_STRING *query, bool get_warnings)
> {
> Si_session_context session_context;
>
> @@ -191,6 +211,11 @@
>
> bool rc= ed_connection->execute_direct(*query);
>
> + if (get_warnings) {
> + /* Push warnings on the THD error stack. */
> + copy_warnings(thd, ed_connection->get_warn_list());
The loop could be executed directly here, instead of calling the function.
> + }
> +
> session_context.restore_si_ctx(thd);
>
> DBUG_RETURN(rc);
Rafal
