<div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jan 24, 2014 at 3:29 AM, Florian Haas <span dir="ltr"><<a href="mailto:florian@hastexo.com" target="_blank">florian@hastexo.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Jan 23, 2014 at 7:22 PM, Ben Nemec <<a href="mailto:openstack@nemebean.com">openstack@nemebean.com</a>> wrote:<br>

> On 2014-01-23 12:03, Florian Haas wrote:<br>
><br>
> Ben,<br>
><br>
> thanks for taking this to the list. Apologies for my brevity and for HTML,<br>
> I'm on a moving train and Android Gmail is kinda stupid. :)<br>
><br>
> I have some experience with the quirks of phone GMail myself. :-)<br>
><br>
> On Jan 23, 2014 6:46 PM, "Ben Nemec" <<a href="mailto:openstack@nemebean.com">openstack@nemebean.com</a>> wrote:<br>
>><br>
>> A while back a change (<a href="https://review.openstack.org/#/c/47820/" target="_blank">https://review.openstack.org/#/c/47820/</a>) was made<br>
>> to allow enabling mysql traditional mode, which tightens up mysql's input<br>
>> checking to disallow things like silent truncation of strings that exceed<br>
>> the column's allowed length and invalid dates (as I understand it).<br>
>><br>
>> IMHO, some compelling arguments were made that we should always be using<br>
>> traditional mode and as such we started logging a warning if it was not<br>
>> enabled.  It has recently come to my attention<br>
>> (<a href="https://review.openstack.org/#/c/68474/" target="_blank">https://review.openstack.org/#/c/68474/</a>) that not everyone agrees, so I<br>
>> wanted to bring it to the list to get as wide an audience for the discussion<br>
>> as possible and hopefully come to a consensus so we don't end up having this<br>
>> discussion every few months.<br>
><br>
> For the record, I obviously am all in favor of avoiding data corruption,<br>
> although it seems not everyone agrees that TRADITIONAL is necessarily the<br>
> preferable mode. But that aside, if Oslo decides that any particular mode is<br>
> required, it should just go ahead and set it, rather than log a warning that<br>
> the user can't possibly fix.<br>
><br>
><br>
> Honestly, defaulting it to enabled was my preference in the first place.  I<br>
> got significant pushback though because it might break consuming<br>
> applications that do the bad things traditional mode prevents.<br>
<br>
</div></div>Wait. So the reasoning behind the pushback was that an INSERT that<br>
shreds data is better than an INSERT that fails? Really?<br>
<div class="im"><br>
> My theory<br>
> was that we could default it to off, log the warning, get all the projects<br>
> to enable it as they can, and then flip the default to enabled.  Obviously<br>
> that hasn't all happened though. :-)<br>
<br>
</div>Wouldn't you think it's a much better approach to enable whatever mode<br>
is deemed appropriate, and have malformed INSERTs (rightfully) break?<br>
Isn't that a much stronger incentive to actually fix broken code?<br>
<br>
The oslo tests do include a unit test for this, jftr, checking for an<br>
error to be raised when a 512-byte string is inserted into a 255-byte<br>
column.<br>
<div class="im"><br>
> Hence my proposal to make this a config option. To make the patch as<br>
> un-invasive as possible, the default for that option is currently empty, but<br>
> if it seems prudent to set TRADITIONAL or STRICT_ALL_TABLES instead, I'll be<br>
> happy to fix the patch up accordingly.<br>
><br>
> Also check out Jay's reply.  It sounds like there are some improvements we<br>
> can make as far as not logging the message when the user enables traditional<br>
> mode globally.<br>
<br>
</div>And then when INSERTs break, it will be much more difficult for an<br>
application developer to figure out the problem, because the breakage<br>
would happen based on a configuration setting outside the codebase,<br>
and hence beyond the developer's control. I really don't like that<br>
idea. All this leads to is bugs being filed and then closed with a<br>
simple "can't reproduce."<br>
<div class="im"><br>
> I'm still not clear on whether there is a need for the STRICT_* modes, and<br>
> if there is we should probably also allow STRICT_TRANS_TABLES since that<br>
> appears to be part of "strict mode" in MySQL.  In fact, if we're going to<br>
> allow arbitrary modes, we may need a more flexible config option - it looks<br>
> like there are a bunch of possible sql_modes available for people who don't<br>
> want the blanket "disallow all the things" mode.<br>
<br>
</div>Fair enough, I can remove the "choices" arg for the StrOpt, if that's<br>
what you suggest. My concern was about unsanitized user input. Your<br>
inline comment on my patch seems to indicate that we should instead<br>
trust sqla to do input sanitization properly.<br>
<br>
I still maintain that leaving $insert_mode_here mode off and logging a<br>
warning is silly. If it's necessary, turn it on and have borked<br>
INSERTs fail. If I understand the situation correctly, they would fail<br>
anyway the moment someone switches to, say, Postgres.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">+1</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">
Doug</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Florian<br>
<div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div></div>