- developers - lists.mariadb.org

[Maria-developers] Item_func_group_concat::fix_fields - indentation and check positioning?
by Daniel Black 18 Apr '17

18 Apr '17

I came across the following gcc warning (6.3.1) sql/item_sum.cc: In member function ‘virtual bool Item_func_group_concat::fix_fields(THD*, Item**)’: sql/item_sum.cc:3478:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] if ((!args[i]->fixed && ^~ sql/item_sum.cc:3482:7: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’ with_subselect|= args[i]->with_subselect; ^~~~~~~~~~~~~~ The with_subselect line was added in 950abd526 In context it is: Item_func_group_concat::fix_fields(THD *thd, Item **ref) { ... for (i=0 ; i < arg_count ; i++) { if ((!args[i]->fixed && args[i]->fix_fields(thd, args + i)) || args[i]->check_cols(1)) return TRUE; with_subselect|= args[i]->with_subselect; } While removing the indent would fix the warning, I'd like someone to check this line should be after the IF statement rather than before.

2 1

Re: [Maria-developers] [JIRA] (MDEV-12499) ARCHIVE tables still don't show up in I_S.TABLES when storage engine is not loaded
by Jean-Francois B. Gagne 17 Apr '17

17 Apr '17

Hi Sergei, (sorry for cross-posting to MariaDB.dev and MariaDB.discuss: I think this interest both audiences.) Sergei Golubchik commented on MDEV-12499: > ----------------------------------------- > > This only happens if you created an ARCHIVE table and never used it before > the upgrade. No INSERT, no SELECT, not even {{SELECT * FROM > INFORMATION_SCHEMA.TABLES}} — nothing. > > ARCHIVE engine supports table discovery, that is, the engine is always the > authoritative source of table metadata, while {{.frm}} files are merely a > metadata cache. When you do {{CREATE TABLE}} the server does not know > whether the engine wants the table metadata to be cached. This happens when > the table is opened for the first time. > > If there is no {{.frm}} file and the ARCHIVE engine is not loaded, the > server has no way of knowing that the ARCHIVE table exists. That's why it's > not shown anywhere. > > If you do {{SELECT * FROM test.a1}} before the upgrade, the engine will > tell the server to create the {{.frm}} file and on the upgrade the plugin > will be loaded. In a practical use case, it's enough to have at least one > ARCHIVE table that was accessed at least once after it was created, and the > ARCHIVE plugin will be loaded on upgrade. > Thanks for the detailed info above. In this, you write ".frm files are merely a metadata cache", is there any sort of documentation about when .frm files are useful, useless, and might get out of sync ? This probably deserves a page in the KB (and Google does not know much on "mariadb frm file site:mariadb.com"). However, your comment above raises the following question: if "ARCHIVE engine supports table discovery, that is, the engine is always the authoritative source of table metadata", where is this information stored ? I guess that unless the plugin is loaded, this information is not accessible. This causes a chicken and egg problem which MDEV-12499 was trying to solve: how can mysql_upgrade knows if it should add BLACKHOLE and ARCHIVE to the list of plugin to load (see MDEV-11942). Finally, you marked MDEV-12499 as "Won't Fix", do you have another solution for ARCHIVE and MDEV-11942 ? Many thanks, JFG

2 1

[Maria-developers] MDEV-12514 Split Item_temporal_func::fix_length_and_dec()
by Alexander Barkov 17 Apr '17

17 Apr '17

Hi Sanja, Please review a patch for MDEV-12514, also fixing this bug: MDEV-12515 Wrong value when storing DATE_ADD() and ADDTIME() to a numeric field Thanks!

1 1

[Maria-developers] MDEV-12506 Split Item_func_min_max::fix_length_and_dec() into methods in Type_handler
by Alexander Barkov 15 Apr '17

15 Apr '17

Hello Alexey, Can you please review a patch for MDEV-12506? Thanks.

1 0

[Maria-developers] Please review patch for 12017 and 12018.
by Sachin Setiya 09 Apr '17

09 Apr '17

Hi Monty!, Please review patch for 12017 and 12018 . this patch solves both the problem. -- Regards Sachin Setiya Software Engineer at MariaDB

2 1

[Maria-developers] compilation failure: my_atomic.h
by Alexander Barkov 08 Apr '17

08 Apr '17

Hi Sergey, kvm-bintar-centos5-x86 fails to compile: http://buildbot.askmonty.org/buildbot/grid?category=main&branch=10.3 with this error: /home/buildbot/buildbot/build/include/my_atomic.h:121:2: error: #error atomic ops for this platform are not implemented Can you please fix this?

3 4

[Maria-developers] Adding "proxy protocol" support to MariaDB
by Jose 07 Apr '17

07 Apr '17

Hi! As you may know, the proxy protocol [1,2] is a way to communicate a client's IP address to a backend server sitting behind a proxy front-end. It is the equivalent of the X-Forwarded-For HTTP header, but done at Layer 4. This protocol was originally proposed by one of the HAProxy developers and its support is becoming more widespread. Percona, Amazon AWS, HAProxy, NGINX, and others. Mariadb's Jira shows an unassigned feature request for it, but with no one apparently assigned to it [3]. Percona supports proxy protocol [4]. For the configuration part, Percona defines a global variable that gives the IP address of the proxy front-ends that are authorized to send a proxy protocol header. Percona's handler code for the proxy protocol (minus the global variable handling) is in the vio/viosockets.c file [4] I looked at Percona's implementation. This file looks very similar to MariaDb's own viosockets.c file [5] so it may a potential good insertion point. Percona's software license seems to be permisive enough for reusing / adapting their code [6]. Please correct me if I'm wrong but it seems no one is currently working on this feature right now. I'd like to know if you would be interested in help side-porting Percona's proxy protocol code to MariaDB. I'll probably need some help or pointers on how to add a global variable, though. Best regards, --jose [1] Proxy protocol documentation https://blog.haproxy.com/haproxy/proxy-protocol/ [2] Proxy protocol specification www.haproxy.org/download/1.8/doc/proxy-protocol.txt [3] Mariadb feature request for proxy protocol https://jira.mariadb.org/browse/MDEV-11159?jql=project%20%3D%20MDEV%20AND%2… [3] Percona proxy protocol manual page https://www.percona.com/doc/percona-server/5.6/flexibility/proxy_protocol_s… [4] Percona proxy protocol handler code https://github.com/percona/percona-server/blob/1e2f003a5bd48763c27e37542d97… https://github.com/percona/percona-server/blob/1e2f003a5bd48763c27e37542d97… [5] mariadb potential insertion pode for proxy protocol code https://github.com/MariaDB/server/blob/bb2c1a52c61706dde8c525a8887f2d364c0d… [6] Percona software license https://www.percona.com/doc/percona-server/LATEST/copyright.html

3 3

Re: [Maria-developers] b4a223e3381: MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT
by Sergei Golubchik 07 Apr '17

07 Apr '17

Hi, Jacob! On Mar 28, jacob.mathew(a)mariadb.com wrote: > revision-id: b4a223e338152c8764c980aeae59fb927eea1b0b (mariadb-10.2.4-98-gb4a223e3381) > parent(s): 3f7455c03008b2428fa9b364b1add4c36834ff71 > author: Jacob Mathew > committer: Jacob Mathew > timestamp: 2017-03-28 17:41:56 -0700 > message: > > MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT > > Fixed handling of default values with cached temporal functions so that the > CREATE TABLE statement now succeeds. > Added clearing of cached function values to trigger function reevaluation > when updating default values and when updating virtual column values. > Fixed the error message. > Added quoting of date/time values in cases when this was omitted. > Added a test case. > Updated test result files that include date/time values that were > previously unquoted. > > diff --git a/mysql-test/r/func_default_between_temporal.result b/mysql-test/r/func_default_between_temporal.result > new file mode 100644 > index 00000000000..db9656f4d37 > --- /dev/null > +++ b/mysql-test/r/func_default_between_temporal.result > @@ -0,0 +1,15 @@ > +CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-01' AND '2012-12-12' ) ) ); > +SHOW CREATE TABLE t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01 00:00:00' and '2012-12-12 00:00:00')) > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +SET timestamp = UNIX_TIMESTAMP( '2004-04-04' ); > +INSERT INTO t1 VALUES( DEFAULT ); > +SET timestamp = DEFAULT; better use another constant here, don't rely on the current timestamp being between 2000 and 2012. I know, sounds crazy. But debian's reproducible builds (*) do everything with the system time seriously off. (*) https://wiki.debian.org/ReproducibleBuilds > +INSERT INTO t1 VALUES( DEFAULT ); > +SELECT * FROM t1; > +col > +1 > +0 > +DROP TABLE t1; > diff --git a/mysql-test/t/func_default_between_temporal.test b/mysql-test/t/func_default_between_temporal.test please, rename the test somehow or put it into default.test. "func_default" means it's about a DEFAULT() function, but your test is not. > new file mode 100644 > index 00000000000..6cbd5a342c6 > --- /dev/null > +++ b/mysql-test/t/func_default_between_temporal.test > @@ -0,0 +1,11 @@ Start the test with a header, like # # MDEV-10355 Weird error message upon CREATE TABLE with DEFAULT # > +CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-01' AND '2012-12-12' ) ) ); > +SHOW CREATE TABLE t1; > + > +SET timestamp = UNIX_TIMESTAMP( '2004-04-04' ); > +INSERT INTO t1 VALUES( DEFAULT ); > +SET timestamp = DEFAULT; > +INSERT INTO t1 VALUES( DEFAULT ); > + > +SELECT * FROM t1; > + > +DROP TABLE t1; > diff --git a/sql/item.cc b/sql/item.cc > index c34d27fa63b..ee3e43b60ad 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -616,8 +616,9 @@ void Item::print_value(String *str) > str->append("NULL"); > else > { > - switch (result_type()) { > + switch (cmp_type()) { > case STRING_RESULT: > + case TIME_RESULT: okay, that fixes quoting. good. > append_unescaped(str, ptr->ptr(), ptr->length()); > break; > case DECIMAL_RESULT: > @@ -9433,17 +9433,26 @@ void Item_cache::store(Item *item) > > void Item_cache::print(String *str, enum_query_type query_type) > { > - if (value_cached) > + if ( example && ( example->type() == FUNC_ITEM ) && // There is a cached function item > + ( query_type & QT_ITEM_IDENT_SKIP_TABLE_NAMES ) ) // Caller is show-create-table 1. better test for QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS 2. Why the test for FUNC_ITEM? I don't see any logical reason why skipping of "<cache>" should only apply to functions. and please watch the spacing around parentheses, try to use the same coding style as the rest of the code > { > - print_value(str); > - return; > + // Instead of "cache" or the cached value, print the function name > + example->print( str, query_type ); > } > - str->append(STRING_WITH_LEN("<cache>(")); > - if (example) > - example->print(str, query_type); > else > - Item::print(str, query_type); > - str->append(')'); > + { > + if (value_cached) Indentation. please, try to use the same coding style as the rest of the code > + { > + print_value(str); > + return; > + } It's not very logical that the value is not printed if one doesn't want "<cache>". I think this method could be like this: ================================== void Item_cache::print(String *str, enum_query_type query_type) { if (value_cached && !(query_type & QT_NO_DATA_EXPANSION)) { print_value(str); return; } if (!(query_type & QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS)) str->append(STRING_WITH_LEN("<cache>(")); if (example) example->print(str, query_type); else Item::print(str, query_type); if (!(query_type & QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS)) str->append(')'); } ================================== That is, print the value unless QT_NO_DATA_EXPANSION is specified. Print "<cache>" unless QT_ITEM_CACHE_WRAPPER_SKIP_DETAILS is specified. And Virtual_column_info::print() should also specify QT_NO_DATA_EXPANSION. > + str->append(STRING_WITH_LEN("<cache>(")); > + if (example) > + example->print(str, query_type); > + else > + Item::print(str, query_type); > + str->append(')'); > + } > } > > /** > diff --git a/sql/item.h b/sql/item.h > index 67640ce5f4d..222f8580cb4 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -1653,6 +1653,7 @@ class Item: public Value_source, > { > return mark_unsupported_function(full_name(), arg, VCOL_IMPOSSIBLE); > } > + virtual bool clear_cached_function_value(void *arg) { return 0; } > virtual bool check_field_expression_processor(void *arg) { return 0; } > virtual bool check_func_default_processor(void *arg) { return 0; } > /* > @@ -5450,8 +5451,21 @@ class Item_cache: public Item_basic_constant, > } > bool check_vcol_func_processor(void *arg) > { > + if ( example ) > + { > + return example->check_vcol_func_processor( arg ); whitespace around parentheses and indentation > + } > return mark_unsupported_function("cache", arg, VCOL_IMPOSSIBLE); > } > + bool clear_cached_function_value( void *arg ) > + { > + if ( example && example->type() == Item::FUNC_ITEM ) > + { > + // Clear the cached function value to trigger reevaluation > + clear(); > + } > + return false; > + } Why did you need a dedicated clear_cached_function_value() processor? There's Item::cleanup() that should be called from fix_session_vcol_expr(). Oh, okay, I see two issues here. First, item->cleanup() does not recurse into function arguments. That's my bug, I should've used something like - vcol->expr->cleanup(); + vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0); It has to be fixed, it's a genuine bug. A test case could be create table t1 (n varchar(100), cu varchar(100) default current_user(), uc varchar(100) default concat(current_user())); create definer=foo@localhost view v1 as select * from t1; insert v1 (n) values ('v1'); select * from t1; This is a slightly modified test from default_session.test file. Second, it's incorrect for Item_cache to use simply example->check_vcol_func_processor(arg); because caching requires re-fixing, even if the underlying example item doesn't. So, this should be something like bool check_vcol_func_processor(void *arg) { if ( example ) { Item::vcol_func_processor_result *res= (Item::vcol_func_processor_result*)arg; example->check_vcol_func_processor(arg); if (res->errors & VCOL_NOT_STRICTLY_DETERMINISTIC) res->errors|= VCOL_SESSION_FUNC; return false; } return mark_unsupported_function("cache", arg, VCOL_IMPOSSIBLE); } this sets VCOL_SESSION_FUNC flag, which will require item to be re-fixed for every statement. But doesn't set it for true constants. With these two fixes Item_cache::cleanup() { clear(); } should works just fine. > /** > Check if saved item has a non-NULL value. > Will cache value of saved item if not already done. > diff --git a/sql/table.cc b/sql/table.cc > index 3a08d1e49ea..9ae035e41aa 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2802,9 +2802,9 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, > > int error= func_expr->walk(&Item::check_vcol_func_processor, 0, &res); > if (error || (res.errors & VCOL_IMPOSSIBLE)) > - { // this can only happen if the frm was corrupted > + { this comment is still true, don't delete it. and there's no need to pass the type inside anymore. > my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0), res.name, > - "???", "?????"); > + vcol_type_name((enum_vcol_info_type) vcol_type), vcol->name.str); > DBUG_RETURN(1); > } > vcol->flags= res.errors; > @@ -7374,6 +7374,10 @@ int TABLE::update_virtual_fields(handler *h, enum_vcol_update_mode update_mode) > > if (update) > { > + // Clear any cached function values in the virtual field expression > + // to trigger their reevaluation > + vcol_info->expr->walk( &Item::clear_cached_function_value, 0, 0 ); // Always returns 0 There's no need to do that for every update_virtual_fields(), that is, no need to do it for every row, only needs to be done once per statement. And fix_session_vcol_expr() will do it for you if you implement Item_cache::cleanup() and set VCOL_SESSION_FUNC and fix fix_session_vcol_expr() to cleanup recursively. > + > int field_error __attribute__((unused)) = 0; > /* Compute the actual value of the virtual fields */ > if (vcol_info->expr->save_in_field(vf, 0)) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 4

Re: [Maria-developers] b7aa15458db: MDEV-11117 CHECK constraint fails on intermediate step of ALTER
by Sergei Golubchik 06 Apr '17

06 Apr '17

Hi, Jacob! On Apr 03, jacob.mathew(a)mariadb.com wrote: > revision-id: b7aa15458db95149af4f473a90009844a16e9db8 (mariadb-10.2.4-103-gb7aa15458db) > parent(s): 3c422e60bbee79bb636e65910c26ac193de70a84 > author: Jacob Mathew > committer: Jacob Mathew > timestamp: 2017-04-03 17:54:32 -0700 > message: > > MDEV-11117 CHECK constraint fails on intermediate step of ALTER > > Fixed the bug by failing the statement with an error message that explains > that an auto-increment column may not be used in an expression for a > check constraint, a default value or a generated column. > Added a test case. > > new file mode 100644 > index 00000000000..e192fd16677 > --- /dev/null > +++ b/mysql-test/t/check_constraint_autoinc.test > @@ -0,0 +1,7 @@ > +# > +# An auto-increment column may not be used in an expression for > +# a check constraint: MDEV-11117 > +# > +--echo # AUTO_INCREMENT IN VCOL > +-- error ER_AUTOINC_IN_VIRTUAL_COLUMN_FUNCTION > +create or replace table t1( c1 int auto_increment primary key, check( c1 > 0 or c1 is null ) ); you could've added this 2-line test to the existing file check_constraint.test No strong arguments either way, though. Pro: mysqltest is executed for every test file, it's faster to have less test files. Don't know if the difference is big, though. Contra: it's easier to see what has failed with one test per file. It doesn't matter now, but might matter in the future, when we'll have more automatic failure analysys. > diff --git a/sql/field.h b/sql/field.h > index 50dcb397616..4bd43551648 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -591,7 +591,8 @@ static inline const char *vcol_type_name(enum_vcol_info_type type) > #define VCOL_NON_DETERMINISTIC 2 > #define VCOL_SESSION_FUNC 4 /* uses session data, e.g. USER or DAYNAME */ > #define VCOL_TIME_FUNC 8 > -#define VCOL_IMPOSSIBLE 16 > +#define VCOL_AUTO_INC 16 > +#define VCOL_IMPOSSIBLE 32 > > #define VCOL_NOT_STRICTLY_DETERMINISTIC \ > (VCOL_NON_DETERMINISTIC | VCOL_TIME_FUNC | VCOL_SESSION_FUNC) > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index 930cd2ceaa2..a78e97c2577 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7142,6 +7142,8 @@ ER_NO_EIS_FOR_FIELD > ER_WARN_AGGFUNC_DEPENDENCE > eng "Aggregate function '%-.192s)' of SELECT #%d belongs to SELECT #%d" > ukr "Агрегатна функція '%-.192s)' з SELECTу #%d належить до SELECTу #%d" > +ER_AUTOINC_IN_VIRTUAL_COLUMN_FUNCTION > + eng "Auto-increment column %`s may not be used in an expression for a check constraint, a default value or a generated column." First, please add error messages at the very end of the file. Second, there's an existing error ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED eng "Function or expression '%s' cannot be used in the %s clause of %`s" which is generally used like my_error(ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0), "NOW()", "CHECK", field->name); or, for subqueries my_error(ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0), "SELECT ...", "DEFAULT", field->name); It'd be best if you could use it. If not, at least rephrase your error message to use ... in the %s clause of %`s pattern, instead of enumerating all possible places where autoincrement is not allowed. > diff --git a/sql/table.cc b/sql/table.cc > index 6b15c06cb91..d1772933190 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2811,6 +2811,19 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, > "???", "?????"); > DBUG_RETURN(1); > } > + else if (res.errors & VCOL_AUTO_INC) > + { > + /* > + An auto_increment field may not be used in an expression for > + a check constraint, a default value or a generated column > + > + Note that this error condition is not detected during parsing > + of the statement because the field item does not have a field > + pointer at that time > + */ > + my_error(ER_AUTOINC_IN_VIRTUAL_COLUMN_FUNCTION, MYF(0), res.name); > + DBUG_RETURN(1); > + } > vcol->flags= res.errors; There is already a check for auto-increment in generated columns. I've added it in commit a418c9920047d5222a0d065343347312127b780f (see the change in item.cc). But the error message is somewhat confusing. And very confusing when applied to CHECK and DEFAULT. Yours is better, so please remove my check, no need to do it twice. Don't forget to re-run tests after that (at least vcol and gcol suites) as some error messages could change. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] mariadb-connector-c on mingw-w64 with pthreads
by Jeroen Ooms 06 Apr '17

06 Apr '17

For the language bindings for the R programming language (RMySQL) I am building mariadb-connector-c-2.3.2 on Windows using the mingw-w64 compiler. Our mingw-w64 compilers have been configured with pthreads support (same as msys2): - i686-493-posix-dwarf-rt_v3-s for win32 - x86_64-493-posix-seh-rt_v3-s for win64 Building libmariadbclient succeeds, however when I try to link the static library into the application I get these errors: C:/Rtools/mingw_64/lib/gcc/x86_64-w64-mingw32/lib/libpthread.a(libwinpthread_la-cond.o):cond.c:(.text+0x300): multiple definition of `pthread_cond_init' libmariadbclient.a(my_pthread.c.obj):my_pthread.c:(.text+0x0): first defined here Same error for 'pthread_cond_destroy', 'pthread_cond_wait', and 'pthread_cond_timedwait'. From the source code of my_pthread.c it seems that mariadb-connector-c is indeed redefining these functions on _WIN32. I guess this assumes MSVC which does not have native pthreads? Some questions: 1. What would be the correct way to configure/build mariadb-connector-c on mingw-w64 with native pthread support? 2. Is there a cmake option to disable threading alltogether? Thank you

2 1