Re: [openstack-dev] [oslo][db] Mysql traditional session mode
On Thu, Jan 23, 2014 at 7:22 PM, Ben Nemec openst...@nemebean.com wrote: On 2014-01-23 12:03, Florian Haas wrote: Ben, thanks for taking this to the list. Apologies for my brevity and for HTML, I'm on a moving train and Android Gmail is kinda stupid. :) I have some experience with the quirks of phone GMail myself. :-) On Jan 23, 2014 6:46 PM, Ben Nemec openst...@nemebean.com wrote: A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. For the record, I obviously am all in favor of avoiding data corruption, although it seems not everyone agrees that TRADITIONAL is necessarily the preferable mode. But that aside, if Oslo decides that any particular mode is required, it should just go ahead and set it, rather than log a warning that the user can't possibly fix. Honestly, defaulting it to enabled was my preference in the first place. I got significant pushback though because it might break consuming applications that do the bad things traditional mode prevents. Wait. So the reasoning behind the pushback was that an INSERT that shreds data is better than an INSERT that fails? Really? My theory was that we could default it to off, log the warning, get all the projects to enable it as they can, and then flip the default to enabled. Obviously that hasn't all happened though. :-) Wouldn't you think it's a much better approach to enable whatever mode is deemed appropriate, and have malformed INSERTs (rightfully) break? Isn't that a much stronger incentive to actually fix broken code? The oslo tests do include a unit test for this, jftr, checking for an error to be raised when a 512-byte string is inserted into a 255-byte column. Hence my proposal to make this a config option. To make the patch as un-invasive as possible, the default for that option is currently empty, but if it seems prudent to set TRADITIONAL or STRICT_ALL_TABLES instead, I'll be happy to fix the patch up accordingly. Also check out Jay's reply. It sounds like there are some improvements we can make as far as not logging the message when the user enables traditional mode globally. And then when INSERTs break, it will be much more difficult for an application developer to figure out the problem, because the breakage would happen based on a configuration setting outside the codebase, and hence beyond the developer's control. I really don't like that idea. All this leads to is bugs being filed and then closed with a simple can't reproduce. I'm still not clear on whether there is a need for the STRICT_* modes, and if there is we should probably also allow STRICT_TRANS_TABLES since that appears to be part of strict mode in MySQL. In fact, if we're going to allow arbitrary modes, we may need a more flexible config option - it looks like there are a bunch of possible sql_modes available for people who don't want the blanket disallow all the things mode. Fair enough, I can remove the choices arg for the StrOpt, if that's what you suggest. My concern was about unsanitized user input. Your inline comment on my patch seems to indicate that we should instead trust sqla to do input sanitization properly. I still maintain that leaving $insert_mode_here mode off and logging a warning is silly. If it's necessary, turn it on and have borked INSERTs fail. If I understand the situation correctly, they would fail anyway the moment someone switches to, say, Postgres. Cheers, Florian ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][db] Mysql traditional session mode
On Fri, Jan 24, 2014 at 3:29 AM, Florian Haas flor...@hastexo.com wrote: On Thu, Jan 23, 2014 at 7:22 PM, Ben Nemec openst...@nemebean.com wrote: On 2014-01-23 12:03, Florian Haas wrote: Ben, thanks for taking this to the list. Apologies for my brevity and for HTML, I'm on a moving train and Android Gmail is kinda stupid. :) I have some experience with the quirks of phone GMail myself. :-) On Jan 23, 2014 6:46 PM, Ben Nemec openst...@nemebean.com wrote: A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. For the record, I obviously am all in favor of avoiding data corruption, although it seems not everyone agrees that TRADITIONAL is necessarily the preferable mode. But that aside, if Oslo decides that any particular mode is required, it should just go ahead and set it, rather than log a warning that the user can't possibly fix. Honestly, defaulting it to enabled was my preference in the first place. I got significant pushback though because it might break consuming applications that do the bad things traditional mode prevents. Wait. So the reasoning behind the pushback was that an INSERT that shreds data is better than an INSERT that fails? Really? My theory was that we could default it to off, log the warning, get all the projects to enable it as they can, and then flip the default to enabled. Obviously that hasn't all happened though. :-) Wouldn't you think it's a much better approach to enable whatever mode is deemed appropriate, and have malformed INSERTs (rightfully) break? Isn't that a much stronger incentive to actually fix broken code? The oslo tests do include a unit test for this, jftr, checking for an error to be raised when a 512-byte string is inserted into a 255-byte column. Hence my proposal to make this a config option. To make the patch as un-invasive as possible, the default for that option is currently empty, but if it seems prudent to set TRADITIONAL or STRICT_ALL_TABLES instead, I'll be happy to fix the patch up accordingly. Also check out Jay's reply. It sounds like there are some improvements we can make as far as not logging the message when the user enables traditional mode globally. And then when INSERTs break, it will be much more difficult for an application developer to figure out the problem, because the breakage would happen based on a configuration setting outside the codebase, and hence beyond the developer's control. I really don't like that idea. All this leads to is bugs being filed and then closed with a simple can't reproduce. I'm still not clear on whether there is a need for the STRICT_* modes, and if there is we should probably also allow STRICT_TRANS_TABLES since that appears to be part of strict mode in MySQL. In fact, if we're going to allow arbitrary modes, we may need a more flexible config option - it looks like there are a bunch of possible sql_modes available for people who don't want the blanket disallow all the things mode. Fair enough, I can remove the choices arg for the StrOpt, if that's what you suggest. My concern was about unsanitized user input. Your inline comment on my patch seems to indicate that we should instead trust sqla to do input sanitization properly. I still maintain that leaving $insert_mode_here mode off and logging a warning is silly. If it's necessary, turn it on and have borked INSERTs fail. If I understand the situation correctly, they would fail anyway the moment someone switches to, say, Postgres. +1 Doug Cheers, Florian ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][db] Mysql traditional session mode
On Fri, Jan 24, 2014 at 4:30 PM, Doug Hellmann doug.hellm...@dreamhost.com wrote: On Fri, Jan 24, 2014 at 3:29 AM, Florian Haas flor...@hastexo.com wrote: On Thu, Jan 23, 2014 at 7:22 PM, Ben Nemec openst...@nemebean.com wrote: On 2014-01-23 12:03, Florian Haas wrote: Ben, thanks for taking this to the list. Apologies for my brevity and for HTML, I'm on a moving train and Android Gmail is kinda stupid. :) I have some experience with the quirks of phone GMail myself. :-) On Jan 23, 2014 6:46 PM, Ben Nemec openst...@nemebean.com wrote: A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. For the record, I obviously am all in favor of avoiding data corruption, although it seems not everyone agrees that TRADITIONAL is necessarily the preferable mode. But that aside, if Oslo decides that any particular mode is required, it should just go ahead and set it, rather than log a warning that the user can't possibly fix. Honestly, defaulting it to enabled was my preference in the first place. I got significant pushback though because it might break consuming applications that do the bad things traditional mode prevents. Wait. So the reasoning behind the pushback was that an INSERT that shreds data is better than an INSERT that fails? Really? My theory was that we could default it to off, log the warning, get all the projects to enable it as they can, and then flip the default to enabled. Obviously that hasn't all happened though. :-) Wouldn't you think it's a much better approach to enable whatever mode is deemed appropriate, and have malformed INSERTs (rightfully) break? Isn't that a much stronger incentive to actually fix broken code? The oslo tests do include a unit test for this, jftr, checking for an error to be raised when a 512-byte string is inserted into a 255-byte column. Hence my proposal to make this a config option. To make the patch as un-invasive as possible, the default for that option is currently empty, but if it seems prudent to set TRADITIONAL or STRICT_ALL_TABLES instead, I'll be happy to fix the patch up accordingly. Also check out Jay's reply. It sounds like there are some improvements we can make as far as not logging the message when the user enables traditional mode globally. And then when INSERTs break, it will be much more difficult for an application developer to figure out the problem, because the breakage would happen based on a configuration setting outside the codebase, and hence beyond the developer's control. I really don't like that idea. All this leads to is bugs being filed and then closed with a simple can't reproduce. I'm still not clear on whether there is a need for the STRICT_* modes, and if there is we should probably also allow STRICT_TRANS_TABLES since that appears to be part of strict mode in MySQL. In fact, if we're going to allow arbitrary modes, we may need a more flexible config option - it looks like there are a bunch of possible sql_modes available for people who don't want the blanket disallow all the things mode. Fair enough, I can remove the choices arg for the StrOpt, if that's what you suggest. My concern was about unsanitized user input. Your inline comment on my patch seems to indicate that we should instead trust sqla to do input sanitization properly. I still maintain that leaving $insert_mode_here mode off and logging a warning is silly. If it's necessary, turn it on and have borked INSERTs fail. If I understand the situation correctly, they would fail anyway the moment someone switches to, say, Postgres. +1 Doug Updated patch set: https://review.openstack.org/#/q/status:open+project:openstack/oslo-incubator+branch:master+topic:bug-1271706,n,z Cheers, Florian ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [oslo][db] Mysql traditional session mode
A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. I remain of the opinion that traditional mode is a good thing and we _should_ be enabling it. I would call silent truncation and bogus date values bugs that should be fixed, but maybe there are other implications of this mode that I'm not aware of. It was also pointed out that the warning is logged even if the user forces traditional mode through my.cnf. While this certainly solves the underlying problem, it doesn't change the fact that the application was trying to do something bad. We tried to make it clear in the log message that this is a developer problem and the user needs to pester the developer to enable the mode, but maybe there's more discussion that needs to go on there as well. Any thoughts on this would be welcomed. Thanks. -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][db] Mysql traditional session mode
On Thu, 2014-01-23 at 11:29 -0600, Ben Nemec wrote: A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. I remain of the opinion that traditional mode is a good thing and we _should_ be enabling it. I would call silent truncation and bogus date values bugs that should be fixed, but maybe there are other implications of this mode that I'm not aware of. Why traditional? Why not STRICT_ALL_TABLES? It was also pointed out that the warning is logged even if the user forces traditional mode through my.cnf. While this certainly solves the underlying problem, it doesn't change the fact that the application was trying to do something bad. We tried to make it clear in the log message that this is a developer problem and the user needs to pester the developer to enable the mode, but maybe there's more discussion that needs to go on there as well. What I was trying to point out with that is that if I see a warning in a log file about not enabling traditional mode, and yet I've set my my.cnf server sql_mode to STRICT_TRANS_TABLES, TRADITIONAL, or STRICT_ALL_TABLES, then I shouldn't see a warning in the code... It's easy enough to check... SHOW [GLOBAL] VARIABLES LIKE 'sql_mode'... Best, -jay Any thoughts on this would be welcomed. Thanks. -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][db] Mysql traditional session mode
Ben, thanks for taking this to the list. Apologies for my brevity and for HTML, I'm on a moving train and Android Gmail is kinda stupid. :) On Jan 23, 2014 6:46 PM, Ben Nemec openst...@nemebean.com wrote: A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention ( https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. For the record, I obviously am all in favor of avoiding data corruption, although it seems not everyone agrees that TRADITIONAL is necessarily the preferable mode. But that aside, if Oslo decides that any particular mode is required, it should just go ahead and set it, rather than log a warning that the user can't possibly fix. I remain of the opinion that traditional mode is a good thing and we _should_ be enabling it. I would call silent truncation and bogus date values bugs that should be fixed, but maybe there are other implications of this mode that I'm not aware of. It was also pointed out that the warning is logged even if the user forces traditional mode through my.cnf. While this certainly solves the underlying problem, it doesn't change the fact that the application was trying to do something bad. We tried to make it clear in the log message that this is a developer problem and the user needs to pester the developer to enable the mode, but maybe there's more discussion that needs to go on there as well. Hence my proposal to make this a config option and actually set the mode on connect. To make the patch as un-invasive as possible, the default for that option is currently empty, but if it seems prudent to set TRADITIONAL or STRICT_ALL_TABLES instead, I'll be happy to fix the patch up accordingly. Cheers, Florian ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][db] Mysql traditional session mode
On 2014-01-23 11:50, Jay Pipes wrote: On Thu, 2014-01-23 at 11:29 -0600, Ben Nemec wrote: A while back a change (https://review.openstack.org/#/c/47820/) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. I remain of the opinion that traditional mode is a good thing and we _should_ be enabling it. I would call silent truncation and bogus date values bugs that should be fixed, but maybe there are other implications of this mode that I'm not aware of. Why traditional? Why not STRICT_ALL_TABLES? If I'm reading the documentation correctly, traditional is actually more strict than the STRICT_* modes. According to http://www.mysqlfaqs.net/mysql-faqs/Client-Server-Commands/What-is-sql-mode-in-MySQL-and-how-can-we-set-it , The TRADITIONAL mode, enables strict mode plus other restrictions on date checking and division by zero. which jives with what I see in the official documentation. Note that strict mode in this case refers to the combination of STRICT_TRANS_TABLES and STRICT_ALL_TABLES. It was also pointed out that the warning is logged even if the user forces traditional mode through my.cnf. While this certainly solves the underlying problem, it doesn't change the fact that the application was trying to do something bad. We tried to make it clear in the log message that this is a developer problem and the user needs to pester the developer to enable the mode, but maybe there's more discussion that needs to go on there as well. What I was trying to point out with that is that if I see a warning in a log file about not enabling traditional mode, and yet I've set my my.cnf server sql_mode to STRICT_TRANS_TABLES, TRADITIONAL, or STRICT_ALL_TABLES, then I shouldn't see a warning in the code... It's easy enough to check... SHOW [GLOBAL] VARIABLES LIKE 'sql_mode'... Yeah, we could do that. I guess part of the problem is that if an application works in traditional mode the developers should be enabling it themselves, and if it doesn't then setting it manually in my.cnf isn't going to work anyway. But I suppose if someone knows to set it in my.cnf then they hopefully understand the implications and can figure out for themselves whether it works. I could get on board with suppressing the warning when it's set in my.cnf. -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][db] Mysql traditional session mode
On 2014-01-23 12:03, Florian Haas wrote: Ben, thanks for taking this to the list. Apologies for my brevity and for HTML, I'm on a moving train and Android Gmail is kinda stupid. :) I have some experience with the quirks of phone GMail myself. :-) On Jan 23, 2014 6:46 PM, Ben Nemec openst...@nemebean.com wrote: A while back a change (https://review.openstack.org/#/c/47820/ [1]) was made to allow enabling mysql traditional mode, which tightens up mysql's input checking to disallow things like silent truncation of strings that exceed the column's allowed length and invalid dates (as I understand it). IMHO, some compelling arguments were made that we should always be using traditional mode and as such we started logging a warning if it was not enabled. It has recently come to my attention (https://review.openstack.org/#/c/68474/ [2]) that not everyone agrees, so I wanted to bring it to the list to get as wide an audience for the discussion as possible and hopefully come to a consensus so we don't end up having this discussion every few months. For the record, I obviously am all in favor of avoiding data corruption, although it seems not everyone agrees that TRADITIONAL is necessarily the preferable mode. But that aside, if Oslo decides that any particular mode is required, it should just go ahead and set it, rather than log a warning that the user can't possibly fix. Honestly, defaulting it to enabled was my preference in the first place. I got significant pushback though because it might break consuming applications that do the bad things traditional mode prevents. My theory was that we could default it to off, log the warning, get all the projects to enable it as they can, and then flip the default to enabled. Obviously that hasn't all happened though. :-) I remain of the opinion that traditional mode is a good thing and we _should_ be enabling it. I would call silent truncation and bogus date values bugs that should be fixed, but maybe there are other implications of this mode that I'm not aware of. It was also pointed out that the warning is logged even if the user forces traditional mode through my.cnf. While this certainly solves the underlying problem, it doesn't change the fact that the application was trying to do something bad. We tried to make it clear in the log message that this is a developer problem and the user needs to pester the developer to enable the mode, but maybe there's more discussion that needs to go on there as well. Hence my proposal to make this a config option. To make the patch as un-invasive as possible, the default for that option is currently empty, but if it seems prudent to set TRADITIONAL or STRICT_ALL_TABLES instead, I'll be happy to fix the patch up accordingly. Also check out Jay's reply. It sounds like there are some improvements we can make as far as not logging the message when the user enables traditional mode globally. I'm still not clear on whether there is a need for the STRICT_* modes, and if there is we should probably also allow STRICT_TRANS_TABLES since that appears to be part of strict mode in MySQL. In fact, if we're going to allow arbitrary modes, we may need a more flexible config option - it looks like there are a bunch of possible sql_modes available for people who don't want the blanket disallow all the things mode. Cheers, Florian Links: -- [1] https://review.openstack.org/#/c/47820/ [2] https://review.openstack.org/#/c/68474/ ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
