[Maria-developers] The Monty Program AB vendor check (rpm)

2013-12-03 Thread Oden Eriksson
Hello,

I got assigned to look at the rpm packaging and fixed some stuff there, but got 
stuck at the Monty Program AB vendor check in support-files/rpm/server-
prein.sh when I wanted to replace the fedora packages with in this case 
mariadb galera packages.

Is this check absolutely nessesary?

This prevents you from an easy switch from the fedora mariadb packages to the 
ones provided by mariadb using yum and a yum repository as provided at 
http://yum.mariadb.org/.

Basically this check prevents me from doing any serious work with the rpm 
packages because you manually have to, by force deinstall any conflicting 
packages and then install the packages provided by us.

Cheers.

___
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] Build fail of MariaDB10 with TokuDB enabled

2013-12-03 Thread Sergei Golubchik
Hi, Leif!

On Dec 02, Leif Walsh wrote:
 Yep, CMAKE_SKIP_BUILD_RPATH=0 would cause this exact problem. We
 should add something to our cmake to warn if that is set to 0. 

I'd suggest this change:

--- storage/tokudb/ft-index/ft/CMakeLists.txt   2013-11-19 14:35:31 +
+++ storage/tokudb/ft-index/ft/CMakeLists.txt   2013-12-03 09:04:29 +
@@ -11 +11 @@
-target_link_libraries(logformat ${LIBTOKUPORTABILITY})
+target_link_libraries(logformat ${LIBTOKUPORTABILITY}_static)

logformat is used internally during the build, it's not going to be
installed. Thus it can use static libraries and avoid the whole rpath
issue altogether.

This should probably be done for all binaries that are only used during
the build.

Regards,
Sergei


___
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 of mdev-5095 (executing triggers on slave)

2013-12-03 Thread Michael Widenius

Hi!

Here is the review of mdev-5095

I got the diff from:
bzr diff -r3942..

 === modified file 'mysql-test/r/mysqld--help.result'
 --- mysql-test/r/mysqld--help.result  2012-10-18 21:33:06 +
 +++ mysql-test/r/mysqld--help.result  2013-12-03 10:53:01 +
 @@ -747,6 +747,14 @@
   --slave-net-timeout=# 
   Number of seconds to wait for more data from a
   master/slave connection before aborting the read
 + --slave-run-triggers-for-rbr=name 
 + Modes for how triggers in row-base replication on slave
 + side will be executed. Legal values are NO (default), YES
 + and LOGGING. NO means that trigger for RBR will not be
 + running on slave YES and LOGGING means that triggers will
 + be running on slave (if there was not triggers running on
 + the naster), LOGGING also mens that flag about executed
 + triggers will be written to binlog.

Suggested new text:

--slave-run-triggers-for-rbr=name 
Modes for how triggers in row-base replication on slave side will be
executed. Legal values are NO (default), YES and LOGGING. NO means
that trigger for RBR will not be running on slave. YES and LOGGING
means that triggers will be running on slave, if there was not
triggers running on the master for the statement. LOGGING also means
that the executed triggers will be written to binlog.
 
cut

 === modified file 'sql/log_event.cc'

cut

 -#if !defined(MYSQL_CLIENT)  defined(HAVE_REPLICATION)
 +#if defined(MYSQL_SERVER)  defined(HAVE_REPLICATION)

Was the above needed or mostly an optimization?

 +
 +static void restore_empty_query_table_list(LEX *lex)
 +{
 +  if (lex-first_not_own_table())
 +  (*lex-first_not_own_table()-prev_global)= NULL;
 +  lex-query_tables= NULL;
 +  lex-query_tables_last= lex-query_tables;
 +}

Add a comment of what is the purpose of the above function.

 @@ -8340,6 +8351,22 @@ int Rows_log_event::do_apply_event(Relay
  /* A small test to verify that objects have consistent types */
  DBUG_ASSERT(sizeof(thd-variables.option_bits) == 
 sizeof(OPTION_RELAXED_UNIQUE_CHECKS));
  
 +if (slave_run_triggers_for_rbr)
 +{
 +  LEX *lex= thd-lex;
 +  uint8 new_trg_event_map= get_trg_event_map();
 +
 +  DBUG_ASSERT(lex-query_tables == NULL);
 +  if ((lex-query_tables= rli-tables_to_lock))
 +rli-tables_to_lock-prev_global= lex-query_tables;

Add a comment why we have to set prev_global (not obvious).
It's also not obvious why restore_empty_tables() is not resetting
rli-tables_to_lock-prev_global as it's reseting the the rest of the
changed things.

 @@ -8429,8 +8462,11 @@ int Rows_log_event::do_apply_event(Relay
   */
  TABLE_LIST *ptr= rli-tables_to_lock;
  for (uint i=0 ;  ptr  (i  rli-tables_to_lock_count); ptr= 
 ptr-next_global, i++)
 +{
const_castRelay_log_info*(rli)-m_table_map.set_table(ptr-table_id, 
 ptr-table);
 -

What does this test really do ?
(I was quickly checking the code to understand what m_table_id stands
for, but it was not obvious).

 +  if (m_table_id == ptr-table_id)
 +master_had_triggers= ((RPL_TABLE_LIST*)ptr)-master_had_triggers;


cut
if (table)
{
  bool transactional_table= table-file-has_transactions();
 @@ -8618,6 +8656,9 @@ int Rows_log_event::do_apply_event(Relay
  */
  thd-reset_current_stmt_binlog_format_row();
  thd-is_slave_error= 1;
 +/* remove trigger's tables */
 +if (slave_run_triggers_for_rbr)
 +  restore_empty_query_table_list(thd-lex);
  DBUG_RETURN(error);
}

You do the above 'restore_empty_query_table_list(thd-lex);' before
every return in this function, except the last return.

- Isn't it better to do a 'error:' tag at the end of the function
  to ensure we don't miss any cases ?

- Add a comment why we don't need to do this at the very end of
  Rows_log_event::do_apply_event().
  Something like:
  /*
 We don't have to call restore_empty_query_table_list() here as
 it's called in rows_event_stmt_cleanup().
  */
  
When is lex-query_tables used?
Is it only used by 'open' ?
If yes, isn't it enough to always reset it after the 'open' call?

 @@ -9773,6 +9831,29 @@ Write_rows_log_event::do_before_row_oper
from the start.
  */
}
 +  if (slave_run_triggers_for_rbr  !master_had_triggers  
 m_table-triggers )
 +  {
 +if (m_table-triggers-has_triggers(TRG_EVENT_DELETE,
 +TRG_ACTION_AFTER))
 +{
 +  /*
 +The table has AFTER DELETE triggers that might access to 
 +subject table and therefore might need delete to be done 
 +immediately. So we turn-off the batching.
 +  */ 
 +  (void) m_table-file-extra(HA_EXTRA_DELETE_CANNOT_BATCH);
 +}
 +if (m_table-triggers-has_triggers(TRG_EVENT_UPDATE,
 +TRG_ACTION_AFTER))
 +{
 +  /*
 +The table has AFTER UPDATE triggers that might access to subject 
 +table and therefore might need update to be done immediately. 
 +So we turn-off 

Re: [Maria-developers] Build fail of MariaDB10 with TokuDB enabled

2013-12-03 Thread Leif Walsh
That's a good solution, but I'd still be wary of messing around too much with 
cmake's RPATH handling. I'll do this in ft-index soon though. 

Sent from my iPhone

 On Dec 3, 2013, at 4:09, Sergei Golubchik s...@mariadb.org wrote:
 
 Hi, Leif!
 
 On Dec 02, Leif Walsh wrote:
 Yep, CMAKE_SKIP_BUILD_RPATH=0 would cause this exact problem. We
 should add something to our cmake to warn if that is set to 0.
 
 I'd suggest this change:
 
 --- storage/tokudb/ft-index/ft/CMakeLists.txt   2013-11-19 14:35:31 +
 +++ storage/tokudb/ft-index/ft/CMakeLists.txt   2013-12-03 09:04:29 +
 @@ -11 +11 @@
 -target_link_libraries(logformat ${LIBTOKUPORTABILITY})
 +target_link_libraries(logformat ${LIBTOKUPORTABILITY}_static)
 
 logformat is used internally during the build, it's not going to be
 installed. Thus it can use static libraries and avoid the whole rpath
 issue altogether.
 
 This should probably be done for all binaries that are only used during
 the build.
 
 Regards,
 Sergei
 

___
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-5345 - Deadlock between mysql_change_user(), SHOW VARIABLES and INSTALL PLUGIN

2013-12-03 Thread Sergey Vojtovich
Hi Sergei,

thanks for your feedback. Comments/questions inline.

On Fri, Nov 29, 2013 at 01:57:13PM +0100, Sergei Golubchik wrote:
 Hi, Sergey!
 
 On Nov 27, Sergey Vojtovich wrote:
  Hi Sergei,
  
  I'm afraid I couldn't find easy fix for this deadlock. The question is if it
  is worth to spend time for more complex (and possibly less stable) solution.
  Details below.
  
  Locking profile
  ---
  INSTALL PLUGIN: LOCK_plugin- 
  LOCK_system_variables_hash (w)
  SHOW VARIABLES: LOCK_system_variables_hash (r) - 
  LOCK_global_system_variables
   change_user(): LOCK_global_system_variables   - LOCK_plugin
  
  INSTALL PLUGIN
  --
  Needs LOCK_plugin to register new plugin.
  Needs LOCK_system_variables_hash to register plugin variables.
  Doesn't seem to need both locks at the same time.
  Similar lock order is used in a few places.
  
  SHOW VARIABLES
  --
  Needs LOCK_system_variables_hash to iterate system_variable_hash.
  Needs LOCK_global_system_variables to read variable value.
  Does seem to need both locks at the same time.
  This lock order is not used in other places.
  
  change_user()
  -
  Needs LOCK_global_system_variables to read 
  global_system_variable.table_plugin.
  Needs LOCK_plugin to my_plugin_lock(table_plugin).
  Does seem to need both locks at the same time.
  Similar lock order is used in a few places.
 
 Observations:
 
 THD::init() starts from locking LOCK_global_system_variables, then
 invokes plugin_thdvar_init(), that invokes cleanup_variables(), that
 read-locks LOCK_system_variables_hash.
 
 This is directly against SHOW VARIABLES order. Suggestion: swap the
 order of cleanup_variables() and LOCK_global_system_variables.
Right and I plan to do something about it later, when MDEV-5345 is fixed.
Anyway currently there should be no deadlock here, since it doesn't conflict
with write-lock order.

 
  SHOW VARIABLES looks correct. So I have only two ideas:
  - Fix INSTALL PLUGIN so it releases LOCK_plugin before registering 
  variables.
Sounds like the best solution, but there are a few more things to fix, 
  e.g.
UNINTALL PLUGIN.
 
 Is it possible? On a quick look I wasn't able to answer that :(
It should be quite easy: for INSTALL PLUGIN we just move test_plugin_options()
to plugin_initialize() while plugin state is PLUGIN_IS_UNINITIALIZED. Same for
UNINSTALL PLUGIN.

Did I miss something important?

 
  - Store default storage engine as a string, not Sys_var_plugin.
 
 That'd mean, that neither global nor local @@default_storage_engine
 setting locks the plugin. We'll need to do a hash lookup and lock the
 plugin for every statement that uses the engine. Which may be or may not
 be a problem.
Right, I can't say if it may or may not be a problem yet.

 
 Here's yet another idea: don't protect plugin-ref_cnt and plugin-state
 with LOCK_plugin. Then intern_plugin_lock and intern_plugin_unlock could
 be completely lock-free. One would need to increment ref_cnt before or
 at the same time as checking the state.  It *seems* that the rest of the
 code could support that with very few changes. But it'd need more
 analysys.
Well, lock-free ref_count is not a problem. Lock-free state is challenging,
but looks doable. What about reap_plugins() which is called by plugin_unlock()?

Thanks,
Sergey

___
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 of mdev-5095 (executing triggers on slave)

2013-12-03 Thread Oleksandr Byelkin

Hi!

03.12.2013 14:09, Michael Widenius пишет:

Hi!

Here is the review of mdev-5095

I got the diff from:
bzr diff -r3942..


[skip]

=== modified file 'sql/log_event.cc'

cut


-#if !defined(MYSQL_CLIENT)  defined(HAVE_REPLICATION)
+#if defined(MYSQL_SERVER)  defined(HAVE_REPLICATION)

Was the above needed or mostly an optimization?
Actually above made to make headers and bodies of methods be controlled 
by the same condition, because it is strange that header has one 
condition and body different (now it means the same bt who know what it 
will be in the future).

[skip]

@@ -8429,8 +8462,11 @@ int Rows_log_event::do_apply_event(Relay
   */
  TABLE_LIST *ptr= rli-tables_to_lock;
  for (uint i=0 ;  ptr  (i  rli-tables_to_lock_count); ptr= 
ptr-next_global, i++)
+{
const_castRelay_log_info*(rli)-m_table_map.set_table(ptr-table_id, 
ptr-table);
-
What does this test really do ?
(I was quickly checking the code to understand what m_table_id stands
for, but it was not obvious).
Following is passing flag about triggers on the server. The problem was 
to pass it between table map event and row event. I do it via extended 
TABLE_LIST (RPL_TABLE_LIST). but row event uses only TABLE so I need to 
find somehow the corresponding TABLE_LIST.



+  if (m_table_id == ptr-table_id)
+master_had_triggers= ((RPL_TABLE_LIST*)ptr)-master_had_triggers;


cut

if (table)
{
  bool transactional_table= table-file-has_transactions();
@@ -8618,6 +8656,9 @@ int Rows_log_event::do_apply_event(Relay
  */
  thd-reset_current_stmt_binlog_format_row();
  thd-is_slave_error= 1;
+/* remove trigger's tables */
+if (slave_run_triggers_for_rbr)
+  restore_empty_query_table_list(thd-lex);
  DBUG_RETURN(error);
}

You do the above 'restore_empty_query_table_list(thd-lex);' before
every return in this function, except the last return.

it is done before very last return but not so close to it.


- Isn't it better to do a 'error:' tag at the end of the function
   to ensure we don't miss any cases ?

- Add a comment why we don't need to do this at the very end of
   Rows_log_event::do_apply_event().
   Something like:
   /*
  We don't have to call restore_empty_query_table_list() here as
  it's called in rows_event_stmt_cleanup().
   */
   
When is lex-query_tables used?
the problem was that trigger procedures works only with 
lex-query_tables (and it was one of the cause of skipping triggers in 
row log events.

Is it only used by 'open' ?

It is where triggers processed (openng and locking tables).

If yes, isn't it enough to always reset it after the 'open' call?
We have a lot of ASSERTS that check the lists when we close tables. they 
have the same problem with MERGE MYISAM tables and went this way I only 
trying to do more or less the same.



@@ -9773,6 +9831,29 @@ Write_rows_log_event::do_before_row_oper
from the start.
  */
}
+  if (slave_run_triggers_for_rbr  !master_had_triggers  m_table-triggers )
+  {
+if (m_table-triggers-has_triggers(TRG_EVENT_DELETE,
+TRG_ACTION_AFTER))
+{
+  /*
+The table has AFTER DELETE triggers that might access to
+subject table and therefore might need delete to be done
+immediately. So we turn-off the batching.
+  */
+  (void) m_table-file-extra(HA_EXTRA_DELETE_CANNOT_BATCH);
+}
+if (m_table-triggers-has_triggers(TRG_EVENT_UPDATE,
+TRG_ACTION_AFTER))
+{
+  /*
+The table has AFTER UPDATE triggers that might access to subject
+table and therefore might need update to be done immediately.
+So we turn-off the batching.
+  */
+  (void) m_table-file-extra(HA_EXTRA_UPDATE_CANNOT_BATCH);
+}
+  }

Good that you noticed and fixed the above.

However, why not just call the function
prepare_triggers_for_insert_stmt()
that does exactly the same thing?
it also has table-mark_columns_needed_for_insert(); Raw log event mark 
all columns in any case also checks some correspondence between real 
columns on master  slave so I prefer do not interfere here.



cut

@@ -10749,6 +10894,17 @@ Delete_rows_log_event::do_before_row_ope
  */
  return 0;
}
+  if (slave_run_triggers_for_rbr  !master_had_triggers  m_table-triggers 

+  m_table-triggers-has_triggers(TRG_EVENT_DELETE,
+TRG_ACTION_AFTER))
+  {
+/*
+  The table has AFTER DELETE triggers that might access to subject table
+  and therefore might need delete to be done immediately. So we turn-off
+  the batching.
+*/
+(void) m_table-file-extra(HA_EXTRA_DELETE_CANNOT_BATCH);
+  }

Would be better if we could do a function 'prepare_triggers_for_delete_stmt()'
and use the same code here and in sql_delete.cc
maybe, but as I it will be temptation to put something more to the 
function without noticing that it is called for events