maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Sanja,


On 12/27/2016 09:39 PM, Oleksandr Byelkin wrote:
> Hi, Alexander!
> 
> It is OK to push after fixing in the comment of the patch
> mysql_list_fields  with mysqld_list_fields (IMHO it is server function
> do not set type etc.)

Thanks for review!

I fixed the comment.

But then I realized that it's a good idea to add tests for other
field types (e.g. DECIMAL, DATETIME, ENUM), and found that my
fix was slightly incomplete:

Item_ident_for_show::Item_ident_for_show() should copy not
only collation from the field, but the entire Type_std_attributes
(i.e. unsigned_flag, decimals and max_length).

So I also fixed this, with some additional minor changes
to avoid code duplication.

This is an updated version. Please have a look.

Thanks!


> 
> On Tue, Dec 27, 2016 at 4:46 PM, Alexander Barkov <bar@xxxxxxxxxxx
> <mailto:bar@xxxxxxxxxxx>> wrote:
> 
>     Hello Sanja,
> 
>     Can you please review a patch for MDEV-11672?
> 
>     Thanks!
> 
> 

commit 3331dc53361bbdc83562732e2eaf6215564296f4
Author: Alexander Barkov <bar@xxxxxxxxxxx>
Date:   Wed Dec 28 10:21:19 2016 +0400

    MDEV-11672 mysql_list_field() returns wrong default values for VIEW
    
    The problem happened because Item_ident_for_show::field_type() always
    returned MYSQL_TYPE_DOUBLE and ignored the actual data type of the
    referenced Field. As a result, the execution always used
    Item_ident_for_show::val_real() to send the default value of the field,
    so most default values for non-numeric types were displayed as '0'.
    
    This patch:
    1. Cleanup:
       a. Removes Send_field::charsetnr, as it's been unused since
          introduction of Item::charset_for_protocol() in MySQL-5.5.
       b. Adds the "const" qualifier to Field::char_length().
          This is needed for (5.a), see below.
    
    2. Introduces a new virtual method Type_handler::charset_for_protocol(),
       returning item->collation.collation for string data types, or
       &my_charset_bin for non-string data types.
    
    3. Changes Item::charset_for_protocol() from virtual to non-virtual.
       It now calls type_handler()->charset_for_protocol().
       As a good side effect, duplicate code in Item::charset_for_protocol() and
       Item_temporal_hybrid_func() is now gone.
    
    4. Fixes Item_ident_for_show::field_type() to correctly return
       its data type according to the data type of the referenced field.
       This actually fixes the problem reported in MDEV-11672.
       Now the default value is sent using a correct method, e.g.
       val_str() for VARCHAR/TEXT, or val_int() for INT/BIGINT.
       This required additional changes:
       a. in DBUG_ASSERT in Protocol::store(const char *,size_t,CHARSET_INFO),
          This method is now used by mysqld_list_fields(), which
          (unlike normal SELECT queries) does not set
          field_types/field_pos/field_count.
       b. Item_ident_for_show::Item_ident_for_show() now set standard attributes
          (collation, decimals, max_length) according to the referenced field,
          to make charset_for_protocol() return the correct value.
    
    5. In order to share the code between Item_field::set_field() and
       Item_ident_for_show::Item_ident_for_show():
       a. Introduces a new method Type_std_attributes::set(const Field*)
       b. To make (a) possible, moves Item::fix_char_length() from Item
          to Type_std_attributes, also moves char_to_byte_length_safe()
          from item.h to sql_type.h
       c. Additionally, moves Item::fix_length_and_charset() and
          Item::max_char_length() from Item to Type_std_attributes.
          This is not directly needed for the fix and is done just for symmetry
          with fix_char_length(), as these three methods are directly related
          to each other.

diff --git a/sql/field.cc b/sql/field.cc
index 480fcfb..48ae682 100644
--- a/sql/field.cc
+++ b/sql/field.cc
@@ -1944,7 +1944,6 @@ void Field::make_field(Send_field *field)
     field->org_col_name= "";
   }
   field->col_name= field_name;
-  field->charsetnr= charset()->number;
   field->length=field_length;
   field->type=type();
   field->flags=table->maybe_null ? (flags & ~NOT_NULL_FLAG) : flags;
@@ -10747,7 +10746,7 @@ Column_definition::Column_definition(THD *thd, Field *old_field,
     length
 */
 
-uint32 Field_blob::char_length()
+uint32 Field_blob::char_length() const
 {
   switch (packlength)
   {
diff --git a/sql/field.h b/sql/field.h
index 7410288..c37c858 100644
--- a/sql/field.h
+++ b/sql/field.h
@@ -1353,7 +1353,7 @@ class Field: public Value_source
   longlong convert_decimal2longlong(const my_decimal *val, bool unsigned_flag,
                                     int *err);
   /* The max. number of characters */
-  virtual uint32 char_length()
+  virtual uint32 char_length() const
   {
     return field_length / charset()->mbmaxlen;
   }
@@ -3344,7 +3344,7 @@ class Field_blob :public Field_longstr {
   bool has_charset(void) const
   { return charset() == &my_charset_bin ? FALSE : TRUE; }
   uint32 max_display_length();
-  uint32 char_length();
+  uint32 char_length() const;
   uint is_equal(Create_field *new_field);
 private:
   int do_save_field_metadata(uchar *first_byte);
@@ -3939,7 +3939,7 @@ class Send_field :public Sql_alloc {
   const char *table_name,*org_table_name;
   const char *col_name,*org_col_name;
   ulong length;
-  uint charsetnr, flags, decimals;
+  uint flags, decimals;
   enum_field_types type;
   Send_field() {}
 };
diff --git a/sql/item.cc b/sql/item.cc
index 2a10587..c37613e 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -2397,7 +2397,6 @@ void Item_ident_for_show::make_field(THD *thd, Send_field *tmp_field)
   tmp_field->table_name= tmp_field->org_table_name= table_name;
   tmp_field->db_name= db_name;
   tmp_field->col_name= tmp_field->org_col_name= field->field_name;
-  tmp_field->charsetnr= field->charset()->number;
   tmp_field->length=field->field_length;
   tmp_field->type=field->type();
   tmp_field->flags= field->table->maybe_null ? 
@@ -2559,15 +2558,11 @@ void Item_field::set_field(Field *field_par)
 {
   field=result_field=field_par;			// for easy coding with fields
   maybe_null=field->maybe_null();
-  decimals= field->decimals();
+  Type_std_attributes::set(field_par);
   table_name= *field_par->table_name;
   field_name= field_par->field_name;
   db_name= field_par->table->s->db.str;
   alias_name_used= field_par->table->alias_name_used;
-  unsigned_flag= MY_TEST(field_par->flags & UNSIGNED_FLAG);
-  collation.set(field_par->charset(), field_par->derivation(),
-                field_par->repertoire());
-  fix_char_length(field_par->char_length());
 
   max_length= adjust_max_effective_column_length(field_par, max_length);
 
@@ -4259,7 +4254,6 @@ void Item_param::make_field(THD *thd, Send_field *field)
   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;
@@ -5792,7 +5786,6 @@ void Item::init_make_field(Send_field *tmp_field,
   tmp_field->org_col_name=	empty_name;
   tmp_field->table_name=	empty_name;
   tmp_field->col_name=		name;
-  tmp_field->charsetnr=         collation.collation->number;
   tmp_field->flags=             (maybe_null ? 0 : NOT_NULL_FLAG) | 
                                 (my_binary_compare(charset_for_protocol()) ?
                                  BINARY_FLAG : 0);
diff --git a/sql/item.h b/sql/item.h
index ed7d07e..8499bef 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -96,13 +96,6 @@ enum precedence {
 
 typedef Bounds_checked_array<Item*> Ref_ptr_array;
 
-static inline uint32
-char_to_byte_length_safe(uint32 char_length_arg, uint32 mbmaxlen_arg)
-{
-   ulonglong tmp= ((ulonglong) char_length_arg) * mbmaxlen_arg;
-   return (tmp > UINT_MAX32) ? (uint32) UINT_MAX32 : (uint32) tmp;
-}
-
 bool mark_unsupported_function(const char *where, void *store, uint result);
 
 /* convenience helper for mark_unsupported_function() above */
@@ -1362,14 +1355,9 @@ class Item: public Value_source,
 
   static CHARSET_INFO *default_charset();
 
-  /*
-    For backward compatibility, to make numeric
-    data types return "binary" charset in client-side metadata.
-  */
-  virtual CHARSET_INFO *charset_for_protocol(void) const
+  CHARSET_INFO *charset_for_protocol(void) const
   {
-    return cmp_type() == STRING_RESULT ? collation.collation :
-                                         &my_charset_bin;
+    return type_handler()->charset_for_protocol(this);
   };
 
   virtual bool walk(Item_processor processor, bool walk_subquery, void *arg)
@@ -1687,20 +1675,8 @@ class Item: public Value_source,
     { return Field::GEOM_GEOMETRY; };
   String *check_well_formed_result(String *str, bool send_error= 0);
   bool eq_by_collation(Item *item, bool binary_cmp, CHARSET_INFO *cs); 
-  uint32 max_char_length() const
-  { return max_length / collation.collation->mbmaxlen; }
   bool too_big_for_varchar() const
   { return max_char_length() > CONVERT_IF_BIGGER_TO_BLOB; }
-  void fix_length_and_charset(uint32 max_char_length_arg, CHARSET_INFO *cs)
-  {
-    max_length= char_to_byte_length_safe(max_char_length_arg, cs->mbmaxlen);
-    collation.collation= cs;
-  }
-  void fix_char_length(uint32 max_char_length_arg)
-  {
-    max_length= char_to_byte_length_safe(max_char_length_arg,
-                                         collation.collation->mbmaxlen);
-  }
   /*
     Return TRUE if the item points to a column of an outer-joined table.
   */
@@ -2319,7 +2295,9 @@ class Item_ident_for_show :public Item
   Item_ident_for_show(THD *thd, Field *par_field, const char *db_arg,
                       const char *table_name_arg):
     Item(thd), field(par_field), db_name(db_arg), table_name(table_name_arg)
-  {}
+  {
+    Type_std_attributes::set(par_field);
+  }
 
   enum Type type() const { return FIELD_ITEM; }
   double val_real() { return field->val_real(); }
@@ -2327,9 +2305,7 @@ class Item_ident_for_show :public Item
   String *val_str(String *str) { return field->val_str(str); }
   my_decimal *val_decimal(my_decimal *dec) { return field->val_decimal(dec); }
   void make_field(THD *thd, Send_field *tmp_field);
-  CHARSET_INFO *charset_for_protocol(void) const
-  { return field->charset_for_protocol(); }
-  enum_field_types field_type() const { return MYSQL_TYPE_DOUBLE; }
+  enum_field_types field_type() const { return field->type(); }
   Item* get_copy(THD *thd, MEM_ROOT *mem_root)
   { return get_item_copy<Item_ident_for_show>(thd, mem_root, this); }
 };
@@ -2507,8 +2483,6 @@ class Item_field :public Item_ident
     DBUG_ASSERT(field_type() == MYSQL_TYPE_GEOMETRY);
     return field->get_geometry_type();
   }
-  CHARSET_INFO *charset_for_protocol(void) const
-  { return field->charset_for_protocol(); }
   friend class Item_default_value;
   friend class Item_insert_value;
   friend class st_select_lex_unit;
diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h
index ad92f50..1014326 100644
--- a/sql/item_timefunc.h
+++ b/sql/item_timefunc.h
@@ -573,18 +573,6 @@ class Item_temporal_hybrid_func: public Item_temporal_func,
   { return Type_handler_hybrid_field_type::result_type(); }
   enum Item_result cmp_type () const
   { return Type_handler_hybrid_field_type::cmp_type(); }
-  CHARSET_INFO *charset_for_protocol() const
-  {
-    /*
-      Can return TIME, DATE, DATETIME or VARCHAR depending on arguments.
-      Send using "binary" when TIME, DATE or DATETIME,
-      or using collation.collation when VARCHAR
-      (which is fixed from @@collation_connection in fix_length_and_dec).
-    */
-    DBUG_ASSERT(fixed == 1);
-    return Item_temporal_hybrid_func::field_type() == MYSQL_TYPE_STRING ?
-           collation.collation : &my_charset_bin;
-  }
   /**
     Fix the returned timestamp to match field_type(),
     which is important for val_str().
diff --git a/sql/protocol.cc b/sql/protocol.cc
index f8b68c0..3cc7307 100644
--- a/sql/protocol.cc
+++ b/sql/protocol.cc
@@ -1116,7 +1116,7 @@ bool Protocol_text::store(const char *from, size_t length,
 #ifndef DBUG_OFF
   DBUG_PRINT("info", ("Protocol_text::store field %u (%u): %.*s", field_pos,
                       field_count, (int) length, (length == 0 ? "" : from)));
-  DBUG_ASSERT(field_pos < field_count);
+  DBUG_ASSERT(field_types == 0 || field_pos < field_count);
   DBUG_ASSERT(field_types == 0 ||
 	      field_types[field_pos] == MYSQL_TYPE_DECIMAL ||
               field_types[field_pos] == MYSQL_TYPE_BIT ||
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index 0f6424e..4dc8031 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -56,6 +56,15 @@ Type_handler_newdecimal  type_handler_newdecimal;
 Type_handler_datetime    type_handler_datetime;
 
 
+void Type_std_attributes::set(const Field *field)
+{
+  decimals= field->decimals();
+  unsigned_flag= MY_TEST(field->flags & UNSIGNED_FLAG);
+  collation.set(field->charset(), field->derivation(), field->repertoire());
+  fix_char_length(field->char_length());
+}
+
+
 /**
   This method is used by:
   - Item_user_var_as_out_param::field_type()
@@ -101,6 +110,23 @@ Type_handler_string_result::type_handler_adjusted_to_max_octet_length(
 }
 
 
+CHARSET_INFO *Type_handler::charset_for_protocol(const Item *item) const
+{
+  /*
+    For backward compatibility, to make numeric
+    data types return "binary" charset in client-side metadata.
+  */
+  return &my_charset_bin;
+}
+
+
+CHARSET_INFO *
+Type_handler_string_result::charset_for_protocol(const Item *item) const
+{
+  return item->collation.collation;
+}
+
+
 const Type_handler *
 Type_handler::get_handler_by_cmp_type(Item_result type)
 {
diff --git a/sql/sql_type.h b/sql/sql_type.h
index e1a5883..12f846e 100644
--- a/sql/sql_type.h
+++ b/sql/sql_type.h
@@ -176,6 +176,13 @@ class DTCollation {
 };
 
 
+static inline uint32
+char_to_byte_length_safe(uint32 char_length_arg, uint32 mbmaxlen_arg)
+{
+   ulonglong tmp= ((ulonglong) char_length_arg) * mbmaxlen_arg;
+   return (tmp > UINT_MAX32) ? (uint32) UINT_MAX32 : (uint32) tmp;
+}
+
 /**
   A class to store type attributes for the standard data types.
   Does not include attributes for the extended data types
@@ -217,6 +224,19 @@ class Type_std_attributes
   {
     *this= other;
   }
+  void set(const Field *field);
+  uint32 max_char_length() const
+  { return max_length / collation.collation->mbmaxlen; }
+  void fix_length_and_charset(uint32 max_char_length_arg, CHARSET_INFO *cs)
+  {
+    max_length= char_to_byte_length_safe(max_char_length_arg, cs->mbmaxlen);
+    collation.collation= cs;
+  }
+  void fix_char_length(uint32 max_char_length_arg)
+  {
+    max_length= char_to_byte_length_safe(max_char_length_arg,
+                                         collation.collation->mbmaxlen);
+  }
 };
 
 
@@ -246,6 +266,7 @@ class Type_handler
   virtual Item_result result_type() const= 0;
   virtual Item_result cmp_type() const= 0;
   virtual const Type_handler *type_handler_for_comparison() const= 0;
+  virtual CHARSET_INFO *charset_for_protocol(const Item *item) const;
   virtual const Type_handler*
   type_handler_adjusted_to_max_octet_length(uint max_octet_length,
                                             CHARSET_INFO *cs) const
@@ -628,6 +649,7 @@ class Type_handler_string_result: public Type_handler
 public:
   Item_result result_type() const { return STRING_RESULT; }
   Item_result cmp_type() const { return STRING_RESULT; }
+  CHARSET_INFO *charset_for_protocol(const Item *item) const;
   virtual ~Type_handler_string_result() {}
   const Type_handler *type_handler_for_comparison() const;
   const Type_handler *
diff --git a/tests/mysql_client_test.c b/tests/mysql_client_test.c
index 58e3dda..a1c28f0 100644
--- a/tests/mysql_client_test.c
+++ b/tests/mysql_client_test.c
@@ -8398,6 +8398,76 @@ static void test_list_fields()
 }
 
 
+static void test_list_fields_default()
+{
+  int rc, i;
+  myheader("test_list_fields_default");
+
+  rc= mysql_query(mysql, "DROP TABLE IF EXISTS t1");
+  myquery(rc);
+
+  rc= mysql_query(mysql,
+                  "CREATE TABLE t1 ("
+                  " i1 INT NOT NULL DEFAULT 0,"
+                  " i3 BIGINT UNSIGNED NOT NULL DEFAULT 0xFFFFFFFFFFFFFFFF,"
+                  " s1 VARCHAR(10) CHARACTER SET latin1 NOT NULL DEFAULT 's1def',"
+                  " d1 DECIMAL(31,1) NOT NULL DEFAULT 111111111122222222223333333333.9,"
+                  " t1 DATETIME(6) NOT NULL DEFAULT '2001-01-01 10:20:30.123456',"
+                  " e1 ENUM('a','b') NOT NULL DEFAULT 'a'"
+                  ")");
+  myquery(rc);
+
+  rc= mysql_query(mysql, "DROP VIEW IF EXISTS v1");
+  myquery(rc);
+
+  rc= mysql_query(mysql, "CREATE VIEW v1 AS SELECT * FROM t1");
+  myquery(rc);
+
+  /*
+    Checking that mysql_list_fields() returns the same result
+    for a TABLE and a VIEW on the same table.
+  */
+  for (i= 0; i < 2; i++)
+  {
+    const char *table_name= i == 0 ? "t1" : "v1";
+    MYSQL_RES *result= mysql_list_fields(mysql, table_name, NULL);
+    mytest(result);
+
+    rc= my_process_result_set(result);
+    DIE_UNLESS(rc == 0);
+
+    verify_prepare_field(result, 0, "i1", "i1", MYSQL_TYPE_LONG,
+                         table_name, table_name, current_db,
+                         11, "0");
+
+    verify_prepare_field(result, 1, "i3", "i3", MYSQL_TYPE_LONGLONG,
+                         table_name, table_name, current_db,
+                         20, "18446744073709551615");
+
+    verify_prepare_field(result, 2, "s1", "s1", MYSQL_TYPE_VAR_STRING,
+                         table_name, table_name, current_db,
+                         10, "s1def");
+
+    verify_prepare_field(result, 3, "d1", "d1", MYSQL_TYPE_NEWDECIMAL,
+                         table_name, table_name, current_db,
+                         33, "111111111122222222223333333333.9");
+
+    verify_prepare_field(result, 4, "t1", "t1", MYSQL_TYPE_DATETIME,
+                         table_name, table_name, current_db,
+                         26, "2001-01-01 10:20:30.123456");
+
+    verify_prepare_field(result, 5, "e1", "e1", MYSQL_TYPE_STRING,
+                         table_name, table_name, current_db,
+                         1, "a");
+
+    mysql_free_result(result);
+  }
+
+  myquery(mysql_query(mysql, "DROP VIEW v1"));
+  myquery(mysql_query(mysql, "DROP TABLE t1"));
+}
+
+
 static void test_bug19671()
 {
   MYSQL_RES *result;
@@ -19558,6 +19628,7 @@ static struct my_tests_st my_tests[]= {
   { "test_fetch_column", test_fetch_column },
   { "test_mem_overun", test_mem_overun },
   { "test_list_fields", test_list_fields },
+  { "test_list_fields_default", test_list_fields_default },
   { "test_free_result", test_free_result },
   { "test_free_store_result", test_free_store_result },
   { "test_sqlmode", test_sqlmode },