Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
Hi, Aleksey,
Thanks. I'll send a separate review email, there will be only replies
here:
On Apr 05, Aleksey Midenkov wrote:
> Hi, Sergei!
>
> > > @@ -5709,8 +5708,6 @@ bool Item_field::fix_fields(THD *thd, Item
> > > **reference)
> > >}
> > > #endif
> > >fixed= 1;
> > > - if (field->vcol_info)
> > > -fix_session_vcol_expr_for_read(thd, field, field->vcol_info);
> >
> > Did you revert the optimization from 7450cb7f69db?
> > It was "re-fix vcols on demand, not always for every SELECT"
> >
> > and as far as I can see now you again always re-fix everything,
> > is that so?
>
> It was your suggestion in the previous review to refix always
> (therefore everything). The original fix worked with
> fix_session_vcol_expr_for_read(). But the current fix is simpler and
> cleaner. Was there a real performance problem or was it a theoretical
> issue?
I checked previous emails and wasn't able to find where I suggested to
refix always.
Anyway, it was indeed a theoretical issue and I have no proofs to claim
that that "optimization" made a difference. So, okay, feel free to refix
always.
> > >if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY &&
> > >!outer_fixed &&
> > >select &&
> > > diff --git a/sql/table.h b/sql/table.h
> > > index 38b63d285c6..ab1d96538c0 100644
> > > --- a/sql/table.h
> > > +++ b/sql/table.h
> > > @@ -1374,8 +1374,13 @@ struct TABLE
> > >*/
> > >bool auto_increment_field_not_null;
> > >bool insert_or_update; /* Can be used by the handler */
> > > + /*
> > > + NOTE: alias_name_used is only a hint! It works only in
> > > need_correct_ident()
> > > + condition. On other cases it is FALSE even if table_name is alias!
> >
> > As I asked earlier, do you have any test case that proves this claim?
>
> I replied that MDEV-25672 bug's own test case proves that, didn't I?
>
> (rr) p alias_name_used
> $4 = false
> (rr) p table_name
> $5 = 0x7fdc28024790 "x"
>
> #0 Item_field::set_field (this=0x7fdc28012e60,
> field_par=0x7fdc28024440) at /home/midenok/src/mariadb/10
> .3/src/sql/item.cc:3289
> #1 0x00b7bc28 in Item_field::fix_fields (this=0x7fdc28012e60,
> thd=0x7fdc28000d28, reference=0x7fdc28034ad8) at
> /home/midenok/src/mariadb/10.3/src/sql/item.cc:6332
> ...
> #15 0x007c65f6 in mysql_parse (thd=0x7fdc28000d28,
> rawbuf=0x7fdc28010520 "UPDATE `test_table` as
> `x` SET order_date = NULL", length=48, parser_state=0x7fdc39d09548,
> is_com_multi=false, is_next_command=false) at
> /home/midenok/src/mariadb/10.3/src/sql/sql_parse.cc:7870
indeed, thanks. Could you add it to the comment? Like "e.g. in update t1 as x
set a = 1"
> > > + */
> > >bool alias_name_used; /* true if table_name is alias */
> > >bool get_fields_in_item_tree; /* Signal to fix_field */
> > > + List vcol_cleanup_list;
> > >CHARSET_INFO *save_character_set_client=
> > > thd->variables.character_set_client;
> > >CHARSET_INFO *save_collation= thd->variables.collation_connection;
> > >Query_arena *backup_stmt_arena_ptr= thd->stmt_arena;
> > > @@ -2831,36 +2841,162 @@ static bool fix_vcol_expr(THD *thd,
> > > Virtual_column_info *vcol)
...
> > > +bool Vcol_expr_context::init()
> > > +{
> > > + /*
> > > + As this is vcol expression we must narrow down name resolution to
> > > + single table.
> > > + */
> > > + if (init_lex_with_single_table(thd, table, &lex))
> >
> > Do you have a test that fails otherwise?
>
> That fails at least 3 tests:
>
> main.default vcol.vcol_syntax gcol.gcol_bugfixes
>
> CURRENT_TEST: main.default
> mysqltest: At line 1302: query 'INSERT INTO t1 VALUES( DEFAULT )'
> failed: 2013: Lost connection to MySQL server during query
doesn't crash for me
(after building your branch and commenting out lex swapping)
> CURRENT_TEST: vcol.vcol_syntax
> mysqltest: At line 118: query 'select * from t1' failed: 2013: Lost
> connection to MySQL server during query
doesn't crash for me. I got a warning about bad double value 'x'
likely because count_cuted_fields being initialized differently in
a new lex or something.
> CURRENT_TEST: gcol.gcol_bugfixes
> mysqltest: At line 579: query 'INSERT INTO t1 (suppliersenttoday)
> VALUES (0)' failed: 2013: Lost connection to MySQL server during q
Re: [Maria-developers] 14cc679c95e: MDEV-27831 Let the SQL SERVICE user set the current user name.
Hi, Alexey, On Apr 04, Alexey Botchkov wrote: > I still like my approach. > > > A plugin name is already known inside the plugin, > > the server should determine it automatically. > > Firstly I don't see any good way for the service to know the name of > the plugin that called the mysql_real_connect_local. I don't see either. I can only think of something like #define plugin_name "spider" (for example), or const char * const plugin_name = "spider"; and #define mysql_real_connect_local(M) sql_service->mysql_real_connect_local_func(M, plugin_name) but it's not per plugin it's per *.so. If one .so would have many plugins, they'll all will have the same "plugin_name" and I have no solution for that. So it's a rather lousy solution, and I hoped you could come up with something better :) > Technically this call doesn't even have to be hard linked to a plugin. > Can be just done by a part of the server. The server doesn't have to use *plugin services*, services are an API for plugins to use. > Then why limit the plugin like this? That doesn't add much to the > security as the plugin can replace that string anyway. Not for security. It's to avoid boilerplate, to not force plugins to tell the server what the server already knows. > Also one plugin can have more than one connection and > I can imagine that different usernames for these connections make sence. well, the point was to identify what plugin makes the call. and almost always it will be a plugin name. > > current_user is the name of the user account and it's used in > > many places as such. Try, for example, to create a view or a stored > > procedure. Who will be a definer? > If not specified, the definer is going to be username@''. > And as a result the view or the procedure will be not functional. > But i think it's rather correct. The user of the SQL service has to specify > the definer explicitly. No, using pluginname@'' can hardly be correct. ''@'' is more reasonable. And it's not only definer, it's what CURRENT_USER shows, it's what privileges are used. Surely, not privileges of the pluginname@'' account, there is no such account. So CURRENT_USER should not be pluginname@''. > > Setting only user() might be ok. > I can agree with that. Setting the ctx->user only. > In this case the DEFINER of the view/procedure is going to be empty > if not explicitly specified. > Though don't see any advantage to what is now. USER() is purely informational. CURRENT_USER() is not, it has a clearly defined meaning, it's the name of the account that the privilege system uses. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
ode= 0;
> +
> + /*
> + NOTE: we refix also tmp tables used in ALTER TABLE,
Why? I don't see how a tmp table from ALTER can possibly
be used in two different statements. There should be no need
to re-fix it. Just fixing once, on open, should be enough.
> + they have (pos_in_table_list == NULL).
> + */
> + TABLE_LIST const *tl= table->pos_in_table_list;
> + DBUG_ASSERT(tl || table->s->tmp_table);
> +
> + if (tl && tl->security_ctx)
> thd->security_ctx= tl->security_ctx;
> - bool res= fix_session_vcol_expr(thd, vcol);
> +
> + inited= true;
> + return false;
> +}
> +
> +Vcol_expr_context::~Vcol_expr_context()
> +{
> + if (!inited)
> +return;
> + table->grant.want_privilege= old_want_privilege;
> + thd->pop_internal_handler();
> + end_lex_with_single_table(thd, table, old_lex);
> + table->map= old_map;
>thd->security_ctx= save_security_ctx;
> - DBUG_RETURN(res);
> + thd->variables.sql_mode= save_sql_mode;
> +}
> +
> +
> +bool TABLE::vcol_fix_expr(THD *thd)
> +{
> + DBUG_ASSERT(pos_in_table_list || s->tmp_table);
> + if ((pos_in_table_list && pos_in_table_list->placeholder()) ||
> + !s->vcol_need_refix)
> +return false;
> +
> + if (!thd->stmt_arena->is_conventional() &&
> + !vcol_cleanup_list.is_empty())
> + {
> +/* NOTE: Under trigger we already have fixed expressions */
> +return false;
> + }
> + DBUG_ASSERT(vcol_cleanup_list.is_empty());
> +
> + Vcol_expr_context expr_ctx(thd, this);
> + if (expr_ctx.init())
> +return true;
> +
> + for (Field **vf= vfield; vf && *vf; vf++)
> +if ((*vf)->vcol_info->fix_session_expr(thd, this))
> + goto error;
> +
> + for (Field **df= default_field; df && *df; df++)
> +if ((*df)->default_value &&
> +(*df)->default_value->fix_session_expr(thd, this))
> + goto error;
> +
> + for (Virtual_column_info **cc= check_constraints; cc && *cc; cc++)
> +if ((*cc)->fix_session_expr(thd, this))
> + goto error;
> +
> + return false;
> +
> +error:
> + DBUG_ASSERT(thd->get_stmt_da()->is_error());
> + return true;
> +}
> +
> +
> +bool TABLE::vcol_cleanup_expr(THD *thd)
> +{
> + if (vcol_cleanup_list.is_empty())
> +return false;
> +
> + List_iterator it(vcol_cleanup_list);
> + bool result= false;
> +
> + while (Virtual_column_info *vcol= it++)
> +result|= vcol->cleanup_session_expr();
> +
> + vcol_cleanup_list.empty();
> +
> + DBUG_ASSERT(!result || thd->get_stmt_da()->is_error());
> + return result;
> }
>
>
> @@ -2939,14 +3072,17 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE
> *table,
> */
> myf warn= table->s->frm_version < FRM_VER_EXPRESSSIONS ? ME_JUST_WARNING
> : 0;
> my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(warn),
> - "AUTO_INCREMENT", vcol->get_vcol_type_name(), res.name);
> + "AUTO_INCREMENT", get_vcol_type_name(), res.name);
> if (!warn)
>DBUG_RETURN(1);
>}
> - vcol->flags= res.errors;
> + flags= res.errors;
>
> - if (vcol->flags & VCOL_SESSION_FUNC)
> -table->s->vcols_need_refixing= true;
> + if (need_refix())
Old code only refixed VCOL_SESSION_FUNC, because those Items
can change the medata on fix_fields. See the comment for 73a220aac384
Refixing VCOL_TIME_FUNC seems to be redundant
> + {
> +table->s->vcol_need_refix= true;
> +cleanup_session_expr();
why?
> + }
>
>DBUG_RETURN(0);
> }
> @@ -3011,11 +3147,13 @@ unpack_vcol_info_from_frm(THD *thd, MEM_ROOT
> *mem_root, TABLE *table,
>if (error)
> goto end;
>
> + lex.sql_command= old_lex->sql_command;
why?
> +
>vcol_storage.vcol_info->set_vcol_type(vcol->get_vcol_type());
>vcol_storage.vcol_info->stored_in_db= vcol->stored_in_db;
>vcol_storage.vcol_info->name= vcol->name;
>vcol_storage.vcol_info->utf8= vcol->utf8;
> - if (!fix_and_check_vcol_expr(thd, table, vcol_storage.vcol_info))
> + if (!vcol_storage.vcol_info->fix_and_check_expr(thd, table))
>{
> *vcol_ptr= vcol_info= vcol_storage.vcol_info; // Expression ok
> DBUG_ASSERT(vcol_info->expr);
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] a10facb656b: MDEV-27065 Partitioning tables with custom data directories moves data back to default directory
Hi, Nayuta, On Mar 29, Nayuta Yanagisawa wrote: > revision-id: a10facb656b (mariadb-10.2.43-48-ga10facb656b) > parent(s): 33ff18627ea > author: Nayuta Yanagisawa > committer: Nayuta Yanagisawa > timestamp: 2022-03-29 23:43:16 +0900 > message: > > MDEV-27065 Partitioning tables with custom data directories moves data back > to default directory > > --- > .../suite/parts/r/partition_exch_qa_14.result | 22 > ++ > sql/ha_partition.cc| 16 > 2 files changed, 38 insertions(+) > > diff --git a/mysql-test/suite/parts/r/partition_exch_qa_14.result > b/mysql-test/suite/parts/r/partition_exch_qa_14.result > index 1420982436a..52839644f55 100644 > --- a/mysql-test/suite/parts/r/partition_exch_qa_14.result > +++ b/mysql-test/suite/parts/r/partition_exch_qa_14.result > @@ -9,6 +9,9 @@ PARTITION BY RANGE (a) > (PARTITION p0 VALUES LESS THAN (10) DATA DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-idx-dir', > PARTITION p1 VALUES LESS THAN (100) DATA DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-idx-dir', > PARTITION p2 VALUES LESS THAN (1000) DATA DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-idx-dir'); > +Warnings: > +Warning 1618 option ignored > +Warning 1618 option ignored This is very confusing. There are four DATA DIRECTORY and four INDEX DIRECTORY in the statement. Which one is ignored? (yes, *I* know, the table level one) > @@ -148,6 +154,9 @@ PARTITION BY RANGE (a) > (PARTITION p0 VALUES LESS THAN (10) , > PARTITION p1 VALUES LESS THAN (100) , > PARTITION p2 VALUES LESS THAN (1000) ); > +Warnings: > +Warning 1618 option ignored > +Warning 1618 option ignored and here DATA/INDEX DIRECTORY behaves not like other options. For a normal option table-level value would apply to all partitions. That is CREATE TABLE t1 (...) OPT="VAL (PARTITION p0 VALUES LESS THAN (10) , PARTITION p1 VALUES LESS THAN (100) , PARTITION p2 VALUES LESS THAN (1000) ); would be the same as CREATE TABLE t1 (...) (PARTITION p0 VALUES LESS THAN (10) OPT="VAL, PARTITION p1 VALUES LESS THAN (100) OPT="VAL, PARTITION p2 VALUES LESS THAN (1000) OPT="VAL); Is that correct? > CREATE TABLE tsp (a INT, b VARCHAR(55), PRIMARY KEY (a)) DATA DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-data-dir' INDEX DIRECTORY = > 'MYSQLTEST_VARDIR/mysql-test-idx-dir' ENGINE = MYISAM > PARTITION BY RANGE (a) > SUBPARTITION BY HASH(a) Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 3be1e491fb5: MDEV-27743 Remove Lex::charset
Hi, Alexander, All comments from the previous review were addressed and there were no other changes. So, ok to push. On Mar 22, Alexander Barkov wrote: > revision-id: 3be1e491fb5 (mariadb-10.6.1-383-g3be1e491fb5) > parent(s): c69defd3b61 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-03-21 21:33:30 +0400 > message: > > MDEV-27743 Remove Lex::charset > > This patch also fixes: > > MDEV-27690 Crash on `CHARACTER SET csname COLLATE DEFAULT` in column > definition > MDEV-27853 Wrong data type on column `COLLATE DEFAULT` and table `COLLATE > some_non_default_collation` > MDEV-28067 Multiple conflicting column COLLATE clauses are not rejected > MDEV-28118 Wrong collation of `CAST(.. AS CHAR COLLATE DEFAULT)` > MDEV-28119 Wrong column collation on MODIFY + CONVERT > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 026e52bad84: MDEV-27743 Remove Lex::charset
Hi, Alexander,
On Mar 21, Alexander Barkov wrote:
> >> +
> >> +DELETE FROM results
> >> +WHERE cs=''
> >> + AND cl0='BINARY'
> >> + AND cl1=''
> >> + AND cl2=''
> >> + AND tcs LIKE 'CHARACTER SET%'
> >> + AND result NOT LIKE 'ERROR%'
> >> + AND result RLIKE CONCAT('COLLATE ', tcs_character_set_name, '_bin');
> >> +SELECT ROW_COUNT();
>
> I suggest too keep DELETEs.
sure, that's up to you
> >> diff --git a/sql/lex_charset.h b/sql/lex_charset.h
> >> new file mode 100644
> >> index 000..d95f0bc1920
> >> --- /dev/null
> >> +++ b/sql/lex_charset.h
> >> @@ -0,0 +1,202 @@
> > ...
> >> +#define LEX_CHARSET_COLLATION_TYPE_BITS 2
> >> + static_assert(((1<=
> >> + TYPE_COLLATE_CONTEXTUALLY_TYPED,
> >> + "Lex_charset_collation_st::Type bits check");
> >> +
> >> +protected:
> >> + CHARSET_INFO *m_ci;
> >> + Type m_type;
> >> +public:
> >> + static CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs);
> >> + static CHARSET_INFO *find_default_collation(CHARSET_INFO *cs);
> >> +public:
> > ...
> >> + void set_contextually_typed_binary_style()
> >> + {
> >> +m_ci= &my_collation_contextually_typed_binary;
> >> +m_type= TYPE_COLLATE_CONTEXTUALLY_TYPED;
> >> + }
> >> + bool is_contextually_typed_collate_default() const
> >> + {
> >> +return m_ci == &my_collation_contextually_typed_default &&
> >> + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED;
> >
> > what does it mean when only one of these two expressions is true?
> > Say, the first true, the second false?
> > Or the first false, the second true?
>
> As you know, in the final patch for "UCA-14.0.0 collations",
> UCA-14.0.0 context collations use a pointer to their utf8mb4
> CHARSET_INFO versions.
>
> For example, uca1400_as_ci uses a pointer to utf8mb4_uca1400_as_ci,
> with m_type==TYPE_COLLATE_CONTEXTUALLY_TYPED indicating that
> it the charset part of the collation is not really known yet and is a
> subject to further resolution.
I see. So it'll be possible that
m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED &&
m_ci != &my_collation_contextually_typed_default
But what about the opposite situation?
If m_ci == &my_collation_contextually_typed_default, then
m_type must be TYPE_COLLATE_CONTEXTUALLY_TYPED, correct?
I mean, it looks that this method will always return the same as yours:
bool is_contextually_typed_collate_default() const
{
return m_ci == &my_collation_contextually_typed_default;
}
as far as I understand.
> >> + }
> >> + bool is_contextually_typed_binary_style() const
> >> + {
> >> +return m_ci == &my_collation_contextually_typed_binary &&
> >> + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED;
same here
> >> + }
> > ...
> >> diff --git a/strings/ctype-bin.c b/strings/ctype-bin.c
> >> index bb746ad90b0..a5c9129ebea 100644
> >> --- a/strings/ctype-bin.c
> >> +++ b/strings/ctype-bin.c
> >> @@ -630,3 +630,69 @@ struct charset_info_st my_charset_bin =
> >> &my_charset_handler,
> >> &my_collation_binary_handler
> >> };
> >> +
> >> +
> >> +struct charset_info_st my_collation_contextually_typed_binary=
> >> +{
> >> +0,0,0,/* number*/
> >
> > I thought you will never need to resove the
> > &my_collation_contextually_typed_binary pointer, that is, I thought the
> > values in the my_collation_contextually_typed_binary struct should not
> > matter as they'll never be used. Was that wrong?
>
> Correct, I will never need to resolve.
> I just thought that using an initialized memory over a non-initialized
> memory is better.
>
> So I'd like to avoid non-initialized variables like this:
>
> struct charset_info_st my_collation_contextually_typed_binary;
>
> I agree that full detailed initializing, similar to my_charset_bin,
> is not really needed for these two contextual variables.
> So something like this should be enough:
>
> struct charset_info_st my_collation_contextually_typed_binary= {0};
>
> Should I fix this way?
I've just realized that it'd be even better to have them as pointers,
like
struct charset_info_st *my_collation_contextually_typed_binary=
(struct charset_info_st *)1;
struct charset_info_st *my_collation_contextually_typed_default=
(struct charset_info_st *)2;
you'll have unique values to compare with, but any attempt to resolve
them will crash, as some kind of a built-in assert for an unresolvable
pointer.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] d8e915a88fc: MDEV-26009 Server crash when calling twice procedure using FOR-loop
Hi, Oleksandr, ok to push also, see below On Mar 20, Oleksandr Byelkin wrote: > revision-id: d8e915a88fc (mariadb-10.3.33-32-gd8e915a88fc) > parent(s): 6a2d88c1322 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-03-18 11:13:09 +0100 > message: > > MDEV-26009 Server crash when calling twice procedure using FOR-loop > > The problem was that instructions sp_instr_cursor_copy_struct and > sp_instr_copen uses the same lex, adding and removing "tail" of > prelocked tables and forgetting that tail of all tables is kept in > LEX::query_tables_last. If the LEX used only by one instruction > or the query do not have prelocked tables it is not important. > But to work correctly in all cases LEX::query_tables_last should > be reset to make new tables added in the correct list (after last > table in the LEX instead after last table of the prelocking "tail" > which was cut). > > diff --git a/sql/sp_head.cc b/sql/sp_head.cc > index 57ab31d9edf..96dc78dcf49 100644 > --- a/sql/sp_head.cc > +++ b/sql/sp_head.cc > @@ -3486,6 +3486,11 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint > *nextp, > lex_query_tables_own_last= m_lex->query_tables_own_last; > prelocking_tables= *lex_query_tables_own_last; > *lex_query_tables_own_last= NULL; > +/* > + Here we cut "tail" of prelocked tables, so have to return last table > + to the original position (wuthout the tail > +*/ > +m_lex->query_tables_last= m_lex->query_tables_own_last; 1. the comment looks truncated 2. I don't even think this line needs a comment, this code block removes the list of prelocked tables from the query_tables list. Looking at how query_tables_last and query_tables_own_last are documented: /* Pointer to next_global member of last element in the previous list. */ TABLE_LIST **query_tables_last; /* If non-0 then indicates that query requires prelocking and points to next_global member of last own element in query table list (i.e. last table which was not added to it as part of preparation to prelocking). 0 - indicates that this query does not need prelocking. */ TABLE_LIST **query_tables_own_last; it is pretty clear that after the "tail" is removed, one has to set query_tables_last to point to the end of the list. The fact that it wasn't done was clearly an omission. That is, existing code and existing comments document it pretty well, your comment looks redundant and even confusing to me. So, please, remove the comment and ok to push > m_lex->mark_as_requiring_prelocking(NULL); >} >thd->rollback_item_tree_changes(); Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 026e52bad84: MDEV-27743 Remove Lex::charset
sult is `select * from results` (with order by, etc).
It's great, that you added that.
Everything else is redundant, I suggest to remove other selects and
deletes.
> +
> +
> +--echo #
> +--echo # End of 10.9 tests
> +--echo #
> diff --git a/sql/lex_charset.h b/sql/lex_charset.h
> new file mode 100644
> index 000..d95f0bc1920
> --- /dev/null
> +++ b/sql/lex_charset.h
> @@ -0,0 +1,202 @@
...
> +#define LEX_CHARSET_COLLATION_TYPE_BITS 2
> + static_assert(((1<=
> + TYPE_COLLATE_CONTEXTUALLY_TYPED,
> + "Lex_charset_collation_st::Type bits check");
> +
> +protected:
> + CHARSET_INFO *m_ci;
> + Type m_type;
> +public:
> + static CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs);
> + static CHARSET_INFO *find_default_collation(CHARSET_INFO *cs);
> +public:
...
> + void set_contextually_typed_binary_style()
> + {
> +m_ci= &my_collation_contextually_typed_binary;
> +m_type= TYPE_COLLATE_CONTEXTUALLY_TYPED;
> + }
> + bool is_contextually_typed_collate_default() const
> + {
> +return m_ci == &my_collation_contextually_typed_default &&
> + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED;
what does it mean when only one of these two expressions is true?
Say, the first true, the second false?
Or the first false, the second true?
> + }
> + bool is_contextually_typed_binary_style() const
> + {
> +return m_ci == &my_collation_contextually_typed_binary &&
> + m_type == TYPE_COLLATE_CONTEXTUALLY_TYPED;
> + }
...
> diff --git a/strings/ctype-bin.c b/strings/ctype-bin.c
> index bb746ad90b0..a5c9129ebea 100644
> --- a/strings/ctype-bin.c
> +++ b/strings/ctype-bin.c
> @@ -630,3 +630,69 @@ struct charset_info_st my_charset_bin =
> &my_charset_handler,
> &my_collation_binary_handler
> };
> +
> +
> +struct charset_info_st my_collation_contextually_typed_binary=
> +{
> +0,0,0,/* number*/
I thought you will never need to resove the
&my_collation_contextually_typed_binary pointer, that is, I thought the
values in the my_collation_contextually_typed_binary struct should not
matter as they'll never be used. Was that wrong?
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 2467eb2d314: MDEV-27743 Remove Lex::charset
Hi, Alexander,
I'd suggest to rename it the way you want in the server,
but keep it in libmariadb as is.
It's apparently part of the API and may be (just may be) some clients
use it.
On Mar 15, Alexander Barkov wrote:
> >> +
> >> + void set_lex_collation(const Lex_collation_st &lc)
> >> + {
> >> +charset= lc.collation();
> >> +if (lc.is_contextually_typed_collation())
> >> + flags|= BINCMP_FLAG;
> >> +else
> >> + flags&= ~BINCMP_FLAG;
> >
> > Isn't COLLATE DEFAULT also contextually typed?
>
> "COLLATE DEFAULT" and "COLLATE uca1400_as_ci" also
> reuse this flag. It should be renamed somehow.
>
> What about CONTEXTUAL_COLLATION_FLAG ?
>
> Note, it's defined in two places:
>
> In the server:
>
> include/mysql_com.h
> #define BINCMP_FLAG 131072U /* Intern: Used by sql_yacc */
>
> In the client library:
>
> libmariadb/include/mariadb_com.h
> #define BINCMP_FLAG 131072
>
> The client library defines, but does not actually use it.
> Should we rename it in the client library? Or remove it?
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 75aa95ada6b: Tricky static asserts did not compile on some platforms. Commenting out.
Hi, Alexander,
On Mar 16, Alexander Barkov wrote:
> revision-id: 75aa95ada6b (mariadb-10.6.1-338-g75aa95ada6b)
> parent(s): e90062c20f1
> author: Alexander Barkov
> committer: Alexander Barkov
> timestamp: 2022-03-09 13:07:35 +0400
> message:
>
> Tricky static asserts did not compile on some platforms. Commenting out.
Details? What doesn't compile and where?
I'd suggest to try to keep the assert in some form
> diff --git a/sql/structs.h b/sql/structs.h
> index abfafb635b1..23958490c7b 100644
> --- a/sql/structs.h
> +++ b/sql/structs.h
> @@ -602,13 +602,16 @@ class DDL_options: public DDL_options_st
>
> struct Lex_length_and_dec_st
> {
> + /*
> + Does not compile on some platforms:
>static constexpr uint collation_type_bits= 2;
>static_assert(((1<=
> Lex_charset_collation_st::TYPE_LAST);
> + */
> protected:
>uint32 m_length;
>uint8 m_dec;
> - uint8 m_collation_type:collation_type_bits;
> + uint8 m_collation_type:2; /*collation_type_bits;*/
>bool m_has_explicit_length:1;
>bool m_has_explicit_dec:1;
> public:
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] e90062c20f1: MDEV-27266 Improve UCA collation performance for utf8mb3 and utf8mb4
Hi, Alexander, also ok On Mar 14, Alexander Barkov wrote: > revision-id: e90062c20f1 (mariadb-10.6.1-337-ge90062c20f1) > parent(s): 6a3201d3769 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-02-28 14:43:43 +0400 > message: > > MDEV-27266 Improve UCA collation performance for utf8mb3 and utf8mb4 > > Adding two levels of optimization: > > include/m_ctype.h | 53 > strings/ctype-uca-scanner_next.inl | 39 +++ > strings/ctype-uca.c| 548 > - > strings/ctype-uca.ic | 30 ++ I thought all *.ic files were renamed to *.inl ? > unittest/strings/strings-t.c | 2 +- > 5 files changed, 660 insertions(+), 12 deletions(-) Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 6a3201d3769: MDEV-27265 Improve contraction performance in UCA collations
Hi, Alexander, that ok, up to you. I'm not looking into the strings/ctype*c code. On Mar 14, Alexander Barkov wrote: > revision-id: 6a3201d3769 (mariadb-10.6.1-336-g6a3201d3769) > parent(s): 49ecf935415 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-02-28 14:05:08 +0400 > message: > > MDEV-27265 Improve contraction performance in UCA collations > > Adding a hash table for contractions. > > The old code iterated through all items in MY_CONTRACTIONS, > and was much slower, especially for those contractions > in the end of the list. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 49ecf935415: MDEV-27009 Add UCA-14.0.0 collations
> + CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin
> + CHAR(10) CHARACTER SET latin1 BINARY .. COLLATE latin1_bin
> */
> -DBUG_ASSERT(0); // Not possible yet
> +if (m_with_collate && m_ci != cl.charset_collation())
> +{
> + my_error(ER_CONFLICTING_DECLARATIONS, MYF(0),
> + "COLLATE ", m_ci->coll_name.str,
> + "COLLATE ", cl.charset_collation()->coll_name.str);
> + return true;
> +}
> +if (!my_charset_same(m_ci, cl.charset_collation()))
> +{
> + my_error(ER_COLLATION_CHARSET_MISMATCH, MYF(0),
> + cl.charset_collation()->coll_name.str, m_ci->cs_name.str);
> + return true;
> +}
> +m_ci= cl.charset_collation();
> +m_with_collate= true;
> return false;
> - }
>
> - /*
> -EXPLICIT + EXPLICIT
> -CHAR(10) CHARACTER SET latin1.. COLLATE latin1_bin
> -CHAR(10) CHARACTER SET latin1 COLLATE latin1_bin .. COLLATE latin1_bin
> -CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin
> -CHAR(10) COLLATE latin1_bin .. COLLATE latin1_bin
> -CHAR(10) CHARACTER SET latin1 BINARY .. COLLATE latin1_bin
> - */
> - if (type() == TYPE_EXPLICIT && collation() != cl.collation())
> - {
> -my_error(ER_CONFLICTING_DECLARATIONS, MYF(0),
> - "COLLATE ", collation()->coll_name.str,
> - "COLLATE ", cl.collation()->coll_name.str);
> -return true;
> - }
> - if (!my_charset_same(collation(), cl.collation()))
> - {
> -my_error(ER_COLLATION_CHARSET_MISMATCH, MYF(0),
> - cl.collation()->coll_name.str, collation()->cs_name.str);
> -return true;
> + case Lex_charset_collation_st::TYPE_COLLATE_CONTEXTUALLY_TYPED:
> +if (cl.is_contextually_typed_collate_default())
> +{
> + /*
> +SET NAMES latin1 COLLATE DEFAULT;
> +ALTER TABLE t1 CONVERT TO CHARACTER SET latin1 COLLATE DEFAULT;
> + */
> + CHARSET_INFO *tmp= Charset_loader_mysys().find_default_collation(m_ci);
> + if (!tmp)
> +return true;
> + m_ci= tmp;
> + m_with_collate= true;
> + return false;
> +}
> +else
> +{
> + /*
> +EXPLICIT + CONTEXT
> +CHAR(10) COLLATE latin1_bin .. COLLATE DEFAULT not possible yet
> +CHAR(10) COLLATE latin1_bin .. COLLATE uca1400_as_ci
> + */
> +
> + const LEX_CSTRING context_cl_name= cl.collation_name_context_suffix();
> + if (!strncasecmp(context_cl_name.str, STRING_WITH_LEN("uca1400_")))
Like above, better
DBUG_ASSERT(!strncasecmp(context_cl_name.str, STRING_WITH_LEN("uca1400_")))
> + {
> +CHARSET_INFO *tmp;
> +Charset_loader_mysys loader;
> +if (!(tmp= loader.get_contextually_typed_collation_or_error(m_ci,
> +
> context_cl_name.str)))
> + return true;
> +m_with_collate= true;
> +m_ci= tmp;
> +return false;
> + }
> +
> + DBUG_ASSERT(0); // Not possible yet
> + return false;
> +}
>}
> - *this= cl;
> + DBUG_ASSERT(0);
>return false;
> }
>
> diff --git a/strings/ctype-uca.c b/strings/ctype-uca.c
> index b89916f3b20..3e6b4e4ce43 100644
> --- a/strings/ctype-uca.c
> +++ b/strings/ctype-uca.c
> @@ -30542,7 +30613,7 @@ static const char vietnamese[]=
>Myanmar, according to CLDR Revision 8900.
>http://unicode.org/cldr/trac/browser/trunk/common/collation/my.xml
> */
> -static const char myanmar[]= "[shift-after-method expand][version 5.2.0]"
> +static const char myanmar[]= "[shift-after-method expand]"
What's going on with myanmar? You removed a version here and
added &my_uca_v520 below in its charset_info_st.
What does this change mean?
> /* Tones */
> "&\\u108C"
> "<\\u1037"
> @@ -37627,7 +37825,7 @@ struct charset_info_st
> my_charset_utf32_myanmar_uca_ci=
> NULL, /* to_lower */
> NULL, /* to_upper */
> NULL, /* sort_order */
> -NULL, /* uca */
> +&my_uca_v520, /* uca */
What does this change?
> NULL, /* tab_to_uni */
> NULL, /* tab_from_uni */
> &my_unicase_unicode520,/* caseinfo */
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] c67789f63c8: MDEV-27009 Add UCA-14.0.0 collations - adding version aware implicit weight handling
Hi, Alexander, this is ok too, thanks On Mar 14, Alexander Barkov wrote: > revision-id: c67789f63c8 (mariadb-10.6.1-334-gc67789f63c8) > parent(s): c1010fdfac2 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-02-28 10:31:31 +0400 > message: > > MDEV-27009 Add UCA-14.0.0 collations - adding version aware implicit weight > handling > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] c1010fdfac2: MDEV-27009 Add UCA-14.0.0 collations - dump logical positions and contractions
Hi, Alexander, this is ok too On Mar 14, Alexander Barkov wrote: > revision-id: c1010fdfac2 (mariadb-10.6.1-333-gc1010fdfac2) > parent(s): ab62059bbd4 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-02-28 10:31:31 +0400 > message: > > MDEV-27009 Add UCA-14.0.0 collations - dump logical positions and contractions > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] ab62059bbd4: MDEV-27009 Add UCA-14.0.0 collations - Adding implicit weight handling for Unicode-14.0.0
Hi, Alexander,
I believe it's ok too,
but see a couple of questions below
On Mar 14, Alexander Barkov wrote:
> revision-id: ab62059bbd4 (mariadb-10.6.1-332-gab62059bbd4)
> parent(s): 5fe33342399
> author: Alexander Barkov
> committer: Alexander Barkov
> timestamp: 2022-02-28 10:31:06 +0400
> message:
>
> MDEV-27009 Add UCA-14.0.0 collations - Adding implicit weight handling for
> Unicode-14.0.0
>
> diff --git a/strings/ctype-uca.c b/strings/ctype-uca.c
> index ad0b46ea296..eb6bc2eb9e6 100644
> --- a/strings/ctype-uca.c
> +++ b/strings/ctype-uca.c
> @@ -31694,7 +31694,7 @@ static inline void
> my_uca_implicit_weight_put(uint16 *to, my_wc_t code, uint level)
> {
>MY_UCA_IMPLICIT_WEIGHT weight;
> - weight= my_uca_520_implicit_weight_on_level(code, level);
> + weight= my_uca_implicit_weight_on_level(520, code, level);
will the version always be a constant in every invocation of
my_uca_implicit_weight_put() ?
>to[0]= weight.weight[0];
>to[1]= weight.weight[1];
>to[2]= 0;
> diff --git a/strings/ctype-uca.h b/strings/ctype-uca.h
> index 01f655b06cb..d439b8c578c 100644
> --- a/strings/ctype-uca.h
> +++ b/strings/ctype-uca.h
> @@ -136,11 +149,11 @@ my_uca_implicit_weight_quaternary()
>
>
> static inline MY_UCA_IMPLICIT_WEIGHT
> -my_uca_520_implicit_weight_on_level(my_wc_t code, uint level)
> +my_uca_implicit_weight_on_level(uint version, my_wc_t code, uint level)
> {
>switch (level) {
>case 0:
> -return my_uca_520_implicit_weight_primary(code);
> +return my_uca_implicit_weight_primary(version, code);
>case 1:
> return my_uca_implicit_weight_secondary();
>case 2:
do you mean that secondary/tertiary/etc weights do not depend on the
version?
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 5fe33342399: MDEV-27009 Add UCA-14.0.0 collations - adding uca-dump into build targets
Hi, Alexander, this is ok (not ok to push as is just yet, please wait until we agree on commits in the branch) On Mar 14, Alexander Barkov wrote: > revision-id: 5fe33342399 (mariadb-10.6.1-331-g5fe33342399) > parent(s): 2467eb2d314 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-02-10 11:53:47 +0400 > message: > > MDEV-27009 Add UCA-14.0.0 collations - adding uca-dump into build targets > > - Adding uca-dump into build targets > - Adding ctype-uca.h and moving implicit weight related routines there > - Reusing implicit weight routines in ctype-uca.c and uca-dump.c > - Adding handling of command line arguments to uca-dump > - Fixing some compile-time warnings in uca-dump.c > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 2467eb2d314: MDEV-27743 Remove Lex::charset
ructs.h
> index 9a5006e8b47..db065c49931 100644
> --- a/sql/structs.h
> +++ b/sql/structs.h
> @@ -599,11 +599,140 @@ class DDL_options: public DDL_options_st
...
> +class Lex_collation: public Lex_collation_st
> +{
> +public:
> + Lex_collation(CHARSET_INFO *collation, Type type)
> + {
> +m_collation= collation;
> +m_type= type;
> + }
> + static Lex_collation national(bool bin_mod)
> + {
> +if (bin_mod)
> + return Lex_collation(&my_charset_utf8mb3_bin, TYPE_EXPLICIT);
> +return Lex_collation(&my_charset_utf8mb3_general_ci, TYPE_IMPLICIT);
utf8mb3? not utf8?
> + }
> +};
> +
> +
> struct Lex_length_and_dec_st
> {
> -private:
> +protected:
>uint32 m_length;
>uint8 m_dec;
> + uint8 m_collation_type:2;
>bool m_has_explicit_length:1;
>bool m_has_explicit_dec:1;
> public:
> diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
> index ae6ae9a0cc0..6ca10267187 100644
> --- a/sql/sql_lex.cc
> +++ b/sql/sql_lex.cc
> @@ -11816,3 +11784,257 @@ bool SELECT_LEX_UNIT::explainable() const
...
> +CHARSET_INFO *
> +Lex_collation_st::resolved_to_character_set(CHARSET_INFO *def) const
> +{
> + DBUG_ASSERT(def);
> + if (m_type != TYPE_CONTEXTUALLY_TYPED)
> + {
> +if (!m_collation)
> + return def; // Empty - not typed at all
> +return m_collation; // Explicitly typed
> + }
> +
> + // Contextually typed
> + DBUG_ASSERT(m_collation);
> +
> + if (m_collation == &my_charset_bin)// CHAR(10) BINARY
> +return find_bin_collation(def);
> +
> + if (m_collation == &my_charset_latin1) // CHAR(10) COLLATE DEFAULT
> +return find_default_collation(def);
Uhm. Could you create two dummy CHARSET_INFO variables for that instead?
Like
CHARSET_INFO binary_collaton, default_collation;
no need to initialize them, but to have distinct values for binary/default
and not reuse my_charset_bin and my_charset_latin1.
> +
> + /*
> +Non-binary and non-default contextually typed collation.
> +We don't have such yet - the parser cannot produce this.
> +But will have soon, e.g. "uca1400_as_ci".
> + */
> + DBUG_ASSERT(0);
> + return NULL;
> +}
> +
> +
...
> diff --git a/sql/field.h b/sql/field.h
> index 2ed02b37cfd..fe5ba435202 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -5493,6 +5492,22 @@ class Column_definition: public Sql_alloc,
>{ return compression_method_ptr; }
>
>bool check_vcol_for_key(THD *thd) const;
> +
> + void set_lex_collation(const Lex_collation_st &lc)
> + {
> +charset= lc.collation();
> +if (lc.is_contextually_typed_collation())
> + flags|= BINCMP_FLAG;
> +else
> + flags&= ~BINCMP_FLAG;
Isn't COLLATE DEFAULT also contextually typed?
> + }
> + Lex_collation lex_collation() const
> + {
> +return Lex_collation(charset,
> + flags & BINCMP_FLAG ?
> + Lex_collation_st::TYPE_CONTEXTUALLY_TYPED :
> + Lex_collation_st::TYPE_IMPLICIT);
> + }
> };
>
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 61b72ed3dde: MDEV-27712 Reduce the size of Lex_length_and_dec_st from 16 to 8
Hi, Alexander,
a question about error codes:
On Mar 11, Alexander Barkov wrote:
> commit 61b72ed3dde
> Author: Alexander Barkov
> Date: Wed Feb 9 17:59:38 2022 +0400
>
> diff --git a/mysql-test/main/type_blob.test b/mysql-test/main/type_blob.test
> index c61ed124139..a5410244ed1 100644
> --- a/mysql-test/main/type_blob.test
> +++ b/mysql-test/main/type_blob.test
> @@ -517,7 +517,7 @@ CREATE TABLE b15776 (a char(2147483648));
> --error ER_TOO_BIG_FIELDLENGTH
> CREATE TABLE b15776 (a char(4294967295));
> # Even BLOB won't hold
> ---error ER_TOO_BIG_FIELDLENGTH
> +--error ER_NOT_SUPPORTED_YET
Why did you change the error?
> CREATE TABLE b15776 (a char(4294967296));
>
>
> diff --git a/mysql-test/main/type_float.result
> b/mysql-test/main/type_float.result
> index 5137a8229b6..9e216711ddd 100644
> --- a/mysql-test/main/type_float.result
> +++ b/mysql-test/main/type_float.result
> @@ -1162,3 +1162,36 @@ fdec
> 123.456.789,1234567890
> #
> # End of 10.4 tests
> #
> +#
> +# Start of 10.9 tests
> +#
> +#
> +# MDEV-27712 Reduce the size of Lex_length_and_dec_st from 16 to 8
> +#
> +CREATE TABLE t1 (a DOUBLE(1000,1000));
> +ERROR 42000: This version of MariaDB doesn't yet support 'scale>255'
> +CREATE TABLE t1 (a DOUBLE(1000,0));
> +ERROR 42000: Display width out of range for 'a' (max = 255)
> +CREATE TABLE t1 (a DOUBLE(0,1000));
> +ERROR 42000: This version of MariaDB doesn't yet support 'scale>255'
> +CREATE TABLE t1 (a DOUBLE(2147483647,2147483647));
> +ERROR 42000: This version of MariaDB doesn't yet support 'scale>255'
> +CREATE TABLE t1 (a DOUBLE(2147483647,0));
> +ERROR 42000: Display width out of range for 'a' (max = 255)
> +CREATE TABLE t1 (a DOUBLE(0,2147483647));
> +ERROR 42000: This version of MariaDB doesn't yet support 'scale>255'
> +CREATE TABLE t1 (a DOUBLE(2147483648,2147483648));
> +ERROR 42000: You have an error in your SQL syntax; check the manual that
> corresponds to your MariaDB server version for the right syntax to use near
> '2147483648,2147483648))' at line 1
This isn't technically a *syntax* error
Can you use a different error here?
> +CREATE TABLE t1 (a DOUBLE(2147483648,0));
> +ERROR 42000: You have an error in your SQL syntax; check the manual that
> corresponds to your MariaDB server version for the right syntax to use near
> '2147483648,0))' at line 1
> +CREATE TABLE t1 (a DOUBLE(0,2147483648));
> +ERROR 42000: You have an error in your SQL syntax; check the manual that
> corresponds to your MariaDB server version for the right syntax to use near
> '2147483648))' at line 1
> +CREATE TABLE t1 (a
> DOUBLE(99,99));
> +ERROR 42000: You have an error in your SQL syntax; check the manual that
> corresponds to your MariaDB server version for the right syntax to use near
> '99,99))' at line 1
> +CREATE TABLE t1 (a DOUBLE(99,0));
> +ERROR 42000: You have an error in your SQL syntax; check the manual that
> corresponds to your MariaDB server version for the right syntax to use near
> '99,0))' at line 1
> +CREATE TABLE t1 (a DOUBLE(0,99));
> +ERROR 42000: You have an error in your SQL syntax; check the manual that
> corresponds to your MariaDB server version for the right syntax to use near
> '99))' at line 1
> +#
> +# End of 10.9 tests
> +#
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 5bb03544762..fdc54f7df27 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -5975,11 +5977,10 @@ field_type_numeric:
> | FLOAT_SYM float_options last_field_options
>{
> $$.set(&type_handler_float, $2);
> -if ($2.length() && !$2.dec())
> +if ($2.has_explicit_length() && !$2.has_explicit_dec())
> {
> - int err;
> - ulonglong tmp_length= my_strtoll10($2.length(), NULL, &err);
> - if (unlikely(err || tmp_length > PRECISION_FOR_DOUBLE))
> + ulonglong tmp_length= $2.length();
you don't really need tmp_length here anymore
> + if (unlikely(tmp_length > PRECISION_FOR_DOUBLE))
> my_yyabort_error((ER_WRONG_FIELD_SPEC, MYF(0),
>Lex->last_field->field_name.str));
>if (tmp_length > PRECISION_FOR_FLOAT)
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 880e92a48ba: MDEV-27760 event may non stop replicate in circular semisync setup
Hi, Andrei, I don't see a point in caching the value in memory that is very cheap to calculate per event. I've pushed f8ec4dd38d0 to 10.4-serg three hours ago and Jenkins said it's fine. So I'd suggest we go with it, because we really have to close this blocker and release ES now. And MDEV-27837 will be fixed as a separate bug. queue_event checks global_system_variables.server_id in multiple places, moving only do_accept_own_server_id into Master_info will not make it any better or worse against MDEV-27837. > > As far as I can see, you can calculate it for every event just the same. > > The per event computation is extraneous and not consistent with the > nature of the flag that belongs to the group of events. > > Also notice (or remember) a "FR" > MDEV-27837 disallow `set @@session.server_id` within transaction > so until this bug is fixed it's not 'just the same'. > > I am recomming the patch, assuming that you'll be fine with the Gtid time > only flag computation. > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 880e92a48ba: MDEV-27760 event may non stop replicate in circular semisync setup
Hi, Andrei,
On Mar 01, Andrei Elkin wrote:
> > On Feb 28, Andrei wrote:
> >> revision-id: 880e92a48ba (mariadb-10.6.6-16-g880e92a48ba)
> >> parent(s): 4030a9fb2eb
> >> author: Andrei
> >> committer: Andrei
> >> timestamp: 2022-02-21 14:58:13 +0200
> >> message:
> >>
> >> MDEV-27760 event may non stop replicate in circular semisync setup
> >>
> >> diff --git a/sql/slave.cc b/sql/slave.cc
> >> index c0eef02ca7a..a1f4f4f0081 100644
> >> --- a/sql/slave.cc
> >> +++ b/sql/slave.cc
> >> @@ -6186,13 +6186,13 @@ static int queue_event(Master_info* mi, const
> >> uchar *buf, ulong event_len)
> >>bool is_rows_event= false;
> >>/*
> >> The flag has replicate_same_server_id semantics and is raised to
> >> accept
> >> -a same-server-id event on the semisync slave, for both the gtid and
> >> legacy
> >> -connection modes.
> >> -Such events can appear as result of this server recovery so the event
> >> -was created there and replicated elsewhere right before the crash. At
> >> recovery
> >> -it could be evicted from the server's binlog.
> >> - */
> >> - bool do_accept_own_server_id= false;
> >> +a same-server-id event group by the gtid strict mode semisync slave.
> >> +Own server-id events can appear as result of this server
> >> crash-recovery:
> >> +the transaction was created on this server then being master, got
> >> replicated
> >> +elsewhere right before the crash before commit;
> >> +finally at recovery the transaction gets evicted from the server's
> >> binlog.
> >> + */
> >> + static bool do_accept_own_server_id= false;
> >
> > No, sorry, it cannot be static. If you need to preserve it between
> > events, you can keep it, for example, in Master_info.
>
> The var is set by Gtid events to serve as a marker of the whole current
> Gtid-headed
> group of event.
> Only next Gtid event may change the current value.
>
> Why can't it?
Why it cannot be static? Because it's not a property of the server, it's
a property of a specific replication stream. Technically one can have a
multi-source and do_accept_own_server_id could have different values for
different masters.
> > So, the question now is, why do you need to preserve the value of
> > do_accept_own_server_id between queue_event invocations?
>
> The flag as said above applies to the whole group of events, so it's
> preserved at least through so many
> queue_event() calls as many event in the group (I count Gtid itself in).
As far as I can see, you can calculate it for every event just the same.
> >> @@ -6281,6 +6281,8 @@ static int queue_event(Master_info* mi, const uchar
> >> *buf, ulong event_len)
> >> dbug_rows_event_count = 0;
> >>};);
> >> #endif
> >> + s_id= uint4korr(buf + SERVER_ID_OFFSET);
> >> +
> >>mysql_mutex_lock(&mi->data_lock);
> >>
> >>switch (buf[EVENT_TYPE_OFFSET]) {
> >> @@ -6722,6 +6724,11 @@ static int queue_event(Master_info* mi, const uchar
> >> *buf, ulong event_len)
> >>
> >> ++mi->events_queued_since_last_gtid;
> >> inc_pos= event_len;
> >> +
> >> +do_accept_own_server_id=
> >> + (s_id == global_system_variables.server_id &&
> >> rpl_semi_sync_slave_enabled
> >> + && mi->using_gtid != Master_info::USE_GTID_NO &&
> >> opt_gtid_strict_mode) ?
> >> + true : false;
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 14cc679c95e: MDEV-27831 Let the SQL SERVICE user set the current user name.
Hi, Alexey, On Mar 04, Alexey Botchkov wrote: > revision-id: 14cc679c95e (mariadb-10.7.2-8-g14cc679c95e) > parent(s): 33fd136c61b > author: Alexey Botchkov > committer: Alexey Botchkov > timestamp: 2022-02-15 11:35:18 +0400 > message: > > MDEV-27831 Let the SQL SERVICE user set the current user name. > > The 'user' argument added to the mysql_real_connect_local. I think this is wrong on many levels. A plugin name is already known inside the plugin, you shouldn't force the plugin to pass its own name as an argument in multiple places, the server should determine it automatically. You should not set current_user to an arbitrary string for audit plugin to see it. current_user is the name of the user account and it's used in many places as such. Try, for example, to create a view or a stored procedure. Who will be a definer? Setting only user() might be ok. Setting @@proxy_user or @@external_user is even better, if your audit plugin can show them. @@external_user would be the best, I think it's purely informational. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 5d8838a79d5: MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable
Hi, Rucha, On Mar 02, Rucha Deodhar wrote: > revision-id: 5d8838a79d5 (mariadb-10.6.1-352-g5d8838a79d5) > parent(s): f8b3c661231 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2022-02-15 22:29:54 +0530 > message: > > MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable > > Analysis: There are 2 server variables- "old_mode" and "old". "old" is no > longer needed as "old_mode" has replaced it (however still used in some places > in the code). "old_mode" and "old" has same purpose- emulate behavior from > previous MariaDB versions. So they can be merged to avoid confusion. > Fix: Deprecate "old" variable and create another mode for @@old_mode to mimic > behavior of previous "old" variable. Create specific modes for specifix task > that --old sql variable was doing earlier and use the new modes instead. > > diff --git a/mysql-test/main/mysqld--help.result > b/mysql-test/main/mysqld--help.result > index 7cbfa52b846..261dbe2ffe0 100644 > --- a/mysql-test/main/mysqld--help.result > +++ b/mysql-test/main/mysqld--help.result > @@ -683,7 +683,8 @@ The following specify which files/extra groups are read > (specified before remain > --old-mode=name Used to emulate old behavior from earlier MariaDB or > MySQL versions. Any combination of: > NO_DUP_KEY_WARNINGS_WITH_IGNORE, NO_PROGRESS_INFO, > - ZERO_DATE_TIME_CAST, UTF8_IS_UTF8MB3 > + ZERO_DATE_TIME_CAST, UTF8_IS_UTF8MB3, > + INDEX_HINT_MASK_JOIN, CHECKSUM_SLOW_NULLS I'm sorry, I still cannot understand what these new modes do :( I know that it was my idea to use "CHECKSUM_SLOW_NULLS", but now I'm, like, "what does it mean?". Let's discuss it on slack INDEX_HINT_MASK_JOIN - this seems to be easier, what about INGORE_INDEX_ONLY_FOR_JOIN ? That's a bit long, but shorter than NO_DUP_KEY_WARNINGS_WITH_IGNORE. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi, Aleksey,
On Feb 15, Aleksey Midenkov wrote:
> > On Feb 12, Aleksey Midenkov wrote:
> > > commit 0402f6dcb67
> > > Author: Aleksey Midenkov
> > > Date: Tue Feb 8 22:54:03 2022 +0300
> > >
> > > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> > > index 0403d6e73c3..06068290247 100644
> > > --- a/sql/sql_delete.cc
> > > +++ b/sql/sql_delete.cc
> > > @@ -507,6 +508,18 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list,
> > > COND *conds,
> > > if (thd->lex->describe || thd->lex->analyze_stmt)
> > >goto produce_explain_and_leave;
> > >
> > > +if (thd->log_current_statement)
> > > +{
> > > + thd->reset_unsafe_warnings();
> > > + if (thd->binlog_query(THD::STMT_QUERY_TYPE,
> > > +thd->query(), thd->query_length(),
> > > +transactional_table, FALSE, FALSE, 0) > 0)
> > > + {
> > > +my_error(ER_ERROR_ON_WRITE, MYF(0), "binary log", -1);
> > > +DBUG_RETURN(1);
> > > + }
> > > +}
> >
> > perhaps, move this block into a function? It's repeated four times here.
> >
>
> Moved.
Thanks!
The new method, binlog_for_noop_dml() looks very puzzling.
I understand what it does, for now, but somebody who wasn't involved in
this bugfixing, likely won't have any idea.
The name is correct, I agree, it says what, but it doesn't explain why.
Could you add a comment there? To say that "noop dml" is "an
insert(odku)/update/delete that doesn't change the table" and that
it is normally not logged, but needs to be logged if it auto-created a
partition as a side effect.
> > by the way, you don't seem to have a test for a zero-rows insert.
> > That is INSERT ... SELECT with an impossible where or limit 0.
> >
> > In fact, you can have a zero-row insert with a normal insert
> > INSERT t1 VALUES (1); if this (1) is a unique key violation.
> > I believe your code handles this case, thought, just a test case is
> > missing.
>
> INSERT does not trigger history. But INSERT .. ODKU does. Added test
> cases for INSERT .. ODKU and INSERT .. SELECT .. ODKU. That required
> to add THD::vers_created_partitions as log_current_statement is also
> used in CREATE TABLE (and select_insert::prepare_eof() is used for
> CREATE TABLE .. SELECT) and that broke some replication tests:
>
> rpl.create_or_replace_mix
> rpl.create_or_replace_row
> rpl.create_or_replace_statement
Indeed, I see.
Can you use OPTION_KEEP_LOG instead?
Or transaction->stmt.modified_non_trans_table ?
I hate it that there're so many ways to force binlogging a statement,
and they're all, of course, are subtly different.
>
> > > +
> > > my_ok(thd, 0);
> > > DBUG_RETURN(0);
> > >}
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9d6be81c0da: MDEV-27832 disable binary logging for SQL SERVICE.
Hi, Alexey,
On Mar 02, Alexey Botchkov wrote:
> revision-id: 9d6be81c0da (mariadb-10.7.2-8-g9d6be81c0da)
> parent(s): 33fd136c61b
> author: Alexey Botchkov
> committer: Alexey Botchkov
> timestamp: 2022-02-15 13:37:59 +0400
> message:
>
> MDEV-27832 disable binary logging for SQL SERVICE.
>
> Binary logging is now disabled for the queries run by SQL SERVICE.
> The binlogging can be turned on with the 'SET SQL_LOG_BIN=On' query.
>
> diff --git a/mysql-test/suite/plugins/r/test_sql_service.result
> b/mysql-test/suite/plugins/r/test_sql_service.result
> index 00f0411b665..943bb239e5f 100644
> --- a/mysql-test/suite/plugins/r/test_sql_service.result
> +++ b/mysql-test/suite/plugins/r/test_sql_service.result
> @@ -2,6 +2,19 @@ install plugin test_sql_service soname 'test_sql_service';
> show status like 'test_sql_service_passed';
> Variable_nameValue
> Test_sql_service_passed 1
> +set global test_sql_service_execute_sql_global= 'create table test.t1 select
> 1 as a, @@SQL_LOG_BIN';
> +set global test_sql_service_execute_sql_local= 'insert into test.t1 select
> 2 as a, @@SQL_LOG_BIN';
> +set global test_sql_service_execute_sql_global= 'SET SQL_LOG_BIN=1';
> +set global test_sql_service_execute_sql_global= 'insert into test.t1 select
> 3 as a, @@SQL_LOG_BIN';
> +set global test_sql_service_execute_sql_global= 'SET SQL_LOG_BIN=0';
> +set global test_sql_service_execute_sql_global= 'insert into test.t1 select
> 4 as a, @@SQL_LOG_BIN';
> +select * from t1 order by a;
this is not a test. Please, enable binlog and see whether queries are
actually logged.
Also, add a test for, say, sql_auto_is_null:
set global test_sql_service_execute_sql_global='set sql_auto_is_null=1';
set global test_sql_service_execute_sql_global='insert test.t1 select 5,
@@sql_auto_is_null';
select * from t1 where a=5;
> +a@@SQL_LOG_BIN
> +10
> +20
> +31
> +40
> +drop table t1;
> set global test_sql_service_run_test= 1;
> show status like 'test_sql_service_passed';
> Variable_nameValue
> diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> index e025147c71e..48681ac0b6f 100644
> --- a/sql/sql_prepare.cc
> +++ b/sql/sql_prepare.cc
> @@ -5617,12 +5617,30 @@ class Protocol_local : public Protocol_text
>THD *new_thd;
>Security_context empty_ctx;
>
> + my_bool do_log_bin;
> +
>Protocol_local(THD *thd_arg, THD *new_thd_arg, ulong prealloc) :
> Protocol_text(thd_arg, prealloc),
> cur_data(0), first_data(0), data_tail(&first_data), alloc(0),
> -new_thd(new_thd_arg)
> +new_thd(new_thd_arg), do_log_bin(FALSE)
>{}
>
> + void set_binlog_vars(my_bool *sav_log_bin, ulonglong *sav_bits)
> + {
> +*sav_log_bin= thd->variables.sql_log_bin;
> +*sav_bits= thd->variables.option_bits;
> +
> +if ((thd->variables.sql_log_bin= do_log_bin))
> + thd->variables.option_bits|= OPTION_BIN_LOG;
> +else
> + thd->variables.option_bits&= ~OPTION_BIN_LOG;
> + }
> + void restore_binlog_vars(my_bool &sav_log_bin, ulonglong &sav_bits)
> + {
> +do_log_bin= thd->variables.sql_log_bin;
> +thd->variables.sql_log_bin= sav_log_bin;
> +thd->variables.option_bits= sav_bits;
that's what sql_auto_is_null test is for.You destroy all other option_bits
here. Better don't save/restore option_bits, but use the same if() as in
set_binlog_vars(). Or, even better, reuse
fix_sql_log_bin_after_update(). I'd suggest to make it a method in THD
> + }
> protected:
>bool net_store_data(const uchar *from, size_t length);
>bool net_store_data_cs(const uchar *from, size_t length,
> @@ -6230,12 +6248,17 @@ loc_advanced_command(MYSQL *mysql, enum
> enum_server_command command,
> Ed_connection con(p->thd);
> Security_context *ctx_orig= p->thd->security_ctx;
> MYSQL_LEX_STRING sql_text;
> +my_bool log_bin_orig;
> +ulonglong option_bits_orig;
> +p->set_binlog_vars(&log_bin_orig, &option_bits_orig);
> +
> DBUG_ASSERT(current_thd == p->thd);
> sql_text.str= (char *) arg;
> sql_text.length= arg_length;
> p->thd->security_ctx= &p->empty_ctx;
> result= con.execute_direct(p, sql_text);
> p->thd->security_ctx= ctx_orig;
> +p->restore_binlog_vars(log_bin_orig, option_bits_orig);
>}
>if (skip_check)
> result= 0;
> @@ -6391,6 +6414,9 @@ extern "C" MYSQL *mysql_real_connect_local(MYSQL *mysql)
> new_thd->security_ctx->skip_grants();
> new_thd->query_ca
Re: [Maria-developers] 880e92a48ba: MDEV-27760 event may non stop replicate in circular semisync setup
Hi, Andrei,
On Feb 28, Andrei wrote:
> revision-id: 880e92a48ba (mariadb-10.6.6-16-g880e92a48ba)
> parent(s): 4030a9fb2eb
> author: Andrei
> committer: Andrei
> timestamp: 2022-02-21 14:58:13 +0200
> message:
>
> MDEV-27760 event may non stop replicate in circular semisync setup
>
> diff --git a/sql/slave.cc b/sql/slave.cc
> index c0eef02ca7a..a1f4f4f0081 100644
> --- a/sql/slave.cc
> +++ b/sql/slave.cc
> @@ -6186,13 +6186,13 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>bool is_rows_event= false;
>/*
> The flag has replicate_same_server_id semantics and is raised to accept
> -a same-server-id event on the semisync slave, for both the gtid and
> legacy
> -connection modes.
> -Such events can appear as result of this server recovery so the event
> -was created there and replicated elsewhere right before the crash. At
> recovery
> -it could be evicted from the server's binlog.
> - */
> - bool do_accept_own_server_id= false;
> +a same-server-id event group by the gtid strict mode semisync slave.
> +Own server-id events can appear as result of this server crash-recovery:
> +the transaction was created on this server then being master, got
> replicated
> +elsewhere right before the crash before commit;
> +finally at recovery the transaction gets evicted from the server's
> binlog.
> + */
> + static bool do_accept_own_server_id= false;
No, sorry, it cannot be static. If you need to preserve it between
events, you can keep it, for example, in Master_info.
yes, I remember, that's where it was and I asked why it's in
Master_info. You could've answered, that you need to preserve the value
between queue_event invocations - I didn't realize that at the time.
So, the question now is, why do you need to preserve the value of
do_accept_own_server_id between queue_event invocations?
>/*
> FD_q must have been prepared for the first R_a event
> inside get_master_version_and_clock()
> @@ -6281,6 +6281,8 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
> dbug_rows_event_count = 0;
>};);
> #endif
> + s_id= uint4korr(buf + SERVER_ID_OFFSET);
> +
>mysql_mutex_lock(&mi->data_lock);
>
>switch (buf[EVENT_TYPE_OFFSET]) {
> @@ -6722,6 +6724,11 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>
> ++mi->events_queued_since_last_gtid;
> inc_pos= event_len;
> +
> +do_accept_own_server_id=
> + (s_id == global_system_variables.server_id &&
> rpl_semi_sync_slave_enabled
> + && mi->using_gtid != Master_info::USE_GTID_NO &&
> opt_gtid_strict_mode) ?
> + true : false;
this is a ridiculous pattern
bool var = (bool expr) ? true : false;
don't do that.
>}
>break;
>/*
> @@ -6909,7 +6916,6 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>*/
>
>mysql_mutex_lock(log_lock);
> - s_id= uint4korr(buf + SERVER_ID_OFFSET);
>/*
> Write the event to the relay log, unless we reconnected in the middle
> of an event group and now need to skip the initial part of the group that
> @@ -6955,7 +6961,7 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>else
>if ((s_id == global_system_variables.server_id &&
> !(mi->rli.replicate_same_server_id ||
> - (do_accept_own_server_id= rpl_semi_sync_slave_enabled))) ||
> + do_accept_own_server_id)) ||
>event_that_should_be_ignored(buf) ||
>/*
> the following conjunction deals with IGNORE_SERVER_IDS, if set
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MDEV-16329 ALTER ONLINE TABLE
+online= false;
> +
> + if (online)
> + {
> +table_list->lock_type= TL_READ;
> + }
>
>DEBUG_SYNC(thd, "alter_table_before_open_tables");
>
> @@ -10961,6 +10983,58 @@ bool mysql_trans_commit_alter_copy_data(THD *thd)
>DBUG_RETURN(error);
> }
>
> +#ifdef HAVE_REPLICATION
> +static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi,
> + Cache_flip_event_log *log)
> +{
> + MEM_ROOT event_mem_root;
> + Query_arena backup_arena;
> + Query_arena event_arena(&event_mem_root, Query_arena::STMT_INITIALIZED);
> + init_sql_alloc(key_memory_gdl, &event_mem_root,
> + MEM_ROOT_BLOCK_SIZE, 0, MYF(0));
> +
> + int error= 0;
> +
> + IO_CACHE *log_file= log->flip();
> +
> + thd_progress_report(thd, 0, my_b_write_tell(log_file));
> +
> + Abort_on_warning_instant_set old_abort_on_warning(thd, 0);
> + do
> + {
> +const auto *descr_event= rgi->rli->relay_log.description_event_for_exec;
> +auto *ev= Log_event::read_log_event(log_file, descr_event, false);
> +if (!ev)
> + break;
> +
> +ev->thd= thd;
> +thd->set_n_backup_active_arena(&event_arena, &backup_arena);
> +error= ev->apply_event(rgi);
> +thd->restore_active_arena(&event_arena, &backup_arena);
> +
> +event_arena.free_items();
> +free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC));
> +if (ev != rgi->rli->relay_log.description_event_for_exec)
> + delete ev;
> +thd_progress_report(thd, my_b_tell(log_file), thd->progress.max_counter);
> +DEBUG_SYNC(thd, "alter_table_online_progress");
> + } while(!error);
> +
> + return error;
> +}
> +#endif // HAVE_REPLICATION
> +
> +static void online_alter_cleanup_binlog(THD *thd, TABLE_SHARE *s)
> +{
> +#ifdef HAVE_REPLICATION
> + if (!s->online_alter_binlog)
> +return;
> + // s->online_alter_binlog->reset_logs(thd, false, NULL, 0, 0);
forgot to delete?
> + s->online_alter_binlog->cleanup();
> + s->online_alter_binlog->~Cache_flip_event_log();
> + s->online_alter_binlog= NULL;
> +#endif
> +}
>
> static int
> copy_data_between_tables(THD *thd, TABLE *from, TABLE *to,
> @@ -11306,6 +11431,76 @@ copy_data_between_tables(THD *thd, TABLE *from,
> TABLE *to,
>cleanup_done= 1;
>to->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY);
>
> +#ifdef HAVE_REPLICATION
> + if (likely(online && error < 0))
> + {
> +Ha_trx_info *trx_info_save= thd->transaction->all.ha_list;
> +thd->transaction->all.ha_list = NULL;
why?
> +thd_progress_next_stage(thd);
> +Table_map_log_event table_event(thd, from, from->s->table_map_id,
> +from->file->has_transactions());
> +Relay_log_info rli(false);
> +rpl_group_info rgi(&rli);
> +RPL_TABLE_LIST rpl_table(to, TL_WRITE, from, table_event.get_table_def(),
> + copy, copy_end);
> +Cache_flip_event_log *binlog= from->s->online_alter_binlog;
> +rgi.thd= thd;
> +rgi.tables_to_lock= &rpl_table;
> +
> +rgi.m_table_map.set_table(from->s->table_map_id, to);
> +
> +DBUG_ASSERT(binlog->is_open());
> +
> +rli.relay_log.description_event_for_exec=
> +new
> Format_description_log_event(4);
> +
> +// We restore bitmaps, because update event is going to mess up with
> them.
> +to->default_column_bitmaps();
> +
> +error= online_alter_read_from_binlog(thd, &rgi, binlog);
> +
> +DEBUG_SYNC(thd, "alter_table_online_before_lock");
> +
> +int lock_error=
> +thd->mdl_context.upgrade_shared_lock(from->mdl_ticket, MDL_EXCLUSIVE,
> +
> (double)thd->variables.lock_wait_timeout);
> +if (!error)
> + error= lock_error;
> +
> +if (!error)
> +{
> + thd_progress_next_stage(thd);
> + error= online_alter_read_from_binlog(thd, &rgi, binlog);
> +}
> +
> +thd->transaction->all.ha_list = trx_info_save;
> + }
> + else if (unlikely(online)) // error was on copy stage
> + {
> +/*
> + We need to issue a barrier to clean up gracefully.
> + Without this, following possible:
> + T1: ALTER TABLE starts
> + T2: INSERT starts
> + T1: ALTER TABLE fails with error (i.e. ER_DUP_KEY)
> + T1: from->s->online_alter_binlog sets to NULL
> + T2: INSERT committs
> + T2: thd->online_alter_cache_list is not empty
> + T2: binlog_commit: DBUG_ASSERT(binlog); is issued.
1. do you have a test for that?
2. can it be fixed, like, on iterating thd->online_alter_cache_list
thd notices that from->s->online_alter_cache_list is NULL, so
it simply discards the cache? Then you won't need MDL_SHARED_NO_WRITE
> +*/
> +// Ignore the return result. We already have an error.
> +thd->mdl_context.upgrade_shared_lock(from->mdl_ticket,
> + MDL_SHARED_NO_WRITE,
> + thd->variables.lock_wait_timeout);
> + }
> +#endif
> +
> + if (error > 0 && !from->s->tmp_table)
> + {
> +/* We are going to drop the temporary table */
> +to->file->extra(HA_EXTRA_PREPARE_FOR_DROP);
> + }
> +
>DEBUG_SYNC(thd, "copy_data_between_tables_before_reset_backup_lock");
>if (backup_reset_alter_copy_lock(thd))
> error= 1;
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] MDEV-16329 ALTER ONLINE TABLE
Hi, Nikita,
On Dec 13, Nikita Malyavin wrote:
> > > -class binlog_cache_mngr {
> > > +class binlog_cache_mngr: public ilist_node<> {
> > > public:
> > >binlog_cache_mngr(my_off_t param_max_binlog_stmt_cache_size,
> > > my_off_t param_max_binlog_cache_size,
> > > ulong *param_ptr_binlog_stmt_cache_use,
> > > ulong *param_ptr_binlog_stmt_cache_disk_use,
> > > ulong *param_ptr_binlog_cache_use,
> > > -ulong *param_ptr_binlog_cache_disk_use)
> > > +ulong *param_ptr_binlog_cache_disk_use,
> > > +TABLE_SHARE *share)
> > > -: last_commit_pos_offset(0), using_xa(FALSE), xa_xid(0)
> > > +: last_commit_pos_offset(0), using_xa(FALSE), xa_xid(0), share(share)
> >
> > does it work, can one do it? I mean share(share)
> > in other places, as far as I remember, arguments always use a distinct
> > name,
> > like share(share_arg) or share(share_) or something.
> >
> Yes, this is a language feature for constructor initializers.
Okay. We don't use it though and it will stop almost everyone looking at
this code, as it's all too easy to assume it's a mistake.
So I'd recommend to rename the argument to avoid misunderstanding.
Like, to `share_`.
But also feel free to keep it your way, it's a valid C++ after all.
> > >{
> > > stmt_cache.set_binlog_cache_info(param_max_binlog_stmt_cache_size,
> > >param_ptr_binlog_stmt_cache_use,
> > > @@ -2116,9 +2144,58 @@ int binlog_commit(THD *thd, bool all, bool ro_1pc)
...
> > > +else
> > > +{
> > > + error= cache_copy(&cache_mngr.trx_cache.cache_log,
> > > +&cache_mngr.stmt_cache.cache_log);
> >
> > Why do you need two caches here? It seems you could have just one trx_cache
> > and if the statement fails you simply truncate it to the beginning of
> > the statement.
> >
> Didn't think about that, but seems so. Why would binlog need two caches
> then?
I don't really know. But, for example, I can think of a multi-table
update that modifies both InnoDB and MyISAM tables and then fails after
updating some rows. It'll be reverted in InnoDB, but not in MyISAM.
With two caches InnoDB row events can go into stmt_cache and MyISAM row
events can go into trx_cache. And later stmt_cache will be discarded,
but MyISAM changes will persist.
This multi-table case does not apply to online alter.
Are there other cases when a separate stmt_cache might be needed?
> > > + binlog_cache_mngr *const cache_mngr= thd->binlog_get_cache_mngr();
> > > + /*
> > > +cache_mngr can be NULL in case if binlog logging is disabled.
> > > + */
> > >if (!cache_mngr)
> > >{
> > > DBUG_ASSERT(WSREP(thd) ||
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 592f57e25a5: MDEV-27760 event may non stop replicate in circular semisync setup
Hi, Andrei,
I'm sorry, but I don't understand that fix at all.
Why did you move do_accept_own_server_id into Master_info if you only
use it in one static function queue_event() ?
Why did you change it under some complex condition that you moved
somewhere up in the queue_event() when you could've done simply
&& opt_gtid_strict_mode
?
On Feb 17, Andrei wrote:
> revision-id: 592f57e25a5 (mariadb-10.6.5-82-g592f57e25a5)
> parent(s): 8d742fe4acb
> author: Andrei
> committer: Andrei
> timestamp: 2022-02-14 16:31:25 +0200
> message:
>
> MDEV-27760 event may non stop replicate in circular semisync setup
>
> diff --git a/sql/slave.cc b/sql/slave.cc
> index 5ff40d8fb25..d5275d17460 100644
> --- a/sql/slave.cc
> +++ b/sql/slave.cc
> @@ -5037,6 +5037,7 @@ log space");
>mi->abort_slave= 0;
>mi->slave_running= MYSQL_SLAVE_NOT_RUN;
>mi->io_thd= 0;
> + mi->do_accept_own_server_id= false;
>/*
> Note: the order of the two following calls (first broadcast, then unlock)
> is important. Otherwise a killer_thread can execute between the calls and
> @@ -6175,15 +6176,6 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>uchar new_buf_arr[4096];
>bool is_malloc = false;
>bool is_rows_event= false;
> - /*
> -The flag has replicate_same_server_id semantics and is raised to accept
> -a same-server-id event on the semisync slave, for both the gtid and
> legacy
> -connection modes.
> -Such events can appear as result of this server recovery so the event
> -was created there and replicated elsewhere right before the crash. At
> recovery
> -it could be evicted from the server's binlog.
> - */
> - bool do_accept_own_server_id= false;
>/*
> FD_q must have been prepared for the first R_a event
> inside get_master_version_and_clock()
> @@ -6272,6 +6264,8 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
> dbug_rows_event_count = 0;
>};);
> #endif
> + s_id= uint4korr(buf + SERVER_ID_OFFSET);
> +
>mysql_mutex_lock(&mi->data_lock);
>
>switch (buf[EVENT_TYPE_OFFSET]) {
> @@ -6713,6 +6707,20 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>
> ++mi->events_queued_since_last_gtid;
> inc_pos= event_len;
> +
> +if (s_id == global_system_variables.server_id &&
> + rpl_semi_sync_slave_enabled &&
> + mi->using_gtid != Master_info::USE_GTID_NO &&
> + opt_gtid_strict_mode)
> +{
> + rpl_gtid *state_gtid= mi->gtid_current_pos.find(event_gtid.domain_id);
> + mi->do_accept_own_server_id=
> + !state_gtid || state_gtid->seq_no < event_gtid.seq_no;
> +}
> +else
> +{
> + mi->do_accept_own_server_id= false;
> +}
>}
>break;
>/*
> @@ -6900,7 +6908,6 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>*/
>
>mysql_mutex_lock(log_lock);
> - s_id= uint4korr(buf + SERVER_ID_OFFSET);
>/*
> Write the event to the relay log, unless we reconnected in the middle
> of an event group and now need to skip the initial part of the group that
> @@ -6946,7 +6953,7 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>else
>if ((s_id == global_system_variables.server_id &&
> !(mi->rli.replicate_same_server_id ||
> - (do_accept_own_server_id= rpl_semi_sync_slave_enabled))) ||
> + mi->do_accept_own_server_id)) ||
>event_that_should_be_ignored(buf) ||
>/*
> the following conjunction deals with IGNORE_SERVER_IDS, if set
> @@ -7006,7 +7013,7 @@ static int queue_event(Master_info* mi, const uchar
> *buf, ulong event_len)
>}
>else
>{
> -if (do_accept_own_server_id)
> +if (mi->do_accept_own_server_id)
> {
>int2store(const_cast(buf + FLAGS_OFFSET),
> uint2korr(buf + FLAGS_OFFSET) | LOG_EVENT_ACCEPT_OWN_F);
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
is redundant, because row_start->cmp()
on itself is sufficient. So, I suppose you've done it as an optimization?
to avoid expensive cmp()?
But in that case you also allow zero date in the row_start, and this looks
like a bug.
I'd suggest to simplify the code and to remove is_max check.
Unless you've run benchmarks and it really helps. And unless
I've misunderstood its purpose.
> + DBUG_RETURN(HA_ERR_WRONG_ROW_TIMESTAMP);
> + }
> +
>MYSQL_INSERT_ROW_START(table_share->db.str, table_share->table_name.str);
>mark_trx_read_write();
>increment_statistics(&SSV::ha_write_count);
> diff --git a/sql/item.cc b/sql/item.cc
> index a016f04953c..cf259ebd53c 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -5986,6 +5986,13 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
> else if (!from_field)
>goto error;
>
> +if (thd->column_usage == MARK_COLUMNS_WRITE &&
> +from_field != view_ref_found &&
> +thd->vers_insert_history(from_field))
Could you try to avoid invoking THD::vers_insert_history()
(which isn't inlined) for every fix_fields on every field that's
going to be modified (so every INSERT and every UPDATE)?
> +{
> + DBUG_ASSERT(from_field->table->versioned());
> + from_field->table->vers_write= false;
> +}
> table_list= (cached_table ? cached_table :
> from_field != view_ref_found ?
> from_field->table->pos_in_table_list : 0);
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 51e4bcad16b..bf65cd4a279 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -6039,7 +6039,8 @@ find_field_in_table(THD *thd, TABLE *table, const char
> *name, size_t length,
>
> if (field->invisible == INVISIBLE_SYSTEM &&
> thd->column_usage != MARK_COLUMNS_READ &&
> -thd->column_usage != COLUMNS_READ)
> +thd->column_usage != COLUMNS_READ &&
> +!thd->vers_insert_history(field))
it's okay to invoke THD::vers_insert_history here, as INVISIBLE_SYSTEM
fields are rare
>DBUG_RETURN((Field*)0);
>}
>else
> @@ -8534,7 +8535,8 @@ fill_record(THD *thd, TABLE *table_arg, List
> &fields, List &values,
> if (table->next_number_field &&
> rfield->field_index == table->next_number_field->field_index)
>table->auto_increment_field_not_null= TRUE;
> -const bool skip_sys_field= rfield->vers_sys_field(); // TODO: &&
> !thd->vers_modify_history() [MDEV-16546]
> +const bool skip_sys_field= rfield->vers_sys_field() &&
> + (update || !thd->vers_insert_history(rfield));
same. it's behind rfield->vers_sys_field() check, so ok.
> if ((rfield->vcol_info || skip_sys_field) &&
> !value->vcol_assignment_allowed_value() &&
> table->s->table_category != TABLE_CATEGORY_TEMPORARY)
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi, Aleksey,
This was quite good, thanks!
Many cases covered, not just ROLLBACK, but also LIMIT 0 and
impossible WHERE.
See a couple of comments below. Nothing big.
On Feb 12, Aleksey Midenkov wrote:
> commit 0402f6dcb67
> Author: Aleksey Midenkov
> Date: Tue Feb 8 22:54:03 2022 +0300
>
> diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> index 0403d6e73c3..06068290247 100644
> --- a/sql/sql_delete.cc
> +++ b/sql/sql_delete.cc
> @@ -507,6 +508,18 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND
> *conds,
> if (thd->lex->describe || thd->lex->analyze_stmt)
>goto produce_explain_and_leave;
>
> +if (thd->log_current_statement)
> +{
> + thd->reset_unsafe_warnings();
> + if (thd->binlog_query(THD::STMT_QUERY_TYPE,
> +thd->query(), thd->query_length(),
> +transactional_table, FALSE, FALSE, 0) > 0)
> + {
> +my_error(ER_ERROR_ON_WRITE, MYF(0), "binary log", -1);
> +DBUG_RETURN(1);
> + }
> +}
perhaps, move this block into a function? It's repeated four times here.
by the way, you don't seem to have a test for a zero-rows insert.
That is INSERT ... SELECT with an impossible where or limit 0.
In fact, you can have a zero-row insert with a normal insert
INSERT t1 VALUES (1); if this (1) is a unique key violation.
I believe your code handles this case, thought, just a test case is missing.
> +
> my_ok(thd, 0);
> DBUG_RETURN(0);
>}
> @@ -932,8 +958,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND
> *conds,
>else
> errcode= query_error_code(thd, killed_status == NOT_KILLED);
>
> - ScopedStatementReplication scoped_stmt_rpl(
> - table->versioned(VERS_TRX_ID) ? thd : NULL);
> + StatementBinlog stmt_binlog(thd, table->versioned(VERS_TRX_ID) ||
> +
> thd->binlog_need_stmt_format(transactional_table));
I know that's not part of this patch, but still. Just trying to understand.
Why did it change to statement-based if VERS_TRX_ID?
>/*
> [binlog]: If 'handler::delete_all_rows()' was called and the
> storage engine does not inject the rows itself, we replicate
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 349283c5e7a: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
#x27;t check that view's timestamp is in the past. If it's not,
it can be changed still within the same microsecond.
> +
> +
> uint TABLE_SHARE::actual_n_key_parts(THD *thd)
> {
>return use_ext_keys &&
> diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h
> index e2e6e1b7acc..70f246d4e0d 100644
> --- a/sql/sql_trigger.h
> +++ b/sql/sql_trigger.h
> @@ -112,7 +112,7 @@ class Trigger :public Sql_alloc
>GRANT_INFO subject_table_grants;
>sql_mode_t sql_mode;
>/* Store create time. Can't be mysql_time_t as this holds also sub seconds
> */
> - ulonglong create_time;
> + ulonglong ms_create_time; // Create time timestamp in microseconds
my_hrtime_t create_time;
>trg_event_type event;
>trg_action_time_type action_time;
>uint action_order;
> @@ -195,7 +195,7 @@ class Table_triggers_list: public Sql_alloc
>*/
>List definition_modes_list;
>/** Create times for triggers */
> - List create_times;
> + List ms_create_times;
ditto
>
>List definers_list;
>
> @@ -331,4 +331,14 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST
> *tables, bool create);
> extern const char * const TRG_EXT;
> extern const char * const TRN_EXT;
>
> +
> +/**
> + Make time compatible with MySQL 5.7 trigger time.
> +*/
> +
> +inline ulonglong make_ms_time(my_time_t time, ulong time_sec_part)
make_hrtime. and better put it together with hrtime_to_time,
hrtime_from_time, etc
> +{
> + return ((ulonglong) time)*100 + time_sec_part;
> +}
> +
> #endif /* SQL_TRIGGER_INCLUDED */
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 020830483c5..3b51f6d2451 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST
> *view,
>DBUG_RETURN(error);
> }
>
> +/**
> + Check is TABLE_LEST and SHARE match
> + @param[in] viewTABLE_LIST of the view
> + @param[in] share Share object of view
> +
> + @return false on error or misspatch
> +*/
>
> +bool mariadb_view_version_get(TABLE_SHARE *share)
> +{
> + DBUG_ASSERT(share->is_view);
> +
> + if (!(share->tabledef_version.str=
> +(uchar*) alloc_root(&share->mem_root,
> +MICROSECOND_TIMESTAMP_BUFFER_SIZE)))
> +return TRUE;
> +
> + DBUG_ASSERT(share->view_def != NULL);
> + if (share->view_def->parse((uchar *) &share->tabledef_version, NULL,
> + view_timestamp_parameters, 1,
> + &file_parser_dummy_hook))
> +return TRUE;
> + DBUG_ASSERT(share->tabledef_version.length ==
> MICROSECOND_TIMESTAMP_BUFFER_SIZE-1);
If you open an old view with -MM-DD... timestamp,
where do you convert it?
If you open a view without a timestamp at all, where do you handle it?
> + return FALSE;
> +}
>
> /**
>read VIEW .frm and create structures
> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
> index 9483db9eff9..138fa4bc631 100644
> --- a/sql/sql_show.cc
> +++ b/sql/sql_show.cc
> @@ -6724,13 +6724,15 @@ static bool store_trigger(THD *thd, Trigger *trigger,
>table->field[14]->store(STRING_WITH_LEN("OLD"), cs);
>table->field[15]->store(STRING_WITH_LEN("NEW"), cs);
>
> - if (trigger->create_time)
> + if (trigger->ms_create_time)
>{
> +/* timestamp is in microseconds */
> table->field[16]->set_notnull();
> thd->variables.time_zone->gmt_sec_to_TIME(×tamp,
> -
> (my_time_t)(trigger->create_time/100));
> -/* timestamp is with 6 digits */
> -timestamp.second_part= (trigger->create_time % 100) * 1;
> + (my_time_t)
> + (trigger->ms_create_time/
> + 100));
> +timestamp.second_part= trigger->ms_create_time % 100;
hrtime_to_time() and hrtime_sec_part()
> ((Field_temporal_with_date*)
> table->field[16])->store_time_dec(×tamp,
> 2);
>}
> @@ -9810,12 +9812,14 @@ static bool show_create_trigger_impl(THD *thd,
> Trigger *trigger)
> trigger->db_cl_name.length,
> system_charset_info);
>
> - if (trigger->create_time)
> + if (trigger->ms_create_time)
>{
> MYSQL_TIME timestamp;
> thd->variables.time_zone->gmt_sec_to_TIME(×tamp,
> -
> (my_time_t)(trigger->create_time/100));
> -timestamp.second_part= (trigger->create_time % 100) * 1;
> + (my_time_t)
> + (trigger->ms_create_time/
> + 100));
> +timestamp.second_part= trigger->ms_create_time % 100;
ditto
> p->store(×tamp, 2);
>}
>else
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 673ea509e2c: MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
Hi, Marko, On Jan 21, Marko Mäkelä wrote: > > > Why InnoDB tests depend on server's symlink support? > > InnoDB does not use symlinks, it uses isl files. > > In https://jira.mariadb.org/browse/MDEV-26870 you suggested using > my_use_symdir, which I did. > That variable is bound to the Boolean start-up parameter symbolic_links. > > Changing InnoDB to use actual symlinks (and to stop creating > "databasename" directories, to be similar to other storage engines) > would be a file format change that could break downgrades to earlier > minor versions. Therefore, it is only doable in a development release. That's not what I meant. I was saying, --skip-symbolic-links and filesystem support of symbolic links are unrelated. If --skip-symbolic-links was not specified, a filesystem can support symbolic links or not support them. In the latter case, I think, InnoDB technically still could use DATA DIRECTORY, even if MyISAM cannot. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 673ea509e2c: MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
Hi, Marko,
On Jan 20, Marko Mäkelä wrote:
> revision-id: 673ea509e2c (mariadb-10.2.40-230-g673ea509e2c)
> parent(s): d28d3aee108
> author: Marko Mäkelä
> committer: Marko Mäkelä
> timestamp: 2022-01-20 16:13:08 +0200
> message:
>
> MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
>
> The InnoDB DATA DIRECTORY attribute is not implemented via
> symbolic links but something similar, *.isl files that contain
> the names of data files.
>
> InnoDB failed to reject the DATA DIRECTORY attribute even though
> the server was started with --skip-symbolic-links.
>
> Because TRUNCATE TABLE cannot easily return an error, it will keep
> rebuilding tables even if they carry the DATA DIRECTORY attribute.
>
> diff --git a/mysql-test/suite/encryption/t/innodb-first-page-read.test
> b/mysql-test/suite/encryption/t/innodb-first-page-read.test
> index d661e4565d2..a0b3ca3f0ff 100644
> --- a/mysql-test/suite/encryption/t/innodb-first-page-read.test
> +++ b/mysql-test/suite/encryption/t/innodb-first-page-read.test
> @@ -1,6 +1,7 @@
> -- source include/have_innodb.inc
> -- source include/have_file_key_management_plugin.inc
> -- source include/not_embedded.inc
> +-- source include/have_symlink.inc
when can have_symlink be false?
Why InnoDB tests depend on server's symlink support?
InnoDB does not use symlinks, it uses isl files.
> --disable_warnings
> SET GLOBAL innodb_file_format = `Barracuda`;
> diff --git a/storage/innobase/handler/ha_innodb.cc
> b/storage/innobase/handler/ha_innodb.cc
> index 5d78e64a06b..a8b16ba1eb0 100644
> --- a/storage/innobase/handler/ha_innodb.cc
> +++ b/storage/innobase/handler/ha_innodb.cc
> @@ -11710,9 +11710,15 @@ create_table_info_t::create_options_are_invalid()
> break;
> }
>
> - if (m_create_info->data_file_name
> - && m_create_info->data_file_name[0] != '\0'
> - && !create_option_data_directory_is_valid()) {
> + if (!m_create_info->data_file_name
> + || !m_create_info->data_file_name[0]) {
> + } else if (!my_use_symdir) {
> + push_warning(
> + m_thd, Sql_condition::WARN_LEVEL_WARN,
> + ER_ILLEGAL_HA_CREATE_OPTION,
> + "InnoDB: DATA DIRECTORY requires HAVE_SYMLINK.");
> + ret = "DATA DIRECTORY";
MyISAM does
push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
WARN_OPTION_IGNORED,
ER_THD(thd, WARN_OPTION_IGNORED),
"DATA DIRECTORY");
Archive does
if (create_info->data_file_name)
my_error(WARN_OPTION_IGNORED, MYF(ME_WARNING), "DATA DIRECTORY");
I suggest you do one of the above too.
> + } else if (!create_option_data_directory_is_valid()) {
> ret = "DATA DIRECTORY";
> }
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 99e2a49acfc: MDEV-27018 IF and COALESCE lose "json" property
Hi, Alexander, On Jan 18, Alexander Barkov wrote: > > Here's a new patch: > > https://github.com/MariaDB/server/commit/0478f474020466c16bfcbc9c7d0ed582a40e4fb8 Thanks! This is ok to push Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] e4ea1544097: MDEV-17124: mariadb 10.1.34, views and prepared statements: ERROR 1615 (HY000): Prepared statement needs to be re-prepared
t;/*
> If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql.
> However, it seems handy if com_stmt_prepare is increased always,
> diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
> index 52817ef65ae..38febce4fef 100644
> --- a/sql/sql_trigger.cc
> +++ b/sql/sql_trigger.cc
> @@ -929,7 +933,21 @@ bool Table_triggers_list::create_trigger(THD *thd,
> TABLE_LIST *tables,
>
>if (!sql_create_definition_file(NULL, &file, &triggers_file_type,
>(uchar*)this, triggers_file_parameters))
> + {
> +// delay if needed to be sure that time of creation is unique
> +for (;;)
> +{
> + my_hrtime_t hrtime= my_hrtime();
> + ulonglong current_time= make_trigger_time(hrtime_to_my_time(hrtime),
> +hrtime_sec_part(hrtime));
> + if (trigger->create_time == current_time)
> +pthread_yield();
> + else
> +break;
> +}
This shouldn't be needed.
Better
1. store statement->create_time with microsecond precision
2. store trigger->create_time with microsecond precision
3. at the end of prepare wait (like above) to make sure
statement->create_time is larger than all triggers' create_time.
this will be a no-op unless someone created a trigger and prepared in
the same microsecond. Safety check will ensure you won't wait longer
than one microsecond.
> DBUG_RETURN(false);
> + }
>
> err_with_cleanup:
>/* Delete .TRN file */
> diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> index 020830483c5..830a42b3cf3 100644
> --- a/sql/sql_view.cc
> +++ b/sql/sql_view.cc
> @@ -1131,7 +1141,31 @@ static int mysql_register_view(THD *thd, TABLE_LIST
> *view,
>DBUG_RETURN(error);
> }
>
> +/**
> + Check is TABLE_LEST and SHARE match
> + @param[in] viewTABLE_LIST of the view
> + @param[in] share Share object of view
> +
> + @return false on error or misspatch
> +*/
>
> +bool mariadb_view_version_get(TABLE_SHARE *share)
> +{
> + DBUG_ASSERT(share->is_view);
> +
> + if (!(share->tabledef_version.str=
> +(uchar*) alloc_root(&share->mem_root, VIEW_MD5_LEN + 1)))
> +return TRUE;
> + share->tabledef_version.length= VIEW_MD5_LEN;
> +
> + DBUG_ASSERT(share->view_def != NULL);
> + if (share->view_def->parse((uchar *) &share->tabledef_version, NULL,
> + view_md5_parameters, 1,
> + &file_parser_dummy_hook))
> +return TRUE;
> + DBUG_ASSERT(share->tabledef_version.length == VIEW_MD5_LEN);
> + return FALSE;
> +}
I don't like using md5 as a uuid. md5 collisions are very much possible.
Better options would be
1. use a timestamp, but store it with microsecond precisions, same logic
as with triggers.
2. use a true tabledef uuid, new field in the view frm, same logic as
with tables.
timestamp is better, because it already exists in frms, and any old view
will naturally have a timestamp before now(), so it doesn't matter that
it won't have microseconds.
uuid is better, because it's more universal, can be used for more than
just reprepare checks.
But for now I think timestamp benefits outweigh that.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
Hi, Aleksey! On Jan 13, Aleksey Midenkov wrote: > > > > But always using expr_arena doesn't necessarily seem to be a correct > > choice. E.g. Arg_comparator creates Item_cache_temporal on every > > fix_field. If vcol expr is re-fixed all the time, expr_arena will > > grow continuously. Same for charset conversion, CASE, IN, etc. I > > suspect that if the vcol expr is always re-fixed, it has to use stmt > > arena. > > stmt_arena cannot be used as long as 'expr' item is created on > expr_arena. 'expr' item is created at the vcol expression parse and is > reused during the whole TABLE lifetime. If some of the containee items > later were allocated on stmt_arena, 'expr' item will try to access > them in a different statement when they are already released. That is > what this bugfix tries to address. > > Yes, this fix introduces a small and in most cases imperceptible > memory leak which can be easily overcame by FLUSH TABLES. The fix for > this leak is non-trivial. > > 1. vcol expr is not always refixed, but conditionally controlled by > vcols_need_refixing. And we must remove this control and truly refix > always. > > 2. we must clone 'expr' item into statement mem_root at each > statement. > > I have attached the leak fix PoC into this email. I do not know > whether this is worth it. vcols_need_refixing isn't run-time conditional, it depends on vcol flags, a specific vcol always needs refixing or never does. And a table has vcols_need_refixing if any of the vcols nees it. Thus, I think, a simpler fix would be to split the cleanup and fix_fields. Now both are done in fix_session_vcol_expr(). We can do cleanup at the end of every statement and fix_fields at the beginning of a statement. Just like for normal non-persistent items. For every vcol that needs it. This way they can safely use execution arena. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 99e2a49acfc: MDEV-27018 IF and COALESCE lose "json" property
Hi, Alexander!
On Jan 17, Alexander Barkov wrote:
> >> diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc
> >> index ddf5fc32ea4..81003be4656 100644
> >> --- a/sql/item_jsonfunc.cc
> >> +++ b/sql/item_jsonfunc.cc
> >> @@ -1441,6 +1441,32 @@ longlong Item_func_json_contains_path::val_int()
...
> >> +static bool is_json_type(const Item *item)
> >> +{
> >> + for ( ; ; )
> >> + {
> >> +if
> >> (Type_handler_json_common::is_json_type_handler(item->type_handler()))
> >> + return true;
> >> +const Item_func_conv_charset *func;
> >> +if (!(func= dynamic_cast(item)))
> >> + return false;
> >> +item= func->arguments()[0];
> >
> > can you have nested CONVERT()'s ?
>
> We cannot have auto-generated nested CONVERT().
>
> The new code just reproduces the old behavior:
> Item_func_conv_charset::is_json_type() traversed
> through all Item_func_conv_charset's recursively.
>
> As the comment says, this is probably not correct.
> Let's fix it under terms of a separate MDEV.
> I can file an MDEV about this.
please, do
> >> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> >> index c1801c1ae3e..3b2753d80a6 100644
> >> --- a/sql/sql_type.cc
> >> +++ b/sql/sql_type.cc
> >> @@ -1755,19 +1755,110 @@ const Type_handler
> >> *Type_handler_typelib::cast_to_int_type_handler() const
> >>
> >>
> >> /***/
> >>
> >> +class Recursive_type_pair_iterator
> >> +{
> >> + const Type_handler *m_a;
> >> + const Type_handler *m_b;
> >> + uint m_switched_to_base_count;
> >> +public:
> >> + Recursive_type_pair_iterator(const Type_handler *a,
> >> + const Type_handler *b,
> >> + uint switched_to_base_count= 0)
> >> + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count)
> >> + { }
> >> + const Type_handler *a() const { return m_a; }
> >> + const Type_handler *b() const { return m_b; }
> >> + Recursive_type_pair_iterator base() const
> >> + {
> >> +Recursive_type_pair_iterator res(m_a->type_handler_base(),
> >> + m_b->type_handler_base());
> >> +res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL);
> >> +if (res.m_a == NULL)
> >> + res.m_a= m_a;
> >> +if (res.m_b == NULL)
> >> + res.m_b= m_b;
> >> +return res;
> >
> > that's an unusual semantics, not what I would expect from an iterator.
> > It'd expect it to iterare with it++; or it.next(); but not it = it.base();
>
> I agree it did not look nice.
>
> I replaced Recursive_type_pair_iterator to a more
> generic new class:
>
> class Type_handler_pair
> {
>const Type_handler *m_a;
>const Type_handler *m_b;
> ...
> };
why did you do it this way? You create an object. Then in the loop you
create a new object, copying the old one. Then you conditionally assign
m_a->type_handler_base(). Then you diff then to see if any condition
from the previous step has succeeded.
Why not, like
class Type_handler_pair {
bool up() {
bool r=false;
n_a=m_a->type_handler_base();
n_b=m_b->type_handler_base();
if (n_a) { m_a=n_a; r=true; }
if (n_b) { m_a=n_b; r=true; }
return r;
}
and later
do {
...
} while (tp.up());
also don't forget to update the commit comment, it still says
"Recursive_type_pair_iterator".
> >> + }
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 99e2a49acfc: MDEV-27018 IF and COALESCE lose "json" property
> + const char *name,
> + Type_handler_hybrid_field_type
> *hybrid,
> + Type_all_attributes *attr,
> + Item **items, uint nitems)
> + const override
> + {
> +if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr,
> + items, nitems))
> + return true;
> +/*
> + The above call can change the type handler on "hybrid", e.g.
> + choose a proper BLOB type handler according to the calculated
> max_length.
> + Convert general purpose string type handler to its JSON counterpart.
> + This makes hybrid functions preserve JSON data types, e.g.:
> +COALESCE(json_expr1, json_expr2) -> JSON
> +*/
> +
> hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler()));
When this line would change hybrid->type_handler() ?
> +return false;
> + }
> };
.
> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> index c1801c1ae3e..3b2753d80a6 100644
> --- a/sql/sql_type.cc
> +++ b/sql/sql_type.cc
> @@ -1755,19 +1755,110 @@ const Type_handler
> *Type_handler_typelib::cast_to_int_type_handler() const
>
> /***/
>
> +class Recursive_type_pair_iterator
> +{
> + const Type_handler *m_a;
> + const Type_handler *m_b;
> + uint m_switched_to_base_count;
> +public:
> + Recursive_type_pair_iterator(const Type_handler *a,
> + const Type_handler *b,
> + uint switched_to_base_count= 0)
> + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count)
> + { }
> + const Type_handler *a() const { return m_a; }
> + const Type_handler *b() const { return m_b; }
> + Recursive_type_pair_iterator base() const
> + {
> +Recursive_type_pair_iterator res(m_a->type_handler_base(),
> + m_b->type_handler_base());
> +res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL);
> +if (res.m_a == NULL)
> + res.m_a= m_a;
> +if (res.m_b == NULL)
> + res.m_b= m_b;
> +return res;
that's an unusual semantics, not what I would expect from an iterator.
It'd expect it to iterare with
it++
or
it.next()
but not
it = it.base()
> + }
> + bool done() const
> + {
> +switch (m_switched_to_base_count)
> +{
> +case 2:
I don't think case 2 is possible anymore.
...
> +}
> + }
> +};
> +
> +
> bool
> Type_handler_hybrid_field_type::aggregate_for_result(const Type_handler
> *other)
> {
> - const Type_handler *hres;
> - const Type_collection *c;
> - if (!(c= Type_handler::type_collection_for_aggregation(m_type_handler,
> other)) ||
> - !(hres= c->aggregate_for_result(m_type_handler, other)))
> -hres= type_handler_data->
> -m_type_aggregator_for_result.find_handler(m_type_handler, other);
> - if (!hres)
> - return true;
> - m_type_handler= hres;
> - return false;
> + Recursive_type_pair_iterator it(m_type_handler, other);
> + for ( ; ; )
do you really still need to do recursive? in what cases?
> + {
> +const Type_handler *hres;
> +const Type_collection *c;
> +if (((c= Type_handler::type_collection_for_aggregation(it.a(), it.b()))
> &&
> + (hres= c->aggregate_for_result(it.a(), it.b( ||
> +(hres= type_handler_data->
> +m_type_aggregator_for_result.find_handler(it.a(), it.b(
> +{
> + m_type_handler= hres;
> + return false;
> +}
> +if ((it= it.base()).done())
> + break;
> + }
> + return true;
> }
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 8f8e7ad5c80: MDEV-27217 DELETE partition selection doesn't work for history partitions
Hi, Aleksey!
ok to push.
see one comment below.
On Jan 13, Aleksey Midenkov wrote:
> revision-id: 8f8e7ad5c80 (mariadb-10.3.31-80-g8f8e7ad5c80)
> parent(s): 88f8aa20bba
> author: Aleksey Midenkov
> committer: Aleksey Midenkov
> timestamp: 2022-01-12 21:47:03 +0300
> message:
>
> MDEV-27217 DELETE partition selection doesn't work for history partitions
>
> LIMIT history switching requires the number of history partitions to
> be marked for read: from first to last non-empty plus one empty. The
> least we can do is to fail with error message if the needed partition
> was not marked for read. As this is handler interface we require new
> handler error code to display user-friendly error message.
>
> diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> index bd73642cd0d..2f945f29ed1 100644
> --- a/sql/ha_partition.cc
> +++ b/sql/ha_partition.cc
> @@ -9872,6 +9874,11 @@ void ha_partition::print_error(int error, myf errflag)
> m_part_info->print_no_partition_found(table, errflag);
> DBUG_VOID_RETURN;
>}
> + else if (error == HA_ERR_PARTITION_LIST)
> + {
> +handler::print_error(error, errflag);
> +DBUG_VOID_RETURN;
> + }
this seems to be redundant, the else branch at the end covers it.
>else if (error == HA_ERR_ROW_IN_WRONG_PARTITION)
>{
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] ef70e809a40: MDEV-27217 DELETE partition selection doesn't work for history partitions
Hi, Aleksey! On Jan 12, Aleksey Midenkov wrote: > revision-id: ef70e809a40 (mariadb-10.3.31-80-gef70e809a40) > parent(s): 88f8aa20bba > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-27217 DELETE partition selection doesn't work for history partitions > > LIMIT history switching requires the number of history partitions to > be marked for read: from first to last non-empty plus one empty. The > least we can do is to fail with error message if the needed partition > was not marked for read. As this is handler interface we require new > handler error code to display user-friendly error message. > > Switching by INTERVAL works out-of-the-box with > ER_ROW_DOES_NOT_MATCH_GIVEN_PARTITION_SET error. Code-wise it's pretty much fine. Two problems related to error messages: We cannot have part of the error message a hard-coded English phrase. It doesn't work with localized error messages: "Невозможно выбрать раздел: modifying system versioned table" Also we cannot add new error messages in 10.3. Or any old GA. Currently you can add new error messages to 10.6 or any later version. You can reuse ER_UNUSED_1 for your new error message, that'll work. And, because you cannot have English phrases in it, I'd suggest something more generic, like Not allowed for system-versioned table %`s.%`s In the context of delete from t1 partition (p1); ERROR HY000: Not allowed for system-versioned table `test`.`t1` it seems to be quite clear. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
Hi, Rucha!
On Jan 12, Rucha Deodhar wrote:
> On Tue, Jan 11, 2022 at 11:49 PM Sergei Golubchik wrote:
> > > +--error ER_PARSE_ERROR
> > > +INSERT INTO t1 VALUES XXX (1),(2);
> >
> > > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > > index 8ebefbb3d82..47ed3ded4a7 100644
> > > --- a/sql/sql_yacc.yy
> > > +++ b/sql/sql_yacc.yy
> > > @@ -8426,6 +8426,8 @@ query_specification_start:
> > >{
> > > SELECT_LEX *sel;
> > > LEX *lex= Lex;
> > > +if (lex->sql_command == SQLCOM_INSERT)
> > > + thd->get_stmt_da()->inc_current_row_for_warning();
> > > if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel))
> > >MYSQL_YYABORT;
> >
> > better not to have an if() in this rule that's used almost for
> > everything.
>
> When we have INSERT...SELECT counter has to be 1 for errors like
> ER_ILLEGAL_VALUE_FOR_TYPE which are caught during parsing.
> Example:
> INSERT INTO t1 SELECT a, b, 1.79769313486232e+308 FROM t2;
>
> So I added if() to increment the counter if we are parsing INSERT so
> that ROW_NUMBER for statements like the one above gives ROW_NUMBER=1.
I see.
But you put a condition on every query's execution path to fix the case
that, I believe, literally nobody cares about.
I don't think any user ever will check the ROW_NUMBER after the syntax
error :) And if someone will, we can say that in
INSERT INTO t1 VALUES XXX (1),(2);
the first row starts after VALUES, so ROW_NUMBER=1 is expected.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
Hi, Kristian! On Jan 11, Kristian Nielsen wrote: > Sergei Golubchik writes: > > >> To the question of the usage of trX cache by non-trX let me answer > >> broadly to mention @@binlog_direct_non_transactional_update = false > >> leads to aggregation of mixed, say innodb + myisam, events in trx > >> cache. > > > > That's different. The bug summary is "GTID event falsely marked > > transactional", meaning, the group of events is not transactional > > and it's falsely marked as transactional. If you have an > > innodb/myisam mix, it is correctly marked transactional. > > Jumping into a disucssion here, so apologies if I misunderstood, > but... In a way :) It doesn't answer any of my questions, but it's an important insight - I didn't know that and how I have even more questions. Thanks for this clarification, it was very helpful! > The point of the "transactional" mark in GTID is to inform parallel > replication that the entire event group is safe for optimistic > parallel replication - basically that it can be rolled back. > > So if a event group contains innodb/myisam mix, and is marked as > "transactional", that is not correct. This would allow parallel > replication to speculatively execute and roll back the mix, and the > myisam changes would remain and cause replication to break. Andrei, so now I have not one but two use cases: 1. InnoDB and FederatedX (or any other transactional but not XA-capable engine) in one transaction - this transaction can be speculatively executed and rolled back if needed. But it doesn't end with Xid_log_event, and after your patch it won't have the trans flag. 2. InnoDB and MyISAM. This "transaction" cannot be rolled back, so must not be executed speculatively. But it does end with a Xid_log_event, so it seems that you'll put a trans flag on it. The first case is a performance issue, but not a problem of correctness. Some some transactions might not be executed speculatively, but everything will still work. The second case can break replication when a speculatively executed InnoDB+MyISAM "transaction" will fail to roll back. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
Hi, Andrei! On Jan 11, Andrei Elkin wrote: > On Tue, Jan 11, 2022 at 1:06 PM Sergei Golubchik wrote: > > > Hi, Andrei! > > > > On Jan 11, Andrei Elkin wrote: > > > Howdy, Sergei! > > > > > > To the question of the usage of trX cache by non-trX let me answer > > > broadly to mention @@binlog_direct_non_transactional_update = false > > > leads to aggregation of mixed, say innodb + myisam, events in trx > > > cache. > > > > That's different. The bug summary is "GTID event falsely marked > > transactional", meaning, the group of events is not transactional and > > it's falsely marked as transactional. If you have an innodb/myisam > > mix, it is correctly marked transactional. > > > > You need to have only non-transactional events in the transactional > > cache. And I'm trying to understand how can that happen. > > Got your very specific point is about a pure myisam can be trx-cached. > This is OPTION_BINLOG_GTID >commit 5426facdcbfba2d78dab3c709cbf278073383b7c 's decision > >> - All tables are regarded as transactional tables in the binary log > (to ensure things are executed exactly as on the master) Okay, thanks, that answers my question Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 7a746072a5e: MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
Hi, Rucha!
On Jan 11, Rucha Deodhar wrote:
> revision-id: 7a746072a5e (mariadb-10.7.1-26-g7a746072a5e)
> parent(s): 7dfaded9625
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2022-01-05 12:05:22 +0530
> message:
>
> MDEV-26878: Query failing with syntax error sets ROW_NUMBER to non-zero
>
> Analysis: m_current_row_for_warning counter is set to 0 at the beginning
> of parsing.
> Fix: Reset it to 1 and increment it before starting to parse insert values,
> during SET and SELECT for INSERT...SET statement and INSERT...SELECT
> respectively.
>
> diff --git a/mysql-test/main/get_diagnostics.result
> b/mysql-test/main/get_diagnostics.result
> diff --git a/mysql-test/main/get_diagnostics.test
> b/mysql-test/main/get_diagnostics.test
> index e8d81dca1e6..467f624c49c 100644
> --- a/mysql-test/main/get_diagnostics.test
> +++ b/mysql-test/main/get_diagnostics.test
> @@ -1687,3 +1687,18 @@ GET DIAGNOSTICS CONDITION 3 @n = ROW_NUMBER;
> SELECT @n;
>
> DROP TABLE t;
> +
> +--echo #
> +--echo # MDEV-26878: Query failing with syntax error sets ROW_NUMBER to
> +--echo # non-zero
> +--echo #
> +
> +CREATE TABLE t1 (id1 INT);
> +
> +--error ER_PARSE_ERROR
> +INSERT INTO t1 VALUES XXX (1),(2);
please add a test, like,
INTO t1 VALUES (1), (ALL);
what ROW_NUMBER will it show?
> +
> +GET DIAGNOSTICS CONDITION 1 @n= ROW_NUMBER;
> +SELECT @n;
> +
> +DROP TABLE t1;
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index 8ebefbb3d82..47ed3ded4a7 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -8426,6 +8426,8 @@ query_specification_start:
>{
> SELECT_LEX *sel;
> LEX *lex= Lex;
> +if (lex->sql_command == SQLCOM_INSERT)
> + thd->get_stmt_da()->inc_current_row_for_warning();
> if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel))
>MYSQL_YYABORT;
better not to have an if() in this rule that's used almost for
everything.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 8e21b91fd31: MDEV-26875: Wrong user in SET DEFAULT ROLE error
Hi, Anel!
On Jan 11, Anel Husakovic wrote:
> revision-id: 8e21b91fd31 (mariadb-10.5.12-71-g8e21b91fd31)
> parent(s): e5a9dcfda20
> author: Anel Husakovic
> committer: Anel Husakovic
> timestamp: 2021-10-25 14:43:38 +0200
> message:
>
> MDEV-26875: Wrong user in SET DEFAULT ROLE error
>
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index 65f053aa09b..0a59f5fce37 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -3273,7 +3273,7 @@ static int check_user_can_set_role(THD *thd, const char
> *user,
>{
> /* Role is not granted but current user can see the role */
> my_printf_error(ER_INVALID_ROLE, "User %`s@%`s has not been granted
> role %`s",
> -MYF(0), thd->security_ctx->priv_user,
> +MYF(0), user,
> thd->security_ctx->priv_host, rolename);
That's strange. you'll use `user` and `thd->security_ctx->priv_host` ?
They generally correspond to two different accounts.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b14b6f31fa2: MDEV-26843: Inconsistent behavior of ROW_NUMBER upon resignalling from function Analysis: ROW_NUMBER should be 0 because the error produced is not related to statem
Hi, Rucha!
What if you have
INSERT t1 VALUES (f1(1)), (f1(2)), (f1());
?
On Jan 11, Rucha Deodhar wrote:
> revision-id: b14b6f31fa2 (mariadb-10.6.1-143-gb14b6f31fa2)
> parent(s): d555ae38381
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-10-21 17:13:13 +0530
> message:
>
> MDEV-26843: Inconsistent behavior of ROW_NUMBER upon resignalling from
> function
> Analysis: ROW_NUMBER should be 0 because the error produced is not related to
> statement that modifies table data. It is just wrong number of arguments.
> However, ROW_NUMBER is 1 because m_current_row_for_warning is not set to 0
> before executing function.
> Fix: Set m_current_row_for_warning to 0.
>
> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> index d1cf5c2d9bd..ebae2f356bd 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -2033,6 +2033,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint
> argcount,
>*/
>if (argcount != m_pcont->context_var_count())
>{
> +thd->get_stmt_da()->reset_current_row_for_warning(0);
> /*
>Need to use my_error here, or it will not terminate the
>invoking query properly.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 394bb653b0b: MDEV-26695: Number of an invalid row is not calculated for table value constructor
Hi, Rucha!
please, see below
On Jan 11, Rucha Deodhar wrote:
> revision-id: 394bb653b0b (mariadb-10.6.1-140-g394bb653b0b)
> parent(s): 3c857a07d92
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-10-20 01:30:04 +0530
> message:
>
> MDEV-26695: Number of an invalid row is not calculated for table value
> constructor
>
> Analysis: counter does not increment while sending rows for table value
> constructor and so row_number assumes the default value (0 in this case).
> Fix: Increment the counter to avoid counter using default value.
>
> diff --git a/sql/sql_tvc.cc b/sql/sql_tvc.cc
> index 49f319b3856..a191ec9c328 100644
> --- a/sql/sql_tvc.cc
> +++ b/sql/sql_tvc.cc
> @@ -422,7 +422,9 @@ bool table_value_constr::exec(SELECT_LEX *sl)
>DBUG_ENTER("table_value_constr::exec");
>List_iterator_fast li(lists_of_values);
>List_item *elem;
> + THD *cur_thd= sl->parent_lex->thd;
>ha_rows send_records= 0;
> + int rc;
>
>if (select_options & SELECT_DESCRIBE)
> DBUG_RETURN(false);
> @@ -438,16 +440,21 @@ bool table_value_constr::exec(SELECT_LEX *sl)
>
>while ((elem= li++))
>{
> +cur_thd->get_stmt_da()->inc_current_row_for_warning();
> if (send_records >= sl->master_unit()->lim.get_select_limit())
> - break;
> + goto reset_counter_and_exit;
Why goto, break worked here just fine, didn't it?
Also, note that if you jump from here out of the loop, you'll do
if (rc>0) where rc wasn't initialized yet.
> -int rc=
> - result->send_data_with_check(*elem, sl->master_unit(), send_records);
> +rc= result->send_data_with_check(*elem, sl->master_unit(), send_records);
> if (!rc)
>send_records++;
> else if (rc > 0)
> - DBUG_RETURN(true);
> + goto reset_counter_and_exit;
break here too?
>}
>
> +reset_counter_and_exit:
> + cur_thd->get_stmt_da()->reset_current_row_for_warning(0);
why do you need to reset here?
> + if (rc>0)
> +DBUG_RETURN(true);
> +
>if (result->send_eof())
> DBUG_RETURN(true);
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
Hi, Andrei!
On Jan 11, Andrei Elkin wrote:
> Howdy, Sergei!
>
> To the question of the usage of trX cache by non-trX let me answer
> broadly to mention @@binlog_direct_non_transactional_update = false
> leads to aggregation of mixed, say innodb + myisam, events in trx
> cache.
That's different. The bug summary is "GTID event falsely marked
transactional", meaning, the group of events is not transactional and
it's falsely marked as transactional. If you have an innodb/myisam mix,
it is correctly marked transactional.
You need to have only non-transactional events in the transactional
cache. And I'm trying to understand how can that happen.
> I may need to process deeper the referred comments though.
>
> Next to the commit's idea, it actually covers your concern. I wonder
> on the 'Wrong' conclusion. There is no intent to xidify anything
> extra. Th commit assumes xid eVent is created, before Gtid one,
> accordingly. And that fact is propagated to Gtid ctor.
Your commit, if I'm not mistaken, assumes that "last event is Xid if and
only if the cache contains a transaction".
Which is incorrect, a transaction does not necesarily have to end with a
Xid event.
/Sergei
> On Mon, 10 Jan 2022, 20:56 Sergei Golubchik, wrote:
>
> > Hi, Andrei!
> >
> > On Jan 10, Andrei Elkin wrote:
> > > revision-id: 9ea85a70a75 (mariadb-10.2.40-4-g9ea85a70a75)
> > > parent(s): 160d97a4aaa
> > > author: Andrei Elkin
> > > committer: Andrei Elkin
> > > timestamp: 2021-08-08 14:20:57 +0300
> > > message:
> > >
> > > MDEV-24654 GTID event falsely marked transactional
> > >
> > > GTID event can be falsely marked transactional in few cases including
> > > binary-logging on the slave side upon execution.
> > > In some execution branches bin-logging of non-transactional
> > > events can be done through transactional cache. E.g see comments
> > > in THD::binlog_write_table_map().
> >
> > I saw that and still couldn't understand why bin-logging of
> > non-transactional events can be done thr-ough transactional cache.
> >
> > > In order to not create the false 'trans' tag the fact of Xid event
> > > logging is checked to compute correct argument to Gtid_log_event
> > > ctor.
> >
> > This is wrong. Transactions only end with a Xid event if all
> > participating engines support XA. Try to create an InnoDB+FederatedX
> > transaction (I just did) and it'll end with Query_log_event("COMMIT")
> >
> > > diff --git a/sql/log.cc b/sql/log.cc
> > > index 1d9b4645421..7fef2e0d739 100644
> > > --- a/sql/log.cc
> > > +++ b/sql/log.cc
> > > @@ -8118,9 +8118,11 @@
> > MYSQL_BIN_LOG::write_transaction_or_stmt(group_commit_entry *entry,
> > > uint64 commit_id)
> > > {
> > >binlog_cache_mngr *mngr= entry->cache_mngr;
> > > + bool has_xid= entry->end_event->get_type_code() == XID_EVENT;
> > >DBUG_ENTER("MYSQL_BIN_LOG::write_transaction_or_stmt");
> > >
> > > - if (write_gtid_event(entry->thd, false, entry->using_trx_cache,
> > commit_id))
> > > + if (write_gtid_event(entry->thd, false,
> > > + entry->using_trx_cache && has_xid, commit_id))
> > > DBUG_RETURN(ER_ERROR_ON_WRITE);
> > >
> > >if (entry->using_stmt_cache && !mngr->stmt_cache.empty() &&
> > >
> > Regards,
> > Sergei
> > VP of MariaDB Server Engineering
> > and secur...@mariadb.org
> >
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] ce69e0c8e78: MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable
Hi, Rucha! First, note that it's now a 10.9 task, so don't push it into 10.8, please. As for the patch - code-wise it's all good. New mode names are confusing and should be clarified, see the comment below. On Jan 10, Rucha Deodhar wrote: > revision-id: ce69e0c8e78 (mariadb-10.6.1-88-gce69e0c8e78) > parent(s): 22556499397 > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-10-06 13:25:29 +0530 > message: > > MDEV-24920: Merge "old" SQL variable to "old_mode" sql variable > > Analysis: There are 2 server variables- "old_mode" and "old". "old" is > no longer needed as "old_mode" has replaced it (however still used in > some places in the code). "old_mode" and "old" has same purpose- > emulate behavior from previous MariaDB versions. So they can be merged > to avoid confusion. Fix: Deprecate "old" variable and create another > mode for @@old_mode to mimic behavior of previous "old" variable. > Create specific modes for specifix task that --old sql variable was > doing earlier and use the new modes instead. > > diff --git a/mysql-test/suite/sys_vars/r/old_mode_basic.result > b/mysql-test/suite/sys_vars/r/old_mode_basic.result > index a6b95f1c60c..2f3742160eb 100644 > --- a/mysql-test/suite/sys_vars/r/old_mode_basic.result > +++ b/mysql-test/suite/sys_vars/r/old_mode_basic.result > @@ -264,3 +264,42 @@ SET @@collation_database = @save_collation_database; > # > # End of 10.6 test > # > +# > +# Beginning of 10.7 test > +# 10.9 now > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index c2a219ee015..d1eaacd777c 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3755,6 +3774,8 @@ static const char *old_mode_names[]= >"NO_PROGRESS_INFO", >"ZERO_DATE_TIME_CAST", >"UTF8_IS_UTF8MB3", > + "INDEX_MASKING", > + "CHECKSUM_FORMATTING", that's confusing. I don't understsand from the name of the option what "CHECKSUM_FORMATTING" or "INDEX_MASKING" changes. "NO_PROGRESS_INFO" is clear, there won't be progress info "UTF8_IS_UTF8MB3" is clear "ZERO_DATE_TIME_CAST" is kind of understandable too, CAST( AS DATETIME) will be "-00-00 ", that is zero date CHECKSUM_FORMATTING and INDEX_MASKING is unclear. For CHECKSUM_FORMATTING I'd suggest CHECKSUM_SLOW_NULLS (see the commit that introduced this behavior: 496741d5761f) "INDEX_MASKING" was introduced in the commit 79542930ea1c, see the comment there, it explains what was done. This should help you to suggest a better name for this mode. >0 > }; > > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9ea85a70a75: MDEV-24654 GTID event falsely marked transactional
Hi, Andrei!
On Jan 10, Andrei Elkin wrote:
> revision-id: 9ea85a70a75 (mariadb-10.2.40-4-g9ea85a70a75)
> parent(s): 160d97a4aaa
> author: Andrei Elkin
> committer: Andrei Elkin
> timestamp: 2021-08-08 14:20:57 +0300
> message:
>
> MDEV-24654 GTID event falsely marked transactional
>
> GTID event can be falsely marked transactional in few cases including
> binary-logging on the slave side upon execution.
> In some execution branches bin-logging of non-transactional
> events can be done through transactional cache. E.g see comments
> in THD::binlog_write_table_map().
I saw that and still couldn't understand why bin-logging of
non-transactional events can be done through transactional cache.
> In order to not create the false 'trans' tag the fact of Xid event
> logging is checked to compute correct argument to Gtid_log_event
> ctor.
This is wrong. Transactions only end with a Xid event if all
participating engines support XA. Try to create an InnoDB+FederatedX
transaction (I just did) and it'll end with Query_log_event("COMMIT")
> diff --git a/sql/log.cc b/sql/log.cc
> index 1d9b4645421..7fef2e0d739 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -8118,9 +8118,11 @@
> MYSQL_BIN_LOG::write_transaction_or_stmt(group_commit_entry *entry,
> uint64 commit_id)
> {
>binlog_cache_mngr *mngr= entry->cache_mngr;
> + bool has_xid= entry->end_event->get_type_code() == XID_EVENT;
>DBUG_ENTER("MYSQL_BIN_LOG::write_transaction_or_stmt");
>
> - if (write_gtid_event(entry->thd, false, entry->using_trx_cache, commit_id))
> + if (write_gtid_event(entry->thd, false,
> + entry->using_trx_cache && has_xid, commit_id))
> DBUG_RETURN(ER_ERROR_ON_WRITE);
>
>if (entry->using_stmt_cache && !mngr->stmt_cache.empty() &&
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 88f8aa20bba: MDEV-23486 RBR can bypass secure_timestamp=YES
Hi, Aleksey! On Jan 09, Aleksey Midenkov wrote: > revision-id: 88f8aa20bba (mariadb-10.3.31-79-g88f8aa20bba) > parent(s): d5451867af1 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-23 04:15:39 +0300 > message: > > MDEV-23486 RBR can bypass secure_timestamp=YES > > Pass to a slave whether master table is system-versioned via > TM_SYSTEM_VERSIONED flag. That deprecates m_vers_from_plain > detection. If secure_timestamp == YES or master did not supply row_end > data treat the row as current data and generate timestamps for it. I don't understand why does it matter whether the master is system versioned or not. secure_timestamp=YES means the table has to store actual timestamps when rows were changed. It doesn't trust the master to set timestamps. So it should not matter whether master table is versioned or not, if you don't trust the master, you cannot trust its TM_SYSTEM_VERSIONED flag either. Even if the table was system versioned on the master, the master could've had secure_timestamp=NO and a completely fake history. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] ce507903d0c: MDEV-22742 UBSAN: Many overflow issues in strings/decimal.c - runtime error: signed integer overflow: x * y cannot be represented in type 'long long int' (on optimi
Hi, Alexey!
On Jan 09, Alexey Botchkov wrote:
> revision-id: ce507903d0c (mariadb-10.2.40-161-gce507903d0c)
> parent(s): 0dae41637ab
> author: Alexey Botchkov
> committer: Alexey Botchkov
> timestamp: 2021-11-22 09:58:46 +0400
> message:
>
> MDEV-22742 UBSAN: Many overflow issues in strings/decimal.c - runtime error:
> signed integer overflow: x * y cannot be represented in type 'long long int'
> (on optimized builds).
>
> Avoid integer overflow, do the check before the calculation.
>
> diff --git a/strings/decimal.c b/strings/decimal.c
> index 9d18a9ce52a..6249d7e097a 100644
> --- a/strings/decimal.c
> +++ b/strings/decimal.c
> @@ -1128,13 +1128,16 @@ int decimal2ulonglong(const decimal_t *from,
> ulonglong *to)
>
>for (intg=from->intg; intg > 0; intg-=DIG_PER_DEC1)
>{
> -ulonglong y=x;
> -x=x*DIG_BASE + *buf++;
> -if (unlikely(y > ((ulonglong) ULONGLONG_MAX/DIG_BASE) || x < y))
> +if (unlikely (
> + x >= ULONGLONG_MAX/DIG_BASE &&
> + (x > ULONGLONG_MAX/DIG_BASE ||
> + *buf > (dec1) (ULONGLONG_MAX%DIG_BASE
This took me a while. Personally I find it easier to understand an
exclusive condition, like
x > ULONGLONG_MAX/DIG_BASE ||
(x == ULONGLONG_MAX/DIG_BASE && *buf > (dec1) (ULONGLONG_MAX%DIG_BASE))
but it's equivalent to your version, so ok to push.
Did you check that this commit fixes all UBSAN issues mentioned in the
MDEV-22742?
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 0dfe5a31573: MDEV-22441 implement a generic way to change a value of a variable in a scope
Hi, Eugene!
On Jan 09, Eugene Kosov wrote:
> revision-id: 0dfe5a31573 (mariadb-10.7.1-13-g0dfe5a31573)
> parent(s): 1e54a9716d2
> author: Eugene Kosov
> committer: Eugene Kosov
> timestamp: 2021-12-03 16:12:27 +0600
> message:
>
> MDEV-22441 implement a generic way to change a value of a variable in a scope
>
> Example:
> {
> auto _= make_scope_value(var, tmp_value);
> }
>
> make_scope_value(): a function which returns RAII object which temporary
> changes a value of a variable
1. Why have you not created a macro, like SCOPE_EXIT to avoid `auto _=` ?
Particularly as one cannot use `_` to backup more than one variable.
2. better remove the new value from make_scope_value(). Creating a RAII
backup and an assignment of a new value are two separate actions. The
assignment can happen much later or conditionally or not at all.
3. (not part of MDEV-22441) See how SCOPE_EXIT is implemented in
rocksdb, perhaps we should do it similarly? It's used like
SCOPE_EXIT { mem_heap_free(heap); }
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
Hi, Aleksey! Could you please explain in the commit comment what the bug was and what you are fixing? I wasn't able to understad that, so I had to debug those crashes myself. So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different. I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere. But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena. Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over? We've already discussed VCOL_TABLE_ALIAS, no need to go there again. On Jan 06, Aleksey Midenkov wrote: > revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) > parent(s): a565e63c54c > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-12-29 00:46:31 +0300 > message: > > MDEV-24176 Server crashes after insert in the table with virtual > column generated using date_format() and if() > > When table is reopened from cache vcol_info contains stale > expression. We refresh expression via TABLE::vcol_fix_exprs() but > first we must prepare a proper context (Vcol_expr_context) and do some > preparations which meet some requirements: > > 1. fields must not be fixed, we unfix them with cleanup() via > cleanup_processor. > > 2. Item_ident must have cached_table cleared. Again, this is stale > data which was already freed. cached_table interferes with > find_field_in_tables(). > > We cannot clear cached_table directly in Item_ident::cleanup() > method. Lots of spaghetti logic depends on cached_table existence even > after cleanup() done. > > 3. If Item_ident has table_name non-NULL we trigger expr update. That > is done via Item_ident::check_vcol_func_processor() and > VCOL_TABLE_ALIAS flag. > > (4-7) The below conditions are prepared by Vcol_expr_context and used in > both fix_session_expr_for_read() and TABLE::vcol_fix_exprs(): > > 4. Any fix_expr() must be done on expr_arena as there may be new items > created. It was a bug in fix_session_expr_for_read(). It was just not > reproduced because there were no second refix. Now refix is done for > more cases so it does reproduce. Tests affected: vcol.binlog > > 5. Also name resolution context must be narrowed to the single table > with all crutches in place. Tests affected: vcol.update > > 6. sql_mode must be clean and not fail expr update. > > sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc > must not affect vcol expression update. If the table was created > successfully any further evaluation must not fail. Tests affected: > main.func_like > > 7. Warnings are disabled during expr update. It is assumed that these > warnings were printed before during initialization phase or later > during execution phase. Tests affected: vcol.wrong_arena > > Dictionary: refresh, update and refix are the synonyms for > vcol_fix_exprs() or fix_session_expr_for_read() calls. > > 8. cleanup_excluding_fields_processor() removed. It was just a quick > hack to exclude wrongly working Item_field from processing. Now it > works due to correct execution environment (Vcol_expr_context). > Related to MDEV-10355. > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 3617add6975: MDEV-27208: Extend CRC32() and implement CRC32C()
Hi, Marko!
On Jan 03, Marko Mäkelä wrote:
> revision-id: 3617add6975 (mariadb-10.6.1-252-g3617add6975)
> parent(s): bc90f5d6967
> author: Marko Mäkelä
> committer: Sergei Golubchik
> timestamp: 2021-12-15 19:54:34 +0100
> message:
>
> MDEV-27208: Extend CRC32() and implement CRC32C()
>
> We used to define a native unary function crc32() that computes the CRC-32
> of a string using the ISO 3309 polynomial that is being used by zlib
> and many others.
Suggestions:
1. one class for both CRC functions, like
class Item_func_crc32 :public Item_long_func
{
extern "C" unsigned (*crc_func)(unsigned int, const char *, size_t);
and in the constructor
crc_func= crc_variant ? my_crc32 : my_crc32c;
there's no need to duplicate everything.
2. swap the arguments:
CRC32(CRC32("Maria"), "DB") = CRC32("MariaDB")
looks kind of better than
CRC32("DB", CRC32("Maria")) = CRC32("MariaDB")
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 734aee3cd3c: MDEV-27018 IF and COALESCE lose "json" property
Hi, Alexander!
> - We'll probably have more FORMATs in the future.
Like what? As far as I can see, standard doesn't support FORMAT in the
column definition at all.
>Adding all format tests inside "normal"
>Field_string/Field_varchar/Field_blob
>is not a good idea.
>
> >> + */
> >> + if (Type_handler_json_common::has_json_valid_constraint(this))
> >
> > could you please use names that don't depend on how exactly we detect
> > json? especially if the goal is to move to FORMAT JSON syntax
>
> For now this name reflects what it actually does.
>
> When we split JSON off and fix the code to have JSON put extra
> data type information into FRM, this test will be gone from
> Field_xxx.
>
> But it will move to the code opening FRM, to be able open
> old tables where JSON is detected only by a CHECK
> constraint. And there it will reflect what it actually
> does again.
>
> I propose to keep it as is.
ok
> >> diff --git a/sql/sql_type.cc b/sql/sql_type.cc
> >> index c1801c1ae3e..3b2753d80a6 100644
> >> --- a/sql/sql_type.cc
> >> +++ b/sql/sql_type.cc
> >> @@ -1755,19 +1755,110 @@ const Type_handler
> >> *Type_handler_typelib::cast_to_int_type_handler() const
> >>
> >>
> >> /***/
> >>
> >> +class Recursive_type_pair_iterator
> >> +{
> >> + const Type_handler *m_a;
> >> + const Type_handler *m_b;
> >> + uint m_switched_to_base_count;
> >> +public:
> >> + Recursive_type_pair_iterator(const Type_handler *a,
> >> + const Type_handler *b,
> >> + uint switched_to_base_count= 0)
> >> + :m_a(a), m_b(b), m_switched_to_base_count(switched_to_base_count)
> >> + { }
> >> + const Type_handler *a() const { return m_a; }
> >> + const Type_handler *b() const { return m_b; }
> >> + Recursive_type_pair_iterator base() const
> >> + {
> >> +Recursive_type_pair_iterator res(m_a->type_handler_base(),
> >> + m_b->type_handler_base());
> >> +res.m_switched_to_base_count= (res.m_a != NULL) + (res.m_b != NULL);
> >> +if (res.m_a == NULL)
> >> + res.m_a= m_a;
> >> +if (res.m_b == NULL)
> >> + res.m_b= m_b;
> >> +return res;
> >> + }
> >> + bool done() const
> >> + {
> >> +switch (m_switched_to_base_count)
> >> +{
> >> +case 2:
> >> + /*
> >> +Expression:
> >> + COALESCE(TYPE1, TYPE2)
> >> +
> >> +where both type handlers derive from some other handlers, e.g.
> >> + VARCHAR -> TYPE1
> >> + TEXT-> TYPE2
> >> +
> >> +Continue aggregation as:
> >> + SELECT COALESCE(VARCHAR, TEXT)
> >> +
> >> +Note, we now have only one level of data type inheritance,
> >> + e.g. VARCHAR -> VARCHAR/JSON
> >> +
> >> +This code branch will change when we have longer inheritance
> >> chains:
> >> + A0 -> A1 -> A2 -> A3
> >> + B0 -> B1 -> B2
> >> +
> >> +Functions like COALESE(A2, B2) will need to do full overload
> >> resolution:
> >> +- iterate through all pairs from the below matrix
> >> +- find all existing candidates
> >> +- resolve ambiguities in case multiple candidates, etc.
> >
> > and I don't see how you're going to do that. you need to try
> > m_a+m_b.base(), m_a.base()+m_b, and m_a.base()+m_b.base().
> > And that's not what you're doing.
> >
> > I suggest to add an assert that m_switched_to_base_count < 2
> > and stop trying to solve complex cases until we have at least one.
>
> Yes, we'll need to do all these combinations eventually.
> But I'm not trying to solve a more complex case
> than it's really necessary now:
>
> If the SQL query is:
>
>SELECT varchar_json_expr + text_json_expr FROM t1;
>
> it works as follows:
>
> - It tries VARCHAR/JSON+TEXT/JSON in Type_collection_json,
>nothing is found
>
> - Then it tries directly VARCHAR+TEXT,
>which is m_a.base()+m_b.base(), and this
>combination is found in Type_collection_std,
I'd expect it to find VARCHAR/JSON+TEXT/JSON right away.
It would've nat
Re: [Maria-developers] 8a84f5a40c1: MDEV-24176 Preparations
Hi, Aleksey! On Dec 29, Aleksey Midenkov wrote: > > > > > + /* > > > > > + NOTE: came from TABLE::alias_name_used and this is only a hint! > > > > > It works > > > > > + only in need_correct_ident() condition. On other cases it is > > > > > FALSE even if > > > > > + table_name is alias! It cannot be TRUE in these cases, lots of > > > > > spaghetti > > > > > + logic depends on that. > > > > > > > > could you elaborate on that? > > I see that. What I mean was, can you show an example? > > Say, a query that sets TABLE::alias_name_used incorrectly? > > I wasn't able to find a test case for that. > > That's the original test case of the task. It has alias_name_used > false. Well, early I had more problems with that. Now I did the PoC > fix again and I see no big problems with the tests > (bb-10.2-midenok-tmp). Does it mean, you'll remove the comment (and workarounds, if any) in your branch? > > Also, why do you even want to re-fix items when a table alias is > > used? Is it for MDEV-25672? But that's already fixed for half a > > year. > > Originally this fix was done before the pushed one. And do you really > want to keep bad value in Item_field? Even if you avoid using it now > someone surely will suffer from that after some new development or > when new use cases happen. If there is a ready fix that eliminates > such an accident in the future why don't you want to push it? I generally prefer not to do work that's not needed. That's called lazy :) If there's a value that's not used, ever, then I would suggest not to spend time cleaning it up. Re-preparing vcols after every DML that happened to use table aliases - that's not lazy, that's too much work. As far as MDEV-25672 is concerned - making ALTER TABLE to check that existing vcols refer to the correct table - that's too much work too, we already know they do. Only new columns need to be checked. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 803b977229c: MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same table: rows are counted twice
Hi, Rucha!
Ok to push. One suggestion below.
On Dec 31, Rucha Deodhar wrote:
> revision-id: 803b977229c (mariadb-10.2.40-192-g803b977229c)
> parent(s): 42fea34d4a9
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-12-29 16:09:10 +0530
> message:
>
> MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same
> table: rows are counted twice
>
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index a331f4f3dbc..c8d5170f568 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -18972,7 +18972,7 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
>bool shortcut_for_distinct= join_tab->shortcut_for_distinct;
>ha_rows found_records=join->found_records;
>COND *select_cond= join_tab->select_cond;
> - bool select_cond_result= TRUE;
> + bool select_cond_result= TRUE, unlock_row= TRUE;
>
>DBUG_ENTER("evaluate_join_record");
>DBUG_PRINT("enter",
> @@ -19124,6 +19124,7 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
>
> if (found)
> {
> + unlock_row= false;
>enum enum_nested_loop_state rc;
>/* A match from join_tab is found for the current partial join. */
>rc= (*join_tab->next_select)(join, join_tab+1, 0);
> @@ -19147,11 +19148,6 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
>if (shortcut_for_distinct && found_records != join->found_records)
> DBUG_RETURN(NESTED_LOOP_NO_MORE_ROWS);
instead of unlock_row() you can put
DBUG_RETURN(NESTED_LOOP_OK);
here. Might be a bit simpler to read.
> }
> -else
> -{
> - join->thd->get_stmt_da()->inc_current_row_for_warning();
> - join_tab->read_record.unlock_row(join_tab);
> -}
>}
>else
>{
> @@ -19160,6 +19156,9 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
>with the beginning coinciding with the current partial join.
> */
> join->join_examined_rows++;
> + }
> + if (unlock_row)
> + {
> join->thd->get_stmt_da()->inc_current_row_for_warning();
> join_tab->read_record.unlock_row(join_tab);
>}
> @@ -26945,6 +26944,12 @@ AGGR_OP::end_send()
>table->reginfo.lock_type= TL_UNLOCK;
>
>bool in_first_read= true;
> +
> + /*
> + Reset the counter before copying rows from internal temporary table to
> + INSERT table.
> + */
> + join_tab->join->thd->get_stmt_da()->reset_current_row_for_warning();
>while (rc == NESTED_LOOP_OK)
>{
> int error;
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4774601a3cd: MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same table: rows are counted twice
Hi, Rucha! On Dec 29, Rucha Deodhar wrote: > On Wed, Dec 29, 2021 at 12:50 AM Sergei Golubchik wrote: > > > Why did put your fix here in a common code path in select, and not > > in, say, select_insert::send_data() ? > > Yes, the counter can be incremented in select_insert::send_data() too. > > But after writing record to INSERT table, we go back to > evaluate_join_records() where the counter is incremented > unconditionally. And CREATE TABLE...SELECT was failing with wrong > output because it also uses this code. > > I could have incremented counter conditionally so that it doesn't > increment twice like so: > > increment counter in select_insert::send_data() after record is written > if (thd->lex->sql_command == SQLCOM_INSERT_SELECT || > thd->lex->sql_command == SQLCOM_CREATE_TABLE) I'd think you wouldn't need this if(), as select_insert::send_data() is not used for other sql commands. > And increment counter in evalaute_join_records() after one nested loop > iteration ends (end_send()) > if (join->thd->lex->sql_command != SQLCOM_INSERT_SELECT && > join->thd->lex->sql_command != SQLCOM_CREATE_TABLE) I see. That's not good. > I had initially implemented it this way but it kinda looked hacky to me. > So I did a more general solution. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4774601a3cd: MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same table: rows are counted twice
Hi, Rucha!
Note test failures in buildbot, don't forget to update all result files
before pushing.
On Dec 28, Rucha Deodhar wrote:
> revision-id: 4774601a3cd (mariadb-10.2.40-192-g4774601a3cd)
> parent(s): 42fea34d4a9
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-12-27 16:48:04 +0530
> message:
>
> MDEV-26698: Incorrect row number upon INSERT .. SELECT from the same
> table: rows are counted twice
>
> Analysis: When the table we are trying to insert into and the SELECT table
> are same for INSERT ... SELECT, rows from the SELECT table are copied into
> internal temporary table and then to the INSERT table. We only want to
> count the rows when we start inserting into the table.
> Fix: Reset the counter to 1 before starting to copy from internal temporary
> table to select table and then increment the counter.
>
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index a331f4f3dbc..7a50f3d74ac 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -3832,6 +3832,8 @@ mysql_select(THD *thd,
> }
>}
>
> + thd->get_stmt_da()->reset_current_row_for_warning();
What is this for? It wasn't needed before.
>if ((err= join->optimize()))
>{
> goto err;// 1
> @@ -19124,10 +19126,10 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
>
> if (found)
> {
> + unlock_row= false;
>enum enum_nested_loop_state rc;
>/* A match from join_tab is found for the current partial join. */
>rc= (*join_tab->next_select)(join, join_tab+1, 0);
> - join->thd->get_stmt_da()->inc_current_row_for_warning();
By moving this down you skip few returns, incl NESTED_LOOP_OK
and NESTED_LOOP_NO_MORE_ROWS. Are you sure it's fine?
Won't the counter be off, if the query execution continues after, say,
NESTED_LOOP_OK?
>if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS)
> DBUG_RETURN(rc);
>if (return_tab < join->return_tab)
> @@ -26945,6 +26943,12 @@ AGGR_OP::end_send()
>table->reginfo.lock_type= TL_UNLOCK;
>
>bool in_first_read= true;
> +
> + /*
> + Reset the counter before copying rows from internal temporary table to
> + INSERT table.
this code is also used for group by. Please, add a test for
insert ... select ... group by
Why did put your fix here in a common code path in select, and not in,
say, select_insert::send_data() ?
> + */
> + join_tab->join->thd->get_stmt_da()->reset_current_row_for_warning();
>while (rc == NESTED_LOOP_OK)
>{
> int error;
> @@ -26968,6 +26972,8 @@ AGGR_OP::end_send()
> else
> {
>rc= evaluate_join_record(join, join_tab, 0);
> + /* Increment the counter after copying rows. */
> + join_tab->join->thd->get_stmt_da()->inc_current_row_for_warning();
> }
>}
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 734aee3cd3c: MDEV-27018 IF and COALESCE lose "json" property
gt; + return false;
> +
> +case 0:
> +default:
> + /*
> +Non of the two handlers are derived from other handlers.
> +Nothing left to try.
> + */
> + return true;
> +}
> + }
> +};
> diff --git a/sql/sql_type_json.h b/sql/sql_type_json.h
> index 6c4ee8cb2eb..4a394809a06 100644
> --- a/sql/sql_type_json.h
> +++ b/sql/sql_type_json.h
> @@ -21,18 +21,145 @@
...
> + bool Item_append_extended_type_info(Send_field_extended_metadata *to,
> + const Item *item) const
> + {
> +return set_format_name(to); // Send "format=json" in the protocol
> + }
> +
> + bool Item_hybrid_func_fix_attributes(THD *thd,
> + const char *name,
> + Type_handler_hybrid_field_type
> *hybrid,
> + Type_all_attributes *attr,
> + Item **items, uint nitems)
> + const override
> + {
> +if (BASE::Item_hybrid_func_fix_attributes(thd, name, hybrid, attr,
> + items, nitems))
> + return true;
> +/*
> + The above call can change the type handler on "hybrid", e.g.
> + choose a proper BLOB type handler according to the calculated
> max_length.
> + Convert general purpose string type handler to its JSON counterpart.
> + This makes hybrid functions preserve JSON data types, e.g.:
> +COALESCE(json_expr1, json_expr2) -> JSON
> +*/
> +
> hybrid->set_handler(json_type_handler_from_generic(hybrid->type_handler()));
I don't get it. It's supposed to "fix attributes", like signed/unsigned,
max length, etc. The correct type handler should've been set already,
otherwise this type handler method wouldn't even be called.
> +return false;
> + }
> };
>
> +
> +class Type_handler_string_json:
> + public Type_handler_general_purpose_string_to_json + type_handler_string>
> +{ };
> diff --git a/sql/sql_type_json.cc b/sql/sql_type_json.cc
> index a804366ec03..a84f720bcc9 100644
> --- a/sql/sql_type_json.cc
> +++ b/sql/sql_type_json.cc
> @@ -20,17 +20,111 @@
> #include "sql_class.h"
>
>
> -Type_handler_json_longtext type_handler_json_longtext;
> +Named_type_handler
> + type_handler_string_json("char/json");
do you mean these names are not shown anywhere?
> +Named_type_handler
> + type_handler_varchar_json("varchar/json");
> +
> +Named_type_handler
> + type_handler_tiny_blob_json("tinyblob/json");
> +
> +Named_type_handler
> + type_handler_blob_json("blob/json");
> +
> +Named_type_handler
> + type_handler_medium_blob_json("mediumblob/json");
> +
> +Named_type_handler
> + type_handler_long_blob_json("longblob/json");
...
> +/*
> + This method ressembles what Field_blob::type_handler()
resembles
(also below)
> + does for general purpose BLOB type handlers.
> +*/
> +const Type_handler *
> +Type_handler_json_common::json_blob_type_handler_by_length_bytes(uint len)
> +{
> + switch (len) {
> + case 1: return &type_handler_tiny_blob_json;
> + case 2: return &type_handler_blob_json;
> + case 3: return &type_handler_medium_blob_json;
> + }
> + return &type_handler_long_blob_json;
> +}
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 8a84f5a40c1: MDEV-24176 Preparations
Hi, Aleksey! On Oct 18, Aleksey Midenkov wrote: > On Sun, Oct 17, 2021 at 4:55 PM Sergei Golubchik wrote: > > On Oct 17, Aleksey Midenkov wrote: > > > commit 8a84f5a40c1 > > > Author: Aleksey Midenkov > > > Date: Thu May 27 17:00:14 2021 +0300 > > > > > > MDEV-24176 Preparations > > > > > > 1. moved fix_vcol_exprs() call to open_table() > > > > > > mysql_alter_table() doesn't do lock_tables() so it cannot win from > > > fix_vcol_exprs() from there. Tests affected: main.default_session > > > > This is likely wrong, but the old code was wrong too. Neither open_table > > nor lock_tables is called under LOCK TABLES, but the session > > environment can change, I suspect. > > Why, open_table() is called under LOCK TABLES. Please see there `if > (best_table)` and a jump to reset where vcol_fix_exprs() is called. > Added test case for LOCK TABLES. Yes, you're right > > > 3. Vanilla cleanups and comments. > > > > > > diff --git a/sql/item.h b/sql/item.h > > > index cc1914a7ad4..7b7fe04f0b2 100644 > > > --- a/sql/item.h > > > +++ b/sql/item.h > > > @@ -2586,6 +2585,12 @@ class Item_ident :public Item_result_field > > >const char *db_name; > > >const char *table_name; > > >const char *field_name; > > > + /* > > > + NOTE: came from TABLE::alias_name_used and this is only a hint! It > > > works > > > + only in need_correct_ident() condition. On other cases it is FALSE > > > even if > > > + table_name is alias! It cannot be TRUE in these cases, lots of > > > spaghetti > > > + logic depends on that. > > > > could you elaborate on that? > > See in next commit check_vcol_func_processor(). I could use `if > (alias_name_used)` instead of `if (table_name)` if alias_name_used > were updated correctly. I see that. What I mean was, can you show an example? Say, a query that sets TABLE::alias_name_used incorrectly? I wasn't able to find a test case for that. Also, why do you even want to re-fix items when a table alias is used? Is it for MDEV-25672? But that's already fixed for half a year. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Review for: MDEV-26938 Support descending indexes internally in InnoDB (server part)
Hi, Sergey! Yes, that's basically how I fixed it. But only in key_rec_cmp(), key_cmp() changes somehow weren't needed. Perhaps they'll be needed for range optimizer though. Please, use preview-10.8-MDEV-26938-desc-indexes for your range optimizer work. On Dec 11, Sergey Petrunia wrote: > On Tue, Dec 07, 2021 at 07:01:45PM +0300, Sergey Petrunia wrote: > > == Issue #1: ordered index scan is not handled in ha_partition == > > I've hit this again when trying to get range optmizer to work. Please find the > patch below. It follows MySQL-8's approach. > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Review for: MDEV-26938 Support descending indexes internally in InnoDB (server part)
Hi, Sergey!
All fixed, thanks!
On Dec 07, Sergey Petrunia wrote:
> Hi Serg,
>
> There is some non-trivial input too, after all. Please find below.
>
> > commit da2903f03de039693354176fda836e6642d6d0f0
> > Author: Sergei Golubchik
> > Date: Wed Nov 24 16:50:21 2021 +0100
> >
> > MDEV-26938 Support descending indexes internally in InnoDB (server part)
> >
> > * preserve DESC index property in the parser
> > * store it in the frm (only for HA_KEY_ALG_BTREE)
> > * read it from the frm
> > * show it in SHOW CREATE
> > * skip DESC indexes in opt_range.cc and opt_sum.cc
> > * ORDER BY test
> >
>
> == Trivial input ==
>
> > diff --git a/sql/unireg.cc b/sql/unireg.cc
> > index 2726b9a68c2..d6449eeca4c 100644
> > --- a/sql/unireg.cc
> > +++ b/sql/unireg.cc
> > @@ -685,6 +685,14 @@ static uint pack_keys(uchar *keybuff, uint key_count,
> > KEY *keyinfo,
> > DBUG_PRINT("loop", ("flags: %lu key_parts: %d key_part: %p",
> > key->flags, key->user_defined_key_parts,
> > key->key_part));
> > +/* For SPATIAL, FULLTEXT and HASH indexes (anything other than B-tree),
> > + ignore the ASC/DESC attribute of columns. */
> > +const uchar ha_reverse_sort= key->algorithm == HA_KEY_ALG_BTREE
> > + ? HA_REVERSE_SORT :
> > + (key->algorithm == HA_KEY_ALG_UNDEF &&
> > + !(key->flags & (HA_FULLTEXT | HA_SPATIAL)))
> > + ? HA_REVERSE_SORT : 0;
>
> The above is hard to read for me. I think if-statement form would be more
> readable:
>
> const uchar ha_reverse_sort= 0;
> if (key->algorithm == HA_KEY_ALG_BTREE ||
> (key->algorithm == HA_KEY_ALG_UNDEF &&
> !(key->flags & (HA_FULLTEXT | HA_SPATIAL))
>ha_reverse_sort= HA_REVERSE_SORT;
>
>
> == Issue #1: ordered index scan is not handled in ha_partition ==
>
> Testcase:
>
> create table t10 (a int, b int, key(a desc)) partition by hash(a) partitions
> 4;
> insert into t10 select seq, seq from seq_0_to_999;
>
> MariaDB [test]> select * from t10 order by a limit 3;
> +--+--+
> | a| b|
> +--+--+
> |3 |3 |
> |7 |7 |
> | 11 | 11 |
> +--+--+
> 3 rows in set (0.002 sec)
>
> Correct output:
>
> MariaDB [test]> select * from t10 use index() order by a limit 3;
> +--+--+
> | a| b|
> +--+--+
> |0 |0 |
> |1 |1 |
> |2 |2 |
> +--+--+
> 3 rows in set (0.011 sec)
>
> == Issue #2: extra warning ==
>
> MariaDB [test]> create table t1 (a int, b int, key(a), key(a));
> Query OK, 0 rows affected, 1 warning (0.016 sec)
>
> MariaDB [test]> show warnings;
> +---+--+--+
> | Level | Code | Message
> |
> +---+--+--+
> | Note | 1831 | Duplicate index `a_2`. This is deprecated and will be
> disallowed in a future release |
> +---+--+--+
>
> 1 row in set (0.000 sec)
>
>
> This is ok.
>
>
> MariaDB [test]> create table t2 (a int, b int, key(a), key(a desc));
>
> Query OK, 0 rows affected, 1 warning (0.004 sec)
>
>
>
> MariaDB [test]> show warnings;
> +---+--+--+
>
> | Level | Code | Message
> |
> +---+--+--+
>
> | Note | 1831 | Duplicate index `a_2`. This is deprecated and will be
> disallowed in a future release |
> +---+--+--
Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Dec 05, Aleksey Midenkov wrote:
> > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > > index ac22a63bdba..459edb614f7 100644
> > > --- a/sql/sql_base.cc
> > > +++ b/sql/sql_base.cc
> > > @@ -4203,6 +,15 @@ bool open_tables(THD *thd, const DDL_options_st
> > > &options,
> > > }
> > >
> > >thd->current_tablenr= 0;
> > > +
> > > +#ifdef WITH_PARTITION_STORAGE_ENGINE
> > > + if (!thd->stmt_arena->is_conventional())
> > > + {
> > > +for (tables= *start; tables; tables= tables->next_global)
> > > + tables->vers_skip_create= false;
> > > + }
> >
> > This looks a bit like an overkill, you do it for every prepare,
> > while auto-adding a partition is a rare event, as you said.
> >
> > May be you can do instead in TABLE::vers_switch_partition
> >
> > uint *create_count= !thd->stmt_arena->is_conventional() &&
> > table_list->vers_skip_create ?
>
> No, this is inside backoff action loop. That will interfere with the
> normal vers_skip_create algorithm (which also applies to PS, SP). The
> goal of the above code is to reset vers_skip_create from the previous
> statement execution.
In that case you can use an auto-resetting value, like,
if tables->vers_skip_create == thd->query_id it means, "skip".
That is vers_skip_create must be of query_id_t. You set it with
tables->vers_skip_create= thd->query_id;
And on the next statement it automatically expires.
This semantics is a bit more complex than boolean, so it'd need a
comment, like
/*
Protect single thread from repeating partition auto-create over
multiple share instances (as the share is closed on backoff action).
Skips auto-create only for one given query id.
*/
query_id_tvers_skip_create;
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] a5fca9a6e30: MENT-651 [6/8] store cache managers in a list
Hi, Nikita! On Nov 24, Nikita Malyavin wrote: > On Mon, 15 Nov 2021 at 13:52, Sergei Golubchik wrote: > > > Hi, Nikita! > > > > On Nov 14, Nikita Malyavin wrote: > > > revision-id: a5fca9a6e30 (mariadb-10.5.2-478-ga5fca9a6e30) > > > parent(s): ad6e7b87107 > > > author: Nikita Malyavin > > > committer: Nikita Malyavin > > > timestamp: 2021-01-27 17:28:05 +1000 > > > message: > > > > > > MENT-651 [6/8] store cache managers in a list > > > > again, a couple of lines with a description would be good here > > I think it should be just a fixup for [4/8] ALTER ONLINE TABLE. > > > > diff --git a/mysql-test/suite/binlog/t/online_alter.test > > > b/mysql-test/suite/binlog/t/online_alter.test > > > index 804e847d008..4ed2db74bd6 100644 > > > --- a/mysql-test/suite/binlog/t/online_alter.test > > > +++ b/mysql-test/suite/binlog/t/online_alter.test > > > @@ -218,7 +219,7 @@ set autocommit = 0; > > > start transaction; > > > update t1 set b= 7007 where a = 7; > > > --error ER_DUP_ENTRY > > > -update t1 set a= 8 where a = 8 or a = 9; > > > +update t1 set a= 8, b= 8008 where a = 8 or a = 9 order by a; > > > > why did you change tests for what is, as far as I understand, just > > an optimization? > > That's the point, that it's not! > > The thing is, in case of an error, the table is invalidated. As far as > I remember, it could happen even if the table could've just evicted > from the cache (in case of a long transaction involving many tables). > > So when committing, corresponding TABLE_SHARE may be inaccessible, by > different reasons. > > So it turns to practically correct that handlerton can't depend from > TABLE or TABLE_SHARE. Okay. Why did you have to change tests though? > > > commit; > > > set autocommit = 1; The rest of my questions... I now understand better what you're doing and why. There are some questions still, and I wanted to look in a debugger, but this branch doesn't compile with gcc 11.2, just too many errors. Could you please move your work on top of 10.8 please? Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Squash aaed3436 MENT-651 [4/8] ALTER ONLINE TABLE
Hi, Nikita! I don't think it's a good idea. Old branches get deleted and garbage-collected eventually. If you think the history is important for understanding the changes - don't squash it. If you think it's not important - you don't need a reference to the unsquashed branch :) On Nov 25, Nikita Malyavin wrote: > I mean, I'd just put the reference to the head in the commit message, i. e. > now the last commit is > >- test progress reporting f1af51e4 > > I'd add a line: > See unsquashed history at f1af51e4 > > In the final commit message > > On Wed, 24 Nov 2021 at 23:50, Sergei Golubchik wrote: > > > Hi, Nikita! > > > > Sorry, I meant what did you mean by > > > > > I would also embed a reference to the original unsquashed branch. > > > It seems to me as a good idea in sense of helping the reader > > > investigate the code > > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Squash aaed3436 MENT-651 [4/8] ALTER ONLINE TABLE
Hi, Nikita! Sorry, I meant what did you mean by > I would also embed a reference to the original unsquashed branch. It > seems to me as a good idea in sense of helping the reader investigate > the code Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Squash aaed3436 MENT-651 [4/8] ALTER ONLINE TABLE
Hi, Nikita! On Nov 24, Nikita Malyavin wrote: > Sergei, I think all the commits starting from [4/8] ALTER ONLINE TABLE > should be squashed. > > I will also add a better description in the commit message covering our > latest discussions. > > I would also embed a reference to the original unsquashed branch. It seems > to me as a good idea in sense of helping the reader investigate the code What do you mean by that? Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4af7a583802: Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Hi, Nikita!
On Nov 23, Nikita Malyavin wrote:
>
> Right. Adding as follows:
>
> Event_log is supposed to be a basic logging class that can write events in
> a single file.
>
> MYSQL_BIN_LOG in comparison will have:
> * rotation support
> * index files
> * purging
> * gtid, xid and other transactional information handling.
> * is dedicated for a general-purpose binlog
Great!
> > > diff --git a/sql/log.h b/sql/log.h
> > > index 73cd66d5a4a..74c409e1ac7 100644
> > > --- a/sql/log.h
> > > +++ b/sql/log.h
> > > @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
> > >uint next_file_id();
> > >inline char* get_index_fname() { return index_file_name;}
> > >inline char* get_log_fname() { return log_file_name; }
> > > - inline char* get_name() { return name; }
> > >inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
> >
> > should remove that too, I reckon
> >
> Well, these are so tiny getters so I'm not sure, whether is it more
> comfortable to jump back and forth to check what's there in Event_log,
> or just admit the cost of duplication for a better read comfort. I'd
> leave it as it is now.
What I mean is, you moved get_log_lock() to MYSQL_LOG class.
You don't need a second copy here, it's redundant.
Except that MYSQL_LOG is private here and you want get_log_lock()
public, apparently? Okay then it's needed here too.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f3603cbfaeb: report progress
Hi, Nikita! On Nov 24, Nikita Malyavin wrote: > >> > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > >> > index 29d0f487303..87dbe226c2e 100644 > >> > --- a/sql/sql_table.cc > >> > +++ b/sql/sql_table.cc > >> > @@ -11300,6 +11300,9 @@ static int online_alter_read_from_binlog(THD > >> > *thd, rpl_group_info *rgi, > >> > > >> >IO_CACHE *log_file= log->flip(); > >> > > >> > + thd_progress_report(thd, 0, my_b_write_tell(log_file) > >> > + / > >> > rgi->tables_to_lock->m_conv_table->s->reclength); > >> > >> 1. why do you do this "/ rgi->...->s->reclength" ? > > > > The idea was to write an approximate number of rows. I think it can > > come in handy. > > > Well, I notice now that progress is in percents. I wonder if there's a > way to get a raw value for a user. I thought it's always in percents, that's why I wondered why you divided the value by a constant :) > > Should start from zero again.. I dont't see the stage change there, maybe > > I wasn't sure should it be another stage or the same one. > > Added in the new commit Okay, good. You cannot have a progress going from 0% to 100% and then again fron 0% to 100% all in the same stage, that's not what users expect from a progress meter. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9277b838aad: Forbid savepoints setup while ONLINE ALTER goes on
Hi, Nikita! May be you don't need to truncate the transaction cache, but can log SAVEPOINT and ROLLBACK TO SAVEPOINT as Query_log_event? That's easier. Normal binlogging does it if the "transaction" modifies some non-transactional tables. But you can just do it unconditionally. Let's keep it as a lower-priority task, though, for after the rest is done. On Nov 24, Nikita Malyavin wrote: > On Mon, 15 Nov 2021 at 16:07, Sergei Golubchik wrote: > > > Hi, Nikita! > > > > On Nov 15, Nikita Malyavin wrote: > > > revision-id: 9277b838aad (mariadb-10.5.2-485-g9277b838aad) > > > parent(s): 37b6f5cb8ef > > > author: Nikita Malyavin > > > committer: Nikita Malyavin > > > timestamp: 2021-01-29 03:47:52 +1000 > > > message: > > > > > > Forbid savepoints setup while ONLINE ALTER goes on > > > > Why is that? > > Well, in short, it's not implemented. If rollback to savepoint is > happened, we should rollback in our transaction cache as well. > > Then we need to remember where the savepoint is, for every online > altering table. It likely resolves with maintaining a hash Savepoint > -> List [ ] > > But now rollback would sole an effect after ALTER TABLE is finished > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 903ae1c03ae: Fix running without binlog
Hi, Nikita! On Nov 24, Nikita Malyavin wrote: > > The way to test with and without binlog is to rename .test in .inc and make > a new test: one that sources have_binlog.inc, and one that doesn't. > > I don't like putting tests in .inc, so decided to leave only one version of > it. Maybe that's wrong > > But what about sourcing a .test file? That's okay. There're tests in mysql-test that use inc, there're tests that include other tests. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] b95c5105e28: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
is assert fails for me in the update-big test
> +my_error(WARN_VERS_PART_FULL, MYF(ME_WARNING|ME_ERROR_LOG),
> +table->s->db.str, table->s->table_name.str,
> +vers_info->hist_part->partition_name, "INTERVAL");
> + }
> +}
> + }
> +
> + return false;
> +}
> +
> +
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index ac22a63bdba..459edb614f7 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -4203,6 +,15 @@ bool open_tables(THD *thd, const DDL_options_st
> &options,
> }
>
>thd->current_tablenr= 0;
> +
> +#ifdef WITH_PARTITION_STORAGE_ENGINE
> + if (!thd->stmt_arena->is_conventional())
> + {
> +for (tables= *start; tables; tables= tables->next_global)
> + tables->vers_skip_create= false;
> + }
This looks a bit like an overkill, you do it for every prepare,
while auto-adding a partition is a rare event, as you said.
May be you can do instead in TABLE::vers_switch_partition
uint *create_count= !thd->stmt_arena->is_conventional() &&
table_list->vers_skip_create ?
> +#endif
> +
> restart:
>/*
> Close HANDLER tables which are marked for flush or against which there
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for OpenSSL 3.0
Hi, Vladislav! On Nov 21, Vladislav Vaintroub wrote: >> H Serg, >> >> I read in openssl/NEWS.md at master · openssl/openssl >> >> (github.com) that “SSL 3, TLS 1.0, TLS 1.1, and DTLS 1.0 only >> >> work at security level 0” >> >this means the documentation is wrong? > >> This means exactly that. I changed th comment like you suggested. > >> >I think the "former" (or, rather existing, conventional) approach is >> >easier to use - assume all CMAKE_REQUIRED_* variables are empty, set > >> I’m not doing to dive into discussion here, so I changed it the way >> you like more, especially since this makes the patch even smaller > >> >It's not that exciting a task to do it many times, I think. Might be >> >better just to do it once, and be done with it. A good argument not >> >to do it (that is, to do it in two steps) could be, that new API >> >that one has to use in 3.0 is incompatible with 1.1, so we'd need to >> >double the number of #ifdef's If this is the case then, indeed, >> >better to wait, say, 5 years, as you suggest, for OpenSSL 1.x to >> >reach EOL > >> A good argument is not to change something if it is not broken, and >> also keep the patch sizes to minimum, which I think happened here. > >> This patch is hopefully applicable back to 10.2 . I do not think we’d >> have a zero porting work for any new version of OpenSSL, since they >> like to change API to the new API-du-jour. That's true. 1.0->1.1 wasn't exactly trivial. >> But, a good argument to change anything is to remove the ifdefs. I >> think it would be possible for 10.4+, in case WolfSSL’s OpenSSL >> compatibility is good enough. I think ideally, it would be done when >> 10.3 is no more supported, so that YASSL , at least is no more >> concern. By that time, OpenSSL 1.0 might no be supported either. Okay. I don't have a strong opinion about it. Let's use your minimal approach, as long as it works. Which, I guess, means practically, as long as no major distro ships openssl 3.0 with compatibility API disabled. >> I also think too much time is spend on discussing SHAx hashing here, >> it is a relatively small thing, after all. I don't understand what you mean, I didn't touch SHAx topic at all. >> >I think we should rather deprecate DES_ENCRYPT and DES_DECRYPT >> >functions. It should've been done long time ago. > >> Ok, but there is also a deprecation procedure for us. We can’t remove >> those functions right now, and DES_XXX would be in deprecated mode >> for a while until they would be removed, in a couple of years. MDEV-27104 Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for OpenSSL 3.0
Hi, Vladislav!
On Nov 19, Vladislav Vaintroub wrote:
>
> Hi Serg,
>
> Re. The SECLEVEL 0 is , as I wrote necessary to get tests TLSv1.0 and
> TLSv1.1 running
>
> I read in openssl/NEWS.md at master · openssl/openssl (github.com)
> that “SSL 3, TLS 1.0, TLS 1.1, and DTLS 1.0 only work at security level 0”
this means the documentation is wrong?
https://www.openssl.org/docs/man3.0/man3/SSL_CTX_get_security_level.html
says what I quoted, TLS versions < 1.1 are not permitted only at level 3.
Do you mean if you change from 0 to 1, tests start to fail?
If the answer is "yes", please mention this in the commit comment, like
- SECLEVEL in CipherString in openssl.cnf
had been downgraded to 0, from 1, to make TLSv1.0 and TLSv1.1 possible
(according to https://github.com/openssl/openssl/blob/openssl-3.0.0/NEWS.md
even though the manual for SSL_CTX_get_security_level claims that it
should not be necessary)
> > > + FOREACH(x INCLUDES LIBRARIES DEFINITIONS)
> > > + SET(SAVE_CMAKE_REQUIRED_${x} ${CMAKE_REQUIRED_${x}})
> > > + ENDFOREACH()
> >
> > why do you set/restore them?
>
> Why do I change CMAKE_REQUIRED_{INCLUDES,LIBRARIES,DEFINITIONS}, and
> restore them is because of how CHECK_SYMBOL_EXISTS work. It is not like I
> added it just for OpenSSL 3.0 . I added CMAKE_REQUIRED_DEFINITIONS,
> because I use that -D at runtime, and it might, at least theoretically
> affect the behaviour . Whether the outcome of the test is really the same,
> I did not even check, and I did not think much about it. It is just the
> safest thing is to use the same flag for the system checks, as it would be
> used later during compilation. I save and restore the CMake variables,
> because it is again, safer than former “overwrite variables, later make
> them empty”, which could lead to unwanted side-effects during cmake run,
I think the "former" (or, rather existing, conventional) approach is
easier to use - assume all CMAKE_REQUIRED_* variables are empty, set
what you must, and clear them afterwards. We do it everywhere.
Why would one ever want for them to be set to some specific non-empty
values between CHECK_SYMBOL_EXISTS invocations?
> >This is just postponing the inevitable.
> >They'll drop the old API eventually. As far as I understand the
> >internals were changed in a way that doesn't fit the old API.
>
> Somehow it did fit, as it works, and the build passes, and the tests pass.
>
> The unique warnings are about SHA{1|224|256|384|512}_Init/Final/Update,
> DES_{ede3_cbc_encrypt|set_key_unchecked}, and DH_new|DH_free.
>
> Once they remove it, we can remove it, too, not a huge deal. I
> propose to keep this patch at minimum, delaying the beautifications
> and clean-ups are until next time, when the inevitable problem
> arises, say, in 5 years, I do not expect that to be sooner.
It's not that exciting a task to do it many times, I think.
Might be better just to do it once, and be done with it.
A good argument not to do it (that is, to do it in two steps) could be,
that new API that one has to use in 3.0 is incompatible with 1.1,
so we'd need to double the number of #ifdef's
If this is the case then, indeed, better to wait, say, 5 years, as you
suggest, for OpenSSL 1.x to reach EOL
> Note- there does not seem to be an non-deprecated replacement for
> DES_set_key_unchecked, so we have to have this anti-deprecate setting
> this or that way, anyway.
I think we should rather deprecate DES_ENCRYPT and DES_DECRYPT
functions. It should've been done long time ago.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
> From: Sergei Golubchik
> Sent: Thursday, 18 November 2021 20:20
> To: Vladislav Vaintroub
> Cc: maria-developers@lists.launchpad.net
> Subject: Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for
> OpenSSL 3.0
>
> Hi, Vladislav!
>
> few questions below:
>
> On Nov 18, Vladislav Vaintroub wrote:
>
> > revision-id: c80991c79f7 (mariadb-10.6.1-213-gc80991c79f7)
> > parent(s): cee33f1ab7c
> > author: Vladislav Vaintroub
> > committer: Vladislav Vaintroub
> > timestamp: 2021-11-09 02:04:22 +0100
> > message:
> >
> > MDEV-25785 Add support for OpenSSL 3.0
> >
> > Summary of changes
> >
> > - MD_CTX_SIZE is increased
> >
> > - EVP_CIPHER_CTX_buf_noconst(ctx) does not work anymore, points
> > to nobody knows where. The assumption made previously was that
> > (since the function does not seem to be documented)
> > was that it points to the last partial source block.
> > Add own partial block buffer for NOPAD encryption instead
> >
> > - SECLEVEL in CipherString in openssl.cnf
Re: [Maria-developers] c80991c79f7: MDEV-25785 Add support for OpenSSL 3.0
Hi, Vladislav!
few questions below:
On Nov 18, Vladislav Vaintroub wrote:
> revision-id: c80991c79f7 (mariadb-10.6.1-213-gc80991c79f7)
> parent(s): cee33f1ab7c
> author: Vladislav Vaintroub
> committer: Vladislav Vaintroub
> timestamp: 2021-11-09 02:04:22 +0100
> message:
>
> MDEV-25785 Add support for OpenSSL 3.0
>
> Summary of changes
>
> - MD_CTX_SIZE is increased
>
> - EVP_CIPHER_CTX_buf_noconst(ctx) does not work anymore, points
> to nobody knows where. The assumption made previously was that
> (since the function does not seem to be documented)
> was that it points to the last partial source block.
> Add own partial block buffer for NOPAD encryption instead
>
> - SECLEVEL in CipherString in openssl.cnf
> had been downgraded to 0, from 1, to make TLSv1.0 and TLSv1.1 possible
The definition of SECLEVEL is:
Level 0
Everything is permitted. This retains compatibility with previous
versions of OpenSSL.
Level 1
The security level corresponds to a minimum of 80 bits of security.
Any parameters offering below 80 bits of security are excluded. As a
result RSA, DSA and DH keys shorter than 1024 bits and ECC keys
shorter than 160 bits are prohibited. All export cipher suites are
prohibited since they all offer less than 80 bits of security. SSL
version 2 is prohibited. Any cipher suite using MD5 for the MAC is
also prohibited. Note that signatures using SHA1 and MD5 are also
forbidden at this level as they have less than 80 security bits.
Only at level 3 does it say "TLS versions below 1.1 are not permitted."
I don't see why you had to change from 1 to 0. Do we have "signatures
using SHA1 and MD5"?
> - ctx_buf buffer now must be aligned to 16 bytes with openssl(
> previously with WolfSSL only), ot crashes will happen
>
> - updated aes-t , to be better debuggable
> using function, rather than a huge multiline macro
> added test that does "nopad" encryption piece-wise, to test
> replacement of EVP_CIPHER_CTX_buf_noconst
>
> diff --git a/cmake/ssl.cmake b/cmake/ssl.cmake
> index 7c2488be8bd..64c93ff9b4f 100644
> --- a/cmake/ssl.cmake
> +++ b/cmake/ssl.cmake
> @@ -139,9 +139,20 @@ MACRO (MYSQL_CHECK_SSL)
>SET(SSL_INTERNAL_INCLUDE_DIRS "")
>SET(SSL_DEFINES "-DHAVE_OPENSSL")
>
> + FOREACH(x INCLUDES LIBRARIES DEFINITIONS)
> +SET(SAVE_CMAKE_REQUIRED_${x} ${CMAKE_REQUIRED_${x}})
> + ENDFOREACH()
why do you set/restore them?
> + # Silence "deprecated in OpenSSL 3.0"
> + IF((NOT OPENSSL_VERSION) # 3.0 not determined by older cmake
> + OR NOT(OPENSSL_VERSION VERSION_LESS "3.0.0"))
> +SET(SSL_DEFINES "${SSL_DEFINES} -DOPENSSL_API_COMPAT=0x1010L")
> +SET(CMAKE_REQUIRED_DEFINITIONS -DOPENSSL_API_COMPAT=0x1010L)
> + ENDIF()
This is just postponing the inevitable.
They'll drop the old API eventually. As far as I understand the
internals were changed in a way that doesn't fit the old API.
Is there some safe subset of OpenSSL API that works both in 1.0 and in
3.0 ? It might be more future proof to use only that.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4492c869f31: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
Hi, Rucha! Thanks! Looks good now. One comment about tests: On Nov 18, Rucha Deodhar wrote: > revision-id: 4492c869f31 (mariadb-10.6.1-212-g4492c869f31) > parent(s): 3f3ec40c91b > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2021-11-08 16:51:33 +0530 > message: > > MDEV-26238: Remove inconsistent behaviour of --default-* options > in my_print_defaults ... > diff --git a/mysql-test/main/my_print_defaults.test > b/mysql-test/main/my_print_defaults.test > index bfd4e563826..b52deaf09ff 100644 > --- a/mysql-test/main/my_print_defaults.test > +++ b/mysql-test/main/my_print_defaults.test > @@ -87,20 +85,74 @@ read_buffer_size=1M ... > +--echo # checking that --defaults* option only works when mentioned at > beginning > + > +--echo # Testing --defaults-file at beginning only > +--exec $MYSQL_MY_PRINT_DEFAULTS > --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld > +--remove_file $MYSQLTEST_VARDIR/tmp/tmp3.cnf How does it test that "--defaults* option only works at beginning"? You test that --defaults-file works at the beginning all right. This is redundant, as there are lots of tests in this file that have --defaults-file, but it doesn't hurt. But you don't test the "only" part. You don't test anything that you've changed - I suspect your new test would happily pass without any changes in my_print_defaults.c To test that --defaults-file works *only* at the beginning, you need to verify that it fails not at the beginning. Like error 1 exec $MYSQL_MY_PRINT_DEFAULTS --myslqd --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf also with -c error 1 exec $MYSQL_MY_PRINT_DEFAULTS -c $MYSQLTEST_VARDIR/tmp/tmp1.cnf --mysqld This should fail even at the beginning. The point is - you need to test the behavior that you've changed. As your test has introduced failures - you need to test for these failures. But all your tests succeed. > +--echo # Testing --defaults-extra-file works at beginning only > +--exec $MYSQL_MY_PRINT_DEFAULTS > --defaults-file=$MYSQLTEST_VARDIR/tmp/tmp1.cnf > --defaults-extra-file=$MYSQLTEST_VARDIR/tmp/tmp2.cnf --mysqld Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9277b838aad: Forbid savepoints setup while ONLINE ALTER goes on
Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 9277b838aad (mariadb-10.5.2-485-g9277b838aad) > parent(s): 37b6f5cb8ef > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-29 03:47:52 +1000 > message: > > Forbid savepoints setup while ONLINE ALTER goes on Why is that? Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 37b6f5cb8ef: decouple commit/rollback code from binlog hton
Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 37b6f5cb8ef (mariadb-10.5.2-484-g37b6f5cb8ef) > parent(s): f3603cbfaeb > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-29 03:47:52 +1000 > message: > > decouple commit/rollback code from binlog hton let's squash it with "[6/8] store cache managers in a list" Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f3603cbfaeb: report progress
Hi, Nikita!
On Nov 15, Nikita Malyavin wrote:
> revision-id: f3603cbfaeb (mariadb-10.5.2-483-gf3603cbfaeb)
> parent(s): 3b6038bee88
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2021-01-29 03:47:52 +1000
> message:
>
> report progress
Perhaps, squash this with your commit "test progress reporting" ?
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 29d0f487303..87dbe226c2e 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -11300,6 +11300,9 @@ static int online_alter_read_from_binlog(THD *thd,
> rpl_group_info *rgi,
>
>IO_CACHE *log_file= log->flip();
>
> + thd_progress_report(thd, 0, my_b_write_tell(log_file)
> + /
> rgi->tables_to_lock->m_conv_table->s->reclength);
1. why do you do this "/ rgi->...->s->reclength" ?
2. How does it work with your "cache flipping"? While data is copied
between tables, concurrent connections write into an IO_CACHE.
Then you flip, they what, write into the second IO_CACHE?
What file length will you use here in thd_progress_report()?
> +
>do
>{
> const auto *descr_event= rgi->rli->relay_log.description_event_for_exec;
> @@ -11319,6 +11322,9 @@ static int online_alter_read_from_binlog(THD *thd,
> rpl_group_info *rgi,
> free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC));
> if (ev != rgi->rli->relay_log.description_event_for_exec)
>delete ev;
> +thd_progress_report(thd, my_b_tell(log_file)
> + /
> rgi->tables_to_lock->m_conv_table->s->reclength,
> +thd->progress.max_counter);
>}
>while(!error);
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 903ae1c03ae: Fix running without binlog
Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 903ae1c03ae (mariadb-10.5.2-479-g903ae1c03ae) > parent(s): a5fca9a6e30 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2021-01-27 17:28:05 +1000 > message: > > Fix running without binlog Strange. With such a comment I'd expect you add a test for ALTER TABLE with disabled binlog. Instead you change an error to a warning in one of the existing tests. This test change doesn't match the commit comment. > diff --git a/mysql-test/suite/binlog/r/online_alter.result > b/mysql-test/suite/binlog/r/online_alter.result > index 9bca94b8a87..bb3fa85e76c 100644 > --- a/mysql-test/suite/binlog/r/online_alter.result > +++ b/mysql-test/suite/binlog/r/online_alter.result > @@ -34,9 +34,9 @@ insert into t1 values (123), (456), (789); > set debug_sync= 'now SIGNAL end'; > connection default; > Warnings: > -Error1364Field 'b' doesn't have a default value > -Error1364Field 'b' doesn't have a default value > -Error1364Field 'b' doesn't have a default value > +Warning 1364Field 'b' doesn't have a default value > +Warning 1364Field 'b' doesn't have a default value > +Warning 1364Field 'b' doesn't have a default value > select * from t1; > ab > 50 > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index f0d1167ad61..e348056646d 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -11308,9 +11308,12 @@ static int online_alter_read_from_binlog(THD *thd, > rpl_group_info *rgi, >break; > > ev->thd= thd; > +bool abort_on_warning= thd->abort_on_warning; > +thd->abort_on_warning= false; > thd->set_n_backup_active_arena(&event_arena, &backup_arena); > error= ev->apply_event(rgi); > thd->restore_active_arena(&event_arena, &backup_arena); > +thd->abort_on_warning= abort_on_warning; > > event_arena.free_items(); > free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC)); > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] a5fca9a6e30: MENT-651 [6/8] store cache managers in a list
Hi, Nikita!
On Nov 14, Nikita Malyavin wrote:
> revision-id: a5fca9a6e30 (mariadb-10.5.2-478-ga5fca9a6e30)
> parent(s): ad6e7b87107
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2021-01-27 17:28:05 +1000
> message:
>
> MENT-651 [6/8] store cache managers in a list
again, a couple of lines with a description would be good here
> diff --git a/mysql-test/suite/binlog/t/online_alter.test
> b/mysql-test/suite/binlog/t/online_alter.test
> index 804e847d008..4ed2db74bd6 100644
> --- a/mysql-test/suite/binlog/t/online_alter.test
> +++ b/mysql-test/suite/binlog/t/online_alter.test
> @@ -218,7 +219,7 @@ set autocommit = 0;
> start transaction;
> update t1 set b= 7007 where a = 7;
> --error ER_DUP_ENTRY
> -update t1 set a= 8 where a = 8 or a = 9;
> +update t1 set a= 8, b= 8008 where a = 8 or a = 9 order by a;
why did you change tests for what is, as far as I understand, just an
optimization?
> commit;
> set autocommit = 1;
>
> diff --git a/sql/handler.cc b/sql/handler.cc
> index 8b95653e69f..8b22167e729 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -6660,7 +6660,16 @@ static int binlog_log_row_online_alter(TABLE* table,
>THD *thd= table->in_use;
>
>if (!table->online_alter_cache)
> -table->online_alter_cache= thd->binlog_setup_cache_data();
> + {
> +auto *cache_mngr= online_alter_binlog_get_cache_mngr(thd, table);
> +// Use transaction cache directly, if it is not multi-transaction mode
> +table->online_alter_cache= binlog_get_cache_data(cache_mngr,
> +
> !thd->in_multi_stmt_transaction_mode());
> +
> +trans_register_ha(thd, false, binlog_hton, 0);
> +if (thd->in_multi_stmt_transaction_mode())
> + trans_register_ha(thd, true, binlog_hton, 0);
I still don't understand that.
Why do you need a handlerton, why do you fake a transaction?
And why do you need to search a list in THD, instead of storing
cache_mngr in TABLE_SHARE?
> + }
>
>// We need to log all columns for the case if alter table changes primary
> key.
>table->use_all_columns();
> diff --git a/sql/log.cc b/sql/log.cc
> index 6c678624230..c58096e6d9a 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -2258,14 +2298,18 @@ static int binlog_rollback(handlerton *hton, THD
> *thd, bool all)
> {
>DBUG_ENTER("binlog_rollback");
>
> - for (TABLE *table= thd->open_tables; table; table= table->next)
> - {
> -if (!table->online_alter_cache)
> - continue;
> -table->online_alter_cache->reset();
> -delete table->online_alter_cache;
> -table->online_alter_cache= NULL;
> - }
> + bool is_ending_trans= ending_trans(thd, all);
> +
> + /*
> +This is a crucial moment that we are running through
> +thd->online_alter_cache_list, and not through thd->open_tables to cleanup
> +stmt cache, though both have it. The reason is that the tables can be
> closed
> +to that moment in case of an error.
> +The same reason applies to the fact we don't store cache_mngr in the
> table
> +itself -- because it can happen to be not existing.
> +Still in case if tables are left opened
> + */
> + binlog_online_alter_cleanup(thd->online_alter_cache_list, is_ending_trans);
still don't understand that comment either :(
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] ad6e7b87107: introduce cache flipping
Hi, Nikita!
Interesting trick.
On Nov 13, Nikita Malyavin wrote:
> revision-id: ad6e7b87107 (mariadb-10.5.2-477-gad6e7b87107)
> parent(s): 85fbd867b4c
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2021-01-27 14:17:39 +1000
> message:
>
> introduce cache flipping
Could you add few lines from the Cache_flip_event_log comment here too
please?
> diff --git a/sql/log.h b/sql/log.h
> index 74c409e1ac7..1e422ae4524 100644
> --- a/sql/log.h
> +++ b/sql/log.h
> @@ -424,10 +424,9 @@ class Event_log: public MYSQL_LOG
>
>bool open(
>const char *log_name,
> - enum_log_type log_type,
>const char *new_name, ulong next_file_number,
>enum cache_type io_cache_type_arg);
> - IO_CACHE *get_log_file() { return &log_file; }
> + virtual IO_CACHE *get_log_file() { return &log_file; }
>
>int write_description_event(enum_binlog_checksum_alg checksum_alg,
>bool encrypt, bool dont_set_created);
> @@ -435,6 +434,73 @@ class Event_log: public MYSQL_LOG
>bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file);
> };
>
> +/**
> + A single-reader, multiple-writer non-blocking layer for Event_log.
> + Provides IO_CACHE for writing and IO_CACHE for reading.\
> +
> + Writers use an overrided get_log_file version for their writes, while
> readers
> + should use flip() to initiate reading.
> + flip() swaps pointers to allow non-blocking reads.
> +
> + Writers can block other writers and a reader with a mutex, but a reader
> only
> + swaps two pointers under a lock, so it won't block writers.
> +
> + TODO should be unnecessary after MDEV-24676 is done
> + */
> +class Cache_flip_event_log: public Event_log {
> + IO_CACHE alt_buf;
> + IO_CACHE *current, *alt;
> +public:
> +
> + Cache_flip_event_log() : Event_log(), alt_buf{},
> + current(&log_file), alt(&alt_buf) {}
> + bool open(enum cache_type io_cache_type_arg)
> + {
> +log_file.dir= mysql_tmpdir;
> +alt_buf.dir= log_file.dir;
> +bool res= Event_log::open(NULL, NULL, 0, io_cache_type_arg);
> +if (res)
> + return res;
> +
> +name= my_strdup(key_memory_MYSQL_LOG_name, "online-alter-binlog",
> +MYF(MY_WME));
> +if (!name)
> + return false;
> +
> +res= init_io_cache(&alt_buf, -1, LOG_BIN_IO_SIZE, io_cache_type_arg, 0,
> 0,
> + MYF(MY_WME | MY_NABP | MY_WAIT_IF_FULL)) != 0;
> +return res;
> + }
> +
> + /**
> +Swaps current and alt_log. Can be called only from the reader thread.
> +@return a new IO_CACHE pointer to read from.
> + */
> + IO_CACHE *flip()
> + {
> +IO_CACHE *tmp= current;
> +reinit_io_cache(alt, WRITE_CACHE, 0, 0, 0);
> +mysql_mutex_lock(get_log_lock());
> +reinit_io_cache(current, READ_CACHE, 0, 0, 0);
> +current= alt;
> +mysql_mutex_unlock(get_log_lock());
> +alt= tmp;
> +
> +return alt;
> + }
> +
> + IO_CACHE *get_log_file() override
> + {
> +return current;
may be mysql_mutex_assert_owner(get_log_lock()) ?
> + }
> +
> + void cleanup()
> + {
> +end_io_cache(&alt_buf);
> +Event_log::cleanup();
> + }
> +};
> +
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4af7a583802: Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Hi, Nikita!
On Nov 12, Nikita Malyavin wrote:
> revision-id: 4af7a583802 (mariadb-10.5.2-474-g4af7a583802)
> parent(s): 2da3dc5d422
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2021-01-08 20:59:09 +1000
> message:
>
> Refactor MYSQL_BIN_LOG: extract Event_log ancestor
Would be good to have some explanation here. I understand that you'll
use this refactoring in following commits, but conceptually, what Event_log is?
I mean, like
Event_log is a log where Log_event_xxx events are written, but not
_something else that MYSQL_BIN_LOG is and Event_log isn't_
or
Event_log is a log where Log_event_xxx events are written. While
MYSQL_BIN_LOG is Event_log that also does
> diff --git a/sql/log.cc b/sql/log.cc
> index 827a34c2883..6f4e3191588 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -3701,64 +3765,25 @@ bool MYSQL_BIN_LOG::open(const char *log_name,
>write_file_name_to_index_file= 1;
> }
a bit strange that MYSQL_BIN_LOG::open doesn't use Event_log::open
> diff --git a/sql/log.h b/sql/log.h
> index 73cd66d5a4a..74c409e1ac7 100644
> --- a/sql/log.h
> +++ b/sql/log.h
> @@ -327,6 +327,7 @@ class MYSQL_LOG
> bool strip_ext, char *buff);
>virtual int generate_new_name(char *new_name, const char *log_name,
> ulong next_log_number);
> + inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
> protected:
>/* LOCK_log is inited by init_pthread_objects() */
>mysql_mutex_t LOCK_log;
> @@ -376,6 +377,64 @@ struct Rows_event_factory
>}
> };
>
> +class Event_log: public MYSQL_LOG
> +{
> +public:
> +#if !defined(MYSQL_CLIENT)
> + Rows_log_event*
> + prepare_pending_rows_event(THD *thd, TABLE* table,
> + binlog_cache_data *cache_data,
> + uint32 serv_id, size_t needed,
> + bool is_transactional,
> + Rows_event_factory event_factory);
forgot to delete the same from MYSQL_BIN_LOG
> +#endif
> +
> + bool flush_and_sync(bool *synced);
same
> + void cleanup()
> + {
> +if (inited)
> + mysql_mutex_destroy(&LOCK_binlog_end_pos);
> +
> +MYSQL_LOG::cleanup();
> + }
> + void init_pthread_objects()
> + {
> +MYSQL_LOG::init_pthread_objects();
> +
> +mysql_mutex_init(m_key_LOCK_binlog_end_pos, &LOCK_binlog_end_pos,
> + MY_MUTEX_INIT_SLOW);
> + }
> +
> + bool open(
> + const char *log_name,
> + enum_log_type log_type,
> + const char *new_name, ulong next_file_number,
> + enum cache_type io_cache_type_arg);
> + IO_CACHE *get_log_file() { return &log_file; }
> +
> + int write_description_event(enum_binlog_checksum_alg checksum_alg,
> + bool encrypt, bool dont_set_created);
> +
> + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file);
> +};
> +
> /* Tell the io thread if we can delay the master info sync. */
> #define SEMI_SYNC_SLAVE_DELAY_SYNC 1
> /* Tell the io thread if the current event needs a ack. */
> @@ -451,7 +510,7 @@ class binlog_cache_data;
> struct rpl_gtid;
> struct wait_for_commit;
>
> -class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
> +class MYSQL_BIN_LOG: public TC_LOG, private Event_log
> {
>/** The instrumentation key to use for @ LOCK_index. */
>PSI_mutex_key m_key_LOCK_index;
> @@ -849,15 +901,13 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
>
> - bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file);
> + bool write_event(Log_event *ev, binlog_cache_data *data, IO_CACHE *file)
> + { return Event_log::write_event(ev, data, file); }
Perhaps
using Event_log::write_event(Log_event *ev, binlog_cache_data *data,
IO_CACHE *file);
?
>bool write_event(Log_event *ev) { return write_event(ev, 0, &log_file); }
>
>bool write_event_buffer(uchar* buf,uint len);
> @@ -918,7 +968,6 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG
>uint next_file_id();
>inline char* get_index_fname() { return index_file_name;}
>inline char* get_log_fname() { return log_file_name; }
> - inline char* get_name() { return name; }
>inline mysql_mutex_t* get_log_lock() { return &LOCK_log; }
should remove that too, I reckon
>inline mysql_cond_t* get_bin_log_cond() { return &COND_bin_log_updated; }
>inline IO_CACHE* get_log_file() { return &log_file; }
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 80749146b7d: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
Hi, Rucha! On Nov 05, Rucha Deodhar wrote: > > On Nov 04, Rucha Deodhar wrote: > > > revision-id: 80749146b7d (mariadb-10.6.1-77-g80749146b7d) > > > parent(s): 76149650764 > > > author: Rucha Deodhar > > > committer: Rucha Deodhar > > > timestamp: 2021-09-12 02:11:19 +0530 > > > message: > > > > > > MDEV-26238: Remove inconsistent behaviour of --default-* options > > > in my_print_defaults > > > > > > diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c > > > index b7f52382721..607509a309e 100644 > > > --- a/extra/my_print_defaults.c > > > +++ b/extra/my_print_defaults.c > > > @@ -141,50 +124,39 @@ static int get_options(int *argc,char ***argv) > > ... > > > + /* > > > + We already process --defaults* options at the beginning in > > > + get_defaults_options(). So skip --defaults* options and > > > + pass remaining options to handle_options(). > > > + */ > > > + if (is_prefix(org_argv[1], "--no-defaults")) > > > +args_used--; > > > > I'm sorry, I don't understand. Why do you need that? > > I made it to keep the output consistent with previous version. > > If --no-defaults is present in the command line, then it is counted in > args_used when get_defaults_options() is called. Later we skip > "args_used" number of arguments so that only non-default options > mentioned after --default-* options are passed to get_options(). > > The above code decreases this count when --no-defaults is present so > that we don't skip it unlike --defaults-* options and pass it to > get_options(). It is required to pass --no-defaults to get_options() > because if it is not passed then it produces output which looks like > --help ( if no other options are there in addition to --no-defaults). > If it is passed to get_options(), then --no-defaults produces no > output like it does in 10.6. But it's correct, isn't it? If you run my_print_defaults with no groups, it'll print the help text. Try, for example, my_print_defaults --defaults-file=var/my.cnf (presuming you're in mysql-test dir). You'll get the help text, because you didn't specify any groups. Or my_print_defaults --defaults-extra-file=var/my.cnf But my_print_defaults --no-defaults doesn't do it. Doesn't behave like other *defaults* options. We can easily preserve the inconsisent "if --no-defaults, no group is required" behavior with, like if (my_no_defaults) cleanup_and_exit(0); (and add my_no_defaults to my_default.h). But I don't think it's necessary. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 50f43d46299: MDEV-14938 make buildbot to include galera into bintars
Hi, Alexey!
On Nov 05, Alexey Bychko wrote:
> revision-id: 50f43d46299 (mariadb-10.2.40-106-g50f43d46299)
> parent(s): 05c3dced861
> author: Alexey Bychko
> committer: Alexey Bychko
> timestamp: 2021-10-22 16:30:56 +0700
> message:
>
> MDEV-14938 make buildbot to include galera into bintars
>
> this commit adds cmake module to process external directory with built
> galera to include galera library and binaries into server build.
> it introduces new cmake variable EXT_GALERA_PATH representing relative
> or absolute path to compiled galera tree.
No compiled galera tree. This is useless.
MDEV description says
Currently bintars that buildbot produces don't contain libgalera_smm.so.
Publishing scripts repackage bintars to add it.
We want to produce correct packages in buildbot and release exactly what
buildbot builds.
It means, buildbot need to produce *exactly* what `prep` script was
generating. `prep` script does not build galera. This is a completely
separate step, `prep` only packages pre-existing galera files.
It's enough to assume that there's ${EXT_GALERA_PATH}/garbd and
${EXT_GALERA_PATH}/libgalera_smm.so. Or if you want it to be really
universal, make two different variables,
cmake -DBINTAR_GARBD=/path/to/garbd
-DBINTAR_LIBGALERA=/path/to/libgalera_smm.so
This will work with a compiled galera tree, with installed galera
package, with just garbd and libgalera_smm.so in $HOME, with everything.
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 67216e0e443..1107013860c 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -153,6 +153,7 @@ INCLUDE(mysql_version)
> INCLUDE(cpack_source_ignore_files)
> INCLUDE(install_layout)
> INCLUDE(wsrep)
> +INCLUDE(galera_external)
>
> # Add macros
> INCLUDE(character_sets)
> diff --git a/cmake/galera_external.cmake b/cmake/galera_external.cmake
> new file mode 100644
> index 000..64ddfc9c498
> --- /dev/null
> +++ b/cmake/galera_external.cmake
> @@ -0,0 +1,75 @@
> +# EXT_GALERA_PATH
> +# a path to built galera git tree
> +# or a path to unpacked binary tarball
> +
> +IF(WIN32 OR NOT EXT_GALERA_PATH)
why do you need WIN32 check here?
> + RETURN()
> +ENDIF()
> +
> +IF(NOT EXISTS ${EXT_GALERA_PATH})
> + MESSAGE(FATAL_ERROR "Galera dir ${EXT_GALERA_PATH} does not exist!")
> +ENDIF()
> +
> +SET(GALERA_PATH ${CMAKE_SOURCE_DIR}/GALERASYM)
> +IF(EXISTS ${GALERA_PATH})
> + FILE({REMOVE ${GALERA_PATH})
> +ENDIF()
> +EXECUTE_PROCESS(COMMAND ${CMAKE_COMMAND} -E create_symlink
> ${EXT_GALERA_PATH} ${GALERA_PATH})
Why do you need this symlink?
> +
> +FIND_LIBRARY(GALERA_LIB
> + NAMES galera_smm galera_enterprise_smm
> + PATHS ${GALERA_PATH}
> + NO_DEFAULT_PATH
> + )
> +
> +IF(NOT GALERA_LIB)
> + MESSAGE(FATAL_ERROR "Galera library not found in ${GALERA_PATH}")
> +ENDIF()
> +
> +INSTALL(PROGRAMS
> + ${GALERA_LIB}
> + DESTINATION lib
> + RENAME libgalera_smm.so
> + COMPONENT Server
> + )
> +
> +INSTALL(PROGRAMS
> + ${GALERA_PATH}/garb/garbd
> + DESTINATION bin
> + COMPONENT Server
> + )
> +
> +INSTALL(PROGRAMS
> + ${GALERA_PATH}/garb/files/garb-systemd
> + DESTINATION bin
> + COMPONENT Server
> + )
> +
> +INSTALL(FILES
> + ${GALERA_PATH}/garb/files/garb.cnf
> + DESTINATION docs/galera
> + COMPONENT Server
> + )
MDEV description says
Currently bintars that buildbot produces don't contain libgalera_smm.so.
Publishing scripts repackage bintars to add it.
We want to produce correct packages in buildbot and release exactly what
buildbot builds.
It means, buildbot need to produce *exactly* what `prep` script was
generating.
Meaning three galera files. Like:
-rwxr-xr-x mariadb-10.2.40-linux-glibc_214-x86_64/bin/garbd
-rw-r--r-- mariadb-10.2.40-linux-glibc_214-x86_64/lib/libgalera_smm.so
lrwxrwxrwx mariadb-10.2.40-linux-glibc_214-x86_64/lib/galera/libgalera_smm.so
-> ../libgalera_smm.so
Nothing else.
Also, please, verify that the generated directory inside the tarball
matches the pattern above. And that the tarbal name does too.
> +
> +INSTALL(FILES
> + ${GALERA_PATH}/AUTHORS
> + ${GALERA_PATH}/COPYING
> + ${GALERA_PATH}/README
> + DESTINATION docs/galera/doc
> + COMPONENT Server
> + )
> +
> +INSTALL(FILES
> + ${GALERA_PATH}/asio/LICENSE_1_0.txt
> + DESTINATION docs/galera/doc
> + RENAME LICENSE.asio
> + COMPONENT Server
> + )
> +
> +INSTALL(FILES
> + ${GALERA_PATH}/chromium/LICENSE
> + DESTINATION docs/galera/doc
> + RENAME LICENSE.chromium
> + COMPONENT Server
> + )
> +
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 80749146b7d: MDEV-26238: Remove inconsistent behaviour of --default-* options in my_print_defaults
Hi, Rucha!
On Nov 04, Rucha Deodhar wrote:
> revision-id: 80749146b7d (mariadb-10.6.1-77-g80749146b7d)
> parent(s): 76149650764
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-09-12 02:11:19 +0530
> message:
>
> MDEV-26238: Remove inconsistent behaviour of --default-* options
> in my_print_defaults
>
> Analysis: --defaults* option is recognized anywhere in the commandline
> instead of only at the beginning because handle_options() recognizes
> options in any order.
> Fix: use get_defaults_options() which recognizes --defaults* options only at
> the beginning. After this is done, we only want to recognize other options
> given in any order which can be done using handle_options(). So only skip
> --defaults* options and pass rest of them to handle_options().
> Also, removed -e, -g and -c because only my_print_defaults supports them.
>
> diff --git a/extra/my_print_defaults.c b/extra/my_print_defaults.c
> index b7f52382721..607509a309e 100644
> --- a/extra/my_print_defaults.c
> +++ b/extra/my_print_defaults.c
> @@ -141,50 +124,39 @@ static int get_options(int *argc,char ***argv)
...
> int main(int argc, char **argv)
> {
> - int count= 0, error, no_defaults= 0;
> + int count, error, args_used;
>char **load_default_groups= 0, *tmp_arguments[6];
>char **argument, **arguments, **org_argv;
>int nargs, i= 0;
>MY_INIT(argv[0]);
>
>org_argv= argv;
> - if (*argv && !strcmp(*argv, "--no-defaults"))
> - {
> -argv++;
> -++count;
> -no_defaults= 1;
> - }
> - /* Copy program name and --no-defaults if present*/
> + args_used= get_defaults_options(argv);
> +
> + /* Copy defaults-xxx arguments & program name */
> + count= args_used;
>arguments= tmp_arguments;
> - memcpy((char*) arguments, (char*) org_argv, (++count)*sizeof(*org_argv));
> + memcpy((char*) arguments, (char*) org_argv, count*sizeof(*org_argv));
>arguments[count]= 0;
I still cannot understand why you're doing it. Why do you need a copy of
argv?
>
> + /*
> + We already process --defaults* options at the beginning in
> + get_defaults_options(). So skip --defaults* options and
> + pass remaining options to handle_options().
> + */
> + if (is_prefix(org_argv[1], "--no-defaults"))
> +args_used--;
I'm sorry, I don't understand. Why do you need that?
all *defaults* options were handled in get_defaults_options().
> +
why do you need org_argv? First you do
org_argv= argv;
then after some code that doesn't change either argv or org_argv, you
copy the vallue back:
> + argv=org_argv;
> + argv+=args_used-1;
> + argc-=args_used-1;
> +
>/* Check out the args */
>if (get_options(&argc,&argv))
> cleanup_and_exit(1);
>
> - if (!no_defaults)
> - {
> -if (opt_defaults_file_used)
> - arguments[count++]= make_args("--defaults-file=", config_file);
> -if (my_defaults_extra_file)
> - arguments[count++]= make_args("--defaults-extra-file=",
> - my_defaults_extra_file);
> - if (my_defaults_group_suffix)
> - arguments[count++]= make_args("--defaults-group-suffix=",
> - my_defaults_group_suffix);
> -arguments[count]= 0;
> - }
> -
>nargs= argc + 1;
>if (opt_mysqld)
> nargs+= array_elements(mysqld_groups);
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] bb1737b0482: MDEV-12459: The information_schema tables for getting temporary tables info is missing, at least for innodb there is no INNODB_TEMP_TABLE_INFO
> @@ -9032,7 +9096,6 @@ ST_FIELD_INFO tables_fields_info[]=
> NOT_NULL, "Comment",
> OPEN_FRM_ONLY),
>Column("MAX_INDEX_LENGTH",ULonglong(), NULLABLE, "Max_index_length",
>
> OPEN_FULL_TABLE),
> - Column("TEMPORARY", Varchar(1), NULLABLE, "Temporary",
> OPEN_FRM_ONLY),
>CEnd()
> };
Let's remove it in a separate commit. After you implement "TEMPORARY"
for table type.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 85aa3f8cf66: MDEV-26102: dgcov: add support for *.gcda.gcov.json.gz files of gcov 9.1+
Hi, Anel! Looks ok. Except that you have that pointless $gcc_version=~ line that I wrote about in a previous review. Did you verify that this script actually delivers meaningful results? On Oct 30, Anel Husakovic wrote: > revision-id: 85aa3f8cf66 (mariadb-10.2.39-96-g85aa3f8cf66) > parent(s): afe00bb7cce > author: Anel Husakovic > committer: Anel Husakovic > timestamp: 2021-07-29 12:42:44 +0200 > message: > > MDEV-26102: dgcov: add support for *.gcda.gcov.json.gz files of gcov 9.1+ > > Reviewed by: s...@mariadb.com > > --- > mysql-test/dgcov.pl | 55 > +++-- > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/mysql-test/dgcov.pl b/mysql-test/dgcov.pl > index 2c00c64d1ff..543099c1b4c 100755 > --- a/mysql-test/dgcov.pl > +++ b/mysql-test/dgcov.pl > @@ -62,12 +62,15 @@ my $res; > + > +my $gcc_version= `gcc -dumpversion`; > +$gcc_version=~ s/(\d).*$/$1/; > > find(\&gcov_one_file, $root); > find(\&write_coverage, $root) if $opt_generate; Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 2c8f52ea53d: MDEV-23328 Server hang due to Galera lock conflict resolution
Hi, Seppo, Jan! this is pretty much the same as in 10.4, so same comments apply, otherwise ok. On Oct 29, sjaakola wrote: > revision-id: 2c8f52ea53d (mariadb-10.5.12-108-g2c8f52ea53d) > parent(s): 910fc72a7f1 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-28 12:00:41 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 778d88ea2d9: MDEV-23328 Server hang due to Galera lock conflict resolution
Hi, Seppo, Jan!
Looks pretty much ok too.
A few comments, see below
On Oct 29, sjaakola wrote:
> revision-id: 778d88ea2d9 (mariadb-10.4.21-76-g778d88ea2d9)
> parent(s): e2e4e06f081
> author: sjaakola
> committer: Jan Lindström
> timestamp: 2021-10-28 11:06:22 +0300
> message:
>
> MDEV-23328 Server hang due to Galera lock conflict resolution
>
> diff --git a/sql/log_event.cc b/sql/log_event.cc
> index c1c753d69fd..a1562016aa4 100644
> --- a/sql/log_event.cc
> +++ b/sql/log_event.cc
> @@ -8987,6 +8987,18 @@ bool Xid_log_event::print(FILE* file,
> PRINT_EVENT_INFO* print_event_info)
>
>
> #if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT)
> +static bool wsrep_must_replay(THD *thd)
> +{
> +#ifdef WITH_WSREP
> + mysql_mutex_lock(&thd->LOCK_thd_data);
> + bool res= WSREP(thd) && thd->wsrep_trx().state() ==
> wsrep::transaction::s_must_replay;
check WSREP(thd) outside of the mutex
> + mysql_mutex_unlock(&thd->LOCK_thd_data);
> + return res;
> +#else
> + return false;
> +#endif
> +}
> +
> int Xid_log_event::do_apply_event(rpl_group_info *rgi)
> {
>bool res;
> diff --git a/sql/service_wsrep.cc b/sql/service_wsrep.cc
> index 6566bb372d8..18067a4d0ec 100644
> --- a/sql/service_wsrep.cc
> +++ b/sql/service_wsrep.cc
> @@ -29,12 +29,14 @@ extern "C" my_bool wsrep_on(const THD *thd)
>
> extern "C" void wsrep_thd_LOCK(const THD *thd)
> {
> + mysql_mutex_lock(&thd->LOCK_thd_kill);
>mysql_mutex_lock(&thd->LOCK_thd_data);
I presume, you can remove wsrep_thd_kill_LOCK() and
wsrep_thd_kill_UNLOCK() that I've added
> }
>
> extern "C" void wsrep_thd_UNLOCK(const THD *thd)
> {
>mysql_mutex_unlock(&thd->LOCK_thd_data);
> + mysql_mutex_unlock(&thd->LOCK_thd_kill);
> }
>
> extern "C" void wsrep_thd_kill_LOCK(const THD *thd)
> @@ -295,8 +301,6 @@ extern "C" my_bool wsrep_thd_is_aborting(const MYSQL_THD
> thd)
> default:
>return false;
>}
> -
> - return false;
this looks like it begs for the compiler warning.
please, keep the return where it was.
> }
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 002192b92cf: MDEV-23328 Server hang due to Galera lock conflict resolution
Hi, Seppo, Jan! On Oct 27, sjaakola wrote: > revision-id: 002192b92cf (mariadb-10.3.31-60-g002192b92cf) > parent(s): 736da112b65 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-27 12:15:48 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution This 10.3 version is also ok to push when you have 10.4 and 10.5 versions ready. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] d0e18d694f0: MDEV-23328 Server hang due to Galera lock conflict resolution
Hi, Seppo, Jan! On Oct 27, sjaakola wrote: > revision-id: d0e18d694f0 (mariadb-10.2.40-128-gd0e18d694f0) > parent(s): 408277e8012 > author: sjaakola > committer: Jan Lindström > timestamp: 2021-10-27 12:12:53 +0300 > message: > > MDEV-23328 Server hang due to Galera lock conflict resolution This commit is ok to push, thanks! Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] d1ec0231553: MDEV-26732 Assertion `0' failed in Item::val_native
Hi, Alexander! On Oct 26, Alexander Barkov wrote: > > > >> Fix: > >> > >> - Adding a new class Item_copy_inet6, which implements val_native(). > >> - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 > >> instead of Item_copy_string. Could there be a universal solution that uses memcpy and doesn't need Item_copy variant for every new type? Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] d1ec0231553: MDEV-26732 Assertion `0' failed in Item::val_native
Hi, Alexander! On Oct 25, Alexander Barkov wrote: > >> > >> MDEV-26732 Assertion `0' failed in Item::val_native > >> > >> Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / > >> Type_handler_inet6::Item_val_native_with_conversion > > > > would be good to have some explanation here, what was wrong, what was > > the fix. > > Right, I suggest this comment: > > > MDEV-26732 Assertion `0' failed in Item::val_native > > Also fixes MDEV-24619 Wrong result or Assertion `0' in > Item::val_native / Type_handler_inet6::Item_val_native_with_conversion > > Type_handler::create_item_copy() created a generic Item_copy_string, > which does not implement val_native() - it has a dummy implementation > with DBUG_ASSERT(0), which made the server crash. Why does it call val_native() if Item_copy_string doesn't implement it? If the item was a string in the first place, that is, if Item_copy_string would've been used correctly where it should've been used, then val_native() wouldn't be called? What would it use, val_str()? So why does it use val_native() here, because that Item_copy_string uses Type_handler_inet6? > Fix: > > - Adding a new class Item_copy_inet6, which implements val_native(). > - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 > instead of Item_copy_string. > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] d1ec0231553: MDEV-26732 Assertion `0' failed in Item::val_native
Hi, Alexander!
On Oct 25, Alexander Barkov wrote:
> revision-id: d1ec0231553 (mariadb-10.5.4-550-gd1ec0231553)
> parent(s): 8b115503563
> author: Alexander Barkov
> committer: Alexander Barkov
> timestamp: 2021-10-07 20:58:18 +0400
> message:
>
> MDEV-26732 Assertion `0' failed in Item::val_native
>
> Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native /
> Type_handler_inet6::Item_val_native_with_conversion
would be good to have some explanation here, what was wrong, what was
the fix.
> diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6.result
> b/plugin/type_inet/mysql-test/type_inet/type_inet6.result
> index da949481337..ac16f5c06ce 100644
> --- a/plugin/type_inet/mysql-test/type_inet/type_inet6.result
> +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6.result
> @@ -2159,3 +2159,38 @@ IFNULL(c, '::1')
> ::1
> DROP TABLE t2;
> DROP TABLE t1;
> +#
> +# MDEV-26732 Assertion `0' failed in Item::val_native
> +#
> +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop('x')
> FROM dual HAVING f > '';
do you need RAND() here for a test case? can you replace it with a
literal?
do you need var_pop() here?
> +fvar_pop('x')
> +Warnings:
> +Warning 1292Truncated incorrect DOUBLE value: 'x'
> +Warning 1292Incorrect inet6 value: ''
> +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop(1) FROM
> dual HAVING f >= '::';
> +fvar_pop(1)
> +:: 0.
> +CREATE TABLE t1(id INET6 NOT NULL PRIMARY KEY, dsc INET6);
> +INSERT INTO t1 VALUES ('::1', '1::1'),('::3', '1::3'),('::4', NULL);
> +CREATE TABLE t2 SELECT COALESCE(t1.dsc), COUNT(*) FROM t1 GROUP BY t1.id;
> +SELECT * FROM t2 ORDER BY 1,2;
> +COALESCE(t1.dsc) COUNT(*)
> +NULL 1
> +1::1 1
> +1::3 1
> +DROP TABLE t1, t2;
> +#
> +# MDEV-24619 Wrong result or Assertion `0' in Item::val_native /
> Type_handler_inet6::Item_val_native_with_conversion
> +#
> +CREATE TABLE t1 (a INET6);
> +INSERT INTO t1 VALUES ('::'),('::');
> +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '';
> +f
> +Warnings:
> +Warning 1292Incorrect inet6 value: ''
> +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::';
> +f
> +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::1';
> +f
> +::
> +DROP TABLE t1;
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 6d0c1f3ae12: MDEV-23328 Server hang due to Galera lock conflict resolution
Hi, Jan!
On Oct 25, Jan Lindström wrote:
> Hi Sergei,
>
> > > --- a/sql/sql_parse.cc
> > > +++ b/sql/sql_parse.cc
> > > @@ -2159,6 +2159,7 @@ bool dispatch_command(
> > > }
> > > DBUG_PRINT("quit",("Got shutdown command for level %u", level));
> > > general_log_print(thd, command, NullS);
> > > +DBUG_EXECUTE_IF("shutdown_unireg_abort", { unireg_abort(1); });
> >
> > what's the point of this test?
>
> I have a debug test for wsrep_close_connections and this will cause it to
> be run
Yes, I saw a test.
What I mean is, DBUG_EXECUTE_IF() is used to have some rare condition
happen deterministically. E.g. to simulate a network error or OOM. Or to
make a server crash at a certain point (like if it got a kill -9) or to
add a delay.
But you're adding a proper server shutdown at a point where none can
ever happen. I don't see how such a test can be of any value.
> > > + /* Here we need to lock THD::LOCK_thd_data to protect from
> > > + concurrent usage or disconnect or delete. */
> > > + DEBUG_SYNC(bf_thd, "wsrep_before_BF_victim_lock");
> > > + my_sleep(10);
> >
> > Eh? Forgot to remove after debugging?
>
> No, I did not, This was added for Rames to test with pquery
Just make sure they're removed before the push :)
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 5c4db10d6ff: MDEV-25114: Crash: WSREP: invalid state ROLLED_BACK (FATAL)
Hi, Jan! On Oct 24, Jan Lindström wrote: > revision-id: 5c4db10d6ff (mariadb-10.3.31-58-g5c4db10d6ff) > parent(s): f9b856b0525 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2021-10-22 07:23:14 +0300 > message: > > MDEV-25114: Crash: WSREP: invalid state ROLLED_BACK (FATAL) > > Revert "MDEV-23328 Server hang due to Galera lock conflict resolution" > > This reverts commit 29bbcac0ee841faaa68eeb09c86ff825eabbe6b6. This was incorrectly reverted. 29bbcac0ee841faaa68eeb09c86ff825eabbe6b6 didn't apply to 10.3 verbatim, there were some merge related changes. To revert it properly you need to look at affected files before the corresponding 10.2->10.3 merge. Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
