- developers - lists.mariadb.org

Re: [Maria-developers] 68ebe0e2f5e: MDEV-29446 Change SHOW CREATE TABLE to display default collation
by Sergei Golubchik 09 Sep '22

09 Sep '22

Hi, Alexander, On Sep 09, Alexander Barkov wrote: > revision-id: 68ebe0e2f5e (mariadb-10.3.36-25-g68ebe0e2f5e) > parent(s): f501f815bc7 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-09-07 08:10:58 +0400 > message: > > MDEV-29446 Change SHOW CREATE TABLE to display default collation > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index a019c3c9dbd..cdf0bc01b6a 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -2253,18 +2247,6 @@ int show_create_table(THD *thd, TABLE_LIST *table_list, String *packet, > { > packet->append(STRING_WITH_LEN(" CHARACTER SET ")); > packet->append(field->charset()->csname); > - } > - /* > - For string types dump collation name only if > - collation is not primary for the given charset > - > - For generated fields don't print the COLLATE clause if > - the collation matches the expression's collation. > - */ > - if (!(field->charset()->state & MY_CS_PRIMARY) && > - (!field->vcol_info || > - field->charset() != field->vcol_info->expr->collation.collation)) why, I think, this second condition could still apply, no need to remove it. > - { > packet->append(STRING_WITH_LEN(" COLLATE ")); > packet->append(field->charset()->name); > } Note that in 10.10 you need to fix json_table code inside a big if() condition: ... (!(m_explicit_cs->state & MY_CS_PRIMARY) && (str->append(STRING_WITH_LEN(" COLLATE ")) || str->append(&m_explicit_cs->coll_name))))) || ... Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] MDEV-25080 Allow pushdown of queries involving UNIONs in outer select to foreign engines
by Oleg Smirnov 09 Sep '22

09 Sep '22

Hello! The implementation for MDEV-25080 is ready and pushed to the *preview-10.11-mdev-25080-union-pushdown *branch. The FederatedX storage engine is adjusted accordingly. The example of how it works can be found at *mysql-test/federated/federatedx_create_handlers.test:* --echo # Pushdown of the whole UNION SELECT * from federated.t1 UNION SELECT * from federated.t2; EXPLAIN SELECT * from federated.t1 UNION SELECT * from federated.t2; --echo # Pushdown of a part of the UNION SELECT * from federated.t1 UNION SELECT * from t3; EXPLAIN SELECT * from federated.t1 UNION SELECT * from t3; -- Best regards, ------------------------- *Oleg Smirnov* Sr. Software Engineer MariaDB Server Team

1 0

[Maria-developers] Cosmetic input re: MDEV-25080 Allow pushdown of UNIONs to foreign engines
by Sergey Petrunia 06 Sep '22

06 Sep '22

Hi Oleg, Please find below coding style/etc input re the patch for MDEV-25080. (See the MDEV for other comments, more are coming). > diff --git a/sql/select_handler.cc b/sql/select_handler.cc > index 795ed8eb641..99bd2c0cf7f 100644 > --- a/sql/select_handler.cc > +++ b/sql/select_handler.cc > @@ -114,10 +125,7 @@ bool select_handler::send_data() > bool select_handler::send_eof() > { > DBUG_ENTER("select_handler::send_eof"); > - > - if (select->join->result->send_eof()) > - DBUG_RETURN(true); > - DBUG_RETURN(false); > + DBUG_RETURN(result->send_eof()); > } What is this change for? If it's just the code cleanup and we HAVE to do it (I don't see an urgent reason), please do it in a separate commit. Reasons against putting this change into this commit: When one sees this change as part of a meaningful patch, they start to examine the change to see what difference it makes. After one minute, one realizes the old and new code are the same. The time and attention have already been wasted, though. > diff --git a/sql/sql_explain.cc b/sql/sql_explain.cc > index 7c3918bfd20..6726e1b28f1 100644 > --- a/sql/sql_explain.cc > +++ b/sql/sql_explain.cc > @@ -555,6 +575,12 @@ int Explain_union::print_explain(Explain_query *query, > } > > if (!using_tmp) > + /* > + The union operation may not employ a temporary table, for example, > + for UNION ALL, in that case the results of the query are sent directly > + to the output. So there is no actual UNION operation and we don't need > + to print the line in the EXPLAIN output. > + */ > return 0; Please add { } braces as the if-part takes more than one line now. > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 9382625a747..437acbb749c 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -5549,9 +5552,13 @@ void st_select_lex::set_explain_type(bool on_the_fly) > type= "MATERIALIZED UNION"; > else > { > - type= is_uncacheable ? "UNCACHEABLE UNION": "UNION"; > + type= is_uncacheable ? "UNCACHEABLE UNION" > + : "UNION"; > Again: please don't just re-format the code > @@ -5561,15 +5568,14 @@ void st_select_lex::set_explain_type(bool on_the_fly) > { > bool uses_cte= false; > for (JOIN_TAB *tab= first_linear_tab(join, WITHOUT_BUSH_ROOTS, > - WITH_CONST_TABLES); > - tab; > - tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS)) > + WITH_CONST_TABLES); > + tab; tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS)) > { and here. > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 45fed46e375..2310583074b 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1843,6 +1843,8 @@ int JOIN::optimize() > join_optimization_state init_state= optimization_state; > if (select_lex->pushdown_select) > { > + if (optimization_state == JOIN::OPTIMIZATION_DONE) > + return 0; Please add and assert that after this we have optimization_state==JOIN::NOT_OPTIMIZED. > // Do same as JOIN::optimize_inner does: > fields= &select_lex->item_list; > > @@ -4885,7 +4887,6 @@ void JOIN::cleanup_item_list(List<Item> &items) const > DBUG_VOID_RETURN; > } > > - Please put this back, the coding style says to use two empty lines between the functions. > /** > @brief > Look for provision of the select_handler interface by a foreign engine .. > -select_handler *find_select_handler(THD *thd, > - SELECT_LEX* select_lex) > +select_handler *find_select_handler(THD *thd, SELECT_LEX *select_lex) ^ Don't reformat. > { > - if (select_lex->next_select()) > - return 0; > if (select_lex->master_unit()->outer_select()) > return 0; > > @@ -4921,15 +4922,14 @@ select_handler *find_select_handler(THD *thd, > { > tbl= select_lex->join->tables_list; > } > - else if (thd->lex->query_tables && > - thd->lex->query_tables->next_global) > + else if (thd->lex->query_tables && thd->lex->query_tables->next_global) > { > tbl= thd->lex->query_tables->next_global; > } > else > return 0; > > - for (;tbl; tbl= tbl->next_global) > + for (; tbl; tbl= tbl->next_global) > { > if (!tbl->table) > continue; > @@ -4937,12 +4937,14 @@ select_handler *find_select_handler(THD *thd, > if (!ht->create_select) > continue; > select_handler *sh= ht->create_select(thd, select_lex); > - return sh; > + if (sh) > + return sh; > } > return 0; > } Don't reformat. meaningful changes are ok. > > > + Remove the 3rd empty line. > /** > An entry point to single-unit select (a select without UNION). > ... diff --git a/sql/sql_union.cc b/sql/sql_union.cc index e8bdbcba2f2..4cb9f3e025a 100644 --- a/sql/sql_union.cc +++ b/sql/sql_union.cc @@ -1294,6 +1294,52 @@ bool init_item_int(THD* thd, Item_int* &item) return true; } > +select_handler *find_unit_handler(THD *thd, > + SELECT_LEX_UNIT *unit) ^^^ coding style. ... > @@ -2053,6 +2106,7 @@ void st_select_lex_unit::optimize_bag_operation(bool is_outer_distinct) > bag_set_op_optimized= true; > } > > +select_handler *find_select_handler(THD *thd, SELECT_LEX *select_lex); Please don't have function declaration in the middle of a file... > @@ -2272,14 +2350,14 @@ bool st_select_lex_unit::exec() > if (union_result->flush()) > { > thd->lex->current_select= lex_select_save; > - DBUG_RETURN(1); > + return true; Please fix identation. > diff --git a/storage/federatedx/federatedx_pushdown.cc b/storage/federatedx/federatedx_pushdown.cc > index 664f0570238..492055773cb 100644 > --- a/storage/federatedx/federatedx_pushdown.cc > +++ b/storage/federatedx/federatedx_pushdown.cc > @@ -163,13 +163,13 @@ void ha_federatedx_derived_handler::print_error(int, unsigned long) > } > > > -static select_handler* > -create_federatedx_select_handler(THD* thd, SELECT_LEX *sel) > +template<typename T> > +static select_handler *create_federatedx_handler(THD *thd, T *sel_lex) > { > if (!use_pushdown) > return 0; > Do we need to generate that much code? What if there was a function like bool check_pushdown_allowed(select_lex_node *node) which could process both SELECT_LEX and SELECT_LEX_UNIT objects. Then, we would need two one-liner functions: create_select_handler(type S) { if (check_pushdown_allowed(S)) return new ha_federatedx_select_handler(S) } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 489a7fba324: MDEV-29322 ASAN heap-use-after-free in Query_log_event::do_apply_event
by Sergei Golubchik 04 Sep '22

04 Sep '22

Hi, Andrei, I still don't understand this. 1. Why rgi->options_to_bin_log is only set for GTID_EVENT? Is there a guarantee that there always be a GTID_EVENT before a QUERY_EVENT? What if gtids aren't enabled? 2. how do you guarantee that all query events for a previous value of options_written_to_bin_log have been applied before updating it in the rgi? 3. and if you do guarantee it, why is it atomic? No query events = nobody reads options_written_to_bin_log concurrently. And why not to do a simple patch, like the one I've attached? (note, it's an intentionally minimal patch to show the fix, practically I'd accompany it with a small cleanup patch) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Sep 02, Andrei Elkin wrote: > > > Query_log_event cannot just randomly use some > > options_written_to_bin_log, it must use options_written_to_bin_log > > from the Format_description_log_event that came from the binlog file > > where the Query_log_event was read from. > > [ Not to engage you into a separate discussion, > still why 'randomly'? In the quoted description p.1 says > the Query_log_event uses `options_written_to_bin_log` from the > immediately preceding-in-binlog FD. ] > > > If rli->relay_log.description_event_for_exec is not that > > Format_description_log_event, and assuming that the correct > > Format_description_log_event is not stored anywhere, the Query_log_event > > has to remember options_written_to_bin_log itself, > > It's an option, but the chosen one avoids such remembering per each > Query event. Instead ... > > > there's not much we can do about it. > > > > Or it can apply the mask in the constructor and store not flags2 but > > the new value of thd->variables.option_bits. > > > > Or it can extend flags2 to cover all OPTIONS_WRITTEN_TO_BINLOG. > > ... of taking care of individual events the patch basically goes along > the base to use a placeholder that > A. lasts (unlike FD instance) the whole slave session time, and > B. the stored `options_written_to_binlog` is guaranteed to be a copy > of the most recent FD before the Query > > To help you to understand the method, here is a sequence diagram, > where on the top line > the slave SQL thread, its Relay-log-info object, a similar to that Worker's instance, > and a Worker thread. > > > SQL thread RLI/RGI Worker RGI Worker > | . . . > --->+ read FD . . i > | . . . > +--------------> > stores options > . > --->+ read Query > | . > +-------------------------->+ > finds Worker RGI > to store in theree a copy FD.options > | . > +-------------------------------------------->+ > schedules Query Worker | > . | > .<----------------+ > reads options | > | applies > ... Query > > It must be clear the A holds. > To prove B let after Query a second FD^2 is read by the SQL thread and > that FD^2 is from a different version server. > Then the SQL thread always have a wait-for-workers-to-become-idle > condition, so execution would go like this: > > --->+ read Query . ... > | . | > +-------------------------->+ | > finds Worker RGI | > to store in theree a copy FD.options | > | | > --->+ read FD^2 | applying of > | | Query > ... | > ... wait for Worker to complete Query | > ... | > +<============================================| > | > +-------------->. > | stores FD^2 > | options > ... > > So FD^2 can't affect queries from earlier binlogs. > > I am all ears to hear about your feedback. > > Cheers, > > Andrei

2 2

Re: [Maria-developers] 489a7fba324: MDEV-29322 ASAN heap-use-after-free in Query_log_event::do_apply_event
by Sergei Golubchik 01 Sep '22

01 Sep '22

Hi, Andrei, On Sep 01, Andrei wrote: > revision-id: 489a7fba324 (mariadb-10.5.17-10-g489a7fba324) > parent(s): 0324bde8469 > author: Andrei > committer: Andrei > timestamp: 2022-09-01 14:15:11 +0300 > message: > > MDEV-29322 ASAN heap-use-after-free in Query_log_event::do_apply_event > > The ASAN report was reported in the parallel slave execution of a Query > event and implicitly involved (so also parallelly run) Format-Description > event. > The Query actually had dependency on a preceding "old" FD whose > instance got destructed, so to cause the ASAN error. > > The case is fixed with copying of the value of > `options_written_to_bin_log` the sensitive member > of FD event into worker's stable placeholders, so FD change > won't affect Queries dependent on "old" instances. > > The method observes the following properties: > 0. each worker gets its private copy; > 1. the copied value gets a part of the execution context for any > Queries that follow the FD in binlog; > 2. any new FD forces a new copy which does not any affect on > the past (in binlog order) Queries which might be still in > execution. Sorry, but this doesn't make any sense. The value of options_written_to_bin_log depends on the version of the server that has written that binlog file. Query_log_event cannot just randomly use some options_written_to_bin_log, it must use options_written_to_bin_log from the Format_description_log_event that came from the binlog file where the Query_log_event was read from. If rli->relay_log.description_event_for_exec is not that Format_description_log_event, and assuming that the correct Format_description_log_event is not stored anywhere, the Query_log_event has to remember options_written_to_bin_log itself, there's not much we can do about it. Or it can apply the mask in the constructor and store not flags2 but the new value of thd->variables.option_bits. Or it can extend flags2 to cover all OPTIONS_WRITTEN_TO_BINLOG. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5154e224a97: MDEV-5215 Granted to PUBLIC
by Sergei Golubchik 25 Aug '22

25 Aug '22

Hi, Oleksandr, > commit 5154e224a97 > Author: Oleksandr Byelkin <sanja(a)mariadb.com> > Date: Mon Dec 13 16:15:21 2021 +0100 > > diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql > index fd2f1c95dda..a1721913b2c 100644 > --- a/scripts/mysql_system_tables.sql > +++ b/scripts/mysql_system_tables.sql > @@ -47,6 +47,10 @@ INSERT IGNORE INTO global_priv SELECT * FROM tmp_user_sys WHERE 0 <> @need_sys_u > DROP TABLE tmp_user_sys; > > > +-- This special "role" needed for GRAND ... TO PUBLIC > +INSERT IGNORE INTO global_priv (Host,User,Priv) VALUES ('', 'PUBLIC', concat('{"access":0,"version_id":',regexp_replace(regexp_replace(@@version, '\\b\\d\\b', '0\\0'), '\\D', ''),',"is_role":true}')); 1. why here and not in mysql_system_tables_data.sql ? 2. why this version magic? mysql_system_tables_data.sql doesn't do it for other accounts 3. why do you do it at all? I thought the server can create this role as needed > + > + > CREATE DEFINER='mariadb.sys'@'localhost' SQL SECURITY DEFINER VIEW IF NOT EXISTS user AS SELECT > Host, > User, > @@ -95,7 +99,9 @@ CREATE DEFINER='mariadb.sys'@'localhost' SQL SECURITY DEFINER VIEW IF NOT EXISTS > ELT(IFNULL(JSON_VALUE(Priv, '$.is_role'), 0) + 1, 'N', 'Y') AS is_role, > IFNULL(JSON_VALUE(Priv, '$.default_role'), '') AS default_role, > CAST(IFNULL(JSON_VALUE(Priv, '$.max_statement_time'), 0.0) AS DECIMAL(12,6)) AS max_statement_time > - FROM global_priv; > + FROM global_priv > +-- Do not show special role for GRANT TO PUBLIC > + WHERE not (Host = "" and User = "PUBLIC"); Why? I don't think it's wrong to show a new PUBLIC role. > > -- Remember for later if user table already existed > set @had_user_table= @@warning_count != 0; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 601cae2e945..aa9bd91ec52 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -316,6 +318,13 @@ static bool show_table_and_column_privileges(THD *, const char *, const char *, > static int show_routine_grants(THD *, const char *, const char *, > const Sp_handler *sph, char *, int); > > +static ACL_ROLE *acl_public= NULL; > + > +inline privilege_t public_access() > +{ > + return (acl_public ? acl_public->access : NO_ACL); > +} Why not to have acl_public always not null? e.g. define it as static ACL_ROLE acl_public; so it's not even a pointer. And don't store it in the dynamic array. Or make it like static ACL_ROLE acl_public_dummy, *acl_public= &acl_public_dummy; then you can keep he real role in the dynamic array, but still acl_public will never be null. but in the standard PUBLIC is not a role at all, so may be it'd be simpler not to store it in the dynamic array > + > class Grant_tables; > class User_table; > class Proxies_priv_table; > @@ -3148,7 +3183,7 @@ bool acl_getroot(Security_context *sctx, const char *user, const char *host, > } > else // Role, not User > { > - ACL_ROLE *acl_role= find_acl_role(user); > + ACL_ROLE *acl_role= find_acl_role(user, false); are there tests for that? > if (acl_role) > { > res= 0; > @@ -3161,6 +3196,26 @@ bool acl_getroot(Security_context *sctx, const char *user, const char *host, > } > } > > + /* > + PUBLIC magic: > + > + Note: for usual user privileges of 3 component merged together: > + 1) user privileges > + 2) set role privileges > + 3) public privileges > + But for this routine (used in Security_context::change_security_context) > + only 2 component merged: > + 1) user OR role privileges we are switching to because if we're switching to a user, there can be no set role? I feel this comment is more confusing, then helping, really, I'd suggest to remove it completely > + 2) public privileges > + */ > + if (acl_public) > + { > + if (ACL_DB *acl_db= acl_db_find(db, public_name.str, "", "", FALSE)) > + sctx->db_access|= acl_db->access; > + > + sctx->master_access|= acl_public->access; > + } > + > mysql_mutex_unlock(&acl_cache->lock); > DBUG_RETURN(res); > } > @@ -3320,20 +3375,22 @@ int acl_setrole(THD *thd, const char *rolename, privilege_t access) > /* merge the privileges */ > Security_context *sctx= thd->security_ctx; > sctx->master_access= access; > - if (thd->db.str) > - sctx->db_access= acl_get(sctx->host, sctx->ip, sctx->user, thd->db.str, FALSE); > - > if (!strcasecmp(rolename, "NONE")) > { > thd->security_ctx->priv_role[0]= 0; > } > else > { > - if (thd->db.str) > - sctx->db_access|= acl_get("", "", rolename, thd->db.str, FALSE); > /* mark the current role */ > strmake_buf(thd->security_ctx->priv_role, rolename); > } > + if (thd->db.str) > + sctx->db_access= acl_get_all3(sctx, thd->db.str, FALSE); > + > + // PUBLIC magic > + if (acl_public) > + sctx->master_access|= acl_public->access; why not to do it in check_user_can_set_role()? Like - *access = acl_user->access | role->access; + *access = acl_user->access | role->access | acl_public->access; > + > return 0; > } > > @@ -3347,9 +3404,13 @@ static uchar* check_get_key(ACL_USER *buff, size_t *length, > > static void acl_update_role(const char *rolename, const privilege_t privileges) > { > - ACL_ROLE *role= find_acl_role(rolename); > + ACL_ROLE *role= find_acl_role(rolename, true); > if (role) > + { > role->initial_role_access= role->access= privileges; > + if (strcasecmp(rolename, public_name.str) == 0) > + acl_public= role; why? I mean, in what case can acl_public != role here? > + } > } > > > @@ -3638,6 +3703,23 @@ privilege_t acl_get(const char *host, const char *ip, > DBUG_RETURN(db_access & host_access); > } > > +/* > + Check if there is access for the host/user, role, public on the database > +*/ > + > +privilege_t acl_get_all3(Security_context *sctx, const char *db, > + bool db_is_patern) > +{ > + privilege_t access= acl_get(sctx->host, sctx->ip, > + sctx->priv_user, db, db_is_patern); > + if (sctx->priv_role[0]) > + access|= acl_get("", "", sctx->priv_role, db, db_is_patern); > + if (acl_public) > + access|= acl_get("", "", public_name.str, db, db_is_patern); how can this be optimized, considering that in most cases and for most db names there will be no grants to role and public? A couple of ideas: maintain counters total_number_of_db_grants_to_roles ad total_number_of_db_grants_to_public. This is rather crude. keep grants to users, to roles, and to public in separate caches. or in different dynamic arrays (won't help much in a default setup) keep propery per ACL_DB or shared between all ACL_DB's of one DB? that'd be rather tricky to maintain > + return access; > +} > + > + > /* > Check if there are any possible matching entries for this host > > @@ -3759,7 +3841,7 @@ static bool add_role_user_mapping(const char *uname, const char *hname, > const char *rname) > { > ACL_USER_BASE *grantee= find_acl_user_base(uname, hname); > - ACL_ROLE *role= find_acl_role(rname); > + ACL_ROLE *role= find_acl_role(rname, true); why? > > if (grantee == NULL || role == NULL) > return 1; > @@ -4247,7 +4329,7 @@ bool is_acl_user(const char *host, const char *user) > if (*host) // User > res= find_user_exact(host, user) != NULL; > else // Role > - res= find_acl_role(user) != NULL; > + res= find_acl_role(user, false) != NULL; this disallows DEFINER PUBLIC, right? I couldn't find a test for that. > > mysql_mutex_unlock(&acl_cache->lock); > return res; > @@ -7537,11 +7620,11 @@ bool mysql_grant_role(THD *thd, List <LEX_USER> &list, bool revoke) > if (user->host.str) > hostname= user->host; > else > - if ((role_as_user= find_acl_role(user->user.str))) > + if ((role_as_user= find_acl_role(user->user.str, false))) Why is it false here? It looks like it disallows GRANT role TO PUBLIC > hostname= empty_clex_str; > else > { > - if (is_invalid_role_name(username.str)) > + if (check_role_name(username.str, false) == ROLE_NAME_INVALID) > { > append_user(thd, &wrong_users, &username, &empty_clex_str); > result= 1; > @@ -8280,19 +8355,14 @@ bool check_grant(THD *thd, privilege_t want_access, TABLE_LIST *tables, > if (any_combination_will_do) > continue; > > - t_ref->grant.grant_table_user= grant_table; // Remember for column test > - t_ref->grant.grant_table_role= grant_table_role; > - t_ref->grant.version= grant_version; > - t_ref->grant.privilege|= grant_table ? grant_table->privs : NO_ACL; > - t_ref->grant.privilege|= grant_table_role ? grant_table_role->privs : NO_ACL; > + t_ref->grant.privilege|= t_ref->grant.aggregate_privs(); > t_ref->grant.want_privilege= ((want_access & COL_ACLS) & ~t_ref->grant.privilege); > > if (!(~t_ref->grant.privilege & want_access)) > continue; > > - if ((want_access&= ~((grant_table ? grant_table->cols : NO_ACL) | > - (grant_table_role ? grant_table_role->cols : NO_ACL) | > - t_ref->grant.privilege))) > + if ((want_access&= ~(t_ref->grant.aggregate_cols() | > + t_ref->grant.privilege))) > { > goto err; // impossible > } 1. why is this if() here, if it's impossible? 2. I'm a bit worried that you set t_ref->grant.version early when it's still possible to goto err. Old code was setting the version very late after all checks have succeeded. > @@ -8335,6 +8405,49 @@ static void check_grant_column_int(GRANT_TABLE *grant_table, const char *name, > } > } > > +inline privilege_t GRANT_INFO::aggregate_privs() > +{ > + return (grant_table_user ? grant_table_user->privs : NO_ACL) | > + (grant_table_role ? grant_table_role->privs : NO_ACL) | > + (grant_public ? grant_public->privs : NO_ACL); > +} > + > +inline privilege_t GRANT_INFO::aggregate_cols() > +{ > + return (grant_table_user ? grant_table_user->cols : NO_ACL) | > + (grant_table_role ? grant_table_role->cols : NO_ACL) | > + (grant_public ? grant_public->cols : NO_ACL); > +} > + > +void GRANT_INFO::refresh(const Security_context *sctx, > + const char *db, const char *table) > +{ > + if (version != grant_version) > + read(sctx, db, table); > +} > + > +void GRANT_INFO::read(const Security_context *sctx, > + const char *db, const char *table) > +{ > +#ifdef EMBEDDED_LIBRARY > + grant_table_user= grant_table_role= grant_public= NULL; > +#else > + grant_table_user= > + table_hash_search(sctx->host, sctx->ip, db, > + sctx->priv_user, > + table, FALSE); /* purecov: inspected */ you don't need to preserve purecov comments when moving the code around > + grant_table_role= > + sctx->priv_role[0] ? table_hash_search("", NULL, db, > + sctx->priv_role, > + table, TRUE) : NULL; > + grant_public= > + acl_public ? table_hash_search("", NULL, db, > + public_name.str, > + table, TRUE) : NULL; > +#endif > + version= grant_version; /* purecov: inspected */ > +} > + > /* > Check column rights in given security context > > @@ -9533,6 +9626,13 @@ static bool show_global_privileges(THD *thd, ACL_USER_BASE *acl_entry, > want_access= ((ACL_ROLE *)acl_entry)->initial_role_access; > else > want_access= acl_entry->access; > + > + // suppress "GRANT USAGE ON *.* TO `PUBLIC`" why? I agree that it's pointless, but disabling just it is kind of difficult to explain. > + if (!(want_access & ~GRANT_ACL) && > + acl_entry->user.length == public_name.length && > + strcasecmp(acl_entry->user.str, public_name.str) == 0) > + return FALSE; > + > if (test_all_bits(want_access, (GLOBAL_ACLS & ~ GRANT_ACL))) > global.append(STRING_WITH_LEN("ALL PRIVILEGES")); > else if (!(want_access & ~GRANT_ACL)) > @@ -11015,10 +11116,15 @@ bool mysql_drop_user(THD *thd, List <LEX_USER> &list, bool handle_as_role) > { > int rc; > user_name= get_current_user(thd, tmp_user_name, false); > - if (!user_name) > + if (!user_name || (handle_as_role && > + (strcasecmp(user_name->user.str, > + public_name.str) == 0))) better add this condition to the following if(), not to this one. Like: - if (handle_as_role != user_name->is_role()) + if (handle_as_role != user_name->is_role() || + (handle_as_role && check_role_name(...) == ROLE_NAME_INVALID)) > { > thd->clear_error(); > - append_str(&wrong_users, STRING_WITH_LEN("CURRENT_ROLE")); > + if (!user_name) > + append_str(&wrong_users, STRING_WITH_LEN("CURRENT_ROLE")); > + else > + append_str(&wrong_users, public_name.str, public_name.length); > result= TRUE; > continue; > } > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 6290a47ee30..143e647f74c 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -5304,7 +5300,7 @@ int get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) > &thd->col_access, NULL, 0, 1) || > (!thd->col_access && check_grant_db(thd, db_name->str))) || > sctx->master_access & (DB_ACLS | SHOW_DB_ACL) || > - acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name->str, 0)) > + acl_get_all3(sctx, db_name->str, 0)) Was it a bug? That one couldn't see db's if privileges were granted via roles? > #endif > { > Dynamic_array<LEX_CSTRING*> table_names(PSI_INSTRUMENT_MEM); > diff --git a/mysql-test/main/public_basic.test b/mysql-test/main/public_basic.test > new file mode 100644 > index 00000000000..3f5993dfe97 > --- /dev/null > +++ b/mysql-test/main/public_basic.test > @@ -0,0 +1,93 @@ > +SHOW GRANTS FOR PUBLIC; > + > +--echo # it is not PUBLIC but an user > +--echo # (this should work as it allowed for roles for example) > +create user PUBLIC; > +create user PUBLIC@localhost; > +GRANT SELECT on test.* to PUBLIC@localhost; > +drop user PUBLIC@localhost; > +drop user PUBLIC; > + > +--echo # preinstalled PUBLIC > +GRANT SELECT on test.* to PUBLIC; > +GRANT SELECT on mysql.db to PUBLIC; > +--replace_regex /"version_id"\:[0-9]+/"version_id":VERSION/ > +select * from mysql.global_priv where user="PUBLIC" ; > + > +SHOW GRANTS FOR PUBLIC; > + > +GRANT UPDATE on test.* to PUBLIC; > +GRANT UPDATE on mysql.db to PUBLIC; > + > +SHOW GRANTS FOR PUBLIC; > + > +REVOKE SELECT on test.* from PUBLIC; > +REVOKE SELECT on mysql.db from PUBLIC; > + > +SHOW GRANTS FOR PUBLIC; > + > +REVOKE UPDATE on test.* from PUBLIC; > +REVOKE UPDATE on mysql.db from PUBLIC; > + > +--error ER_NONEXISTING_GRANT > +REVOKE UPDATE on test.* from PUBLIC; > +--error ER_NONEXISTING_TABLE_GRANT > +REVOKE UPDATE on mysql.db from PUBLIC; > + > +SHOW GRANTS FOR PUBLIC; > + > +--echo # automaticly added PUBLIC > +delete from mysql.global_priv where user="PUBLIC"; > +flush privileges; > +select * from mysql.global_priv where user="PUBLIC" ; > +GRANT SELECT on test.* to PUBLIC; > +GRANT SELECT on mysql.db to PUBLIC; > +--replace_regex /"version_id"\:[0-9]+/"version_id":VERSION/ > +select * from mysql.global_priv where user="PUBLIC" ; > + > +SHOW GRANTS FOR PUBLIC; > + > +GRANT UPDATE on test.* to PUBLIC; > +GRANT UPDATE on mysql.db to PUBLIC; > + > +SHOW GRANTS FOR PUBLIC; > + > +REVOKE SELECT on test.* from PUBLIC; > +REVOKE SELECT on mysql.db from PUBLIC; > + > +SHOW GRANTS FOR PUBLIC; > + > +REVOKE UPDATE on test.* from PUBLIC; > +REVOKE UPDATE on mysql.db from PUBLIC; > + > +SHOW GRANTS FOR PUBLIC; > + > +--error ER_INVALID_ROLE > +GRANT XXXXXX TO CURRENT_USER; > +--echo # following should fail with the same error as above > +--error ER_INVALID_ROLE > +GRANT PUBLIC TO CURRENT_USER; > + > +--error ER_INVALID_ROLE > +REVOKE XXXXXX FROM CURRENT_USER; > +--echo # following should fail with the same error as above > +--error ER_INVALID_ROLE > +REVOKE PUBLIC FROM CURRENT_USER; > +--error ER_CANNOT_USER > + > +drop role XXXXXX; > +--echo # following should fail with the same error as above > +--error ER_CANNOT_USER > +drop role PUBLIC; > + > +--error ER_INVALID_ROLE > +SET ROLE XXXXXX; > +--echo # following should fail with the same error as above > +--error ER_INVALID_ROLE > +SET ROLE PUBLIC; > + > +--error ER_INVALID_ROLE > +SET DEFAULT ROLE XXXXXX; > +--echo # following should fail with the same error as above > +--error ER_INVALID_ROLE > +SET DEFAULT ROLE PUBLIC; This is good, but one can update global_priv table directly and put the default_role=PUBLIC there. Please, add a test for that. and also a couple of tests for `public` (in backticks) > diff --git a/mysql-test/main/public_privileges.test b/mysql-test/main/public_privileges.test > new file mode 100644 > index 00000000000..a542376f05c > --- /dev/null > +++ b/mysql-test/main/public_privileges.test > @@ -0,0 +1,293 @@ > +--echo # > +--echo # Test DB/TABLE/COLUMN privileges in queries > +--echo # > + > +SHOW GRANTS FOR PUBLIC; > + > +create user testuser; > +create database testdb1; > +use testdb1; > +create table t1 (a int, b int); > +insert into t1 values (1,2); > +create database testdb2; > +use testdb2; > +create table t2 (a int, b int); > +insert into t2 values (1,2); > +create table t3 (a int, b int); > +insert into t3 values (1,2); > + > +connect (testuser,localhost,testuser,,); > +connection testuser; "connect testuser" also switches to this connection, the following "connection testuser" is redundant > +--error ER_TABLEACCESS_DENIED_ERROR > +select * from testdb1.t1; > +--error ER_TABLEACCESS_DENIED_ERROR > +select * from testdb2.t2; > +--error ER_TABLEACCESS_DENIED_ERROR > +select b from testdb2.t3; > +--error ER_TABLEACCESS_DENIED_ERROR > +select a from testdb2.t3; > + > +connection default; > + > +GRANT SELECT ON testdb1.* to PUBLIC; > +GRANT SELECT ON testdb2.t2 to PUBLIC; > +GRANT SELECT (b) ON testdb2.t3 to PUBLIC; > + > +disconnect testuser; > +connect (testuser,localhost,testuser,,); > +connection testuser; > +select * from testdb1.t1; > +select * from testdb2.t2; > +select b from testdb2.t3; > +--error ER_COLUMNACCESS_DENIED_ERROR > +select a from testdb2.t3; > + > +connection default; > + > +--replace_column 1 # 3 # 6 # 7 # > +SHOW PROCESSLIST; why? > + > +connection default; > + > +use test; > +disconnect testuser; > +REVOKE SELECT ON testdb1.* from PUBLIC; > +REVOKE SELECT ON testdb2.t2 from PUBLIC; > +REVOKE SELECT (b) ON testdb2.t3 from PUBLIC; may be `revoke all privileges, grant option` ? looks like a good place to have it tested > +drop user testuser; > +drop database testdb1; > +drop database testdb2; > + > +--echo # > +--echo # test global process list privilege and EXECUTE db level > +--echo # > + > +create user testuser; > +create database testdb; > +use testdb; > +create procedure p1 () select 1; > + > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +--replace_column 1 # 3 # 6 # 7 # > +SHOW PROCESSLIST; > +--error ER_PROCACCESS_DENIED_ERROR > +call testdb.p1(); > + > +connection default; > + > +GRANT PROCESS ON *.* to PUBLIC; > +GRANT EXECUTE ON testdb.* to PUBLIC; > + > +disconnect testuser; > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +--replace_column 1 # 3 # 6 # 7 # > +SHOW PROCESSLIST; > +call testdb.p1(); > + > +connection default; > + > +--replace_column 1 # 3 # 6 # 7 # > +SHOW PROCESSLIST; > + > +connection default; > + > +use test; > +disconnect testuser; > +REVOKE PROCESS ON *.* from PUBLIC; > +REVOKE EXECUTE ON testdb.* from PUBLIC; > +drop user testuser; > +drop database testdb; > + > +--echo # > +--echo # test DB privilege to allow USE statement > +--echo # > + > +create user testuser; > +create database testdb; > + > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +--error ER_DBACCESS_DENIED_ERROR > +use testdb; > + > +connection default; > + > +GRANT LOCK TABLES ON testdb.* to PUBLIC; > + > +disconnect testuser; > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +use testdb; > + > +connection default; > + > +use test; > +disconnect testuser; > +REVOKE LOCK TABLES ON testdb.* from PUBLIC; > +drop user testuser; > +drop database testdb; > + > + > +--echo # > +--echo # test DB privilege to allow USE statement (as above) > +--echo # test current db privileges > +--echo # > + > +create user testuser; > +create database testdb; > +use testdb; > +create table t1 (a int); > +insert into t1 values (1); > +GRANT LOCK TABLES ON testdb.* to PUBLIC; > + > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +use testdb; > +--error ER_TABLEACCESS_DENIED_ERROR > +update t1 set a=a+1; > + > +connection default; > + > +GRANT UPDATE,SELECT ON testdb.* to PUBLIC; > + > +disconnect testuser; > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +use testdb; > +update t1 set a=a+1; > + > +connection default; > +select * from testdb.t1; > + > +use test; > +disconnect testuser; > +REVOKE LOCK TABLES ON testdb.* from PUBLIC; > +REVOKE UPDATE,SELECT ON testdb.* from PUBLIC; > +drop user testuser; > +drop database testdb; > + > + > +--echo # > +--echo # test DB privilege to allow USE statement (as above) > +--echo # test table/column privileges in current DB > +--echo # > + > +create user testuser; > +create database testdb; > +use testdb; > +create table t1 (a int); > +insert into t1 values (1); > +create table t2 (a int, b int); > +insert into t2 values (1,2); > +GRANT LOCK TABLES ON testdb.* to PUBLIC; > + > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +use testdb; > +--error ER_TABLEACCESS_DENIED_ERROR > +delete from t1; > +--error ER_TABLEACCESS_DENIED_ERROR > +select b from t2; > +--error ER_TABLEACCESS_DENIED_ERROR > +select a from t2; > + > +connection default; > + > +GRANT DELETE ON testdb.t1 to PUBLIC; > +GRANT SELECT (a) ON testdb.t2 to PUBLIC; > + > +disconnect testuser; > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +use testdb; > +delete from t1; > +select a from t2; > +--error ER_COLUMNACCESS_DENIED_ERROR > +select b from t2; > + > +connection default; > +select * from testdb.t1; > + > +use test; > +disconnect testuser; > +REVOKE LOCK TABLES ON testdb.* from PUBLIC; > +REVOKE DELETE ON testdb.t1 from PUBLIC; > +REVOKE SELECT (a) ON testdb.t2 from PUBLIC; > +drop user testuser; > +drop database testdb; > + > +--echo # > +--echo # test function privilege > +--echo # > + > +create user testuser; > +create database testdb; > +use testdb; > +create function f1() returns int return 2; > + > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +--error ER_PROCACCESS_DENIED_ERROR > +alter function testdb.f1 comment "A stupid function"; > +--error ER_PROCACCESS_DENIED_ERROR > +select testdb.f1(); > + > +connection default; > + > +GRANT ALTER ROUTINE ON testdb.* to PUBLIC; > + > +disconnect testuser; > +connect (testuser,localhost,testuser,,); > +connection testuser; > + > +alter function testdb.f1 comment "A stupid function"; > +--error ER_PROCACCESS_DENIED_ERROR > +select testdb.f1(); > + > +connection default; > + > +use test; > +disconnect testuser; > +REVOKE ALTER ROUTINE ON testdb.* from PUBLIC; > +drop function testdb.f1; > +drop user testuser; > +drop database testdb; > + > +--echo # > +--echo # bug with automatically added PUBLIC role > +--echo # > + > +--echo # automaticly added PUBLIC > +delete from mysql.global_priv where user="PUBLIC"; > +flush privileges; > +GRANT SELECT on test.* to PUBLIC; > + > +REVOKE SELECT on test.* from PUBLIC; > + > +create user testuser; > +create database testdb1; > +use testdb1; > +create table t1 (a int, b int); > +insert into t1 values (1,2); > + > +connect (testuser,localhost,testuser,,); > +connection testuser; > +--error ER_TABLEACCESS_DENIED_ERROR > +select * from testdb1.t1; > + > +connection default; > + > +disconnect testuser; > +drop user testuser; > +drop database testdb1; > diff --git a/mysql-test/suite/roles/none_public.test b/mysql-test/suite/roles/none_public.test > index a0ec2315cfc..d0ba62d226b 100644 > --- a/mysql-test/suite/roles/none_public.test > +++ b/mysql-test/suite/roles/none_public.test > @@ -19,8 +19,9 @@ grant select on *.* to none; > grant public to role1; > --error ER_INVALID_ROLE > grant role1 to public; ^^^ this shouldn't fail > ---error ER_INVALID_ROLE > -grant select on *.* to public; > +# PUBLIC is legal role > +#--error ER_INVALID_ROLE > +#grant select on *.* to public; you can just remove it, commenting out isn't better > --error ER_INVALID_ROLE > grant role1 to current_role; > @@ -40,16 +41,18 @@ revoke select on *.* from public; > > --error ER_INVALID_ROLE > show grants for none; > ---error ER_INVALID_ROLE > -show grants for public; > +# PUBLIC is legal role > +#--error ER_INVALID_ROLE > +#show grants for public; > > --error ER_INVALID_ROLE > create definer=none view test.v1 as select 1; > ---error ER_INVALID_ROLE > -create definer=public view test.v1 as select 1; > +# PUBLIC is legal role > +#--error ER_INVALID_ROLE > +#create definer=public view test.v1 as select 1; eh, no, PUBLIC isn't a valid definer, as far as I understand. > > drop role role1; > > -insert mysql.global_priv values ('', 'none', '{"is_role":true}'), ('', 'public', '{"is_role":true}'); > +insert mysql.global_priv values ('', 'none', '{"is_role":true}'); > flush privileges; > -delete from mysql.global_priv where host=''; > +delete from mysql.global_priv where host='' and user='none'; > diff --git a/mysql-test/main/sp_notembedded.result b/mysql-test/main/sp_notembedded.result > index e03361598a6..239f8acb7f3 100644 > --- a/mysql-test/main/sp_notembedded.result > +++ b/mysql-test/main/sp_notembedded.result > @@ -345,7 +345,6 @@ show grants; > Grants for foo1@localhost > GRANT USAGE ON *.* TO `foo1`@`localhost` IDENTIFIED BY PASSWORD '-F3A2A51A9B0F2BE2468926B4132313728C250DBF' > GRANT CREATE ROUTINE ON `test`.* TO `foo1`@`localhost` > -GRANT EXECUTE, ALTER ROUTINE ON PROCEDURE `test`.`spfoo` TO `foo1`@`localhost` Why did this line disappear? > connection default; > disconnect foo; > drop procedure spfoo; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] bcfc424d859: MDEV-5215 prerequisite: remove test and test_* database hacks in the test suite
by Sergei Golubchik 25 Aug '22

25 Aug '22

Hi, Oleksandr, On Aug 25, Oleksandr Byelkin wrote: > revision-id: bcfc424d859 (mariadb-10.6.1-483-gbcfc424d859) > parent(s): 1c3182a99e5 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-08-16 11:49:41 +0200 > message: > > MDEV-5215 prerequisite: remove test and test_* database hacks in the test suite let's say, it's ok. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 1c3182a99e5: MDEV-5215 prerequisite of prerequisite: if DB is not mentioned in connect ignore errors of switching to it
by Sergei Golubchik 25 Aug '22

25 Aug '22

Hi, Oleksandr, On Aug 25, Oleksandr Byelkin wrote: > revision-id: 1c3182a99e5 (mariadb-10.6.1-482-g1c3182a99e5) > parent(s): 7253cdf8928 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-08-16 11:49:41 +0200 > message: > > MDEV-5215 prerequisite of prerequisite: if DB is not mentioned in connect ignore errors of switching to it > > diff --git a/mysql-test/main/init_connect.test b/mysql-test/main/init_connect.test > index e96d02fe0d1..60549c3458e 100644 > --- a/mysql-test/main/init_connect.test > +++ b/mysql-test/main/init_connect.test > @@ -10,31 +10,31 @@ > > --source include/add_anonymous_users.inc > > -connect (con0,localhost,root,,); > +connect (con0,localhost,root,,test); why did you specify a db explicitly (here and in many other places)? I thought the whole point was to modify mysqltest in a way that would avoid the need of changing tests. > connection con0; > select hex(@a); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
by Sergei Golubchik 24 Aug '22

24 Aug '22

Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2021-03-01 17:43:34 +0300 > message: > > MDEV-16546 System versioning setting to allow history modification > > 1. force_fields_visible session variable makes system-invisible and > user-invisible fields visible on next table open. FLUSH TABLES > required for already opened tables. * system_versioning_insert_history * allows pseudocolumns ROW_START and ROW_END be specified in INSERT * doesn't affect invisible columns * FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better > 2. If secure_timestamp allows to modify timestamp variable then > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > specify row_start and row_end system field values. when system_versioning_insert_history=1 > 3. Cleaned up select_insert::send_data() from setting vers_write as > this parameter is now set on TABLE initialization. I don't see where it's set. This commit removes two table->vers_write= table->versioned(); and adds one from_field->table->vers_write= false; in Item_field::fix_fields. But where is vers_write set to true? Was it done in some other commit? > diff --git a/mysql-test/suite/versioning/r/insert.result b/mysql-test/suite/versioning/r/insert.result > index 2645d0184e8..00d1bb705c4 100644 > --- a/mysql-test/suite/versioning/r/insert.result > +++ b/mysql-test/suite/versioning/r/insert.result > @@ -112,3 +112,95 @@ x y > 9 9001 > drop table t1; > drop table t2; > +# > +# MDEV-16546 System versioning setting to allow history modification > +# > +# restart: --secure-timestamp=NO > +set @@session.time_zone='+00:00'; > +create table t1(x int) with system versioning; > +insert into t1(x) values (1); > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); > +ERROR 42S22: Unknown column 'row_start' in 'field list' > +set session force_fields_visible= 1; > +flush tables; > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL, > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, shouldn't be visible here, only in insert and load. > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', '1980-01-01 00:00:01'); please, add tests for insert into t1 values (4) this should work, row_start/row_end must be mentioned explicitly. Also insert into t1 set x=5, row_start=..., row_end=... this should work too. And when only one row_start or row_end is mentioned as well. But update t1 set row_start=... insert into t1 (x, row_start, row_end) values (...) on duplicate key update row_start=... this should fail. While replace load data ... replace should work, but as DELETE+INSERT (I mean that DELETE in versioned tables doesn't actually delete history). If that's too difficult, then an error too. > +update t1 set x= x + 1; > +select x, check_row_ts(row_start, row_end) from t1 for system_time all order by x; ... > diff --git a/mysql-test/suite/versioning/t/insert.opt b/mysql-test/suite/versioning/t/insert.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes why is this? > -- source suite/versioning/common_finish.inc > diff --git a/mysql-test/suite/versioning/t/insert2.opt b/mysql-test/suite/versioning/t/insert2.opt > new file mode 100644 > index 00000000000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert2.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes and this? > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d23b190b2c5..93f8a8434e4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid &gtid) > #endif > } > > +bool THD::vers_modify_sys_field() const don't call it vers_modify_sys_field, your previous name vers_modify_history was better. There may be other sys fields that will never be directly writable (like ROWID, for example). > +{ > + if (lex->sql_command != SQLCOM_INSERT && > + lex->sql_command != SQLCOM_INSERT_SELECT && > + lex->sql_command != SQLCOM_LOAD) > + return false; I don't think this is enough to reject insert ... on duplicate key update row_start=xxx > + if (opt_secure_timestamp >= SECTIME_REPL || this is fixed in a later commit, so ok > + (opt_secure_timestamp == SECTIME_SUPER && > + !(security_ctx->master_access & SUPER_ACL))) > + return false; > + return true; > +} > + > + > void > wait_for_commit::reinit() > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2913a4a8350..520ea1d63bf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -820,6 +820,7 @@ typedef struct system_variables > > vers_asof_timestamp_t vers_asof_timestamp; > ulong vers_alter_history; > + my_bool force_fields_visible; system_versioning_insert_history > } SV; > > /** > @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */ > Item *sp_prepare_func_item(Item **it_addr, uint cols= 1); > bool sp_eval_expr(Field *result_field, Item **expr_item_ptr); > > + bool vers_modify_sys_field() const; vers_modify_history > }; > > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index f0b84ab126f..84c2ae4ab36 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) does it even make sense? I mean direct insertion of transaction ids. > { > bitmap_set_bit(table->write_set, table->vers_start_field()->field_index); > table->file->column_bitmaps_signal(); > @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) > info->deleted++; > if (!table->file->has_transactions()) > thd->transaction->stmt.modified_non_trans_table= TRUE; > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes. > { > store_record(table, record[2]); > error= vers_insert_history_row(table); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index f817f9086a9..4429308a097 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size( > SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024), > BLOCK_SIZE(1)); > + > +static Sys_var_mybool Sys_force_fields_visible( > + "force_fields_visible", "Make invisible fields visible on next table open", "system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns" > + SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE)); > diff --git a/sql/table.cc b/sql/table.cc > index f4bdbdeac5a..520f3738c9e 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share, > { > if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, outparam))) > goto err; > + if (thd->variables.force_fields_visible && > + (*field_ptr)->invisible <= INVISIBLE_SYSTEM) > + (*field_ptr)->invisible= VISIBLE; Not really, system_versioning_insert_history should not affect visibility of anything besides ROW_START and ROW_END pseudocolumns in the INSERT and LOAD DATA statements. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] Review request for MDEV-12325 Unexpected data type and truncation when using CTE
by Alexander Barkov 06 Aug '22

06 Aug '22

Hello Monty. The patch looks OK for me. I have just one question below: On 8/5/22 7:15 PM, Michael Widenius wrote: > Hi! > > Just to make things easy, here is the patch: > commit a556c4d972eaa09bb8ad586b9bda6cfb2a7b574a > Author: Monty <monty(a)mariadb.org> > Date: Fri Aug 5 17:57:27 2022 +0300 > > MDEV-12325 Unexpected data type and truncation when using CTE > > When creating a recursive CTE, the column types are taken from the > non recursive part of the CTE (this is according to the SQL standard). > > This patch adds code to abort the CTE if the calculated values in the > recursive part does not fit in the fields in the created temporary table. > > The new code only affects recursive CTE, so it should not cause any notable > problems for old applications. > <cut> > diff --git a/sql/sql_union.cc b/sql/sql_union.cc > index 48d8c16db68..d271a919a4e 100644 > --- a/sql/sql_union.cc > +++ b/sql/sql_union.cc > @@ -111,6 +111,8 @@ int select_unit::send_data(List<Item> &values) > { > int rc; > int not_reported_error= 0; > + enum_check_fields save_count_cuted_fields= thd->count_cuted_fields; > + > if (unit->offset_limit_cnt) > { // using limit offset,count > unit->offset_limit_cnt--; > @@ -118,8 +120,10 @@ int select_unit::send_data(List<Item> &values) > } > if (thd->killed == ABORT_QUERY) > return 0; > + > if (table->no_rows_with_nulls) > table->null_catch_flags= CHECK_ROW_FOR_NULLS_TO_REJECT; > + > if (intersect_mark) > { > fill_record(thd, table, table->field + 1, values, TRUE, FALSE); > @@ -127,6 +131,8 @@ int select_unit::send_data(List<Item> &values) > } > else > fill_record(thd, table, table->field, values, TRUE, FALSE); > + thd->count_cuted_fields= save_count_cuted_fields; > + > if (unlikely(thd->is_error())) > { > rc= 1; > @@ -297,7 +303,13 @@ bool select_unit::send_eof() > Why do you need above changes in select_unit::send_data()? Shouldn't it be enough just to have below changes in select_union_recursive::send_data()? > int select_union_recursive::send_data(List<Item> &values) > { > + Abort_on_warning_instant_set handle_abort(thd, 1); > + enum_check_fields save_count_cuted_fields= thd->count_cuted_fields; > + > + /* For recursive CTE's give warnings for wrong field info */ > + thd->count_cuted_fields= CHECK_FIELD_WARN; > int rc= select_unit::send_data(values); > + thd->count_cuted_fields= save_count_cuted_fields; > > if (rc == 0 && > write_err != HA_ERR_FOUND_DUPP_KEY && <cut>

1 1

Re: [Maria-developers] a507f126b03: MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability
by Sergei Golubchik 05 Aug '22

05 Aug '22

Hi, Alexander, a thought about the error message text and a couple of questions about changes in results, please, see below: On Aug 05, Alexander Barkov wrote: > revision-id: a507f126b03 (mariadb-10.7.4-39-ga507f126b03) > parent(s): 97d16c7544c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-08-05 09:42:46 +0400 > message: > > MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability > > 1. Store assignment failures on incompatible data types now cause errors if: > - STRICT_ALL_TABLES or STRICT_TRANS_TABLES sql_mode is used, and > - IGNORE is not used > > Otherwise, only a warning is raised and the statement continues. > > 2. Changing the error/warning test as follows: > > -ERROR HY000: Illegal parameter data types inet6 and int for operation 'SET' > +ERROR HY000: Unknown CAST(int AS inet6) in assignment of 'target' Better not to write 'CAST(int AS inet6)' if there's no explicit CAST() function, I think it might be confusing. I'd say Cannot cast int as inet6 in assignment of 'target' > so in case of a big table it's easier to see which column has the problem. > The new error text is aslo applied to SP variables. s/aslo/also/ > diff --git a/mysql-test/main/sp-vars.result b/mysql-test/main/sp-vars.result > index 127a0993e2b..feef67ccb1d 100644 > --- a/mysql-test/main/sp-vars.result > +++ b/mysql-test/main/sp-vars.result > @@ -1026,11 +1026,11 @@ BEGIN > SELECT arg; > END| > CALL p1((1, 2)); > -ERROR HY000: Illegal parameter data types tinyint and row for operation 'SET' > +ERROR 21000: Operand should contain 1 column(s) Why not "Unknown CAST" like elsewhere? > diff --git a/mysql-test/main/type_geometry_mix_int.result b/mysql-test/main/type_geometry_mix_int.result > index e5d00c63727..0d0a21a8d29 100644 > --- a/mysql-test/main/type_geometry_mix_int.result > +++ b/mysql-test/main/type_geometry_mix_int.result > @@ -6,6 +6,7 @@ > # > CREATE TABLE t1 (target GEOMETRY DEFAULT POINT(1,1), source INT DEFAULT 0); > # Start of type_store_assignment_incompatible.inc > +SET @sql_mode_save= (SELECT @@sql_mode); strange, why not `SET @sql_mode_save= @@sql_mode` ? > SET @source_type= (SELECT COLUMN_TYPE FROM INFORMATION_SCHEMA.COLUMNS > WHERE COLUMN_NAME='source' > AND TABLE_NAME='t1' > @@ -47,6 +52,7 @@ INSERT INTO t3 VALUES > (2, > (SELECT source FROM t2 ORDER BY id LIMIT 1), > (SELECT source FROM t2 ORDER BY id LIMIT 1)); > +ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field What's that? It wasn't an error, and now it is? > SELECT * FROM v3; > id target source > 1 POINT(1 1) 0 Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: [Maria-developers] f3fc58149dd: MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability
by Sergei Golubchik 05 Aug '22

05 Aug '22

Hi, Alexander, A couple of comments: On Aug 03, Alexander Barkov wrote: > revision-id: f3fc58149dd (mariadb-10.7.4-39-gf3fc58149dd) > parent(s): 97d16c7544c > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-08-03 11:28:31 +0400 > message: > > MDEV-29159 Patch for MDEV-28918 introduces more inconsistency than it solves, breaks usability > > Store assignment failures on incompatible data types now cause errors if: > - STRICT_ALL_TABLES or STRICT_TRANS_TABLES sql_mode is used, and > - IGNORE is not used > > Otherwise, only a warning is raised and the statement continues. > > TODO: add MTR tests for "loose" modes. > > diff --git a/sql/field.cc b/sql/field.cc > index 249269a6b1d..e997e2cbd69 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -964,7 +964,7 @@ Type_handler::aggregate_for_result_traditional(const Type_handler *a, > } > > > -bool Field::check_assignability_from(const Type_handler *from) const > +bool Field::check_assignability_from(const Type_handler *from, bool error) const "error" is very confusing here, I had to read the whole patch to understand what it means. Better names: * only_warning * ignore * strict * issue_error > { > /* > Using type_handler_for_item_field() here to get the data type handler > @@ -982,6 +982,15 @@ bool Field::check_assignability_from(const Type_handler *from) const > type_handler_for_item_field()); > if (th.aggregate_for_result(from->type_handler_for_item_field())) > { > + if (!error) > + { > + THD *thd= get_thd(); > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, > + ER_THD(thd, ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION), > + type_handler()->name().ptr(), from->name().ptr(), "SET"); > + return false; > + } > my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(0), > type_handler()->name().ptr(), from->name().ptr(), "SET"); > return true; if you wouldn't want to change the error message in a followup commit, I'd suggested something like my_error(ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION, MYF(error ? 0 : ME_WARNING), type_handler()->name().ptr(), from->name().ptr(), "SET"); return error; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] Review: MDEV-29075 Changing explicit_defaults_for_timestamp within stored procedure works inconsistently
by Alexander Barkov 01 Aug '22

01 Aug '22

Hello Sergei, I have a couple of suggestions: > commit 7b8304045272111a6f4d44196d6b37cbfef06f37 > Author: Sergei Golubchik <serg(a)mariadb.org> > Date: Wed Jul 20 17:31:48 2022 +0200 > > MDEV-29075 Changing explicit_defaults_for_timestamp within stored procedure works inconsistently > > move the OPTION_EXPLICIT_DEF_TIMESTAMP check from the parsing step > to the execution > <cut> > > diff --git a/sql/field.cc b/sql/field.cc > index 2e0c70d3d13..23ebb07b7f7 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -10824,6 +10824,7 @@ Column_definition::Column_definition(THD *thd, Field *old_field, > comment= old_field->comment; > vcol_info= old_field->vcol_info; > option_list= old_field->option_list; > + explicitly_nullable= !(old_field->flags & NOT_NULL_FLAG); This introduces asymmetry in how NULL and NOT NULL attributes are handled. I suggest considering these ways: 1. Change "bool explicitly_nullable" to "bool explicit_nullability", which will be: - false if nothing was specified - true if NULL or NOT NULL was specified. 2. Or don't store NOT_NULL_FLAG in Column_definition::flags at all. Add a new Column_definition enum member with three values for: - not specified - explicit NULL - explicit NOT NULL As discussed on slack, the latter will need some changes. But I think not that much, as only Column_definition needs changes, while Field does not seem to need at this point. > compression_method_ptr= 0; > versioning= VERSIONING_NOT_SET; > invisible= old_field->invisible; > diff --git a/sql/field.h b/sql/field.h > index 5d5be5204cd..87a30bd9f95 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -5281,7 +5281,7 @@ class Column_definition: public Sql_alloc, > uint flags, pack_length; > List<String> interval_list; > engine_option_value *option_list; > - > + bool explicitly_nullable; > > /* > This is additinal data provided for any computed(virtual) field. > @@ -5303,7 +5303,7 @@ class Column_definition: public Sql_alloc, > comment(null_clex_str), > on_update(NULL), invisible(VISIBLE), char_length(0), > flags(0), pack_length(0), > - option_list(NULL), > + option_list(NULL), explicitly_nullable(false), > vcol_info(0), default_value(0), check_constraint(0), > versioning(VERSIONING_NOT_SET), period(NULL) > { > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 3eeabe20c71..8c31f988a05 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -2316,6 +2316,8 @@ void promote_first_timestamp_column(List<Create_field> *column_definitions) > if (column_definition.is_timestamp_type() || // TIMESTAMP > column_definition.unireg_check == Field::TIMESTAMP_OLD_FIELD) // Legacy > { > + if (!column_definition.explicitly_nullable) > + column_definition.flags|= NOT_NULL_FLAG; I think this does not work well with PS. On the very first execution the field becomes NOT NULL forever, if the current OPTION_EXPLICIT_DEF_TIMESTAMP says so. > DBUG_PRINT("info", ("field-ptr:%p", column_definition.field)); > if (first && > (column_definition.flags & NOT_NULL_FLAG) != 0 && // NOT NULL, > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 44a51428280..dc0c0b1e0cf 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -4258,18 +4258,6 @@ void Type_handler_temporal_with_date::Item_update_null_value(Item *item) const > (void) item->get_date(thd, &ltime, Datetime::Options(thd)); > } > > -bool > -Type_handler_timestamp_common:: > -Column_definition_set_attributes(THD *thd, > - Column_definition *def, > - const Lex_field_type_st &attr, > - column_definition_type_t type) const > -{ > - Type_handler::Column_definition_set_attributes(thd, def, attr, type); > - if (!(thd->variables.option_bits & OPTION_EXPLICIT_DEF_TIMESTAMP)) > - def->flags|= NOT_NULL_FLAG; > - return false; > - > > void Type_handler_string_result::Item_update_null_value(Item *item) const > { > diff --git a/sql/sql_type.h b/sql/sql_type.h > index 7ff4bc64679..e5f54535ece 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -6655,11 +6655,6 @@ class Type_handler_timestamp_common: public Type_handler_temporal_with_date > bool Item_func_min_max_get_date(THD *thd, Item_func_min_max*, > MYSQL_TIME *, date_mode_t fuzzydate) > const override; > - bool Column_definition_set_attributes(THD *thd, > - Column_definition *def, > - const Lex_field_type_st &attr, > - column_definition_type_t type) > - const override; > }; > > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 5416cec49b5..f326d8c0cc5 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -5831,7 +5831,6 @@ field_def: > | opt_generated_always AS virtual_column_func > { > Lex->last_field->vcol_info= $3; > - Lex->last_field->flags&= ~NOT_NULL_FLAG; // undo automatic NOT NULL for timestamps > } > vcol_opt_specifier vcol_opt_attribute > { > @@ -6313,7 +6312,12 @@ attribute_list: > ; > > attribute: > - NULL_SYM { Lex->last_field->flags&= ~ NOT_NULL_FLAG; $$.init(); } > + NULL_SYM > + { > + Lex->last_field->flags&= ~NOT_NULL_FLAG; > + Lex->last_field->explicitly_nullable= true; > + $$.init(); > + } > | DEFAULT column_default_expr { Lex->last_field->default_value= $2; $$.init(); } > | ON UPDATE_SYM NOW_SYM opt_default_time_precision > {

3 3

[Maria-developers] review for (MDEV-28632) bugfix: DEFAULT NULL was allowed for NOT NULL columns
by Alexander Barkov 29 Jul '22

29 Jul '22

Hello Sergei, > commit 5a362d486b30fdaf3c7a360737331767154b4ee8 > Author: Sergei Golubchik <serg(a)mariadb.org> > Date: Mon Jul 18 22:53:27 2022 +0200 > > bugfix: DEFAULT NULL was allowed for NOT NULL columns > > this was detected during parsing (so NOT NULL DEFAULT NULL was an error), > but if a column was made NOT NULL later (e.g. as a part of primary key, > in mysql_prepare_create_table()), DEFAULT NULL was accepted and ignored. > > to fix this the NOT NULL DEFAULT NULL was moved into > mysql_prepare_create_table(). I'm not sure that in case of auto_increment we should really disallow DEFAULT NULL under terms of this TIMESTAMP task. Look at this script: DROP TABLE IF EXISTS t1; CREATE TABLE t1 (a INT NOT NULL AUTO_INCREMENT PRIMARY KEY); INSERT INTO t1 VALUES (NULL); Query OK, 1 row affected (0.012 sec) NULL can be inserted without errors. Yes, it's inserted with a side effect - it's replaced to the next auto increment value. But why this side effect makes NULL an invalid default? Note, I'm not against this change. Stricter behavior is good :) But I'm worried that: - It can break compatibility on one hand. - It goes without it's own MDEV on the other hand. This change deserves it's own MDEV (with motivation and consequences explained) and should be properly documented and mentioned in release notes. <cut> > index 712b8629c09..d8471894db9 100644 > --- a/mysql-test/main/view.result > +++ b/mysql-test/main/view.result > @@ -4082,12 +4082,12 @@ DROP TABLE t1,t2; > # MDEV-6251: SIGSEGV in query optimizer (in set_check_materialized > # with MERGE view) > # > -CREATE TABLE t1 (a1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); > -CREATE TABLE t2 (b1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); > -CREATE TABLE t3 (c1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); > -CREATE TABLE t4 (d1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); > -CREATE TABLE t5 (e1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); > -CREATE TABLE t6 (f1 INT(11) NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); > +CREATE TABLE t1 (a1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY); > +CREATE TABLE t2 (b1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY); > +CREATE TABLE t3 (c1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY); > +CREATE TABLE t4 (d1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY); > +CREATE TABLE t5 (e1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY); > +CREATE TABLE t6 (f1 INT(11) NOT NULL AUTO_INCREMENT PRIMARY KEY); It seems you removed all cases with the old syntax. So now it's not recorded in MTR what happens on this statement: CREATE TABLE t1 (a INT NOT NULL DEFAULT NULL AUTO_INCREMENT PRIMARY KEY); Please cover this scenario in some relevant *.test file. <cut> > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index cf0bbfb1130..3eeabe20c71 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc <cut> > @@ -3647,6 +3643,15 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info, > { > Field::utype type= (Field::utype) MTYP_TYPENR(sql_field->unireg_check); > > + if (sql_field->default_value && > + sql_field->default_value->expr->type() == Item::NULL_ITEM && > + sql_field->flags & NOT_NULL_FLAG && > + !(sql_field->flags & AUTO_INCREMENT_FLAG)) > + { > + my_error(ER_INVALID_DEFAULT, MYF(0), sql_field->field_name.str); > + DBUG_RETURN(TRUE); > + } ^^^ now obvious errors are caught only during EXECUTE, although it's already clear at the PREPARE stage that the DEFAULT is wrong: MariaDB [test]> PREPARE stmt FROM 'CREATE TABLE T1 (a INT NOT NULL DEFAULT NULL)'; Query OK, 0 rows affected (0.000 sec) Statement prepared MariaDB [test]> EXECUTE stmt; ERROR 1067 (42000): Invalid default value for 'a' I suggest we still catch non-unambiguous cases (when nothing depends on system variables) during PREPARE. Inside some existing or a new Type_handler method. Also, please add PS tests: - for unambiguous scenarios like here - for ambiguous scenarios, e.g. with TIMESTAMP columns changing OPTION_EXPLICIT_DEF_TIMESTAMP between PREPARE and EXECUTE. It seems this code needs to be changed: > /* > Set NO_DEFAULT_VALUE_FLAG if this field doesn't have a default value and > it is NOT NULL, not an AUTO_INCREMENT field, not a TIMESTAMP and not > updated trough a NOW() function. > */ > if (!sql_field->default_value && > !sql_field->has_default_function() && > (sql_field->flags & NOT_NULL_FLAG) && > (!sql_field->is_timestamp_type() || > (thd->variables.option_bits & OPTION_EXPLICIT_DEF_TIMESTAMP))&& > !sql_field->vers_sys_field()) > { > sql_field->flags|= NO_DEFAULT_VALUE_FLAG; > sql_field->pack_flag|= FIELDFLAG_NO_DEFAULT; > } It can only add NO DEFAULT related flags. Shouldn't it also remove NO DEFAULT flags if the current PS EXECUTE time option OPTION_EXPLICIT_DEF_TIMESTAMP is unset? <cut> > diff --git a/storage/spider/mysql-test/spider/bg/t/spider_fixes.test b/storage/spider/mysql-test/spider/bg/t/spider_fixes.test > index b222f494ba1..41f87e61416 100644 > --- a/storage/spider/mysql-test/spider/bg/t/spider_fixes.test > +++ b/storage/spider/mysql-test/spider/bg/t/spider_fixes.test > @@ -294,14 +294,14 @@ if ($USE_CHILD_GROUP2) > --disable_query_log > echo CREATE TABLE ta_l ( > a int(11) NOT NULL DEFAULT '0', > - b char(1) DEFAULT NULL, > - c datetime DEFAULT NULL, > + b char(1), > + c datetime, > PRIMARY KEY (a, b, c) > ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT5_2_1; > eval CREATE TABLE ta_l ( > a int(11) NOT NULL DEFAULT '0', > - b char(1) DEFAULT NULL, > - c datetime DEFAULT NULL, > + b char(1), > + c datetime, > PRIMARY KEY (a, b, c) > ) $MASTER_1_ENGINE $MASTER_1_CHARSET $MASTER_1_COMMENT5_2_1; > --enable_query_log Why this change ^^^ ? > diff --git a/storage/spider/mysql-test/spider/t/spider_fixes.test b/storage/spider/mysql-test/spider/t/spider_fixes.test > index 47bc225d614..75f108f1164 100644 > --- a/storage/spider/mysql-test/spider/t/spider_fixes.test > +++ b/storage/spider/mysql-test/spider/t/spider_fixes.test > @@ -294,14 +294,14 @@ if ($USE_CHILD_GROUP2) > --disable_query_log > echo CREATE TABLE ta_l ( > a int(11) NOT NULL DEFAULT '0', > - b char(1) DEFAULT NULL, > - c datetime DEFAULT NULL, > + b char(1), > + c datetime, > PRIMARY KEY (a, b, c) > ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT5_2_1; > eval CREATE TABLE ta_l ( > a int(11) NOT NULL DEFAULT '0', > - b char(1) DEFAULT NULL, > - c datetime DEFAULT NULL, > + b char(1), > + c datetime, > PRIMARY KEY (a, b, c) > ) $MASTER_1_ENGINE $MASTER_1_CHARSET $MASTER_1_COMMENT5_2_1; > --enable_query_log Same question.

2 2

Re: [Maria-developers] Move on with Gtid strict mode
by Kristian Nielsen 13 Jul '22

13 Jul '22

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Kristian, salve! Hi Andrei, thanks for interesting comments and discussion! >> 3. To solve (2), we recover the old master with >> --tc-heuristic-recover=rollback. This makes it rollback (=discard) any >> transaction that was not fully committed to disk (does it also roll back the >> binary log for such transactions? I don't recall this detail). >> >> 4. Because of (3), there might be a transaction that originated on the old >> master (has it's server_id), but which is now missing from that server (now >> a slave) as it was deliberately discarded. >> >> 5. To solve (4), we will use circular replication > > Well, the circularity is not necessary for (4). The ex-master just needs > to be configured as slave. Right, I see now. > Also let me call in the following this stage of receiving/accepting own > events as 'slave recovery'. But thinking more, there's another point that I don't understand, around the use of --tc-heuristic-recover=rollback. From memory, and also from reading docs on https://mariadb.com/kb/en/heuristic-recovery-with-the-transaction-coordinat…, this will forcibly roll back any prepared-but-not-committed InnoDB transaction, but _not_ roll back any corresponding binlog entry. Doesn't this leave the binlog inconsistent with the storage engine? Of the last few transactions in the binlog, some may be in InnoDB, some may not. And if so, doesn't this make the binlog useless for deciding which of our own transactions to ignore? When the old-master-now-slave gets one of its own events that's also in the binlog, we don't know if it is in the engine. If we apply it, we may end up with it double. If we do not apply it, we may be missing it. Is there some mechanims used in this feature/MDEV that handles this correctly that I'm missing? >> So isn't there a better way to achieve the underlying use case? For example, >> why not just use the new MASTER_DEMOTE_TO_SLAVE option to set the correct >> slave position from which to replicate from in the old master (now >> slave)? > > That's how it should normally do, yet it's an optimization (to save on > traffic) part of the solution. Interesting. Can you elaborate on this optimization - how does it save on traffic? > In the worst case `gtid_slave_pos` can be even empty for the > ex-master's domain, so the ex-master slave might keep receiving a bulk > of own events, only to ignore the most of them. > And that's the patch's base code logics as well. I guess there's something here that I'm not understanding. If MASTER_DEMOTE_TO_SLAVE is used to copy the gtid_binlog_pos into the gtid_slave_pos, how can the gtid_slave_pos be empty for the ex-master's domain? > ... , `gtid_ignore_duplicate = OFF` would have to go in my perception. > And if it goes, we'd remain with the only "choice" of > `gtid_ignore_duplicate = ON` :-) so the option would be gone. Hm. So are you suggesting that the current behaviour when --gtid-ignore-duplicates=ON becomes the default behaviour - in fact the only behaviour? As I remember it, the --gtid-ignore-duplicates=ON option is a very dangerous option unless the servers are managed very strictly and correctly. In fact I was expecting to find a dire warning written by myself about this, but didn't find anything in a quick web search ;-) I think the main problem is if an incorrect transaction done on a slave somewhere introduces an out-of-order GTID sequence, Then the replication will silently ignore potentially a large number of transactions that have their sequence number smaller than the incorrect out-of-order transaction. Out-of-order will normally be an error. But mistakes happen, and it is bad if this causes replication to become silently corrupt, as opposed to flagging the error (assuming --gtid-strict-mode=ON). What do you think? Again, it's cool that things are being worked on in MariaDB GTID, and I hope my comments don't come out as purely negative. While MariaDB/MySQL replication is in some ways overly complex, this is also what gives it the enormous flexibility for advanced users to do crazy^H^H^H^H^Hfantastic stuff! So there's some leeway for allowing complexity. But there are also some very delicate pitfalls that the original design tried hard to avoid, with decisions that may not always be obvious. So anything I can do there to help with history and design principles, I'm happy to assist with... - Kristian.

1 0

Re: [Maria-developers] ce5cc8fb905: MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added
by Sergei Golubchik 13 Jul '22

13 Jul '22

Hi, Nikita, This is good, but I think fill_extra_persistent_columns() shouldn't be used at all. It doesn't handle default values. I've changed rpl_alter_extra_persistent test as -alter table t1 add column z1 int as(a+1) virtual, add column z2 int as (a+2) persistent; +alter table t1 add column z1 int as(a+1) virtual, add column z2 int default (a+2); and, of course, new column z2 wasn't updated properly. An easy fix would be to use your code in all cases and remove fill_extra_persistent_columns(). Could you do that too, please? And add the test case too. May be just, append , add column z3 int default (a+2) On Jul 07, Nikita Malyavin wrote: > revision-id: ce5cc8fb905 (mariadb-10.6.1-506-gce5cc8fb905) > parent(s): 49ad875902e > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-07-04 15:55:03 +0300 > message: > > MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added > > We shouldn't rely on `fill_extra_persistent_columns`, as it only updates > fields which have an index > cols->n_bits (replication bitmap width). > > Normal update_virtual_fields+update_default_fields shoudl be done. > > diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc > index 17731e64600..42f476fc112 100644 > --- a/sql/rpl_record.cc > +++ b/sql/rpl_record.cc > @@ -412,6 +412,26 @@ int unpack_row(rpl_group_info *rgi, TABLE *table, uint const colcnt, > { > copy->do_copy(copy); > } > + if (table->default_field) > + { > + error= table->update_default_fields(table->in_use->lex->ignore); > + if (unlikely(error)) > + DBUG_RETURN(error); > + } > + if (table->vfield) > + { > + error= table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE); > + if (unlikely(error)) > + DBUG_RETURN(error); > + } > + } > + else > + { > + /* > + Add Extra slave persistent columns > + */ > + if (unlikely(error= fill_extra_persistent_columns(table, cols->n_bits))) > + DBUG_RETURN(error); > } > > /* > @@ -439,12 +459,6 @@ int unpack_row(rpl_group_info *rgi, TABLE *table, uint const colcnt, > } > } > > - /* > - Add Extra slave persistent columns > - */ > - if (unlikely(error= fill_extra_persistent_columns(table, cols->n_bits))) > - DBUG_RETURN(error); > - > /* > We should now have read all the null bytes, otherwise something is > really wrong. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 3e2d297830b: MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter with long unique
by Sergei Golubchik 13 Jul '22

13 Jul '22

Hi, Nikita, On Jul 08, Nikita Malyavin wrote: > revision-id: 3e2d297830b (mariadb-10.6.1-511-g3e2d297830b) > parent(s): c1feb8c3c32 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-07-05 15:20:04 +0300 > message: > > MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter with long unique > > 1. ER_KEY_NOT_FOUND > Some virtual columns were not updated because were not included in read_set > to the moment of calculation. > > In Row_logs_event::do_apply_event some fields are excluded based on m_cols > value, the number of replicated rows. the number of replicated *columns* > We can't rely on this. Basically, we'd be satisfied, if all columns were > just set, at least for now. > > 2. ER_LOCK_WAIT_TIMEOUT > This is a long unique specific problem. > > Sometimes, lookup_handler is created for to->file. To properly free it, > ha_reset should be called. It is usually done by calling > close_thread_table, but ALTER TABLE makes it differently. Hence, a single > ha_reset call is added to mysql_alter_table. > > Also, the lifetime of lookup_handler is corrected: it's been allocated on > thd->mem_root, which in case of online alter > is event's mem_root, and can vary other ways. > It is changed to table->mem_root, which is more appropriate: > ha_reset is called only when table is closed, or in a few additional cases, > which correspond to a statement end. So memory leaks are unlikely here. > > diff --git a/sql/handler.cc b/sql/handler.cc > index d71c0fc83a3..fbebfd27203 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -3212,7 +3212,7 @@ int handler::create_lookup_handler() > handler *tmp; > if (lookup_handler != this) > return 0; > - if (!(tmp= clone(table->s->normalized_path.str, table->in_use->mem_root))) > + if (!(tmp= clone(table->s->normalized_path.str, &table->mem_root))) that's wrong, the lifetime of a lookup handler is one statement, it's destroyed at the end. You cannot keep allocating new handlers on the table->mem_root for every statement. > return 1; > lookup_handler= tmp; > return lookup_handler->ha_external_lock(table->in_use, F_RDLCK); > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 89ca2a41937..2c19bf67586 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -6007,6 +6007,11 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > if (m_width == table->s->fields && bitmap_is_set_all(&m_cols)) > set_flags(COMPLETE_ROWS_F); > > + Rpl_table_data rpl_data{}; > + if (rgi) rgi->get_table_data(table, &rpl_data); > + > + if (!rpl_data.copy_fields) > + { > /* > Set tables write and read sets. > > @@ -6027,17 +6027,19 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > bitmap_set_all(table->read_set); > if (get_general_type_code() == DELETE_ROWS_EVENT || > get_general_type_code() == UPDATE_ROWS_EVENT) > bitmap_intersect(table->read_set,&m_cols); > > bitmap_set_all(table->write_set); > > /* WRITE ROWS EVENTS store the bitmap in m_cols instead of m_cols_ai */ > MY_BITMAP *after_image= ((get_general_type_code() == UPDATE_ROWS_EVENT) ? > &m_cols_ai : &m_cols); > bitmap_intersect(table->write_set, after_image); > > this->slave_exec_mode= slave_exec_mode_options; // fix the mode > + } I'm sorry, I don't quite understand. "some fields are excluded based on m_cols value" - what columns were excluded? What was the value of m_cols here? > table->rpl_write_set= table->write_set; > > // Do event specific preparations > error= do_before_row_operations(rli); > @@ -6045,8 +6052,6 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > extra columns on the slave. In that case, do not force > MODE_NO_AUTO_VALUE_ON_ZERO. > */ > - Rpl_table_data rpl_data{}; > - if (rgi) rgi->get_table_data(table, &rpl_data); > sql_mode_t saved_sql_mode= thd->variables.sql_mode; > if (!is_auto_inc_in_extra_columns()) > thd->variables.sql_mode= (rpl_data.copy_fields ? saved_sql_mode : 0) > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 29c4d09cd39..a3efcdd9785 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -11011,6 +11011,9 @@ do_continue:; > !(table->file->ha_table_flags() & HA_REUSES_FILE_NAMES) && > !(new_table->file->ha_table_flags() & > HA_REUSES_FILE_NAMES)); > + > + // Close lookup_handler. > + new_table->file->ha_reset(); please, put it not here, but inside the following thd->drop_temporary_table() call. It'll also fix Sanja's bug MDEV-26433. And please coordinate with him to put it on the same place so that git would detect a conflict and we could merge it to have only one reset call in that method, not two. > /* > Close the intermediate table that will be the new table, but do > not delete it! Even though MERGE tables do not have their children > @@ -11854,8 +11857,11 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to, > > // We'll be filling from->record[0] from row events > bitmap_set_all(from->write_set); > - // We restore bitmaps, because update event is going to mess up with them. > + // Use all columns. UPDATE/DELETE events reset read_set and write_set to > + // def_*_set after each row operation, so using all_set won't work. > to->default_column_bitmaps(); > + bitmap_set_all(&to->def_read_set); > + bitmap_set_all(&to->def_write_set); > > end_read_record(&info); > init_read_record_done= false; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 73c4fd41afc: Do not ignore sql_mode when replicating
by Sergei Golubchik 13 Jul '22

13 Jul '22

Hi, Nikita, On Jul 07, Nikita Malyavin wrote: > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index aba846453e9..89ca2a41937 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -6045,9 +6045,12 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > extra columns on the slave. In that case, do not force > MODE_NO_AUTO_VALUE_ON_ZERO. > */ > + Rpl_table_data rpl_data{}; > + if (rgi) rgi->get_table_data(table, &rpl_data); > sql_mode_t saved_sql_mode= thd->variables.sql_mode; > if (!is_auto_inc_in_extra_columns()) > - thd->variables.sql_mode= MODE_NO_AUTO_VALUE_ON_ZERO; > + thd->variables.sql_mode= (rpl_data.copy_fields ? saved_sql_mode : 0) you start using this condition in many places and it's quite cryptic for those, who don't know the code. I'd suggest to add method, like struct Rpl_table_data { ... bool is_online_alter() { return copy_fields != NULL; } ... }; btw, it fits very well in your Rpl_table_data simplification. > + | MODE_NO_AUTO_VALUE_ON_ZERO; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] Move on with Gtid strict mode
by Kristian Nielsen 11 Jul '22

11 Jul '22

Hi Andrei, Good to hear from you, and to see that things are still going on with MariaDB GTID replication! I tried looking at the MDEVs that you referred to. Here is how I understand the motivation for this, correct me if I'm wrong: 1. We're considering a slave with "lossless semi-sync slave", where phantom reads are avoided by delaying storage engine commit until at least one slave has acknowledged a transaction. 2. If a master crashes, and comes back up after recovery, we want to prevent it from having any extra transactions that are not replicated to the rest of the replication servers (one of which will have been promoted as a new master). Such a transaction would prevent demoting the old master as a slave, as it would contain a rouge transaction not known to the rest of the replication topology. 3. To solve (2), we recover the old master with --tc-heuristic-recover=rollback. This makes it rollback (=discard) any transaction that was not fully committed to disk (does it also roll back the binary log for such transactions? I don't recall this detail). 4. Because of (3), there might be a transaction that originated on the old master (has it's server_id), but which is now missing from that server (now a slave) as it was deliberately discarded. 5. To solve (4), we will use circular replication so that the missing transaction gets replicated back to the old master (now slave). And we want to have a slave replicate its own transactions (replicate_same_sever_id), which is normally not done. And we want to use the server's binlog to know which transactions to replicate and which to ignore. Is that correctly understood? I must admit, this sounds horribly complex to me. So my question would be, do we really need to do this to solve the fundamental use case? In my belief, the main drawback with MariaDB replication and GTID is the complexity of especially the more advanced setups, which is barely feasible for even a skilled DBA to fully understand. Does this come from trying to port over some specific MySQL replication/GTID feature to MariaDB GTID? One important point is that MariaDB GTID was originally designed to keep the concept of master/binlog and slave/relaylog cleanly separated. A master transaction needs only be written to the binlog, there is no need to update any replication state (=overhead). Similarly, a slave transaction needs only be replicated to the storage engine, there is no need to write it to the binlog (=overhead). IIRC, MySQL (GTID) replication required the binlog (and --log-slave-updates) to be enabled on a slave) This is one of the improvements in the design of MariaDB GTID. So I don't particularly like the idea of now involving the binlog in how a slave replicates transactions. On the contrary, the MASTER_USE_GTID=current_pos feature is one thing I regret from the original GTID design, precisely because it violates the principle. And I was happy to see work to deprecate this, which I think is the prefered direction. So isn't there a better way to achieve the underlying use case? For example, why not just use the new MASTER_DEMOTE_TO_SLAVE option to set the correct slave position from which to replicate from in the old master (now slave)? And then use the existing --gtid-ignore-duplicate option? Since this is GTID-based circular replication, there must already be configured a separate gtid_domain_id for each master? Then you suggest to change the current --gtid-strict-mode and --gtid-ignore-duplicate options to a single option. I feel less strongly about this change; as you say, --gtid-strict-mode=OFF + --gtid-ignore-duplicates=ON may not make much sense. Still, I wonder what is the the need for such a change? This would be a not-backwards-compatible change, with little real benefit, as the semantics would be the same? Also, the --gtid-strict-mode option is an option that is highly recommended for any GTID use, and which should preferably be as simple as possible to understand to be able to be widely used by DBAs. On the contrary --gtid-ignore-duplicates is a very complex option only useful for very complex replication topologies, and with many subtle pitfalls that is very hard to fully understand. So there is real benefit to keeping them separate. As to the --gtid-strict-mode=OFF. The main original motivation for this option (and its default value of "OFF") was to be able to enable GTID by default, for any server installation upgraded (to was it 10.0 that introduced GTID? Long time back!). This is another big improvement (so I think) in MariaDB GTID over MySQL, that GTID is available by default, and compatible with any replication topology that works with old-style MySQL replication. Is it really so that no-one is using old-style, non-GTID replication any more, with all its sloppy setups possible, Sure, then non-strict GTID mode can be deprecated, I suppose - but it seems doubtful that this would be the case for *all* users? I hope I managed to explain where I didn't fully understand the motivation and need for some of these proposals? I'd be very interested to understand better any underlying necessities that I didn't catch from your mail, and maybe refine my comments. And once again, great to see the work still going on with MariaDB replication and GTID! Hope this helps, - Kristian. Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Two points of concern of this email deal with observations around GTID strict mode. > > > 1. About gtid_ignore_duplicates > > Introduced by > > hash: 2c2478b82260 > author: unknown > date: 2014-03-09 10:27:38 > > MDEV-5804: If same GTID is ... > Before, the arrival of same GTID twice in multi-source replication > would cause double-apply or in gtid strict mode an error. > > with a clear motivation to overcome the gtid strict mode error policy. > That is `gtid_ignore_duplicates = ON' softens `gtid_strict_mode = ON' > to ignore a duplicate gtid without a slave error/interruption. > As `gtid_strict_mode = OFF', `gtid_ignore_duplicates = ON' combination is > hardly ever relevant/practical, the `gtid_ignore_duplicates = ON' then can be fairly > perceived as `gtid_strict_mode=ON' 's "mild" submode to suggest > enum `gtid_stict_mode' { OFF, ON, "ON_BUT_IGNORE_DUPLICATES_RATHER_THAT_ERROR" } > which would obsolete `gtid_ignore_duplicates'. > > Why I'd prefer this change actually also deals with the circular replication > that - MDEV-28609 - forces us to do searching for duplicates > in the slave's binlog. > MDEV-28609 presents a case where a same server-id transaction received > by a semisync slave requires a more sophisticated decision making than > before (the same server-id based) for weather it to be ignored or > accepted. > > The accept policy of even own server-id gtids (transactions) is necessary for > MDEV-21117 semisync slave recovery, and the legacy > `replicate_same_sever_id' filtering can't help being too coarse as we need to consider > the gtid seq_no. > So under those conditions the same server-id transaction is made to be accepted > when it does not exist in the slave's binlog. > Otherwise, when it does exist in binlog, it'd be (like) a duplicate, arriving through > a circular topology from a "source" of the slave local binlog, only > to be ignored, like the same server-id gtids normally are. > The ignore policy in this case corresponds to the proposed > `gtid_stict_mode = "ON_BUT_IGNORE..."'. > > It should be naturally default as it inherits its pre-gtid equivalent of > `replicate_same_sever_id = NO' > and is safe (the slave's gtid state) for a real multi-source configuration as well. > > `gtid_stict_mode = ON' still would make sense as a method to > catch misconfigurations of the likes of Change-Master's do,ignore- domain_id ones. > > Your take? > > 2. But it's not clear what `gtid_strict_mode = OFF' is for. > > The ON mode is pretty flexible allowing gtid seq_no holes that is setting the gtid state > to any greater than the current (logical timestamp) value. > To set the gtid "clock" backward is still possible though requires slave restart. > > So how would you comment on an idea to deprecate OFF? > > Cheers, > > Andrei

1 0

Re: [Maria-developers] [Commits] 78370ea: MDEV-28965 Assertion failure when preparing UPDATE with derived table in WHERE
by Oleksandr Byelkin 11 Jul '22

11 Jul '22

Hi, Igor! Generally all looks good, but see my notes and couple of questions below. On Thu, Jul 7, 2022 at 6:57 AM IgorBabaev <igor(a)mariadb.com> wrote: > revision-id: 78370ea0fd21f66a4084d488ddcebac541249241 > (mariadb-10.6.1-477-g78370ea) > parent(s): 2db18fdb3d68d906fbd188ec570a64502ba55849 > author: Igor Babaev > committer: Igor Babaev > timestamp: 2022-07-06 21:57:23 -0700 > message: > > MDEV-28965 Assertion failure when preparing UPDATE with derived table in > WHERE > > This patch fixes not only the assertion failure in the function > Field_iterator_table_ref::set_field_iterator() but also: > - fixes the problem of forced materialization of derived tables used > in subqueries contained in WHERE clauses of single-table and multi-table > UPDATE and DELETE statements > - fixes the problem of MDEV-17954 that prevented execution of multi-table > DELETE statements if they use in their WHERE clauses references to > the tables that are updated. > > The patch must be considered a complement to the patch for MDEV-28883. > > --- > mysql-test/main/delete_use_source.result | 184 ++++++++++- > mysql-test/main/delete_use_source.test | 120 ++++++++ > mysql-test/main/derived.result | 336 > +++++++++++++++++++++ > mysql-test/main/derived.test | 210 +++++++++++++ > mysql-test/main/derived_cond_pushdown.result | 27 +- > mysql-test/main/derived_cond_pushdown.test | 2 +- > mysql-test/main/multi_update.result | 2 - > mysql-test/main/multi_update.test | 2 - > mysql-test/main/subselect.result | 1 - > mysql-test/main/subselect.test | 1 - > mysql-test/main/subselect_no_exists_to_in.result | 1 - > mysql-test/main/subselect_no_mat.result | 1 - > mysql-test/main/subselect_no_opts.result | 1 - > mysql-test/main/subselect_no_scache.result | 1 - > mysql-test/main/subselect_no_semijoin.result | 1 - > .../engines/iuds/r/update_delete_number.result | 22 +- > .../suite/engines/iuds/t/update_delete_number.test | 20 +- > sql/opt_subselect.cc | 56 +++- > sql/sql_base.cc | 41 ++- > sql/sql_delete.cc | 63 ++-- > sql/sql_delete.h | 14 +- > sql/sql_lex.cc | 28 +- > sql/sql_lex.h | 1 + > sql/sql_update.cc | 21 +- > sql/sql_update.h | 16 +- > sql/sql_yacc.yy | 13 + > sql/table.cc | 10 +- > 27 files changed, 1106 insertions(+), 89 deletions(-) > > [skip] > > --- a/mysql-test/suite/engines/iuds/t/update_delete_number.test > +++ b/mysql-test/suite/engines/iuds/t/update_delete_number.test > @@ -285,8 +285,18 @@ SELECT * FROM t1,t2 WHERE t2.c1=t1.c2; > DELETE FROM a1, a2 USING t1 AS a1 INNER JOIN t2 AS a2 WHERE a2.c1=a1.c2; > --sorted_result > SELECT * FROM t1,t2 WHERE t2.c1=t1.c2; > ---error ER_UPDATE_TABLE_USED > -DELETE FROM t1,t2 using t1,t2 where t1.c1=(select c1 from t1); > +TRUNCATE TABLE t1; > +TRUNCATE TABLE t2; > +INSERT INTO t1 VALUES(254,127,1),(0,-128,2),(1,127,3),(3,NULL,5); > +INSERT INTO t2 VALUES(127,255,1),(127,1,2),(-128,0,3),(-1,NULL,5); > +# After the patch for MDEV-28883 this should not report > +# --error ER_UPDATE_TABLE_USED anymore > Why is the above left as a comment? (the same in other cases) [skip] > diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc > index fa338f0..ac47566 100644 > --- a/sql/opt_subselect.cc > +++ b/sql/opt_subselect.cc > @@ -30,6 +30,8 @@ > #include "sql_base.h" > #include "sql_const.h" > #include "sql_select.h" > +#include "sql_update.h" > +#include "sql_delete.h" > it would be nice to have comment why you include it as it only for one function as I can see, something like: #include "sql_update.h" // processing_as_multitable_update_prohibited (the same for delete and for including in #include "sql_update.h" & "sql_base.cc") #include "filesort.h" > #include "opt_subselect.h" > #include "sql_test.h" > @@ -532,6 +534,48 @@ bool is_materialization_applicable(THD *thd, > Item_in_subselect *in_subs, > return FALSE; > } > > [skip] > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 2b41b78..447573b 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -47,6 +47,8 @@ > #include "sql_prepare.h" > #include "sql_statistics.h" > #include "sql_cte.h" > +#include "sql_update.h" > +#include "sql_delete.h" > see above > #include <m_ctype.h> > #include <my_dir.h> > #include <hash.h> > @@ -1164,7 +1166,7 @@ TABLE_LIST* find_dup_table(THD *thd, TABLE_LIST > *table, TABLE_LIST *table_list, > /* > If we found entry of this table or table of SELECT which already > processed in derived table or top select of > multi-update/multi-delete > - (exclude_from_table_unique_test) or prelocking placeholder. > + (exclude_from_table_unique_test) or prelocking placeholder. > Please fix formatting above > */ > DBUG_PRINT("info", > ("found same copy of table or table which we should > skip")); > @@ -1175,16 +1177,43 @@ TABLE_LIST* find_dup_table(THD *thd, TABLE_LIST > *table, TABLE_LIST *table_list, > We come here for queries of type: > INSERT INTO t1 (SELECT tmp.a FROM (select * FROM t1) as tmp); > > - Try to fix by materializing the derived table > + Try to fix by materializing the derived table if one can't do > without it. > */ > TABLE_LIST *derived= res->belong_to_derived; > if (derived->is_merged_derived() && !derived->derived->is_excluded()) > { > - DBUG_PRINT("info", > + bool materialize= true; > + if (thd->lex->sql_command == SQLCOM_UPDATE) > + { > + Sql_cmd_update *cmd= (Sql_cmd_update *) (thd->lex->m_sql_cmd); > + if (cmd->is_multitable()) > + materialize= false; > + else if (!cmd->processing_as_multitable_update_prohibited(thd)) > + { > + cmd->set_as_multitable(); > + materialize= false; > + } > + } > + else if (thd->lex->sql_command == SQLCOM_DELETE) > + { > + Sql_cmd_delete *cmd= (Sql_cmd_delete *) (thd->lex->m_sql_cmd); > + if (cmd->is_multitable()) > + materialize= false; > + if (!cmd->processing_as_multitable_delete_prohibited(thd)) > + { > + cmd->set_as_multitable(); > + materialize= false; > + } > + } > + if (materialize) > + { > + DBUG_PRINT("info", > ("convert merged to materialization to resolve the > conflict")); > - derived->change_refs_to_fields(); > - derived->set_materialized_derived(); > - goto retry; > + derived->change_refs_to_fields(); > + derived->set_materialized_derived(); > + // derived->field_translation= 0; > above comment has no big sens, looks not cleaned up some chenges? > + goto retry; > + } > } > } > DBUG_RETURN(res); > > [skip] > diff --git a/sql/sql_delete.h b/sql/sql_delete.h > index e1d5044..ffb8173 100644 > --- a/sql/sql_delete.h > +++ b/sql/sql_delete.h > @@ -44,11 +44,12 @@ class Sql_cmd_delete final : public Sql_cmd_dml > { > public: > Sql_cmd_delete(bool multitable_arg) > - : multitable(multitable_arg), save_protocol(NULL) {} > + : orig_multitable(multitable_arg), save_protocol(NULL) > + { multitable= orig_multitable; } > why do not use the same initializers? : multitable(multitable_arg), rig_multitable(multitable_arg), save_protocol(NULL) {} > enum_sql_command sql_command_code() const override > { > - return multitable ? SQLCOM_DELETE_MULTI : SQLCOM_DELETE; > + return orig_multitable ? SQLCOM_DELETE_MULTI : SQLCOM_DELETE; > } > > DML_prelocking_strategy *get_dml_prelocking_strategy() > > [skip] > diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc > index 78c067a..4059c0e 100644 > --- a/sql/sql_lex.cc > +++ b/sql/sql_lex.cc > @@ -37,6 +37,8 @@ > #include "sql_partition.h" > #include "sql_partition_admin.h" // > Sql_cmd_alter_table_*_part > #include "event_parse_data.h" > +#include "sql_update.h" > +#include "sql_delete.h" > see above > > void LEX::parse_error(uint err_number) > { > @@ -4037,9 +4039,8 @@ bool LEX::can_use_merged() > SYNOPSIS > LEX::can_not_use_merged() > > - @param no_update_or_delete Set to 1 if we can't use merge with > multiple-table > - updates, like when used from > - TALE_LIST::init_derived() > + @param forced_no_merge_for_update_delete Set to 1 if we can't use merge > with > + multiple-table updates/deletes > > DESCRIPTION > Temporary table algorithm will be used on all SELECT levels for > queries > @@ -4050,7 +4051,7 @@ bool LEX::can_use_merged() > TRUE - VIEWs with MERGE algorithms can be used > */ > > -bool LEX::can_not_use_merged(bool no_update_or_delete) > +bool LEX::can_not_use_merged(bool forced_no_merge_for_update_delete) > { > switch (sql_command) { > case SQLCOM_CREATE_VIEW: > @@ -4064,18 +4065,29 @@ bool LEX::can_not_use_merged(bool > no_update_or_delete) > return TRUE; > > case SQLCOM_UPDATE_MULTI: > - case SQLCOM_DELETE_MULTI: > - if (no_update_or_delete) > + if (forced_no_merge_for_update_delete) > return TRUE; > /* Fall through */ > > case SQLCOM_UPDATE: > - if (no_update_or_delete && m_sql_cmd && > - (m_sql_cmd->sql_command_code() == SQLCOM_UPDATE_MULTI || > + if (forced_no_merge_for_update_delete && > + (((Sql_cmd_update *) m_sql_cmd)->is_multitable() || > query_tables->is_multitable())) > return TRUE; > + return FALSE; > + > + case SQLCOM_DELETE_MULTI: > + if (forced_no_merge_for_update_delete) > + return TRUE; > /* Fall through */ > > + case SQLCOM_DELETE: > + if (forced_no_merge_for_update_delete && > + (((Sql_cmd_delete *) m_sql_cmd)->is_multitable() || > + query_tables->is_multitable())) > + return TRUE; > + return FALSE; > + > default: > return FALSE; > } > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index e733b4f..402e3bd 100644 > > [skip] > diff --git a/sql/sql_update.cc b/sql/sql_update.cc > index 6b14c4f..7769777 100644 > --- a/sql/sql_update.cc > +++ b/sql/sql_update.cc > @@ -2807,6 +2807,23 @@ bool multi_update::send_eof() > > > /** > + @brief Check whether conversion to multi-table update is prohibited > + > + @param thd global context the processed statement > + @returns true if conversion is prohibited, false otherwise > + > + @todo > + Introduce handler level flag for storage engines that would prohibit > + such conversion for any single-table update. > +*/ > + > +bool Sql_cmd_update::processing_as_multitable_update_prohibited(THD *thd) > +{ > + return false; > +} > + > + > +/** > @brief Perform precheck of table privileges for update statements > > @param thd global context the processed statement > @@ -2889,7 +2906,9 @@ bool Sql_cmd_update::prepare_inner(THD *thd) > "updating and querying the same temporal periods table"); > DBUG_RETURN(TRUE); > } > - multitable= true; > + if (!table_list->is_multitable() && > + !processing_as_multitable_update_prohibited(thd)) > + multitable= true; > } > } > > diff --git a/sql/sql_update.h b/sql/sql_update.h > index d0fc7cb..4aff77a 100644 > --- a/sql/sql_update.h > +++ b/sql/sql_update.h > @@ -46,12 +46,12 @@ class Sql_cmd_update final : public Sql_cmd_dml > { > public: > Sql_cmd_update(bool multitable_arg) > - : multitable(multitable_arg) > - { } > + : orig_multitable(multitable_arg) > + { multitable= orig_multitable; } > why do not use construction for both? Sql_cmd_update(bool multitable_arg) : multitable(multitable_arg) : orig_multitable(multitable_arg) {} > > enum_sql_command sql_command_code() const override > { > - return multitable ? SQLCOM_UPDATE_MULTI : SQLCOM_UPDATE; > + return orig_multitable ? SQLCOM_UPDATE_MULTI : SQLCOM_UPDATE; > } > > DML_prelocking_strategy *get_dml_prelocking_strategy() > @@ -59,6 +59,12 @@ class Sql_cmd_update final : public Sql_cmd_dml > return &multiupdate_prelocking_strategy; > } > > + bool processing_as_multitable_update_prohibited(THD *thd); > + > + bool is_multitable() { return multitable; } > + > + void set_as_multitable() { multitable= true; } > + > protected: > /** > @brief Perform precheck of table privileges for update statements > @@ -89,13 +95,15 @@ class Sql_cmd_update final : public Sql_cmd_dml > */ > bool multitable; > > + /* Original value of the 'multitable' flag set by constructor */ > + const bool orig_multitable; > + > /* The prelocking strategy used when opening the used tables */ > Multiupdate_prelocking_strategy multiupdate_prelocking_strategy; > > public: > /* The list of the updating expressions used in the set clause */ > List<Item> *update_value_list; > - > }; > > #endif /* SQL_UPDATE_INCLUDED */ > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index a587a37..13dc602 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -13370,8 +13370,21 @@ delete_single_table: > YYPS->m_lock_type, > YYPS->m_mdl_type, > NULL, > + 0))) > + MYSQL_YYABORT; > + Select->table_list.save_and_clear(&Lex->auxiliary_table_list); > + Lex->table_count= 1; > Why it is not enough to have this counter (table_count) set by open_tables? May be it would be better to use mysql_init_select() to set everything needed by SELECT for sure? > + Lex->query_tables= 0; > + Lex->query_tables_last= &Lex->query_tables; > + if (unlikely(!Select-> > + add_table_to_list(thd, $2, NULL, > TL_OPTION_UPDATING, > + YYPS->m_lock_type, > + YYPS->m_mdl_type, > + NULL, > $3))) > MYSQL_YYABORT; > + Lex->auxiliary_table_list.first->correspondent_table= > + Lex->query_tables; > YYPS->m_lock_type= TL_READ_DEFAULT; > YYPS->m_mdl_type= MDL_SHARED_READ; > } > [skip]

1 0

[Maria-developers] Account locked and USERS view?
by Martijn Tonies (Upscene Productions) 06 Jul '22

06 Jul '22

Hi, Any reason why the ‘account locked’ attribute isn’t included in the USERS view? https://mariadb.com/kb/en/account-locking/ It could be added like this: if(json_value(mysql.global_priv.Priv, '$.account_locked') = 1, 'Y', 'N') AS is_locked With regards, Martijn Tonies Upscene Productions https://www.upscene.com Database Workbench - developer tool for Oracle, SQL Server, PostgreSQL, MySQL, InterBase, NexusDB and Firebird.

1 0

Re: [Maria-developers] MDEV-28881 Server crashes in Dep_analysis_context::create_table_value
by Sergey Petrunia 05 Jul '22

05 Jul '22

Hi Oleg, > commit 812f8cbfb3a7a02705f65d8bb7e30ae2faf13b5d > Author: Oleg Smirnov <olernov(a)gmail.com> > Date: Tue Jun 21 16:20:17 2022 +0700 > > MDEV-28881 Server crashes in Dep_analysis_context::create_table_value > > SELECT_LEX::first_select()->join may be NULL for some scenarios. > This commit adds a check for this. Looking into Jira I can infer that the patch fixes two issues, but the commit comment doesn't say that. Please avoid doing this in the future. Please split the commit into two: - first one fixing the SELECT_LEX::first_select()->join == NULL part - the second fixing the memory management. For the second part, I think it's not a good approach when most of the Table Elimination data resides in a MEM_ROOT with exception of std::set which keeps its data on the heap. I was wondering if there's a way to get STL containers to reside on MEM_ROOT as well, and found that yes there is, and MySQL actually already has it: https://dev.mysql.com/worklog/task/?id=6074 https://github.com/mysql/mysql-server/blob/8.0/sql/mem_root_allocator.h Can we copy mem_root_allocator.h from them (in a separate commit) and then fix memory management here by making std::set use it? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] 9ec0b36ee8b: MDEV-28838 password_reuse_check plugin mixes username and password
by Sergei Golubchik 04 Jul '22

04 Jul '22

Hi, Oleksandr, On Jul 04, Oleksandr Byelkin wrote: > revision-id: 9ec0b36ee8b (mariadb-10.7.4-27-g9ec0b36ee8b) > parent(s): ac0af4ec4ab > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-06-29 17:25:44 +0200 > message: > > MDEV-28838 password_reuse_check plugin mixes username and password > > To prevent the problem of mixing user name and password and > host name and user name we add length of the hostname and user name > to the hash. > > diff --git a/plugin/password_reuse_check/password_reuse_check.c b/plugin/password_reuse_check/password_reuse_check.c > index ff0364ce007..4d07cda878a 100644 > --- a/plugin/password_reuse_check/password_reuse_check.c > +++ b/plugin/password_reuse_check/password_reuse_check.c > @@ -165,9 +166,11 @@ static int validate(const MYSQL_CONST_LEX_STRING *username, > return 1; > } > > - memcpy(buff, hostname->str, hostname->length); > - memcpy(buff + hostname->length, username->str, username->length); > - memcpy(buff + hostname->length + username->length, password->str, > + int2store(buff, hostname->length); > + int2store(buff + 2, username->length); > + memcpy(buff + 4, hostname->str, hostname->length); > + memcpy(buff + 4 + hostname->length, username->str, username->length); > + memcpy(buff + 4 + hostname->length + username->length, password->str, > password->length); why? let's use the standard length,string format, why risk inventing something unusual here for no benefits at all? I'd do like: static char *store_str(char *to, MYSQL_CONST_LEX_STRING from) { int2store(to, from.length); memcpy(to+2, from.str, from.length); return to+2+from.length; } and buff= store_str(buff, hostname); buff= store_str(buff, username); buff= store_str(buff, password); > buff[key_len]= 0; > memset(hash, 0, sizeof(hash)); > @@ -235,7 +238,7 @@ maria_declare_plugin(password_reuse_check) > 0x0100, ^^^ this should be 0x0101 (or 0x0200 as I suggest below) > NULL, > sysvars, > - "1.0", > + "1.1", make it 2.0 please, this is an incompatible change > MariaDB_PLUGIN_MATURITY_GAMMA > } > maria_declare_plugin_end; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e9293e31ec6: MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE
by Sergei Golubchik 30 Jun '22

30 Jun '22

Hi, Nikita, This is very good. Just one comment, see below: On Jun 29, Nikita Malyavin wrote: > revision-id: e9293e31ec6 (mariadb-10.6.1-499-ge9293e31ec6) > parent(s): 6cfcb36d027 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-06-29 20:16:19 +0300 > message: > > MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE > > ALTER ONLINE TABLE acquires table with TL_READ. Myisam normally acquires > TL_WRITE for DML, which makes it hang until table is freed. > > We deadlock once ALTER upgrades its MDL lock. > > Solution: > Unlock table earlier. We don't need to hold TL_READ once we finished > copying. Relay log replication requires no data locks on `from` table. > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 425de469a7f..90f6283d9df 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -5734,7 +5734,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) > used in the transaction and proceed with execution of the actual > event. > */ > - if (!thd->lock) > + if (!thd->lock && rgi->tables_to_lock_count) > { > /* > Lock_tables() reads the contents of thd->lex, so they must be Instead of adding a second condition to the execution path for every row event in the replication, consider this: - if (!thd->lock) + if (!thd->open_tables) this will work identically for replication, I hope, but will be always false in your case, as you still have the table open. I mean, that's the idea. I run the rpl suite with it, but haven't done any further testing, so not completely sure it'll work. When making this change, don't forget to update the comment right above this if(). Ok to push into bb-10.10-MDEV-16329 after that. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

[Maria-developers] How to know when a FUNCTION is AGGREGATE?
by Martijn Tonies (Upscene Productions) 17 Jun '22

17 Jun '22

Hi all, Since MariaDB 10.3, you can use an AGGREGATE stored function. https://mariadb.com/kb/en/stored-aggregate-functions/ When examining the meta data, how does one distinguish between aggregate and normal stored functions? Can’t find anything in the information_schema about this. With regards, Martijn Tonies Upscene Productions https://www.upscene.com Database Workbench - developer tool for Oracle, SQL Server, PostgreSQL, MySQL, InterBase, NexusDB and Firebird.

3 3