Sergei Golubchik <serg@stripped> writes:
> wrong comment, should be "throw warning, auto-correct the value"
True.
>> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
>> --- a/sql/sql_parse.cc 2007-12-13 15:26:24 +01:00
>> +++ b/sql/sql_parse.cc 2008-01-07 02:47:54 +01:00
>> @@ -3127,6 +3127,18 @@ end_with_restore_list:
>> thd->one_shot_set|= lex->one_shot_set;
>> send_ok(thd);
>> }
>> + else
>> + {
>> + /*
>> + We encountered some sort of error, but no message was sent.
>> + Send something semi-generic here since we don't know which
>> + assignment in the list caused the error.
>> + */
>> + if (!thd->is_error())
>> + my_error(ER_WRONG_ARGUMENTS,MYF(0),"SET");
>
> Can you create a test case that shows this error ?
Not throwing errors was the known way of running into the above
branch. Now that we do, that documented way of triggering that
issue has gone; I left the check in to avoid future regressions
as it comes at "no cost" performance-wise (even if we were to
argue "set var" is time-critical, we already did check against
failures anyway, and we only run into this new check on failure),
hence the check rather than DBUG_ASSERT(). Now that the original
problem was fixed elsewhere instead (in the plug-in interface), I
don't know of an assignment that triggers this, but that doesn't
mean it might not be possible to trigger this, especially where an
additional custom check_func is used.
>> diff -Nrup a/sql/sql_plugin.cc b/sql/sql_plugin.cc
>> --- a/sql/sql_plugin.cc 2007-12-13 12:49:55 +01:00
>> +++ b/sql/sql_plugin.cc 2008-01-07 02:47:54 +01:00
>> @@ -1916,16 +1928,28 @@ static int check_func_long(THD *thd, str
>> else
>> *(long *)save= (long) getopt_ll_limit_value(tmp, &options, &fixed);
>>
>> - if (fixed)
>> + if (*(long *)save != (long) tmp)
>
> why ?
> in set_var.cc you use didn't change 'fixed' to 'save != tmp'.
That's pre-existing code actually that just moved upwards within
the same function. 'tmp != save' is a superset of 'fixed';
fixed meaning 'variable was corrected for more than just block-
size.' The test moved within the function because we now throw
errors as needed, and if we do, we don't wish to throw a warning
as well, hence we can't test the error condition AFTER throwing
the warning like we did before.
Consequently I think that's asking the question the wrong way
around. The way I see it, it's more "Why did whoever wrote that
part of the plug-in interface add incompatible semantics without
explanation, and without changing the code-analogue in set_var?"
So if your question is, "Should STRICT affect SET in the first
place," then my answer certainly is, "beats me." I for one was
surprised to see that assumption coded; I don't necessarily find
it intuitive from the SQL level. To add insult to injury, the
return-types from the various check() functions vary (int with a
documented range of 1, 0, -1 ("error, message sent", "OK", "error,
no message sent") vs bool (error, no error), and it isn't immediately
obvious whether the signalling is always correct (int(1) signalled,
but no message was sent -- for a while, I wasn't even sure if it
shouldn't read "1 -- error, send message" and "-1 -- error, but
keep quiet"). This arguably bears investigation.
>> {
>> char buf[22];
>> + if (var->flags & PLUGIN_VAR_UNSIGNED)
>> + ullstr(tmp, buf);
>> + else
>> + llstr(tmp, buf);
>> +
>> + if (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES)
>> + {
>> + my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0),
>> + var->name, buf);
>> + return TRUE;
>> + }
>> + else if (fixed)
>> push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
>> ER_TRUNCATED_WRONG_VALUE,
>> ER(ER_TRUNCATED_WRONG_VALUE), var->name,
>> ullstr(tmp, buf));
>> }
>> - return (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES) &&
>> - (*(long *)save != (long) tmp);
>
> You didn't fix set_var.cc
It wasn't broken. You cannot, to my knowledge, get the hang from
built-in variables (but even if you could before, the failsafe
should catch that now, see above).
> and server built-in variables still
> auto-correct invalid values even in strict mode.
>
> Wouldn't you avoid code duplication if you add thsi check to
> throw_bounds_warning() ? (and use it in sql_plugin.cc, of course)
Gladly. But first, we need to know whether STRICT should even be
heeded at all, and if so, whether it should affect both, or, for
some reason we're unaware of, only plugins, and that's not my call
to make.
thanks/regards
Tatiana
--
Tatiana A. Nurnberg, Bugs Bunny * MySQL AB, http://www.mysql.de/ * EST
IRC: Azundris * Skype: azundris * MySQL-SIP: 4550 * eMail: azundris@stripped
.de: Firmensitz: MySQL GmbH, Radlkoferstr. 2, D-81373 Muenchen
Geschaeftsfuehrer: Hans von Bell, Kaj Arnoe - HRB Muenchen 162140
