- developers - lists.mariadb.org

[Maria-developers] Refactoring in LEX_STRING and LEX_CSTRING
by Alexander Barkov 21 Apr '17

21 Apr '17

Hello all, Monty is now doing refactoring: 1. Replacing a lot of LEX_STRING to LEX_CSTRING. 2. Fixing functions and methods that have arguments of types: a. pointer "const LEX_STRING *name" and b. reference "const LEX_STRING &name". to the same style. Monty chose passing by pointer (2a). I think that: - #1 is good. - #2 is good, in the sense of changing to the same style. But I think that the choice of "passing by pointer" in #2 is bad. Passing by reference for LEX_STRING/LEX_CSTRING related things would be better. With the "passing by pointer" approach whenever one needs to pass a LEX_STRING to a function accepting LEX_CSTRING, one will have to do: void func1(const LEX_STRING *str) { LEX_CSTRING tmp= {str->str, str->length}; func2(&tmp); } I'd propose two things instead: 1. Add a class: class Lex_cstring: public LEX_CSTRING { public: Lex_cstring() {} /*Uninitialized */ Lex_cstring(const char *str, size_t length) { LEX_STRING::str= str; LEX_STRING::length= length; } Lex_cstring(const LEX_STRING *str) { LEX_STRING::str= str->str; LEX_STRING::length= str->length; } }; 2. Instead of "const LEX_CSTRING *str" we use "const Lex_cstring &str" all around C++ code. So the above example with passing a LEX_STRING to something that expect a LEX_CSTRING would be simplified to void func1(const LEX_STRING *str) { func2(str); } 3. In some classes now we have duplicate methods and constructors like this: class XXX { public: XXX(const char *str, size_t length) { ... } XXX(const LEX_STRING *str) { ... } void method(const char *str, size_t length) {...} void method(const LEX_STRING *str) {...} }; to make it possible to call like this: new XXX(str, length); new XXX(lex_str_ptr) new XXX(&lex_str) method(str, length); method(lex_str_ptr); method(&lex_str); We change this to "passing by reference" and remove duplicates: class XXX { public: XXX(const Lex_cstring &str) { ... } void method(const Lex_cstring &str) {...} }; After this change we can call like this: new XXX(Lex_cstring(str, length)); new XXX(*lex_str_ptr) new XXX(lex_str); method(Lex_cstring(str, length)); method(*lex_str_ptr); method(lex_str); Notice, not needs for separate methods accepting two arguments "const char *str, size_t length".

4 5

[Maria-developers] How to report progress for InnoDB background tasks (esp. at startup/shutdown)?
by Marko Mäkelä 18 Apr '17

18 Apr '17

Hi all, MariaDB Server features a nice-looking interface for reporting progress on long-running SQL operations: https://mariadb.com/kb/en/mariadb/progress-reporting/ MySQL 5.7 implemented something similar for reporting progress for ALTER TABLE…ALGORITHM=INPLACE. I should reimplement that for MariaDB using the existing MariaDB infrastructure. I wonder if the same interface could be used for tracking progress on periodic background tasks that might not be directly related to the execution of any SQL statements. InnoDB has a number of background tasks, such as: * purging unneeded history (removing undo log records and delete-marked index records) * rolling back recovered transactions (mainly, if the server was killed) * index defragmentation, encryption key rotation * redo log checkpoints (flush all dirty pages up to the checkpoint LSN; most interesting at shutdown) The above would need to be monitored also while the server is shutting down. I understand that at startup, we would probably need a different progress reporting interfaces for the very first InnoDB task of redo log apply. Everything else can be done in the background while the server is accepting connections. Related to this, I wonder if it is possible or feasible to change the SQL layer logic so that at shutdown, it will not issue ROLLBACK (as part of KILL QUERY) to the currently active connections, but let the subsequent startup do the rollback in the background. This should allow faster shutdowns and restarts. Best regards, Marko -- DON’T MISS M|17April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

2 2

[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