Hi! On Fri, Apr 16, 2021 at 5:44 PM Sergei Golubchik <serg@mariadb.org> wrote:
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index aecb00563f7..b23522ac830 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7975,3 +7975,5 @@ ER_PK_INDEX_CANT_BE_IGNORED eng "A primary key cannot be marked as IGNORE" ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE eng "Function '%s' cannot be used in the %s clause" +ER_ORACLE_COMPAT_FUNCTION_ERROR + eng "Oracle compatibility function error: %s"
Why? We normally just say "invalid argument" or something, in no other case we say "oracle compatibility function" or "sybase compatibility function" or "odbc compatibility function".
Fixed by reusing ER_STD_INVALID_ARGUMENT
diff --git a/sql/sql_string.cc b/sql/sql_string.cc index 95a57017c53..9c57bb22085 100644 --- a/sql/sql_string.cc +++ b/sql/sql_string.cc @@ -1275,3 +1275,15 @@ void Binary_string::shrink(size_t arg_length) } } } + +bool Binary_string::strfill(char fill, size_t len) +{ + if (len) + { + if (alloc(length() + len)) + return 1; + memset(Ptr + str_length, fill, len); + str_length+= (uint32) len; + } + return 0; +}
There's Binary_string::fill() already.
better use it or, at the very least, declare
bool strfill(char fill, size_t len) { return fill(str_length + len, fill); }
in sql_string.h. And this swapped order of arguments is confusing, please fix it too.
Agree, I did miss this when fixing a fill loop in the original commit. I have now removed strfill(), as you requested. <cut>
diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h index af266956b05..9b78d6c159e 100644 --- a/sql/item_timefunc.h +++ b/sql/item_timefunc.h
<cut>
+public: + Item_func_tochar(THD *thd, Item *a, Item *b): + Item_str_func(thd, a, b), locale(0) + { + /* NOTE: max length of warning message is 64 */ + warning_message.alloc(64); + warning_message.length(0); + }
As far as I understand, this warning_message was introduced to issue the same error for every row, even if the format string is const_item and is parsed only once, in fix_fields.
I don't think it's worth the trouble. Two simpler approaches are: * if the format string is invalid - parse it every time even if const, or * if the const format string is invalid - issue the error only once.
so, please, remove warning_message and just use push_warning or my_error where the error is discovered.
That may not work well for prepared statements or stored procdures where we may not call fix_length_and_dec() twice. I also think it is a bit wrong that one gets a different number of warning messages depending on when the arguments are processed. For now, I rather keep things as they are. Someone that has time to do checking of the above cases and ensures that we do not miss any warnings messages in some context can then consider what to do.
--- a/sql/item_timefunc.cc +++ b/sql/item_timefunc.cc
<cut>
+ Models for datetime, used by TO_CHAR/TO_DATE. Normal format characters are + stored as short integer < 256, while format characters are stored as a + integer > 256 +*/ + +#define FMT_BASE 128
128? or 256?
128. I have fixed the error message
+#define FMT_AD FMT_BASE+1 +#define FMT_AD_DOT FMT_BASE+2 +#define FMT_AM FMT_BASE+3 +#define FMT_AM_DOT FMT_BASE+4 <cut>
Not enum? Not even safe (with parentheses) #define? enum would be ideal here but at the very least make these defines safe.
This is 100% safe in the context it is used. However, I agree that enum is better and I have now converted the above to an enum.
+ +/** + Modify the quotation flag and check whether the subsequent process is skipped
Could you reword it please?
Changed to: Flip 'quotation_flag' if we found a quote (") character. The old function comment explains this in more detail.
+ + @param cftm Character or FMT... format descriptor + @param quotation_flag Points to 'true' if we are inside a quoted string + + @return true If we are inside a quoted string or if we found a '"' character + @return false Otherwise +*/ + +static inline bool check_quotation(uint16 cfmt, bool *quotation_flag) +{ + if (cfmt == '"') + { + *quotation_flag= !*quotation_flag; + return true; + } + return *quotation_flag; +} + +#define INVALID_CHARACTER(x) (((x) >= 'A' && (x) <= 'Z') ||((x) >= '0' && (x) <= '9') || (x) >= 127 || ((x) < 32))
why not to make this static inline too? side-effect safe, no risk of double evaluation of x.
The above is safe in the context it is used. For simple things like this, which is only used in a few places in the current file, I personally prefer #defines, so I am ok with the current code. Regards, Monty