Open Stack

On Fri, Jan 24, 2014 at 4:30 PM, Doug Hellmann
<doug.hellmann at dreamhost.com> wrote:
>
>
>
> On Fri, Jan 24, 2014 at 3:29 AM, Florian Haas <florian at hastexo.com> wrote:
>>
>> On Thu, Jan 23, 2014 at 7:22 PM, Ben Nemec <openstack at 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" <openstack at 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