August 2022 - developers - lists.mariadb.org

Re: [Maria-developers] e2f8dffc056: MDEV-29069 ER_KEY_NOT_FOUND on online autoinc addition + concurrent DELETE
by Sergei Golubchik 03 Oct '22

03 Oct '22

Hi, Nikita, Few comments below On Aug 27, Nikita Malyavin wrote: > commit e2f8dffc056 > Author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > Date: Mon Jul 25 23:52:24 2022 +0300 > > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index 422d496d5c3..ca17c5c07a8 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -7949,14 +7952,35 @@ uint8 Write_rows_log_event::get_trg_event_map() > **************************************************************************/ > > #if defined(HAVE_REPLICATION) > -/* > - Compares table->record[0] and table->record[1] > +/** > + @brief Compares table->record[0] and table->record[1] > + > + @param masrter_columns a number of columns on the source replica, > + 0 if ONLINE ALTER TABLE > > - Returns TRUE if different. > + @returns true if different. > */ > -static bool record_compare(TABLE *table) > +static bool record_compare(TABLE *table, uint master_columns) > { > - bool result= FALSE; > + bool result= false; > + > + /* > + Determine if the optimized check is possible (and we can > + goto record_compare_exit). > + We should ensure that all extra columns (i.e. fieldnr > master_columns) > + have explicit values. If not, we will exclude them from comparison, > + as they can contain non-deterministic values. > + > + master_columns == 0 case is optimization for ONLINE ALTER to check > + all columns fast. > + */ > + bool all_values_set= master_columns == 0 > + && bitmap_is_set_all(&table->has_value_set); > + for (uint i = master_columns; all_values_set && i < table->s->fields; i++) > + { > + all_values_set= bitmap_is_set(&table->has_value_set, i); > + } it'd be safer and faster to do all_values_set= bitmap_is_set_all(&table->has_value_set); and, please, add replication (not online alter) tests: 1. slave has more columns 2. binlog row format is "minimal", this might trigger it too > + > /** > Compare full record only if: > - there are no blob fields (otherwise we would also need > @@ -7969,47 +7993,68 @@ static bool record_compare(TABLE *table) > */ > if ((table->s->blob_fields + > table->s->varchar_fields + > - table->s->null_fields) == 0) > + table->s->null_fields) == 0 > + && all_values_set) > { > result= cmp_record(table,record[1]); > goto record_compare_exit; > } > > /* Compare null bits */ > - if (memcmp(table->null_flags, > + if (all_values_set && memcmp(table->null_flags, > table->null_flags+table->s->rec_buff_length, > table->s->null_bytes)) > { > - result= TRUE; // Diff in NULL value > + result= true; // Diff in NULL value > goto record_compare_exit; > } > > /* Compare fields */ > for (Field **ptr=table->field ; *ptr ; ptr++) > { > + Field *f= *ptr; > if (table->versioned() && (*ptr)->vers_sys_field()) > { > continue; > } > - /** > - We only compare field contents that are not null. > - NULL fields (i.e., their null bits) were compared > - earlier. > + /* > + We only compare fields that exist on the source (or in ONLINE > + ALTER case, that were in the original table). For reference, > + replica tables can also contain extra fields. > */ > - if (!(*(ptr))->is_null()) > - { > - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length)) > + if (f->field_index > master_columns && !f->has_explicit_value()) > + continue; why do you check for f->field_index > master_columns? isn't !f->has_explicit_value() the one that decided? in what case will be ok to compare values when f->field_index < master_columns but no explicit value was set? > + > + if (f->is_null() != f->is_null(table->s->rec_buff_length) > + || f->cmp_binary_offset(table->s->rec_buff_length)) > { > - result= TRUE; > + result= true; > goto record_compare_exit; > } > } > - } > > record_compare_exit: > return result; > } > > +/** > + Newly added fields with non-deterministic defaults (i.e. DEFAULT(RANDOM()), > + CURRENT_TIMESTAMP, AUTO_INCREMENT) should be excluded from key search. > + Basically we exclude all the default-filled fields based on > + has_explicit_value bitmap. > + */ > +bool Rows_log_event::key_suits_event(const KEY *key) const > +{ > + uint master_columns= m_online_alter ? 0 : m_cols.n_bits; > + for (uint p= 0; p < key->ext_key_parts; p++) > + { > + uint field_idx= key->key_part[p].fieldnr - 1; > + if (field_idx >= master_columns > + && !bitmap_is_set(&m_table->has_value_set, field_idx)) > + return false; > + } > + return true; > +} > > /** > Find the best key to use when locating the row in @c find_row(). > @@ -8030,6 +8075,18 @@ int Rows_log_event::find_key() > DBUG_ENTER("Rows_log_event::find_key"); > DBUG_ASSERT(m_table); > > + #ifdef DBUG_TRACE > + auto _ = make_scope_exit([this]() { > + DBUG_EXECUTE_IF("rpl_report_chosen_key", > + push_warning_printf(m_table->in_use, > + Sql_condition::WARN_LEVEL_NOTE, > + ER_UNKNOWN_ERROR, "Key chosen: %d", > + m_key_nr == MAX_KEY ? > + -1 : m_key_nr);); > + }); > + #endif > + > + m_key_nr= MAX_KEY; it would've been simpler and shorter to do it the old style, with a goto > best_key_nr= MAX_KEY; > > /* > @@ -8041,6 +8098,8 @@ int Rows_log_event::find_key() > { > if (!m_table->s->keys_in_use.is_set(i)) > continue; > + if (!key_suits_event(key)) > + continue; 1. again, add a replication test, please. when, e.g., slave has more columns and a unique index over new columns. 2. going over all keyparts for every event is rather redundant. you can calculate the set of usable keys once and put it, for example, in m_table->keys_in_use_for_query. > /* > We cannot use a unique key with NULL-able columns to uniquely identify > a row (but we can still select it for range scan below if nothing better > @@ -8221,7 +8280,8 @@ int Rows_log_event::find_row(rpl_group_info *rgi) > DBUG_DUMP("record[0]", table->record[0], table->s->reclength); > > if ((table->file->ha_table_flags() & HA_PRIMARY_KEY_REQUIRED_FOR_POSITION) && > - table->s->primary_key < MAX_KEY) > + table->s->primary_key < MAX_KEY && > + m_key_nr == table->s->primary_key) add a replication test for that: slave has an extra column which is a PK, and HA_PRIMARY_KEY_REQUIRED_FOR_POSITION is set (InnoDB) > { > /* > Use a more efficient method to fetch the record given by > @@ -8494,12 +8554,18 @@ Delete_rows_log_event::do_before_row_operations(const Slave_reporting_capability > if (get_flags(STMT_END_F)) > status_var_increment(thd->status_var.com_stat[SQLCOM_DELETE]); > > + const uchar *curr_row_end= m_curr_row_end; > + unpack_row(rgi, m_table, m_width, m_curr_row, &m_cols, > + &curr_row_end, &m_master_reclength, m_rows_end); > + > if ((m_table->file->ha_table_flags() & HA_PRIMARY_KEY_REQUIRED_FOR_POSITION) && > - m_table->s->primary_key < MAX_KEY) > + m_table->s->primary_key < MAX_KEY > + && key_suits_event(&m_table->key_info[m_table->s->primary_key])) > { > /* > We don't need to allocate any memory for m_key since it is not used. > */ > + m_key_nr= m_table->s->primary_key; > return 0; > } This is very strange if(). 1. it's only for delete, not for update 2. it's before invoking triggers (bug) 3. it's wrong if the master table has a different primary key (bug) I'd suggest to remove it completely. An optimization of avoiding a malloc can be done inside find_key() after the best key is found. > if (do_invoke_trigger()) Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 349283c5e7a: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
by Sergei Golubchik 27 Sep '22

27 Sep '22

Hi, Oleksandr, On Jan 27, Oleksandr Byelkin wrote: > commit 349283c5e7a > Author: Oleksandr Byelkin <sanja(a)mariadb.com> > Date: Wed Apr 17 15:50:59 2019 +0200 > > MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared > > The problem is that if table definition cache (TDC) is full of real tables > which are in tables cache, view definition can not stay there so will be > removed by its own underlying tables. > In situation above old mechanism of detection matching definition in PS > and current version always require reprepare and so prevent executing > the PS. > > One work around is to increase TDC, other - improve version check for > views/triggers (which is done here). Now in suspicious cases we check: > - timestamp (ms) of the view to be sure that version really have changed; > - time (ms) of creation of a trigger related to time (ms) of statement > preparation. not "ms" but "us" or "µs". microseconds, not millisecons. > diff --git a/mysql-test/r/ps_ddl.result b/mysql-test/r/ps_ddl.result > index e69c6e06193..005c78a0319 100644 > --- a/mysql-test/r/ps_ddl.result > +++ b/mysql-test/r/ps_ddl.result > @@ -831,8 +831,8 @@ a b c > 20 40 100 > 30 60 150 > call p_verify_reprepare_count(1); > -SUCCESS > - > +ERROR > +Expected: 1, actual: 0 oops, forgot to update? > # Check that we properly handle ALTER VIEW statements. > execute stmt; > a b c > @@ -882,9 +882,9 @@ a b c > 10 50 60 > 20 100 120 > 30 150 180 > -call p_verify_reprepare_count(1); > -SUCCESS > - > +call p_verify_reprepare_count(0); > +ERROR > +Expected: 0, actual: 1 oops, updated too much? > execute stmt; > a b c > 10 50 60 > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 3f0fba8fc10..b9bb9c978fb 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1079,6 +1079,7 @@ class Statement: public ilink, public Query_arena > > LEX_STRING name; /* name for named prepared statements */ > LEX *lex; // parse tree descriptor > + ulonglong ms_prepare_time; // time of preparation in microseconds "ms" here too :( Better just say my_hrtime_t prepare_time; > /* > Points to the query associated with this statement. It's const, but > we need to declare it char * because all table handlers are written > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 64e4cd30561..a4a24cc1a80 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -4177,6 +4177,9 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len) > Query_arena *old_stmt_arena; > DBUG_ENTER("Prepared_statement::prepare"); > DBUG_ASSERT(m_sql_mode == thd->variables.sql_mode); > + > + // The same format as for triggers to compare > + ms_prepare_time= my_hrtime().val; do it at the end instead? > /* > If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql. > However, it seems handy if com_stmt_prepare is increased always, > @@ -4519,6 +4523,22 @@ Prepared_statement::execute_loop(String *expanded_query, > DBUG_ASSERT(thd->get_stmt_da()->sql_errno() == ER_NEED_REPREPARE); > thd->clear_error(); > > + { > + /* > + Check if we too fast with reprepare: > + we can be so fast that: > + 1) make change of a trigger, > + 2) prepare, > + 3) try to exacute and reprepare > + in 1 microsecond, so we will wait till > + next microsecond before last reprepare > + */ > + while (first_prepared == my_hrtime().val) > + { > + pthread_yield(); > + } > + } I wouldn't bother. A more likely case is when a system time goes back, e.g. if there's an ntpd running and your loop doesn't help there. I suggest not to do anything special in this case. And if an ::execute() will open a trigger or a view with a timestamp >= than ms_prepare_time, it will cause reprepare every time. Which is exactly the behavior before your bugfix - opening of a trigger or a view causes reprepare - and it is not a problem unless one has a very small table definition cache. > + > error= reprepare(); > > if (! error) /* Success */ > diff --git a/sql/parse_file.cc b/sql/parse_file.cc > index 999f53bd681..92e7d82af1c 100644 > --- a/sql/parse_file.cc > +++ b/sql/parse_file.cc > @@ -174,11 +174,11 @@ write_parameter(IO_CACHE *file, uchar* base, File_option *parameter) > { > /* string have to be allocated already */ > LEX_STRING *val_s= (LEX_STRING *)(base + parameter->offset); > - time_t tm= my_time(0); > - > - get_date(val_s->str, GETDATE_DATE_TIME|GETDATE_GMT|GETDATE_FIXEDLENGTH, > - tm); > - val_s->length= PARSE_FILE_TIMESTAMPLENGTH; > + ulonglong tm= my_hrtime().val; add a comment here, "number of microseconds since Epoch, timezone-independent" or something like that > + // Paded to 19 characters for compatibility > + val_s->length= snprintf(val_s->str, MICROSECOND_TIMESTAMP_BUFFER_SIZE, > + "%019lld", tm); > + DBUG_ASSERT(val_s->length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); > if (my_b_write(file, (const uchar *)val_s->str, > PARSE_FILE_TIMESTAMPLENGTH)) > DBUG_RETURN(TRUE); > @@ -835,15 +835,15 @@ File_parser::parse(uchar* base, MEM_ROOT *mem_root, > /* string have to be allocated already */ > LEX_STRING *val= (LEX_STRING *)(base + parameter->offset); > /* yyyy-mm-dd HH:MM:SS = 19(PARSE_FILE_TIMESTAMPLENGTH) characters */ a comment is now wrong > - if (ptr[PARSE_FILE_TIMESTAMPLENGTH] != '\n') > + if (ptr[MICROSECOND_TIMESTAMP_BUFFER_SIZE-1] != '\n') > { > my_error(ER_FPARSER_ERROR_IN_PARAMETER, MYF(0), > parameter->name.str, line); > DBUG_RETURN(TRUE); > } > - memcpy(val->str, ptr, PARSE_FILE_TIMESTAMPLENGTH); > - val->str[val->length= PARSE_FILE_TIMESTAMPLENGTH]= '\0'; > - ptr+= (PARSE_FILE_TIMESTAMPLENGTH+1); > + memcpy(val->str, ptr, MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); > + val->str[val->length= MICROSECOND_TIMESTAMP_BUFFER_SIZE-1]= '\0'; > + ptr+= MICROSECOND_TIMESTAMP_BUFFER_SIZE; > break; > } > case FILE_OPTIONS_STRLIST: > diff --git a/sql/table.h b/sql/table.h > index 14d6b787b4e..82508388f84 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -2159,7 +2175,7 @@ struct TABLE_LIST > LEX_STRING source; /* source of CREATE VIEW */ > LEX_STRING view_db; /* saved view database */ > LEX_STRING view_name; /* saved view name */ > - LEX_STRING timestamp; /* GMT time stamp of last operation */ > + LEX_STRING ms_timestamp; /* GMT time stamp of last operation */ better keep "timestamp" name. Or "hrtimestamp" if you'd like > st_lex_user definer; /* definer of view */ > ulonglong file_version; /* version of file's field set */ > ulonglong mariadb_version; /* version of server on creation */ > diff --git a/sql/table.cc b/sql/table.cc > index ca6ce02e4f2..24412966b36 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -8480,6 +8480,53 @@ bool TABLE_LIST::is_with_table() > } > > > +bool TABLE_LIST::is_table_ref_id_equal(THD* thd, TABLE_SHARE *s) > +{ > + enum enum_table_ref_type tp= s->get_table_ref_type(); > + if (m_table_ref_type == tp) > + { > + bool res= m_table_ref_version == s->get_table_ref_version(); > + > + /* > + If definition is different check content version > + */ > + if (tabledef_version.length && > + tabledef_version.length == s->tabledef_version.length && > + memcmp(tabledef_version.str, s->tabledef_version.str, > + tabledef_version.length) == 0) > + { > + if (table && table->triggers) > + { > + > + ulonglong ms_stmt_prepare= thd->ms_prepare_time; > + if (ms_stmt_prepare) > + for(uint i= 0; i < TRG_EVENT_MAX; i++) > + for (uint j= 0; j < TRG_ACTION_MAX; j++) > + { > + Trigger *tr= > + table->triggers->get_trigger((trg_event_type)i, > + (trg_action_time_type)j); > + if (tr) > + if (ms_stmt_prepare <= tr->ms_create_time) > + { > + set_tabledef_version(s); > + return FALSE; > + } > + } > + } > + set_table_id(s); > + return TRUE; > + } > + else > + tabledef_version.length= 0; > + return res; > + } > + else > + set_tabledef_version(s); > + return FALSE; > +} this is now a very confusing function. it doesn't only check is_table_ref_id_equal, it also sets some of them (tabledef or both tabledef and ref_id), and under unclear conditions. Could you please comment it? And you don't check that view's timestamp is in the past. If it's not, it can be changed still within the same microsecond. > + > + > uint TABLE_SHARE::actual_n_key_parts(THD *thd) > { > return use_ext_keys && > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h > index e2e6e1b7acc..70f246d4e0d 100644 > --- a/sql/sql_trigger.h > +++ b/sql/sql_trigger.h > @@ -112,7 +112,7 @@ class Trigger :public Sql_alloc > GRANT_INFO subject_table_grants; > sql_mode_t sql_mode; > /* Store create time. Can't be mysql_time_t as this holds also sub seconds */ > - ulonglong create_time; > + ulonglong ms_create_time; // Create time timestamp in microseconds my_hrtime_t create_time; > trg_event_type event; > trg_action_time_type action_time; > uint action_order; > @@ -195,7 +195,7 @@ class Table_triggers_list: public Sql_alloc > */ > List<ulonglong> definition_modes_list; > /** Create times for triggers */ > - List<ulonglong> create_times; > + List<ulonglong> ms_create_times; ditto > > List<LEX_STRING> definers_list; > > @@ -331,4 +331,14 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create); > extern const char * const TRG_EXT; > extern const char * const TRN_EXT; > > + > +/** > + Make time compatible with MySQL 5.7 trigger time. > +*/ > + > +inline ulonglong make_ms_time(my_time_t time, ulong time_sec_part) make_hrtime. and better put it together with hrtime_to_time, hrtime_from_time, etc > +{ > + return ((ulonglong) time)*1000000 + time_sec_part; > +} > + > #endif /* SQL_TRIGGER_INCLUDED */ > diff --git a/sql/sql_view.cc b/sql/sql_view.cc > index 020830483c5..3b51f6d2451 100644 > --- a/sql/sql_view.cc > +++ b/sql/sql_view.cc > @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view, > DBUG_RETURN(error); > } > > +/** > + Check is TABLE_LEST and SHARE match > + @param[in] view TABLE_LIST of the view > + @param[in] share Share object of view > + > + @return false on error or misspatch > +*/ > > +bool mariadb_view_version_get(TABLE_SHARE *share) > +{ > + DBUG_ASSERT(share->is_view); > + > + if (!(share->tabledef_version.str= > + (uchar*) alloc_root(&share->mem_root, > + MICROSECOND_TIMESTAMP_BUFFER_SIZE))) > + return TRUE; > + > + DBUG_ASSERT(share->view_def != NULL); > + if (share->view_def->parse((uchar *) &share->tabledef_version, NULL, > + view_timestamp_parameters, 1, > + &file_parser_dummy_hook)) > + return TRUE; > + DBUG_ASSERT(share->tabledef_version.length == MICROSECOND_TIMESTAMP_BUFFER_SIZE-1); If you open an old view with YYYY-MM-DD... timestamp, where do you convert it? If you open a view without a timestamp at all, where do you handle it? > + return FALSE; > +} > > /** > read VIEW .frm and create structures > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 9483db9eff9..138fa4bc631 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6724,13 +6724,15 @@ static bool store_trigger(THD *thd, Trigger *trigger, > table->field[14]->store(STRING_WITH_LEN("OLD"), cs); > table->field[15]->store(STRING_WITH_LEN("NEW"), cs); > > - if (trigger->create_time) > + if (trigger->ms_create_time) > { > + /* timestamp is in microseconds */ > table->field[16]->set_notnull(); > thd->variables.time_zone->gmt_sec_to_TIME(&timestamp, > - (my_time_t)(trigger->create_time/100)); > - /* timestamp is with 6 digits */ > - timestamp.second_part= (trigger->create_time % 100) * 10000; > + (my_time_t) > + (trigger->ms_create_time/ > + 1000000)); > + timestamp.second_part= trigger->ms_create_time % 1000000; hrtime_to_time() and hrtime_sec_part() > ((Field_temporal_with_date*) table->field[16])->store_time_dec(&timestamp, > 2); > } > @@ -9810,12 +9812,14 @@ static bool show_create_trigger_impl(THD *thd, Trigger *trigger) > trigger->db_cl_name.length, > system_charset_info); > > - if (trigger->create_time) > + if (trigger->ms_create_time) > { > MYSQL_TIME timestamp; > thd->variables.time_zone->gmt_sec_to_TIME(&timestamp, > - (my_time_t)(trigger->create_time/100)); > - timestamp.second_part= (trigger->create_time % 100) * 10000; > + (my_time_t) > + (trigger->ms_create_time/ > + 1000000)); > + timestamp.second_part= trigger->ms_create_time % 1000000; ditto > p->store(&timestamp, 2); > } > else Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: [Maria-developers] ed477a6d30c: MDEV-29056 Replica thread reports error on ALTER ONLINE after LOCK WRITE
by Sergei Golubchik 09 Sep '22

09 Sep '22

Hi, Nikita, No, in this case (and commonly for collateral changes) I'd rather prefer to refine the original (878a92e) commit to not cause the MDEV-29056 in the first place. Ignoring LOCK= clause is logically correct and that's what the old code was doing anyway. But perhaps ALTER TABLE still needs to check that ALGORITHM= and LOCK= clauses are compatible? On Aug 27, Nikita Malyavin wrote: > revision-id: ed477a6d30c (mariadb-10.6.1-521-ged477a6d30c) > parent(s): 56206e60315 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2022-07-26 00:41:00 +0300 > message: > > MDEV-29056 Replica thread reports error on ALTER ONLINE after LOCK WRITE > > Commit 878a92e (MDEV-28943) changes the behavior of ALTER TABLE under > prelocking. It now ignores passed LOCK= value in that case. > > However, LOCK TABLES command is never replicated, so the replica node > remains unaware of it. > > The solution would be to guess on the replica side on the mode used by > master. To make a deduction reliable, master's locked_tables_mode state > is passed for query log events. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

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