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