Hello,
* Alexander Nozdrin <alik@stripped> [08/07/18 20:00]:
There is one last design issue that I've just stumbled over.
In Item_param::set_value you iterate over result_type() of the item.
Ideally you should use the underlying field. This is necessary
to preserve accurate out parameters metadata in case of the binary
protocol.
I.e. right now max_length and precision of the out parameter
is not available to the binary protocol client.
Perhaps a solution is to pass this data over in Out_param_info,
and pass Out_param_info as an additional member of set_value()..
> 2675 Alexander Nozdrin 2008-07-18
> The second patch for WL#4435: Support OUT-parameters in prepared statements.
Please write a short description of the implementation to the
comment, like:
After execution of a prepared statement, send OUT parameters
of the invoked stored procedure, if any, to the client.
When using the binary protocol, send the parameters in an
additional result set over the wire.
When using the text protocol, assign out parameters to the
user variables from the CALL(@var1, @var2, ...) specification.
> +int STDCALL mysql_stmt_next_result(MYSQL_STMT *stmt)
> +{
> + MYSQL *mysql= stmt->mysql;
> + int rc;
> + DBUG_ENTER("mysql_stmt_next_result");
> +
> + rc= mysql_next_result(mysql);
> +
> + if (rc)
> + DBUG_RETURN(rc);
> +
> + if (!(mysql->server_status & SERVER_MORE_RESULTS_EXISTS))
> + DBUG_RETURN(-1);
I think we need to perform a few more safety checks here.
I am looking at mysql_stmt_execute().
First it does:
if (!mysql)
{
/* Error is already set in mysql_detatch_stmt_list */
DBUG_RETURN(1);
}
This code works in case of disconnect/automatic reconnect.
I think we need to have it here too.
Next is:
if (reset_stmt_handle(stmt, RESET_STORE_RESULT | RESET_CLEAR_ERROR))
DBUG_RETURN(1);
I believe this is also necessary, to free the memory that the
current result set occupies. Perhaps we should not call RESET_CLEAR_ERROR,
but in that case, we should check for stmt->last_error and do nothing
(return) if it is set.
We should not RESET_STORE_RESULT if there is no result. So I suggest
to peek into mysql to see if there is a next result
(server_status & SERVER_MORE_RESULTS), and if yes, clear the old result.
While we are at it, have you checked that the tests for
mysql_next_result work in embedded library?
You should run libmysqld/tests/mysql_client_test for it.
> + stmt->state= MYSQL_STMT_EXECUTE_DONE;
> + stmt->mysql->unbuffered_fetch_owner=
> &stmt->unbuffered_fetch_cancelled;
> + stmt->unbuffered_fetch_cancelled= FALSE;
> + stmt->read_row_func= stmt_read_row_unbuffered;
> + stmt->mysql->status= MYSQL_STATUS_GET_RESULT;
I think you split this piece of mysql_stmt_execute into
a helper function, rather than copy-paste it.
> +DROP PROCEDURE IF EXISTS p_str;
> +DROP PROCEDURE IF EXISTS p_dbl;
> +DROP PROCEDURE IF EXISTS p_int;
> +DROP PROCEDURE IF EXISTS p_dec;
Would it be possible to use names them p_string, p_double, p_decimal instead?
Please add a comment to the test case describing the idea of the test.
Please add tests that test out parameters in Dynamic SQL.
> +
> +bool
> +Item_param::set_value(THD *thd, sp_rcontext *ctx, Item **it)
> +{
> + Item *value= *it;
> +
> + if (value->is_null())
> + {
> + set_null();
> + return FALSE;
> + }
> +
> + null_value= FALSE;
> +
> + switch (value->result_type())
> + {
> + case STRING_RESULT:
> + {
Please realign to follow the coding style.
> + char str_buffer[STRING_BUFFER_USUAL_SIZE];
> + String sv_buffer(str_buffer, sizeof(str_buffer), &my_charset_bin);
> + String *sv= value->val_str(&sv_buffer);
> +
> + if (!sv)
> + return TRUE;
> +
> + set_str(sv->c_ptr_safe(), sv->length());
> + str_value_ptr.set(str_value.ptr(),
> + str_value.length(),
> + str_value.charset());
> + collation.set(str_value.charset(), DERIVATION_COERCIBLE);
> + decimals= 0;
> + param_type= MYSQL_TYPE_STRING;
> + }
> + break;
> +
> + case REAL_RESULT:
> + set_double(value->val_real());
> + param_type= MYSQL_TYPE_DOUBLE;
> + break;
> +
> + case INT_RESULT:
> + set_int(value->val_int(), value->max_length);
> + param_type= MYSQL_TYPE_LONG;
> + break;
> +
> + case DECIMAL_RESULT:
> + {
> + my_decimal dv_buf;
> + my_decimal *dv= value->val_decimal(&dv_buf);
> +
> + if (!dv)
> + return TRUE;
> +
> + set_decimal(dv);
> + param_type= MYSQL_TYPE_NEWDECIMAL;
> + }
> + break;
> +
> + default:
> + return TRUE;
Please add a DBUG_ASSERT in the default: branch -- it can't happen.
In production, fall through into STRING_VALUE branch
or set the value to NULL.
> +void
> +Item_param::set_out_param_info(Out_param_info *opi)
> +{
> + m_out_param_info= opi;
> +}
> +
> +const Out_param_info *
> +Item_param::get_out_param_info() const
> +{
> + return m_out_param_info;
> +}
Please add comments for these functions: when they are
called, what they are used for.
> +void Item_param::make_field(Send_field *field)
> +{
> + Item::make_field(field);
Please add an assert that out_param_info is assigned.
> +
> + if (m_out_param_info)
> + {
> + // This is an OUT-parameter of stored procedure.
> + // We should use OUT-parameter info to fill out the names.
Please use a /* */ comment.
> +
> + field->db_name= m_out_param_info->db_name;
> + field->table_name= m_out_param_info->sp_name;
> + field->org_table_name= m_out_param_info->sp_name;
> + field->col_name= m_out_param_info->var_name;
> + field->org_col_name= m_out_param_info->var_name;
> + }
> }
>
> +bool Protocol_text::send_out_parameters(Statement *ps)
> +{
Please pass List<Item_param> here, instead of passing in
the entire statement.
Please add a formal comment for this function.
> + DBUG_ASSERT(ps->lex->param_list.elements ==
> + thd->lex->prepared_stmt_params.elements);
> +
> + List_iterator_fast<Item_param> item_param_it(ps->lex->param_list);
> + List_iterator_fast<LEX_STRING>
> user_var_name_it(thd->lex->prepared_stmt_params);
> + while (true)
> + {
> + Item_param *item_param= item_param_it++;
> + LEX_STRING *user_var_name= user_var_name_it++;
> +
> + if (!item_param || !user_var_name)
> + break;
Perhaps a matter of taste,
but a
for (; item_param= item_param_it++, user_var_name=user_var_name_it++;
item_param && user_var_name)
would make the code easier to understand. In other places infinite loops
are mostly used for finite state machines, thread main loops, etc.
> + // We have to set SERVER_PS_OUT_PARAMS in THD::server_status,
> + // because it is used in send_fields()
Please use /* */ comments.
> + List_iterator_fast<Item> item_it(out_param_lst);
> +
> + while (true)
> + {
> + Item *item= item_it++;
Same here, please use the canonical while ((item= item_it++))
loop here, or a for () loop.
> +
> + if (!item)
> + break;
> +
> + item->send(this, &str);
In theory this may return an error.
Better yet, move the part of select_send::send_data that
does the actual sending to class Protocol (the base class),
and reuse this method here.
> @@ -1994,6 +1994,10 @@ sp_head::execute_procedure(THD *thd, Lis
> if (spvar->mode == sp_param_in)
> continue;
>
> + Out_param_info *out_param_info=
> + new Out_param_info(m_db.str, m_name.str, spvar->name.str);
> + // XXX: it seems, it will be allocate at PS's memroot => not freed.
> +
No, you're fine here, the memory is allocated in the execution memory root.
> +++ b/sql/sql_prepare.cc 2008-07-18 15:54:23 +0000
> @@ -3566,6 +3566,8 @@ bool Prepared_statement::execute(String
> if (state == Query_arena::PREPARED)
> state= Query_arena::EXECUTED;
>
> + protocol->send_out_parameters(this);
> +
Please add if (thd->sql_command == SQLCOM_CALL) here,
that would save us from a function call and an unnecessary loop
in the general case.
> + myheader("test_wl4435");
Please take a look at test_ps_i18n test. Rewrite this
test to do character set conversions, use long data, etc,
but also have out parameters, and check that everything
works okay.
> + con= mysql_init(NULL);
> + if (!con)
> + {
> + fprintf(stderr, "\n mysql_init() failed");
> + exit(1);
> + }
> +
> + if (!(mysql_real_connect(con, opt_host, opt_user,
> + opt_password, current_db, opt_port,
> + opt_unix_socket, CLIENT_MULTI_RESULTS)))
> + {
> + fprintf(stderr, "\n connection failed(%s)", mysql_error(con));
> + exit(1);
> + }
> + con->reconnect= 1;
I think you don't need a separate connection. Make sure the option
to request out parameters is set at start in the default connection.
> +
> + //
> + // Init PS-parameters.
> + //
This is a C file. // comments are a no-no.
--
