[Maria-developers] check_sql_mode
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. What is my mistake ? Best regards, Jérôme.
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. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
On 27 Dec 2016, at 11:35, Sergei Golubchik <serg@mariadb.org> 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
Thanks Simon, Sergei. Initially , I've done this change only for sql_mode=oracle (where concat null and a string doesn’t return null) but as SQLServer has an option to have the same behavior (set CONCAT_NULL_YIELDS_NULL ON|OFF), I was thinking to add a dedicated option. How replication will work if we add/change SQL function (for MDEV-10142 PL/SQL Parser) if slave hasn't the same level ? I supposed that in this case, only row_based logging is safe. Alexander, what is your point of view ? Sergei, all works fine with a my_set_bits64(). Jérôme. -----Message d'origine----- De : Simon J Mudd [mailto:simon.mudd@booking.com] Envoyé : mardi 27 décembre 2016 13:17 À : jerome brauge; serg@mariadb.org Cc : maria-developers@lists.launchpad.net Objet : Re: [External] [Maria-developers] check_sql_mode
On 27 Dec 2016, at 11:35, Sergei Golubchik <serg@mariadb.org> 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
Hi, Simon! On Dec 27, Simon J Mudd wrote:
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.
Yes, precisely. I thought today what should be the criteria for choosing between a new sql mode or a separate variable, like @@concat_null_is_null. And I realized that one such criteria could be - does it need to be stored *per routine*? Saved with every storage procedure, function, trigger and event definition?
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.
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.
I think that a global variable does not solve the replication issue, when slaves are of a different version. When slaves don't implement this configurable option (whether sql_mode or a separate variable), the only way for replication to work is not to change this option on the master. It means that for a new sql_mode the default backward compatible behavior should be "new sql mode is not set" and for the global variable the default value should provide the compatible behavior. Although a global variable does provide a benefit that only a SUPER user can change it (and thus break the replication). Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Sergei, On SQLServer these kind of option (ANSI_NULLS, CONCAT_NULL_YIELDS_NULL and so on ) can be set at session level or inside stored procedure (but not in function). In the last case, the scope of the setting is limited to the stored procedure. Jérôme. -----Message d'origine----- De : Sergei Golubchik [mailto:serg@mariadb.org] Envoyé : mardi 27 décembre 2016 17:01 À : Simon J Mudd Cc : jerome brauge; maria-developers@lists.launchpad.net Objet : Re: [External] [Maria-developers] check_sql_mode Hi, Simon! On Dec 27, Simon J Mudd wrote:
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.
Yes, precisely. I thought today what should be the criteria for choosing between a new sql mode or a separate variable, like @@concat_null_is_null. And I realized that one such criteria could be - does it need to be stored *per routine*? Saved with every storage procedure, function, trigger and event definition?
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.
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.
I think that a global variable does not solve the replication issue, when slaves are of a different version. When slaves don't implement this configurable option (whether sql_mode or a separate variable), the only way for replication to work is not to change this option on the master. It means that for a new sql_mode the default backward compatible behavior should be "new sql mode is not set" and for the global variable the default value should provide the compatible behavior. Although a global variable does provide a benefit that only a SUPER user can change it (and thus break the replication). Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, jerome! On Dec 27, jerome brauge wrote:
Sergei, On SQLServer these kind of option (ANSI_NULLS, CONCAT_NULL_YIELDS_NULL and so on) can be set at session level or inside stored procedure (but not in function). In the last case, the scope of the setting is limited to the stored procedure.
That's not what I mean. When you create a stored routine or, say, a trigger in MariaDB (or in MySQL, fwiw), it remembers the current value of sql_mode and automatically sets it temporarity when parsing or executing the routine. Because if you'd create a routine in the ANSI_QUOTES mode, it may not even parse without it. If you create it with PIPES_AS_CONCAT or HIGH_NOT_PRECEDENCE mode, it'll parse, but will work incorrectly. That is, to preserve the original semantics of a stored routine, it needs to work in the sql_mode in which it was created. On the other hand, say, join_buffer_size setting is not stored, it does not normally affect the semantics of a routine, it's just a tuning configuration parameter. So, sql_mode is not something you can set per routine, it's something that the server automatically needs to save/restore per routine to preserve this routine's semantics. In your case, I think, CONCAT_NULL_YIELDS_NULL fits in this category and thus it shoud be in the sql_mode.
Yes, precisely. I thought today what should be the criteria for choosing between a new sql mode or a separate variable, like @@concat_null_is_null.
And I realized that one such criteria could be - does it need to be stored *per routine*? Saved with every storage procedure, function, trigger and event definition?
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (3)
-
jerome brauge -
Sergei Golubchik -
Simon J Mudd