Re: bzr commit into mysql-6.0 branch (alik:2675) WL#4435

[Konstantin Osipov]

* Alexander Nozdrin <alik@stripped> [08/08/05 22:35]:
>   libmysql/libmysql.c
>     1. Fix alloc_stmt_fields() -- MUSQL_FIELD::catalog wasn't assigned as it should
> have been.
MYSQL_FIELD
>     2. Change update_stmt_fields() so that it works with result sets with different
> structures.
>     3. Implement mysql_stmt_next_result().

Is it possible to make sure the comments fit 80 columns?-)

> === modified file 'libmysql/libmysql.c'
> --- a/libmysql/libmysql.c	2008-06-17 20:04:19 +0000
> +++ b/libmysql/libmysql.c	2008-08-05 18:04:27 +0000
> @@ -1727,6 +1727,9 @@ static void alloc_stmt_fields(MYSQL_STMT
>  
>    stmt->field_count= mysql->field_count;
>  
> +  if (!stmt->field_count)
> +    return;
> +

Please add a DBUG_ASSERT(stmt->field_count); here instead.

> +int STDCALL mysql_stmt_next_result(MYSQL_STMT *stmt)
> +{
> +  MYSQL *mysql= stmt->mysql;
> +  int rc;
> +  DBUG_ENTER("mysql_stmt_next_result");
> +
> +  if (!mysql)
> +    DBUG_RETURN(1);
> +
> +  if (stmt->last_errno)
> +    DBUG_RETURN(stmt->last_errno);
> +
> +  if (mysql->server_status & SERVER_MORE_RESULTS_EXISTS)
> +  {
> +    if (reset_stmt_handle(stmt, RESET_STORE_RESULT))
> +      DBUG_RETURN(1);
> +  }
> +
> +  rc= mysql_next_result(mysql);

Good.
> +
> +  if (rc)
> +    DBUG_RETURN(rc);

Good.


Per our discussion on the phone, this piece should look
like:

> +  alloc_stmt_fields(stmt);
> +  stmt->bind_result_done= FALSE;
> +
> +  if (!(mysql->server_status & SERVER_MORE_RESULTS_EXISTS))
> +    DBUG_RETURN(-1);
> +
> +  stmt->state= MYSQL_STMT_EXECUTE_DONE;
> +  stmt->mysql->status= MYSQL_STATUS_GET_RESULT;
> +
> +  prepare_to_fetch_result(stmt);
> +
> +  DBUG_RETURN(0);

 stmt->state= MYSQL_STMT_EXECUTE_DONE;
 stmt->bind_result_done= FALSE;

 if (mysql->field_count)
 {
   alloc_stmt_fields(stmt);
   prepare_to_fetch_result(stmt);
 }

After this change the last "OK" packet will be
treated as a separate result. This is 
expected, and similar to multiple results 
of conventional mysql_query(). Please adjust the tests
accordingly.

> +  stmt->mysql->status= MYSQL_STATUS_GET_RESULT;

This line is not necessary, mysql->status will be assigned
in  mysql_next_result().

> +}
> +
> +bool
> +Item_param::set_value(THD *thd, sp_rcontext *ctx, Item **it)
> +{

Please add a doxygen comment.
There should be two empty lines between methods.

> +  switch (value->result_type())
> +  {
> +  case STRING_RESULT:
> +    {
> +      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:
> +    /* That can not happen. */
> +
> +    DBUG_ASSERT(TRUE);  // Abort in debug mode.
> +
> +    set_null();         // Set to NULL in release mode.
> +    return FALSE;
> +  }

Please fix the alignment of the switch.

Here is how it should look like:

  switch (value->result_type()) {
  case STRING_RESULT:
  {
    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:
    /* That can not happen. */

    DBUG_ASSERT(TRUE);  // Abort in debug mode.

    set_null();         // Set to NULL in release mode.
    return FALSE;
  }

> +
> +void Item_param::make_field(Send_field *field)
> +{

Please add a doxygen comment about specifics
of this method.

I.e.:

If this is an out parameter of a stored procedure,
preserve the result set metadata.
> +  Item::make_field(field);
> +
> +  if (!m_out_param_info)
> +    return;
> +
> +  /*
> +    This is an OUT-parameter of stored procedure. We should use
> +    OUT-parameter info to fill out the names.
> +  */
> +
> +  field->db_name= m_out_param_info->db_name;
> +  field->table_name= m_out_param_info->table_name;
> +  field->org_table_name= m_out_param_info->org_table_name;
> +  field->col_name= m_out_param_info->col_name;
> +  field->org_col_name= m_out_param_info->org_col_name;
> +  field->length= m_out_param_info->length;
> +  field->charsetnr= m_out_param_info->charsetnr;
> +  field->flags= m_out_param_info->flags;
> +  field->decimals= m_out_param_info->decimals;
> +  field->type= m_out_param_info->type;

Please add a test case demonstrating that these fields
are correct. E.g.:

create procedure p1(a int(11) out, b int(10) out, c varchar(5) not
null out, d char(10) out) and check in mysql_client_test.c
that the properties of out parameters are preserved.


>  
> +bool Protocol::send_row_items(List<Item> *row_items)
> +{

Please add a doxygen comment.

> +  char buffer[MAX_FIELD_WIDTH];
> +  String str_buffer(buffer, sizeof (buffer), &my_charset_bin);
> +  List_iterator_fast<Item> it(*row_items);
> +
> +  DBUG_ENTER("Protocol::send_row_items");
> +
> +  for (Item *item= it++; item; item= it++)
> +  {
> +    if (item->send(this, &str_buffer) || thd->is_error())
> +    {
> +      this->free();				// Free used buffer
> +      my_message(ER_OUT_OF_RESOURCES, ER(ER_OUT_OF_RESOURCES), MYF(0));
> +      DBUG_RETURN(TRUE);
> +    }
> +  }
> +
> +  DBUG_RETURN(FALSE);
> +}
> +
> +
>  /**
>    Send \\0 end terminated string.
>  
> @@ -1044,6 +1066,53 @@ bool Protocol_text::store_time(MYSQL_TIM
>    return net_store_data((uchar*) buff, length);
>  }
>  
> +
> +bool Protocol_binary::send_out_parameters(List<Item_param> *sp_params)
> +{
> +  if (!(thd->client_capabilities & CLIENT_PS_MULTI_RESULTS))
> +  {
> +    /* The client does not support OUT-parameters. */
> +    return FALSE;
> +  }
> +
> +  List<Item> out_param_lst;
> +
> +  {
> +    List_iterator_fast<Item_param> item_param_it(*sp_params);
> +
> +    while (true)
> +    {
> +      Item_param *item_param= item_param_it++;
> +
> +      if (!item_param)
> +        break;
> +
> +      if (!item_param->get_out_param_info())
> +        continue; // It's an IN-parameter.

> +
> +      out_param_lst.push_back(item_param);

Please add a check for an error here.

> +
> +  /* Send EOF-packet. */
> +  net_send_eof(thd, thd->server_status, thd->total_warn_count);

The eof packet here is rudimentary. Please pass 0 instead of 
thd->total_warn_count, warning count is meaningful only in the
last packet sent for a statement.

BTW, send_row_items could be named send_result_set_row().
While you are at it, you could rename send_fields to 
send_result_set_metadata().

> +
> +  if (strlen(test_dir) + strlen(result_file_name) + 10 > FILE_PATH_SIZE)

What is 10 here for? Doesn't seem to be used anywhere below.

> +  {
> +    printf("Warning: MYSQL_TEST_DIR is too long. "
> +           "Result checking is impossible.\n");
> +    return 0;
> +  }
> +
> +  my_snprintf(result_file_path, FILE_PATH_SIZE,
> +              "%s/%s",
> +              (const char *) test_dir,
> +              (const char *) result_file_name);
> +
> +  my_fclose(mct_log_file, MYF(0));
> +  mct_log_file= NULL;
> +
> +  snprintf(diff_cmd, CMD_BUFFER_SIZE, "diff -u '%s' '%s'",
> +           (const char *) result_file_path,
> +           (const char *) mct_log_file_path);
> +
> +  puts("");
> +  fflush(stdout);
> +  fflush(stderr);
> +
> +  return system(diff_cmd);

diff is not available on windows, unfortunately.
Perhaps easier to write a simple diff implementation 
as a function.


> @@ -18265,7 +18788,7 @@ int main(int argc, char **argv)
>                          (char**) embedded_server_groups))
>      DIE("Can't initialize MySQL server");
>  
> -  client_connect(0);       /* connect to server */
> +  client_connect(CLIENT_MULTI_RESULTS);       /* connect to server */

Shouldn't it be CLIENT_PS_MULTI_RESULTS?
BTW, CLIENT_PS_OUT_PARAMS is  now only present in changeset
comments. Please update them.

The patch is OK to push with the changes requested above.

--