maria-developers team mailing list archive

[Simon J Mudd <simon.mudd@xxxxxxxxxxx>]

> On 27 Dec 2016, at 11:35, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> 
> Hi, jerome!
> 
> On Dec 27, jerome brauge wrote:
>> Hello,
>> I'm trying to add an option to sql_mode.
>> For this, I'm adding in sql_class.h :
>> #define MODE_CONCAT_NULL_YIELDS_NULL    (1ULL << 32)
>> 
>> But now, my serveur doesn't start and show "Sysvar 'sql_mode' failed 'def_val <= my_set_bits(typelib.count)'"
>> The problem seems to be in sys_vars.ic (Sys_var_set) :
>> 
>>  {
>>    option.var_type|= GET_SET;
>>    global_var(ulonglong)= def_val;
>>    SYSVAR_ASSERT(typelib.count > 0);
>>    SYSVAR_ASSERT(typelib.count <= 64);
>>    SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count));
>>    SYSVAR_ASSERT(size == sizeof(ulonglong));
>>  }
>> 
>> Function my_set_bits works on 32bit and typelib.count may be up to 64.
>> In my case typelib.count is 33 and my_set_bits(33) returns 1, and the assert is false.
> 
> I'd say it's a bug.
> It seems that we didn't have any Sys_var_set variable with more than 32
> elements in the set yet, so it didn't show up.
> 
> To continue, I think, you can fix my_set_bits to work on ulonglongs.
> Or create a second my_set_bits64().
> 
> Btw, I suspect you'll see more sql_mode issues caused by you going
> over 32 bits.

Slightly off topic but changes in sql_mode has the potential to cause problems with major version changes and incompatible behaviour.
So combining all sorts of new settings into this variable does not seem like a good idea to me. It’s already rather complex.

Note: don’t just think of how the server behaves with this change but also think of replication. Downstream slaves
may not be running the same (major or minor) version of MariaDB (or MySQL) and sql_mode is fed into the binlog stream so will be interpreted
by the SQL thread(s) of the downstream slaves too.

Consequently behavioural changes like this by adding new values may not be as simple as you expect.

I’d be much more comfortable using a new global variable which defines this behaviour, though that _may_
require the setting being propagated via the binlog stream if setting the value globally on the downstream slave
is not good enough.

Just a thought.

Simon