Re: [Maria-developers] MDEV-28152: Features for sequence

[Michael Widenius]

Hi!

I couple of small comments in addition to Sanjas review:

> > +longlong sequence_definition::truncate_value(const Longlong_hybrid& 
> > original)
> > +{
> > +  if (is_unsigned)
> > +return original.to_ulonglong(value_type_max());
> > +  else if (original.is_unsigned_outside_of_signed_range())
> > +return value_type_max();
> > +  else
> > +return original.value() > value_type_max() ? value_type_max()
> > +  : original.value() < value_type_min() ? value_type_min()
> > +  : original.value();

  Please remove all 'else' above.
  not needed and makes lines shorter.

  Please cache also original.value() in a local variable.
  Yes, it's a currently defined as { return m_value; } but as this may change
  in the future, it's better to be sure and store in a variable.
  Doing that will also make the code text sligtly shorter:

  Please also adjust the indentention to be as follows:

   return (value > value_type_max() ? value_type_max() :
  value < value_type_min() ? value_type_min()
  value);
In other words
- Operators last on the line
- Long operations should be surronded by braces to make it easier for the editor
  to align things properly.


> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index e3298a4a6c1..27ce8bcdbfc 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -2605,13 +2611,25 @@ sequence_defs:
> >  ;
> >
> >  sequence_def:
> > -  MINVALUE_SYM opt_equal sequence_truncated_value_num
> > +  AS int_type field_options
> > +  {

Please create a local variable:
 sequence_definition *seq= lex->create_info.seq_create_info;
and use it below. Makes the long lines much shorter!

> > +if (unlikely(Lex->create_info.seq_create_info->used_fields &
> > + seq_field_used_as))
> > +  my_yyabort_error((ER_DUP_ARGUMENT, MYF(0), "AS"));
> > +if ($3 & ZEROFILL_FLAG)
> > +my_yyabort_error((ER_BAD_OPTION_VALUE, MYF(0), "ZEROFILL", 
> > "AS"));
> > +Lex->create_info.seq_create_info->value_type = 
> > $2->field_type();
> > +Lex->create_info.seq_create_info->is_unsigned = $3 & 
> > UNSIGNED_FLAG ? true : false;
> > +Lex->create_info.seq_create_info->used_fields|= 
> > seq_field_used_as;
> > +  }

Regards,
Monty

___
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: MDEV-29075 Changing explicit_defaults_for_timestamp within stored procedure works inconsistently

[Michael Widenius]

Hi!

On Fri, 29 Jul 2022, 07:43 Alexander Barkov,  wrote:

>Hello Sergei,
>
> I have a couple of suggestions:
>


>

I suggest considering these ways:
>
>
> 1. Change "bool explicitly_nullable" to "bool explicit_nullability",
> which will be:
>
> - false if nothing was specified
> - true if NULL or NOT NULL was specified.
>
>
> 2. Or don't store NOT_NULL_FLAG in Column_definition::flags at all.
>
> Add a new Column_definition enum member with three values for:
> - not specified
> - explicit NULL
> - explicit NOT NULL
>

> This would require 4 or 8 bytes extra per field (and each field definition
> is stored twice per table), so this is probably not a good idea.


Regards,
Monty
___
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] eebebe8ef75: MDEV-23842 Atomic RENAME TABLE

[Michael Widenius]

Hi!

>> diff --git a/client/mysqltest.cc b/client/mysqltest.cc
>> index 350c55edee2..133d1f76839 100644
>> --- a/client/mysqltest.cc
>> +++ b/client/mysqltest.cc
>> @@ -8095,9 +8095,10 @@ void handle_error(struct st_command *command,
>>const char *err_sqlstate, DYNAMIC_STRING *ds)
>>  {
>>int i;
>> -
>>DBUG_ENTER("handle_error");
>>
>> +  var_set_int("$errno", err_errno);
>
>there was $mysql_errno already, was is not something
>you could've used?
>
>We had both errno and mysql_errno in the code.
>It think it is safer to reuse $errno as it is less used.
>Now errno is both latest sys error and lates sql error.
>
>> diff --git a/mysql-test/suite/atomic/rename_case.result 
>> b/mysql-test/suite/atomic/rename_case.result
>> new file mode 100644
>> index 000..4b58c555cf2
>> --- /dev/null
>> +++ b/mysql-test/suite/atomic/rename_case.result
>> @@ -0,0 +1,52 @@
>> +create database test2;
>> +#
>> +# Testing rename error in different places
>> +#
>> +create table t1 (a int);
>> +create table T2 (b int);
>> +create table t3 (c int);
>> +create table T4 (d int);
>> +insert into t1 values(1);
>> +insert into T2 values(2);
>> +insert into t3 values(3);
>> +insert into T4 values(4);
>> +create temporary table tmp1 (a int);
>> +create temporary table tmp2 (b int);
>> +create temporary table tmp3 (c int);
>> +create temporary table tmp4 (d int);
>> +insert into tmp1 values(11);
>> +insert into tmp2 values(22);
>> +insert into tmp3 values(33);
>> +insert into tmp4 values(44);
>> +rename table t3 to T4, t1 to t5, T2 to t1, t5 to T2;
>> +ERROR 42S01: Table 'T4' already exists
>> +rename table t1 to t5, t3 to T4, T2 to t1, t5 to T2;
>> +ERROR 42S01: Table 'T4' already exists
>> +rename table t1 to t5, T2 to t1, t3 to T4, t5 to T2;
>> +ERROR 42S01: Table 'T4' already exists
>> +rename table t1 to t5, T2 to t1, t5 to T2, t3 to T4;
>> +ERROR 42S01: Table 'T4' already exists
>> +# Try failed rename using two databases
>> +rename table test.t1 to test2.t5, test.T2 to test.t1, t5 to test.T2;
>> +ERROR 42S02: Table 'test.t5' doesn't exist
>> +select t1.a+T2.b+t3.c+T4.d from t1,T2,t3,T4;
>
>better do
>
>  select t1.a,T2.b,t3.c,T4.d from t1,T2,t3,T4;
>
>same one row, but it can detect when tables are swapped.

My row also detects if tables are swapped and is safe as it always returns
the same result.

The reason it detects swapped tabels is that each table has it's own column
names and you will get an error if the tables would be swapped.

>> --- a/sql/sql_class.h
>> +++ b/sql/sql_class.h
>> @@ -2835,6 +2835,7 @@ class THD: public THD_count, /* this must be first */
>>
>>  #ifndef MYSQL_CLIENT
>>binlog_cache_mngr *  binlog_setup_trx_data();
>> +  ulonglong binlog_xid;   /* binlog request storing of xid */
>
>"Tell binlog to store thd->query_id in the next Query_log_event"

Changed to:
  /*
If set, tell binlog to store the value as query 'xid' in the next
Query_log_event
  */

It is not always we store query_id and even if it would, the value of
binlog_xid is what is stored.

>> +++ b/sql/log_event_client.cc
>> @@ -1822,7 +1822,7 @@ bool Query_log_event::print_query_header(IO_CACHE* 
>> file,
>>  my_b_printf(file,
>>  "\t%s\tthread_id=%lu\texec_time=%lu\terror_code=%d\n",
>>  get_type_str(), (ulong) thread_id, (ulong) exec_time,
>> -error_code))
>> +error_code, (ulong) xid))
>
>huh? you didn't add "\nxid=%lu" to the format string

Ouch. Fixed



>>goto err;
>>}
>>
>> diff --git a/sql/debug_sync.h b/sql/debug_sync.h
>> index 3b8aa8815e1..4e3e10fcc51 100644
>> --- a/sql/debug_sync.h
>> +++ b/sql/debug_sync.h
>> @@ -53,4 +53,12 @@ static inline bool debug_sync_set_action(THD *, const 
>> char *, size_t)
>>  { return false; }
>>  #endif /* defined(ENABLED_DEBUG_SYNC) */
>>
>> +#ifndef DBUG_OFF
>> +void debug_crash_here(const char *keyword);
>> +bool debug_simulate_error(const char *keyword, uint error);
>> +#else
>> +#define debug_crash_here(A) do { } while(0)
>> +#define debug_simulate_error(A, B) 0
>> +#endif
>
>this doesn't belong to debug_sync.h or debug_sync.cc
>
>put it, hmm, may be in sql_priv.h ? or sql_class.h/sql_class.cc

Added it to sql/dbug.cc and sql/debug.h as agreed

>>  #endif /* DEBUG_SYNC_INCLUDED */
>> diff --git a/sql/debug_sync.cc b/sql/debug_sync.cc
>> index 1fbb15592a4..f523e22f6ce 100644
>> --- a/sql/debug_sync.cc
>> +++ b/sql/debug_sync.cc
>> @@ -1628,3 +1628,74 @@ bool debug_sync_set_action(THD *thd, const char 
>> *action_str, size_t len)
>>  /* prevent linker/lib warning about file without public symbols */
>>  int debug_sync_dummy;
>>  #endif /* defined(ENABLED_DEBUG_SYNC) */
>> +
>> +
>> +/**
>> +   Debug utility to do crash after a set number of executions
>> +
>> +   The user variable, either @debug_crash_counter or @debug_error_counter,
>
>this is very hackish. better to use @@debug_dbug_counter,
>something like that. also it could be global for an 

Re: [Maria-developers] c80f867b0df: MDEV-23844 Atomic DROP TABLE

[Michael Widenius]

Hi!

On Wed, May 12, 2021 at 3:18 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On May 12, Michael Widenius wrote:
> > revision-id: c80f867b0df (mariadb-10.6.0-78-gc80f867b0df)
> > parent(s): eebebe8ef75
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-05-04 21:09:11 +0300
> > message:
> >
> > MDEV-23844 Atomic DROP TABLE
> ...
> > diff --git a/sql/sql_view.cc b/sql/sql_view.cc
> > index e6d726b30d7..ff8aa55adef 100644
> > --- a/sql/sql_view.cc
> > +++ b/sql/sql_view.cc
> > @@ -1861,8 +1868,18 @@ bool mysql_drop_view(THD *thd, TABLE_LIST *views, 
> > enum_drop_mode drop_mode)
> >  not_exists_count++;
> >continue;
> >  }
> > +if (!first_table++)
>
> yuck. so your "first_table" actually means "not_first_view",
> because when it's FALSE, it's the first view.
>
> why not to rename it to "not_first_view" ?

I have renamed it to view_count.
I prefer to use a counter as it is easer to set and test at the same time.
Also, when debugging it also helps to see where one is.

> > +{
> > +  if (ddl_log_drop_view_init(thd, _log_state))
> > +DBUG_RETURN(TRUE);
>
> what about a crash here? the first entry is written, the second is not.

The init creates the bases for a set of views to drop.
We do init once and then we only need to do ddl_log_drop_view for the rest.

Most ddl_log operations has only one call.  Only those that can have
multiple operations,
like drop or rename has an init call.

> > +}
> > +if (ddl_log_drop_view(thd, _log_state, , >db,
> > +  >table_name))
> > +  DBUG_RETURN(TRUE);



> > diff --git a/sql/log.cc b/sql/log.cc
> > index 6e0a9706ec8..dba999054e4 100644
> > --- a/sql/log.cc
> > +++ b/sql/log.cc
> > @@ -10471,16 +10483,16 @@ int TC_LOG_BINLOG::recover(LOG_INFO *linfo, const 
> > char *last_log_name,
> >bool last_gtid_standalone= false;
> >bool last_gtid_valid= false;
> >  #endif
> > +  DBUG_ENTER("TC_LOG_BINLOG::recover");
> >
> >if (! fdle->is_valid() ||
> > -  (do_xa && my_hash_init(key_memory_binlog_recover_exec, ,
> > - _charset_bin, TC_LOG_PAGE_SIZE/3, 0,
> > - sizeof(my_xid), 0, 0, MYF(0
> > +  (my_hash_init(key_memory_binlog_recover_exec, ,
> > +_charset_bin, TC_LOG_PAGE_SIZE/3, 0,
> > +sizeof(my_xid), 0, 0, MYF(0
> >  goto err1;
> >
> > -  if (do_xa)
> > -init_alloc_root(key_memory_binlog_recover_exec, _root,
> > -TC_LOG_PAGE_SIZE, TC_LOG_PAGE_SIZE, MYF(0));
> > +  init_alloc_root(key_memory_binlog_recover_exec, _root,
> > +  TC_LOG_PAGE_SIZE, TC_LOG_PAGE_SIZE, MYF(0));
>
> I don't think these two changes are correct. do_xa is TRUE on crash recovery
> and FALSE on gtid state recovery on 5.5->10.0 upgade or if gtid state
> file was deleted.

The problem is that do_xa is not always true on crash recovery.

For example if the following is not true in do_binlog_recovery:

if (ev->flags & LOG_EVENT_BINLOG_IN_USE_F)

It will execute binglog recovery later with do_xa_recovery == false trough
the following code inTC_LOG_BINLOG::open(const char *opt_name)

if (!binlog_state_recover_done)
{
  binlog_state_recover_done= true;
  if (do_binlog_recovery(opt_bin_logname, false))
DBUG_RETURN(1);
}

Here is a trace:
#0  MYSQL_BIN_LOG::recover (this=0x2783d80 ,
linfo=0x7fff8590, last_log_name=0x7fff8100
"./master-bin.01", first_log=0x7fff8410, fdle=0x35685a8,
do_xa=false) at /my/maria-10.6/sql/log.cc:10474
#1  0x00dc4c10 in MYSQL_BIN_LOG::do_binlog_recovery
(this=0x2783d80 , opt_name=0x319d098 "master-bin",
do_xa_recovery=false) at /my/maria-10.6/sql/log.cc:10779
#2  0x00db0c90 in MYSQL_BIN_LOG::open (this=0x2783d80
, log_name=0x319d098 "master-bin", new_name=0x0,
next_log_number=0, io_cache_type_arg=WRITE_CACHE,
max_size_arg=1073741824, null_created_arg=false, need_mutex=true) at
/my/maria-10.6/sql/log.cc:3617
#3  0x007c0d24 in init_server_components () at

We also have to be able to collect xa events if someone has deleted the
gtid state file for ddl_recovery to work.

> There is no point to initialize the xid hash or mem_root,
> as it's not a crash recovery and there can be no incomplete transactions
> or ddl statements.

> Yes, I've seen the commit comment:
>
> > - TC_LOG_BINLOG::recover() was changed to always collect Xid's from the
> >   binary log and always call ddl_log_close_binlogged

Re: [Maria-developers] 9083627c50f: Make rename atomic/repeatable in MyISAM and Aria

[Michael Widenius]

Hi!

> > +  /*
> > +This code is written so that it should be possible to re-run a
> > +failed rename (even if there is a server crash in between the
> > +renames) and complete it.
> > +  */
> >
> > fn_format(from,old_name,"",MARIA_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> >
> > fn_format(to,new_name,"",MARIA_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> >if (mysql_file_rename_with_symlink(key_file_kfile, from, to,
> >   MYF(MY_WME | sync_dir)))
> > -DBUG_RETURN(my_errno);
> > +index_file_rename_error= my_errno;
>
> Shouldn't you continue only if index_file_rename_error == ENOENT ?

I thought about that, but in the end thought that there in almost all
real world cases
not be any harm in trying to rename the other file too.  If there
would be some kind of privilege error, we would probably get it for
the other file too.

> also, I only very quickly looked at mysql_file_rename_with_symlink,
> but it didn't look atomic to me when symlinks are present.

Yes, very likely. I did however not had time to look at symlinks as I
with the big hard disks of today
I don't think many are using symlinks.  Should be checked at some point.

> >
> > fn_format(from,old_name,"",MARIA_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> >
> > fn_format(to,new_name,"",MARIA_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> ...
> > diff --git a/storage/myisam/mi_rename.c b/storage/myisam/mi_rename.c
> > index 19df2e54213..13cf8c60897 100644
> > --- a/storage/myisam/mi_rename.c
> > +++ b/storage/myisam/mi_rename.c
> > @@ -22,6 +22,7 @@
> >  int mi_rename(const char *old_name, const char *new_name)
> >  {
> >char from[FN_REFLEN],to[FN_REFLEN];
> > +  int save_errno= 0;
> >DBUG_ENTER("mi_rename");
> >
> >  #ifdef EXTRA_DEBUG
> > @@ -32,10 +33,13 @@ int mi_rename(const char *old_name, const char 
> > *new_name)
> >
> > fn_format(from,old_name,"",MI_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> >fn_format(to,new_name,"",MI_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> >if (mysql_file_rename_with_symlink(mi_key_file_kfile, from, to, 
> > MYF(MY_WME)))
> > -DBUG_RETURN(my_errno);
> > +save_errno= my_errno;
> >
> > fn_format(from,old_name,"",MI_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> >fn_format(to,new_name,"",MI_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT);
> > -  DBUG_RETURN(mysql_file_rename_with_symlink(mi_key_file_dfile,
> > - from, to,
> > - MYF(MY_WME)) ? my_errno : 0);
> > +  if (mysql_file_rename_with_symlink(mi_key_file_dfile,
> > + from, to,
> > + MYF(MY_WME)))
> > +if (save_errno)
> > +  save_errno= my_errno;
> > +  DBUG_RETURN(save_errno);
>
> Why mi_rename isn't trying to undo a half-failed rename like ma_rename does?

Older code and also a very unlikely case.

When thinking about it, the code in Aria is probably wrong and should be fixed.
We have already written a log entry that we are going to rename the tables.
If we revert the table name change, the aria log entry will on
recovery force the table to be
renamed anyway, which is not good. Should be fixed at some point.

Regards,
Monty

___
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] 32c73138ff4: Remove some usage of Check_level_instant_set and Sql_mode_save

[Michael Widenius]

Hi!

On Mon, Apr 19, 2021 at 9:55 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 19, Michael Widenius wrote:
> > revision-id: 32c73138ff4 (mariadb-10.5.2-559-g32c73138ff4)
> > parent(s): 07708eb9b28
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-25 12:06:34 +0200
> > message:
> >
> > Remove some usage of Check_level_instant_set and Sql_mode_save
> >
> > The reason for the removal are:
> > - Generates more code
> >   - Storing and retreving THD
> >   - Causes extra code and daata to be generated to handle possible throw
> > exceptions (which never happens in MariaDB code)
> > - Uses more stack space
>
> No, Monty.
> We've discussed it already, this makes code more complex, fragile and
> bug prone.

Yes, we discussed this but we did not reach an agreement.
I say that the other version generates slopper, harder to understand code
that makes it easier to do mistakes.
I also HATE with passion code that does things behind the scenes.
I want to understand what the code does without having to loo at every
function that is used by the code.
> And it does *not* generates extra code or data or stack - before writing
> this email I've compiled mariadbd both ways and compared the generated
> code for the function in question, it was *identical*.

I did the same and they are NOT identical in cases where there was
more than 'one value saved'.
Depends probably on compiler options. Also, if we ever would need to
allow 'throw', then any
usage of constructs like this gets much worse.  Try and check yourself.

Note that I did only change this in very simple functions and kept
usage of Check_level_instant_set
in other places.

Regards,
Monty

___
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] 6b06a99e4a4: Avoid creating the .frm file twice in some cases

[Michael Widenius]

Hi!

On Mon, Apr 19, 2021 at 8:17 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 19, Michael Widenius wrote:
> > revision-id: 6b06a99e4a4 (mariadb-10.5.2-557-g6b06a99e4a4)
> > parent(s): 9c1e36696f3
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-25 12:06:34 +0200
> > message:
> >
> > Avoid creating the .frm file twice in some cases
> >
> > diff --git a/sql/handler.cc b/sql/handler.cc
> > index c286aef3a9e..fe2fe6e9426 100644
> > --- a/sql/handler.cc
> > +++ b/sql/handler.cc
> > @@ -5536,8 +5536,9 @@ int ha_create_table(THD *thd, const char *path,
> >
> >if (frm)
> >{
> > -bool write_frm_now= !create_info->db_type->discover_table &&
> > -!create_info->tmp_table();
> > +bool write_frm_now= (!create_info->db_type->discover_table &&
> > + !create_info->tmp_table() &&
> > + !create_info->frm_is_created);
>
> Is this the real problem? frm created twice?

Yes, this happens in case of discovery as part of alter table.
 I found this out while debugging ddl log crashing.
> Why ha_create_table() is invoked twice at all?

As far as I remember, once to discover (and create the frm) and once for
creating the internal table structures or something like that.

The code comment gives a bit more information:
/*
  Avoid creating frm again in ha_create_table() if inplace alter will not
  be used.
*/
create_info->frm_is_created= 1;

> > diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> > index e00d3ee1ed2..73642a5bd97 100644
> > --- a/sql/sql_table.cc
> > +++ b/sql/sql_table.cc
> > @@ -547,8 +547,13 @@ uint build_table_filename(char *buff, size_t bufflen, 
> > const char *db,
> >
> >(void) tablename_to_filename(db, dbbuff, sizeof(dbbuff));
> >
> > -  /* Check if this is a temporary table name. Allow it if a corresponding 
> > .frm file exists */
> > -  if (is_prefix(table_name, tmp_file_prefix) && strlen(table_name) < 
> > NAME_CHAR_LEN &&
> > +  /*
> > +Check if this is a temporary table name. Allow it if a corresponding 
> > .frm
> > +file exists.
> > +  */
> > +  if (!(flags & FN_IS_TMP) &&
> > +  is_prefix(table_name, tmp_file_prefix) &&
> > +  strlen(table_name) < NAME_CHAR_LEN &&
>
> good idea!
>
> >check_if_frm_exists(tbbuff, dbbuff, table_name))
> >  flags|= FN_IS_TMP;
> >
> > @@ -9817,7 +9827,7 @@ do_continue:;
> >
> >if (table->s->tmp_table != NO_TMP_TABLE)
> >{
> > -/* Close lock if this is a transactional table */
> > +/* Unlock lock if this is a transactional temporary table */
>
> strictly speaking, you don't "unlock lock",
> you "unlock a resource" (e.g. a table)
> or you "release a lock"

One a door, you unlock a lock.
Changed comment to release lock.

Regards,
Monty

___
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] 9c1e36696f3: MDEV-20025: ADD_MONTHS() Oracle function

[Michael Widenius]

Hi!

On Fri, Apr 16, 2021 at 8:25 PM Sergei Golubchik  wrote:

> > +++ b/mysql-test/suite/compat/oracle/r/func_add_months.result
> > @@ -0,0 +1,85 @@
> > +Test for ADD_MONTHS
> > +CREATE TABLE t1(c1 int, c2 datetime, c3 date, c4 time, c5 timestamp);
> > +INSERT INTO t1 VALUES (1, '2011-11-12 12:10:11', '2011-11-12', '12:10:11', 
> > '2011-11-12 12:10:11');
> > +INSERT INTO t1 VALUES (2, '2021-11-12 00:23:12', '2021-11-12', '00:23:12', 
> > '2021-11-12 00:23:12');
> > +INSERT INTO t1 VALUES (3, '2011-01-22 16:45:45', '2011-01-22', '16:45:45', 
> > '2011-01-22 16:45:45');
> > +INSERT INTO t1 VALUES (4, '2031-05-12 04:11:34', '2031-05-12', '04:11:34', 
> > '2031-05-12 04:11:34');
> > +INSERT INTO t1 VALUES (5, '2031-09-02 08:15:22', '2031-09-02', '08:15:22', 
> > '2031-09-02 08:15:22');
> > +INSERT INTO t1 VALUES (6, '-09-02 00:00:00', '-09-02', '00:00:00', 
> > '1980-09-02 00:00:00');
> > +INSERT INTO t1 VALUES (7, '-09-02', '-09-02', '00:00:00', 
> > '1980-09-02');
>
> there's no last-day test here. Like ADD_MONTHS('2012-01-31', 1) = '2012-02-29'
> should be here, as Oracle manual specifically documents this case.

Added to test case.

I didn't originally think that was important as ADD_MONTS is just a wrapper for
Item_date_add_internval, which should already have tests for things like this.

Regards,
Monty

___
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] fbe1dad15a1: Ensure that we do not allocate strings bigger than 4G in String objects.

[Michael Widenius]

Hi!

On Fri, Apr 16, 2021 at 8:08 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 16, Michael Widenius wrote:
> > revision-id: fbe1dad15a1 (mariadb-10.5.2-555-gfbe1dad15a1)
> > parent(s): 57c19902326
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-25 12:06:34 +0200
> > message:
> >
> > Ensure that we do not allocate strings bigger than 4G in String objects.
> >
> > This is needed as we are using uint32 for allocated and current length.
> >
> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index 9c57bb22085..fedf5f4a48a 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -37,6 +37,8 @@ bool Binary_string::real_alloc(size_t length)
> >DBUG_ASSERT(arg_length > length);
> >if (arg_length <= length)
> >  return TRUE; /* Overflow */
> > +  if (arg_length >= UINT_MAX32)
> > +return FALSE;
> >str_length=0;
> >if (Alloced_length < arg_length)
> >{
> > @@ -45,7 +47,6 @@ bool Binary_string::real_alloc(size_t length)
> >  arg_length,MYF(MY_WME | (thread_specific ?
> >  MY_THREAD_SPECIFIC : 0)
> >return TRUE;
> > -DBUG_ASSERT(length < UINT_MAX32);
> >  Alloced_length=(uint32) arg_length;
> >  alloced=1;
>
> I liked assert more. It meant that we should never under any
> circumstances request more than 4G from Binary_string::real_alloc.

I did not just remove the assert for the fun of it.

The problem with the assert is there was ways with wrong test cases to
get sql_string to allocate
too big strings. Instead of having these fail with an assert, we now
get a signal 'allocation failed' for these
(for code that checks the string argument).

I must have got this problem while testing something.

It may however be a good idea to generate a malloc fail error for this case.

Regards.
Monty

___
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] c157c285db9: Optimize Sql_alloc

[Michael Widenius]

Hi!

On Sat, Mar 27, 2021 at 7:14 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Mar 27, Michael Widenius wrote:
> > revision-id: c157c285db9 (mariadb-10.5.2-514-gc157c285db9)
> > parent(s): 16e38888c06
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 14:31:53 +0200
> > message:
> >
> > Optimize Sql_alloc
> >
> > - Remove 'dummy_for_valgrind' overrun marker as this doesn't help much.
> >   The element also distorts the sizes of objects a bit, which makes it
> >   harder to calculate gain in object sizes when doing size optimizations.
> > - Avoid one extra call indirection when using thd_get_current_thd(), which
> >   is used by Sql_alloc.



> >  MYSQL_THD _current_thd() { return THR_THD; }
> > +THD *thd_get_current_thd() { return THR_THD; }
>
> I'd rather remove thd_get_current_thd() completely, why do we need two
> identical functions?
>
> Sql_alloc can use _current_thd() just fine, I've tried (also rocksdb
> uses thd_get_current_thd in one place, but it can use current_thd like
> the rest of the server code).

I thought that thd_get_current_thd() was an interface function for
external (not server code).
We have had it in since 2015 when Svoj added it.

ok, I will add a patch before Optimize sql_alloc to remove it

Regards,
Monty

___
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] 21eb8969ce9: Improved storage size for Item, Field and some other classes

[Michael Widenius]

Hi!



> Right. But non-existent commits mean you cannot check out the bitfield
> code and run tests with it. It won't compile because columnstore will
> fail to check out and cmake will stop before any compilation will even
> be able to start.

I don't really care if one for 3-5 commits in a stack of 80 cannot
compile with Columnstore.

What I care of is to record in the source the iterations of bit fields.

Regards,
Monty

___
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] 4088269c578d: Improved storage size for Item, Field and some other classes

[Michael Widenius]

Hi!

On Fri, Dec 4, 2020 at 2:50 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Dec 03, Michael Widenius wrote:
> > revision-id: 4088269c578d (mariadb-10.5.2-270-g4088269c578d)
> > parent(s): 994ea2af3973
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2020-09-17 12:25:44 +0300
> > message:
> >
> > Improved storage size for Item, Field and some other classes
> >
> > diff --git a/storage/columnstore/columnstore 
> > b/storage/columnstore/columnstore
> > index b6b02ed516f9..f606e76fb779 16
> > --- a/storage/columnstore/columnstore
> > +++ b/storage/columnstore/columnstore
> > @@ -1 +1 @@
> > -Subproject commit b6b02ed516f92055127d416370799d91a82754ea
> > +Subproject commit f606e76fb779e40f3376693fff9969e4f2b7669a
>
> Again. You should not push invalid commit hashes into the main branch

This is a side effect of doing rebases and working with sub projects
that one has to update.
On the top level the tree is fine.

Regards,
Monty

___
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] 16e38888c06: Improved storage size for Item, Field and some other classes

[Michael Widenius]

Hi!

On Sat, Mar 27, 2021 at 6:53 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> Did you consider trying -Wpadded?
>
> '-Wpadded'
>  Warn if padding is included in a structure, either to align an
>  element of the structure or to align the whole structure.
>  Sometimes when this happens it is possible to rearrange the fields
>  of the structure to reduce the padding and so make the structure
>  smaller.

No, I have not used that. Good to know about if I will try to do
rearrangement of some other structures later
(There are a lot of big structures in the server that would benefit of
rearrangement of variables)

Regards,
Monty

___
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] a94502ab674: Added typedef decimal_digits_t (uint16) for number of decimals

[Michael Widenius]

Hi!

>> Added typedef decimal_digits_t (uint16) for number of decimals
>> For fields and Item's uint8 should be good enough. After
>> discussions with Alexander Barkov we choose uint16 (for now)
>> as some format functions may accept +256 digits.
>>
>> The reason for this patch was to make the storage of decimal
>> digits simlar. Before this patch decimals was stored/used as
>> uint8, int and uint.
>>
>> Changed most decimal variables and functions to use the new
>> typedef.
>>
>> diff --git a/include/decimal.h b/include/decimal.h
>> index cab18f99348..f713409ede1 100644
>> --- a/include/decimal.h
>> +++ b/include/decimal.h
>> @@ -53,10 +53,11 @@ int decimal2double(const decimal_t *from, double *to);
>>  int double2decimal(double from, decimal_t *to);
>>  int decimal_actual_fraction(const decimal_t *from);
>>  int decimal2bin(const decimal_t *from, uchar *to, int precision, int scale);
>> -int bin2decimal(const uchar *from, decimal_t *to, int precision, int scale);
>> +int bin2decimal(const uchar *from, decimal_t *to, int precision,
>> +decimal_digits_t scale);
>>
>> -int decimal_size(int precision, int scale);
>> -int decimal_bin_size(int precision, int scale);
>> +int decimal_size(int precision, decimal_digits_t scale);
>> +int decimal_bin_size(int precision, decimal_digits_t scale);
>
>this is *very* confusing.
>
>"decimal" in the context of an Item mean "number of digits after a dot"
>"decimal" in decimal.h means the whole number, and after a dot is "scale"

The intention with the patch was to have (over time) one type for all
lengths associated with length-of-numbers. This include precision,
scale, digits before '.', digits after '.' etc.

I started with 'digits after dot', and then fixed other related things.

I have now, by your request, created a full patch that fixes all of the
above.



>> -static uint get_decimal_precision(uint len, uint8 dec, bool unsigned_val)
>> +static decimal_digits_t get_decimal_precision(uint len, decimal_digits_t 
>> dec,
>> +  bool unsigned_val)
>>  {
>>uint precision= my_decimal_length_to_precision(len, dec, unsigned_val);
>> -  return MY_MIN(precision, DECIMAL_MAX_PRECISION);
>> +  return (decimal_digits_t) MY_MIN(precision, DECIMAL_MAX_PRECISION);
>
>No, this is wrong (or, rather, inconsistent with your other changes).
>"precision" for DECIMAL is the total number of digits, not a number of
>digits after the dot. Judging by your edits in decimal.h you don't want
>that to be decimal_digits_t.

It's not inconsistent or a contradiction, it is an extension ;)
I agree that I should have mentioned in the commit comment that the new
type is also used for precision in some places.
Anyway, the new patch should fix our concerns.



>>{
>>  return type_handler()->Item_decimal_scale(this);
>>}
>> @@ -4853,7 +4853,7 @@ class Item_temporal_literal :public Item_literal
>>  Item_literal(thd)
>>{
>>  collation= DTCollation_numeric();
>> -decimals= dec_arg;
>> +decimals= (decimal_digits_t) dec_arg;
>
>why do you cast? I'd expect dec_arg to be decimal_digits_t already

  Item_temporal_literal(THD *thd, uint dec_arg):
Item_literal(thd)
  {
collation= DTCollation_numeric();
decimals= (decimal_digits_t) dec_arg;
  }

It was not. I will fix that.
I only add cast when the compiler requires or asks for it..

>> diff --git a/sql/item_func.cc b/sql/item_func.cc
>> index a0ef4020aae..db51639a5af 100644
>> --- a/sql/item_func.cc
>> +++ b/sql/item_func.cc
>> @@ -2710,7 +2710,7 @@ my_decimal *Item_func_round::decimal_op(my_decimal 
>> *decimal_value)
>>  dec= INT_MIN;
>>
>>if (!(null_value= (value.is_null() || args[1]->null_value ||
>> - value.round_to(decimal_value, (uint) dec,
>> + value.round_to(decimal_value, (int) dec,
>>  truncate ? TRUNCATE : HALF_UP) > 1)))
>
>I don't think you need to cast decimal_digits_t (aka uint16) to int
>explicitly. A compiler can handle it, it's not lossy.

Here dec is calculated is a longlong, so it needs a cast.
(We got this from a val_int() call).

Note that in this particular place, dec can be negative and that is ok
for value.round().
The orginal code was wrong. It should never have been casted to an int.



>> --- a/sql/item_func.h
>> +++ b/sql/item_func.h
>> @@ -1383,10 +1383,10 @@ class Item_decimal_typecast :public Item_func
>>  {
>>my_decimal decimal_value;
>>  public:
>> -  Item_decimal_typecast(THD *thd, Item *a, uint len, uint dec)
>> +  Item_decimal_typecast(THD *thd, Item *a, uint len, decimal_digits_t dec)
>> :Item_func(thd, a)
>>{
>> -decimals= (uint8) dec;
>> +decimals= (decimal_digits_t)  dec;
>
>not needed, dec is decimal_digits_t already

thanks.  I probably just replaced one cast, for which I got a warning,
with another.

>
>>  collation= DTCollation_numeric();
>>  

Re: [Maria-developers] 2be9b69f4ff: MDEV-23001 Precreate static Item_bool() to simplify code

[Michael Widenius]

Hi!



> > MDEV-23001 Precreate static Item_bool() to simplify code
> >
> > The following changes where done:
> > - Create global Item: Item_false and Item_true
> > - Replace all creation if 'FALSE' and 'TRUE' top level items used for
> >   WHERE/HAVING/ON clauses to use Item_false and Item_true.
> >
> > The benefit are:
> > - No test needed if we where able to create the new item.
> > - Fixed possible errors if 'new' would have failed for the Item_bool's
>
> agree
>
> > - Less and faster code
>
> I don't quite agree with that, it's not less code:

It is less code to add usage of a static bool item and it is faster to execute
it (no fix fields etc).

> >  7 files changed, 65 insertions(+), 43 deletions(-)
>
> and I doubt that speedup can be measured in any benchmarks whatsoever.

I agree. However this is a test for being able to create static items, that is
useful in any case.

> > diff --git a/sql/item.cc b/sql/item.cc
> > index 1a86b8b3114..0f160fa257b 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -55,7 +55,8 @@ const char *item_empty_name="";
> >  const char *item_used_name= "\0";
> >
> >  static int save_field_in_field(Field *, bool *, Field *, bool);
> > -
> > +const Item_bool_static Item_false("FALSE", 0);
> > +const Item_bool_static Item_true("TRUE", 1);
>
> The main question - we have tons of modifiable Item members. How do you
> know none of them will modified in your Item_bool_static objects?

Beause these are global const. We will get an assert if anyone tries.


> > @@ -421,6 +421,9 @@ Item::Item(THD *thd):
> > /* Initially this item is not attached to any JOIN_TAB. */
> >join_tab_idx= MAX_TABLES;
> >
> > +   /* thd is NULL in case of static items like Item_true */
> > +  if (!thd)
> > +return;
>
> No, please, don't.
> thd can be NULL only twice, for statically initialized Item_true and
> Item_false. For that you removed a useful assert and added an if() that
> is completely unnecessary for billions of items created runtime.
>
> Better to have special contructor just for these two very special items.

You mean like Item()?
Good idea, will do that.

However, when trying that, it was not very easy because of the Item
hierachy that bar has created :(
Items are depending on a lot of other items. This is just the tip of
the iceberg:

diff --git a/sql/item.cc b/sql/item.cc
index 49e7b9a6266..fb63b4b7fb2 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -421,9 +421,6 @@ Item::Item(THD *thd):
/* Initially this item is not attached to any JOIN_TAB. */
   join_tab_idx= MAX_TABLES;

-   /* thd is NULL in case of static items like Item_true */
-  if (!thd)
-return;
   /* Put item in free list so that we can free all items at end */
   next= thd->free_list;
   thd->free_list= this;
@@ -441,6 +438,21 @@ Item::Item(THD *thd):
   }
 }

+/*
+  This is only used for static const items
+*/
+
+Item::Item():
+  is_expensive_cache(-1), rsize(0), name(null_clex_str), orig_name(0),
+  common_flags(IS_AUTO_GENERATED_NAME)
+{
+  marker= 0;
+  maybe_null= null_value= with_window_func= with_field= false;
+  in_rollup= 0;
+  with_param= 0;
+  join_tab_idx= MAX_TABLES;
+}
+

 const TABLE_SHARE *Item::field_table_or_null()
 {
diff --git a/sql/item.h b/sql/item.h
index e188663fddd..a9b622553ae 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -963,6 +963,7 @@ class Item: public Value_source,
  optimisation changes in prepared statements
   */
   Item(THD *thd, Item *item);
+  Item();/* For const item */
   virtual ~Item()
   {
 #ifdef EXTRA_DEBUG
@@ -2866,6 +2867,7 @@ class Item_basic_constant :public Item_basic_value
 {
 public:
   Item_basic_constant(THD *thd): Item_basic_value(thd) {};
+  Item_basic_constant(): Item_basic_value() {};
   bool check_vcol_func_processor(void *arg) { return false; }
   const Item_const *get_item_const() const { return this; }
   virtual Item_basic_constant *make_string_literal_concat(THD *thd,
@@ -3247,7 +3249,8 @@ class Item_literal: public Item_basic_constant
 public:
   Item_literal(THD *thd): Item_basic_constant(thd)
   { }
-  Type type() const override { return CONST_ITEM; }
+  Item_literal(): Item_basic_constant()
+Type type() const override { return CONST_ITEM; }
   bool check_partition_func_processor(void *int_arg) override { return false;}
   bool const_item() const override { return true; }
   bool basic_const_item() const override { return true; }
@@ -3260,6 +3263,7 @@ class Item_num: public Item_literal
 {
 public:
   Item_num(THD *thd): Item_literal(thd) { collation= DTCollation_numeric(); }
+  Item_num(): Item_literal() { collation= DTCollation_numeric(); }
   Item *safe_charset_converter(THD *thd, CHARSET_INFO *tocs) override;
   bool get_date(THD *thd, MYSQL_TIME *ltime, date_mode_t fuzzydate) override
   {
@@ -4239,6 +4243,10 @@ class Item_int :public Item_num
   unsigned_flag= flag;
 }
   Item_int(THD *thd, const char *str_arg, size_t length=64);
+  Item_int(ulonglong i, size_t length):
+

Re: [Maria-developers] c76ac1e6de8: MDEV-24089 support oracle syntax: rownum

[Michael Widenius]

Hi!

On Mon, Apr 19, 2021 at 10:52 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 18, Michael Widenius wrote:
> > > > diff --git a/mysql-test/main/rownum.test b/mysql-test/main/rownum.test
> > > > new file mode 100644
> > > > index 000..8d89eed2f86
> > > > --- /dev/null
> > > > +++ b/mysql-test/main/rownum.test
> > > > @@ -0,0 +1,542 @@
> > > > +--source include/have_sequence.inc
> > > > +#
> > > > +# Test of basic rownum() functionallity
> > > > +# Test are done with Aria to ensure that row order is stable
> > >
> > > what does that mean?
> >
> > It means that SELECT * will return the rows in the same order for the test.
> > (There is normally no guaranteed that insert and select order are the same).
>
> But both MyISAM and InnoDB guarantee that too. What's so special with
> Aria?

MyISAM does guarantee that, but not InnoDB (returns rows in primary
key order) and
Aria with page storage will not do it either.


> > > > +  OPEN c_cursor;
> > > > +  FETCH c_cursor INTO v_a, v_b, v_rn;
> > > > +  WHILE c_cursor%FOUND LOOP
> > > > + SELECT concat(v_a,'--',v_b,'--',v_rn);
> > > > + FETCH c_cursor INTO v_a, v_b, v_rn;
> > > > +  END LOOP;
> > > > +  CLOSE c_cursor;
> > > > +END;|
> > > > +DELIMITER ;|
> > > > +
> > > > +--error ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE
> > > > +select a, rownum from t1 group by a, rownum having rownum < 3;
> > > > +select a, rownum from t1 group by a, rownum having "rownum" < 3;
> > >
> > > what's the difference between these two? Why the first is an error and the
> > > second is not?
> >
> > There are quotes around rownum in the second version.  In Oracle mode that 
> > is
> > an error, in MariaDB mode it is not.
>
> I can see the quotes, that's what I asked about.
> Why *in Oracle mode* the first is an error and the second is not?
> I'd expect them to be identical.

In Oracle you cannot use rownum function in having.
You can however, thanks to MariaDB, refer to a column in the select part.

> > > > +select a, rownum as r from t1 group by a, rownum having r < 3;
> > >
> > > And the this one. Why is it not an error either?
> >
> > Because 'r' is a reference, not a function.  'r' works on the result set.
> > rownum cannot be executed in the having clause (which is executed
> > after all rows are returned).
>
> I don't understand it. HAVING is not executed *after* all rows are
> returned, it's executed *while* they're returned. What is the difference
> between rownum directly in HAVING and referring to it by an alias?

having rownum < 0 is NOT using an alias.

You can not use a row function on a group result.
The group does NOT have a rownum.  Each group consist of a set of rows
each having it's own rownum.  Which rownum should the rownum usage in
having get?

To put things simple, we are doing exactly what Oracle is doing, which
is the purpose of this patch.



> > > how can rownum be NULL?
> >
> > In context where there are no rows.   like SET @A=rownum;
>
> Do you have test cases for that?

Yes, of course. Was part of the patch:

--echo #
--echo # Rownum() used in not supported places (returns 0 or gives an error)
--echo #

set @a=rownum();
select @a;

> > > > +return FALSE;
> > > > +  }
> > > > +  void cleanup() override
> > > > +  {
> > > > +Item_longlong_func::cleanup();
> > > > +/* Ensure we don't point to freed memory */
> > > > +accepted_rows= 0;
> > > > +  }
> > > > +  bool check_vcol_func_processor(void *arg) override
> > > > +  {
> > > > +return mark_unsupported_function(func_name(), "()", arg,
> > > > + VCOL_IMPOSSIBLE);
> > > > +  }
> > > > +  bool check_handler_func_processor(void *arg) override
> > > > +  {
> > > > +return mark_unsupported_function(func_name(), "()", arg,
> > > > + VCOL_IMPOSSIBLE);
> > >
> > > first, as I wrote above, I think that there's no logical reason why 
> > > rownum should
> > > not work with handler.
> >
> > Please read the patch. It cannot be in the handler for multiple reasons:
> > - INSERT, UPDATE, LOAD DATA
> > - handler does not know if the row is accepted
> > - joins
> >  etc.
>
> I mean the HANDLER statement, that's

Re: [Maria-developers] 57c19902326: MDEV-20017 Implement TO_CHAR() Oracle compatible function

[Michael Widenius]

Hi!

On Fri, Apr 16, 2021 at 5:44 PM Sergei Golubchik  wrote:

> > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> > index aecb00563f7..b23522ac830 100644
> > --- a/sql/share/errmsg-utf8.txt
> > +++ b/sql/share/errmsg-utf8.txt
> > @@ -7975,3 +7975,5 @@ ER_PK_INDEX_CANT_BE_IGNORED
> >  eng "A primary key cannot be marked as IGNORE"
> >  ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE
> >  eng "Function '%s' cannot be used in the %s clause"
> > +ER_ORACLE_COMPAT_FUNCTION_ERROR
> > +eng "Oracle compatibility function error: %s"
>
> Why? We normally just say "invalid argument" or something, in no other case
> we say "oracle compatibility function" or "sybase compatibility function" or
> "odbc compatibility function".

Fixed by reusing ER_STD_INVALID_ARGUMENT

> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index 95a57017c53..9c57bb22085 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -1275,3 +1275,15 @@ void Binary_string::shrink(size_t arg_length)
> >  }
> >}
> >  }
> > +
> > +bool Binary_string::strfill(char fill, size_t len)
> > +{
> > +  if (len)
> > +  {
> > +if (alloc(length() + len))
> > +  return 1;
> > +memset(Ptr + str_length, fill, len);
> > +str_length+= (uint32) len;
> > +  }
> > +  return 0;
> > +}
>
> There's Binary_string::fill() already.
>
> better use it or, at the very least, declare
>
>  bool strfill(char fill, size_t len) { return fill(str_length + len, fill); }
>
> in sql_string.h. And this swapped order of arguments is confusing,
> please fix it too.

Agree, I did miss this when fixing a fill loop in the original commit.
I have now removed strfill(), as you requested.



> > diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h
> > index af266956b05..9b78d6c159e 100644
> > --- a/sql/item_timefunc.h
> > +++ b/sql/item_timefunc.h



> > +public:
> > +  Item_func_tochar(THD *thd, Item *a, Item *b):
> > +Item_str_func(thd, a, b), locale(0)
> > +  {
> > +/* NOTE: max length of warning message is 64 */
> > +warning_message.alloc(64);
> > +warning_message.length(0);
> > +  }
>
> As far as I understand, this warning_message was introduced to issue
> the same error for every row, even if the format string is const_item and
> is parsed only once, in fix_fields.
>
> I don't think it's worth the trouble. Two simpler approaches are:
> * if the format string is invalid - parse it every time even if const, or
> * if the const format string is invalid - issue the error only once.
>
> so, please, remove warning_message and just use push_warning or my_error
> where the error is discovered.

That may not work well for prepared statements or stored procdures
where we may not call fix_length_and_dec() twice.

I also think it is a bit wrong that one gets a different number of
warning messages depending on when the arguments are processed.

For now, I rather keep things as they are. Someone that has time to do
checking of the above cases and ensures that we do not miss any warnings
messages in some context can then consider what to do.

> > --- a/sql/item_timefunc.cc
> > +++ b/sql/item_timefunc.cc



> > +  Models for datetime, used by TO_CHAR/TO_DATE. Normal format characters 
> > are
> > +  stored as short integer < 256, while format characters are stored as a
> > +  integer > 256
> > +*/
> > +
> > +#define FMT_BASE   128
>
> 128? or 256?

128. I have fixed the error message

> > +#define FMT_AD FMT_BASE+1
> > +#define FMT_AD_DOT FMT_BASE+2
> > +#define FMT_AM FMT_BASE+3
> > +#define FMT_AM_DOT FMT_BASE+4


> Not enum? Not even safe (with parentheses) #define?
> enum would be ideal here but at the very least make these defines safe.

This is 100% safe in the context it is used.  However, I agree that enum
is better and I have now converted the above to an enum.

> > +
> > +/**
> > +  Modify the quotation flag and check whether the subsequent process is 
> > skipped
>
> Could you reword it please?

Changed to:

   Flip 'quotation_flag' if we found a quote (") character.

The old function comment explains this in more detail.
>
> > +
> > +  @param cftm Character or FMT... format descriptor
> > +  @param quotation_flag   Points to 'true' if we are inside a quoted string
> > +
> > +  @return true  If we are inside a quoted string or if we found a '"' 
> > character
> > +  @return false Otherwise
> > +*/
> > +
> > +static inline bool check_quotation(uint16 cfmt, bool *quotation_flag)
> > +{
> > +  if (cfmt == '"')
> > +  {
> > +*quotation_flag= !*quotation_flag;
> > +return true;
> > +  }
> > +  return *quotation_flag;
> > +}
> > +
> > +#define INVALID_CHARACTER(x) (((x) >= 'A' && (x) <= 'Z') ||((x) >= '0' && 
> > (x) <= '9') || (x) >= 127 || ((x) < 32))
>
> why not to make this static inline too?
> side-effect safe, no risk of double evaluation of x.

The above is safe in the context it is used. For simple things like
this, which is only used in a few places 

Re: [Maria-developers] a1440737662: MDEV-20021 sql_mode="oracle" does not support MINUS set operator

[Michael Widenius]

Hi!



> > MINUS is mapped to EXCEPT
> >
> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index 57ba9df42c0..edd2f353dd0 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -16037,6 +16038,7 @@ reserved_keyword_udt_not_param_type:
> >  | MINUTE_MICROSECOND_SYM
> >  | MINUTE_SECOND_SYM
> >  | MIN_SYM
> > +| MINUS_ORACLE_SYM
> >  | MODIFIES_SYM
> >  | MOD_SYM
> >  | NATURAL
>
> this is not good. MINUS should be in reserved_keyword_udt_not_param_type
> only in oracle mode, and otherwise it should be in
> keyword_sp_var_and_label (or not a keyword at all, but I don't think
> it's possible).

Good catch. Fixed according to suggestions (must be a keyword as you thought).
I also extended  the oracle/minus.test to cover this.

Regards,
Monty

___
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] 04a13e6ab8f: MDEV-24285 support oracle build-in function: sys_guid

[Michael Widenius]

Hi!

On Tue, Apr 13, 2021 at 11:37 AM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 13, Michael Widenius wrote:
> > revision-id: 04a13e6ab8f (mariadb-10.5.2-552-g04a13e6ab8f)
> > parent(s): c76ac1e6de8
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 19:42:30 +0200
> > message:
> >
> > MDEV-24285 support oracle build-in function: sys_guid
> >
> > SYS_GUID() returns same as UUID(), but without any '-'
>
> I'd rather do it as a flag in Item_func_uuid.
> Less code, no need to copy-paste everything.

I thought about that, but it would be more work and I did not have
time to do that
and as this was not much code, easier to go with this implementation.

Regards,
Monty

___
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] c76ac1e6de8: MDEV-24089 support oracle syntax: rownum

[Michael Widenius]

On Mon, Apr 12, 2021 at 7:20 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Apr 12, Michael Widenius wrote:
> > revision-id: c76ac1e6de8 (mariadb-10.5.2-551-gc76ac1e6de8)
> > parent(s): a507eb17708
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 19:42:24 +0200
> > message:

Please include always the first row in the commit message so that I
can know which commit you are reviewing.
(revision id's are often useless for reviews)

> > diff --git a/mysql-test/main/rownum.test b/mysql-test/main/rownum.test
> > new file mode 100644
> > index 000..8d89eed2f86
> > --- /dev/null
> > +++ b/mysql-test/main/rownum.test
> > @@ -0,0 +1,542 @@
> > +--source include/have_sequence.inc
> > +#
> > +# Test of basic rownum() functionallity
> > +# Test are done with Aria to ensure that row order is stable
>
> what does that mean?

It means that SELECT * will return the rows in the same order for the test.
(There is normally no guaranteed that insert and select order are the same).

Please do not include things that your not commenting upon, to make it
easier to find your
comments.

> > +  OPEN c_cursor;
> > +  FETCH c_cursor INTO v_a, v_b, v_rn;
> > +  WHILE c_cursor%FOUND LOOP
> > + SELECT concat(v_a,'--',v_b,'--',v_rn);
> > + FETCH c_cursor INTO v_a, v_b, v_rn;
> > +  END LOOP;
> > +  CLOSE c_cursor;
> > +END;|
> > +DELIMITER ;|
> > +
> > +--error ER_FUNCTION_CANNOT_BE_USED_IN_CLAUSE
> > +select a, rownum from t1 group by a, rownum having rownum < 3;
> > +select a, rownum from t1 group by a, rownum having "rownum" < 3;
>
> what's the difference between these two? Why the first is an error and the
> second is not?

There are quotes around rownum in the second version.  In Oracle mode that is
an error, in MariaDB mode it is not.

> > +select a, rownum as r from t1 group by a, rownum having r < 3;
>
> And the this one. Why is it not an error either?

Because 'r' is a reference, not a function.  'r' works on the result set.
rownum cannot be executed in the having clause (which is executed
after all rows are returned).

> > diff --git a/sql/filesort.h b/sql/filesort.h
> > index 29ae5e20cc6..806a4a8b027 100644
> > --- a/sql/filesort.h
> > +++ b/sql/filesort.h
> > @@ -44,24 +44,21 @@ class Filesort: public Sql_alloc
> >/** Number of records to return */
> >ha_rows limit;
> >/** ORDER BY list with some precalculated info for filesort */
> > +  ha_rows *accepted_rows;   /* For ROWNUM */
> >SORT_FIELD *sortorder;
>
> first, I suspect that you want to put your accepted_rows declaration
> before the comment that applies to sortorder.

Fixed.

> second, could you do a bit more than just "For ROWNUM"?
> why do you need something "for ROWNUM" inside Filesort?

Because we excute the WHERE clause under filesort.
and then rownum function needs to know how many rows we have found so far.


I added
  /* Used with ROWNUM. Contains the number of rows filesort has found so far */

 > index 53c236bbce7..c237fcd9ecc 100644
> > --- a/sql/item_func.h
> > +++ b/sql/item_func.h
> > @@ -2125,6 +2125,63 @@ class Item_func_rand :public Item_real_func
> >  };
> >
> >
> > +class Item_func_rownum :public Item_longlong_func
> > +{
> > +  /*
> > +This points to a variable that contains the number of rows
> > +accpted so far in the result set
> > +  */
> > +  ha_rows *accepted_rows;
> > +  SELECT_LEX *select;
> > +public:
> > +  Item_func_rownum(THD *thd);
> > +  longlong val_int() override;
> > +  LEX_CSTRING func_name_cstring() const override
> > +  {
> > +static LEX_CSTRING name= {STRING_WITH_LEN("rownum") };
> > +return name;
> > +  }
> > +  enum Functype functype() const override { return ROWNUM_FUNC; }
> > +  void update_used_tables() override {}
> > +  bool const_item() const override { return 0; }
> > +  void fix_after_optimize(THD *thd) override;
> > +  bool fix_fields(THD* thd, Item **ref) override;
> > +  bool fix_length_and_dec() override
> > +  {
> > +unsigned_flag= 1;
> > +used_tables_cache= RAND_TABLE_BIT;
> > +const_item_cache=0;
> > +set_maybe_null();
>
> how can rownum be NULL?

In context where there are no rows.   like SET @A=rownum;

> > +return FALSE;
> > +  }
> > +  void cleanup() override
> > +  {
> > +Item_longlong_func::cleanup();
> > +/* Ensure we don't point to freed memory */
> > +accepted_rows= 0;
> > +  }
&g

Re: [Maria-developers] d221068ae8b: Change CHARSET_INFO character set and collaction names to LEX_CSTRING

[Michael Widenius]

Hi!

On Thu, Apr 1, 2021 at 4:19 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Apr 01, Michael Widenius wrote:
> > revision-id: d221068ae8b (mariadb-10.5.2-540-gd221068ae8b)
> > parent(s): 896c3b0a00a
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 19:13:42 +0200
> > message:
> >
> > Change CHARSET_INFO character set and collaction names to LEX_CSTRING
> >
> > This change removed 68 strlen() calls from the code.
> >
> > The following renames was to ensure we don't use the old names
> > when merging code from earlier releases, as using the new variables
> > for print function could result in crashes:
> > - charset->csname renamed to charset->cs_name
> > - charset->name renamed to charset->col_name
>
> Two comments:
>
> 1. please, use coll_name.
>
> in our codebase "cs" conventionally means "charset", "col"
> is usually used for "column", "coll" - for "collation"

I can do that, but that will cause changes also in columnstore.
Will try to fix...

> 2. You changed the client api, as can be seen from
>
> > diff --git a/client/mysql.cc b/client/mysql.cc
> > index e6c7d0f091c..8c64d01539d 100644
> > --- a/client/mysql.cc
> > +++ b/client/mysql.cc
> > @@ -4719,9 +4719,8 @@ sql_real_connect(char *host,char *database,char 
> > *user,char *password,
> >  return -1;   // Retryable
> >}
> >
> > -  charset_info= get_charset_by_name(mysql.charset->name, MYF(0));
> > -
> > -
> > +  charset_info= 
> > get_charset_by_name(IF_EMBEDDED(mysql.charset->col_name.str,
> > +mysql.charset->name), 
> > MYF(0));
> >connected=1;
> >  #ifndef EMBEDDED_LIBRARY
> >mysql_options(, MYSQL_OPT_RECONNECT, _info_flag);
>
> Luckily, C/C is a separate project now, so most clients won't be
> affected. But anything linking with embedded might be.

Yes, I know the effect of this. However I did check with you that this
is ok to do
for 10.6.

This only affects clients that access the collation name directly,
which one should
not really do.

Regards,
Monty

___
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] e3971006cca: Give a readable error in mtr if resolve_at_variable fails

[Michael Widenius]

Hi!

On Mon, Apr 5, 2021 at 5:27 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 05, Michael Widenius wrote:
> > revision-id: e3971006cca (mariadb-10.5.2-547-ge3971006cca)
> > parent(s): ae009435e72
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 19:25:09 +0200
> > message:
> >
> > Give a readable error in mtr if resolve_at_variable fails
>
> This change is wrong. Clearly, the || '' part was intentional.
> Check the history, it was added in the commit
>
>   commit 897b51db434
>   Author: Sergei Golubchik 
>   Date:   Thu Sep 10 12:12:47 2020 +0200
>
>   make S3 tests to run when S3 is statically linked
>
>   * use the environment variable HA_S3_SO, not a literal ha_s3 in cnf 
> files
>   * make ConfigFactory to support empty option values
>   * update no_s3.result after MDEV-11412
>
> so, apparently, the idea was that you should be able to write
>
>   plugin-load-add=$HA_S3_SO
>
> and it will *not* fail when $HA_S3_SO is not defined, because S3 engine
> is statically linked in.

I did not do this for the HA_S3 engine.  I got under some conditions
that I cannot remember now, warnings when running mtr about accessing
not allocated
variables.  The patch helped me to find and fix those.

___
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] 405a89353b2: Don't reset StringBuffers in loops when not needed

[Michael Widenius]

Hi!

On Sat, Apr 3, 2021 at 12:54 AM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> On Apr 02, Michael Widenius wrote:
> > revision-id: 405a89353b2 (mariadb-10.5.2-542-g405a89353b2)
> > parent(s): 2dd8d472fcc
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 19:25:08 +0200
> > message:
> >
> > Don't reset StringBuffers in loops when not needed
> >
> > - Moved out creating StringBuffers in loops and instead create them
> >   outside and just reset the buffer if it was not allocated (to avoid
> >   a possible malloc/free for every entry)
> >
> > diff --git a/sql/sql_string.h b/sql/sql_string.h
> > index 29d01779b71..8df50ac5e66 100644
> > --- a/sql/sql_string.h
> > +++ b/sql/sql_string.h
> > @@ -1045,6 +1045,12 @@ class StringBuffer : public String
> >{
> >  length(0);
> >}
> > +  void set_buffer_if_not_allocated(CHARSET_INFO *cs)
> > +  {
> > +if (is_alloced())
> > +  Static_binary_string::set(buff, buff_sz);
>
> it seems to be doing exactly the opposite of what the name suggests.
> It sets the buffer *if allocated*.
>
> Also, it looks logical to do length(0) here, so that this method
> would reset the StringBuffer to its initial state.

What I have in my tree:
  /*
This is used to set a new buffer for String.
However if the String already has an allocated buffer, it will
keep that one.
It's not to be used to set the value or length of the string.
  */
  inline void set_buffer_if_not_allocated(char *str, size_t arg_length)
  {
if (!alloced)
{
  /*
Following should really be str_length= 0, but some code may
depend on that the String length is same as buffer length.
  */
  Ptr= str;
  str_length= Alloced_length= (uint32) arg_length;
}
  }

___
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] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

[Michael Widenius]

Hi!

On Thu, Apr 1, 2021 at 12:08 AM Sergei Golubchik  wrote:
>
> Hi, Monty!



> Hmm, I don't understand. The difference between
>
>   String a("Hello", 5);
>
> and
>
>   char hello[5];
>   String a(buf, 5);
>
> is that in the first case the argument is const char*. And in that case
> Alloced_length=0 and in that case you can expect the string to be \0
> terminated.

There is no guarantee at all that when one has allocated length == 0 that
the string is 0 terminated.

I tried your suggestion and it did not work, we got
a lot of calls to malloc() that never gets freed.

Here is an example:

diff --git a/sql/sql_string.h b/sql/sql_string.h
index aa773ba396f..80c672c0137 100644
--- a/sql/sql_string.h
+++ b/sql/sql_string.h
@@ -609,7 +609,7 @@ class Binary_string: public Sql_alloc
   This is to handle the case of String("Hello",5) and
   String("hello",5) efficently.
 */
-if (unlikely(!alloced && !Ptr[str_length]))
+if (unlikely(!Alloced_length && !Ptr[str_length]))
   return Ptr;
 if (str_length < Alloced_length)

mtr main.grant
main.grant   [ pass ]943
***Warnings generated in error logs during shutdown after running
tests: main.grant
Warning: Memory not freed: 256

I traced it to this code, that is also in 10.5:
  String(const char *str, size_t len, CHARSET_INFO *cs)
   :Charset(cs),
Binary_string((char *) str, len)
  { }
Removing the const caused Alloced_length to be set, which caused
the extra allocations.

After fixing this I was able to run the full test suite without issues.
Will try again with valgrind today.

> In your example that we do "a lot", the first argument is const char*,
> so String will have Alloced_length==0 and my suggestion will work
> exactly as planned, it'll use a \0 terminated fast path.

Yes, we have a lot, but we have also calls with not const that we have
to consider.

The current assumptions when using String and wanting a \0 terminated
string are:
- If you let String allocate the memory, c_ptr() will always be safe and fast.
- If you use your own buffer that you fill in and you assign it to a
String and you
  expect to use it for functions that expects \0 terminated strings, you should
  add the extra \0 yourself if you want c_ptr() to be fast and safe.
If not, then
  use c_ptr_safe().

The thing to consider is which option is better: to use 'alloced' or
Alloced_length.
alloced is a weaker test and will cover more cases and will this cause fewer
memory allocations. Alloced_length is probably a bit safer. In practice it looks
like with current code both works (I have in the past tested alloced with
valgrind and there has been no issues.

Regards,
Monty

___
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] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

[Michael Widenius]

Hi!

On Thu, Apr 1, 2021 at 12:03 AM Sergei Golubchik  wrote:


> > > > -  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > > > +  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> > > >part_def->part_id, part_def->is_subpart,
> > > > -  part_name));
> > > > +  length, part_name));
> > >
> > > These two changes contradict each other. Either part_name must be
> > > \0-terminated, and then you don't need to specify a length in printf.
> > > Or it is not \0-terminated and you must specify a length.
> >
> > part_name is not null terminated and that is why I had to add .* and length.
> > What is the contradiction ?
>
> You've cut too much from my email. "contradiction" referred to the
> change just above this one, it was:
>
> > -  @param part_name  Partition name to match.
> > +  @param part_name  Partition name to match.  Must be \0 terminated!
>
> so, please, decide what it is. Either the comment is wrong or the code
> is redundant.

Neither. The comment is right. part_name must at this point in time be
null terminated, but
may not have to be that in the future.

The printf string for DBUG_PRINT is correct.  It uses the length,
which avoids a strlen() inside
sprintf() and is also future proof.

The only reason that part_name needs to be null terminated is because
of this code:

  if (!part_def)
  {
my_error(ER_UNKNOWN_PARTITION, MYF(0), part_name, table->alias.c_ptr());
DBUG_RETURN(true);
  }
Whenever we fix the error message to use %.*s, then we can lift the
restrictions that
part_name needs to be null terminated.

Regards,
Monty

> > > > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, 
> > > > uint len, CHARSET_INFO *cs)
> > > >  // Shrink the buffer, but only if it is allocated on the heap.
> > > >  void Binary_string::shrink(size_t arg_length)
> > > >  {
> > > > -if (!is_alloced())
> > > > -return;
> > > > -if (ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > > > +  if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > > > +  {
> > > > +char *new_ptr;
> > > > +/* my_realloc() can't fail as new buffer is less than the original 
> > > > one */
> > > > +if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, 
> > > > arg_length,
> > >
> > > you don't need an if() because, as you wrote yourself "my_realloc()
> > > can't fail" here.
> >
> > Yes, this should be true, but one never knows with allocation libraries.
> > There is implementation of realloc that makes a copy and deletes the old 
> > one.
> > However I am not sure what happens if there is no place to make a copy.
> > Better safe than sorry.
>
> No. I've fixed this quite a while ago. my_realloc() can *never* return
> NULL when reducing a buffer size. System realloc(), indeed, can, I've
> researched that topic when making the change. But my_realloc() has a
> protection against it and it never returns NULL in this case.
> We have few places in the code that rely on it.

We also have code that relies on that my_malloc() never fails.
Note what this commit did was just an cleanup of the original code,
not trying to do any big changes of logic, except for c_ptr().

Anyway, I can remove the if here to keep you happy.

> > > > +{
> > > > +  Ptr[str_length]=0;
> > > > +  return Ptr;
> > > > +}
> > > > +(void) realloc(str_length+1);   /* This will add end 
> > > > \0 */
> > > >  return Ptr;
> > > >}
> > > > +  /* Use c_ptr() instead. This will be deleted soon, kept for 
> > > > compatiblity */
> > > >inline char *c_ptr_quick()
> > > >{
> > > > -if (Ptr && str_length < Alloced_length)
> > > > -  Ptr[str_length]=0;
> > > > -return Ptr;
> > > > +return c_ptr_safe();
> > >
> > > 1. why not to remove it now?
> > > 2. it's strange that you write "use c_ptr() instead" but you actually
> > >use c_ptr_safe() instead.
> >
> > What I meant is that the developer should instead use c_ptr().
> > I will make the message more clear.
> > I decided to call c_ptr_safe() here as this is the most safe
> > version to use.  In practice the new c_ptr() should be safe
> > for 99% of our code.
>
> Let's remove c_ptr_quick() now. Why keep it?

Not enough time to do it now. I rather work on fixing review comments.

Regards,
Monty

___
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] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

[Michael Widenius]

Hi!

Sorry, this was because gmail send it before I wanted to.
(I do not like that ctrl+return sends email as this causes problems
when one uses this a lot in slack!)


> > > --- a/sql/sql_string.h
> > > +++ b/sql/sql_string.h
> > > @@ -600,25 +600,34 @@ class Binary_string: public Static_binary_string
> > >
> > >inline char *c_ptr()
> > >{
> > > -DBUG_ASSERT(!alloced || !Ptr || !Alloced_length ||
> > > -(Alloced_length >= (str_length + 1)));
> > > -
> > > -if (!Ptr || Ptr[str_length])  // Should be safe
> > > -  (void) realloc(str_length);
> > > +if (unlikely(!Ptr))
> > > +  return (char*) "";
> > > +/*
> > > +  Here we assume that any buffer used to initalize String has
> > > +  an end \0 or have at least an accessable character at end.
> > > +  This is to handle the case of String("Hello",5) efficently.
> > > +*/
> > > +if (unlikely(!alloced && !Ptr[str_length]))
> > > +  return Ptr;
> >
> > No, this is not good. Note the difference between
> >
> >   String a("Hello", 5)
> >
> > and
> >
> >   char hello[5];
> >   String a(buf, 5);
> >
> > Your assumption should only apply to the first case, not to the second.
> > In  the first case alloced=Alloced_length=0, in the second case only
> > alloced=0 and Alloced_length=5. So in the if() above you need to look
> > at Alloced_length:
> >
> >   if (!Alloced_length && !Ptr[str_length])
> > return Ptr;

Unfortunately this does not work good with our code.

We have a lot of code that does things like this:
 String *new_str= new (thd->mem_root) String((const char*) name.str,
  name.length,
  system_charset_info)
Where string is 0 terminated.
With your proposed change, all of these will require a full realloc when using
c_ptr().

Regards,
Monty

___
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] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

[Michael Widenius]

On Wed, Mar 31, 2021 at 9:45 PM Michael Widenius
 wrote:
>
> Cut
> > > Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
> > >
> > > The proble was that hen using String::alloc() to allocate a string,
> >
> > "hen" ? after a minute of staring I realized that you probably meant "when".
> >
> > (also, "problem")
> >
> > > the String ensures that there is space for an extra NULL byte in the
> > > buffer and if not, reallocates the string. This is a problem with the
> > > String::set_int() that calls alloc(21), which forces
> > > an extra malloc/free to happen.
> > >
> > > - We do not anymore re-allocate String if alloc() is called with the
> > >   Allocated_length. This reduces number of malloc() allocations,
> > >   especially one big re-allocation in Protocol::send_result_Set_metadata()
> > >   for almost every query that produced a result to the connnected client.
> > > - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE
> > >   This can now be done as alloc() doesn't increase buffers if new length 
> > > is
> > >   not bigger than old one.
> > > - c_ptr() is redesigned to be safer (but a bit longer) than before.
> > > - Remove wrong usage of c_ptr_quick()
> > >   c_ptr_quick() where used in many cases to get the pointer to the used
> >
> > s/where/was/
> >
> > >   buffer, even when it didn't need to be \0 terminated. In this case
> > >   ptr() is a better substitute.
> > >   Another problem with c_ptr_quick() is that it didn't guarantee that
> > >   the string would be \0 terminated.
> > > - item_val_str(), an API function not used currently by the server,
> > >   now always returns a null terminated string (before it didn't always
> > >   do that).
> > > - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old
> > >   mixed usage of performance keys caused assert's when String buffers
> > >   where shrunk.
> > > - Binary_string::shrink() is simplifed
>
> Will fix the typos. Thanks!
>
> > > diff --git a/sql/item.cc b/sql/item.cc
> > > index f6f3e2720fa..a8a43c6266a 100644
> > > --- a/sql/item.cc
> > > +++ b/sql/item.cc
> > > @@ -4703,7 +4703,7 @@ Item *Item_param::value_clone_item(THD *thd)
> > >  return 0; // Should create Item_decimal. See MDEV-11361.
> > >case STRING_RESULT:
> > >  return new (mem_root) Item_string(thd, name,
> > > -  
> > > Lex_cstring(value.m_string.c_ptr_quick(),
> > > +  Lex_cstring(value.m_string.ptr(),
> >
> > Hmm, you said that LEX_CSTRING::str should always be \0-terminated.
> > If that's the case, then c_ptr() would be correct here, not ptr()
> >
> > >
> > > value.m_string.length()),
> > >value.m_string.charset(),
> > >collation.derivation,
>
> Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes
> build these
> up I do not want to depend on that for generic usage. For usage to
> printf() than we
> should trust that the developer knows what he is doing.
>
> However Item_string() will only use the exact length and never call
> printf, so here this is safe.
>
> > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
> 
>
> > > -  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > > +  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> > >part_def->part_id, part_def->is_subpart,
> > > -  part_name));
> > > +  length, part_name));
> >
> > These two changes contradict each other. Either part_name must be
> > \0-terminated, and then you don't need to specify a length in printf.
> > Or it is not \0-terminated and you must specify a length.
>
> part_name is not null terminated and that is why I had to add .* and length.
> What is the contradiction ?
>
> > > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > > index f2a0f55aec8..95a57017c53 100644
> > > --- a/sql/sql_string.cc
> > > +++ b/sql/sql_string.cc
> > > @@ -118,9 +121,14 @@ bool Binary_string::realloc_raw(size_t alloc_length)
> > >return FALSE;
> > >  }
> > >
> > > +
> &

Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

[Michael Widenius]

Cut
> > Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
> >
> > The proble was that hen using String::alloc() to allocate a string,
>
> "hen" ? after a minute of staring I realized that you probably meant "when".
>
> (also, "problem")
>
> > the String ensures that there is space for an extra NULL byte in the
> > buffer and if not, reallocates the string. This is a problem with the
> > String::set_int() that calls alloc(21), which forces
> > an extra malloc/free to happen.
> >
> > - We do not anymore re-allocate String if alloc() is called with the
> >   Allocated_length. This reduces number of malloc() allocations,
> >   especially one big re-allocation in Protocol::send_result_Set_metadata()
> >   for almost every query that produced a result to the connnected client.
> > - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE
> >   This can now be done as alloc() doesn't increase buffers if new length is
> >   not bigger than old one.
> > - c_ptr() is redesigned to be safer (but a bit longer) than before.
> > - Remove wrong usage of c_ptr_quick()
> >   c_ptr_quick() where used in many cases to get the pointer to the used
>
> s/where/was/
>
> >   buffer, even when it didn't need to be \0 terminated. In this case
> >   ptr() is a better substitute.
> >   Another problem with c_ptr_quick() is that it didn't guarantee that
> >   the string would be \0 terminated.
> > - item_val_str(), an API function not used currently by the server,
> >   now always returns a null terminated string (before it didn't always
> >   do that).
> > - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old
> >   mixed usage of performance keys caused assert's when String buffers
> >   where shrunk.
> > - Binary_string::shrink() is simplifed

Will fix the typos. Thanks!

> > diff --git a/sql/item.cc b/sql/item.cc
> > index f6f3e2720fa..a8a43c6266a 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -4703,7 +4703,7 @@ Item *Item_param::value_clone_item(THD *thd)
> >  return 0; // Should create Item_decimal. See MDEV-11361.
> >case STRING_RESULT:
> >  return new (mem_root) Item_string(thd, name,
> > -  
> > Lex_cstring(value.m_string.c_ptr_quick(),
> > +  Lex_cstring(value.m_string.ptr(),
>
> Hmm, you said that LEX_CSTRING::str should always be \0-terminated.
> If that's the case, then c_ptr() would be correct here, not ptr()
>
> >value.m_string.length()),
> >value.m_string.charset(),
> >collation.derivation,

Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes
build these
up I do not want to depend on that for generic usage. For usage to
printf() than we
should trust that the developer knows what he is doing.

However Item_string() will only use the exact length and never call
printf, so here this is safe.

> > diff --git a/sql/partition_info.cc b/sql/partition_info.cc


> > -  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > +  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> >part_def->part_id, part_def->is_subpart,
> > -  part_name));
> > +  length, part_name));
>
> These two changes contradict each other. Either part_name must be
> \0-terminated, and then you don't need to specify a length in printf.
> Or it is not \0-terminated and you must specify a length.

part_name is not null terminated and that is why I had to add .* and length.
What is the contradiction ?

> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index f2a0f55aec8..95a57017c53 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -118,9 +121,14 @@ bool Binary_string::realloc_raw(size_t alloc_length)
> >return FALSE;
> >  }
> >
> > +
> >  bool String::set_int(longlong num, bool unsigned_flag, CHARSET_INFO *cs)
> >  {
> > -  uint l=20*cs->mbmaxlen+1;
> > +  /*
> > +This allocates a few bytes extra in the unlikely case that 
> > cs->mb_maxlen
> > +> 1, but we can live with that
>
> dunno, it seems that utf8 is the *likely* case and it's getting more and
> more likely with the time,

Not for integers, as we often use binary strings for these.

> > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, uint 
> > len, CHARSET_INFO *cs)
> >  // Shrink the buffer, but only if it is allocated on the heap.
> >  void Binary_string::shrink(size_t arg_length)
> >  {
> > -if (!is_alloced())
> > -return;
> > -if (ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > +  if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > +  {
> > +char *new_ptr;
> > +/* my_realloc() can't fail as new buffer is less than the original one 
> > */
> > +if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, 
> > arg_length,
>
> you 

Re: [Maria-developers] 1bbc852ef71: Changed field_index to use field_index_t instead of uint16

[Michael Widenius]

Hi!

On Mon, Mar 29, 2021 at 3:55 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Mar 29, Michael Widenius wrote:
> > revision-id: 1bbc852ef71 (mariadb-10.5.2-525-g1bbc852ef71)
> > parent(s): 039f4a054bb
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 14:31:54 +0200
> > message:
> >
> > Changed field_index to use field_index_t instead of uint16
>
> Just curious. Why?

Mainly because it makes the code easier to read:

 find_field_in_table(THD *thd, TABLE *table, const char *name, size_t length,
-bool allow_rowid, uint16 *cached_field_index_ptr)
+bool allow_rowid, field_index_t *cached_field_index_ptr)
...
-static inline uint16 read_frm_fieldno(const uchar *data)
+static inline field_index_t read_frm_fieldno(const uchar *data)

With the new version, one can easier understand what the function expects
as arguments and what the function returns.

I had also seen different types used for field numbers in some
functions/classes and that also did bother me a bit so I decided to
fix this once and for all.

For example, in 10.5 we have:
item.h:  uint cached_field_index;

> > diff --git a/sql/unireg.cc b/sql/unireg.cc
> > index 51dd1fb6e8c..f1a0c5c5269 100644
> > --- a/sql/unireg.cc
> > +++ b/sql/unireg.cc
> > @@ -146,8 +146,8 @@ get_fieldno_by_name(HA_CREATE_INFO *create_info, 
> > List _fiel
> >{
> >  if (field_name.streq(sql_field->field_name))
> >  {
> > -  DBUG_ASSERT(field_no <= uint16(~0U));
> > +  DBUG_ASSERT(field_no < NO_CACHED_FIELD_INDEX);
>
> this look suspicios. NO_CACHED_FIELD_INDEX is ((uint)(-1))
> which is always greater than anything you can put into field_index_t.

We have code that uses NO_CACHED_FIELD_INDEX in field_no
to indicate that there was no such field.

I looked at current code and we have:
#define NO_CACHED_FIELD_INDEX ((uint16) ~0)

> You want to ensure that field_no is valid when casted into
> field_index_t. I'd suggest to redefine NO_CACHED_FIELD_INDEX as
>
> ((field_index_t)~0U)

I can redefine as that.

> > -  return uint16(field_no);
> > +  return (field_index_t) field_no;
>
> why not to declare field_no to be field_index_t? then you won't need a
> cast.

I have now fixed the above and and also changed type for the function
and also updated this in table.cc:
-static uint find_field(Field **fields, uchar *record, uint start, uint length);
+static field_index_t find_field(Field **fields, uchar *record, uint start,
+uint length);

I missed some of these cases as I did not get warnings from these from
the compiler...

Regards,
Monty

___
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] 1d577194654: Revert MDEV-16592 "Change Item::with_sum_func to a virtual method"

[Michael Widenius]

Hi!

On Mon, Mar 29, 2021 at 4:20 PM Sergei Golubchik  wrote:
>
> Hi, Michael!

Only people who do not know me calls me Michael...
Who are you?

> > Revert MDEV-16592 "Change Item::with_sum_func to a virtual method"
> >
> > diff --git a/sql/item.cc b/sql/item.cc
> > index e5be8699d80..e0ba4ff2c89 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -975,9 +975,7 @@ class Item :public Value_source,
> >void share_name_with(const Item *item)
> >{
> >  name= item->name;
> > -common_flags= static_cast
> > - ((common_flags & ~IS_AUTO_GENERATED_NAME) |
> > -  (item->common_flags & IS_AUTO_GENERATED_NAME));
> > +is_autogenerated_name= item->is_autogenerated_name;
>
> this, apparently, should be in the commit 039f4a054bb
> "Removed Item::common_flags and replaced it with bit fields"

Have now done this.
I assume you understand that moving a commit like this between version
takes about 10-30 min and have no affect on the end result.

I am happy to do that if you think it is important, but doing it over
and over again
is a bit waste of time that I could use for more critical items...

Regards,
Monty

___
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] f8901333a82: Add support for minimum field width for strings to my_vsnprintf()

[Michael Widenius]

Hi!

On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Mar 29, Michael Widenius wrote:
> > revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82)
> > parent(s): a194c5cfd41
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 15:53:35 +0200
> > message:
> >
> > Add support for minimum field width for strings to my_vsnprintf()
> >
> > diff --git a/mysql-test/main/almost_full.result 
> > b/mysql-test/main/almost_full.result
> > index b2d7092aa51..4b5c8417412 100644
> > --- a/mysql-test/main/almost_full.result
> > +++ b/mysql-test/main/almost_full.result
> > @@ -7,24 +7,24 @@ INSERT INTO t1 SET b=repeat('a',600);
> >  ERROR HY000: The table 't1' is full
> >  CHECK TABLE t1 EXTENDED;
> >  TableOp  Msg_typeMsg_text
> > -test.t1  check   warning Datafile is almost full, 65448 of 65534 used
> > +test.t1  check   warning Datafile is almost full,  65448 of  
> > 65534 used
>
> This happens because mi_check.c has
>
>mi_check_print_warning(param, "Datafile is almost full, %10s of %10s used",
>   ...

Yes, and that is exactly how I want it.  It is same output we have for
aria_check and
myisamchk and there it makes even more sense.

> could you please remove explicit width specification from these
> messages? this looks rather ugly in warnings. This applies both to
> MyISAM and Aria.

No, see above.

This is however not related to the patch at all.  We did not support
minimum width before
and this is something that is sometimes needed.

> >  test.t1  check   status  OK
> >  UPDATE t1 SET b=repeat('a', 800) where a=10;
> >  ERROR HY000: The table 't1' is full
> > diff --git a/mysql-test/main/grant4.result b/mysql-test/main/grant4.result
> > index 981bbdeddf5..ba64010f1bb 100644
> > --- a/mysql-test/main/grant4.result
> > +++ b/mysql-test/main/grant4.result
> > @@ -187,8 +187,8 @@ connection con1;
> >  check table mysqltest_db1.t1;
> >  TableOp  Msg_typeMsg_text
> >  mysqltest_db1.t1 check   warning 1 client is using or hasn't closed 
> > the table properly
> > -mysqltest_db1.t1 check   error   Size of indexfile is: 1024
> > Should be: 2048
> > -mysqltest_db1.t1 check   warning Size of datafile is: 14   Should 
> > be: 7
> > +mysqltest_db1.t1 check   error   Size of indexfile is: 1024
> > Should be: 2048
> > +mysqltest_db1.t1 check   warning Size of datafile is:14   
> > Should be: 7
>
> Same here.

It is exactly how it i want it.  One can get +100 of these errors and
it is hopeless to read
them if things are not aligned.



> > diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c
> > index a2e3f9b738d..1621db2e412 100644
> > --- a/strings/my_vsnprintf.c
> > +++ b/strings/my_vsnprintf.c
> > @@ -224,12 +224,27 @@ static char *backtick_string(CHARSET_INFO *cs, char 
> > *to, const char *end,
> >  */
> >
> >  static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end,
> > - size_t width, char *par, uint print_type,
> > - my_bool nice_cut)
> > + longlong length_arg, size_t width, char *par,
> > + uint print_type, my_bool nice_cut)
> >  {
> >int well_formed_error;
> >uint dots= 0;
> >size_t plen, left_len= (size_t) (end - to) + 1, slen=0;
> > +  my_bool left_fill= 1;
> > +  size_t length;
> > +
> > +  /*
> > +The sign of the length argument specific the string should be right
> > +or left adjusted
> > +  */
> > +  if (length_arg < 0)
>
> You're trying to support left alignment, but you have yourself added a
> test that shows that it doesn't work.

Yes, as I did not need left alignment and if someone wants to add it,
they are welcome to
do that.

> So this code is dead, never used. See below.

It is used if someone (wrongly?) uses a negative argument.
If the code would not be there, we would probably get
a crash. Would you prefer that instead of ignoring it like now?

> > +  {
> > +length= -length_arg;
> > +left_fill= 0;
> > +  }
> > +  else
> > +length= (size_t) length_arg;
> > +
> >if (!par)
> >  par = (char*) "(null)";
> >
> > @@ -693,7 +720,8 @@ size_t my_vsnprintf_ex(CHARSET_INFO *cs, char *to, 
> > size_t n,
> >  if (*fmt == 's' || *fmt == 'T')  /* String parameter */
> >  {
> >

Re: [Maria-developers] 25fe7dbad5d: Reduce usage of strlen()

[Michael Widenius]

Hi!

On Wed, Mar 31, 2021 at 5:18 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Mar 31, Michael Widenius wrote:
> > commit 25fe7dbad5d
> > Author: Michael Widenius 
> > Date:   Wed Aug 12 20:29:55 2020 +0300
> >
> >Reduce usage of strlen()
>
> I'll only mention actual mistakes, no style comments or suggestions or
> question, just want to get it over with.
>
> > diff --git a/sql/item.h b/sql/item.h
> > index 9a6d0b4abfe..d6ff10bf3ba 100644
> > --- a/sql/item.h
> > +++ b/sql/item.h
> > @@ -1093,7 +1093,7 @@ class Item :public Value_source,
> >void share_name_with(const Item *item)
> >{
> >  name= item->name;
> > -copy_flags(item, item_base_t::IS_AUTOGENERATED_NAME);
> > +copy_flags(item, item_base_t::IS_EXPLICIT_NAME);
>
> belongs to a previous commit

Sorry about that, must have happened when I did a fixup trying to keep
things clean.
How can I move most easily to the right commit.
Do a rebase and do "edit" for the previous commit, do the change by
hand and continue the rebase?
Will try that.

> > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc
> > index 262481d019b..a9e008b0b92 100644
> > --- a/sql/item_windowfunc.cc
> > +++ b/sql/item_windowfunc.cc
> > @@ -99,13 +99,15 @@ Item_window_func::fix_fields(THD *thd, Item **ref)
> >
> >if (window_spec->window_frame && is_frame_prohibited())
> >{
> > -my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0), 
> > window_func()->func_name());
> > +my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0),
> > + window_func()->func_name());
>
> This should be func_name().str, otherwise it'll crash or
> print garbage on Windows. Same below, in a couple of places.

Nope, this works thanks to this:

  inline const char *func_name() const
  { return (char*) func_name_cstring().str; }

So func_name() can be used (instead of the old variable func_name)
when one wants to have the a char* pointer.
This was convenient to have for future merges and make the changes in
the commit easier to read.

Regards,
Monty

___
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] dbb62d20f36: Fixes that enables my_new.cc (new wrapper using my_malloc)

[Michael Widenius]

Hi!

On Sun, Mar 28, 2021 at 11:18 PM Sergei Golubchik  wrote:
>
> Hi, Monty!



> Here's a simplified patch that works (I've applied it, compiled, run
> the test suite, and verified in a debugger that new() from my_new.cc is
> used):

One which systems did you test it?

Also on older systems that does not provide  ?

Please always provide a diff on top of my diff, so that I can see what
changes you propose,
or comment inline what you want to change.
I did not find the diff you provide usable (without too much work on
my side comparing diffs.

Anyway, I do feel my version is safer as it takes into account if
HAVE_CXX_NEW exists or not,
so I will stick with it.

Regards,
Monty

___
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] dbb62d20f36: Fixes that enables my_new.cc (new wrapper using my_malloc)

[Michael Widenius]

Hi!

On Sun, Mar 28, 2021 at 3:00 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Mar 28, Michael Widenius wrote:
> > revision-id: dbb62d20f36 (mariadb-10.5.2-518-gdbb62d20f36)
> > parent(s): 0c37211c2a2
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 14:31:53 +0200
> > message:
> >
> > Fixes that enables my_new.cc (new wrapper using my_malloc)
> >
> > This is not enabled by default as there are leaks in the
> > server that needs to be fixed first. One can compile
> > with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the
> > memory leaks from 'new'
>
> The change in my_new.cc is ok. The change in my_global.h and
> CMakeLists.txt is confusing and redundant.
>
> You unconditionaly define USE_MYSYS_NEW then conditionally undefine it
> again? Everything already worked just fine. By default cxx new is used,
> if one wants to use mysys version, it's done with
>
>   -DHAVE_CXX_NEW=0
>
> this works for me in 10.5 and fails to compile my_new.cc, as it should.

The problem is that one cannot and should never use HAVE_CXX_NEW
as this is something that is found by cmake.
In the future USE_MYSYS_NEW should be on by default and one would have to
use -DUSE_MYSYS_NEW=0 if one does not want to use it.

We cannot do that yet, which is why I added REALLY_USE_MYSYS_NEW
temporarily to allow one to test, debug and fix MYSYS_NEW.

One cannot fix this by reseting HAVE_CXX_NEW, because my_new.cc
requires this to be defined
if the platform supports it!  Not doing that gave me compiler errors.

> Please, only keed your my_new.cc changes and say in the commit message
> "One can compile with -DHAVE_CXX_NEW=0 to find memory leaks from 'new'"

Does not work, see above.

> As for -DSF_REMEMBER_FRAMES=14, perhaps it's better to do it
> automatically? Like
>
>   #ifdef USE_MYSYS_NEW
>   #define SF_REMEMBER_FRAMES 14
>   #else
>   #define SF_REMEMBER_FRAMES 8
>   #endif
>
> or may be even just always make it 14.
> It should be easy to enable this feature, right?

yes, this is a good idea, will do.

> > --- a/mysys/my_static.c
> > +++ b/mysys/my_static.c
> > @@ -48,6 +48,7 @@ PSI_memory_key key_memory_my_err_head;
> >  PSI_memory_key key_memory_my_file_info;
> >  PSI_memory_key key_memory_pack_frm;
> >  PSI_memory_key key_memory_charsets;
> > +PSI_memory_key key_memory_new;
>
> where do you initialize it?
> I'd suggest statically, like
>
>   PSI_memory_key key_memory_new= PSI_INSTRUMENT_MEM;
>
> >
> >  #ifdef _WIN32
> >  PSI_memory_key key_memory_win_SECURITY_ATTRIBUTES;
>
> 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] 0c37211c2a2: Fixed my_addr_resolve

[Michael Widenius]

Hi!

On Sun, Mar 28, 2021 at 2:54 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Mar 28, Michael Widenius wrote:
> > revision-id: 0c37211c2a2 (mariadb-10.5.2-517-g0c37211c2a2)
> > parent(s): 555c4470130
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2021-03-24 14:31:53 +0200
> > message:
> >
> > Fixed my_addr_resolve
> >
> > At least on openSUSE Leap 15.1 one should not use the dli_fbase offset
> > provided by dladdr() for addresses in the main program.
> > Without this patch the stack traces from sf_report_leaked_memory()
> > are not usable.
>
> This is very strange. First, the whole purpose of this code is to use
> dli_fbase offset for the main program. That's why it was added in the
> first place. PIE binaries can be loaded at any address in memory and the
> address you see with addr2line or nm or readelf is not the same address
> you'll see in gdb or in a strack trace. We have to subtract the base
> address first.
>
> But may be openSUSE is different?
> So, I built on openSUSE Leap and the stack trace was perfectly usable
> without your patch. I tried both `kill -ABRT` and an artificially
> introduced memory leak.
>
> So I suspect there was some misunderstanding and the problem was
> somewhere else (I cannot know what, of course, but just as an example -
> the build was bad and recompilation fixed it).

This was checked extensively by both me and Vicentiu (both looking at
the assembler and the examining the output).
Without the patch I get constantly wrong stack traces for the main program.
We also tried to find exactly how dli_fbase exactly should be used on
the internet, but could not find any
good documentation for it.

The simple fact is that I am using this patch on my 10.6 tree and
stack traces are good with it and has been
for several months, so it is impossible that this is related to a bad
binary or bad build but something that is
required on my system

Regards,
Monty

___
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] 12d5c4f2813: Fixed problems with S3 after "DROP TABLE FORCE" changes

[Michael Widenius]

Hi!

On Sun, Oct 18, 2020 at 3:38 PM Sergei Golubchik  wrote:
>
> Hi, Monty!

> > Fixed problems with S3 after "DROP TABLE FORCE" changes
> >
> > MDEV-23691 S3 storage engine: delayed slave can drop the table
>
> please put the MDEV on the first line of the commit comment

I prefer to have the first row readable and understandable for all
possible readers, which the MDEV
doesn't do.  I can move the MDEV to the first row in this case, but if
the commit touches
several MDEV's, I will list these separately and keep the first row as
a summary of what the commit is for.

> > This commit also fixes all failing replication S3 tests.
> >
> > A slave is delayed if it is trying to execute replicated queries on a
> > table that is already converted to S3 by the master.
>
> What do you mean, a slave is delayed?

The above explains it, as those the MDEV.
It means that the slave is not up to date with the master (either
intentionally with stop slave or because the slave still has local
not-s3 tables as it has not yet seen ALTER TABLE ... engine=s3).

The assumption is also that anyone reading the commit knows what
'slave' and 'master' and 'delayed slave' means



> > diff --git a/sql/log.cc b/sql/log.cc
> > index 2a887a68606..9c656387e90 100644
> > --- a/sql/log.cc
> > +++ b/sql/log.cc
> > @@ -2151,6 +2151,12 @@ static int binlog_commit(handlerton *hton, THD *thd, 
> > bool all)
> >
> >  DBUG_RETURN(0);
> >}
> > +  /*
> > +This is true if we are doing an alter table that is replicated as
> > +CREATE TABLE ... SELECT
> > +  */
> > +  if (thd->variables.option_bits & OPTION_BIN_COMMIT_OFF)
> > +DBUG_RETURN(0);
>
> why binlog_commit is even called here?

Binlog_drop_table() and binlog_create_table() calls binlog_query(),
which always does a commit
for any query events.


> > +The following is needed if the query was "ignored" by a shared 
> > distributed
> > +engine, in which case decide_logging_format() will set the binlog 
> > filter
> > +  */
> > +  reset_binlog_local_stmt_filter();
> > +  clear_binlog_local_stmt_filter();
> > +  return binlog_query(THD::STMT_QUERY_TYPE, query(), query_length(),
> > +  /* is_trans */ FALSE,
> > +  /* direct */   FALSE,
> > +  /* suppress_use */ FALSE,
> > +  /* Error */0) > 0;
> > +}

> you created a generally looking function with a rather special
> behavior. Please rename it to make sure it's not a general purpose
> helper. For example, call it binlog_current_query_unfiltered() or
> force_binlog_current_query(). And the function comment should say
>
>   Force-binlog current query as a statement ignoring the binlog filter setting

First a note. The function can be used anywhere where one would binlog
the current query.
The ..._local_stmt_filter() functions are only there to force the
query to be logged statement
based even if decide_logging_format(), used by some statement, would
have wanted to
do things differently.

I have updated the function comment.
I am not sure how many will understand the word 'unfiltered'
I have expanded the function comment to explain this in more detail and also
renamed the function binlog_current_query_unfiltered().

> >  void
> >  THD::wait_for_wakeup_ready()
> >  {
> > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> > index 4ad4c478937..5f59e064a35 100644
> > --- a/sql/sql_insert.cc
> > +++ b/sql/sql_insert.cc
> > @@ -724,6 +724,7 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list,
> >Item *unused_conds= 0;
> >DBUG_ENTER("mysql_insert");
> >
> > +  bzero((char*) ,sizeof(info));
>
> why did you move it up?

Proper place to have it at start. Not directly related to this patch.


> >create_explain_query(thd->lex, thd->mem_root);
> >/*
> >  Upgrade lock type if the requested lock is incompatible with
> > @@ -764,16 +765,27 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list,
> >  DBUG_RETURN(TRUE);
> >value_count= values->elements;
> >
> > -  if (mysql_prepare_insert(thd, table_list, table, fields, values,
> > -   update_fields, update_values, duplic,
> > -   _conds, FALSE))
> > +  if ((res= mysql_prepare_insert(thd, table_list, table, fields, values,
> > + update_fields, update_values, duplic,
> > + _conds, FALSE)))
> > +  {
> > +retval= thd->is_error();
> > +if (res < 0)
> > +{
> > +  /*
> > +Insert should be ignored but we have to log the query in statement
> > +format in the binary log
> > +  */
> > +  res= thd->binlog_current_query();
>
> abort label doesn't look at `res` it returns `retval`.

Yes, this is correct.  This is why 'retval' is set to is_error() above.
res is only used to know if the binlog

> So I don't think you need `retval= thd->is_error()` at all,
> and instead you need

>if (res < 0)
>  retval= 

Re: [Maria-developers] 21eb8969ce9: Improved storage size for Item, Field and some other classes

[Michael Widenius]

Hi!

On Fri, Sep 11, 2020 at 1:10 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> Looks ok, but again it doesn't seem you've squashed intermediate commits
> as you said you did.

I have squashed everything that makes sense.
I did leave commits that shows me trying with bit fields and then moving to
flags as these can be useful for anyone following the development process.
It also allows one to checkout the bitfield code and run tests with it.

> Bit fields and non-existent commits in columnstore - it's is clearly an
> intermediate work-in-progress state, all fixed in your later commits.

For columnstore I added a commit on top to allow columnstore to compile,
yes. That was the best way to get this work done.
As soon I have pushed, I hope the columnstore team will take the
patches and add it to
standard columnstore...


Regards,
Monty

___
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] 2564334b4c1: Backported setting of transcation.on=1 in THD::reset_for_reuse()

[Michael Widenius]

Hi!

On Fri, Sep 18, 2020 at 1:54 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> > diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> > index 7327f270c33..8f6356b15c7 100644
> > --- a/sql/sql_class.cc
> > +++ b/sql/sql_class.cc
> > @@ -1640,6 +1640,7 @@ void THD::reset_for_reuse()
> >abort_on_warning= 0;
> >free_connection_done= 0;
> >m_command= COM_CONNECT;
> > +  transaction.on= 1;
> >  #if defined(ENABLED_PROFILING)
> >profiling.reset();
> >  #endif
>
> This looks risky, to change transaction.on at some random point in time.

This is only for change user (when there can't be any transactions
around) or when reusing THD for a new connection.
Before we left it at a random value. Better to ensure it's always set.

> I understand that it's not a random point and it should be actually safe
> here. But perhaps you can add an assert to document that it is safe?

Assert for what?  The THD is reset from a random state to its initial
state here.
It's quite hard to test that 'is THD valid for all cases' and is not really part
of this patch.

> Like, transaction.on should be already 1 or there should be no active
> transaction.

Wrong place to test this.  Any test for 'active transactions' should be done
when THD is removed from reuse.

Regards,
Monty

___
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] fc07306120f: MDEV-23586 Mariabackup: GTID saved for replication in 10.4.14 is wrong

[Michael Widenius]

Hi!

On Fri, Sep 25, 2020 at 12:11 PM Sergei Golubchik  wrote:
>
> Hi, Monty!
>
> Just a couple of minor comments
>
> Note, the below is from the combined diff of this commit and your
> earlier MDEV-21953 commit, so you can see some lines here that are not
> part of the fc07306120f.
>
> > diff --git a/sql/handler.cc b/sql/handler.cc
> > index cc7eedd18f2..402db310ed6 100644
> > --- a/sql/handler.cc
> > +++ b/sql/handler.cc
> > @@ -133,6 +133,23 @@ static plugin_ref ha_default_tmp_plugin(THD *thd)
> >return ha_default_plugin(thd);
> >  }
> >
> > +#if defined(WITH_ARIA_STORAGE_ENGINE) && MYSQL_VERSION_ID < 100500
> > +void ha_maria_implicit_commit(THD *thd, bool new_trn)
> > +{
> > +  if (ha_maria::has_active_transaction(thd))
> > +  {
> > +int error;
> > +MDL_request mdl_request;
> > +mdl_request.init(MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, 
> > MDL_EXPLICIT);
> > +error= thd->mdl_context.acquire_lock(_request,
> > + thd->variables.lock_wait_timeout);
> > +ha_maria::implicit_commit(thd, new_trn);
> > +if (!error)
> > +  thd->mdl_context.release_lock(mdl_request.ticket);
>
> This looks rather fishy. You commit in aria unconditionally?
> whether you got the lock or not?

Yes, that is true. As Aria doesn't have rollback, we have always to commit.
That was true even in the original code, in which the aria commit was
done very early in ha_commit_trans().

In 10.5 this will be handled by the Aria rollback code (which does a
commit with a warning and we don't need the commit call above),
but in 10.4 we don't have that option.



> > -commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool 
> > is_real_trans)
> > +commit_one_phase_2(THD *thd, bool all, THD_TRANS *trans, bool 
> > is_real_trans,
> > +   bool rw_trans)
>
> rw_trans seems to be unused here
> (also in ha_commit_one_phase())

I originally decided to leave rw_trans as a not used parameter, as I
think that commit_one_phase2()
could later benefit of that. However I agree it's better to remove it
for now and add it back again
when it will be used.

> >  {
> >int error= 0;
> >uint count= 0;
> > diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> > index 40e606425c5..71b447fb920 100644
> > --- a/sql/sql_class.cc
> > +++ b/sql/sql_class.cc
> > @@ -1383,7 +1384,11 @@ void THD::update_all_stats()
> >  void THD::init_for_queries()
> >  {
> >set_time();
> > -  ha_enable_transaction(this,TRUE);
> > +  /*
> > +We don't need to call ha_enable_transaction() as we can't have
> > +any active transactions that has to be commited
> > +  */
> > +  transaction.on= TRUE;
>
> I just commented on the similar code in another commit of yours.
> don't just say "can't have any active transactions",
> add an assert instead, please.

I don't really know how to check that there are no active transactions.
The reason I think this is safe is that this function is called once
before doing any handler calls or starting any transactions.
The reason for the comment was mostly to make it clear why I removed
the original ha_enable_transaction() code
Do we really need an assert for a function that is called once during
startup of a thread?

Regards,
Monty

___
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] On bb-10.6-monty work

[Michael Widenius]

Hi!

On Fri, Sep 11, 2020 at 8:11 AM Nikita Malyavin
 wrote:



> > -static void _ma_check_print(HA_CHECK *param, const char* msg_type,
> > +static void _ma_check_print(HA_CHECK *param, const LEX_CSTRING *msg_type,
> >  const char *msgbuf)
>
> I would also suggest passing LEX_CSTRING by value. Its size is not more than
> two pointers, so it can be passed by registers by most ABIs.

I greatly prefer to always pass things as pointers to ensure that one
doesn't accidentally
pass bug structures on the stack (a problem we have had several times).
Yes, it's true that for a simple function that directly uses the
LEX_CSTRING then it's
not a big difference in speed or size when calling with LEX_CSTRING. However if
the LEX_CSTRING is passed down to other functions that in their turn do the same
then things will start to matter.
Because the caller should not know how deep the called function will
go, I prefer
to use pointers in most cases.


> Passing it by a pointer add a second dereference, which is costy

Not really as when you pass the LEX_CSTRING on the stack, you have to
read and copy
the pointers.  It's less code to do the deference of LEX_CSTRING in
the call than in all the
100+ places that may call the function.


> >  {
> > -  if (msg_type == MA_CHECK_INFO)
> > +  if (msg_type == _CHECK_INFO)
> >  sql_print_information("%s.%s: %s", param->db_name, param->table_name,
> >msgbuf);
> > -  else if (msg_type == MA_CHECK_WARNING)
> > +  else if (msg_type == _CHECK_WARNING)
> >  sql_print_warning("%s.%s: %s", param->db_name, param->table_name,
> >msgbuf);

Not also that the above code would not work if the LEX_CSTRING would
be an object on the stack.

Regards,
Monty

___
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] On bb-10.6-monty work

[Michael Widenius]

Hi!

Sorry for the delay, have been very busy with working on bug fixes and
internal discussions.

On Fri, Sep 11, 2020 at 7:50 AM Nikita Malyavin
 wrote:
>
> Hello, Monty!
>
> I've been asked to review your recent work regarding memory footprint 
> optimizations.



> > --- a/sql/item.h
> > +++ b/sql/item.h
> > @@ -4236,6 +4239,23 @@ class Item_bool :public Item_int
> >  };
> >
> >
> > +class Item_bool_static :public Item_bool
> > +{
> > +public:
> > +  Item_bool_static(const char *str_arg, longlong i):
> > +Item_bool(NULL, str_arg, i) {};
> > +
> > +  /* Create dummy functions for things that may change the static item */
> > +  void fix_length_and_charset(uint32 max_char_length_arg, CHARSET_INFO *cs)
> > +  {}
> > +  void fix_char_length(size_t max_char_length_arg)
> > +  {}
> > +  void set_join_tab_idx(uint join_tab_idx_arg)
> > +  { DBUG_ASSERT(0); }
> > +};
> > +
> > +extern const Item_bool_static Item_false, Item_true;
> > +
> >  class Item_uint :public Item_int
> >  {
> >  public:
>
>
>
> Do we really need an additional class for static Item_bool's?
> I know that we have constructors hidden for Item, so maybe just
> unhide it in Item_bool, and that's it.

I need the constructor to ensure that no one calls set_join_tab_idx()
(which can be called for any item)
and possible other methods.

The two other methods are a safeguard as it's not obvious when they
could be called.

> Besides, I see fix_char_length is not virtual at all, so reimplementing it 
> has no efect.

Yes, I missed that. I have now deleted that one.

> And why can't we do all that in Item_bool itself?

We can't fix join_tab_idx().

> Note that adding new class increases a virtual method table, and it's huge 
> already, for Item hierarchy.

Adding a new class should not increase the size of the virtual table
for items.  That only contains virtual functions.
Add a new item with virtual methods does create a new virtual table,
but it's not that huge (but it's not that small either).  It's around
1654 bytes for Item's in 10.6

> We could also add some other frequently used Items, like 0, 1, '', NULL, to a 
> static memory.

We create very few item's with fixed value on the fly.

Another problem with static item's is that we have functions that
change them, with _set_join_tab_idx() and marker() for example, which
means most items can't be static.

We only use the new Item_true and Item_false to replace top level
items, which are not marked. Other places are not safe.  This makes it
hard to use other constants like 0, 1 and NULL as static items.

> > commit 6ca3ab6c2c18cdbf614984cb9d5312d58e718372
>

> > +
> > +
> > +/* Make operations in item_base_t and item_with_t work like 'int' */
> > +static inline item_base_t operator&(const item_base_t a, const item_base_t 
> > b)
> > +{
> > +  return (item_base_t) (((item_flags_t) a) & ((item_flags_t) b));
> > +}
> > +
> > +static inline item_base_t & operator&=(item_base_t , item_base_t b)
> > +{
> > +  a= (item_base_t) (((item_flags_t) a) & (item_flags_t) b);
> > +  return a;
> > +}
> > +
> > +static inline item_base_t operator|(const item_base_t a, const item_base_t 
> > b)
> > +{
> > +  return (item_base_t) (((item_flags_t) a) | ((item_flags_t) b));
> > +}
> > +
> > +static inline item_base_t & operator|=(item_base_t , item_base_t b)
> > +{
> > +  a= (item_base_t) (((item_flags_t) a) | (item_flags_t) b);
> > +  return a;
> > +}
> > +
> > +static inline item_base_t operator~(const item_base_t a)
> > +{
> > +  return (item_base_t) ~(item_flags_t) a;
> > +}
> > +
> > +static inline item_with_t operator&(const item_with_t a, const item_with_t 
> > b)
> > +{
> > +  return (item_with_t) (((item_flags_t) a) & ((item_flags_t) b));
> > +}
> > +
> > +static inline item_with_t & operator&=(item_with_t , item_with_t b)
> > +{
> > +  a= (item_with_t) (((item_flags_t) a) & (item_flags_t) b);
> > +  return a;
> > +}
> > +
> > +static inline item_with_t operator|(const item_with_t a, const item_with_t 
> > b)
> > +{
> > +  return (item_with_t) (((item_flags_t) a) | ((item_flags_t) b));
> > +}
> > +
> > +static inline item_with_t & operator|=(item_with_t , item_with_t b)
> > +{
> > +  a= (item_with_t) (((item_flags_t) a) | (item_flags_t) b);
> > +  return a;
> > +}
> > +
> > +static inline item_with_t operator~(const item_with_t a)
> > +{
> > +  return (item_with_t) ~(item_flags_t) a;
> > +}



> Nice experimentation! I wanted to suggest using constexprs inside Item, but
> this is even better, because you cant freely add a bit that not in the enum.
>
> Though I think it is too bloaty to specify all the operators each time,
> so I'd prefer to see duplications metaprogrammed out somehow.

When I wrote the above I was considering doing the above with a template, but
as it was only needed for two item's I decided it was not yet time to
do that. Doing it for
2 instances and 5 functions would in the end make the code longer.
If we would ever need to add a third flag, the yes we should
definitely try to automate

Re: [Maria-developers] 099596296ce: Avoid mallocs when using LONGLONG_BUFFER_SIZE

[Michael Widenius]

Hi!

On Fri, Sep 11, 2020 at 1:29 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> This is a good catch!
>
> But note that LONGLONG_BUFFER_SIZE already reserves +1 for '\0', it's
> defined as
>
> /* Max length needed for a buffer to hold a longlong or ulonglong + end \0 */
> #define LONGLONG_BUFFER_SIZE 21
>
> The problem is that String::set_int doesn't use LONGLONG_BUFFER_SIZE, it uses
> 20*cs->mbmaxlen+1
>
> This +1 is supposed to reserve one byte for '\0'. Which is incorrect,
> because alloc() does it too, so a better fix would be to remove +1 from
> String::set_int().

This is part of the problem.  However if String::set_int would use
LONGLONG_BUFFER_SIZE
for allocation (which it should instead of 20) we still would have a problem.

I think it's right that any allocation for a String buffer should
include space for the extra end null,
as we are in may cases uses c_ptr() for these buffers.

alloc() does reserve place for an extra \0.  However the problem is
that realloc tests
if requested_buffer_length >= allocated_length and if not does an allocation.
In effect this means that the buffer, with current code, needs to be 2
bigger than
the 'size-for-string-data'

The fix I intend to do later in in 10.6 is to change the >= to > in realloc()
This will fix the problem and we can use LONGLONG_BUFFER_SIZE everywhere
without having a second malloc.  This will however require some other changes
and testing that I don't have time to do now, so I prefer to keep
things as it's now
until the above code is done.

Regards,
Monty

___
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] c48b190824a: Fixed error messages from DROP VIEW to align with DROP TABLE

[Michael Widenius]

Hi!

On Sat, Jun 13, 2020 at 5:37 PM Sergei Golubchik  wrote:



> >  DROP VIEW IF EXISTS t1;
> >  Warnings:
> > +Warning  1347'test.t1' is not of type 'VIEW'
>
> please, add a test for the case when a user has no privileges on
> test.t1, the object existence is not leaked out (if one has no
> privileges to know whether t1 exists or whether t1 is a view or a table,
> the error message should not divulge it).
>
> (the same for tables and for sequences in your next commit)

I was about to create a test do that, but then I started to think that
we only have a drop
privilege that works for 'anything with this name This means that if
one is allowed to drop a view named
't1', one is also allowed to drop a table named t1.

This also means that I don't know what to add to the test.  The
privileges are checked first and
if one tries to drop an object that one is not allowed to access, one
will get an error before DROP VIEW code
is executed.
When in DROP, either DROP SEQUENCES, DROP VIEW or DROP TABLE no
privilege checking is made.
This means that a user can always find out if there is an object of a
certain type by executing one of the above drops.
In other mens, there is nothing to hide in DROP VIEW that the user
can't find out anyway.

Regards,
Monty

___
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] 250bc3b6d74: Ensure that table is truly dropped when using DROP TABLE

[Michael Widenius]

> Hi, Michael!

Please use Monty. No reason to be formal!

> > Ensure that table is truly dropped when using DROP TABLE
> >
> > MDEV-11412 AliSQL: [Feature] Issue#34 Support force drop table
>
> please, put this line ^^^ first in the comment comment.

Already done, as we discussed

> > The used code is largely based on code from Tencent
> >
> > The problem is that in some rare cases there may be a conflict between .frm
> > files and the files in the storage engine. In this case the DROP TABLE
> > was not able to properly drop the table.
> >
> > Some MariaDB/MySQL forks has solved this by adding a FORCE option to
>
> TSQL? Or some other fork?
>

At least Alibaba and TSQL.


> > DROP TABLE. After some discussion among MariaDB developers, we concluded
> > that users expects that DROP TABLE should always work, even if the
> > table would not be consistent. There should not be a need to use a
> > separate keyword to ensure that the table is really deleted.
> >
> > The used solution is:
> > - If a .frm table doesn't exists, try dropping the table from all storage
> >   engines.
> > - If the .frm table exists but the table does not exist in the engine
> >   try dropping the table from all storage engines.
>
> I don't see why is that needed, if .frm exists, it tells exactly what
> storage engine to use, there is no point in trying other engines.

I had cases on my machine where I had a frm but tables from another engine.
May happen when you do an alter table from one engine to another.

If the .frm is inconsistent, there is no harm to try a drop in all engines.
After all, this is not something that happens often and when it happens,
better to check things around.




> >
> > Bugs fixed introduced by the original version of this commit:
> > MDEV-22826 Presence of Spider prevents tables from being force-deleted from
> >other engines
>
> I don't think this part of the comment makes any sense, when one looks
> through git history there is no "original version of this commit".

I want to record that a MDEV is fixed by this commit. I think that is
perfectly reasonable and correct thing to do. So this is more a comment
for Jira than anything else.

> It's like a rolled back transaction, that value did not exist, unless
> you use READ UNCOMMITTED :)

Yes, it doesn't exist in the code, but it exists in Jira.
I assume you would not like me to delete the Jira entry?

> > diff --git a/mysql-test/main/drop_table_force.test 
> > b/mysql-test/main/drop_table_force.test
> > new file mode 100644
> > index 000..8fdd79465b7
> > --- /dev/null
> > +++ b/mysql-test/main/drop_table_force.test
> > @@ -0,1 +1,197 @@
> >  +--source include/have_log_bin.inc
> > +--source include/have_innodb.inc
> > +
> > +#
> > +# This test is based on the orginal test from Tencent for DROP TABLE ... 
> > FORCE
> > +# In MariaDB we did reuse the code but MariaDB does not require the FORCE
> > +# keyword to drop a table even if the .frm file or some engine files are
> > +# missing.
> > +# To make it easy to see the differences between the orginal code and
> > +# the new one, we have left some references to the original test case
> > +#
> > +
> > +CALL mtr.add_suppression("Operating system error number");
> > +CALL mtr.add_suppression("The error means the system cannot");
> > +CALL mtr.add_suppression("returned OS error 71");
>
> not sure it's a good idea, error numbers aren't very portable, this will
> blow up some day like a time bomb

This is from the original commit. I don't think they are relevant anymore
but no harm in keeping them. Buildbot will show us if things are wrong,
so nothing that can really 'blow up'.


> > +# create test user
> > +create user test identified by '123456';
> > +grant all privileges on test.t1 to 'test'@'%'identified by '123456' with 
> > grant option;
>
> why with grant option?
> better remove it if it's not essential for drop to work.
> if it is essential, why is it?

Read the comment in the beginning of the test.
This is to test that one doesn't need a separate privilge to be able to
drop an inconsistent table.



> > +
> > +# connect as test
> > +connect (con_test, localhost, test,'123456', );
> > +--connection con_test
> > +
> > +# drop table with user test
> > +drop table t1;
> > +--error ER_BAD_TABLE_ERROR
> > +drop table t1;
> > +
> > +# connect as root
> > +--connection default
> > +
> > +--disconnect con_test
> > +drop user test;
> > +
> > +# check files in datadir about t1
> > +--list_files  $DATADIR/test/
> > +
> > +--echo #Test4: drop table can drop consistent table as well
> > +create table t1(a int) engine=innodb;
> > +drop table t1;
>
> aha, so it works! :)

Orginal code was drop table followed by drop table force.



> > +# second drop with if exists will success
> > +drop table if exists t1;
>
> you've just tested it in Test7, haven't you?

Read again comment at the beginning of the test file.



> > +# create Aria table t2 and rm .MAI and .MAD
> > +create table t2(a int) engine=aria;
> > 

Re: [Maria-developers] b22a28c2295: fixup! 3fe5cd5e1785e3e8de7add9977a1c2ddd403538b

[Michael Widenius]

Hi!

On Fri, May 22, 2020 at 3:27 PM Andrei Elkin  wrote:


   CORNER CASES: read-only, pure myisam, binlog-*, @@skip_log_bin, etc
>
> Aria just makes yet another previously unknown use case of an engine
> that produces THD::ha_info but does not support 2pc, which the assert
> implied.
>
> To explain more, the original block
>
> #ifndef DBUG_OFF
>   for (ha_info= thd->transaction.all.ha_list; rw_count > 1 && ha_info;
>ha_info= ha_info->next())
> DBUG_ASSERT(ha_info->ht() != binlog_hton);
> #endif
>
> claims there most be no binlog hton in a transaction consisting of more
> than 1 hton:s, *when* (at this point) this transaction has not been
> binlogged yet.
> So combination of binlog + Innodb hton would be raise the assert, to
> question "why the heck binlogging has not been done yet?!".
>
> Aria is different from Innodb in this context in that binlogging
> was done at the end of the statement, so to miss
> `cache_mngr->need_unlog' flagging (which is at xa prepare time logging).

Thanks for the explanation, we should have had that in the code.

> I think we should fix the assert rather than to remove. This way:
>
> cat > assert.diff <<.
> diff --git a/sql/log.cc b/sql/log.cc
> index 792c6bb1a99..aaf1fae1cd6 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -10124,6 +10124,16 @@ int TC_LOG_BINLOG::unlog_xa_prepare(THD *thd, bool 
> all)
>  Ha_trx_info *ha_info;
>  uint rw_count= ha_count_rw_all(thd, _info);
>  bool rc= false;
> +#ifndef DBUG_OFF
> +bool no_binlog= true, exist_no_2pc= false;
> +for (ha_info= thd->transaction->all.ha_list; rw_count > 1 && ha_info;
> + ha_info= ha_info->next())
> +{
> +  no_binlog=no_binlog&& ha_info->ht() != binlog_hton;
> +  exist_no_2pc= exist_no_2pc || !ha_info->ht()->prepare;
> +}
> +DBUG_ASSERT(no_binlog || exist_no_2pc);
> +#endif
>
>  if (rw_count > 0)
>  {
> .
>
> I tested it briefly with running XA:s on combination of engines
> including.

On which tree did you test?  bb-10.5-aria?

In the end, after discussions on slack, we ended with:
#ifndef DBUG_OFF
if (rw_count > 1)
{
  /*
There must be no binlog_hton used in a transaction consisting of more
than 1 engine, *when* (at this point) this transaction has not been
binlogged. The one exception is if there is an engine without a
prepare method, as in this case the engine doesn't support XA and
we have to ignore this check.
  */
  bool binlog= false, exist_hton_without_prepare= false;
  for (ha_info= thd->transaction->all.ha_list; ha_info;
   ha_info= ha_info->next())
  {
if (ha_info->ht() == binlog_hton)
  binlog= true;
if (!ha_info->ht()->prepare)
  exist_hton_without_prepare= true;
  }
  DBUG_ASSERT(!binlog || exist_hton_without_prepare);
}
#endif

> > The reason it was not hit
> > is that before Aria was not treated as transactional we could only
> > come here in case of errors
> > and that code was was apparently not tested, at least with binary logging 
> > on.
> >
>
> > With Aria we could come here in case of rollback and we got assert for
> > cases that was perfectly ok.
>
> Well, what 'rollback' do you mean? The function is invoked only
> for ha_prepare().

As far as I remember, I come into this code in some edge case when
something failed that normally never fails.
I think the issue was that Aria doesn't have a prepare handler, which
caused issues in ha_prepare(), but not sure.
Anyway, we now have a solution for this.

Regards,
Monty

___
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] acbe14b122c: Aria will now register it's transactions

[Michael Widenius]

Hi!

On Wed, May 20, 2020 at 10:52 AM Sergei Golubchik  wrote:
>
> Hi, Michael!

> > +class start_new_trans
> > +{
> > +  /* container for handler's private per-connection data */
> > +  Ha_data old_ha_data[MAX_HA];
> > +  struct THD::st_transactions *old_transaction, new_transaction;
> > +  Open_tables_backup open_tables_state_backup;
> > +  MDL_savepoint mdl_savepoint;
> > +  PSI_transaction_locker *m_transaction_psi;
> > +  THD *org_thd;
> > +  uint in_sub_stmt;
> > +  uint server_status;
> > +
> > +public:
> > +  start_new_trans(THD *thd);
> > +  ~start_new_trans()
> > +  {
> > +destroy();
> > +  }
> > +  void destroy()
> > +  {
> > +if (org_thd)// Safety
> > +  restore_old_transaction();
> > +new_transaction.free();
> > +  }
> > +  void restore_old_transaction();
>
> interesting. You made restore_old_transaction() public and you
> use it in many places. Why not to use destroy() instead?
> When one would want to use restore_old_transaction() instead of destroy()?

We already discussed this on slack.
- In some cases it's an advantage/a need to end the transaction before the
  variable is destroyed.
- I don't like to have things happen automatically, especially not something
  as important as doing a commit.  I prefer to have it spelled out.
  The commit in 'destroy()' is a there mainly as a safeguard if one forgets
  to call restore_old_transaction().  I may at some point add an assert to
  force one to call restore_old_transaction() at least in debug code.



> > +int THD::commit_whole_transaction_and_close_tables()
> > +{
> > +  int error, error2;
> > +  DBUG_ENTER("THD::commit_whole_transaction_and_close_tables");
> > +
> > +  /*
> > +This can only happened if we failed to open any table in the
> > +new transaction
> > +  */
> > +  if (!open_tables)
> > +DBUG_RETURN(0);
>
> Generally, I think, it should still end the transaction here.
> Or add an assert that there is no active transaction at the moment.

There can't be any active transactions if there is no open tables.
So this is safe.

> > +
> > +  /*
> > +Ensure table was locked (opened with open_and_lock_tables()). If not
> > +the THD can't be part of any transactions and doesn't have to call
> > +this function.
> > +  */
> > +  DBUG_ASSERT(lock);
> > +
> > +  error= ha_commit_trans(this, FALSE);
> > +  /* This will call external_lock to unlock all tables */
> > +  if ((error2= mysql_unlock_tables(this, lock)))
> > +  {
> > +my_error(ER_ERROR_DURING_COMMIT, MYF(0), error2);
> > +error= error2;
> > +  }
> > +  lock= 0;
> > +  if ((error2= ha_commit_trans(this, TRUE)))
> > +error= error2;
> > +  close_thread_tables(this);
>
> I wonder why you're doing it in that specific order.
> commit(stmt)-unlock-commit(all)-close

Because of unlock tables calls external_lock which affects
transactions in some engines.
This is the fastest way to safely close a transaction.

The other option would be:
commit(stmt) - close()  - commit(all)

but this may in some cases be not optimal if the table is evicted from
the table cache between close() and commit().
(At least Aria will keep the read view open until the table is
commited and the external unlock is called, even over close).



> > +start_new_trans::start_new_trans(THD *thd)
> > +{
> > +  org_thd= thd;
> > +  mdl_savepoint= thd->mdl_context.mdl_savepoint();
> > +  memcpy(old_ha_data, thd->ha_data, sizeof(old_ha_data));
> > +  thd->reset_n_backup_open_tables_state(_tables_state_backup);
> > +  bzero(thd->ha_data, sizeof(thd->ha_data));
> > +  old_transaction= thd->transaction;
> > +  thd->transaction= _transaction;
> > +  new_transaction.on= 1;
> > +  in_sub_stmt= thd->in_sub_stmt;
> > +  thd->in_sub_stmt= 0;
> > +  server_status= thd->server_status;
> > +  m_transaction_psi= thd->m_transaction_psi;
> > +  thd->m_transaction_psi= 0;
> > +  thd->server_status&= ~(SERVER_STATUS_IN_TRANS |
> > + SERVER_STATUS_IN_TRANS_READONLY);
> > +  thd->server_status|= SERVER_STATUS_AUTOCOMMIT;
> > +}
>
> Few thoughts:
>
> 1. If you need to save and restore _all that_ then, perhaps,
>all that should be inside st_transactions ?

Yes, I think that would be much better. We even discussed this and I suggested
that we do that in 10.6

> 2. strictly speaking, ha_data is _per connection_. If you just bzero it,
>the engine will think it's a new connection, and you cannot just
>overwrite it on restore without hton->close_connection.

Yes, and that is how things works now.  The engine threats this as a new
connection and when the new transactions ends, we call hton->free_connection()
on the ha_data before restoring it.

>A strictly "proper" solution would be to introduce ha_data per
>transaction in addition to what we have now. But it looks like an
>overkill. So I'd just add close_system_tables() now into 
> restore_old_transaction()

We already have in restore_old_transaction the code:

Re: [Maria-developers] b22a28c2295: fixup! 3fe5cd5e1785e3e8de7add9977a1c2ddd403538b

[Michael Widenius]

Hi!

On Wed, May 20, 2020 at 8:14 PM Sergei Golubchik  wrote:


> > MDEV-22607 Assertion `ha_info->ht() != binlog_hton' failed in
> >MYSQL_BIN_LOG::unlog_xa_prepare
> >
> > diff --git a/sql/log.cc b/sql/log.cc
> > index 7e9e231358a..792c6bb1a99 100644
> > --- a/sql/log.cc
> > +++ b/sql/log.cc
> > @@ -10128,11 +10128,6 @@ int TC_LOG_BINLOG::unlog_xa_prepare(THD *thd, bool 
> > all)
> >  if (rw_count > 0)
> >  {
> >/* an empty XA-prepare event group is logged */
> > -#ifndef DBUG_OFF
> > -  for (ha_info= thd->transaction->all.ha_list; rw_count > 1 && ha_info;
> > -   ha_info= ha_info->next())
> > -DBUG_ASSERT(ha_info->ht() != binlog_hton);
> > -#endif
> >rc= write_empty_xa_prepare(thd, cache_mngr); // normally gains 
> > need_unlog
> >trans_register_ha(thd, true, binlog_hton, 0); // do it for future 
> > commmit
> >  }
>
> What does this assertion mean and what does it mean that it no longer
> holds?

As far as I understand it was always wrong and didn't make sense.  The
reason it was not hit
is that before Aria was not treated as transactional we could only
come here in case of errors
and that code was was apparently not tested, at least with binary logging on.

With Aria we could come here in case of rollback and we got assert for
cases that was perfectly ok.

Regards,
Monty

___
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] 20d54b09830: Update galera to work with independent sub transactions

[Michael Widenius]

Hi!

On Wed, May 20, 2020 at 8:32 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> I see, you basically disable wsrep for these out-of-band transactions.
> I'm sure it works fine now, with Aria.
> But will it work with InnoDB?

> On May 20, Michael Widenius wrote:
> > revision-id: 20d54b09830 (mariadb-10.5.2-257-g20d54b09830)
> > parent(s): b22a28c2295
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2020-05-19 17:55:00 +0300
> > message:
> >
> > Update galera to work with independent sub transactions
> >
> > diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> > index 51d7380f622..4c87cbeee59 100644
> > --- a/sql/sql_class.cc
> > +++ b/sql/sql_class.cc
> > @@ -5806,6 +5806,8 @@ start_new_trans::start_new_trans(THD *thd)
> >server_status= thd->server_status;
> >m_transaction_psi= thd->m_transaction_psi;
> >thd->m_transaction_psi= 0;
> > +  wsrep_on= thd->variables.wsrep_on;
> > +  thd->variables.wsrep_on= 0;
> >thd->server_status&= ~(SERVER_STATUS_IN_TRANS |
> >   SERVER_STATUS_IN_TRANS_READONLY);
> >thd->server_status|= SERVER_STATUS_AUTOCOMMIT;

Yes it will work with InnoDB.
The reason is that except for statistical tables, all new transactions
are read only and will thus not affect Galera.
Statistical tables are also ok as they are not transactional and in
any case they should
not be sent to slaves.

Regards,
Monty

___
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] 8742d176bc2: Added support for more functions when using partitioned S3 tables

[Michael Widenius]

Hi!

On Fri, Apr 17, 2020 at 3:49 PM Sergei Golubchik  wrote:



> > +++ b/mysql-test/suite/s3/replication_partition.test
> > @@ -0,0 +1,170 @@
> > +--source include/have_s3.inc
> > +--source include/have_partition.inc
> > +--source include/master-slave.inc
>
> master-slave should always be included last, after all other have_xxx 
> includes.



> > diff --git a/mysys/my_symlink.c b/mysys/my_symlink.c
> > index cbee78a7f5c..323ae69a39c 100644
> > --- a/mysys/my_symlink.c
> > +++ b/mysys/my_symlink.c
> > @@ -154,7 +154,8 @@ int my_realpath(char *to, const char *filename, myf 
> > MyFlags)
> >original name but will at least be able to resolve paths that starts
> >with '.'.
> >  */
> > -DBUG_PRINT("error",("realpath failed with errno: %d", errno));
> > +if (MyFlags)
> > +  DBUG_PRINT("error",("realpath failed with errno: %d", errno));
>
> This is kind of against the concept of dbug. dbug's idea is that one should
> not edit the code every time, but put DBUG points one and enable/disable them
> at run-time.
>
> In cases like that you can specify precisely what you want to see in the trace
> with complicated command-line --debug string. But I usually just let it
> log everything and then use dbug/remove_function_from_trace.pl script.

The reason for the above is that it was not a real error, but a check if the
file was a path.

The reason for disabling this is that when I have a problem, I usually search in
the trrace for "error:".  With big traces, there is a lot of calls to
realpath and I
have to skip all these which is very annoying and not helpful as the above
case is not an error!

In other words, we should only log things with label "error" when it's
really is an error
Changing the label depending on MyFlags is not helpful as the rest of
the dbug trace
will anyway show the result from realpath()


> > diff --git a/sql/discover.cc b/sql/discover.cc
> > index e49a2a3b0c0..7f3fe73c155 100644
> > --- a/sql/discover.cc
> > +++ b/sql/discover.cc
> > @@ -99,23 +99,23 @@ int readfrm(const char *name, const uchar **frmdata, 
> > size_t *len)
> >
> >
> >  /*
> > -  Write the content of a frm data pointer
> > -  to a frm file.
> > +  Write the content of a frm data pointer to a frm or par file.
>
> really? writefrm() writes to a .par file?
Yes, that was confusing and the reason I changed the comment.

> may be rename it to writefile() then?

Yes, I will do a rename of this function.

> > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> > index 0ec1f2138ab..b8b5085f389 100644
> > --- a/sql/ha_partition.cc
> > +++ b/sql/ha_partition.cc
> > @@ -629,7 +629,8 @@ int ha_partition::rename_table(const char *from, const 
> > char *to)
> >
> >SYNOPSIS
> >  create_partitioning_metadata()
> > -name  Full path of table name
> > +path  Full path of new table name
> > +old_p Full path of old table name
>
> This is a rather meaningless phrase, there cannot be a "path" of a "name"
>
> May be, write "A path to the new/old frm file but without the extension"
> instead?

Fixed

> >  create_info   Create info generated for CREATE 
> > TABLE
> >
> >RETURN VALUE
> > @@ -678,6 +681,19 @@ int ha_partition::create_partitioning_metadata(const 
> > char *path,
> >DBUG_RETURN(1);
> >  }
> >}
> > +
> > +  /* m_part_info is only NULL when we failed to create a partition table */
>
> How can m_part_info be NULL here?
> create_handler_file() dereferences m_part_info unconditionally, if
> m_part_info would've been NULL it'll crash.

Yes, but create_handler_part() is only used for create, not f or drop
or rename and in these
cases m_part_info can be 0.

I added this, as I got this error while testing S3. One of the test in
S3 covers this code.
If you comment the 'if', you will get a core dump (just tested that)
in s3.replication_partition

> > @@ -1604,6 +1620,7 @@ int ha_partition::prepare_new_partition(TABLE *tbl,
> >
> >if (!(file->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> >  tbl->s->connect_string= p_elem->connect_string;
> > +  create_info->options|= HA_CREATE_TMP_ALTER;
>
> This looks wrong. The partition isn't created as a temporary file.
> HA_CREATE_TMP_ALTER is used for autiding and perfschema to distinguish
> between normal tables, temporary tables and these alter-tmp-tables.

> I see that you've hijacked this flag and check it inside s3, but it wasn't
> designed for that, so change s3 instead, not what the flag means.
> I don't think you need that if inside s3 at all, if hton flag says
> HTON_TEMPORARY_NOT_SUPPORTED, then s3 won't be asked to create a temporary
> table and you don't need a check for that in create.

It's acutally the other way around.  S3 only allows creation of
temporary tables, so I need
a flag for this.
In addition, prepare_new_partition() creates temporary tables, like:
t1#P#p0#TMP# , which will later be 

Re: [Maria-developers] 9243921c84b: Make all #sql temporary table names uniform

[Michael Widenius]

Hi!

On Fri, 17 Apr 2020, 15:52 Marko Mäkelä,  wrote:

> Monty,
>
> On Thu, Apr 16, 2020 at 3:32 PM Michael Widenius
>  wrote:
> > > > ALTER PARTITION shadow files:
> > > > #sql-shadow-'original_table_name'
> > >
> > > Please, add a thread_id here at the end. normally MDL should ensure
> that
> > > no two threads can have a shadow for the same table at the same time,
> > > but we have enough bugs as it is to introduce another vector when two
> > > threads can overwrite each other temp files.
> >
> > That would make the file name even longer and I am not sure what
> > happens if we table names goes much over
> >  NAME_LEN.  I don't have time just now to check for possible name
> > overruns (I think it should be save as most
> > engines are using FN_REFLEN, but better safe than sorry.
>
> FN_REFLEN limits the path name to only 511 or 512 bytes.
> When I started using GNU/Linux in 1993, the maximum path name length
> was 4095 or 4096 bytes. Already back then, extfs or xiafs or something
> supported up to 255 bytes per path name component (name in a
> directory).
>

I understand the problem, but not the suggested solution?
We can't just increase FN_REFLEN as it will increase stack and memory
usage.  And increasing it will not help when the filename part becomes too
long.
In reality this is more a theoretical (or hacker security problem) as long
table names are not common. Don't remember a single real bug report about
this.

Regards,
Monty

>
___
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] faf8de3aa98: Fixed some assert crashes related to keyread.

[Michael Widenius]

Hi!

On Fri, 17 Apr 2020, 15:21 Sergei Golubchik,  wrote:

> Hi, Michael!
>
> On Apr 16, Michael Widenius wrote:
> > Hi!
> >
> > On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik 
> wrote:
> >
> > 
> > > where's a test case for MDEV-22077?
> >
> > One can't use the test case in MDEV-22077 as it involves a table
> > created by a 32 bit MariaDB version. Doing another test case it's very
> > hard so I ignored it and just did run the test to verify the patch.
>
> Alice posted a simple test case in the comments.
> It has a very similar stack trace.
> Could you check it out? Is it a same bug or a different one?
>
> > > > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> > > > index 19eabbb053c..2fc0de4345f 100644
> > > > --- a/sql/sql_delete.cc
> > > > +++ b/sql/sql_delete.cc
> > > > @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST
> *table_list, COND *conds,
> > > >  else
> > > >  {
> > > >ha_rows scanned_limit= query_plan.scanned_rows;
> > > > +  table->no_keyread= 1;
> > > >query_plan.index= get_index_for_order(order, table, select,
> limit,
> > > >  _limit,
> > > >
> _plan.using_filesort,
> > > >  );
> > > > +  table->no_keyread= 0;
> > >
> > > why?
> >
> > get_index_for_order is not allowed to change to use key_reads as there
> > is no guarantee
> > at this point that the suggested index will be used. That was the
> > reason we got crashes.
>
> Why does it enable keyread then? Where?
>

Don't know exactly and don't care for the moment.  The problem is that we
try to do a last desperate attempt to use a better index after all
optimization decissions are already done and this is an effect of this.
Changing this is too time consuming at this point (been in discussions for
years and not done already as it's not trivial) so I don't have time to do
it now.

Regards,
Monty

>
___
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] faf8de3aa98: Fixed some assert crashes related to keyread.

[Michael Widenius]

Hi!

On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik  wrote:


> where's a test case for MDEV-22077?

One can't use the test case in MDEV-22077 as it involves a table
created by a 32 bit MariaDB
version. Doing another test case it's very hard so I ignored it and
just did run the test to verify
the patch.

> > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> > index 19eabbb053c..2fc0de4345f 100644
> > --- a/sql/sql_delete.cc
> > +++ b/sql/sql_delete.cc
> > @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, 
> > COND *conds,
> >  else
> >  {
> >ha_rows scanned_limit= query_plan.scanned_rows;
> > +  table->no_keyread= 1;
> >query_plan.index= get_index_for_order(order, table, select, limit,
> >  _limit,
> >  _plan.using_filesort,
> >  );
> > +  table->no_keyread= 0;
>
> why?

get_index_for_order is not allowed to change to use key_reads as there
is no guarantee
at this point that the suggested index will be used. That was the
reason we got crashes.

> > @@ -28568,8 +28568,6 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER 
> > *order, TABLE *table,
> >if (new_used_key_parts != NULL)
> >  *new_used_key_parts= best_key_parts;
> >table->file->ha_end_keyread();
> > -  if (is_best_covering && !table->no_keyread)
> > -table->file->ha_start_keyread(best_key);
>
> I find it very confusing that test_if_cheaper_ordering has a side effect
> of ending keyread. It was ok, when it ended previous keyread and started
> a new one. But just disabling - it's looks very strange.

It has to be disabled as we may in the future use another index than
the one we originally planned to use (with keyread).
This is something that should be re-factored in the future so that we
only enable key-read's when we have done the final
decision of which index we will really use.

Regards,
Monty

___
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] fb29c886701: Handle errors from external_unlock & mysql_unlock_tables

[Michael Widenius]

Hi!

> >
> > Handle errors from external_unlock & mysql_unlock_tables
> >
> > Other things:
> > - Handler errors from ha_maria::implict_commit
> > - Disable DBUG in safe_mutex_lock to get trace file easier to read
> >
> > diff --git a/mysys/thr_mutex.c b/mysys/thr_mutex.c
> > index 4f495048f63..f32132136b8 100644
> > --- a/mysys/thr_mutex.c
> > +++ b/mysys/thr_mutex.c
> > @@ -233,6 +233,7 @@ int safe_mutex_lock(safe_mutex_t *mp, myf my_flags, 
> > const char *file,
> >int error;
> >DBUG_PRINT("mutex", ("%s (0x%lx) locking", mp->name ? mp->name : "Null",
> > (ulong) mp));
> > +  DBUG_PUSH("");
> >
> >pthread_mutex_lock(>global);
> >if (!mp->file)
> > @@ -283,7 +284,7 @@ int safe_mutex_lock(safe_mutex_t *mp, myf my_flags, 
> > const char *file,
> >{
> >  error= pthread_mutex_trylock(>mutex);
> >  if (error == EBUSY)
> > -  return error;
> > +  goto end;
> >}
> >else
> >  error= pthread_mutex_lock(>mutex);
> > @@ -393,6 +394,8 @@ int safe_mutex_lock(safe_mutex_t *mp, myf my_flags, 
> > const char *file,
> >  }
> >}
> >
> > +end:
> > +  DBUG_POP();
> >DBUG_PRINT("mutex", ("%s (0x%lx) locked", mp->name, (ulong) mp));
>
> this message is no longer always correct, is it?

Good catch. Will fix.

Regards,
Monty

___
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] bc5c062b1d1: Don't try to open temporary tables if there are no temporary tables

[Michael Widenius]

Hi!

On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Apr 13, Michael Widenius wrote:
> > revision-id: bc5c062b1d1 (mariadb-10.5.2-124-gbc5c062b1d1)
> > parent(s): fb29c886701
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2020-04-09 01:37:02 +0300
> > message:
> >
> > Don't try to open temporary tables if there are no temporary tables

> Why?

Because it's quite slow to loop over all tables to check if could be a
temporary tables
in the default case where we don't have any temporary tables at all.
This will improve performance for almost any table open.

I have added this to the commit message

> > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > index 4d7a7606136..be19a2e1d82 100644
> > --- a/sql/sql_base.cc
> > +++ b/sql/sql_base.cc
> > @@ -3704,9 +3704,9 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, 
> > uint *counter, uint flags,
> >  The problem is that since those attributes are not set in merge
> >  children, another round of PREPARE will not help.
> >  */
> > -error= thd->open_temporary_table(tables);
> > -
> > -if (!error && !tables->table)
> > +if (!thd->has_temporary_tables() ||
> > +(!(error= thd->open_temporary_table(tables)) &&
> > + !tables->table))
>
> please don't write conditions like that. keep it as before, two
> statements:

Why?  It's perfectly clear and not worse than

  if (!thd->has_temporary_tables())
if (!(error= thd->open_temporary_table(tables)) &&  !tables->table))


>if (thd->has_temporary_tables())
>  error= thd->open_temporary_table(tables);
>if (!error && !tables->table)

Sorry, the above is worse and more error prone as you error may no be
set when you test it.

> > --- a/sql/temporary_tables.cc
> > +++ b/sql/temporary_tables.cc
> > @@ -338,9 +338,9 @@ bool THD::open_temporary_table(TABLE_LIST *tl)
> >  have invalid db or table name.
> >  Instead THD::open_tables() should be used.
> >*/
> > -  DBUG_ASSERT(!tl->derived && !tl->schema_table);
> > +  DBUG_ASSERT(!tl->derived && !tl->schema_table && has_temporary_tables());
>
> please, never do asserts with &&-ed conditions, this should be
>
>  DBUG_ASSERT(!tl->derived);
>  DBUG_ASSERT(!tl->schema_table);
>  DBUG_ASSERT(has_temporary_tables());

ok, I can change that.

Regards,
Monty

___
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] bf32018be96: Added support for VISIBLE attribute for indexes in CREATE TABLE

[Michael Widenius]

Hi!

On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Apr 13, Michael Widenius wrote:
> > revision-id: bf32018be96 (mariadb-10.5.2-126-gbf32018be96)
> > parent(s): 62c2d0f3e1f
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2020-04-09 01:37:02 +0300
> > message:
> >
> > Added support for VISIBLE attribute for indexes in CREATE TABLE
> >
> > MDEV-22199 Add VISIBLE attribute for indexes in CREATE TABLE
> >
> > This was done to make it easier to read in dumps from MySQL 8.0
>
> This is not a "dump from MySQL 8.0". It is an SQL export file generated
> by MySQL Workbench. And MySQL Workbench has a configuration option
> whether to put this VISIBLE in the sql file or not.

It's a dump from MySQL 8.0 done by workbench. I don't know if the configuration
is default or not, but I don't see any reason to not support it as
others can make
easily the same mistake.

Regards,
Monty

___
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] 9243921c84b: Make all #sql temporary table names uniform

[Michael Widenius]

Hi!



> > ALTER PARTITION shadow files:
> > #sql-shadow-'original_table_name'
>
> Please, add a thread_id here at the end. normally MDL should ensure that
> no two threads can have a shadow for the same table at the same time,
> but we have enough bugs as it is to introduce another vector when two
> threads can overwrite each other temp files.

That would make the file name even longer and I am not sure what
happens if we table names goes much over
 NAME_LEN.  I don't have time just now to check for possible name
overruns (I think it should be save as most
engines are using FN_REFLEN, but better safe than sorry.

> > Names from the temp pool:
> > #sql-name-current_pid-pool_number
>
> Would you mind if I drop temp pool completely in 10.5?
> It was added by Jeremy in January 2001 with the comment
>
>   Added --temp-pool option to mysqld.  This will cause temporary files
>   created to use a small set of filenames, to try and avoid problems
>   in the Linux kernel.
>
> And I doubt it's still an issue in 2020.

I think that reusing file names is still something that is a problem.
We should do some tests for that before skipping this code.
It's not a lot a code after all.

Regard,
Monty

___
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] 3cbe15bd78c: Fixed core dump in alter table if ADD PARTITION fails

[Michael Widenius]

Hi!

On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik  wrote:
>
> Hi, Michael!
>
> On Apr 13, Michael Widenius wrote:
> > revision-id: 3cbe15bd78c (mariadb-10.5.2-120-g3cbe15bd78c)
> > parent(s): d4d332d196d
> > author: Michael Widenius 
> > committer: Michael Widenius 
> > timestamp: 2020-04-09 01:37:01 +0300
> > message:
> >
> > Fixed core dump in alter table if ADD PARTITION fails
> >
> > I didn't add a test case as to reproduce this we need to
> > have a failed write in on of the engines. Bug found and bug fix
> > verified while debugging S3 and partitions.
> >
> > ---
> >  sql/sql_partition.cc | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
> > index ef8ef5114a8..4e984fa775d 100644
> > --- a/sql/sql_partition.cc
> > +++ b/sql/sql_partition.cc
> > @@ -6817,7 +6817,8 @@ static void 
> > handle_alter_part_error(ALTER_PARTITION_PARAM_TYPE *lpt,
> >DBUG_ENTER("handle_alter_part_error");
> >DBUG_ASSERT(table->m_needs_reopen);
> >
> > -  if (close_table)
> > +  /* The table may not be open if ha_partition::change_partitions() failed 
> > */
> > +  if (close_table && !table->file->is_open())
>
> This looks *very* confusing, like you close the table only if it's *not*
> open. But then the whole if() is very confusing, as it closes in both
> branches, so the meaning of "close_table" is totally not clear.

Agree it looks confusing but I am reasonable sure it did stop a crash.
I can't verify that as it was very hard to repeat.

> May be instead of this your fix you'd better cherry-pick
> 9c02b7d6670b069866 from MySQL? It removes close_table completely,
> so I expect it to work for S3 just as you wanted.

I ported the above to MariaDB and I agree it looks this is a better solution.

Regards,
Monty

___
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] 5365db70cb8: Improve update handler (long unique keys on blobs)

[Michael Widenius]

Hi!



> I mean, you have tests, both with PERIOD, but one in the main suite, the
> other in the versioning suite. While they both should be in the period
> suite. Here, your test from the main/long_unique.test:

> +CREATE OR REPLACE TABLE t1 (a INT, b BLOB, s DATE, e DATE, PERIOD FOR 
> app(s,e), UNIQUE(b)) ENGINE=MyISAM PARTITION BY HASH(a) PARTITIONS 2;
> +INSERT INTO t1 VALUES (1,'foo','2022-01-01', '2025-01-01');
> +--error ER_DUP_ENTRY
> +DELETE FROM t1 FOR PORTION OF app FROM '2023-01-01' TO '2024-01-01';
> +DROP TABLE t1;
>
> Your test from the suite/versioning/t/long_unique.test:
>
> +CREATE TABLE t1 (f VARCHAR(4096), s DATE, e DATE, PERIOD FOR app(s,e), 
> UNIQUE(f)) ENGINE=MyISAM;
> +INSERT INTO t1 VALUES ('foo', '2023-08-30', '2025-07-09'),('bar', 
> '2021-01-01', '2021-12-31');
> +DELETE FROM t1 FOR PORTION OF app FROM '2023-08-29' TO '2025-07-01';
> +DROP TABLE t1;

The above are actual different, indendent tests not related to each other.
But I agree that they are both related to period.

I will move the PERIOD test from
main/long_unique.test
and
suite/versioning/t/long_unique.test
to
suite/period/t/long_unique.test

Sorry, I got confused about the suggestion with moving whole test files.


> > > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> > > > index 73191907aac..c7e61864366 100644
> > > > --- a/sql/ha_partition.cc
> > > > +++ b/sql/ha_partition.cc
> > > > @@ -4340,6 +4339,10 @@ int ha_partition::write_row(const uchar * buf)
> > > >}
> > > >m_last_part= part_id;
> > > >DBUG_PRINT("info", ("Insert in partition %u", part_id));
> > > > +  if (table->s->long_unique_table &&
> > > > +  m_file[part_id]->update_handler == m_file[part_id] && inited == 
> > > > RND)
> > > > +m_file[part_id]->prepare_for_insert(0);
> > >
> > > This looks quite incomprehensible. If the table has a long unique key
> > > and the update_handler is the handler itself you _prepare for insert_ ???
> >
> > This is basically the original code we had in handler::write_row.
> > We have to have this in this place as we don't want to call
> > handler->prepare_for_insert() for all partitions, only those
> > that where really used.
>
> I mean not the condition, but the "prepare_for_insert" method name that
> you've introduced, it didn't exist in the original code.
>
> If the name would've been, say, "prepare_update_handler" or
> "prepare_long_unique_handler" then the code would've looked fine and I
> wouldn't have asked.
>
> > > if it's an insert it should've been prepared earlier, like, normally for
> > > an insert, and it don't have to do anything with long uniques.
> >
> > See above. The above is to avoid creating new handlers when we don't need
> > them!  If you have 100 partitions, you really don't want to create 100 
> > cloned
> > table handlers.
>
> Of course. I just mean the method name. If it is "preparing for insert"
> as the name suggests, one could've expected it to be done earlier. I'm
> saying, the name should not suggest it's a preparation for an insert,
> call it, for example, "prepare_update_handler"

See my comments. It's not only intended for update handler, so I prefer
to keep the current name as that will be more future proof.

Also, it should only be called when one does insert's, not because
there exists an update handler.  It's perfectly safe to call before
any insert usage. The current code just optimized when it's called.

>
> > > What you actually do here (as I've found out looking firther in the patch)
> > > you prepare update_handler. That is "prepare_for_insert" is VERY confusing
> > > here, it sounds like it prepares for a normal insert and should be
> > > called at the beginning of mysql_insert(). And it's nothing like that.

We are calling it as start of mysql_insert(), however we have an if around
it as a small optimization.  We can remove the if you want, things will
work as before if we d.

> > > It would remove half of the questions if you'd call it
> > > prepare_update_handler() or, better, prepare_auxiliary_handler() (because
> > > it'll be used not only for long uniques) or something like that.

As one ONLY have to call it for insert and it's safe to call if one has
update handler or not, I don't think any of the above names are any better.

> > The idea with prepare_for_insert is that we can add special handler code
> > that we want to do once in case of insert. We didn't have that before and
> > I can see many use of that.
> >
> > In any case, I will add a comment to the above partition code to clarify
> > what we are doing:
> >
> > "We have to call prepare_for_insert() if we have an update handler
> > in the underlying table (to clone the handler). This is because for
> > INSERT's prepare_for_insert() is only called for the main table,
> > not for all partitions. This is to reduce the huge overhead of cloning
> > a possible not needed handler if there are many partitions."
>
> I don't think this is needed. The confusing part was not the condition,
> but 

Re: [Maria-developers] 5365db70cb8: Improve update handler (long unique keys on blobs)

[Michael Widenius]

Hi!



> > Improve update handler (long unique keys on blobs)


>
> you may want to remove this line from the commit comment

Fixed

> >
> > diff --git a/mysql-test/main/long_unique.result 
> > b/mysql-test/main/long_unique.result
> > index a4955b3e7b5..fee8e7721bf 100644
> > --- a/mysql-test/main/long_unique.result
> > +++ b/mysql-test/main/long_unique.result
> > @@ -1477,4 +1477,28 @@ id select_type table   typepossible_keys 
> >   key key_len ref rowsExtra
> >  SELECT t2.b FROM t1 JOIN t2 ON t1.d = t2.f WHERE t2.pk >= 20;
> >  b
> >  drop table t1,t2;
> > +#
> > +# MDEV-21470 MyISAM start_bulk_insert doesn't work with long unique
> > +#
> > +CREATE TABLE t1 (a INT, b BLOB) ENGINE=MyISAM;
> > +INSERT INTO t1 VALUES (1,'foo'),(2,'bar');
> > +CREATE TABLE t2 (c BIT, d BLOB, UNIQUE(d)) ENGINE=MyISAM;
> > +INSERT INTO t2 SELECT * FROM t1;
> > +Warnings:
> > +Warning  1264Out of range value for column 'c' at row 2
> > +DROP TABLE t1, t2;
> > +#
> > +# MDEV-19338 Using AUTO_INCREMENT with long unique
> > +#
> > +CREATE TABLE t1 (pk INT, a TEXT NOT NULL DEFAULT '', PRIMARY KEY (pk), b 
> > INT AUTO_INCREMENT, UNIQUE(b), UNIQUE (a,b)) ENGINE=myisam;
> > +ERROR HY000: Function or expression 'AUTO_INCREMENT' cannot be used in the 
> > UNIQUE clause of `a`
>
> Quite confusing error message :(
> I understand where it comes from, but for a user, I'm afraid, it just won't
> make any sense.
>
> A reasonable message could be, for example,
>
>   AUTO_INCREMENT column %`s cannot be used in the UNIQUE index %`s

I was trying to not have to create a new error message just for this
very unlikely case.
But if it makes you happy, I can add a new one

>
> > +#
> > +# MDEV-21819 Assertion `inited == NONE || update_handler != this'
> > +# failed in handler::ha_write_row
> > +#
> > +CREATE OR REPLACE TABLE t1 (a INT, b BLOB, s DATE, e DATE, PERIOD FOR 
> > app(s,e), UNIQUE(b)) ENGINE=MyISAM PARTITION BY HASH(a) PARTITIONS 2;
> > +INSERT INTO t1 VALUES (1,'foo','2022-01-01', '2025-01-01');
> > +DELETE FROM t1 FOR PORTION OF app FROM '2023-01-01' TO '2024-01-01';
> > +ERROR 23000: Duplicate entry 'foo' for key 'b'
> > +DROP TABLE t1;
> >  set @@GLOBAL.max_allowed_packet= @allowed_packet;
> > diff --git a/mysql-test/suite/versioning/r/long_unique.result 
> > b/mysql-test/suite/versioning/r/long_unique.result
> > new file mode 100644
> > index 000..da07bc66e22
> > --- /dev/null
> > +++ b/mysql-test/suite/versioning/r/long_unique.result
> > @@ -0,0 +1,8 @@
> > +#
> > +# Assertion `inited == NONE || update_handler != this' failed in
> > +# handler::ha_write_row
> > +#
> > +CREATE TABLE t1 (f VARCHAR(4096), s DATE, e DATE, PERIOD FOR app(s,e), 
> > UNIQUE(f)) ENGINE=MyISAM;
> > +INSERT INTO t1 VALUES ('foo', '2023-08-30', '2025-07-09'),('bar', 
> > '2021-01-01', '2021-12-31');
> > +DELETE FROM t1 FOR PORTION OF app FROM '2023-08-29' TO '2025-07-01';
> > +DROP TABLE t1;
>
> you've had almost the same test (MDEV-21819) in main/long_unique.test
>
> it's a bit strange to have to very similar tests in different suites

Agree. However I didn't want to do too much cleanup work
(Already done way more than my share :( )

However, I noticed that there was different code path's for versioning when it
comes to unique handling so I had to add the test to many places.

> I suggest to move MDEV-21819 test from main/long_unique.test to this file.
Don't know what you mean with 'this file'. The versioning/long_unique file
only contains the test relevant for versioning, there is no reason to have
any more tests in there.

> and then move this whole file from versioning suite to the period suite.
As this particular test crashes with just versioning, it should be there,
not in period.
Feel free to do the above cleanup later if you want. However, I think
that main unique testing should be in the main test suite, not in period.

> > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> > index 73191907aac..c7e61864366 100644
> > --- a/sql/ha_partition.cc
> > +++ b/sql/ha_partition.cc
> > @@ -4340,6 +4339,10 @@ int ha_partition::write_row(const uchar * buf)
> >}
> >m_last_part= part_id;
> >DBUG_PRINT("info", ("Insert in partition %u", part_id));
> > +  if (table->s->long_unique_table &&
> > +  m_file[part_id]->update_handler == m_file[part_id] && inited == RND)
> > +m_file[part_id]->prepare_for_insert(0);
>
> This looks quite incomprehensible. If the table has a long unique key
> and the update_handler is the handler itself you _prepare for insert_ ???

This is basically the original code we had in handler::write_row.
We have to have this in this place as we don't want to call
handler->prepare_for_insert() for all partitions, only those
that where really used.

> if it's an insert it should've been prepared earlier, like, normally for
> an insert, and it don't have to do anything with long uniques.

See above. The above is to avoid creating new handlers when we don't need
them!  If you have 100 

Re: [Maria-developers] 256753e8ae8: Clean up and speed up interfaces for binary row logging

[Michael Widenius]

Hi!

On Fri, Feb 28, 2020 at 10:00 PM Sergei Golubchik  wrote:
...
> > diff --git a/mysql-test/suite/rpl/r/create_or_replace_mix.result 
> > b/mysql-test/suite/rpl/r/create_or_replace_mix.result
> > index 661278aa7ef..6c83d27eef9 100644
> > --- a/mysql-test/suite/rpl/r/create_or_replace_mix.result
> > +++ b/mysql-test/suite/rpl/r/create_or_replace_mix.result
> > @@ -223,26 +226,12 @@ Log_namePos Event_type  Server_id 
> >   End_log_pos Info
> >  slave-bin.01 #   Gtid#   #   GTID #-#-#
> >  slave-bin.01 #   Query   #   #   use `test`; create 
> > table t1 (a int)
> >  slave-bin.01 #   Gtid#   #   BEGIN GTID #-#-#
> > -slave-bin.01 #   Annotate_rows   #   #   insert into 
> > t1 values (0),(1),(2)
> > -slave-bin.01 #   Table_map   #   #   table_id: # 
> > (test.t1)
> > -slave-bin.01 #   Write_rows_v1   #   #   table_id: # 
> > flags: STMT_END_F
> > +slave-bin.01 #   Query   #   #   use `test`; insert 
> > into t1 values (0),(1),(2)
>
> why is it STATEMENT now?

" - In the original code, the master could come into a state where row
  logging is enforced for all future events if statement could be used.
  This is now partly fixed."

The test is run in mix mode.  Insert are normally run in statement
mode for simple tables like the above.
The old code assumed that if you have created a temporary table, then
all future statements until the
temporary table is dropped will always be in binary logging mode.  I
have now fixed so that only
statements that are actually using the temporary table will use binary logging.




> > diff --git a/mysql-test/suite/rpl/t/rpl_foreign_key.test 
> > b/mysql-test/suite/rpl/t/rpl_foreign_key.test
> > new file mode 100644
> > index 000..50be97af24d
> > --- /dev/null
> > +++ b/mysql-test/suite/rpl/t/rpl_foreign_key.test
> > @@ -0,0 +1,18 @@
> > +--source include/have_innodb.inc
> > +--source include/have_binlog_format_row.inc
> > +
> > +CREATE TABLE t1 (
> > +id INT,
> > +k INT,
> > +c CHAR(8),
> > +KEY (k),
> > +PRIMARY KEY (id),
> > +FOREIGN KEY (id) REFERENCES t1 (k)
> > +) ENGINE=InnoDB;
> > +LOCK TABLES t1 WRITE;
> > +SET SESSION FOREIGN_KEY_CHECKS= OFF;
> > +SET AUTOCOMMIT=OFF;
> > +INSERT INTO t1 VALUES (1,1,'foo');
> > +DROP TABLE t1;
> > +SET SESSION FOREIGN_KEY_CHECKS= ON;
> > +SET AUTOCOMMIT=ON;
>
> What kind of replication test is it? You don't check binlog events, you
> don't compare master and slave, you don't even run anything on the slave
> to check whether your staments were replicated correctly.

The test case is from mdev-21617.  It caused a crash in one version of the code
as such. I agree that it would be better to check the result, but as
this was never
a problem I didn't think about adding it.

> In fact, you don't have any slave, you have not included
> master-slave.inc, you only have binlog, so this test should be in the
> binlog suite, not in the rpl suite - it's a binlog test, not replication
> test.

> And even in the binlog suite it would make sense to see what's actually
> in a binlog. Just as a test that it's ok.
I have now moved it to binlog and check the binary log, even if it's
not really needed
for the MDEV.

> > diff --git a/sql/ha_sequence.cc b/sql/ha_sequence.cc
> > index 6cb9937ebb4..71da208d775 100644
> > --- a/sql/ha_sequence.cc
> > +++ b/sql/ha_sequence.cc
> > @@ -202,7 +202,11 @@ int ha_sequence::write_row(const uchar *buf)
> >DBUG_ENTER("ha_sequence::write_row");
> >DBUG_ASSERT(table->record[0] == buf);
> >
> > -  row_already_logged= 0;
> > +  /*
> > +Log to binary log even if this function has been called before
> > +(The function ends by setting row_logging to 0)
> > +  */
> > +  row_logging= row_logging_init;
>
> this is a sequence-specific hack, so you should define row_logging_init
> in ha_sequence class, not in the base handler class

This would force me to make prepare_for_row_logging() and ha_reset()
virtual or add extra virtual
functions that would have to be called for all handlers in both of the
above cases.
The overhead is much bigger than having two set of the above variable
in the two above functions.

> > index 1e3f987b4e5..4096ae8b90f 100644
> > --- a/sql/handler.cc
> > +++ b/sql/handler.cc
> > @@ -6224,32 +6225,37 @@ bool ha_show_status(THD *thd, handlerton *db_type, 
> > enum ha_stat_type stat)
> >  1  Row needs to be logged
> >  */
> >
> > -bool handler::check_table_binlog_row_based(bool binlog_row)
> > +bool handler::check_table_binlog_row_based()
> >  {
> > -  if (table->versioned(VERS_TRX_ID))
> > -return false;
> > -  if (unlikely((table->in_use->variables.sql_log_bin_off)))
> > -return 0;/* Called by partitioning engine 
> > */
> >  #ifdef WITH_WSREP
> > -  if (!table->in_use->variables.sql_log_bin &&
> > -  wsrep_thd_is_applying(table->in_use))

Re: [Maria-developers] 5ae74b4823a: mysqld --help will now load mysqld.options table

[Michael Widenius]

Hi!

On Fri, Feb 28, 2020 at 4:37 PM Sergei Golubchik  wrote:
>
> Hi, Michael!

> > --- a/sql/mysqld.cc
> > +++ b/sql/mysqld.cc
> > @@ -8511,8 +8511,8 @@ static void option_error_reporter(enum loglevel 
> > level, const char *format, ...)
> >va_start(args, format);
> >
> >/* Don't print warnings for --loose options during bootstrap */
> > -  if (level == ERROR_LEVEL || !opt_bootstrap ||
> > -  global_system_variables.log_warnings)
> > +  if (level == ERROR_LEVEL ||
> > +  (!opt_bootstrap && global_system_variables.log_warnings > 1))
>
> You've completely suppressed all --loose warnings during bootstrap.
> Before your patch they were basically always enabled (because of
> log_warnings==2).

Yes, and log_warnings == 2 is still default, so normal users will
still get loose warnings, except
if they put log_warnings explicitly to 1 to make startup more silent.

> I am not sure it's a good idea to disable warnings completely in
> bootstrap.
>
> If fact, I don't see why bootstrap should be special, so I'd simply
> remove !opt_bootstrap condition completely here. But if you want to keep
> it you can do something like

The change was not for bootstrap. I just keep it there.
However I don't it's important to have loose warnings during bootstrap,
as this is something that we only do during test or upgrades.


> > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc
> > index d7d7fcca4a2..31de259a218 100644
> > --- a/sql/sql_plugin.cc
> > +++ b/sql/sql_plugin.cc
> > @@ -1679,7 +1680,22 @@ int plugin_init(int *argc, char **argv, int flags)
> >  global_system_variables.table_plugin =
> >intern_plugin_lock(NULL, plugin_int_to_ref(plugin_ptr));
> >  DBUG_SLOW_ASSERT(plugin_ptr->ref_count == 1);
> > +  }
> > +  /* Initialize Aria plugin so that we can load mysql.plugin */
> > +  plugin_ptr= plugin_find_internal(, MYSQL_STORAGE_ENGINE_PLUGIN);
> > +  DBUG_ASSERT(plugin_ptr || !mysql_mandatory_plugins[0]);
> > +  if (plugin_ptr)
> > +  {
> > +DBUG_ASSERT(plugin_ptr->load_option == PLUGIN_FORCE);
> >
> > +if (plugin_initialize(_root, plugin_ptr, argc, argv, false))
> > +{
> > +  if (!opt_help)
> > +goto err_unlock;
> > +  plugin_ptr->state= PLUGIN_IS_DISABLED;
> > +}
> > +else
> > +  aria_loaded= 1;
> >}
> >mysql_mutex_unlock(_plugin);
>
> I think this should be done differently. In a completely opposite way.
>
> I had unfurtunately hard-coded MyISAM here. A proper fix here could be
> to remove this special treatment of MyISAM instead of adding another
> special treatment of Aria.
>
> Then plugin_init() could work like:
>
>  * run dd_frm_type() for the mysql.plugin table - like it's done now
>  * instead of hard-coding MyISAM (and Aria), find this engine name
>in the plugin_array[] (note, all builtin plugins are already there)
>  * initialize it and (if successful) load mysql.plugin table
>
> It only concerns the sql_plugin.cc part of your commit. Your aria part
> of the commit is still needed, because a good-behaving engine has to be
> read-only in --help.

I would suggest that we keep things like above for now and then you
can hack it when you are enough annoyed about this ;)

Regards,
Monty

___
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

[Maria-developers] Review 2 of compressed columns

[Michael Widenius]

Hi!

Here is a review for the patches marked with
fixup! MDEV-11371 - column compression

> commit f1a3cd40ecc4c144ac32d43ab5c1e88c8e4fae7e
> Author: Sergey Vojtovich 
> Date:   Mon Jul 31 16:07:39 2017 +0400
>
> fixup! MDEV-11371 - column compression
>
> In worst case we have to store data length + 1 for compressed data,
> we should increase field length by 1 internally so that data is not
> truncated.
>
> diff --git a/mysql-test/t/column_compression.test 
> b/mysql-test/t/column_compression.test
> index 256e96c..d3f8481 100644
> --- a/mysql-test/t/column_compression.test
> +++ b/mysql-test/t/column_compression.test
> @@ -54,8 +54,8 @@ SHOW CREATE TABLE t1;
>  --cat_file $MYSQLD_DATADIR/test/t1.CSV
>  DROP TABLE t1;
>
> ---echo # Test fields with length equal to data length
> -CREATE TABLE t1(a VARCHAR(10) COMPRESSED);
> +--echo # Test fields that don't fit data
> +CREATE TABLE t1(a VARCHAR(9) COMPRESSED);
>  --error ER_DATA_TOO_LONG
>  INSERT INTO t1 VALUES(REPEAT('a', 10));
>  INSERT INTO t1 VALUES(REPEAT(' ', 10));

Please also insert a correct row, and not only edge cases.
(You may do it already, but I can't see that in the diff)

> @@ -70,3 +70,12 @@ INSERT INTO t1 VALUES(REPEAT(' ', 255));
>  SET column_compression_threshold=DEFAULT;
>  SELECT a, LENGTH(a) FROM t1;
>  DROP TABLE t1;
> +
> +--echo # Corner case: VARCHAR(255) COMPRESSED must have 2 bytes pack length
> +CREATE TABLE t1(a VARCHAR(255) COMPRESSED);
> +SHOW CREATE TABLE t1;
> +SET column_compression_threshold=300;
> +INSERT INTO t1 VALUES(REPEAT('a', 255));
> +SET column_compression_threshold=DEFAULT;
> +SELECT a, LENGTH(a) FROM t1;
> +DROP TABLE t1;

Please insert also a row without column_compression_threshold, just
to cover all cases in the test case.

> diff --git a/sql/field.cc b/sql/field.cc
> index 216fb95..e1dc193 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -7555,7 +7555,8 @@ void Field_varstring::sql_type(String ) const
>length= cs->cset->snprintf(cs,(char*) res.ptr(),
>   res.alloced_length(), "%s(%d)",
>(has_charset() ? "varchar" : "varbinary"),
> - (int) field_length / charset()->mbmaxlen);
> + (int) field_length / charset()->mbmaxlen -
> + MY_TEST(compression_method()));
>res.length(length);
>if ((thd->variables.sql_mode & (MODE_MYSQL323 | MODE_MYSQL40)) &&
>has_charset() && (charset()->state & MY_CS_BINSORT))
> @@ -10015,6 +10016,8 @@ void 
> Column_definition::create_length_to_internal_length(void)
>case MYSQL_TYPE_STRING:
>case MYSQL_TYPE_VARCHAR:
>  length*= charset->mbmaxlen;
> +if (sql_type == MYSQL_TYPE_VARCHAR && compression_method())
> +  length++;

Add a comment:

/*
  Compressed columns needs one extra byte to store the compression method.
  This byte is invisible to the end user, but not for the storage engine.
*/

>  key_length= length;
>  pack_length= calc_pack_length(sql_type, length);
>  break;



> commit 274ab73a89573554d07a6de1deb846ad47eb1004
> Author: Sergey Vojtovich 
> Date:   Thu Jul 27 13:06:33 2017 +0400
>
> fixup! MDEV-11371 - column compression
>
> Fixed BLOB COMPRESSED -> BLOB COMPRESSED replication.
>
> The problem was that Field_blob::unpack gets compressed data and calls
> Field_blob_compressed::store(), which compressed it again. Fixed by 
> calling
> Field_blob::set_ptr() instead of ::store().
>
> Note that now no data copy is performed and blob is pointing to data 
> allocated
> by replication routines. It used to work this way before 06fb8c2d10, but 
> still
> should be reviewed carefully to make sure no copy is actually needed.
>

patch ok.

> commit 93e00970100503c3f365063d9261119f096bee48
> Author: Sergey Vojtovich 
> Date:   Thu Jul 27 11:44:20 2017 +0400
>
> Get rid of Field::do_save_field_metadata()
>
> It doesn't serve any purpose, but generates extra virtual function call.
>

ok

> commit e1247d4991bb4ebd3f98788c7bfe31a34faf7b69
> Author: Sergey Vojtovich 
> Date:   Thu Jul 27 10:58:47 2017 +0400
>
> fixup! MDEV-11371 - column compression
>
> This is an addition to "Fixed compressed columns to work with 
> partitioning".
>
> Mroonga calls key_copy() on write/delete, when column is not in read_set.
> Adjust read_set for the duration of val_str().
>
> diff --git a/sql/field.cc b/sql/field.cc
> index ec82984..ab96ee9 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -7659,8 +7659,12 @@ uint Field_varstring::get_key_image(uchar *buff, uint 
> length,
>  {
>String val;
>uint local_char_length;
> +  my_bitmap_map *old_map;
>
> +  old_map= dbug_tmp_use_all_columns(table, table->read_set);
>val_str(, );
> +  dbug_tmp_restore_column_map(table->read_set, old_map);
> +

Shouldn't this be in mronga code and not in MariaDB?

Looking at 

Re: [Maria-developers] Refactoring in LEX_STRING and LEX_CSTRING

[Michael Widenius]

Hi!

On 20 Apr 2017 06:10, "Alexander Barkov"  wrote:

Hello all,


Monty is now doing refactoring:

1. Replacing a lot of LEX_STRING to LEX_CSTRING.

2. Fixing functions and methods that have arguments of types:
a. pointer "const LEX_STRING *name" and
b. reference "const LEX_STRING ".
to the same style. Monty chose passing by pointer (2a).


I think that:

- #1 is good.
- #2 is good, in the sense of changing to the same style.

But I think that the choice of "passing by pointer" in #2 is bad.
Passing by reference for LEX_STRING/LEX_CSTRING related things would be
better.


With the "passing by pointer" approach whenever one needs to pass
a LEX_STRING to a function accepting LEX_CSTRING, one will have to do:


void func1(const LEX_STRING *str)
{
  LEX_CSTRING tmp= {str->str, str->length};
  func2();
}

The above is not a problem:


- We don't have less than 5 places in the code where LEX_STRING is changed
to LEX_CSTRING.
- In these places one can do this with a simple cast, nothing elaborated.



I'd propose two things instead:

1. Add a class:


No reason to make a complex solution for a problem that doesn't exist.



2. Instead of "const LEX_CSTRING *str"
we use "const Lex_cstring " all around C++ code.


No, no, no.

I do not want to use & anywhere as arguments in the code. It hides things
and lends people to do errors like we had before where they declared some
functions to take full structure she references in other places, which
creates a mess.

Regards,
Monty
___
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

[Maria-developers] CREATE SEQUENCE is coming

[Michael Widenius]

Hi!

For those that are trying to port applications to MariaDB from other
databases or need more features than the current AUTO_INCREMENT
provides, the following may be of interest:

First version of sequence is pushed to bb-10.2-sequence. It should be
merged to bb-10.2-compatilbity and then to 10.3 shortly (1-2 weeks).

What is working for the moment:

CREATE OR REPLACE [TEMPORARY] SEQUENCE [IF NOT EXISTS] name
[ INCREMENT [ BY | = ] increment ]
[ MINVALUE [=] minvalue | NO MINVALUE ]
[ MAXVALUE [=] maxvalue | NO MAXVALUE ]
[ START [ WITH | = ] start ] [ CACHE [=] cache ] [ [ NO ] CYCLE ]
ENGINE=xxx COMMENT=".."
SELECT NEXT VALUE FOR sequence_name;
SELECT NEXTVAL(sequence_name);
SELECT PREVIOUS VALUE FOR sequence_name;
SELECT LASTVAL(sequence_name);

SHOW CREATE SEQUENCE sequence_name;
SHOW CREATE TABLE sequence_name;
CREATE TABLE sequence-structure ... SEQUENCE=1
ALTER TABLE sequence RENAME TO sequence2;
RENAME TABLE sequence RENAME TO sequence2;
DROP [TEMPORARY] SEQUENCE sequence_name [IF EXISTS]

See https://jira.mariadb.org/browse/MDEV-10139 for progress.
See commit messages in bb-10.2-sequence for what is still to be done.

Some documentation can be found at:
https://mariadb.com/kb/en/mariadb/create-sequence/
This will be improved as the work progress.

One of the goals with the SEQUENCE implementation is that all old
tools, like mysqldump, should work unchanged, while still keeping
normal usage of sequence standard compatibly.

The make this possible, the sequence is currently implemented as a
table with a few exclusive properties.

The main disadvantage of having sequence as a table is that it uses
the same name space as tables. In other words, you can't have a table
and a sequence of the same name. The benefit is that sequences shows
up in 'show tables', one can create a sequence also with 'create
table' and drop it with 'drop table'.  One can select from it as from
any other table. This ensures that all old tools that works with
tables should work with sequences.

The special properties for sequence tables are:
- A sequence table has always one row.
- When one creates a sequence, either with CREATE TABLE or CREATE
SEQUENCE, one row will be inserted.
- Normal table options works for CREATE SEQUENCE. One can use
ENGINE=xxx, COMMENT=xxx etc.
- If one tries to insert into a sequence table, the single row will be
updated.  This allows mysqldump to work but also gives the additional
benefit that one can change all properties of a sequence with a single
insert. New applications can of course also use ALTER SEQUENCE.
- Updates to the sequence table will change the single row.
- Doing a select on the sequence shows the current state of the
sequence, except the values that are reserved in the cache.  The
column 'next_value' shows the next value not reserved by the cache.
- Truncate on a sequence table will give an error.
- Alter table and rename works on sequences.
- If one creates a sequence with INCREMENT 0, then the sequence will
use auto_increment_increment and auto_increment_offset for the
sequence, just like AUTO_INCREMENT.  This allows sequences to work
reliable in a  master-master environment and with Galera.

Internally sequence tables are created as a normal table without
rollback (InnoDB, Aria and MySAM supports this) with is wrapped by a
sequence engine.  This allowed me to create sequences with almost no
performance impact for normal tables. (The cost is one 'if' per insert
if binary log is enabled).

MariaDB 10.3 will support both the ANSI SQL and Oracle syntax for
creating and accessing sequences. As ANSI SQL doesn't have an easy
access to the last generated value, 103 also supports 'PREVIOUS VALUE
FOR sequence_name', like IBM DB2, and LASTVAL(sequence_name) as
PostgreSQL.

I want to thank Jianwe Zhao from Aliyun for his work on SEQUENCE in
AliSQL which gave me ideas and inspiration for this work.
I also want to thank Marko Mäkelä for his help in the InnoDB part of the code.

There is still a lot of work to fix edge cases, but in the current
implementation most major things seams to work...

Comments, suggestions or questions?

Regards,
Monty

___
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] [s...@mariadb.org: [Commits] 02d155a: MDEV-11418 - AliSQL: [Feature] Issue#1 KILL IDLE TRANSACTIONS]

[Michael Widenius]

Hi!



On Tue, Feb 21, 2017 at 2:37 PM, Sergey Vojtovich  wrote:
> Hi Monty,
>
> Please review this patch.
>
> Thanks,
> Sergey
>
> - Forwarded message from Sergey Vojtovich  -
>
> Date: Tue, 21 Feb 2017 16:29:36 +0400
> From: Sergey Vojtovich 
> To: comm...@mariadb.org
> Subject: [Commits] 02d155a: MDEV-11418 - AliSQL: [Feature] Issue#1 KILL IDLE 
> TRANSACTIONS
>
> revision-id: 02d155aeec87aac8a827a29328d68ecb156094cc 
> (mariadb-10.2.2-162-g02d155a)
> parent(s): 1563354570c5634cb2b448b47a138b4ceaff1a72
> committer: Sergey Vojtovich
> timestamp: 2017-02-21 16:28:42 +0400
> message:
>
> MDEV-11418 - AliSQL: [Feature] Issue#1 KILL IDLE TRANSACTIONS
>
> Terminate idle transactions safely in server layer by setting up socket 
> timeout
> parameter. Percona provides another patch to resolve similar problem, but it
> calls server layer's callback in InnoDB plugin to close THD, which crashes in
> some testcases. See https://bugs.launchpad.net/percona-server/+bug/901060 for
> more detailed information.
>
>   1. export parameter trx_idle_timeout to handle all kinds of transactions, 
> the priority is highest
>   2. export parameter trx_readonly_idle_timeout to handle read-only 
> transactions
>   3. export parameter trx_changes_idle_timeout to handle read-write 
> transactions



> +BEGIN;
> +INSERT INTO t1 VALUES (4),(5),(6);
> +SELECT * FROM t1;
> +ERROR HY000: MySQL server has gone away

It's not optimal to get this kind of error for a timeout as it's make it's
very hard for the application to know what is going on.

An alternative approach would be to instead of closing the connection
do a rollback and for next query send an error "transactions aborted and rolled
back because of transaction timeout"

What do you think about this approach?

To get this done, we should remember in 'net' that we are using a transaction
timeout (not default) and when we get the timeout we should do:

- Do part of the rollback code + unlock tables in THD::cleanup()
  (Probably move to a sub function)
- Remember that we did a timeout rollback.
- When we got next statement and timout_rollback was done, give an error,
  clear flag and wait for next statement.


> +++ b/sql/sql_parse.cc
> @@ -1209,7 +1209,23 @@ bool do_command(THD *thd)
>  number of seconds has passed.
>*/
>if(!thd->skip_wait_timeout)
> -my_net_set_read_timeout(net, thd->variables.net_wait_timeout);
> +  {
> +if (thd->in_active_multi_stmt_transaction())
> +{
> +  bool is_trx_rw= thd->transaction.all.is_trx_read_write();
> +
> +  if (thd->variables.trx_idle_timeout > 0)
> +my_net_set_read_timeout(net, thd->variables.trx_idle_timeout);
> +  else if (thd->variables.trx_readonly_idle_timeout > 0 && !is_trx_rw)
> +my_net_set_read_timeout(net, 
> thd->variables.trx_readonly_idle_timeout);
> +  else if (thd->variables.trx_changes_idle_timeout > 0 && is_trx_rw)
> +my_net_set_read_timeout(net, 
> thd->variables.trx_changes_idle_timeout);
> +  else
> +my_net_set_read_timeout(net, thd->variables.net_wait_timeout);
> +}
> +else
> +  my_net_set_read_timeout(net, thd->variables.net_wait_timeout);
> +  }

Please do it this way instead (less code):

  if (!thd->skip_wait_timeout)
  {
ulong timeout= thd->variables.net_wait_timeout;
if (thd->in_active_multi_stmt_transaction())
{
  bool is_trx_rw= thd->transaction.all.is_trx_read_write();

  if (thd->variables.trx_idle_timeout > 0)
timeout= thd->variables.trx_idle_timeout;
  else if (thd->variables.trx_readonly_idle_timeout > 0 && !is_trx_rw)
timeout= thd->variables.trx_readonly_idle_timeout;
  else if (thd->variables.trx_changes_idle_timeout > 0 && is_trx_rw)
timeout= thd->variables.trx_changes_idle_timeout;
}
my_net_set_read_timeout(net, timeout);
  }

Regards,
Monty

___
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] Server crashes in inline_mysql_mutex_lock

[Michael Widenius]

Hi!

> Thread 1 (Thread 0x7fa0f7549700 (LWP 21346)):
> #0  0x7fa0f9511101 in __pthread_kill (threadid=,
> signo=11) at ../nptl/sysdeps/unix/sysv/linux/pthread_kill.c:61
> #1  0x013f4d28 in my_write_core (sig=11) at
> /data/src/bb-10.0-monty/mysys/stacktrace.c:478
> #2  0x009f6fc3 in handle_fatal_signal (sig=11) at
> /data/src/bb-10.0-monty/sql/signal_handler.cc:285
> #3  
> #4  0x0093123c in inline_mysql_mutex_lock (that=0x0,
> src_file=0x14df108 "/data/src/bb-10.0-monty/sql/rpl_parallel.cc",
> src_line=1278) at
> /data/src/bb-10.0-monty/include/mysql/psi/mysql_thread.h:662
> #5  0x00936842 in handle_rpl_parallel_thread (arg=0x7fa0df2d3ee8) at
> /data/src/bb-10.0-monty/sql/rpl_parallel.cc:1278
> #6  0x7fa0f950c0a4 in start_thread (arg=0x7fa0f7549700) at
> pthread_create.c:309
> #7  0x7fa0f76c487d in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

The problem is flush tables with read locks combined with a stop slave
of the only replication thread that forces a resize of thread pool.
This can get the replication thread into a wrong state where it tries
to delete itself before waiting for the flush tables, which the thread
pool doesn't like.

Fixed by adding a check that we are not in flush tables with read lock
when we delete ourselves from the pool.

Regards,
Monty

___
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] Creating test cases for Spider

[Michael Widenius]

Hi!

> I will today update the 10.2 spider tree with the latest 10.2 tree
> that includes all gcov patches and then try this myself.

I have now merged 10.2-spider tree with the latest 10.2 tree, which
includes the new HASH and better support for gcov.

I am now reviewing the last part of multi_range_read (all files except
ha_partition.cc are reviewed).
Hope to have this last part done soon.

Regards,
Monty

___
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

[Maria-developers] Temporary push of multi_range_read support to 10.2

[Michael Widenius]

Hi!

Kentoku, I have now pushed into 10.2 the code for spider patches:
014,015,023,033,035,037,040,042,044,045,049,050,051,053,059

The push is done to make it easier for Jacob and you to make test
cases for the new code.

I will work on reviewing and cleaning up this patch during the next
few days and then do a push a final version when I have got some more
test cases that covers most of the new code.

Regards,
Monty

___
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] Working on spider patches, MDEV-7701

[Michael Widenius]

Hi!

>> However, I would like to have a clear comment the purpose of the function
>> set_top_table_and_fields().
>
> This function is used to get correlating of a parent (table/column)
> and children (table/column). When conditions are pushed down to child
> table (like child of myisammarge), child table needs to know about
> which table/column is my parent for understanding conditions.

Thanks. I have now added this as a comment to where the function is defined.



>> + virtual int set_top_table_and_fields(TABLE *top_table,
> ... cut ...
>> Why is this int and not void ?
>> Do you have in mind that this may be possible to fail in the future?
>> If it's never going to change, then better to do this void.
>>
>> I will make it void for now and change the relevant code. We can make it
>> int again if needed in the future.
>
> VP allocates some memories in this function. So this function has
> possibility of causing out of memory error. That's why this function
> returns int.

ok. I will change the code for this.

>> I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which
>> allwoed me to make the above tests and the following identical ones
>> much
>> simpler.
>
> ok. And now everyone can create myisammarge type storage engine!


good. We do need some tests for this...

Regards,
Monty

___
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] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Thu, Nov 24, 2016 at 8:21 PM, kentoku  wrote:
> Hi!
>
>> In which case is it lost ?
>> Can you add a test to federated_partition.test that shows in which case the
>> connection information is lost?
>
> when create a table like the following
>
> create table tbl_a(
>   col_a int,
>   primary key(col_a)
> )engine=myisam connection 'some settings for whole table'
> partition by range(col_a)
> (
>   partition pt1 values less than (100) connection 'some settings for pt1',
>   partition pt2 values less than maxvalue connection 'some settings for pt2'
> );
>
> "connection 'some settings for whole table'" is lost. It is not only
> behavior of federated_x.
>
>> One of the original ideas with the partition engine is that each
>> partition can have options
>> that are different from the other partitions. For example, one
>> partition could be with InnoDB, another with MyISAM.  For this to
>> work, there needs to exist a mechanism for specifing options per
>> partition, which the federated engine is indirectly using.
>
> Does it means all options have to write with partition information?

All options that you want to see in SHOW CREATE.

> How about "engine"? Don't you think it is useful if default engine can
> write as table option and only partition specific engines are write as
> partition option? I thought just like this about connect string, too.

Normally you don't need to write special options for each partition. In
this case federatedx is a special case.

>> Where should the connection strings for each partition be stored?
>
> I think this is already stored in par file. This is not problem.
>
>> this needs to work both with Spider but also with other usage of the
>> partitioning engine.
>
> O.K. I understand.
>
>> Where does spider store the tables connect strings ?
>
> Spider doesn't store it. It's in frm file.
>
>> How do you suggest this should work when spider is not used?
>
> I suggest ...
> These connect strings have priority level. Connect strings of
> sub-partition are the first priority. Connect strings of partition are
> the second priority. Connect string of table is the third priority.
>
>> Can you create a patch that will not cause federated_partiton.test to fail?
>
> Yes, I think I can. I'll create it.

Does this patch do what you need:

This preserve the CONNECT string from the top table level as it was
originally entered.

I tested this by doing the following create:

create table t1 (s1 int primary key) engine=federated connection="QQQ"
  partition by list (s1)
  (partition p1 values in (1,3)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1',
   partition p2 values in (2,4)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2');

and show create table was able to show the original connection="QQQ"

--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -3486,13 +3486,15 @@ int ha_partition::open(const char *name, int
mode, uint test_if_locked)
file= m_file;
do
{
+  LEX_STRING save_connect_string= table->s->connect_string;
   create_partition_name(name_buff, name, name_buffer_ptr, NORMAL_PART_NAME,
 FALSE);
   table->s->connect_string = m_connect_string[(uint)(file-m_file)];
-  if ((error= (*file)->ha_open(table, name_buff, mode,
-   test_if_locked | HA_OPEN_NO_PSI_CALL)))
+  error= (*file)->ha_open(table, name_buff, mode,
+  test_if_locked | HA_OPEN_NO_PSI_CALL);
+  table->s->connect_string= save_connect_string;
+  if (error)
 goto err_handler;
-  bzero(>s->connect_string, sizeof(LEX_STRING));
   if (m_file == file)
 m_num_locks= (*file)->lock_count();
   DBUG_ASSERT(m_num_locks == (*file)->lock_count());

If this is ok, I will add this and close MDEV-7700

Regards,
Monty

___
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

[Maria-developers] Working on spider patches, MDEV-7701

[Michael Widenius]

Hi!

Next patch, MDEV 7701

+++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc2016-05-05
21:25:04.289731712 +0900
@@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu
   }
 /* Category 9) Operations only used by MERGE */
   case HA_EXTRA_ADD_CHILDREN_LIST:
+DBUG_RETURN(loop_extra(operation));
   case HA_EXTRA_ATTACH_CHILDREN:
+int result;
+handler **file;
+if ((result = loop_extra(operation)))
+  DBUG_RETURN(result);
+m_num_locks = 0;
+additional_table_flags =
+  (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE);
+file = m_file;
+do
+{
+  m_num_locks += (*file)->lock_count();
+  additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^
+(HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE)));

The above test could be simple:

additional_table_flags &= (ulonglong) ((*file)->ha_table_flags();

Should be the same thing.  I did however remove this code, as
HA_CAN_BG... are not used anywhere in the current spider code. See
comment below.

I also changed so that m_num_locks always contains the upper limit
number of locks needed.  Without this change, the call to lock_count() would
have returneed m_total_parts * 'new-counted-locks' which would be been
way too much.


@@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons
   handler **file= m_file;
   COND *res_cond = NULL;
   DBUG_ENTER("ha_partition::cond_push");
+#ifdef HANDLER_HAS_TOP_TABLE_FIELDS
+  if (set_top_table_fields)
+  {
+do
+{
+  if ((*file)->set_top_table_and_fields(top_table,
+top_table_field,
+top_table_fields))
+DBUG_RETURN(cond);
+} while (*(++file));
+file= m_file;
+  }
+#endif

I added this without the define, but I merged it to the loop for cond_push.

Instead of having defines in the sql code, I will fix that in spider and vp
that I will set the proper defines based on the MariaDB version.

However, I would like to have a clear comment the purpose of the function
set_top_table_and_fields().

 struct st_mysql_storage_engine partition_storage_engine=
 { MYSQL_HANDLERTON_INTERFACE_VERSION };

+++ ./003_mariadb-10.2.0-vp/sql/handler.h2016-05-05 21:25:04.314731713 +0900
@@ -255,6 +257,11 @@ enum enum_alter_inplace_result {
  */
 #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE)

+#define HA_CAN_BG_SEARCH   (1LL << 45)
+#define HA_CAN_BG_INSERT   (1LL << 46)
+#define HA_CAN_BG_UPDATE   (1LL << 47)
+#define HA_CAN_MULTISTEP_MERGE (1LL << 48)

Do you really need HA_CAN_BG_XXX flags?
I checked your spider tree and this is not used anywhere, so I think
we should leave them out for now.
If we don't need these, we can remove the loop for setting them in
HA_EXTRA_ATTACH_CHILDREN too.

If we need them, I would like to have a comment for why they are needed and
when we will need then add these in a separate patch.

@@ -3525,6 +3540,29 @@ public:
Pops the top if condition stack, if stack is not empty.
  */
  virtual void cond_pop() { return; };
+ virtual int set_top_table_and_fields(TABLE *top_table,
+  Field **top_table_field,
+  uint top_table_fields)
+ {
+   if (!set_top_table_fields)
+   {
+ set_top_table_fields = TRUE;
+ this->top_table = top_table;
+ this->top_table_field = top_table_field;
+ this->top_table_fields = top_table_fields;
+   }
+   return 0;
+ }

Why is this int and not void ?
Do you have in mind that this may be possible to fail in the future?
If it's never going to change, then better to do this void.

I will make it void for now and change the relevant code. We can make it
int again if needed in the future.

diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc
./003_mariadb-10.2.0-vp/sql/sql_base.cc
--- ./002_mariadb-10.2.0-spider/sql/sql_base.cc2016-04-17
02:47:27.0 +0900
+++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc2016-05-05
21:25:04.319731713 +0900
@@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table

   table= table->find_table_for_update();

-  if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM)
-  {
-TABLE_LIST *child;
+  if (table->table &&
+(
+  table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM ||
+  (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE)
+)
+  )
+  continue

I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which
allwoed me to make the above tests and the following identical ones
much
simpler.

This is now pushed into 10.2-spider

Regards,
Monty

___
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] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Thu, Nov 24, 2016 at 5:18 PM, Michael Widenius
<michael.widen...@gmail.com> wrote:
> Hi!
>
> On Wed, Nov 23, 2016 at 3:38 PM, kentoku <kentokush...@gmail.com> wrote:
>> Hi Monty!
>>
>> Thank you for starting this task. I appreciate it.
>>
>>> Kentoku, do you have patches for the test files, or should I just take
>>> them from the above spider branch or from somewhere else ?
>>
>> I just attached test files into MDEV-7698. Please use it. And please
>> let me know if you get a error from test. Sometimes, test results are
>> changed by patches. In this case I should check it.

All spider tests now passes in 10.2

I removed a few changes in test results that was affected by some of
the not yet pushed patches, like:

  SHOW STATUS LIKE 'Spider_direct_aggregate';
  Variable_name Value
! Spider_direct_aggregate   1

This fails as the above variable doesn't yet exists.
I will update the test results as soon the variable exists again.

Thanks!

Regards,
Monty

___
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] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Wed, Nov 23, 2016 at 3:38 PM, kentoku  wrote:
> Hi Monty!
>
> Thank you for starting this task. I appreciate it.
>
>> Kentoku, do you have patches for the test files, or should I just take
>> them from the above spider branch or from somewhere else ?
>
> I just attached test files into MDEV-7698. Please use it. And please
> let me know if you get a error from test. Sometimes, test results are
> changed by patches. In this case I should check it.

I will start working on applying the test files to 10.2-spider now
I was mostly confused by that in the current 10.2 the test and result
files doesn't match.

>> I can't figure out,why we get the above warnings.
>> This is from a patch we discussed at booking.com one year ago.  Any
>> explanation for the above warnings would be appreciated.
>>
>> You can branch 10.2-spider and check the current state.
>
> O.K. I'll check it.

Please send an email when you have figured out why we get the new
warnings. This looks very strange.

Here is the executed code with warnings:
mysql-test-run spider/handler.basic_sql

CREATE TABLE ta_l (
PRIMARY KEY(a)
) ENGINE=Spider DEFAULT CHARSET=utf8 COMMENT='database
"auto_test_remote", table "ta_r"'
CONNECTION='host "localhost", socket
"/home/my/maria-10.2-spider/mysql-test/var/tmp/mysqld.2.1.sock", user
"root",
password ""'
IGNORE SELECT a, b, c FROM tb_l;
Warnings:
Warning1062Duplicate entry '1' for key 'PRIMARY'
Warning1062Duplicate entry '2' for key 'PRIMARY'
Warning1062Duplicate entry '3' for key 'PRIMARY'
Warning1062Duplicate entry '4' for key 'PRIMARY'
Warning1062Duplicate entry '5' for key 'PRIMARY

But when we do a select from the table, it looks like all rows are there.

Is this because the table ta_l is connected to a remote table trough
spider that already has the
rows in it ?
If yes, then it would be good to have a comment about this is in the
test and also insert
data that is different from the original one to make this part clear.

Regards,
Monty

___
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] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Wed, Nov 23, 2016 at 4:31 PM, kentoku  wrote:
> Hi Monty!
>
> Please see comment at 2016-05-07 19:15 for patches for MariaDB 10.2.
>
>> I don't think this it will work removing the usage of p_elem->connect_string
>>
>> This is because each partition may have a different connect string.
>
> I understand this behavior. But  I don't think this overwriting is a
> good idea.

The problem is that your patch causes a usage case of partitioning and
federated_x to fail, which is proven by the usage case:

fedarated_partion.test

We can't add a patch that will cause current correct usage of
partition and federated_x
to fail.

> Because when an user writes both table's connect string and
> partition's connect string, table's connect string is lost.

In which case is it lost ?
Can you add a test to federated_partition.test that shows in which case the
connection information is lost?

> This behavior causes a confusion of an user.

I don't see how you can use federated_x and partition without having a different
connect string for each partition.  I also don't see how this is
confusing for the end user.

One of the original ideas with the partition engine is that each
partition can have options
that are different from the other partitions. For example, one
partition could be with InnoDB, another with MyISAM.  For this to
work, there needs to exist a mechanism for specifing options per
partition, which the federated engine is indirectly using.

> In the other hand, Spider can read both table's connect string and
> partition's connect string without overwriting.

Where should the connection strings for each partition be stored?
this needs to work both with Spider but also with other usage of the
partitioning engine.

> Spider uses table's
> connect string for common settings and partition's connect string for
> partition specific settings.
> This is why this patch removes overwriting logic of connect string.

As far as I saw, the patch did remove all storing and reading of the
connect strings for the partitions which causes the test to fail.

Where does spider store the tables connect strings ?
How do you suggest this should work when spider is not used?
Can you create a patch that will not cause federated_partiton.test to fail?

>> +#define PLUGIN_VAR_CAN_MEMALLOC
>> +
>>  #ifndef MYSQL_CLIENT
>>
>> The above is not needed, as all code that is testing this is doing:
>
> Please move it under "#define SPIDER_HEX_VERSION" in
> "storage/spider/spd_include.h" for now. This is checked in Spider
> code.
>
>> #if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 10
>>
>> Which is always true in MariaDB 10.x
>
> Is this in Spider code, right?
Yes

> This is needed for supporting other versions. Spider still supports MySQL 5.5.

So you mean that PLUGIN_VAR_CAN_MEMALLOC is only needed for MySQL?

Looking at the code in spd_param.cc, there is no need to have
PLUGIN_VAR_CAN_MEMALLOC

Better to instead test MYSQL versions than have a define that is
always declared.
With current code, the last #else in spd_param.cc will never be used:

#ifdef PLUGIN_VAR_CAN_MEMALLOC
static MYSQL_SYSVAR_STR(
  remote_access_charset,
  spider_remote_access_charset,
  PLUGIN_VAR_MEMALLOC |
  PLUGIN_VAR_RQCMDARG,
  "Set remote access charset at connecting for improvement performance
of connection if you know",
  NULL,
  NULL,
  NULL
);
#else

dead code

Regards,
Monty

___
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] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff

+++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc2015-10-14
01:48:53.392665313 +0900
@@ -327,7 +327,9 @@ void ha_partition::init_handler_variable
   m_file_buffer= NULL;
   m_name_buffer_ptr= NULL;
   m_engine_array= NULL;
+/*
   m_connect_string= NULL;
+*/
   m_file= NULL;
   m_file_tot_parts= 0;
   m_reorged_file= NULL;
@@ -1516,4 +1518,6 @@ int ha_partition::prepare_new_partition(
   if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem)))
 goto error_create;

+/*
   tbl->s->connect_string = p_elem->connect_string;
+*/


I don't think this it will work removing the usage of p_elem->connect_string

This is because each partition may have a different connect string.

Here is an example from fedarated_partion.test:

eval create table t1 (s1 int primary key) engine=federated
  partition by list (s1)
  (partition p1 values in (1,3)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1',
   partition p2 values in (2,4)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2');

The above works in mariadb 10.2 but not in your spider tree.

>From this patch I will, for now, only take the code related to

HA_EXTRA_WRITE_CAN_REPLACE
HA_EXTRA_WRITE_CANNOT_REPLACE

--- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_plugin.cc
2015-10-13 23:49:10.188129839 +0900
+++ ./002_mariadb-10.1.8-spider/sql/sql_plugin.cc2015-10-14
01:48:54.296665317 +0900
@@ -2757,6 +2757,7 @@ static void update_func_str(THD *thd, st
   *(char**) tgt= my_strdup(value, MYF(0));
 else
   *(char**) tgt= 0;
-my_free(old);
+if (old)
+  my_free(old);

As my_free is safe to call with NULL, the above is not needed

diff -Narup ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h
./002_mariadb-10.1.8-spider/sql/sql_priv.h
--- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h
2015-10-13 23:49:10.189129839 +0900
+++ ./002_mariadb-10.1.8-spider/sql/sql_priv.h2015-10-14
01:48:54.642665315 +0900
@@ -27,6 +27,8 @@
 #ifndef SQL_PRIV_INCLUDED
 #define SQL_PRIV_INCLUDED

+#define PLUGIN_VAR_CAN_MEMALLOC
+
 #ifndef MYSQL_CLIENT

The above is not needed, as all code that is testing this is doing:

#if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 10

Which is always true in MariaDB 10.x

Regards,
Monty

___
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

[Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

I have now started to work on the spider patches for MariaDB 10.2, MDEV-7698.

I have moved all code from maria-10.1-spider to a new branch
maria-10.2-spider and added some more patches.
I have closed all related MDEV's in MDEV-7698 that is now included in
10.2-spider.

While doing this, I noticed that spider/handler test was not included
in the test suite. I added the missing suite.pm and suite.opt files
and got the tests to work.

However, when I tried to run test to verify my changes, I noticed that
a lot of test in spider/handler where failing:

mysql-test-run --suite=spider/handler
produces these failures:

spider/handler.spider3_fixes spider/handler.direct_aggregate
spider/handler.direct_update spider/handler.spider_fixes
spider/handler.function spider/handler.ha spider/handler.vp_fixes

All failures are because .test and .result file doesn't match.

I checked the patch file:
http://spiderformysql.com/downloads/spider-3.2/patch_mariadb-10.1.8.tgz
but this doesn't include any updates to the handler test files:
grep mysql_test *  returns nothing.

However the .tar file:
http://spiderformysql.com/downloads/spider-3.2/mariadb-10.1.8-spider-3.2-vp-1.1.tgz
Contains a lot of updated .test and .result files.

Kentoku, do you have patches for the test files, or should I just take
them from the above spider branch or from somewhere else ?

Another question:
After applying the patches:

013_mariadb-10.0.15.vp_handler.diff
034_mariadb-10.0.15.vp_handler2.diff
005_mariadb-10.0.15.hs.diff
041_mariadb-10.0.15.vp_handler2.diff

I get the following change in spider/handler/basic_sql.result:

--- a/storage/spider/mysql-test/spider/handler/r/basic_sql.result
+++ b/storage/spider/mysql-test/spider/handler/r/basic_sql.result
@@ -70,6 +70,12 @@ CREATE TABLE ta_l (
 PRIMARY KEY(a)
 ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT_2_1
 IGNORE SELECT a, b, c FROM tb_l
+Warnings:
+Warning1062Duplicate entry '1' for key 'PRIMARY'
+Warning1062Duplicate entry '2' for key 'PRIMARY'
+Warning1062Duplicate entry '3' for key 'PRIMARY'
+Warning1062Duplicate entry '4' for key 'PRIMARY'
+Warning1062Duplicate entry '5' for key 'PRIMARY'

I can't figure out,why we get the above warnings.
This is from a patch we discussed at booking.com one year ago.  Any
explanation for the above warnings would be appreciated.

You can branch 10.2-spider and check the current state.

Regards,
Monty

___
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: Fwd: [Commits] 8cd4778: MDEV-9114: Bulk operations (Array binding)

[Michael Widenius]

Hi!

On Sun, Oct 9, 2016 at 1:45 AM, Andrea <mon...@askmonty.org> wrote:
>
>
>
> --- Forwarded message ---
> From: Oleksandr Byelkin <sa...@montyprogram.com>
> Date: 8 October 2016 1:01:58 p.m.
> Subject: REVIEW: Fwd: [Commits] 8cd4778: MDEV-9114: Bulk operations (Array
> binding)
> To: Michael Widenius <mo...@mariadb.com>
>
> Hi, Monty!
>
>
> It is the patch. There left 2 "TODO" marks about embedded I think that
> best variant leave there it as is, but maybe you have other opinion.

Embedded is ok to not have bulk insert (for now), but we should give
an error, not an assert if this is used!

Review of bulk operations patch


> diff --git a/include/mysql.h.pp b/include/mysql.h.pp
> index 857f5b9..c985792 100644
> --- a/include/mysql.h.pp
> +++ b/include/mysql.h.pp
> @@ -11,11 +11,17 @@ enum enum_server_command
>COM_STMT_RESET, COM_SET_OPTION, COM_STMT_FETCH, COM_DAEMON,
>COM_MDB_GAP_BEG,
>COM_MDB_GAP_END=250,
> -  COM_SLAVE_WORKER,
> -  COM_SLAVE_IO,
> -  COM_SLAVE_SQL,
> -  COM_MULTI,
> -  COM_END
> +  COM_SLAVE_WORKER=251,
> +  COM_SLAVE_IO=252,
> +  COM_SLAVE_SQL=253,
> +  COM_MULTI=254,
> +  COM_END=255
> +};

Why the numbering ?
(Just curious as this shouldn't be needed)

> +enum enum_indicator_type
> +{
> +  STMT_INDICATOR_NONE= 0,
> +  STMT_INDICATOR_NULL,
> +  STMT_INDICATOR_DEFAULT
>  };

Please add comment what this enum is used for.
Note that this enum_indicator_type isn't used in this commit.
(I have a fix for this later on)
You should probably just rename it to indicator_type.
(On can always use 'enum indicator_type' if one wants to point out in the code
that it's an enum).


> diff --git a/sql/item.cc b/sql/item.cc
> index 61635ea..41a7aaf 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -812,6 +818,13 @@ bool Item_ident::collect_outer_ref_processor(void *param)
>return FALSE;
>  }
>
> +void Item_ident::set_default_value_target(Item *item)
> +{
> +  if ((default_value_target= item) && fixed &&

Please add a comment under which circumstances fixed could be false.
Also add a comment what it means when set_default_value_source isn't called.
Will it be called later or isn't it needed ?

> +  type() == FIELD_ITEM)
> +default_value_target->set_default_value_source(((Item_field *)this)->
> +   field);
> +}


> @@ -7242,6 +7300,7 @@ bool Item_ref::fix_fields(THD *thd, Item **reference)
>  Item_field* fld;
>  if (!(fld= new (thd->mem_root) Item_field(thd, from_field)))
>goto error;

> +fld->set_default_value_target(default_value_target);

Add a comment like: /* Note that fld->fixed isn't set here */


> @@ -8428,36 +8487,38 @@ bool Item_default_value::eq(const Item *item, bool 
> binary_cmp) const
>  bool Item_default_value::fix_fields(THD *thd, Item **items)
>  {
>Item *real_arg;
> -  Item_field *field_arg;
>Field *def_field;
>DBUG_ASSERT(fixed == 0);
>
> -  if (!arg)
> +  if (!arg && !arg_fld)
>{
>  fixed= 1;
>  return FALSE;
>}
> -  if (!arg->fixed && arg->fix_fields(thd, ))
> -goto error;
> +  if (arg)
> +  {
> +if (!arg->fixed && arg->fix_fields(thd, ))
> +  goto error;
>
>
> -  real_arg= arg->real_item();
> -  if (real_arg->type() != FIELD_ITEM)
> -  {
> -my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0), arg->name);
> -goto error;
> -  }
> +real_arg= arg->real_item();
> +if (real_arg->type() != FIELD_ITEM)
> +{
> +  my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0), arg->name);
> +  goto error;
> +}
>
> -  field_arg= (Item_field *)real_arg;
> -  if ((field_arg->field->flags & NO_DEFAULT_VALUE_FLAG))
> +arg_fld= ((Item_field *)real_arg)->field;
> +  }


What is the reason from having arg_field (type field) instead of
field_arg (type Item_field) ?

arg_field is a bad name for a class variable as it doesn't tell us
what the variable actually contains. A variable name in a function is
not that critical to name right as one can understand what it contains
by looking at is usage.  For a class variable this isn't the case.

field_in_table or just field would be a bit more clear (but not yet perfect).

I looked at the useage arg_field and don't understand why this needs to be
part of the class and not just a local variable.
As def_field is basically a copy of def_field, then another possible name
for arg_fld would be def_field_org.

> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -1872,6 +1872,9 @@ class Item: public Value_source,
>{
>  marker &= ~EXTRACTION_MASK;
>}
> +
>

Re: [Maria-developers] [Commits] 49b2502: Fix assertion/hang in read_init_file()

[Michael Widenius]

Hi!



> Thanks for the patch and for finding the bug!
>
> However, Serg already removed the thread count handling from
> do_handle_bootstrap.
> I will today check if the problem still exists and if yes, fix it by
> always sending the broadcast. (A quick look suggest it still exist).

Just noticed that you had pushed your patch into the main 10.2 tree.
I checked it and think that's the right way to fix this.  Thanks also
for the great
comment in the patch that explains it!

I also agree that's a bit strange that read_init_file is using the
bootstrap() to read the
init file.  This is probably perfectly safe, but it would have been
good if the comment for the bootstrap() function would have said so!
Will add this next time I touch the code in this area.

Regards,
Monty

___
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] [Commits] 49b2502: Fix assertion/hang in read_init_file()

[Michael Widenius]

Hi!

On Fri, Sep 9, 2016 at 7:31 PM, Kristian Nielsen
 wrote:
> Monty,
>
> My latest push uncovered a problem in the 10.2 tree; however the problem was
> there before, just not triggered by the testsuite. I pushed the below patch
> to fix the test failures in Buildbot, but I am not sure it is the correct
> solution, so please check it.
>
> It appears the problem was introduced with this patch:
>
> commit 3d4a7390c1a94ef6e07b04b52ea94a95878cda1b
> Author: Monty 
> Date:   Mon Feb 1 12:45:39 2016 +0200
>
> MDEV-6150 Speed up connection speed by moving creation of THD to new 
> thread
>
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 84f0c63..355f62d 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -844,12 +844,13 @@ void do_handle_bootstrap(THD *thd)
>delete thd;
>
>  #ifndef EMBEDDED_LIBRARY
> -  thread_safe_decrement32(_count);
> +  DBUG_ASSERT(thread_count == 1);
>in_bootstrap= FALSE;
> -
> -  mysql_mutex_lock(_thread_count);
> -  mysql_cond_broadcast(_thread_count);
> -  mysql_mutex_unlock(_thread_count);
> +  /*
> +dec_thread_count will signal bootstrap() function that we have ended as
> +thread_count will become 0.
> +  */
> +  dec_thread_count();
>my_thread_end();
>pthread_exit(0);
>  #endif
>
> The problem is that do_handle_bootstrap is not only used for bootstrap - it
> is also used for read_init_file(). In the latter case, it is _not_
> guaranteed that thread_count will drop to zero. For example, the binlog
> background thread might be running. You can see this in 10.2 (before my
> just-pushed fix) by running

>   ./mtr main.init_file --mysqld=--log-bin=mysql-bin
>
> It will assert, or hang if assert is disabled.
>
> To get rid of the failures in Buildbot, I pushed the below patch. I think
> the patch is perfectly safe. However, maybe there is a better way - it seems
> a bit messy, mixing bootstrap and read_init_file() and having these
> if (opt_bootstrap) conditions. And maybe there are other problems caused by
> this as well that I did not notice? As I am not much familiar with how this
> thread count checking / bootstrap / init file handling works.

Thanks for the patch and for finding the bug!

However, Serg already removed the thread count handling from
do_handle_bootstrap.
I will today check if the problem still exists and if yes, fix it by
always sending the broadcast. (A quick look suggest it still exist).

Regards,
Monty

___
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] Problem with parallel replication in 10.2

[Michael Widenius]

Hi!

>> Is it possible for you to create a clean patch for async deadlock for
>> bb-10.2-jan
>> that Jan and I can review and apply?
>
> Yes, I'll prepare a patch for review. It's probably easier if I apply it
> myself, as I want to do a minimal patch in 10.1 to get optimistic parallel
> replication working with TokuDB; and a full patch in 10.2 which includes the
> InnoDB simplifications.

It would be great if you could do the above!
Feel free to push this into the bb-10.2-jan tree or into the 10.2 tree
when Jan is ready with his merge!

Regards,
Monty

___
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] Problem with parallel replication in 10.2

[Michael Widenius]

Hi!



> 2016-09-01 10:33:20 140078976283392 [ERROR] Slave SQL: Error during XID 
> COMMIT: failed to update GTID state in mysql.gtid_slave_pos: 1062: Duplicate 
> entry '0-53' for key 'PRIMARY', Gtid 0-1-52, Internal MariaDB error code: 1942
>
> This happens because the mysql.gtid_slave_pos table is MyISAM (which is
> default in mysql-test-run, but not in the normal server install), and
> parallel replication needs to roll back a transaction after it has updated
> the table. Because of MyISAM, the gtid_slave_pos change cannot be rolled
> back.

Sorry about the myisam part. I did say in #maria at once after I sent
the email that the problem with 10.2 was wrong used engine, but
apparently you missed that.

> Maybe parallel replication could in this case manually undo its change in
> the table as part of the rollback. It's just a DELETE of the row previously
> inserted.

That could be a solution.  In any case, I should at least look at
adding a better error message if this happens.

> In any case, currently the fix is to use InnoDB for the table:
>
> --- rpl_skr.test~   2016-09-01 10:27:21.214633498 +0200
> +++ rpl_skr.test2016-09-01 10:35:50.660242337 +0200
> @@ -8,6 +8,9 @@
>  --connection server_2
>  SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads;
>  --source include/stop_slave.inc
> +SET sql_log_bin=0;
> +ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB;
> +SET sql_log_bin=1;
>  SET GLOBAL slave_parallel_threads=10;
>  SET GLOBAL slave_parallel_mode='conservative';
>  --source include/start_slave.inc

Yes, same fix that I did.

>> bb-10.2-jan tree is a working tree for a merge of MariaDB 10.2 and MySQL 5.7
>>
>> When running rpl_skr in 10.2 it takes 2 seconds
>> When running it in the bb-10.2-jan tree it takes either  a long time
>> or we get a timeout.
>
> This is because of errorneous merge. The original code:
>
> if (waitee_buf_ptr) {
> lock_report_waiters_to_mysql(waitee_buf_ptr,
>  start_mysql_thd,
>  victim_trx_id);
>
> The bb-10.2-jan code:
>
> if (victim_trx && waitee_buf_ptr) {
> lock_report_waiters_to_mysql(waitee_buf_ptr,
>  start_mysql_thd,
>  victim_trx->id);
>
> So if victim_trx is NULL the waits are not reported to parallel replication
> at all, causing the stalls and/or hangs. victim_trx is NULL unless InnoDB
> itself detects a deadlock.
>
> I've attached a patch that fixes this, can also be pulled from here:
>
>   https://github.com/knielsen/server/commits/montyrpl
>
> Or should I push it directly into bb-10.2-jan? This makes the rpl_skr.test
> complete correctly in < 1 second.

Thanks a lot for the patch!
Jani will pull it into his working tree

>> This is probably because of the new lock code in lock0lock.cc and
>> lock0wait.cc which doesn't break conflicting transaction but instead
>> waits for a timeout
>
> The merge appears very rough. Shouldn't the waitee_buf be integrated into
> the new DeadlockChecker class? Why is it necessary to thd_report_wait_for()
> on internal transactions like here?
>
> /* m_trx->mysql_thd is NULL if it's an internal trx. So current_thd is 
> used */
> if (err == DB_LOCK_WAIT) {
> ut_ad(wait_for && wait_for->trx);
> wait_for->trx->abort_type = TRX_REPLICATION_ABORT;
> thd_report_wait_for(current_thd, wait_for->trx->mysql_thd);
> wait_for->trx->abort_type = TRX_SERVER_ABORT;
> }
> return(err);
>
> Maybe I should try to write a better patch for integrating this in the new
> InnoDB code.

It would be great if you could help Jan with a better patch!
He still has a lot of merge work to do and the whole server team is
waiting on Jan to be ready so that we can add the final touches and
release MariaDB 10.2-beta.

> What do you think about changing this to use the async deadlock kill in
> background thread, as discussed in this thread?
>
>   https://lists.launchpad.net/maria-developers/msg09902.html
>
> This would allow to simplify the code in lock0lock.cc, and avoid the locking
> hacks in innobase_kill_query()?

I was trying to find the exact patch/patches you are referring to.

https://github.com/knielsen/server/commit/841ada8c8ac39c024cd1eafe4b346deecbe48ca3

https://github.com/knielsen/server/commit/b256733df2cf9f10d38e44ca4979843a3b0d1884

Is it possible for you to create a clean patch for async deadlock for
bb-10.2-jan
that Jan and I can review and apply?

Regards and thanks,
Monty

___
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

[Maria-developers] Problem with parallel replication in 10.2

[Michael Widenius]

Hi!

I was trying to run a test that fails in the upcoming bb-10.2-jan on
the normal 10.2 tree, when I noticed this strange issue:

- Test fails with timeout when running with --debug
- When looking at the trace file, I notice that we get a duplicate key
error for the table gtid_slave_post (MyISAM table).  Is this something
normal ?

To repeat:

Store the included test in suite/rpl and run it with:
mysql-test-run --debug --record rpl_skr

Issue 2:

bb-10.2-jan tree is a working tree for a merge of MariaDB 10.2 and MySQL 5.7

When running rpl_skr in 10.2 it takes 2 seconds
When running it in the bb-10.2-jan tree it takes either  a long time
or we get a timeout.

This is probably because of the new lock code in lock0lock.cc and
lock0wait.cc which doesn't break conflicting transaction but instead
waits for a timeout

Would appreciate any help with this!

Note that rpl_skr is a test that was originally part of
rpl_parallel.test, but I have made it separate to be able to more
easily test for this issue.

Before testing bb-10.2-jan, please apply this patch that fixes one
critical issue in this tree related to group-commit:

diff --git a/storage/innobase/handler/ha_innodb.cc
b/storage/innobase/handler/ha_innodb.cc
index b69468d..0e9caed 100644
--- a/storage/innobase/handler/ha_innodb.cc
+++ b/storage/innobase/handler/ha_innodb.cc
@@ -4976,16 +4953,15 @@ innobase_commit(
 thd_wakeup_subsequent_commits(thd, 0);

 /* Now do a write + flush of logs. */
-if (!read_only) {
-trx_commit_complete_for_mysql(trx);
-}

+trx_commit_complete_for_mysql(trx);
 trx_deregister_from_2pc(trx);

 } else {
 /* We just mark the SQL statement ended and do not do a
 transaction commit */

+   DBUG_PRINT("info", ("Just mark SQL statement"));
 /* If we had reserved the auto-inc lock for some
 table in this SQL statement we release it now */

Regards,
Monty


rpl_skr.test
Description: Binary data
___
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] Patch for parallel replication DBUG_ASSERT check on DROP TABLE et al

[Michael Widenius]

Hi!

On Thu, Aug 18, 2016 at 11:03 PM, Kristian Nielsen
<kniel...@knielsen-hq.org> wrote:
> Michael Widenius <michael.widen...@gmail.com> writes:
>
>> Do you happen to know of any failures caused by the patch?
>
> As I mentioned on IRC, the assertion triggers during rpl_mdev6020, for
> example here:
>
>   
> https://buildbot.askmonty.org/buildbot/builders/work-amd64-valgrind/builds/9199/steps/test/logs/stdio
>
> As we discussed, one possible reason is a race because did_mark_start_commit
> is set only after waking up the next thread.

Just a note about rpl_mdev6020

This test started to fail with a timeout, as some parallel threads are
waiting on each-other, between MariaDB 10.0.17 and MariaDB 10.0.18

To get it to fail, one must run with valgrind:
mysql-test-run --valgrind rpl_mdev6020,innodb_plugin,mix

Now working on trying to see what changes caused it to fail.

Regards,
Monty

___
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] Patch for parallel replication DBUG_ASSERT check on DROP TABLE et al

[Michael Widenius]

Hi!

On Thu, Aug 18, 2016 at 12:24 PM, Kristian Nielsen
 wrote:
> Monty,
>
> Apparently you pushed this patch into 10.0, even though I explained that it
> is incorrect, and why. That's not cool, and you can even see it failing in
> Buildbot now.

I pushed the patch as I didn't see (probably missed) a review from you
for more than a day.  I was also going away for a few days and I
wanted that Elena would have my code in 10.0 while she was testing
things that could trigger the assert.

As this was a DBUG_ASSERT and could not cause a problem for anyone in
production I didn't think it was totally critical to push it before
the review.

> Can you please fix it ASAP?

Of course. Still I don't know of any case in buildbot where the patch
has caused any issues.  I checked with Elena and she couldn't find
anything either that she could attribute to the patch.

Do you happen to know of any failures caused by the patch?

Regards,
Monty

___
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] Joinable threads

[Michael Widenius]

Hi!

On 5 May 2016 17:42, "Sergey Vojtovich"  wrote:
>
> Hi Monty,
>
> Thanks for you reply. Please allow me to disagree with some points. :)
>
> Attached please find 2 patches, both fix this memory leak. One approach is
> for detached threads, another one is for joinable threads.

Sorry, I missed the part about service threads. For these I don't mind to
use joinable threads.

Just one question:
How to handle hanged threads?
With the old code we could, if this would ever be a problem, just change
cond_wait to cond_timed_wait.
Don't know how to do this with joinable threads, except using alarms which
is a bit of code (20 lines)

Regards,
Monty
___
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] Joinable threads

[Michael Widenius]

Hi!

On Wed, May 4, 2016 at 11:13 AM, Sergey Vojtovich  wrote:
> Hi Monty,
>
> I vaguely recall you told me once that you like detached threads more than
> joinable, specifically for service threads. Could you remind me what were
> the reasons behind it?
>
> I need this to fix https://jira.mariadb.org/browse/MDEV-9994 properly.

The main reason is just that with joinable threads you have more work
to manage the threads as you need to remember to join them as
otherwise you will get a memory loss.

The only time the server has a benefit of join able threads, at least
as far as I am aware of it, is at shutdown, which hasn't been a real
problem as in most cases
my_thread_end() will ensure that all threads has been calling
my_thread_end() before the server dies.

Referring to the valgrind issue, I would just ignore the memory loss
for tls, as this isn't a real error.  We are already ignoring this on
other platforms.

Regards,
Monty

___
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] Deadlock with STOP SLAVE and LOCK_active_mi

[Michael Widenius]

Hi!

On Mon, Apr 25, 2016 at 1:18 PM, Kristian Nielsen
<kniel...@knielsen-hq.org> wrote:
> Michael Widenius <michael.widen...@gmail.com> writes:
>
>>> 2. To protect access to master_info_index, eg. to prevent a Master_info from
>>> disappearing while it is accessed.
>>
>> When reading about this, I think it would be better to have a counter
>> in Master_info if someone is using it and only delete if if the
>> counter is zero. Would be trivial to add an increment of the counter
>> in get_master_info().
>>
>> The counter could either be protected by the LOCK_active_mi or one of
>> the mutexes in Master_info like data_lock.
>
> Sounds reasonable. Will you do a patch? I guess this is code from the
> multisource feature, so you are the one familiar with it (before
> multi-source, the Master_info could not be created and deleted dynamically,
> could it?)

I can start by doing a patch for protecting get_master_info.
Will do that tomorrow.

Regards,
Monty

___
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] Deadlock with STOP SLAVE and LOCK_active_mi

[Michael Widenius]

Hi!

>>> Any suggestions for how this is supposed to work? Or is it just broken
>>> by design, but saved because normally slave threads do not need to
>>> access SHOW STATUS or system variables?



> Hm. So I went through all of the code that references LOCK_active_mi to try
> and understand this. It seems there are two main uses:
>
> 1. As you say, to serialise admin commands. I think the list is: end_slave()
> (server shutdown); CHANGE MASTER; START/STOP SLAVE; START/STOP ALL SLAVES;
> RESET SLAVE; FLUSH SLAVE.
>
> 2. To protect access to master_info_index, eg. to prevent a Master_info from
> disappearing while it is accessed.

When reading about this, I think it would be better to have a counter
in Master_info if someone is using it and only delete if if the
counter is zero. Would be trivial to add an increment of the counter
in get_master_info().

The counter could either be protected by the LOCK_active_mi or one of
the mutexes in Master_info like data_lock.

> There is a comment in rpl_rli.h that LOCK_active_mi protects
> Relay_log_info::inited, but I did not understand it - maybe it is wrong?

Setting inited is indirectly protected by LOCK_active_mi, as
init_all_master_info() which calls init_master_info(), is protected by
LOCK_active_mi.

Looking at the code, I didn't however see any need to protect inited
as this is an internal
flag that is always 1 after start. It's main usage is to avoid some
re-initialization of relay logs on retries.

> So the idea would be to make a new lock for (1), and keep LOCK_active_mi for
> (2).
>
> Actually, using a lock for (1) seems a bad idea - STOP SLAVE can take a long
> time to run, and a mutex makes it unkillable. A condition variable might be
> better (to make STOP SLAVE killable). But I guess that is a separate issue,
> it would not affect the possibility of deadlocks.

Agree that a condition variable would be better, but not critical as
one has already a problem if stop_slave doesn't work.

> Both LOCK_active_mi and the new LOCK_serialize_replication_admin_commands
> must by themselves prevent master_info_index from having elements added or
> removed. This is needed for start_all_slaves() and stop_all_slaves() to work
> correctly, I think.

Why both?
Isn't it enough to first take LOCK_serialize_replication_admin_commands and
then take LOCK_active_mi if one needs to access master_info_index ?

> Something will need to be done for remove_master_info(). Currently, it also
> tries to stop slave threads (from within free_key_master_info()) while
> holding LOCK_active_mi. It seems that threads should be stopped before
> attempting to remove their mi from master_info_index, probably?

To do that we would need to:
- Add a flag in Master_info that it's not usable anymore and change
this flag only under LOCK_active_mi.  get_master_info() could wait (on
a condition) if this flag is set.
- Add a counter that Master_info is in use.
- Add a function to release Master_info.
- Call terminate_slave_threads() outside of free_key_master_info()

> Actually, there would still be the deadlock problem after introducing the
> new lock, because it is possible for a slave thread to run CHANGE MASTER!
> (and I think START SLAVE / RESET SLAVE as well). But that is probably a bug,
> does not seem good that this is possible. I guess these need a check to
> throw an ER_LOCK_OR_ACTIVE_TRANSACTION error, like STOP SLAVE.

Wouldn't the new lock LOCK_serialize_replication_admin_commands fix that?

> I think I also found another lock problem while reading the code, for
> MASTER_POS_WAIT() og SHOW BINLOG EVENTS. They both grab a Master_info under
> LOCK_active_mi, but then proceed to access it after releasing the lock. I do
> not see anything that prevents the Master_info to disappear under their feet
> if RESET SLAVE ALL is run in another thread? But again, that's a separate
> issue, I suppose. Probably there are some other tricky deadlock issues
> lurking here.

I checked the function mysql_show_binlog_events() but could not find
any access to
Master_info after mysql_mutex_unlock(_active_mi) was released.

> I don't know. It seems splitting into a new
> LOCK_serialize_replication_admin_commands could solve the deadlock issue,
> maybe, if the other problems mentioned above are addressed. It seems very
> hard to convince oneself that this would not introduce new problems
> somewhere, the locking in replication is really complex and feels very
> fragile :-/ It doesn't really feel like something one would want to do in
> 10.1 (certainly not 10.0), but maybe 10.2?
>
> It's kind of a very edge-case problem (and seems to have been there for a
> _long_ time). Still, hanging the mysqld process hard, requiring kill -9, is
> also not nice...

Adding two new flags, one if master_info is in use and one if it's
unusable, shouldn't be
that hard to make reasonable safe in 10.1

I am now back in Finland and working. Feel free to call me to discuss
this any time.

Regards,
Monty

Re: [Maria-developers] A proposal to deprecate syntax: SELECT a'test'

[Michael Widenius]

Hi!

On 25 Feb 2016 19:16, "Alexander Barkov"  wrote:

>> As ' and " are identical in MySQL mode, I don't see how you can fix
>> this without causing even more confusion.

I want to emphasise the above, as you haven't really understood the
implications of this.
This, in addition to that we support that one can use a string as an alias
makes things a bit tricky to fix in a compatible way.

> I only propose to change the behavior for the SELECT expression aliases,
> not to all strings generally.

As I understand it, you propose to change things in the following ways:

From:
- aliases can be identifiers or strings
To:
- aliases has to be identifiers or double quoted strings when used without
AS. When used with AS they can be identifiers or single or double quoted
strings.

I don't think there is any clear logic for the end user to understand why
we would do this.

>>> What can be used instead
>>> 
>>> We have a number of other ways to specify aliases:
>>>
>>> The standard ways:
>>>
>>> SELECT a AS test;
>>> SELECT a "test";
>>> SELECT a AS "test";
>>
>>
>> Ok, this answers my question above. The standard says that an alias is
>> a text string.

> Sorry, no. The standard says that an alias is an identifier.
> Text strings are not valid aliases.

Sorry, I was not clear.
In MySQL mode " is a text string. If we want to support the above
statements , we also have to support using ' for aliases.
The other option would be to say that one must use back tick for aliases,
as that's the equal to ansi sql ".

> I only propose to deprecate string literals (i.e. single quoted strings)

In MariaDB, a string literal starts with ' or ". If your are deprecating
one in some context you have also to depricate the other.

> as SELECT expression aliases, because this is a non-standard and
> harmful  extension, which conflicts with another standard syntax assumed
> for typified literals.

To make things clear. There are 3 non standard things we are talking about
here:
- Both ' and " are string delimiters.
- quoting of identifiers are done with back tick.
- Aliases can be given with identifiers or strings.

You can't depricate something in one context and keep it in another
context. That's not consistent. Either ' and " are string delimiterx or
they are not. They should not be something different depending on context.

>>
>>> MySQL/MariaDB extensions:
>>>
>>>
>>> SELECT a `test`;
>>> SELECT a AS `test`;
>>>
>>> That should be enough.
>>
>>
>> Note that this has nothing exclusively to do with alias, but with the
>> MySQL/MariaDB extension that you can use ' instead of " as a string
>> delimiter.
>
>
> My proposal is exclusively about aliases, not about using of " as a
> string delimiter.

See above. You can't make a special case for aliases that breaks the
general rules.
Things gets to complex to describe.

>> This would break the specification of what a text strings is in
>> MariaDB.

> No, this will only change  what a SELECT expression alias is.

If your proposal would be to only accept identifiers as aliases, then we
shouldn't accept " either for aliases. You can't say that " is sometimes an
identifier and sometimes a string.

>> It would also be very confusing that text strings needs to be
>> specified differently in different places of the SQL syntax.
>
> You're reordering causes and consequences :)
>
> Again, this is not about text strings. It's about
> what can be used as an alias.

It's well defined what a text string is in MySQL mode. You can't change the
definition just for some part of the alias syntax.

> I think it will be very obvious:
>
> - Expressions support single-quoted and double-quoted strings
>
> - Aliases support regular identifiers, delimited (i.e. double-quoted)
>   identifiers and backtick-quoted identifiers.

We don't have double quoted identifiers in MySQL mode!
You CAN NOT change the meaning of " just for aliases.
Note also that depending on the syntax suggested, you would in some cases
allow strings but not in other cases.

> Expressions and aliases are instances of very different nature.

Syntax of things must be consistent. You should be able to look at any part
of a sql construct and know what the part is. In your proposal the meaning
of " depends of context, which is the wrong way to do it.

Your options are to allow one to specify aliases with either identifiers or
strings. You should not try to change the meaning of ' or " depending on
context.

> Expression is a value that has a data type and attributes.
> Alias is an identifier, nothing else.

If that's would be the case in MySQL mode, then you should not be allowed
to use " for aliases either.

> There is nothing common in them at all.
> It's quite obvious that they *can* have different syntactic rules.

Sorry, no. It's important to be consistent.

>>> 2. In 10.2 we disallow this syntax by default and add either sql_mode or
>>> old_mode to enable the old behavior.
>>
>>
>> As you can already get the 

Re: [Maria-developers] A proposal to deprecate syntax: SELECT a'test'

[Michael Widenius]

Hi!

>   Hello Monty, all,
>
>
> We discussed this with Sergei and both think that it will be a good idea
> to deprecate this syntax:
>
> SELECT a'test';
>
> where 'a' is an identifier and 'test' its alias.

Is this dependent on if there is a space or not between a and 'test' ?
Note that in MySQL and MariaDB, ' and " are both used for quoting
strings and I don't think we can change that without affecting a lot
of applications!

I think it's ok to require one to have a space between a and 'test',
if that would help at all.

> This is a non-standard way, and it conflicts with some other important
> standard SQL grammar. See below.

Note that in MySQL/MariaDB, when not using ANSI mode, ' and " are
equal and ` is used for identifiers, so this is expected to be
non-standard. For quoting identifiers ` should be used.

Of course, if you are using ANSI mode, then things should be as close
to ANSI as possible.

> Typified literals and a syntax conflict
> ---
>
> An identifier followed by a single-quoted text string is needed
> for typified literals:
>
> SELECT INET6'::';
>
> We already support this for temporal literals, which is a part of
> the SQL standard:
>
> SELECT TIME'10:10:10';
> SELECT TIMESTAMP'2001-01-01 10:20:30';
> SELECT DATE'2001-01-01';
>
> Note, the conflict already exists in MySQL and MariaDB.
>
>
> This script:
>
> DROP TABLE IF EXISTS t1;
> CREATE TABLE t1 (date date);
> INSERT INTO t1 VALUES ('2016-02-19');
> SELECT date'2001-01-01' FROM t1;
>
> returns:
>
> +--+
> | date'2001-01-01' |
> +--+
> | 2001-01-01   |
> +--+
>
> Notice, it returns a DATE literal, it does not return the value of
> the column t1.date using '2001-01-01' as an alias.

As ansi sql doesn't allow you to use date as a column name, this isn't
really as big problem as it may seam ;)

Anyway, I agree that this can be confusing, but so is having date as a
column namn.


> Notice the difference:
>
> SELECT date`2001-01-01` FROM t1;
> ++
> | 2001-01-01 |
> ++
> | 2016-02-19 |
> ++
>
> This is confusing.

Not really, as you are using different quotes and you get the expected answer.
This should be same as if you used " in ANSI SQL, isn't it?

> Shift-reduce conflicts
> --
> By the way, the fact that we support single-quoted string as an alias
> is the reason for shift-reduce conflicts in sql_yacc.yy, because
> TIME/DATE/TIMESTAMP followed by 'string' can be interpreted in two ways:
> - a column name followed by an alias
> - an SQL-standard time literal

What is the SQL-standard way to use an alias?
(Sorry, can't access the standard just now):

SELECT date "2001-01-01" FROM t1;
or
SELECT date '2001-01-01' FROM t1;

And if you execute both of these queries, will you not get the same
confusing result for one of them?

As ' and " are identical in MySQL mode, I don't see how you can fix
this without causing even more confusion.

> What can be used instead
> 
> We have a number of other ways to specify aliases:
>
> The standard ways:
>
> SELECT a AS test;
> SELECT a "test";
> SELECT a AS "test";

Ok, this answers my question above. The standard says that an alias is
a text string. In MySQL / MariaDB a text string can be specified with
both ' or ".
I don't think it's good idea to make alias a special case where you
have to specify
the text string only with ".

> MySQL/MariaDB extensions:
>
>
> SELECT a `test`;
> SELECT a AS `test`;
>
> That should be enough.

Note that this has nothing exclusively to do with alias, but with the
MySQL/MariaDB extension that you can use ' instead of " as a string
delimiter.

> Proposal
> 
> 1. In 10.1 we add a warning when a single-quoted string is used as a
> column alias. Something like this should work:
>
> Single quotes in a select sublist alias are deprecated. Use double
> quotes instead.

This would break the specification of what a text strings is in
MariaDB. It would also be very confusing that text strings needs to be
specified differently in different places of the SQL syntax.

> 2. In 10.2 we disallow this syntax by default and add either sql_mode or
> old_mode to enable the old behavior.

As you can already get the behavior you want by specifying ANSI MODE,
I don't think this change is necessary. I think it's also bad as it
makes it harder to define what a text string is.

For example:
In MariaDB, a text string can be delimited with ' or ", except in the
case of alias when you have to use '

I think it's better to require, as most ANSI SQL databases does, one
to quote column names like 'date' and 'time' to ensure they are not
mixed with operators or string prefixes.

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : 

Re: [Maria-developers] Likely a dead optimizer related code

[Michael Widenius]

Hi!

 Hi Igor,

 I noticed that during Item_cond::fix_fields() and
 Item_func_between::fix_fields() update these optimizer
 related SELECT_LEX members:

 thd-lex-current_select-cond_count and
 thd-lex-current_select-between_count

 The purpose of these members is to allocate optimizer related buffers
 in:
 - update_ref_and_keys() in sql_select.cc and
 - check_func_dependency() in opt_table_elimination,cc
 and it seems they have no any other purposes..

Correct.

  From my understanding, this is a dead code and the collected values are
 never used:

 - cond_count and_between_count are later always initialized to 0
 before the walk(count_sargable_conds) call in update_ref_and_keys().

And the call to check_func_dependency() is done after this call.

 - moreover, using the values collected during fix_fields() would not be
 correct to allocate optimized related buffers, because the code
 in fix_fields() does not distinguish between different query parts,
 so the items can be just a part of the SELECT list rather than a
 part of WHERE/HAVING/ON.

This would note be a problem as the count was supposed to be an upper bound.
As it's not normal that you have many IF or BETWEEN in the SELECT
part, this was not a problem.

I looked at the patch and it's ok to commit.

Regards,
Monty

___
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] Probably in a bug in simple_pred()

[Michael Widenius]

Hi!

 From: Alexander Barkov b...@mariadb.org

 Hi Sergey,

 I probably found a bug in simple_pred() in opt_sum.cc.

 In this piece of code:

   case 3:
 /* field BETWEEN const AND const */
 item= func_item-arguments()[0]-real_item();
 if (item-type() == Item::FIELD_ITEM)
 {
   args[0]= item;
   for (int i= 1 ; i = 2; i++)
   {
 item= func_item-arguments()[i]-real_item();
 if (!item-const_item())
   return 0;
 args[i]= item;
 if (check_item1_shorter_item2(args[0], args[1]))


 Should't it be

 if (check_item1_shorter_item2(args[0], args[i]))

 ?

   return 0;
   }
 }


 If this is a bug, can you please help to make an SQL scrip which would
 be affected by this bug?

yes, it's a bug.  Here is a script that shows that:

drop table if exists t1;
create table t1 (a varchar(10), key (a));
insert into t1 values(bar),(Cafe);
explain select min(a) from t1 where a between a and Cafe2;
explain select min(a) from t1 where a between a and
Cafee;
explain select min(a) from t1 where a between a
and Cafe2;

The output is:
 1 | SIMPLE  | NULL  | NULL | NULL  | NULL | NULL|
NULL | NULL | Select tables optimized away |

For the two first cases and:
|1 | SIMPLE  | t1| index | a | a| 13
| NULL |2 | Using where; Using index |

for the last
The second case should have the same output as the third.

Will fix and push into 10.1

Regards,
Monty

___
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-4119 patch comments

[Michael Widenius]

Hi!

On Mon, Jul 6, 2015 at 7:37 PM, Sergey Petrunia ser...@mariadb.com wrote:
 Hi Monty,

 Please find my comments on the patch for MDEV-4119 below. Sorry for the delay.

No problem, thanks for looking into this.

 +  @param split_flagsZero or more of the following flags
 + SPLIT_FUNC_SKIP_REGISTERED:
 +Function be must skipped for registered SUM
 +SUM items
 +SPLIT_FUNC_SELECT
 +We are called on the select level and have 
 to
 +register items operated on sum function
 Typo, it's not SPLIT_FUNC_SELECT, it is SPLIT_SUM_SELECT

Fixed

cut

 +If this is an item in the SELECT list then we also have to split out
 +all arguments to functions used together with the sum function.
 +For example in case of SELECT A*sum(B) we have to split out both
 +A and sum(B).
 +This is not needed for ORDER BY, GROUP BY or HAVING as all references
 +to items in the select list are already of type REF

I left the above as this was here to explain why we can not remove
SPLIT_FUNC_SELECT from any arguments.

cut

  void Item_cond::split_sum_func(THD *thd, Item **ref_pointer_array,
 -   ListItem fields)
 +   ListItem fields, uint flags)
  {
List_iteratorItem li(list);
Item *item;
while ((item= li++))
 -item-split_sum_func2(thd, ref_pointer_array, fields, li.ref(), TRUE);
 +item-split_sum_func2(thd, ref_pointer_array, fields, li.ref(),
 +  flags | SPLIT_SUM_SKIP_REGISTERED);

 Why not clear SPLIT_SUM_SELECT flag here?
 The Item_cond may be at the select list, but its parts are definitely not in
 the select list?

 The same applies to Item_row::split_sum_func and Item_func::split_sum_func.

We can't ever clear SPLIT_SUM_SELECT, which is what I tried to explain
in the above comment.

I noticed this in the following query, part of group_by.test

select a*sum(b) from t1 where a=1 group by c;

The first select item is Item_mul(Item_field(a), Item_sum(Item_field(b)))

We can't remove SPLIT_SUM_SELECT for the arguments to Item_mul, as in this
case Item_field(a) would not be converted to an Item_ref.
The end result is that 'a' would be calculated from the last row found in
the table, not for the last row matching the where or part of the group.

Forcing 'a' to be a ref, will ensure that 'a' will be stored in the
created temporary and when we access it when calculating a*sum(b) it
will have the correct value for the group.

So the rule is:

- If we are in the SELECT, we have to change all references to field
to Item_ref, as long as they are not inside a SUM() function.
- For ORDER BY, HAVING, GROUP BY it's not needed as all fields are
  automaticly converted to references as part of fix_fields()

cut

 +++ b/sql/item_func.cc
 @@ -419,11 +419,12 @@ Item *Item_func::compile(Item_analyzer analyzer, uchar 
 **arg_p,
  */

  void Item_func::split_sum_func(THD *thd, Item **ref_pointer_array,
 -   ListItem fields)
 +   ListItem fields, uint flags)
  {
Item **arg, **arg_end;
for (arg= args, arg_end= args+arg_count; arg != arg_end ; arg++)
 -(*arg)-split_sum_func2(thd, ref_pointer_array, fields, arg, TRUE);
 +(*arg)-split_sum_func2(thd, ref_pointer_array, fields, arg,
 +flags | SPLIT_SUM_SKIP_REGISTERED);
 Why not clear SPLIT_SUM_SELECT flag here? (same question as in 
 Item_cond::split_sum_func)

Same answer as above.

 +++ b/sql/sql_select.cc
 @@ -1944,8 +1954,9 @@ int JOIN::init_execution()
/*
  Enable LIMIT ROWS EXAMINED during query execution if:
  (1) This JOIN is the outermost query (not a subquery or derived table)
 -This ensures that the limit is enabled when actual execution 
 begins, and
 -not if a subquery is evaluated during optimization of the outer 
 query.
 +This ensures that the limit is enabled when actual execution begins,
 +and not if a subquery is evaluated during optimization of the outer
 +query.
  (2) This JOIN is not the result of a UNION. In this case do not apply 
 the
  limit in order to produce the partial query result stored in the
  UNION temp table.
 @@ -2562,8 +2573,12 @@ void JOIN::exec_inner()
skip_sort_order= 0;
  }
  bool made_call= false;
 +SQL_SELECT *select= join_tab[const_tables].select;
 This line causes a crash in a few tests:

Sorry about that; I had already fixed it shortly after I gave you the
original patch.  I fixed it by adding

if (order  join_tab)

Before the above row.

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More 

[Maria-developers] Enabling feedback pluging for MariaDB 10.1.4

[Michael Widenius]

Hi!

We had originally planned to enable the feedback plugin by default for
the Alpha and Beta stages of MariaDB 10.1, but we forgot to do this
for the alpha so I suggested Sergei today that we should enable it for
the beta period of MariaDB 10.0

We agreed that before doing this we would have a discussion on the
maria developer list to explain why we would like to do this.

The reason we would like to enable it is to get information about
which features people are testing and using in MariaDB 10.1, so that
we can concentrate our testing and bug fixing during the beta phase
for those areas that people are actually using.  This would help our
development work a lot and would also help us to plan for MariaDB 10.2
development.

Anyone will be able to disable the feedback plugin by adding:
--feedback=OFF
to their my.cnf file.

In MariaDB 10.1 gamma/release we would make it default OFF again.

We will of course inform everyone about this change, including how to
disable the feedback, in the MariaDB changelog and release notes.

We do of course hope that as many as possible will have it on, even
after gamma, to help us with our development work!

Information about the MariaDB feedback plugin and the statistics it
provides can be found at:
http://mariadb.org/feedback_plugin
https://mariadb.com/kb/feedback-plugin

As most MariaDB users should know, the feedback is totally anonymous
and no private or sensitive information is being sent.

Any comments, suggestions or recommendations?

Regards,
Michael Widenius
Creator of MySQL and MariaDB

___
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] Group commit id in mysqlbinlog output

[Michael Widenius]

Hi!

 Kristian == Kristian Nielsen kniel...@knielsen-hq.org writes:

cut

Kristian I am not very familiar with timestamps in binlog events. However, all 
the
Kristian transactions in the group commit are written out one after the other, 
in a
Kristian single thread holding the lock on the binlog. And the GTID events are
Kristian generated during that loop. So I think that at least the timestamps 
of the
Kristian GTID events in the group can never walk backwards, nor can they from 
one group
Kristian to the next. But I am not sure if the time stamps of other events in 
the
Kristian transaction can be earlier (maybe they were generated when the query 
was run,
Kristian not when it was committed?

Time stamps in a statement is based on when the statement started, not
when it committed.  This is needed to ensure that all rows has the
same value f you do:

update big_table set timpestamp=now();

So it's normal that in a group commit, the transactions in the same
commit can have totally different timestamps.  Longer running
transactions will typically have older timestamps.

Regards,
Monty


___
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] first results from replication benchmarks

[Michael Widenius]

Hi!

 Axel == Axel Schwenke a...@askmonty.org writes:

Axel Hi everybody,
Axel here are some first results from my benchmarks of replication / group 
commit
Axel performance.

Axel The benchmark is sysbench OLTP rw, data set fitting in memory. I tested a
Axel total of 27 configurations, 9 each for Maria-5.5.36, Maria-10.0.9 and
Axel MySQL-5.6.16. Variations were for binlog on/off, SBR vs. RBR and
Axel sync_binlog=0/1. For MariaDB I also tested XtraDB vs. InnoDB, for MySQL it
Axel was GTID on vs. GTID off.

cut

Axel Next step is applying binlogs to a slave with different settings for 
number
Axel of slave threads etc.

So the current benchmark was when using one slave thread?

When trying parallel replication, remember to test both when doing
updates in one databases and in many databases.

In this case MySQL 5.6 should only scale in the second case.

One reason GTID could be slow on MySQL 5.6 is that it will automaticly
enable the binary log on the slave.
Do you think this is the case?


Regards,
Monty

___
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

[Maria-developers] MariaDB MySQL community event 3rd of April 2014 in Santa Clara

[Michael Widenius]

Hi!

I am happy to announce that the MariaDB Foundation is organising a
MariaDB  MySQL community event in Santa Clara on Thursday the 3rd of
April. The venue is the Hilton Santa Clara hotel, a short walk from
the Percona Live 2014 event.

You can find more information on my blog at:
http://monty-says.blogspot.fi/2014/03/mariadb-mysql-community-event-2014-in.html

We already got some very interesting talks and speakers, but I would
like to see even more talk proposals.

It's ok to propose a talk that it's also hold at Percona Live. As this
event is free, I expect a totally different group of attendees, so
that should be fine!

When we have got a nice list of proposed talks, we will post them here
so that everyone has a chance of commenting of which talks should be
accepted.
(Sorry, but this time we don't have time to create a proper selection
committee so we have to do this a bit rough)

Personally I will talk about MariaDB 10.0, which should be declared
stable by then.

Hope to see many MariaDB developers and users in Santa Clara!

You can sign up for the event and/or dinner at:

http://www.eventbrite.com/e/mariadb-mysql-community-event-2014-tickets-10801604891

Regards,
Monty

PS: Feel free to discuss the event on this email list!  We want to
have as many as possible participating in making this the best
possible MariaDB and MySQL event.

___
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] slave_ddl_exec_mode and incompatible change in MariaDB 10.0.8

[Michael Widenius]

Hi!

 Pavel == Pavel Ivanov piva...@google.com writes:

Pavel On Wed, Feb 26, 2014 at 8:12 PM, Michael Widenius mo...@askmonty.org 
wrote:
Pavel And then it said that slave died with the stack trace
 
Pavel sql/transaction.cc:139(trans_begin(THD*, unsigned int))[0x788e20]
Pavel 
sql/log_event.cc:6478(Gtid_log_event::do_apply_event(rpl_group_info*))[0x93a685]
Pavel sql/log_event.h:1341(Log_event::apply_event(rpl_group_info*))[0x5ca108]
Pavel sql/slave.cc:3191(apply_event_and_update_pos(Log_event*, THD*,
Pavel rpl_group_info*, rpl_parallel_thread*))[0x5c0da8]
Pavel sql/slave.cc:3464(exec_relay_log_event)[0x5c1498]
Pavel sql/slave.cc:4516(handle_slave_sql)[0x5c44e9]
 
 
Pavel Which means that slave tries to execute BEGIN event while OPTION_BEGIN
Pavel is set which shouldn't ever happen.
 
 The assert you put in the code doesn't show that anything is wrong.
 
 The reason is the following code in log_event.cc:
 
thd- variables.option_bits|= OPTION_BEGIN | OPTION_GTID_BEGIN;
 DBUG_PRINT(info, (Set OPTION_GTID_BEGIN));
 trans_begin(thd, 0);
 
 In other words, we do set OPTION_BEGIN just before calling
 trans_begin(), so the assert is wrong.

Pavel To me this code looks clearly wrong. You set OPTION_BEGIN just before
Pavel calling trans_begin() which forces trans_begin() to kick off the
Pavel commit machinery. And even though it ends up doing nothing, I don't
Pavel know how trivial is the number of CPU cycles it spends on that. But
Pavel why set OPTION_BEGIN if it will be set in the trans_begin() anyway?

The reason was simply that I wanted almost all lines that was using
OPTION_GTID_BEGIN to set and reset OPTION_BEGIN.  This was mainly to
make it trivial to ensure with 'grep' that I did not miss any cases.

What I had missed was that trans_begin() did check for OPTION_BEGIN
and when this was set did a few extra unnecessary tests to commit
non-existing things. I agree that it should not be set here.

Pavel So I fixed that and made the line to look like

thd- variables.option_bits|= OPTION_GTID_BEGIN;

Corect.

Pavel But testing that more and looking at the code I realized there's
Pavel another problem in these 3 lines: why did you add the call to
Pavel trans_begin() at all? Right after it mysql_parse() is called to
Pavel execute BEGIN statement, which again kicks off the commit machinery
Pavel without any necessity to do that.

The original reason was to not have to call mysql_parse() at all for
this case (no reason to parse something that we know what it is).

Apparently I missed to remove the extra calls. I will fix that now.
Thanks a lot for noticing this.

Pavel So FYI: I removed setting OPTION_BEGIN here and removed the call to
Pavel trans_begin() and all tests passed (including my additional assert).

Thanks for testing this!

Regards,
Monty

___
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] slave_ddl_exec_mode and incompatible change in MariaDB 10.0.8

[Michael Widenius]

Hi!

First, very sorry that I was not able to answer this question until
now.  I have been traveling in Asia for 2 weeks with very limited
time for internet + email.

Now I am however back and hope to answer all possible questions you
have on this topic.

 Pavel == Pavel Ivanov piva...@google.com writes:

Pavel Hi,
Pavel I've discovered a change in MariaDB 10.0.8 that I don't quite
Pavel understand motivation for and which looks really surprising to me. So
Pavel I would appreciate if you could tell me what's the motivation and why
Pavel you think it's appropriate to do that.

Pavel The change in question is
Pavel http://bazaar.launchpad.net/~maria-captains/maria/10.0/revision/3961.1.7.
Pavel I see several issues with it:

Pavel 1. It introduces a pretty significant variable slave_ddl_exec_mode
Pavel which wasn't announced in the Release Notes.

It looks like Daniel accidently forget to mention this in the release notes.
I have now added thus to:
https://mariadb.com/kb/en/mariadb-1008-release-notes/

I did however originally document this change in:
https://mariadb.com/kb/en/create-table/
https://mariadb.com/kb/en/drop-table/
https://mariadb.com/kb/en/mysqld-options-full-list/
https://mariadb.com/kb/en/what-is-mariadb-100/
https://mariadb.com/kb/en/replication-and-binary-log-server-system-variables/

and I wrote a very detail blog post why this change was needed:
http://monty-says.blogspot.fi/2014/02/the-final-piece-of-puzzle.html

Pavel 2. The default variable value was set to IDEMPOTENT which changes
Pavel MariaDB slave behavior in an incompatible way. Not everyone will want
Pavel to have this variable set to IDEMPOTENT (I'd actually argue that no
Pavel one should set it to IDEMPOTENT), but without any mentioning of this
Pavel variable in the Release Notes people basically have the only chance to
Pavel get knowledge about this change (let me stress out: _incompatible_
Pavel change) from production incidents or from accidental code inspection.

Incompatible in which way?
Can you please show me some common scenarions when the current default would
not be what you want or expect?

My argument is that most people would expect the following from the
MariaDB replication:
(Most of the following was not true before my patch and is not true
for MySQL)

- That row based, statement based and mixed mode replication would
  produce the same number of GTID.
- One should be able to have different replication mode on master and
  slave and still get same number of GTID's.
- Row based, statement based and mixed mode replication would
  produce the same data (was not true before my patch).
- If a slave would die, it should be able restart without user
  intervention as long as the data on the slave and master will be
  exactly the same.
- Statements should always be executed in the same order on the slave
  and master even if the slave and master used different storage
  engines (transactional or not).

Yes, things are not 100 % compatible with how MySQL does things, but
in almost all cases things are better than before.  The slave will be
able to handle restarts in a lot of common scenarios it could not
handle before.

Bcause of this I would argue that almost every single user would
prefer the new mode compared to how things where before.


Pavel 3. When slave_ddl_exec_mode is set to IDEMPOTENT every DROP TABLE
Pavel event in master's binlog is converted into DROP TABLE IF EXISTS in
Pavel slave's binlog, which I believe is a major no-no for replication.

Why ?
In which scenario is this not desirable?

Pavel Binlogs on master and slave should be identical, otherwise after
Pavel failover even if one would set slave_ddl_exec_mode = STRICT it will
Pavel still execute DROP TABLE IF EXISTS because new master has that in
Pavel binlogs.

The binlogs on master and slave have never been identical, especially
if you are using different replication modes.
Here is some strange usage:

- For DROP of temporary tables IF EXISTS was added in some cases, in
  other cases the DROP was never written to the binary log.
- CREATE IF NOT EXISTS  LIKE other_table could create different
  tables based on replication mode.
- CREATE ... SELECT was replicated with 1 GTID on statement based mode
  and 2 GTID's in row based mode.
  - MySQL 5.6 doesn't even allow one to use CREATE ... SELECT with GTID.

The CREATE ... REPLACE patch fixes many cases that before causes data
and tables to be different on master and slave.

Here is some scenarios of things can go wrong and how the new mode
will handle it better than the old one:

1) Repeatable DROP

master:
DROP TABLE t1,t2;

old slave:
DROP TABLE t1
crash; restart
DROP TABLE t1
- Slave will stop as t1 doesn't exist anymore.

New slave:
DROP TABLE IF EXISTS t1  ; Drops t1
crash; restart
DROP TABLE IF EXISTS t1
DROP TABLE IF EXISTS t2

End result, master and slave are consistent.

2) Repeatable CREATE

master:
CREATE TABLE t1 (a int);

old slave:
CREATE TABLE t1 (a int);
- Crash before binary log is 

Re: [Maria-developers] slave_ddl_exec_mode and incompatible change in MariaDB 10.0.8

[Michael Widenius]

Hi!

 Pavel == Pavel Ivanov piva...@google.com writes:

Pavel And now I found that this change is actually buggy. It turns out that
Pavel when slave executes a standalone CREATE TABLE event now it will set
Pavel OPTION_BEGIN flag in thd-variables.option_bits and won't reset it. I
Pavel don't know whether slave keeps transaction actually not committed
Pavel and/or whether it doesn't clean up some other transaction data, but
Pavel execution of the next event will always think there is a transaction
Pavel open and it needs to be auto-committed.

I checked my patch, but I could not find any cases where I had added
setting OPTION_BEGIN, except in connection with OPTION_GTID_BEGIN.
OPTION_GTID_BEGIN is only set when we *know* that there will be a
COMMIT event following in the log.

I also try to verfiy this by running a test that does this on the master:

create table t2 (a int) engine=myisam

I added a breakpoint for the slave in
mysql_create_table

Neiter when the function was entered or exited was the OPTION_BEGIN
flag set.

Can you give me an example of where things goes wrong, preferably with
an extract from the binary log that shows what is actually logged.

For example, here is how a normal create table is logged.
(From suite/rpl/r/create_or_replace_row.result)

slave-bin.01#   Gtid#   #   GTID #-#-#
slave-bin.01#   Query   #   #   use `test`; create 
table t2 (a int) engine=myisam
slave-bin.01#   Gtid#   #   BEGIN GTID #-#-#

The GTID above should not set OPTON_BEGIN or OPTION_GTID_BEGIN on the
slave.

However a CREATE ... SELECT will look like:

master-bin.01   #   Gtid#   #   BEGIN GTID #-#-#
master-bin.01   #   Query   #   #   use `test`; CREATE TABLE
 `t1` (
  `f1` int(1) NOT NULL DEFAULT '0'
)
master-bin.01   #   Table_map   #   #   table_id: # (tes
t.t1)
master-bin.01   #   Write_rows_v1   #   #   table_id: # flag
s: STMT_END_F
master-bin.01   #   Query   #   #   COMMIT

The above will set the OPTION_BEGIN and OPTION_GTID_BEGIN for the
CREATE STATEMENT and this will be reset by the COMMIT (that is
guaranteed to follow).

Pavel But that also means that this
Pavel state cannot be distinguished from the case when slave received BEGIN
Pavel event, but didn't receive COMMIT event, i.e. either binlog on master
Pavel is corrupted or slave somehow skipped some events.

- Corrupted binary logs should not be a concern.  In this case the
  binary log can contain anything, including wrong DROP DATABASE
  commands that could do anything.
- If the master fails, the slave will notice this because it finds a
  'binlog start event', which will reset the BEGIN bits.
- In other words, there will always be a COMMIT event (either explicit
  or implicite, like with a binlog start event)
- The slave can only skip events with slave_skip_counter, but in this
  case it will not be in BEGIN mode. During slave_skip_counter COMMIT
  events will be noticed and the bit will be reset.

How can the binlog be corrupted?
How do you expect the master to handle corruption?
Why is CREATE TABLE a special case you are concerned about, compared
to other things like DELETE FROM TABLE in row based replication?
(DELETE FROM expect a BEGIN, table_id, many delete-row-events, COMMIT).

Pavel Would MariaDB consider this as a serious problem?

Please show me a test case first so that I can understand the problem.

Regards,
Monty

___
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