Re: bk commit into 5.0 tree (davi:1.2578) BUG#32265

[Konstantin Osipov]

* Davi Arnaut <davi@stripped> [08/02/14 23:13]:

> ChangeSet@stripped, 2008-02-14 17:42:46-02:00, davi@stripped +2 -0
>   Bug#32265 Server returns different metadata if prepared statement is used
>   
>   Executing a prepared statement associated with a materialized
>   cursor yields to the client a metadata packet with wrong table
>   and database names. The problem was occurring because the server
>   was sending the the name of the temporary table used by the cursor
>   instead of the table name of the original table. The same problem
>   occurs when selecting from views, in which case the table name was
>   being sent and not the name of the view.
>   
>   The solution is to backup the table and database names of the fields
>   of the original tables or views and restore then later into the
>   fields of the temporary table.

Hello Davi,

Thank you very much for working on this.

I agree with the approach, but thing the implementation could be
slightly different.

Please see the attached patch below. I'd like to ask you to review
it :)

Let me know what you think about it.
Besides, I only tested it with mysql_client_test).

It is OK to push the attached patch after adding doxygen comments
to the new methods ;)


===== sql/sql_cursor.cc 1.14 vs edited =====
--- 1.14/sql/sql_cursor.cc	2008-02-19 14:42:58 +03:00
+++ edited/sql/sql_cursor.cc	2008-02-20 16:53:17 +03:00
@@ -88,6 +88,8 @@
 public:
   Materialized_cursor(select_result *result, TABLE *table);
 
+  int fill_item_list(THD *thd, List<Item> &send_fields);
+
   virtual bool is_open() const { return table != 0; }
   virtual int open(JOIN *join __attribute__((unused)));
   virtual void fetch(ulong num_rows);
@@ -96,6 +98,8 @@
 };
 
 
+class Materialized_cursor;
+
 /**
   Select_materialize -- a mediator between a cursor query and the
   protocol. In case we were not able to open a non-materialzed
@@ -109,6 +113,7 @@
 {
   select_result *result; /**< the result object of the caller (PS or SP) */
 public:
+  Materialized_cursor *materialized_cursor;
   Select_materialize(select_result *result_arg) :result(result_arg) {}
   virtual bool send_fields(List<Item> &list, uint flags);
 };
@@ -151,7 +156,7 @@
 
   if (! (sensitive_cursor= new (thd->mem_root) Sensitive_cursor(thd, result)))
   {
-    delete result;
+    delete result_materialize;
     return 1;
   }
 
@@ -173,13 +178,13 @@
   /*
     Possible options here:
     - a sensitive cursor is open. In this case rc is 0 and
-      result_materialize->table is NULL, or
+      result_materialize->materialized_cursor is NULL, or
     - a materialized cursor is open. In this case rc is 0 and
-      result_materialize->table is not NULL
-    - an error occured during materializaton.
-      result_materialize->table is not NULL, but rc != 0
+      result_materialize->materialized is not NULL
+    - an error occurred during materialization.
+      result_materialize->materialized_cursor is not NULL, but rc != 0
     - successful completion of mysql_execute_command without
-      a cursor: rc is 0, result_materialize->table is NULL,
+      a cursor: rc is 0, result_materialize->materialized_cursor is NULL,
       sensitive_cursor is not open.
       This is possible if some command writes directly to the
       network, bypassing select_result mechanism. An example of
@@ -190,7 +195,7 @@
 
   if (sensitive_cursor->is_open())
   {
-    DBUG_ASSERT(!result_materialize->table);
+    DBUG_ASSERT(!result_materialize->materialized_cursor);
     /*
       It's safer if we grab THD state after mysql_execute_command
       is finished and not in Sensitive_cursor::open(), because
@@ -201,18 +206,10 @@
     *pcursor= sensitive_cursor;
     goto end;
   }
-  else if (result_materialize->table)
+  else if (result_materialize->materialized_cursor)
   {
-    Materialized_cursor *materialized_cursor;
-    TABLE *table= result_materialize->table;
-    MEM_ROOT *mem_root= &table->mem_root;
-
-    if (!(materialized_cursor= new (mem_root)
-                               Materialized_cursor(result, table)))
-    {
-      rc= 1;
-      goto err_open;
-    }
+    Materialized_cursor *materialized_cursor=
+      result_materialize->materialized_cursor;
 
     if ((rc= materialized_cursor->open(0)))
     {
@@ -228,8 +225,6 @@
 err_open:
   DBUG_ASSERT(! (sensitive_cursor && sensitive_cursor->is_open()));
   delete sensitive_cursor;
-  if (result_materialize->table)
-    free_tmp_table(thd, result_materialize->table);
 end:
   delete result_materialize;
   return rc;
@@ -554,6 +549,45 @@
 }
 
 
+int
+Materialized_cursor::fill_item_list(THD *thd, List<Item> &send_fields)
+{
+  Query_arena backup_arena;
+  int rc;
+  List_iterator_fast<Item> it_org(send_fields);
+  List_iterator_fast<Item> it_dst(item_list);
+  Item *item_org;
+  Item *item_dst;
+
+  thd->set_n_backup_active_arena(this, &backup_arena);
+
+  if ((rc= table->fill_item_list(&item_list)))
+    goto end;
+
+  DBUG_ASSERT(send_fields.elements == item_list.elements);
+
+  /*
+    Unless we preserve the original metadata, it will be lost,
+    since new fields describe columns of the temporary table.
+    Allocate a copy of the name for safety only. Currently
+    items with original names are always kept in memory,
+    but in case this changes a memory leak may be hard to notice.
+  */
+  while ((item_dst= it_dst++, item_org= it_org++))
+  {
+    Send_field send_field;
+    Item_ident *ident= static_cast<Item_ident *>(item_dst);
+    item_org->make_field(&send_field);
+
+    ident->db_name=    thd->strdup(send_field.db_name);
+    ident->table_name= thd->strdup(send_field.table_name);
+  }
+end:
+  thd->restore_active_arena(this, &backup_arena);
+  /* Check for thd->is_error() in case of OOM */
+  return rc || thd->is_error();
+}
+
 int Materialized_cursor::open(JOIN *join __attribute__((unused)))
 {
   THD *thd= fake_unit.thd;
@@ -562,8 +596,7 @@
 
   thd->set_n_backup_active_arena(this, &backup_arena);
   /* Create a list of fields and start sequential scan */
-  rc= (table->fill_item_list(&item_list) ||
-       result->prepare(item_list, &fake_unit) ||
+  rc= (result->prepare(item_list, &fake_unit) ||
        table->file->ha_rnd_init(TRUE));
   thd->restore_active_arena(this, &backup_arena);
   if (rc == 0)
@@ -669,6 +702,24 @@
   if (create_result_table(unit->thd, unit->get_unit_column_types(),
                           FALSE, thd->options | TMP_TABLE_ALL_COLUMNS, ""))
     return TRUE;
+
+  materialized_cursor= new (&table->mem_root)
+                       Materialized_cursor(result, table);
+
+  if (! materialized_cursor)
+  {
+    free_tmp_table(table->in_use, table);
+    table= 0;
+    return TRUE;
+  }
+  if (materialized_cursor->fill_item_list(unit->thd, list))
+  {
+    delete materialized_cursor;
+    table= 0;
+    materialized_cursor= 0;
+    return TRUE;
+  }
+
   return FALSE;
 }
 
===== tests/mysql_client_test.c 1.261 vs edited =====
--- 1.261/tests/mysql_client_test.c	2008-01-11 04:04:30 +03:00
+++ edited/tests/mysql_client_test.c	2008-02-20 15:51:09 +03:00
@@ -17231,6 +17231,87 @@
   DBUG_VOID_RETURN;
 }
 
+
+/**
+  Bug#32265 Server returns different metadata if prepared statement is used
+*/
+
+static void test_bug32265()
+{
+  int rc, i;
+  MYSQL_STMT *stmt;
+  MYSQL_FIELD *field;
+
+  DBUG_ENTER("test_bug32265");
+  myheader("test_bug32265");
+
+  rc= mysql_query(mysql, "DROP TABLE IF EXISTS t1");
+  myquery(rc);
+  rc= mysql_query(mysql, "CREATE  TABLE t1 (a INTEGER)");
+  myquery(rc);
+  rc= mysql_query(mysql, "INSERT INTO t1 VALUES (1)");
+  myquery(rc);
+  rc= mysql_query(mysql, "CREATE VIEW v1 AS SELECT * FROM t1");
+  myquery(rc);
+
+  stmt= open_cursor("SELECT * FROM t1");
+  rc= mysql_stmt_execute(stmt);
+  check_execute(stmt, rc);
+
+  field= stmt->mysql->fields;
+  DIE_UNLESS(strcmp(field->table, "t1") == 0);
+  DIE_UNLESS(strcmp(field->org_table, "t1") == 0);
+  DIE_UNLESS(strcmp(field->db, "client_test_db") == 0);
+  mysql_stmt_close(stmt);
+
+  stmt= open_cursor("SELECT a '' FROM t1 ``");
+  rc= mysql_stmt_execute(stmt);
+  check_execute(stmt, rc);
+
+  field= stmt->mysql->fields;
+  DIE_UNLESS(strcmp(field->table, "") == 0);
+  DIE_UNLESS(strcmp(field->org_table, "t1") == 0);
+  DIE_UNLESS(strcmp(field->db, "client_test_db") == 0);
+  mysql_stmt_close(stmt);
+
+  stmt= open_cursor("SELECT a '' FROM t1 ``");
+  rc= mysql_stmt_execute(stmt);
+  check_execute(stmt, rc);
+
+  field= stmt->mysql->fields;
+  DIE_UNLESS(strcmp(field->table, "") == 0);
+  DIE_UNLESS(strcmp(field->org_table, "t1") == 0);
+  DIE_UNLESS(strcmp(field->db, "client_test_db") == 0);
+  mysql_stmt_close(stmt);
+
+  stmt= open_cursor("SELECT * FROM v1");
+  rc= mysql_stmt_execute(stmt);
+  check_execute(stmt, rc);
+
+  field= stmt->mysql->fields;
+  DIE_UNLESS(strcmp(field->table, "v1") == 0);
+  DIE_UNLESS(strcmp(field->org_table, "t1") == 0);
+  DIE_UNLESS(strcmp(field->db, "client_test_db") == 0);
+  mysql_stmt_close(stmt);
+
+  stmt= open_cursor("SELECT * FROM v1 /* SIC */ GROUP BY 1");
+  rc= mysql_stmt_execute(stmt);
+  check_execute(stmt, rc);
+
+  field= stmt->mysql->fields;
+  DIE_UNLESS(strcmp(field->table, "v1") == 0);
+  DIE_UNLESS(strcmp(field->org_table, "t1") == 0);
+  DIE_UNLESS(strcmp(field->db, "client_test_db") == 0);
+  mysql_stmt_close(stmt);
+
+  rc= mysql_query(mysql, "DROP VIEW v1");
+  myquery(rc);
+  rc= mysql_query(mysql, "DROP TABLE t1");
+  myquery(rc);
+
+  DBUG_VOID_RETURN;
+}
+
 /*
   Read and parse arguments and MySQL options from my.cnf
 */
@@ -17535,6 +17616,7 @@
   { "test_bug20023", test_bug20023 },
   { "test_bug31418", test_bug31418 },
   { "test_bug31669", test_bug31669 },
+  { "test_bug32265", test_bug32265 },
   { 0, 0 }
 };
 
-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY