Hi Sergei, On 12/28/2016 08:01 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Dec 26, Alexander Barkov wrote:

...

Hello Sergei,

can you please review a fix for MDEV-11134.

Sure. Where is it? :)

Sorry, forgot to attach it.

Regards, Sergei Chief Architect MariaDB and security@mariadb.org

Hello Sergei, On 12/29/2016 05:33 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Dec 26, Alexander Barkov wrote:

...

Hello Sergei,

can you please review a fix for MDEV-11134.

I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table().

I thought that normally basic const items are always fixed. So, Item_param is an exception? It's basic const, but not fixed?

...

But perhaps it can be done in different ways:

1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false.

or

2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1)

Yes, I think it's reasonable. It'll make Item_param behave as other basic constants do.

Please find a new version attached. It makes sure that Item_param::fixed, Item_param::state and Item_param::item_type are in sync to each other. Thanks!

Regards, Sergei Chief Architect MariaDB and security@mariadb.org

Hi Sergei, On 01/23/2017 07:50 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Jan 11, Alexander Barkov wrote:

...

commit adf6bb91e349e8983b146cbfcaaa002edff54fe5 Author: Alexander Barkov Date: Wed Jan 11 13:42:52 2017 +0400

MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)

Problem: Item_param::basic_const_item() returned true when fixed==false. This unexpected combination made Item::const_charset_converter() crash on asserts.

Fix: - Changing all Item_param::set_xxx() to set "fixed" to true. This fixes the problem. - Additionally, changing all Item_param::set_xxx() to set Item_param::item_type, to avoid duplicate code, and for consistency, to make the code symmetric between different constant types. Before this patch only set_null() set item_type. - Moving Item_param::state and Item_param::item_type from public to private, to make sure easier that these members are in sync with "fixed" and to each other. - Adding a new argument "unsigned_arg" to Item::set_decimal(), and reusing it in two places instead of duplicate code. - Adding a new method Item_param::fix_temporal() and reusing it in two places. - Adding methods is_no_value(), is_long_data_value(), is_int_value(), instead of direct access to Item_param::state.

Looks ok to push.

Stylistic comments: * too many fixed= true in all set_* methods. I'd rather added a method fix_type(), to be used like

- type= Item::STRING_ITEM + fix_type(Item::STRING_ITEM)

and in this method I'd { type=type_arg; fixed= true }.

* names like is_no_value/is_int_value/etc look strange, I'd use has_no_value, has_int_value, etc

Thanks! Addressed your suggestions and pushed.

Regards, Sergei Chief Architect MariaDB and security@mariadb.org