- developers - lists.mariadb.org

[Maria-developers] MDEV-8831: review request
by Nirbhay Choubey 01 Oct '15

01 Oct '15

Hi Jan, Could you please review the attached patch? Thanks, Nirbhay

1 0

[Maria-developers] Groups in PAM plugin.
by Alexey Botchkov 28 Sep '15

28 Sep '15

Hi. I came up with a small program that checks if an Unix user belongs to a group 'wheel'. That can be attached to the 'plugin/auth_pam/mapper/pam_user_map.c' in no time if that's what we want. http://myoffice.izhnet.ru/~af/gr.cc An it must be LDAP-compliant AFAIK. Best regards. HF

1 0

[Maria-developers] New Programmer(Maria-DB)
by indiavaibhav vaibhavindia 28 Sep '15

28 Sep '15

Hello everyone, My name is Vaibhav Choudhary.I am in my second year undergraduation and am new to open source. I would like to contribute to your project.Can anyone please guide me as to how should i work on a patch after i have identified the problem on JIRA? I have a good knowledge of C++ and am studying MySQL.But I am unable to figure out as to how should I go about writing the code for the issues on JIRA. Thanks Vaibhav

2 4

Re: [Maria-developers] [Commits] 1d92131: MDEV-4829 BEFORE INSERT triggers dont issue 1406 error.
by Sergei Golubchik 27 Sep '15

27 Sep '15

Hi, Holyfoot! On Sep 24, holyfoot(a)askmonty.org wrote: > revision-id: 1d92131613770b505fa462b33c1091e00d9475be (mariadb-10.1.7-61-g1d92131) > parent(s): e5418942609833edb681d16c4e2705f8c338bfee > committer: Alexey Botchkov > timestamp: 2015-09-24 14:31:06 +0500 > message: > > MDEV-4829 BEFORE INSERT triggers dont issue 1406 error. > Fixed as it's done in MySQL 5.7. > The Strict_error_handler introduced to intercept such states and is activated > in the sp_head::execute_trigger() Hmm... This is, basically, a new implementation of the strict mode. But enabled only for triggers, which is kind of weird. And potentially very fragile and inconsistent, as it's a strict mode we're talking about, it's confusing and not very consistent on itself. Having two different implementations of it doesn't help to make it clear or consistent. If you're introducing a new implementation - use it consistently everywhere. If that won't change existing intentional behavior. And please talk to Bar about his recent experiences in this area. There were also few comments about the code, see below. > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index a117963..ff6d2b4 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -54,6 +54,35 @@ > > #include <my_user.h> > > +/** > + A simple RAII wrapper around Strict_error_handler. > +*/ > +class Strict_error_handler_wrapper > +{ > + THD *m_thd; > + Strict_error_handler m_strict_handler; > + bool m_active; > + > +public: > + Strict_error_handler_wrapper(THD *thd, sp_head *sp_head) > + : m_thd(thd), m_active(false) > + { > + if (sp_head->m_sql_mode & (MODE_STRICT_ALL_TABLES | > + MODE_STRICT_TRANS_TABLES)) > + { > + m_thd->push_internal_handler(&m_strict_handler); > + m_active= true; > + } > + } > + > + ~Strict_error_handler_wrapper() > + { > + if (m_active) > + m_thd->pop_internal_handler(); > + } > +}; Oh, really? A wrapper per handler? Make it a template, please, so that it could be used for any error handler. Like this: Error_handler_wraper<Strict_error_handler> strict_error_handler(thd, this); Also, it would be good if you do a second commit using this wrapper in other places, where appropriate. > @@ -1576,6 +1605,9 @@ sp_head::execute_trigger(THD *thd, > DBUG_ENTER("sp_head::execute_trigger"); > DBUG_PRINT("info", ("trigger %s", m_name.str)); > > + // Push Strict_error_handler if the SP was created in STRICT mode. > + Strict_error_handler_wrapper strict_wrapper(thd, this); > + > #ifndef NO_EMBEDDED_ACCESS_CHECKS > Security_context *save_ctx= NULL; > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 9162969..ca0699c 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -96,6 +96,74 @@ bool No_such_table_error_handler::safely_trapped_errors() > > > /** > + Implementation of STRICT mode. > + Upgrades a set of given conditions from warning to error. > +*/ > +bool Strict_error_handler::handle_condition_ext(THD *thd, > + uint sql_errno, > + const char *sqlstate, > + Sql_condition::enum_warning_level *level, > + const char *msg, > + Sql_condition **cond_hdl) > +{ > + /* STRICT MODE should affect only the below statements */ Really? > + switch (thd->lex->sql_command) > + { > + case SQLCOM_CREATE_TABLE: > + case SQLCOM_DROP_INDEX: > + case SQLCOM_INSERT: > + case SQLCOM_REPLACE: > + case SQLCOM_REPLACE_SELECT: > + case SQLCOM_INSERT_SELECT: > + case SQLCOM_UPDATE: > + case SQLCOM_UPDATE_MULTI: > + case SQLCOM_DELETE: > + case SQLCOM_DELETE_MULTI: > + case SQLCOM_ALTER_TABLE: > + case SQLCOM_LOAD: > + case SQLCOM_CALL: > + case SQLCOM_END: > + case SQLCOM_SET_OPTION: > + case SQLCOM_SELECT: > + break; > + default: > + return false; > + } > + > + switch (sql_errno) > + { > + case ER_TRUNCATED_WRONG_VALUE: > + case ER_WRONG_VALUE_FOR_TYPE: > + case ER_WARN_DATA_OUT_OF_RANGE: > + case ER_DIVISION_BY_ZERO: > + case ER_TRUNCATED_WRONG_VALUE_FOR_FIELD: > + case WARN_DATA_TRUNCATED: > + case ER_DATA_TOO_LONG: > + case ER_BAD_NULL_ERROR: > + case ER_NO_DEFAULT_FOR_FIELD: > + case ER_TOO_LONG_KEY: > + case ER_WRONG_ARGUMENTS: > + case ER_NO_DEFAULT_FOR_VIEW_FIELD: > + case ER_WARN_NULL_TO_NOTNULL: > + case ER_CUT_VALUE_GROUP_CONCAT: > + case ER_DATETIME_FUNCTION_OVERFLOW: > + case ER_WARN_TOO_FEW_RECORDS: > + if ((*level == Sql_condition::WARN_LEVEL_WARN) && > + thd->transaction.stmt.m_unsafe_rollback_flags == 0 || > + (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES)) > + { > + (*level)= Sql_condition::WARN_LEVEL_ERROR; > + thd->killed= KILL_BAD_DATA; > + } > + break; > + default: > + break; > + } > + return false; > +} > + > + > +/** > This internal handler is used to trap ER_NO_SUCH_TABLE and > ER_WRONG_MRG_TABLE errors during CHECK/REPAIR TABLE for MERGE > tables. > diff --git a/sql/sql_base.h b/sql/sql_base.h > index 1c0029d..c5a76c5 100644 > --- a/sql/sql_base.h > +++ b/sql/sql_base.h > @@ -676,5 +676,30 @@ class No_such_table_error_handler : public Internal_error_handler > int m_unhandled_errors; > }; > > +/** > + This internal handler implements upgrade from SL_WARNING to SL_ERROR "SL_ERROR"? "SL_WARNING"? > + for the error codes affected by STRICT mode. Currently STRICT mode does > + not affect SELECT statements. > +*/ > + > +class Strict_error_handler : public Internal_error_handler > +{ > +protected: > + bool handle_condition(THD *thd, > + uint sql_errno, > + const char* sqlstate, > + Sql_condition::enum_warning_level level, > + const char* msg, > + Sql_condition ** cond_hdl) > + { DBUG_ASSERT(FALSE); return 0; } > +public: > + bool handle_condition_ext(THD *thd, > + uint sql_errno, > + const char* sqlstate, > + Sql_condition::enum_warning_level *level, > + const char* msg, > + Sql_condition ** cond_hdl); 1. why not to change handle_condition() ? 2. strict mode may change the sql_errno, handle_condition_ext doesn't support it. > +}; > + > > #endif /* SQL_BASE_INCLUDED */ Regards, Sergei

1 2

Re: [Maria-developers] [Commits] d3f10e3: MDEV-8431 Feedback plugin needs an option for http proxy.
by Sergei Golubchik 26 Sep '15

26 Sep '15

Hi, Holyfoot! On Sep 21, holyfoot(a)askmonty.org wrote: > revision-id: d3f10e30ec39bccf1172c7b2eb159f40e412bdfd (mariadb-10.0.21-14-gd3f10e3) > parent(s): abd31ca2b6cde024772fcac0f4953219d76c6344 > committer: Alexey Botchkov > timestamp: 2015-09-21 14:06:15 +0500 > message: > > MDEV-8431 Feedback plugin needs an option for http proxy. > 'feedback_proxy_server' system variable added to specify the > proxy server. Not a dynamic one. How did you test this feature? > diff --git a/plugin/feedback/feedback.cc b/plugin/feedback/feedback.cc > index f644bd5..f76c52a 100644 > --- a/plugin/feedback/feedback.cc > +++ b/plugin/feedback/feedback.cc > @@ -250,6 +251,9 @@ static int init(void *p) > > prepare_linux_info(); > > + proxy_url= proxy_server ? > + Url::create(proxy_server, strlen(proxy_server)) : 0; Some diagnostics would be nice here. At least "feedback: invalid proxy url: '%.*s'". See how ivalid feedback urls are reported. > + > url_count= 0; > if (*url) > { > @@ -347,6 +354,9 @@ static MYSQL_SYSVAR_ULONG(send_timeout, send_timeout, PLUGIN_VAR_RQCMDARG, > static MYSQL_SYSVAR_ULONG(send_retry_wait, send_retry_wait, PLUGIN_VAR_RQCMDARG, > "Wait this many seconds before retrying a failed send.", > NULL, NULL, 60, 1, 60*60*24, 1); > +static MYSQL_SYSVAR_STR(proxy_server, proxy_server, > + PLUGIN_VAR_READONLY | PLUGIN_VAR_RQCMDARG, > + "Proxy server address.", NULL, NULL,0); Mention that it must be an url. "Proxy server URL" or "Proxy server address, in the correct URI format", or something. > static struct st_mysql_sys_var* settings[] = { > MYSQL_SYSVAR(server_uid), > diff --git a/plugin/feedback/url_http.cc b/plugin/feedback/url_http.cc > index f214f7a..eade67c 100644 > --- a/plugin/feedback/url_http.cc > +++ b/plugin/feedback/url_http.cc > @@ -148,9 +148,12 @@ int Url_http::send(const char* data, size_t data_length) > my_socket fd= INVALID_SOCKET; > char buf[1024]; > uint len= 0; > + Url_http *proxy_loc= (Url_http *) proxy_url; > > addrinfo *addrs, *addr, filter= {0, AF_UNSPEC, SOCK_STREAM, 6, 0, 0, 0, 0}; > - int res= getaddrinfo(host.str, port.str, &filter, &addrs); > + int res= proxy_url ? > + getaddrinfo(proxy_loc->host.str, proxy_loc->port.str, &filter, &addrs) : > + getaddrinfo(host.str, port.str, &filter, &addrs); > First, you don't know whether proxy_url is Url_http. It might be easier to have an option --feedback-http-proxy=host:port (not the url). It also solves the potential future confusion if we'll have more than one transport. Second, I don't think proxies work this way at all, for a http proxy, you need to specify the host in th GET request, like POST http://foo.bar/bla/bla So, I think, a better implementation would be to take proxy into account not in Url_http::send(), but already in http_create(). And create Url_http objects for feedback url with proxy's host and port, and with the appropriately adjusted path. Regards, Sergei

1 0

Re: [Maria-developers] [Commits] efcbbed: MDEV-8682 - CSV engine does not properly process "", in quotes
by Sergei Golubchik 26 Sep '15

26 Sep '15

Hi, Sergey! On Sep 22, Sergey Vojtovich wrote: > revision-id: efcbbed977dfcd9ee0ed035cbb0df4404483b0c8 (mariadb-10.1.7-53-gefcbbed) > parent(s): 7cbecadf9b59d06f4d9a9e776f99418b273c2ebc > committer: Sergey Vojtovich > timestamp: 2015-09-22 16:52:30 +0400 > message: > > MDEV-8682 - CSV engine does not properly process "", in quotes > > Added per-table boolean IETF_QUOTES variable to CSV storage engine. It allows to > enable IETF-compatible parsing of embedded quote and comma characters. Disabled > by default. > > This patch is based on Percona revision: > https://github.com/percona/percona-server/commit/b32fbf02766f8db00f9ff4e6dc… > > Note that original patch adds server variable, while this patch adds per-table > variable. I agree that a per-table option is safer. Ok to push. Regards, Sergei

1 0

Re: [Maria-developers] [Commits] 5f461a9: MDEV-7640: CHANGE MASTER TO doesn't work with prepared statements
by Kristian Nielsen 21 Sep '15

21 Sep '15

nirbhay(a)mariadb.com (Nirbhay Choubey) writes: > revision-id: 5f461a94a5aa2e1a20fa559ca612d96b53201e4e > parent(s): 29ac245dd04c5416d57a90b0ab3c4d08cbc4d723 > committer: Nirbhay Choubey > branch nick: 5.5 > timestamp: 2015-09-16 18:10:06 -0400 > message: > > MDEV-7640: CHANGE MASTER TO doesn't work with prepared statements > > When CHANGE MASTER was executed as a PS, its attributes were wrongly > getting reset toward the end of PREPARE. As a result, the subsequent > executions had no effect. Fixed by making sure that the CHANGE MASTER > attributes are preserved during the lifetime of the PS. Hm, this MDEV seems now assigned to me, but I did not see any mail from you about it? Did you expect a review? But in any case, this code around prepare/execute is not something I am familiar with. I seem to recall that Sanja worked on several issues related to incorrect prepare/execute, maybe he can have some ideas or suggest a reviewer. Apparently this is something that used to work but started to break sometimes in 5.5. Do you know how the problem was introduced? - Kristian (sadder by the hour over devs not using the mailing list).

2 1

[Maria-developers] Super Read Only in MariaDB (10.1) ?
by Jean-François Gagné 19 Sep '15

19 Sep '15

Hi, a super_read_only option is making its way in MySQL 5.7: * http://www.tocker.ca/2015/07/09/mysql-5-7-8-now-featuring-super_read_only-a… * http://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_… Are there plans to have a similar option in MariaDB (it would be great to have it in MariaDB 10.1) ? Many thanks, -- Jean-François Gagné System Engineer Booking.com BV Rembrandt Square Office Herengracht 597, 1017 CE Amsterdam

2 1

[Maria-developers] New programmer
by Acha Bill 18 Sep '15

18 Sep '15

Hello guys, I'm Acha Bill. Final year undergraduate in Computer Engineering. I would like to be an active contributor to your ongoing projects. I have good knowledge of C/C++ and MySQL. As this is my first approach to open source, any guidance will be much appreciated. Bill.

3 3

Re: [Maria-developers] Please review MDEV-8468 MDEV-8466
by Sergei Golubchik 17 Sep '15

17 Sep '15

Hi, Alexander! On Jul 21, Alexander Barkov wrote: > > I have one question about "HANDLER FOR SQLWARNING". > It does not catch messages of level "Note". > Looks like a bug. Please confirm, I'll file a report if so. Hmm, I don't know. According to the standard, SQLWARNING should match sqlstates in the category W, while SQLEXCEPTION should be for sqlstates in the category X. But we don't do that, 22007 is an exception (category X), but we treat it as a warning. So, the standard doesn't help to answer your question. We seem to use the logic where a level "error" means an SQL exception and a level "warning" means an SQL warning. Using this logic, "notes" are neither exceptions nor warnings, and neither SQLEXCEPTION nor SQLWARNING should match them. I'd say it's not a bug. The patch itself looks great, these changes in behavior are very welcomed. Just one small comment regarding THD::check_edom_and_truncation(), see below. > diff --git a/sql/sql_class.h b/sql/sql_class.h > index a8d8444..c19174b 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3270,6 +3270,53 @@ class THD :public Statement, > (!transaction.stmt.modified_non_trans_table || > (variables.sql_mode & MODE_STRICT_ALL_TABLES))); > } > + /** > + Check string-to-number conversion and produce a warning if > + - could not convert any digits (EDOM-alike error) > + - found garbage at the end of the string > + See also Field_num::check_edom_and_truncation() for a similar function. > + > + @param thd - the thread > + @param type - name of the data type (e.g. "INTEGER", "DECIMAL", "DOUBLE") > + @param edom - of a EDOM-alike error happened > + @param cs - character set of the original string > + @param str - the original string > + @param end_of_num - the position where the string-to-number routine stopped. > + @param end - the end of the string > + > + Unlike Field_num::check_edom_and_truncation(), this function does not > + distinguish between EDOM and truncation and reports the same warning for > + both cases. Perhaps we should eventually print different warnings, to make > + the explicit CAST work closer to the implicit cast in Field_xxx::store(). Why haven't you done it now? It causes too many changes in tests? > + */ > + void check_edom_and_truncation(const char *type, bool edom, > + CHARSET_INFO *cs, > + const char *str, > + const char *end_of_num, > + const char *end) Move this out of THD please. You don't need to call current_thd every time for it, but only when a warning should be issued. > + { > + DBUG_ASSERT(str <= end_of_num); > + DBUG_ASSERT(end_of_num <= end); > + if (edom || > + (end_of_num < end && !check_if_only_end_space(cs, end_of_num, end))) > + { > + /* > + We can use err.ptr() here as ErrConvString is guranteed to put an > + end \0 here. > + */ > + push_warning_printf(this, Sql_condition::WARN_LEVEL_WARN, > + ER_TRUNCATED_WRONG_VALUE, > + ER(ER_TRUNCATED_WRONG_VALUE), type, > + ErrConvString(str, end - str, cs).ptr()); > + } > + else if (end_of_num < end) > + { > + push_warning_printf(this, Sql_condition::WARN_LEVEL_NOTE, > + ER_TRUNCATED_WRONG_VALUE, > + ER(ER_TRUNCATED_WRONG_VALUE), type, > + ErrConvString(str, end - str, cs).ptr()); > + } > + } Regards, Sergei

2 1