Open Stack

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.

Cheers,
Florian