- developers - lists.mariadb.org

Re: 44e692d0dbc: MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
by Sergei Golubchik 06 Sep '23

06 Sep '23

Hi, Oleksandr, On Sep 04, Oleksandr Byelkin wrote: > revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc) > parent(s): 9cb75f333fa > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-09-04 14:12:29 +0200 > message: > > MDEV-29167 new db-level SHOW CREATE ROUTINE privilege > > -- Create general_log if CSV is enabled. > diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql > --- a/scripts/mariadb_system_tables_fix.sql > +++ b/scripts/mariadb_system_tables_fix.sql > @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_ > > UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0; > > +ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; > +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N'; Why two statements? > + > ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections, > ADD authentication_string TEXT NOT NULL AFTER plugin; > ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name, > NULL, db, > sctx->priv_role, > name, sph, 0))) > - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS); > + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS); Oh, now I get it. All the time up until this line I thought that SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the routine needs to have" And now I see that it means to "show procedure without definition". Please, rename to something, like, SHOW_PROC_EXISTANCE or SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But SHOW_PROC_NO_DEF is rather misleading. > } > mysql_rwlock_unlock(&LOCK_grant); > return no_routine_acl; > @@ -9237,7 +9242,7 @@ static const char *command_array[]= > "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", > "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", > "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", > - "BINLOG REPLAY", "SLAVE MONITOR" > + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE" > }; > > static uint command_lengths[]= > @@ -9250,7 +9255,7 @@ static uint command_lengths[]= > 11, 5, 7, 17, 14, > 8, 15, 16, 15, > 23, 24, 12, > - 13, 13 > + 13, 13, 19 May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course. > }; > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, > DBUG_RETURN(0); > > if (!full_access) > - full_access= !strcmp(sp_user, definer.str); > + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL, > + db.str, name.str, sph, TRUE); Wait, I thought we agreed that to see a routine body one has to be either the definer or granted SHOW CREATE ROUTINE. As far as I can see check_db_routine_access doesn't check for the user being the definer. > if (!full_access && > check_some_routine_access(thd, db.str, name.str, sph)) > DBUG_RETURN(0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 3

Re: 82a49812507: MDEV-22597 Add views for periods in information_schema
by Sergei Golubchik 05 Sep '23

05 Sep '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 82a49812507 (mariadb-11.0.1-228-g82a49812507) > parent(s): 23b15d24eb3 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 22:49:36 +0400 > message: > > MDEV-22597 Add views for periods in information_schema > > Two new information_schema views are added: > * PERIOD table -- columns TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, > PERIOD_NAME, START_COLUMN_NAME, END_COLUMN_NAME. > * KEY_PERIOD_USAGE -- works similar to KEY_COLUMN_USAGE, but for periods. > Columns CONSTRAINT_CATALOG, CONSTRAINT_SCHEMA, CONSTRAINT_NAME, > TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, PERIOD_NAME > > Two new columns are added to the COLUMNS view: > IS_SYSTEM_TIME_PERIOD_START, IS_SYSTEM_TIME_PERIOD_END - contain YES/NO. > > diff --git a/mysql-test/main/information_schema-big.result b/mysql-test/main/information_schema-big.result > index 94c8d274fad..ad0451293c9 100644 > --- a/mysql-test/main/information_schema-big.result > +++ b/mysql-test/main/information_schema-big.result good, you didn't forget to update big and embedded tests > diff --git a/mysql-test/suite/period/r/i_s_notembedded.result b/mysql-test/suite/period/r/i_s_notembedded.result > new file mode 100644 > index 00000000000..879376cc8b4 > --- /dev/null > +++ b/mysql-test/suite/period/r/i_s_notembedded.result > @@ -0,0 +1,52 @@ > +select * from information_schema.periods; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +create or replace table t1 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)); > +create or replace table t2 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)) with system versioning; > +show columns from t1; > +Field Type Null Key Default Extra > +id int(11) NO PRI NULL > +s timestamp(6) NO NULL > +e timestamp(6) NO NULL > +select * from information_schema.periods where table_schema = 'test'; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +def test t2 SYSTEM_TIME row_start row_end I don't think so, they're INVISIBLE_SYSTEM. for all practical purposes they don't exist as columns in the table. Shouldn't be shown either. But don't remove this test, let's keep it to show that row_start/row_end are *not* shown here. And add table t3 with full standard definition of period for system_time. it should be shown all right. > +def test t2 mytime s e > +def test t1 mytime s e > +create user periods_hidden@localhost; ... > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1dc60a01f38..876d6c6f244 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6165,11 +6166,83 @@ static void store_variable_type(THD *thd, const sp_variable *spvar, > } > } > > +static int store_schema_period_record(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name, > + const TABLE_SHARE::period_info_t &period) > +{ > + TABLE_SHARE *s= tl->table->s; > + const CHARSET_INFO *cs= system_charset_info; > + > + int err= schema_table->field[0]->store(STRING_WITH_LEN("def"), cs); > + if(!err) err= schema_table->field[1]->store(db_name, cs); > + if(!err) err= schema_table->field[2]->store(table_name, cs); > + if(!err) err= schema_table->field[3]->store(period.name, cs); well, ok. but as you've seen elsewhere these go without checks, as these calls generally cannot fail, they don't even allocate any memory. > + > + if (err) return err; > + > + int period_field= 4; > + for (auto *field: {period.start_field(s), period.end_field(s)}) > + { > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool col_granted= get_column_grant(thd, &tl->grant, > + db_name->str, table_name->str, > + field->field_name.str) & COL_ACLS; > + if (col_granted) > +#endif > + { > + schema_table->field[period_field]->set_notnull(); > + err= schema_table->field[period_field]->store(field->field_name, cs); > + } > + > + if (err) > + return err; > + period_field++; > + } > + > + return schema_table_store_record(thd, schema_table); > +} > + > + > +static int > +get_schema_period_records(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, //!< @ref > + //!< Show::periods_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > +{ > + TABLE *table= tl->table; > + > + if (!table || (!table->s->period.name && !table->versioned())) > + return 0; > + > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + check_access(thd, SELECT_ACL, db_name->str, > + &tl->grant.privilege, 0, 0, MY_TEST(tl->schema_table)); > + if (is_temporary_table(tl)) > + { > + tl->grant.privilege|= TMP_TABLE_ACLS; > + } > +#endif > + int err= 0; > + if (table->versioned()) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->vers); > + if (!err && table->s->period.name) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->period); > + > + return err; > +} > > -static int get_schema_column_record(THD *thd, TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static > +int get_schema_column_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, //!< @ref Show::columns_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > LEX *lex= thd->lex; > const char *wild= lex->wild ? lex->wild->ptr() : NullS; > @@ -7513,30 +7592,41 @@ static int get_schema_triggers_record(THD *thd, TABLE_LIST *tables, > DBUG_RETURN(0); > } > > +static int > +store_key_column_usage(TABLE *table, const LEX_CSTRING &db_name, > + const LEX_CSTRING &table_name, > + const LEX_CSTRING &key_name, > + const LEX_CSTRING &col_name) > +{ > + CHARSET_INFO *cs= system_charset_info; > + static const LEX_CSTRING def{STRING_WITH_LEN("def")}; > + const LEX_CSTRING *values[] { &def, &db_name, &key_name, > + &def, &db_name, &table_name, &col_name }; > + int err = 0; > + for (uint i = 0; i < array_elements(values) && !err; i++) > + table->field[i]->store(values[i], cs); you forgot `err=` here :) but as I wrote above, it's not needed here anyway > + return 0; > +} > > static void > store_key_column_usage(TABLE *table, const LEX_CSTRING *db_name, > const LEX_CSTRING *table_name, const char *key_name, > - size_t key_len, const char *con_type, size_t con_len, > + size_t key_len, const char *col_name, size_t col_len, > longlong idx) > { > - CHARSET_INFO *cs= system_charset_info; > - table->field[0]->store(STRING_WITH_LEN("def"), cs); > - table->field[1]->store(db_name->str, db_name->length, cs); > - table->field[2]->store(key_name, key_len, cs); > - table->field[3]->store(STRING_WITH_LEN("def"), cs); > - table->field[4]->store(db_name->str, db_name->length, cs); > - table->field[5]->store(table_name->str, table_name->length, cs); > - table->field[6]->store(con_type, con_len, cs); > + store_key_column_usage(table, *db_name, *table_name, {key_name, key_len}, > + {col_name, col_len}); pretty, but better, please, put it back. You can use the old store_key_column_usage from your get_schema_key_period_usage_record() just the same. There's no need to have two store_key_column_usage() functions. > table->field[7]->store((longlong) idx, TRUE); > } > > > -static int get_schema_key_column_usage_record(THD *thd, > - TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static int > +get_schema_key_column_usage_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, > + //!< @ref Show::key_column_usage_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > DBUG_ENTER("get_schema_key_column_usage_record"); > if (res) > @@ -9703,6 +9824,18 @@ ST_FIELD_INFO key_column_usage_fields_info[]= > CEnd() > }; > > +ST_FIELD_INFO key_period_usage_fields_info[]= > +{ > + Column("CONSTRAINT_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("PERIOD_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), Cannot they all be OPEN_FRM_ONLY? > + CEnd() > +}; > + > > ST_FIELD_INFO table_names_fields_info[]= > { > @@ -10208,6 +10358,8 @@ ST_SCHEMA_TABLE schema_tables[]= > {0, 0, 0, 0, 0, 0, 0, 0, 0, 0} > }; > > +static_assert(array_elements(schema_tables) == SCH_ENUM_SIZE + 1, > + "Update enum_schema_tables as well."); +1 > > int initialize_schema_table(st_plugin_int *plugin) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 6

Re: 8114fb63bbb: MDEV-31684: Add timezone information to DATE_FORMAT
by Sergei Golubchik 04 Sep '23

04 Sep '23

Hi, Rucha, Few comments, after I saw the commit in the branch: On Sep 04, Rucha Deodhar wrote: > revision-id: 8114fb63bbb (mariadb-11.0.1-229-g8114fb63bbb) > parent(s): 7ba9c7fb84b > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2023-09-01 11:01:09 +0530 > message: > > MDEV-31684: Add timezone information to DATE_FORMAT > > Before starting to go over the format string, prepare the current time > zone information incase '%z' or '%Z' is encountered. > This information can be obtained as given below: > > A) If timezone is not set ( meaning we are working with system timezone): > Get the MYSQL_TIME representation for current time and GMT time using > current thread variable for timezone and timezone variable for UTC > respectively. This MYSQL_TIME variable will be used to calculate time > difference. Also convert current time in second to tm structure to > get system timezone information. > > B) If timezone is set as offset: > Get timezone information using current timezone information and store > in appropriate variable. > > C) If timezone is set as some place (example: Europe/Berlin) > Get timezone information by searching the timezone. During internal > timezone search, information like timeoffset from UTC and abbrevation > is stored in another relevant structure. Hence use the same information. > > diff --git a/mysql-test/main/date_formats.result b/mysql-test/main/date_formats.result > index 463cce39520..23aeffedef7 100644 > --- a/mysql-test/main/date_formats.result > +++ b/mysql-test/main/date_formats.result > @@ -573,3 +573,67 @@ time_format('01 02:02:02', '%T') > select time_format('2001-01-01 02:02:02', '%T'); > time_format('2001-01-01 02:02:02', '%T') > 02:02:02 > +# > +# Beginning of 11.3 test > +# > +# MDEV-31684: Add timezone information to DATE_FORMAT > +# > +SET @old_timezone= @@time_zone; > +SET TIME_ZONE='Japan'; > +SELECT DATE_FORMAT('2009-10-04 22:23:00', '%z %Z') AS current_timezone; > +current_timezone > ++0900 CJT > +SELECT DATE_FORMAT(CURDATE(), '%z %Z') AS current_tz; 1) please, don't use CURDATE/CURRENT_TIMESTAMP/etc. The time zone abbreviation depends on the date: $ TZ=Europe/Berlin date -d '2020-01-01 1:2:3' Wed Jan 1 01:02:03 CET 2020 $ TZ=Europe/Berlin date -d '2020-06-01 1:2:3' Mon Jun 1 01:02:03 CEST 2020 See? Could be CET or CEST. Japan doesn't seem to have daylight saving, so it'll be the same abbreviation throughout the year, but it can still change at some point. So always use a fixed date, like in your first test. 2) why is it CJT at all? $ TZ=Japan date -d '2009-10-04 22:23:00' +'%z %Z' +0900 JST it looks like it should be JST? ... 3) also, please test that CET/CEST example. Or, rather, MET/MEST/MST/MMT example - the data for these abbreviations are already present in mariadb_test_data_timezone.sql That is, add lines like SET @@time_zone='Europe/Moscow'; SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); and with different dates it should return different abbreviations for the same time zone. > +# > +# output depends on system time information. Hence replacing > +# to avoid result diff test failures. > +# > +SET @@time_zone= default; > +SELECT DATE_FORMAT('2009-10+|-HH:MM ABC22:23:00', '%z %Z') AS current_timezone; 1) I don't understand what you're testing here :( 2) Anyway, note that you can put, say, --timezone=Europe/Berlin into the date_formats.opt file and the server will be restarted with this value in TZ. That is, you can force the system time zone to be a specific fixed value. > diff --git a/scripts/mariadb_test_data_timezone.sql b/scripts/mariadb_test_data_timezone.sql > index 8d07d413cef..83bb548478d 100644 > --- a/scripts/mariadb_test_data_timezone.sql > +++ b/scripts/mariadb_test_data_timezone.sql > @@ -14,8 +14,8 @@ > -- along with this program; if not, write to the Free Software > -- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA > > -INSERT INTO time_zone_name (Name, Time_Zone_id) ... > -INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... > -INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... > -INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... > -INSERT INTO time_zone_leap_second (Transition_time, Correction) ... > +INSERT INTO time_zone_name (Name, Time_Zone_id) VALUES ... ('India/Kolkata', 6); > +INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... > +INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... > +INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... > +INSERT INTO time_zone_leap_second (Transition_time, Correction) ... 1. Why? I didn't see where you use this new value in tests. 2. Is it really India/Kolkata? The official https://www.iana.org/time-zones seems to have Asia/Kolkata. > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc > index 26adc4eddaa..f8d3fe7dfe8 100644 > --- a/sql/item_timefunc.cc > +++ b/sql/item_timefunc.cc > @@ -468,20 +468,111 @@ static bool extract_date_time(THD *thd, DATE_TIME_FORMAT *format, > DBUG_RETURN(1); > } > > +/* > + > + Gets local time in MYSQL_TIME structure and tm struct. > + > +*/ > +void get_timezone_offset_information(THD *thd, Time_zone *tz, int &hr, > + int &min, char* abbrevation, > + bool &is_neg) > +{ > + if (!tz) > + return; > + > + if (tz->get_name()->length() == tz_SYSTEM_name.length() && > + !strncmp((const char*)(tz->get_name()->ptr()), > + (const char*)(tz_SYSTEM_name.ptr()), 6)) > + { > + struct tm tm_local_time; > + time_t time_sec= my_time(0); > + MYSQL_TIME local_TIME, gmt_TIME; > + ulonglong seconds_diff; > + ulong microsec_diff; > + > + /* Get local time in MYSQL_TIME struct. */ > + thd->variables.time_zone->gmt_sec_to_TIME(&local_TIME, > + thd->query_start()); > + /* Get GMT time in MYSQL_TIME struct. */ > + my_tz_UTC->gmt_sec_to_TIME(&gmt_TIME, thd->query_start()); > + > + /* We need above two MYSQL_TIME to get time difference. */ > + is_neg= calc_time_diff(&local_TIME, &gmt_TIME, 1, > + &seconds_diff, &microsec_diff); > + > + /* > + once we have time difference in seconds_diff, > + convert it into hr and min. > + */ > + hr= (int)(seconds_diff/3600L); > + int temp= (int)(seconds_diff%3600L); > + min= temp/60L; > + > + /* > + Get timezone abbrevation. It is stored in tm structure, > + so convert time to tm struct and copy it. > + */ 1) tm structure also has the offset, you don't need to calculate it manually. 2) you should use not my_time(0) but the value of the first DATE_FORMAT() argument > + localtime_r(&time_sec, &tm_local_time); > +# ifdef __USE_MISC > + int len= strlen((tm_local_time.tm_zone)); > + strncpy(abbrevation, (tm_local_time.tm_zone), len); > + abbrevation[len]= '\0'; > +# endif > + } > + else > + { > + /* > + Information for "named" timezone and offset can be obtained from their > + relevant class. So get the timezone information first. > + */ > + struct tz tmp; > + tz->get_timezone_information(&tmp); > + is_neg= tmp.is_behind; > + > + /* Get time in hour and min. */ > + time_t time_in_sec= abs(tmp.seconds_offset); > + hr= (int)(time_in_sec/3600L); > + int temp= (int)(time_in_sec%3600L); > + min= temp/60L; > + > + /* Copy the abbrevation. */ > + strncpy(abbrevation, (const char*)(tmp.abbrevation), > + strlen((const char*)(tmp.abbrevation))); > + abbrevation[strlen((const char*)(tmp.abbrevation))+1]='\0'; > + } > + > + return; > +} > + > > /** > Create a formatted date/time value in a string. > */ > > -static bool make_date_time(const String *format, const MYSQL_TIME *l_time, > - timestamp_type type, const MY_LOCALE *locale, > - String *str) > +static bool make_date_time(THD *thd, const String *format, > + const MYSQL_TIME *l_time, timestamp_type type, > + const MY_LOCALE *locale, String *str) > { > char intbuff[15]; > uint hours_i; > uint weekday; > ulong length; > const char *ptr, *end; > + int diff_hr=0, diff_min=0; > + bool curr_time_behind_utc= false; > + char abbrevation[8]; > + > + Time_zone* curr_timezone= my_tz_find(thd, > + thd->variables.time_zone->get_name()); > + > + /* > + precalculate local time and timezone information because > + %z or %Z maybe present in the format. > + */ > + memset(abbrevation, 0, sizeof(abbrevation)); > + get_timezone_offset_information(thd, curr_timezone, diff_hr, > + diff_min, abbrevation, > + curr_time_behind_utc); Just a thought. get_timezone_offset_information() doesn't seem to be very cheap. May be do it only when %z or %Z is requested? Like case 'z': if (!have_tz_info) { memset(abbrevation, 0, sizeof(abbrevation)); get_timezone_offset_information(thd, curr_timezone, diff_hr, diff_min, abbrevation, curr_time_behind_utc); have_tz_info= true; } Or better something with less code duplication :) > str->length(0); > > @@ -699,6 +790,31 @@ static bool make_date_time(const String *format, const MYSQL_TIME *l_time, > str->append_with_prefill(intbuff, length, 1, '0'); > break; > > + case 'z': > + { > + if (curr_time_behind_utc) > + str->append("-", 1, system_charset_info); > + else > + str->append("+", 1, system_charset_info); > + > + if (diff_hr/10 == 0) > + str->append("0", 1, system_charset_info); > + length= (uint) (int10_to_str(diff_hr, intbuff, 10) - intbuff); > + str->append_with_prefill(intbuff, length, 1, '0'); > + if (diff_min/10 == 0) > + str->append("0", 1, system_charset_info); > + length= (uint) (int10_to_str(diff_min, intbuff, 10) - intbuff); > + str->append_with_prefill(intbuff, length, 1, '0'); why do you append "0" manually, if you use append_with_prefill(), which, I suppose, does it automatically? or, if you add "0" manually, you should use simple append(). > + } > + break; > + > + case 'Z': > + { > + str->append(abbrevation, > + (uint) strlen(abbrevation), > + system_charset_info); > + } > + break; > default: > str->append(*ptr); > break; > @@ -1912,7 +2028,7 @@ String *Item_func_date_format::val_str(String *str) > > /* Create the result string */ > str->set_charset(collation.collation); > - if (!make_date_time(format, &l_time, > + if (!make_date_time(thd, format, &l_time, > is_time_format ? MYSQL_TIMESTAMP_TIME : > MYSQL_TIMESTAMP_DATE, > lc, str)) > diff --git a/sql/tztime.cc b/sql/tztime.cc > index e77a7529332..4662fa082e0 100644 > --- a/sql/tztime.cc > +++ b/sql/tztime.cc > @@ -55,15 +55,10 @@ > > /* > Now we don't use abbreviations in server but we will do this in future. > + Edit: Startd needing abbrevation in server as part of task MDEV-31684. typo > */ > -#if defined(TZINFO2SQL) || defined(TESTTIME) > -#define ABBR_ARE_USED > -#else > -#if !defined(DBUG_OFF) > -/* Let use abbreviations for debug purposes */ > -#undef ABBR_ARE_USED > -#define ABBR_ARE_USED > -#endif /* !defined(DBUG_OFF) */ > +#ifndef ABBR_ARE_USED > +#define ABBR_ARE_USED > #endif /* defined(TZINFO2SQL) || defined(TESTTIME) */ > > /* Structure describing local time type (e.g. Moscow summer time (MSD)) */ > @@ -1008,11 +1003,6 @@ TIME_to_gmt_sec(const MYSQL_TIME *t, const TIME_ZONE_INFO *sp, uint *error_code) > > #if !defined(TESTTIME) && !defined(TZINFO2SQL) > > -/* > - String with names of SYSTEM time zone. > -*/ > -static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); > - > > /* > Instance of this class represents local time zone used on this system > @@ -1031,6 +1021,7 @@ class Time_zone_system : public Time_zone > virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > }; > > > @@ -1097,6 +1088,12 @@ Time_zone_system::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_system::get_timezone_information(struct tz* curr_tz) const > +{ this is weird. You have a special if (tz->get_name()->length() == tz_SYSTEM_name.length() && !strncmp((const char*)(tz->get_name()->ptr()), (const char*)(tz_SYSTEM_name.ptr()), 6)) to detect SYSTEM time zone by name in get_timezone_offset_information(). why not to put that logic here, in the dedicated method, like you did for other time zones? > + return; > +} > + > > /* > Get name of time zone > @@ -1128,6 +1125,7 @@ class Time_zone_utc : public Time_zone > uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > }; > > > @@ -1175,6 +1173,12 @@ Time_zone_utc::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_utc::get_timezone_information(struct tz* curr_tz) const > +{ what is printed for UTC? Please, add a test for that. > + return; > +} > + > > /* > Get name of time zone > @@ -1210,6 +1214,7 @@ class Time_zone_db : public Time_zone > virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > private: > TIME_ZONE_INFO *tz_info; > const String *tz_name; > @@ -1280,6 +1285,16 @@ Time_zone_db::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_db::get_timezone_information(struct tz* curr_tz) const > +{ > + curr_tz->seconds_offset= tz_info->revtis->rt_offset; > + curr_tz->is_behind= tz_info->revtis->rt_offset < 0 ? true : false; > + strncpy((char*)(curr_tz->abbrevation), (const char*)(tz_info->chars), > + strlen((const char*)(tz_info->chars))+1); > + curr_tz->abbrevation[strlen((const char*)(tz_info->chars))+1]='\0'; here and below (in Time_zone_offset) 1. why casts? 2. you use strncpy incorrectly, the third argument specifies the size of the buffer, like strncpy(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)); but better use strmake, it'll do the zero-termination for you strmake(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)-1); > +} > + > > /* > Get name of time zone > @@ -1309,6 +1324,7 @@ class Time_zone_offset : public Time_zone > uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > /* > This have to be public because we want to be able to access it from > my_offset_tzs_get_key() function > @@ -1434,6 +1450,16 @@ Time_zone_offset::get_name() const > return &name; > } > > +void > +Time_zone_offset::get_timezone_information(struct tz* curr_tz) const > +{ > + curr_tz->seconds_offset= offset; > + const char *name= get_name()->ptr(); > + curr_tz->is_behind= name[0] == '+' ? false : true; > + strncpy((char*)(curr_tz->abbrevation), name, this->get_name()->length()); > + curr_tz->abbrevation[this->get_name()->length()]= '\0'; > +} > + > > static Time_zone_utc tz_UTC; > static Time_zone_system tz_SYSTEM; > diff --git a/sql/tztime.h b/sql/tztime.h > index 6d8af62ecd4..57e4aa2021b 100644 > --- a/sql/tztime.h > +++ b/sql/tztime.h > @@ -26,6 +26,11 @@ > #include "sql_list.h" /* Sql_alloc */ > #include "sql_string.h" /* String */ > > +/* > + String with names of SYSTEM time zone. > +*/ > +static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); No, sorry. It's a .h file included in many .cc files. You'll have a separate `static const String` in every single source file that includes this header. > + > class THD; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 23b15d24eb3: cleanup: add Field::store_yesno
by Sergei Golubchik 31 Aug '23

31 Aug '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 23b15d24eb3 (mariadb-11.0.1-227-g23b15d24eb3) > parent(s): cb37c99dd87 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 13:18:52 +0400 > message: > > cleanup: add Field::store_yesno I agree, it's a good idea to have a helper for storing YES/NO in the field. But I think Field class itself is a wrong place for it. YES/NO is exclusively INFORMATION_SCHEMA thing and it feels kind of wrong to have it in the base class. I'd do a separate function, like store_yes_no(). Or something. Also, note that you missed few places where it's needed, like pos=(uchar*) ((field->flags & NOT_NULL_FLAG) ? "NO" : "YES"); table->field[6]->store((const char*) pos, strlen((const char*) pos), cs); or if (updatable_view) table->field[5]->store(STRING_WITH_LEN("YES"), cs); else table->field[5]->store(STRING_WITH_LEN("NO"), cs); both in sql_show.cc. There are more. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: Client-server protocol improvement proposal: Connection redirection
by Otto Kekäläinen 28 Aug '23

28 Aug '23

Hi Pali! On Fri, 18 Aug 2023 at 11:47, Pali <pali(a)cpan.org> wrote: > CCing choroba (for Perl's DBD::MariaDB) and replying to developers@ ML. > > I have read proposal and the first thing which worries me is how the > proposal handles security aspect of the whole solution. > ... > If TLS is used then any protocol redirection information must happen > inside of the TLS tunnel. Thanks for the comments! Actually Daniel's submission has already this covered and the redirect works only if the connection is TLS protected: * https://github.com/MariaDB/server/pull/2681, server-side implementation * https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, MariaDB Connector/C implementation

1 0

Re: [MariaDB commits] [PATCH 4/4] MDEV-31273: Precompute binlog checksums
by Marko Mäkelä 25 Aug '23

25 Aug '23

On Fri, Aug 25, 2023 at 10:16 AM Kristian Nielsen via commits <commits(a)lists.mariadb.org> wrote: > > Compute binlog checksums (when enabled) already when writing events > into the statement or transaction caches, where before it was done > when the caches are copied to the real binlog file. This moves the > checksum computation outside of holding LOCK_log, improving > scalabitily. > > At stmt/trx cache write time, the final end_log_pos values are not > known, so with this patch these will be set to 0. [...] > Checksums cannot be pre-computed when binlog encryption is enabled, as > encryption relies on correct end_log_pos to provide part of the > nonce/IV. Checksum pre-computation is also disabled for WSREP/Galera. Have you thought about a format change that would address these issues? In MDEV-14425 https://github.com/MariaDB/server/commit/685d958e38b825ad9829be311f26729ccc… I redesigned the InnoDB log file format in a way that allows checksums to be computed before the final position of the records (the log sequence number of LSN) is known. For encryption, an additional 64-bit nonce will be written; this currently is the LSN at the time the encryption was scheduled. Tablespace identifiers, page numbers and file names are not encrypted, because these were never fully encrypted in innodb_encrypt_tables=ON either. This allows the metadata to be used as initialization vectors, and also avoids the need for backup tools to know any encryption keys. Thanks to MDEV-27774, not only InnoDB computes checksums while not holding any mutex, but multiple threads can copy their mini-transactions to the shared log_sys.buf while holding a shared lock_sys.latch. An exclusive latch will only be needed during a log checkpoint when we need to guarantee that all changes up to a particular LSN have been written to the log buffer. This would not be possible without the format change that was implemented in MDEV-14425. > +/* Convenience macros since ulong is 32 or 64 bit depending on platform. */ > +#if SIZEOF_LONG == 4 > +#define my_atomic_storeul(P, D) my_atomic_store32((int32 volatile *)(P), (D)) In all currently supported MariaDB Server versions, C++11 is available. Is there a particular reason why you would not use std::atomic<size_t> here? Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB plc

2 3

Client-server protocol improvement proposal: Connection redirection
by Lenski, Daniel 18 Aug '23

18 Aug '23

Hi! I would like to propose a new feature in the MariaDB client-server protocol<https://mariadb.com/kb/en/clientserver-protocol>: application-layer redirection of client connections. We want the MariaDB server to be able to tell clients connecting to it, “Sorry, this server is unavailable. Connect to an alternate server instead.” This mechanism is inspired by HTTP 302 (“temporary redirect”) mechanism familiar to all developers of web applications, and is intended to have similar semantics and security properties, since these have now been widely deployed and tested for decades. I have submitted a minimal but viable implementation of this at: * https://github.com/MariaDB/server/pull/2681, server-side implementation * https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, MariaDB Connector/C implementation I am seeking advice and opinions on these, and will proceed to finalize this in the following 1-2 months. In its current form, this implementation allows the MariaDB server administrator to set two variables: * SET GLOBAL SERVER_REDIRECT_MODE={ON,OFF} (the default is OFF) * SET GLOBAL SERVER_REDIRECT_TARGET='my-new-server.example.com' (or 192.168.0.123:3307, or new-server.com:3308, etc) When SERVER_REDIRECT_MODE is set to on, the server will stop accepting new TCP-based connections (unless they are to the extra port<https://mariadb.com/kb/en/thread-pool-in-mariadb/#configuring-the-extra-port>, which is intended for emergency administrative use), and it will instead respond to all clients with an error packet<https://mariadb.com/kb/en/err_packet/>: $ mariadb --host my-server.example.com ERROR 4189 (HY000): |Server is redirecting clients to 'my-new-server.example.com:3307'|my-new-server.example.com:3307 When an appropriately-updated client receives this error packet (error code 4196, message formatted as |Human readable message|<value of SERVER_REDIRECT_TARGET>), it will parse the redirection target out from the error message and attempt a new connection: $ updated-mariadb --host my-server.example.com -e 'select @@hostname' Got server redirect to 'my-new-server.example.com' (port 3307) +---------------------------+ | @@hostname | +---------------------------+ | my-new-server.example.com | +---------------------------+ The feature is gracefully backwards-compatible. Old clients that do not support redirection will fail and show the message in a form from which humans can deduce what happened. New clients that do support it will follow it to the new server. The feature is also designed to be extensible in a forwards-compatible way. The SERVER_REDIRECT_MODE system variable could be expanded to support additional redirection modes. For example, it could be augmented to redirect new connections only from specific IP subnets, or to direct the client to interpret the SERVER_REDIRECT_TARGET variable in a new form (such as a<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> JDBC-compatible mysql:// URL<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> as proposed in MDEV-15935<https://jira.mariadb.org/browse/MDEV-15935>). While researching and designing this connection redirection mechanism, I discovered or rediscovered a couple of major TLS-related security vulnerabilities in the MariaDB Connector/C client [1, 2]. These vulnerabilities affect all MariaDB users who care about the confidentiality and authenticity of their client/server connections, and they become even more severe if a connection redirection mechanism exists — because this entails the client trusting the server to send it instructions for creating a new network connection. Unless these are resolved, any connection redirection mechanism will be susceptible to hijacking and redirection to an attacker-controlled server even if the client thinks it is using TLS/SSL. What are the use cases for this connection redirection feature? Even in this simple, initial form, the feature is already useful for database administrators who want to gracefully shut down a MariaDB server (for example, to upgrade it), and to redirect new client connections to a replica or alternate server that can accept the traffic. Server-side connection redirection can be combined with existing MariaDB server features, such as SET GLOBAL READ_ONLY=ON and KILLing of idle or long-running connections, in order to build a full switchover solution for shunting traffic from one server to another as gracefully as possible. Currently, MariaDB database administrators tend to use proxies and wrappers (such as HikariCP) to redirect client connections from one MariaDB server to another. Supporting this redirection mechanism directly in the server, and in the client-server protocol, will allow many simple use cases of redirection without extra software, making MariaDB easier and simpler to manage for minimal downtime situations. Please post on this mailing list what you think about this suggestion. I am looking forward to polishing this feature and submitting it in a form that meets the quality requirements for new features from MariaDB core developers. Thank you! Daniel Lenski Amazon RDS MySQL/MariaDB engine team --- Notes on TLS vulnerabilities in MariaDB Connector/C: [1] CONC-648<https://jira.mariadb.org/browse/CONC-648>, reported by me on 5 June: when a Connector/C-based client is using connections secured by TLS/SSL, Connector/C will nevertheless receive plain-text, unencrypted error packets prior to the TLS handshake, and present them to the client in a way that’s indistinguishable from TLS-protected error packets sent by the server after the TLS handshake. This longstanding vulnerability is a consequence of the tangled and unsystematic way that the Connector/C codebase interleaves the MariaDB application-layer authentication protocol with the transport-layer security protocol. My standalone mariadb-connector-c PR#223<https://github.com/mariadb-corporation/mariadb-connector-c/pull/223> fixes the most straightforward manifestation of this vulnerability. Until this issue is resolved in Connector/C, it will be trivial for MITM attackers to inject plaintext connection redirection packets into MariaDB client/server connections, and thereby redirect clients to attacker-controlled servers despite those clients’ intention to use only TLS-secured connections. [2] MDEV-28634<https://jira.mariadb.org/browse/MDEV-28634>, reported almost 3 years ago by Geoff Montee: Connector/C clients which specify --ssl without --ssl-verify-server-cert will allow their connections to be silently downgraded to unencrypted, plaintext connections. This vulnerability can be used by on-path attackers to trivially downgrade most MariaDB client-server connections from TLS-encrypted to plaintext. My standalone mariadb-connector-c PR#224<https://github.com/mariadb-corporation/mariadb-connector-c/pull/224> fixes this. It is a backwards-incompatible change in that clients which specify --ssl (and thus clearly expect TLS-secured connections) will no longer succeed in connecting to servers which don’t support TLS. This should be considered a good thing: MariaDB Connector/C-based clients have been misleading their users about whether their connections are guaranteed to be encrypted, and will no longer do so. Essentially all software that purports to secure network connections with TLS have implemented similar changes, most of it about a decade ago when the pervasiveness of such attacks<https://en.wikipedia.org/wiki/Global_surveillance_disclosures_(2013%E2%80%9…> (by ISPs, censors, intelligenceagencies, etc) became very widely-known.

8 28

Re: [Commits] cb06612a9da: MDEV-31655: Parallel replication deadlock victim preference code errorneously removed
by Kristian Nielsen 15 Aug '23

15 Aug '23

Hi Marko, I'd like to ask you to review the below patch for MDEV-31655. https://jira.mariadb.org/browse/MDEV-31655 https://github.com/MariaDB/server/commit/cb06612a9da09a7981ada84768793f2ff3… This patch restores code in InnoDB that was originally part of the parallel replication implementation. It makes InnoDB choose the right victim when two transactions in parallel replication deadlock with each other. The code was erroneously removed as part of the VATS work in InnoDB. I saw that you later deprecated and eventually removed the VATS code. I saw that the InnoDB deadlock code is substantially changed in 10.6, so I will need to rework the patch in that version. But I wanted to ask your opinion about this 10.4 version first. In the patch, note in particular the modification to has_higher_priority(), which I think has the effect of choosing which of two waiting transactions to grant a lock to. I don't remember adding this myself, so it might have been added as part of VATS work. But it sounds appropriate; if T1 and T2 are both waiting for a lock, there is no point in granting it to T2 over T1, as then T2 will just be immediately killed and rolled back by replication. - Kristian. Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > revision-id: cb06612a9da09a7981ada84768793f2ff3ef842c (mariadb-10.4.30-3-gcb06612a9da) > parent(s): dbe5c20b755b87f67d87990c3baabc866667e41b > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2023-07-11 00:31:29 +0200 > message: > > MDEV-31655: Parallel replication deadlock victim preference code errorneously removed > > Restore code to make InnoDB choose the second transaction as a deadlock > victim if two transactions deadlock that need to commit in-order for > parallel replication. This code was erroneously removed when VATS was > implemented in InnoDB. > > Also add a test case for InnoDB choosing the right deadlock victim. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > > --- > .../suite/binlog_encryption/rpl_parallel.result | 42 ++++++++++++- > mysql-test/suite/rpl/r/rpl_parallel.result | 42 ++++++++++++- > mysql-test/suite/rpl/t/rpl_parallel.test | 71 +++++++++++++++++++++- > sql/rpl_parallel.cc | 7 ++- > sql/sql_class.cc | 43 +++++++++++++ > storage/innobase/lock/lock0lock.cc | 12 ++++ > storage/innobase/trx/trx0trx.cc | 12 ++++ > 7 files changed, 225 insertions(+), 4 deletions(-) > > diff --git a/mysql-test/suite/binlog_encryption/rpl_parallel.result b/mysql-test/suite/binlog_encryption/rpl_parallel.result > index b75a66a634a..b24ff7ba53d 100644 > --- a/mysql-test/suite/binlog_encryption/rpl_parallel.result > +++ b/mysql-test/suite/binlog_encryption/rpl_parallel.result > @@ -2,6 +2,7 @@ include/master-slave.inc > [connection master] > connection server_2; > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > SET GLOBAL slave_parallel_threads=10; > ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first > include/stop_slave.inc > @@ -1680,13 +1681,52 @@ a > 2000 > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > a > +MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +connection server_1; > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +COMMIT; > +include/save_master_gtid.inc > +connection server_2; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 5; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > +connection master; > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > +SET @commit_id= 1+1000; > +SET @commit_id= 2+1000; > +SET @commit_id= 3+1000; > +SET @commit_id= 4+1000; > +SET @commit_id= 5+1000; > +SET @commit_id= 6+1000; > +SET @commit_id= 7+1000; > +SET @commit_id= 8+1000; > +SET @commit_id= 9+1000; > +SET @commit_id= 10+1000; > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > +include/save_master_gtid.inc > +connection server_2; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > connection server_2; > include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > connection server_1; > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/r/rpl_parallel.result b/mysql-test/suite/rpl/r/rpl_parallel.result > index 9b2e68d366e..ef89d954faa 100644 > --- a/mysql-test/suite/rpl/r/rpl_parallel.result > +++ b/mysql-test/suite/rpl/r/rpl_parallel.result > @@ -2,6 +2,7 @@ include/master-slave.inc > [connection master] > connection server_2; > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > SET GLOBAL slave_parallel_threads=10; > ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first > include/stop_slave.inc > @@ -1679,13 +1680,52 @@ a > 2000 > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > a > +MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +connection server_1; > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +COMMIT; > +include/save_master_gtid.inc > +connection server_2; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 5; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > +connection master; > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > +SET @commit_id= 1+1000; > +SET @commit_id= 2+1000; > +SET @commit_id= 3+1000; > +SET @commit_id= 4+1000; > +SET @commit_id= 5+1000; > +SET @commit_id= 6+1000; > +SET @commit_id= 7+1000; > +SET @commit_id= 8+1000; > +SET @commit_id= 9+1000; > +SET @commit_id= 10+1000; > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > +include/save_master_gtid.inc > +connection server_2; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > connection server_2; > include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > connection server_1; > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_parallel.test b/mysql-test/suite/rpl/t/rpl_parallel.test > index 9ba7a30f2eb..d43cec4df34 100644 > --- a/mysql-test/suite/rpl/t/rpl_parallel.test > +++ b/mysql-test/suite/rpl/t/rpl_parallel.test > @@ -13,6 +13,7 @@ > > --connection server_2 > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > --error ER_SLAVE_MUST_STOP > SET GLOBAL slave_parallel_threads=10; > --source include/stop_slave.inc > @@ -2203,16 +2204,84 @@ SELECT * FROM t1 WHERE a>=2000 ORDER BY a; > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > > > +--echo MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +# The problem was that InnoDB would choose the wrong deadlock victim. > +# Create a lot of transactions that can cause deadlocks, and use error > +# injection to check that the first transactions in each group is never > +# selected as deadlock victim. > +--let $rows= 10 > +--let $transactions= 5 > +--let $gcos= 10 > + > +--connection server_1 > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +--disable_query_log > +--let $i= 0 > +while ($i < 10) { > + eval INSERT INTO t7 VALUES ($i, 0); > + inc $i; > +} > +--enable_query_log > +COMMIT; > +--source include/save_master_gtid.inc > + > +--connection server_2 > +--source include/sync_with_master_gtid.inc > +--source include/stop_slave.inc > +eval set @@global.slave_parallel_threads= $transactions; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +# This error injection will allow no retries for GTIDs divisible by 1000. > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > + > +--connection master > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > + > +--let $j= 1 > +while ($j <= $gcos) { > + eval SET @commit_id= $j+1000; > + --let $i= 0 > + while ($i < $transactions) { > + --disable_query_log > + eval SET SESSION gtid_seq_no= 1000 + 1000*$j + $i; > + BEGIN; > + --let $k= 0 > + while ($k < $rows) { > + eval UPDATE t7 SET b=b+1 WHERE a=(($i+$k) MOD $rows); > + inc $k; > + } > + COMMIT; > + --enable_query_log > + inc $i; > + } > + inc $j; > +} > + > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > + > +--source include/save_master_gtid.inc > + > +--connection server_2 > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > + > + > # Clean up. > --connection server_2 > --source include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > --source include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > > --connection server_1 > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > > --source include/rpl_end.inc > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index b550315d69f..1aeb1257c4a 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -1385,8 +1385,13 @@ handle_rpl_parallel_thread(void *arg) > err= dbug_simulate_tmp_error(rgi, thd);); > if (unlikely(err)) > { > + ulong max_retries= slave_trans_retries; > convert_kill_to_deadlock_error(rgi); > - if (has_temporary_error(thd) && slave_trans_retries > 0) > + DBUG_EXECUTE_IF("rpl_mdev31655_zero_retries", > + if ((rgi->current_gtid.seq_no % 1000) == 0) > + max_retries= 0; > + ); > + if (has_temporary_error(thd) && max_retries > 0) > err= retry_event_group(rgi, rpt, qev); > } > } > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 8ed3f8a9c5e..e6ed7ca1cc4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5247,6 +5247,49 @@ thd_need_ordering_with(const MYSQL_THD thd, const MYSQL_THD other_thd) > return 0; > } > > + > +/* > + If the storage engine detects a deadlock, and needs to choose a victim > + transaction to roll back, it can call this function to ask the upper > + server layer for which of two possible transactions is prefered to be > + aborted and rolled back. > + > + In parallel replication, if two transactions are running in parallel and > + one is fixed to commit before the other, then the one that commits later > + will be prefered as the victim - chosing the early transaction as a victim > + will not resolve the deadlock anyway, as the later transaction still needs > + to wait for the earlier to commit. > + > + The return value is -1 if the first transaction is prefered as a deadlock > + victim, 1 if the second transaction is prefered, or 0 for no preference (in > + which case the storage engine can make the choice as it prefers). > +*/ > +extern "C" int > +thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2) > +{ > + rpl_group_info *rgi1, *rgi2; > + > + if (!thd1 || !thd2) > + return 0; > + > + /* > + If the transactions are participating in the same replication domain in > + parallel replication, then request to select the one that will commit > + later (in the fixed commit order from the master) as the deadlock victim. > + */ > + rgi1= thd1->rgi_slave; > + rgi2= thd2->rgi_slave; > + if (rgi1 && rgi2 && > + rgi1->is_parallel_exec && > + rgi1->rli == rgi2->rli && > + rgi1->current_gtid.domain_id == rgi2->current_gtid.domain_id) > + return rgi1->gtid_sub_id < rgi2->gtid_sub_id ? 1 : -1; > + > + /* No preferences, let the storage engine decide. */ > + return 0; > +} > + > + > extern "C" int thd_non_transactional_update(const MYSQL_THD thd) > { > return(thd->transaction.all.modified_non_trans_table); > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index b1cf2152cd6..469d03eaa06 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -49,6 +49,8 @@ Created 5/7/1996 Heikki Tuuri > #include <mysql/service_wsrep.h> > #endif /* WITH_WSREP */ > > +extern "C" int thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2); > + > /** Lock scheduling algorithm */ > ulong innodb_lock_schedule_algorithm; > > @@ -1538,6 +1540,16 @@ static bool has_higher_priority(lock_t *lock1, lock_t *lock2) > } else if (!lock_get_wait(lock2)) { > return false; > } > + // Ask the upper server layer if any of the two trx should be prefered. > + int preference = thd_deadlock_victim_preference(lock1->trx->mysql_thd, > + lock2->trx->mysql_thd); > + if (preference == -1) { > + // lock1 is preferred as a victim, so lock2 has higher priority > + return false; > + } else if (preference == 1) { > + // lock2 is preferred as a victim, so lock1 has higher priority > + return true; > + } > return lock1->trx->start_time_micro <= lock2->trx->start_time_micro; > } > > diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc > index 7cd95878b0c..0771d764fb6 100644 > --- a/storage/innobase/trx/trx0trx.cc > +++ b/storage/innobase/trx/trx0trx.cc > @@ -52,6 +52,9 @@ Created 3/26/1996 Heikki Tuuri > #include <set> > #include <new> > > +extern "C" > +int thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2); > + > /** The bit pattern corresponding to TRX_ID_MAX */ > const byte trx_id_max_bytes[8] = { > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff > @@ -1906,6 +1909,15 @@ trx_weight_ge( > { > ibool a_notrans_edit; > ibool b_notrans_edit; > + int pref; > + > + /* First ask the upper server layer if it has any preference for which > + to prefer as a deadlock victim. */ > + pref= thd_deadlock_victim_preference(a->mysql_thd, b->mysql_thd); > + if (pref < 0) > + return FALSE; > + else if (pref > 0) > + return TRUE; > > /* If mysql_thd is NULL for a transaction we assume that it has > not edited non-transactional tables. */ > _______________________________________________ > commits mailing list -- commits(a)lists.mariadb.org > To unsubscribe send an email to commits-leave(a)lists.mariadb.org

2 7

Re: 9c52eede2c8: MDEV-31646 Online alter applies binlog cache limit to cache writes
by Sergei Golubchik 15 Aug '23

15 Aug '23

Hi, Nikita, On Aug 14, Nikita Malyavin wrote: > revision-id: 9c52eede2c8 (mariadb-11.0.1-175-g9c52eede2c8) > parent(s): 209c894b431 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-11 16:29:37 +0400 > message: > > MDEV-31646 Online alter applies binlog cache limit to cache writes > > 1. Make online disk writes unlimited, same as filesort does. > 2. Make proper error handling -- in 32-bit build IO_CACHE capacity limit is > 4GB, so it is quite possible to overfill there. > 3. Event_log::write_cache complicated with event reparsing, and as it was > proven by QA, contains some mistakes. Rewrite introbuce a simpler and much > faster version, not featuring reparsing and therefore copying a whole > buffer at once. This also disables checksums and crypto. > > As a result, online alter is untied of several binlog variables, which was > a second aim of this patch. > > diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result > index 021267a15f5..3a615372ed5 100644 > --- a/mysql-test/main/alter_table_online_debug.result > +++ b/mysql-test/main/alter_table_online_debug.result > @@ -409,23 +409,23 @@ set debug_sync= 'now SIGNAL start_replication'; > set debug_sync= 'now WAIT_FOR applied'; > select stage, progress from INFORMATION_SCHEMA.PROCESSLIST where id = @con; > stage progress > -3 51.220 > +3 53.390 why is that? > set debug_sync= 'now SIGNAL proceed WAIT_FOR applied'; > select stage, progress from INFORMATION_SCHEMA.PROCESSLIST where id = @con; > stage progress > diff --git a/sql/log.cc b/sql/log.cc > index d9e81700463..8c57487d57e 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2286,13 +2286,21 @@ int binlog_log_row_online_alter(TABLE* table, const uchar *before_record, > MY_BITMAP *old_rpl_write_set= table->rpl_write_set; > table->rpl_write_set= &table->s->all_set; > > + table->online_alter_cache->store_prev_position(); > int error= (*log_func)(thd, table, table->s->online_alter_binlog, > - table->online_alter_cache, true, > + table->online_alter_cache, > + table->file->has_transactions_and_rollback(), > before_record, after_record); > > table->rpl_write_set= old_rpl_write_set; > > - return unlikely(error) ? HA_ERR_RBR_LOGGING_FAILED : 0; > + if (unlikely(error)) > + { > + table->online_alter_cache->restore_prev_position(); what is this for? > + return HA_ERR_RBR_LOGGING_FAILED; > + } > + > + return 0; > } > > static void > @@ -7748,7 +7763,7 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > { > DBUG_ASSERT(cache.cache_log.type != READ_CACHE); > mysql_mutex_lock(binlog->get_log_lock()); > - error= binlog->write_cache(thd, &cache.cache_log); > + error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: ca64ddcc709: MDEV-31646 Online alter applies binlog cache limit to cache writes
by Sergei Golubchik 10 Aug '23

10 Aug '23

Hi, Nikita, This is a review of all the three commits On Jul 20, Nikita Malyavin wrote: > revision-id: ca64ddcc709 (mariadb-11.0.1-144-gca64ddcc709) > parent(s): f34c7419cb8 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-19 02:31:32 +0400 > message: > > MDEV-31646 Online alter applies binlog cache limit to cache writes > diff --git a/sql/log.cc b/sql/log.cc > index afd0643fe82..7466d63d325 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2291,8 +2294,14 @@ int binlog_log_row_online_alter(TABLE* table, const uchar *before_record, > before_record, after_record); > > table->rpl_write_set= old_rpl_write_set; > + thd->pop_internal_handler(); > + > + if (unlikely(error)) > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, ER_DISK_FULL, > + "Broken online alter log. " > + "ALTER TABLE will finish with error."); I don't think so. Why would a DML statement get a warning about the concurrently running ALTER TABLE? It didn't do anything wrong, I don't think there should be a warning here Like, someone is running a series of DML statements, or the same statement many times. At some point another connection runs ALTER TABLE and suddenly those perfectly good DML statements start spewing out warnings. ALTER ends and all is clear again - looks wrong to me > > - return unlikely(error) ? HA_ERR_RBR_LOGGING_FAILED : 0; > + return 0; > } > > static void > @@ -2592,7 +2601,7 @@ void Event_log::set_write_error(THD *thd, bool is_transactional) > DBUG_VOID_RETURN; > } > > -bool Event_log::check_write_error(THD *thd) > +bool MYSQL_BIN_LOG::check_write_error(THD *thd) What's the difference? it's a static method anyway, FWIW it could be outside of any class. What's the point of moving it from one class to another? > { > DBUG_ENTER("MYSQL_BIN_LOG::check_write_error"); > > @@ -3806,9 +3815,9 @@ bool MYSQL_BIN_LOG::open_index_file(const char *index_file_name_arg, > } > > > -bool Event_log::open(enum cache_type io_cache_type_arg) > +bool Event_log::open(enum cache_type io_cache_type_arg, size_t buffer_size) > { > - bool error= init_io_cache(&log_file, -1, LOG_BIN_IO_SIZE, io_cache_type_arg, > + bool error= init_io_cache(&log_file, -1, buffer_size, io_cache_type_arg, why? it's only used by online alter, where would you need a different buffer size? only in your DBUG_EXECUTE_IF? > 0, 0, MYF(MY_WME | MY_NABP | MY_WAIT_IF_FULL)); > > log_state= LOG_OPENED; > @@ -6544,7 +6562,8 @@ Event_log::flush_and_set_pending_rows_event(THD *thd, Rows_log_event* event, > if (writer.write(pending)) > { > set_write_error(thd, is_transactional); > - if (check_write_error(thd) && cache_data && > + if (dynamic_cast<MYSQL_BIN_LOG*>(this) && why? > + MYSQL_BIN_LOG::check_write_error(thd) && cache_data && > stmt_has_updated_non_trans_table(thd)) > cache_data->set_incident(); > delete pending; > @@ -7721,8 +7749,11 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > { > DBUG_ASSERT(cache.cache_log.type != READ_CACHE); > mysql_mutex_lock(binlog->get_log_lock()); > - error= binlog->write_cache(thd, &cache.cache_log); > + error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > + > + if (unlikely(error)) > + binlog->set_write_error(my_errno); shouldn't binlog->write_cache() call set_write_error() internally? why would the caller have to do it? > } > } > else if (!commit) // rollback > @@ -7736,14 +7767,14 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > cache.store_prev_position(); > } > > + thd->pop_internal_handler(); > > if (error) > { > - my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG), > - binlog->get_name(), errno); > - binlog_online_alter_cleanup(thd->online_alter_cache_list, > - is_ending_transaction); > - DBUG_RETURN(error); > + push_warning_printf(thd, Sql_state_errno_level::WARN_LEVEL_WARN, > + ER_ERROR_ON_WRITE, ER_THD(thd, ER_ERROR_ON_WRITE), > + binlog->get_name(), errno); > + DBUG_ASSERT(binlog->get_write_error()); same comment as above about not showing ALTER problems in concurrent threads > } > } > > diff --git a/sql/log.h b/sql/log.h > index 83e323cb6fb..39512b01991 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -444,17 +445,20 @@ class Event_log: public MYSQL_LOG > */ > class Cache_flip_event_log: public Event_log { > IO_CACHE alt_buf; > +IF_DBUG(public:,) yuck I don't see you using online_write_error or ref_count in DBUG macros > IO_CACHE *current, *alt; > std::atomic<uint> ref_count; > + std::atomic<int> online_write_error; > public: > > Cache_flip_event_log() : Event_log(), alt_buf{}, > - current(&log_file), alt(&alt_buf), ref_count(1) {} > - bool open(enum cache_type io_cache_type_arg) > + current(&log_file), alt(&alt_buf), ref_count(1), > + online_write_error(0) {} > + bool open(size_t buffer_size) looks like Cache_flip_event_log::open() doesn't need any arguments at all > { > log_file.dir= mysql_tmpdir; > alt_buf.dir= log_file.dir; > - bool res= Event_log::open(io_cache_type_arg); > + bool res= Event_log::open(WRITE_CACHE); > if (res) > return res; > > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 10180a95d99..0c4c284a40f 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -761,16 +761,21 @@ Log_event::Log_event(const uchar *buf, > > int Log_event::read_log_event(IO_CACHE* file, String* packet, > const Format_description_log_event *fdle, > - enum enum_binlog_checksum_alg checksum_alg_arg) > + enum enum_binlog_checksum_alg checksum_alg_arg, > + size_t max_allowed_packet) > { > ulong data_len; > char buf[LOG_EVENT_MINIMAL_HEADER_LEN]; > uchar ev_offset= packet->length(); > #if !defined(MYSQL_CLIENT) > - THD *thd=current_thd; > - ulong max_allowed_packet= thd ? thd->slave_thread ? slave_max_allowed_packet > - : thd->variables.max_allowed_packet > - : ~(uint)0; > + if (max_allowed_packet == 0) may be better pass THD as an argument? or even correct value of max_allowed_packet. As ulong max_allowed_packet(THD *thd) { return thd ? ... : ~(uint)0; } on the second thought, do you need to ignore max_allowed_packet here for online ALTER? Is there no event size limit when the event is written into binlog? > + { > + THD *thd=current_thd; > + max_allowed_packet= thd ? thd->slave_thread > + ? slave_max_allowed_packet > + : thd->variables.max_allowed_packet > + : ~(uint)0; > + } > #endif > DBUG_ENTER("Log_event::read_log_event(IO_CACHE*,String*...)"); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 7