- developers - lists.mariadb.org

Re: [Maria-developers] c8248ca9a22: MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK
by Sergei Golubchik 23 Jan '19

23 Jan '19

Hi, Thiru! Thanks! This is, indeed, much easier to follow. Nicely done. Ok to push. On Jan 23, Thirunarayanan Balathandayuthapani wrote: > revision-id: c8248ca9a22 (mariadb-10.0.37-45-gc8248ca9a22) > parent(s): 12f362c3338 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2019-01-18 12:01:18 +0530 > message: > > MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 2302026b18b..645746a7ab0 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6513,6 +6519,17 @@ static bool fill_alter_inplace_info(THD *thd, > new_field->field->field_index != key_part->fieldnr - 1) > goto index_changed; > } > + > + /* > + Rebuild the index if following condition get satisfied: > + > + (i) Old table doesn't have primary key, new table has it and vice-versa > + (ii) Primary key changed to another existing index > + */ > + if ((new_key == new_pk) != > + (table_key == &table->key_info[table->s->primary_key])) I would suggest to rewrite the condition as table_key - table->key_info == table->s->primary_key it means the same thing, but doesn't confuse the reader with a question "what's with a buffer overrun if table->s->primary_key == MAX_KEY?" > + goto index_changed; > + > continue; > > index_changed: > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] c3ef531aaff: MDEV-10963 Fragmented BINLOG query
by Sergei Golubchik 22 Jan '19

22 Jan '19

Hi, Andrei! Thanks! We're almost there. just a couple of minor comments re. format strings, see below. On Dec 18, andrei.elkin(a)pp.inet.fi wrote: > revision-id: c3ef531aaff39d3262882213157d36a545b600dd (mariadb-10.1.37-31-gc3ef531aaff) > parent(s): 541500295abdba7fa619065069291ac9c1dd6e83 > author: Andrei Elkin > committer: Andrei Elkin > timestamp: 2018-12-18 18:28:59 +0200 > message: > > MDEV-10963 Fragmented BINLOG query ... > diff --git a/sql/log_event.cc b/sql/log_event.cc > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -10491,12 +10505,128 @@ void Rows_log_event::pack_info(Protocol *protocol) > #endif > > #ifdef MYSQL_CLIENT > +/** > + Print an event "body" cache to @c file possibly in two fragments. > + Each fragement is optionally per @c do_wrap to produce an SQL statement. > + > + @param file a file to print to > + @param body the "body" IO_CACHE of event > + @param do_wrap whether to wrap base64-encoded strings with > + SQL cover. > + @param delimiter delimiter string > + > + The function signals on any error through setting @c body->error to -1. > +*/ > +void copy_cache_to_file_wrapped(FILE *file, > + IO_CACHE *body, > + bool do_wrap, > + const char *delimiter) > +{ > + const char str_binlog[]= "\nBINLOG '\n"; > + const char fmt_delim[]= "'%s\n"; > + const char fmt_n_delim[]= "\n'%s\n"; > + const my_off_t cache_size= my_b_tell(body); > + > + if (reinit_io_cache(body, READ_CACHE, 0L, FALSE, FALSE)) > + { > + body->error= -1; > + goto end; > + } > + > + if (!do_wrap) > + { > + my_b_copy_to_file(body, file, SIZE_T_MAX); > + } > + else if (4 + sizeof(str_binlog) + cache_size + sizeof(fmt_delim) > > + opt_binlog_rows_event_max_encoded_size) > + { > + /* > + 2 fragments can always represent near 1GB row-based > + base64-encoded event as two strings each of size less than > + max(max_allowed_packet). Greater number of fragments does not > + save from potential need to tweak (increase) @@max_allowed_packet > + before to process the fragments. So 2 is safe and enough. > + > + Split the big query when its packet size's estimation exceeds a > + limit. The estimate includes the maximum packet header > + contribution of non-compressed packet. > + */ > + const char fmt_frag[]= "%sSET @binlog_fragment_%d ='\n"; > + > + my_fprintf(file, fmt_frag, "\n", 0); > + if (my_b_copy_to_file(body, file, cache_size/2 + 1)) > + { > + body->error= -1; > + goto end; > + } > + my_fprintf(file, fmt_n_delim, delimiter); see, you use fmt_n_delim only once. here. but you end it with \n. And it forced you to start fmt_frag with %s. please, remove \n from the end of fmt_n_delim and add it to the beginning of fmt_frag > + > + my_fprintf(file, fmt_frag, "", 1); > + if (my_b_copy_to_file(body, file, SIZE_T_MAX)) > + { > + body->error= -1; > + goto end; > + } > + my_fprintf(file, fmt_delim, delimiter); and here - no \n before ' ? I've got lost in all these new lines and format constants. can you please show what the binlog dump will look like? > + > + my_fprintf(file, "BINLOG @binlog_fragment_%d, @binlog_fragment_%d%s\n", > + 0, 1, delimiter); don't pass 0 and 1 as parameters, it's not like they can ever change :) > + } > + else > + { > + my_fprintf(file, str_binlog); > + if (my_b_copy_to_file(body, file, SIZE_T_MAX)) > + { > + body->error= -1; > + goto end; > + } > + my_fprintf(file, fmt_delim, delimiter); > + } > + reinit_io_cache(body, WRITE_CACHE, 0, FALSE, TRUE); > + > +end: > + return; > +} > + > +/** > + The function invokes base64 encoder to run on the current > + event string and store the result into two caches. > + When the event ends the current statement the caches are is copied into > + the argument file. > + Copying is also concerned how to wrap the event, specifically to produce > + a valid SQL syntax. > + When the encoded data size is within max(MAX_ALLOWED_PACKET) > + a regular BINLOG query is composed. Otherwise it is build as fragmented > + > + SET @binlog_fragment_0='...'; > + SET @binlog_fragment_1='...'; > + BINLOG @binlog_fragment_0, @binlog_fragment_1; > + > + where fragments are represented by a pair of indexed user > + "one shot" variables. > + > + NOTE. still @note > + If any changes made don't forget to duplicate them to > + Old_rows_log_event as long as it's supported. > + > + @param file pointer to IO_CACHE > + @param print_event_info pointer to print_event_info specializing > + what out of and how to print the event > + @param name the name of a table that the event operates on > + > + The function signals on any error of cache access through setting > + that cache's @c error to -1. > +*/ > void Rows_log_event::print_helper(FILE *file, > PRINT_EVENT_INFO *print_event_info, > char const *const name) > { > IO_CACHE *const head= &print_event_info->head_cache; > IO_CACHE *const body= &print_event_info->body_cache; > + bool do_print_encoded= > + print_event_info->base64_output_mode != BASE64_OUTPUT_DECODE_ROWS && > + !print_event_info->short_form; > + > if (!print_event_info->short_form) > { > bool const last_stmt_event= get_flags(STMT_END_F); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02587f61f40: MDEV-13095 Implement User Account locking
by Sergei Golubchik 21 Jan '19

21 Jan '19

Hi, Robert! On Jan 21, Robert Bindar wrote: > Hi Sergei! Thanks for reviewing this :-) > > > > You could simply add accessors like > > > > longlong get_password_lifetime() const > > { > > Field *f= get_field(end_priv_columns + 11, MYSQL_TYPE_LONG); > > return f ? f->val_int() : 0; > > } > > > > without changing anything else in User_table_tabular. > > Correct me if I'm mistaken. Let's assume we index like that, > > then get_is_role will become: > > bool get_is_role() const > { > Field *f = get_field(end_priv_columns + 14, MYSQL_TYPE_ENUM); > return f ? f->val_int() - 1 : 0; > } > > If we'll start the server with a 10.4.1 datadir for instance, > get_is_role will go beyond num_fields() and thus will always return > false. From my understanding so far, this means we won't be able to > support previous MariaDB datadirs without upgrading. Is this the > intended behavior? You've missed that I also suggested _not_ to add any more columns to the mysql.user table :) So you can never have is_role and password_lifetime in the same table. It's either 5.7 structure or 10.3 structure. And get_is_role() stays as it was: bool get_is_role() const { Field *f = get_field(end_priv_columns + 11, MYSQL_TYPE_ENUM); return f ? f->val_int() - 1 : 0; } That's exactly what I meant by "without changing anything else in User_table_tabular" Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02587f61f40: MDEV-13095 Implement User Account locking
by Sergei Golubchik 21 Jan '19

21 Jan '19

Hi, Robert! On Jan 20, Robert Bindar wrote: > revision-id: 02587f61f40 (mariadb-10.4.1-100-g02587f61f40) > parent(s): 937c90ce2d3 > author: Robert Bindar <robert(a)mariadb.org> > committer: Sergei Golubchik <serg(a)mariadb.com> > timestamp: 2019-01-20 00:33:44 +0100 > message: > > MDEV-13095 Implement User Account locking > > Add server support for user account locking. > This patch adds two new statements for denying a user's subsequent login > attempts: > LOCK USER[S] > user_name [, user2_name ] ... > UNLOCK USER[S] > user_name [, user2_name ] ... This doesn't seem to be correct anymore :) > The SHOW GRANTS command was updated to support locking state > information. > > diff --git a/mysql-test/include/switch_to_mysql_user.inc b/mysql-test/include/switch_to_mysql_user.inc > index f5801db6114..eff1d98c2df 100644 > --- a/mysql-test/include/switch_to_mysql_user.inc > +++ b/mysql-test/include/switch_to_mysql_user.inc > @@ -45,6 +45,9 @@ CREATE TABLE mysql.user ( > plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, > authentication_string TEXT NOT NULL, > password_expired ENUM('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, > + password_last_changed datetime NULL, > + password_lifetime int(20) unsigned NULL, > + account_locked enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, No-no. mysql.user structure was frozen in time the moment I pushed the commit with the new structure. It doesn't change anymore. To test 5.7 support, just add ALTER TABLE in one specific test file. See main/system_mysql_db_507.test > is_role enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, > default_role char(80) binary DEFAULT '' NOT NULL, > max_statement_time decimal(12,6) DEFAULT 0 NOT NULL, > diff --git a/mysql-test/main/lock_user.result b/mysql-test/main/lock_user.result > --- /dev/null > +++ b/mysql-test/main/lock_user.result > @@ -0,0 +1,132 @@ > +connect con2,localhost,user2; > +disconnect con2; > +connection default; > +alter user user1@localhost account unlock; > +# > +# A user shouldn't be able to lock itself out "lock himself out" And why not? A user can drop himself or change the password to some garbage. It's the same as locking or worse. > +# > +create user newuser@localhost; > +grant CREATE USER on *.* to newuser@localhost; > +connect con1,localhost,newuser; > +connection con1; > +alter user newuser@localhost account lock; > +ERROR HY000: Operation ALTER USER failed for 'newuser'@'localhost' > +disconnect con1; > +connection default; > +drop user newuser@localhost; ... > +alter user user1@localhost account lock; > +show grants for user1@localhost; > +Grants for user1@localhost > +GRANT USAGE ON *.* TO 'user1'@'localhost' > +ALTER USER 'user1'@'localhost' ACCOUNT LOCK No, I don't think so. It's MySQL compatibility feature, make it MySQL compatible. And it's just not very logical to put ALTER USER in SHOW GRANTS. So, SHOW GRANTS should not show ACCOUNT LOCK, but SHOW CREATE USER should. > diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql > index f788f5d67d5..01d686ac345 100644 > --- a/scripts/mysql_system_tables.sql > +++ b/scripts/mysql_system_tables.sql > @@ -79,7 +79,10 @@ CREATE DEFINER=root@localhost SQL SECURITY DEFINER VIEW IF NOT EXISTS user AS SE > CAST(IFNULL(JSON_VALUE(Priv, '$.max_user_connections'), 0) AS SIGNED) AS max_user_connections, > IFNULL(JSON_VALUE(Priv, '$.plugin'), '') AS plugin, > IFNULL(JSON_VALUE(Priv, '$.authentication_string'), '') AS authentication_string, > - 'N' AS password_expired, > + ELT(IFNULL(JSON_VALUE(Priv, '$.password_expired'), 0) + 1, 'N', 'Y') AS password_expired, > + CAST(JSON_UNQUOTE(JSON_EXTRACT(Priv, '$.password_last_changed')) AS DATETIME) AS password_last_changed, > + CAST(JSON_VALUE(Priv, '$.password_lifetime') AS UNSIGNED) AS password_lifetime, > + ELT(IFNULL(JSON_VALUE(Priv, '$.account_locked'), 0) + 1, 'N', 'Y') AS account_locked, No-no. mysql.user is frozen, it doesn't change anymore. It stays always as it was in 10.3. This was the whole point of moving to JSON, to never ever add new columns to mysql.user table. > 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 > diff --git a/scripts/mysql_system_tables_fix.sql b/scripts/mysql_system_tables_fix.sql > index 82ec2faa12d..5442e5fee73 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -643,12 +643,16 @@ ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, > ALTER TABLE user MODIFY plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL, > MODIFY authentication_string TEXT NOT NULL; > ALTER TABLE user ADD password_expired ENUM('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; > +ALTER TABLE user ADD password_last_changed datetime NULL after password_expired; > +ALTER TABLE user ADD password_lifetime int(20) unsigned NULL after password_last_changed; > +ALTER TABLE user ADD account_locked enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL after password_lifetime; But that's okay, because otherwise CREATE ... SELECT below won't work. > ALTER TABLE user ADD is_role enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; > ALTER TABLE user ADD default_role char(80) binary DEFAULT '' NOT NULL; > ALTER TABLE user ADD max_statement_time decimal(12,6) DEFAULT 0 NOT NULL; > -- Somewhere above, we ran ALTER TABLE user .... CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin. > -- we want password_expired column to have collation utf8_general_ci. > ALTER TABLE user MODIFY password_expired ENUM('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; > +ALTER TABLE user MODIFY account_locked enum('N', 'Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL; I don't think it's needed, we're dropping the table anyway. > -- Checking for any duplicate hostname and username combination are exists. > @@ -804,6 +808,10 @@ IF 'BASE TABLE' = (select table_type from information_schema.tables where table_ > 'max_statement_time', max_statement_time, > 'plugin', if(plugin>'',plugin,if(length(password)=16,'mysql_old_password','mysql_native_password')), > 'authentication_string', if(plugin>'',authentication_string,password), > + 'password_expired', 'Y'=password_expired, > + 'password_last_changed', password_last_changed, > + 'password_lifetime', password_lifetime, It would be better to put password expiration changes in the password expiration commit. > + 'account_locked', 'Y'=account_locked, > 'default_role', default_role, > 'is_role', 'Y'=is_role)) as Priv > FROM user; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index fe6fc9148bd..e4fc96b61cd 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -798,12 +806,25 @@ class Grant_table_base > else if (start_priv_columns) > break; > } > + > + end_security_columns= end_priv_columns; > + for (; end_security_columns < num_fields(); end_security_columns++) > + { > + Field *field= m_table->field[end_security_columns]; > + const char *colname= field->field_name.str; > + if (field->real_type() == MYSQL_TYPE_ENUM && > + !strcmp(colname, "is_role")) > + break; > + } Are you trying to make it super-generic, like with privileges, for an arbitrary number of security columns? Don't bother, 5.7 was the last MySQL version with privilege tables, they won't be adding more "security columns" to it. You could simply add accessors like longlong get_password_lifetime() const { Field *f= get_field(end_priv_columns + 11, MYSQL_TYPE_LONG); return f ? f->val_int() : 0; } without changing anything else in User_table_tabular. > } > > /* The index at which privilege columns start. */ > uint start_priv_columns; > /* The index after the last privilege column */ > uint end_priv_columns; > + /* The index after the last security column, i.e. index of is_role. This index > + is equal to num_fields if the is_role column does not exist */ > + uint end_security_columns; > > TABLE *m_table; > }; > @@ -8758,7 +8838,6 @@ static ROLE_GRANT_PAIR *find_role_grant_pair(const LEX_CSTRING *u, > return (ROLE_GRANT_PAIR *) > my_hash_search(&acl_roles_mappings, (uchar*)pair_key.ptr(), key_length); > } > - > static bool show_role_grants(THD *thd, const char *username, restore the empty line please > const char *hostname, ACL_USER_BASE *acl_entry, > char *buff, size_t buffsize) > @@ -10555,6 +10634,52 @@ int mysql_alter_user(THD* thd, List<LEX_USER> &users_list) > DBUG_RETURN(result); > } > > +/* > + Mark an user account as locked or unlocked. > + > + SYNOPSIS Doxygen comments. This style that you've copied is from (iirc) before 2005:) > + lock_user_account() > + thd The current thread. > + user_table A TL_WRITE opened User_table > + lex_user The user to lock/unlock. > + acl_user The acl user to be updated > + > + RETURN > + > 0 Error. Error message already sent. > + 0 OK. > +*/ > +static int lock_user_account(THD* thd, const User_table &user_table, > + LEX_USER *lex_user, ACL_USER *acl_user) > +{ > + bool update_account_locking= thd->lex->account_options.update_account_locking; > + bool account_locked_value= thd->lex->account_options.account_locked_value; > + > + DBUG_ENTER("lock_user_account"); > + > + if (!update_account_locking) > + DBUG_RETURN(0); > + > + mysql_mutex_assert_owner(&acl_cache->lock); > + > + /* > + Do not allow the current user to lock itself out. > + */ > + ACL_USER *current_acl_user= find_user_wild(thd->security_ctx->priv_host, > + thd->security_ctx->priv_user); > + if (!current_acl_user || > + (!strcmp(lex_user->user.str, current_acl_user->user.str) && > + !my_strcasecmp(system_charset_info, lex_user->host.str, > + current_acl_user->host.hostname))) > + { > + DBUG_RETURN(1); > + } > + > + acl_user->account_locked= account_locked_value; > + > + user_table.set_account_locked(account_locked_value); > + > + DBUG_RETURN(0); > +} > > static bool > mysql_revoke_sp_privs(THD *thd, Grant_tables *tables, const Sp_handler *sph, > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 8f62dca4aec..6faa5966d29 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1148,6 +1148,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); > Non-reserved keywords > */ > > +%token <kwd> ACCOUNT_SYM comment /* MYSQL */ > %token <kwd> ACTION /* SQL-2003-N */ > %token <kwd> ADMIN_SYM /* SQL-2003-N */ > %token <kwd> ADDDATE_SYM /* MYSQL-FUNC */ > +opt_account_option: > + ACCOUNT_SYM LOCK_SYM > + { > + Lex->account_options.update_account_locking= true; > + Lex->account_options.account_locked_value= true; > + } > + | ACCOUNT_SYM UNLOCK_SYM > + { > + Lex->account_options.update_account_locking= true; > + Lex->account_options.account_locked_value= false; Why wouldn't you make it a 3-state enum { NOT_SPECIFIED, LOCKED, UNLOCKED } ? looks more natural here. > + } > + ; > + > ev_alter_on_schedule_completion: > /* empty */ { $$= 0;} Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] Fwd: ce4f8955d8d: MDEV-17082 Application period tables: CREATE
by Nikita Malyavin 17 Jan '19

17 Jan '19

---------- Forwarded message --------- From: Nikita Malyavin <nikitamalyavin(a)gmail.com> Date: пт, 18 янв. 2019 г. в 01:46 Subject: Re: ce4f8955d8d: MDEV-17082 Application period tables: CREATE To: Sergei Golubchik <serg(a)mariadb.org> Hi, Sergei! > > +# SQL17 p.832 > > +# Let IDCN be an implementation-dependent <constraint name> that is not > > +# equivalent to the <constraint name> of any table constraint descriptor > > +# included in S. > > Not enough context. It was quite difficult to find this quote. > > When you're quoting the standard, please, specify the part name and the > section name. And there's no need to specify a page. For example: > > SQL2016, Part 2, 11.27 <add table period definition>, General Rules, 2)a) > Fixed, as well as all the rest cites > > + Table_period_info("SYSTEM_TIME", sizeof "SYSTEM_TIME" - 1), > > We have a macro for that, use > > Table_period_info(STRING_WITH_LEN("SYSTEM_TIME")) > ✅ > > + then no promition is done. > > promotion ✅ > > + Table_period_info& get_table_period_info() > > + { > > + return create_info.period_info; > > + } > > really? :) you've introduced a new method get_table_period_info > only to use it two lines below and nowhere else? > Not really, of course. But agree, it's not worth to have such a shortcut. > > + > > + int add_period(Lex_ident name, Lex_ident_sys_st start, Lex_ident_sys_st end) > > + { > > + Table_period_info &info= get_table_period_info(); > > + if (info.is_set()) > > + { > > + my_error(ER_PERIOD_MAX_COUNT_EXCEEDED, MYF(0), "application"); > > + return 1; > > + } > > + info.set_period(start, end); > > + info.name= name; > > + > > + Virtual_column_info *constr= new Virtual_column_info(); > > + constr->expr= lt_creator.create(thd, create_item_ident_nosp(thd, &start), > > + create_item_ident_nosp(thd, &end)); > > + add_constraint(&null_clex_str, constr, false); > > + return 0; > > + } > > + > > sp_package *get_sp_package() const; > > > > /** > > diff --git a/sql/table.cc b/sql/table.cc > > index ab1a04ccae4..87fa1ba4d61 100644 > > --- a/sql/table.cc > > +++ b/sql/table.cc > > @@ -1228,6 +1228,14 @@ static const Type_handler *old_frm_type_handler(uint pack_flag, > > } > > > > > > +bool TABLE_SHARE::init_period_from_extra2(period_info_t &period, > > + const uchar *data) > > +{ > > + period.start_fieldno= uint2korr(data); > > + period.end_fieldno= uint2korr(data + sizeof(uint16)); > > not "sizeof(uint16)" but 2. I don't think it's guaranteed that > sizeof(uint16) == 2, but as you do uint2korr, you only read 2 bytes, > so data + 2 is always correct. > Yes, it's correct, and I've fixed it, but it wasn't about coorrectness -- but readability. Now what 4, or what is 8 -- is the question that the reader could сonsider. This numbers look magical. With using sizeof(uint16) the one could at least understand, that this is about several double-byte words read. The another problem is that sizeof(uint16) is already used in system versioning, few lines below, so the resulting code will look patchy here. > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > > + f->field_name.str, "VIRTUAL or GENERATED"); > > just use "GENERATED", no English in the error message parameters, please. > VIRTUAL/GENERATED? What'd You say? > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > > + f->field_name.str, "NULL"); > > why is that? it's unrelated and inconsistent with cases like > > create table t1 (a int null, primary key (a)); > > where a column is silently created NOT NULL. > So, you should ignore explicit NULL and create columns NOT NULL. > > If you want to change this, it should be consistent in all cases. > create an MDEV, suggest there that the statement above should be an error. > but I don't think it's worth spending time on now. > Ok, I understand. Fixed. > > + else if (f->flags & VERS_SYSTEM_FIELD) > > + { > > + my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), f->field_name.str, > > + f->flags & VERS_SYS_START_FLAG ? "ROW START" : "ROW END"); > > No need to have a special case for that. Use the same error as for vcol_info: > > if (f->vcol_info || f->flags & VERS_SYSTEM_FIELD) > { > my_error(ER_PERIOD_FIELD_WRONG_ATTRIBUTES, MYF(0), > f->field_name.str, "GENERATED ALWAYS AS"); > Looks not very much user-friendly I think. Let's leave the verbosity as it was > What does this error have to do with MDEV-16976? > SQL standard clearly says (part 2, 11.3 <table definition>, syntax rules, 2)d) > > d) No <column name> in any <column definition> shall be equivalent to PN. > > (here PN is "period name"). So MDEV-16976 or not, a period name should > never match a column name. > I was thinking to implement it through virtual column. Then this check will become redundant, and we'll get duplicate field error > > + eng "Temporary tables with application period not allowed" > > "application time period" > ✅ > > + > > +ER_PERIOD_FIELD_NOT_FOUND > > + eng "Field %`s in PERIOD FOR %`s is not found in table" > > I don't think this needs a new error message, just use ER_BAD_FIELD_ERROR > ✅ > > + > > +ER_PERIOD_TYPES_MISMATCH > > + eng "PERIOD FOR %`s fields types mismatch" > > + > > +ER_PERIOD_MAX_COUNT_EXCEEDED > > better say ER_MORE_THAN_ONE_PERIOD or something... > "max count exceeded" suggests there is some not-trivial > (and possibly variable) max count > > Or ER_PERIOD_ALREADY_DEFINED or ER_DUPLICATE_PERIOD etc. > "already defined" looks like a good one. > > And I've already commented (in another patch) on using English > words as arguments - it will look like a bug when the error message is > translated > I like ER_MORE_THAN_ONE_PERIOD more. IMO application-time periods designed in the way to support a possibility of several periods definition, so i like to treat it as "N periods max supported, but now N=1" > > + eng "cannot specify more than one %s-time period" > > + > > +ER_PERIOD_NAME_IS_NOT_ALLOWED_FOR_FIELD > > + eng "Could not specify name %`s for field. It is already used by period." > > I don't think this needs a new error message, just use ER_DUP_FIELDNAME > ✅ > > + > > +ER_PERIOD_FIELD_WRONG_ATTRIBUTES > > + eng "Period field %`s cannot be %s" > > + > > +ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD > > + eng "Field %`s is specified for more than one period" > > This looks unused ✅ Sincerely Nikita Malyavin

1 0

[Maria-developers] Please review MDEV-18195 ASAN use-after-poison in my_strcasecmp_utf8 / Item::eq upon prepared statement with ORDER BY NAME_CONST
by Alexander Barkov 17 Jan '19

17 Jan '19

Hi Sanja, Please review a patch for MDEV-18195. Thanks.

1 0

Re: [Maria-developers] d08031dfae2: MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK
by Sergei Golubchik 17 Jan '19

17 Jan '19

Hi, Thirunarayanan! On Jan 16, Thirunarayanan Balathandayuthapani wrote: > revision-id: d08031dfae2 (mariadb-10.0.37-45-gd08031dfae2) > parent(s): 12f362c3338 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2019-01-16 15:46:28 +0530 > message: > > MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique index nominated as PK > > Problem: > ======== > Server fails to notify the engine by not setting the ADD_PK_INDEX and > DROP_PK_INDEX When there is a > i) Change in candidate for primary key. > ii) New candidate for primary key. > > Fix: > ==== > Server sets the ADD_PK_INDEX and DROP_PK_INDEX while doing alter for the > above problematic case. Sorry, that's way too complex. I mean the original code with candidate_key_count, is_candidate_key, and all that. And you're making it more complex. If a unique key is promoted to a primary key, this unique key is always be the first key in the table. So what you need to check is * if the old table didn't have a primary key and the new table has it, then a primary key was added. As easy as if (table->s->primary_key >= MAX_KEY && altered_table->s->primary_key != MAX_KEY) * The opposite case, a primary key was removed * if both tables had a primary key, but the key number 0 in the new table was not key number 0 in the old table - a primary key was changed: if (key_info->usable_key_parts) ... And that should cover all possible cases, as far as I can see. May need some tweaking for key_info->usable_key_parts of newly added indexes, I'm not quite sure what number they get. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] (no subject)
by varsha kukreja 12 Jan '19

12 Jan '19

Hey this is Varsha Kukreaj a third year student from SPITMumbai ..I would like to contribute to the open source community .Can someone help me get started?..I have majorly worked on backend on 2 college sponsored projects and working currently on a project by Government Organization. I have fair knowledge in Javascript, NodeJs, Typescript, Spring Boot, Laravel , Docker and apache thrift. It woukd be great if could if someone could help me get started

2 1

Re: [Maria-developers] MDEV 18092
by Anel Husakovic 10 Jan '19

10 Jan '19

Hi Alexander, I was on vacation too, just came back, thank you for teaching me. Indeed it works, here is a patch with quicker solution and the test case *https://github.com/an3l/server/commit/252ca79493e18fa8fd364992f5022af8a57b79d5 <https://github.com/an3l/server/commit/252ca79493e18fa8fd364992f5022af8a57b7…>* I added the PR also, so if something needs to be improved please let me know. In addition I will try later also to add `store_schema_params()` function to handler. Thanks, Anel On Mon, Dec 31, 2018 at 7:02 AM Alexander Barkov <bar(a)mariadb.com> wrote: > Hi Anel, > > > > On 12/28/2018 02:12 PM, Anel Husakovic wrote: > > Hi Alexander, > > my name is Anel, and I'm working for Foundation. > > I would like to take/ https://jira.mariadb.org/browse/MDEV-18092/ if > > you allow me, and would appreciate any hint in order to learn more about > > this codebase :). > > Nice to meet you! Thanks for working on this! > > > I debugged already a bit and have seen that there is parser error, > > where /*parser_state = m_buf = 0x7fff6403ec90 "CREATE DEFINER=\"\" > > PACKAGE BODY \"db1\".\"employee_tools\" BEGIN END"/ > > is raised because the incorrect body of /Sp_handler_package_spec > > /and/or /Sp_handler_package_body /I suppose ? > > (/empty_body_lex_cstring/() for *Sp_handler_package_spec *and > > *Sp_handler_package_body*), > > > > I tried to change the body of the above methods, but either my syntax is > > not good ("/AS BEGIN NULL; END/") or this is incorrect conclusion :). > > > It seems Sp_handler::sp_load_for_information_schema() > creates a wrong CREATE statement that cannot be parsed. > > This should be fixed. sp_load_for_information_schema() is > supposed to build a simplified CREATE statement that can be parsed. > > > But for exactly this problem, it's probably not necessarily. > > > I think store_schema_params() should just skip PACKAGE and PACKAGE BODY > records quickly. > > These stored objects do not have any parameters or return values > (only procedures and functions have). > So no needs to build a CREATE statement and parse it: > this won't give us any data useful for INFORMATION_SCHEMA.PARAMETERS > anyway. > > > > Ideally, store_schema_params() should be turned into virtual > methods in Sp_handler, doing: > - empty job for PACKAGE and PACKAGE BODY > - adding a return data type record for FUNCTION > - adding parameters for FUNCTION and PROCEDURE > > > A quicker solution would be just to change this block: > > if (!sph) > DBUG_RETURN(0); > > > to something like this: > > if (!sph || > sph->type() == TYPE_ENUM_PACKAGE || > sph->type() == TYPE_ENUM_PACKAGE_BODY) > DBUG_RETURN(0); > > > <skip> > > > Thanks in advance and happy holidays, > > Thanks! Same to you! > > > I'll be on holidays until the 10th of January. > Sorry for possible slow replies during these days. > > > > > > Anel >

2 1

Re: [Maria-developers] Expiration of user passwords
by Sergei Golubchik 08 Jan '19

08 Jan '19

Hi, Robert! On Jan 07, Robert Bindar wrote: > Hi! > > With this email I intend to kick off a debate on the design of > password expiration in MariaDB. > Given this feature may impact the end user experience, I would love if > the experienced folks here will step in and help us come up with the > best approach possible. Random thoughts are highly appreciated as > well. > > *-- Some thoughts* > > I have been trying to wrap my head around how password management > should look like in MariaDB, my conclusion so far is that the less > intrusive it is the greater are our chances for the common user to > like it. > > MySQL released the first version of password expiration in 5.6.6 > (~2012, didn't change much since then). From my research, it seems the > common user was caught off-guard by this feature, got annoyed by its > restrictiveness and chose to disable it permanently. I believe we > should avoid this situation as much as possible. > I can see the whole story going, nevertheless, in a different > direction for our business clients. I suspect the password expiration is more useful in satisfying various enterprise security checklists and requirements than in actually improving security. For example, https://cryptosmith.com/password-sanity/exp-harmful/ http://blog.axantum.com/2009/12/password-expiration-is-meaningless.html etc, you can google up as many as you'd like > **-- How MySQL does it* > > This is a short summary of what's in the MySQL docs about the password > management system. I'll have it here so people that are unaware of how > this feature works in MySQL are up to speed. > > MySQL introduced 3 new columns in mysql.user: > > /** *//*password_expired* (Y/N)// > // /** If 'Y', the user's password will be considered expired */// > //** **password_last_changed* (timestamp)// > // /** The datetime when the user changed the password last time */// > //** **password_lifetime* (smallint)// > // /** The number of days until the password expires (it's not clear whether// > // the days are counted since password_last_changed or since when the// > // expiration statement was executed) *// > > Given we support MySQL 5.7, we have to have them as well I believe. > > The /CREATE USER/ and /ALTER USER/ statements were extended as follows:** > > *** /[ALTER|CREATE] USER user@host PASSWORD EXPIRE [option];/ > > - If no option is specified, the password is expired with immediate effect. > - If option is /DEFAULT/, the /*password_lifetime*/ column is set to the > number of days specified in the */default_password_lifetime/* system var. > - If option is /NEVER/, the user password_lifetime column is set to 0, the > password is never expired for this account. > - Option can also be /INTERVAL n DAY/, in which case password_lifetime > is set to /'n'/ (see column definition above). > > *Password expiration behavior* > MySQL controls the effect of an expired password with the > */disconnect_on_expired_password/* system var. > For active sessions, when a password is expired, the server either > disconnects the client or puts it in a sandbox mode. > New client connections can specify an option so the server knows not to > disconnect them, otherwise the new connection is handled according to the > system var above. > In sandbox mode, the client is able to execute only /SET PASSWORD/, > /ALTER USER/ and /SET/ statements for vars. > > *Other additions* > - C library API for specifying disconnect/sandbox option this should be already done, CONC-314. > - mysql client new */--connect-expired-password/* option > * > **-- Things to improve* > > Below are a few points I found so far that I believe should be discussed > before moving on to a prototype implementation: > > *1.* Syntax wise > > Do we want to follow the path of MySQL and extend the /ALTER USER/ and > /CREATE USER/ statements for password expiration support? > *Opinion**|* The /ALTER/CREATE USER/ statements seem to be doing quite a lot > of things already. Introducing a new statement such as > > * * */PASSWORD EXPIRE user@host [option];/ > > may be worth exploring. I think ALTER/CREATE USER is perfectly expected to set or modify various properties of an account. So it's quite fitting to use it here. "PASSWORD EXPIRE" looks strange. First, SQL statements start with a verb. Second "PASSWORD EXPIRE" (or "EXPIRE PASSWORD", if you'd like) means to expire it now, not somewhere in the future. And anyway, I don't see why one would need a new statement to modify just one property of an account, if all other properties are modified with ALTER USER. > *2.* Password Expiration behavior > > Is it actually a good idea to overengineer the feature with > sandbox/disconnect modes? > *Opinion|* Quite a lot of people are complaining on online platforms > that their SQL statements stopped working suddenly or that they can't > connect to the MySQL server anymore. What about we just ask for a new > password when the next login is attempted? We have ways to disconnect > a client connection anyway if needed. A good choice for the default > behavior of this feature might also do the trick. I'd disable password expiration by default, see urls above :) > *3.* Having passwords expire on a specific date > > MySQL introduced the concept of expiring the passwords after /N/ days. > I can see a good use-case in wanting to expire an account's password > on a specific date/timestamp as well: > > /ALTER USER user1@host PASSWORD EXPIRE DATE tstamp;/ > or > /PASSWORD EXPIRE user1@host DATE tstamp;/ > > Example use cases: > * Let's assume company X has a security sanity check on 10th of November > and wants everybody to update their passwords that day. > * Company Y wants the password expiration event to happen before the > working day starts and thus not interrupt their employees during their > productive hours. Now, that is overengineering, in my opinion :) Let's postpone it until users will be requesting it. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0