[Maria-developers] Please review MDEV-10386 Assertion `fixed == 1' failed in virtual String* Item_func_conv_charset::val_str(String*)
Hi, Alexander! On Dec 19, Alexander Barkov wrote:
Hello Sergei,
Please review a patch for MDEV-10386.
Thanks!
diff --git a/sql/item.cc b/sql/item.cc index 53666aa..44bd620 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -1164,7 +1164,9 @@ Item *Item_cache::safe_charset_converter(CHARSET_INFO *tocs) if (conv == example) return this; Item_cache *cache; - if (!conv || !(cache= new Item_cache_str(conv))) + if (!conv || + conv->fix_fields(current_thd, (Item **) NULL) || + !(cache= new Item_cache_str(conv))) return NULL; // Safe conversion is not possible, or OEM cache->setup(conv); cache->fixed= false; // Make Item::fix_fields() happy
Difficult to review without a commit comment. What's going on here? How did it work before without fix_fields? Why is it suddenly needed? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hello Sergei, On 12/21/2016 12:48 AM, Sergei Golubchik wrote:
Hi, Alexander!
On Dec 19, Alexander Barkov wrote:
Hello Sergei,
Please review a patch for MDEV-10386.
Thanks!
diff --git a/sql/item.cc b/sql/item.cc index 53666aa..44bd620 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -1164,7 +1164,9 @@ Item *Item_cache::safe_charset_converter(CHARSET_INFO *tocs) if (conv == example) return this; Item_cache *cache; - if (!conv || !(cache= new Item_cache_str(conv))) + if (!conv || + conv->fix_fields(current_thd, (Item **) NULL) || + !(cache= new Item_cache_str(conv))) return NULL; // Safe conversion is not possible, or OEM cache->setup(conv); cache->fixed= false; // Make Item::fix_fields() happy
Difficult to review without a commit comment. What's going on here? How did it work before without fix_fields?
I guess that the problem sneaked in with this commit: b96c196f1cd5d77e524cbf952539bdd33c65ffc1 I didn't try to revert it though. But it seems that this patch added a new call for safe_charset_converter() without a corresponding fix_fields(). My patch eliminates this disbalance.
Why is it suddenly needed?
Looks like a rarely used combination and it was not covered in mtr. Btw, b96c196f1cd5d77e524cbf952539bdd33c65ffc1 doesn't have tests for some reasons, neither has an MDEV. Note, in case of literals it worked fine. Only subselects were affected. Please suggest a good commit comment. Thanks!
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Alexander! On Dec 21, Alexander Barkov wrote:
diff --git a/sql/item.cc b/sql/item.cc index 53666aa..44bd620 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -1164,7 +1164,9 @@ Item *Item_cache::safe_charset_converter(CHARSET_INFO *tocs) if (conv == example) return this; Item_cache *cache; - if (!conv || !(cache= new Item_cache_str(conv))) + if (!conv || + conv->fix_fields(current_thd, (Item **) NULL) || + !(cache= new Item_cache_str(conv))) return NULL; // Safe conversion is not possible, or OEM cache->setup(conv); cache->fixed= false; // Make Item::fix_fields() happy
Difficult to review without a commit comment. What's going on here? How did it work before without fix_fields?
I guess that the problem sneaked in with this commit: b96c196f1cd5d77e524cbf952539bdd33c65ffc1
I didn't try to revert it though. But it seems that this patch added a new call for safe_charset_converter() without a corresponding fix_fields(). My patch eliminates this disbalance.
Why is it suddenly needed?
Looks like a rarely used combination and it was not covered in mtr. Btw, b96c196f1cd5d77e524cbf952539bdd33c65ffc1 doesn't have tests for some reasons, neither has an MDEV.
Note, in case of literals it worked fine. Only subselects were affected.
okay
Please suggest a good commit comment.
Just explain all the above there. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik