Re: [Maria-developers] [Commits] Rev 2864: MWL#136: Cross-engine consistency for START TRANSACTION WITH CONSISTENT SNAPSHOT in http://bazaar.launchpad.net/~maria-captains/maria/5.1
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> At http://bazaar.launchpad.net/~maria-captains/maria/5.1 knielsen> ------------------------------------------------------------ knielsen> revno: 2864 knielsen> revision-id: knielsen@knielsen-hq.org-20101107213743-3luszsivft2vt7t6 knielsen> parent: knielsen@knielsen-hq.org-20101103155438-vdou0fngj6hpgpsd knielsen> committer: knielsen@knielsen-hq.org knielsen> branch nick: work-5.1-mwl136 knielsen> timestamp: Sun 2010-11-07 22:37:43 +0100 knielsen> message: knielsen> MWL#136: Cross-engine consistency for START TRANSACTION WITH CONSISTENT SNAPSHOT <cut> knielsen> === modified file 'client/mysqldump.c' <cut> knielsen> -static int do_show_master_status(MYSQL *mysql_con) knielsen> +static int do_show_master_status(MYSQL *mysql_con, int consistent_binlog_pos) knielsen> { knielsen> MYSQL_ROW row; knielsen> MYSQL_RES *master; knielsen> + char binlog_pos_file[FN_REFLEN]; knielsen> + char binlog_pos_offset[LONGLONG_LEN+1]; knielsen> + char *file, *offset; knielsen> const char *comment_prefix= knielsen> (opt_master_data == MYSQL_OPT_MASTER_DATA_COMMENTED_SQL) ? "-- " : ""; knielsen> - if (mysql_query_with_error_report(mysql_con, &master, "SHOW MASTER STATUS")) knielsen> + knielsen> + if (consistent_binlog_pos) knielsen> { knielsen> - return 1; knielsen> + if(!check_consistent_binlog_pos(binlog_pos_file, binlog_pos_offset)) knielsen> + return 1; knielsen> + file= binlog_pos_file; knielsen> + offset= binlog_pos_offset; knielsen> } knielsen> else knielsen> { knielsen> + if (mysql_query_with_error_report(mysql_con, &master, "SHOW MASTER STATUS")) knielsen> + return 1; knielsen> + knielsen> row= mysql_fetch_row(master); knielsen> if (row && row[0] && row[1]) knielsen> { knielsen> + file= row[0]; knielsen> + offset= row[1]; knielsen> } knielsen> + else knielsen> { You need a mysql_free_result(master) here. knielsen> + if (!ignore_errors) knielsen> + { knielsen> + /* SHOW MASTER STATUS reports nothing and --force is not enabled */ knielsen> + my_printf_error(0, "Error: Binlogging on server not active", knielsen> + MYF(0)); knielsen> + maybe_exit(EX_MYSQLERR); knielsen> + return 1; knielsen> + } knielsen> + else knielsen> + { knielsen> + return 0; knielsen> + } knielsen> } knielsen> } <cut> knielsen> === modified file 'sql/log.cc' knielsen> --- a/sql/log.cc 2010-11-02 07:40:27 +0000 knielsen> +++ b/sql/log.cc 2010-11-07 21:37:43 +0000 knielsen> @@ -62,6 +62,7 @@ static int binlog_savepoint_rollback(han knielsen> static int binlog_commit(handlerton *hton, THD *thd, bool all); knielsen> static int binlog_rollback(handlerton *hton, THD *thd, bool all); knielsen> static int binlog_prepare(handlerton *hton, THD *thd, bool all); knielsen> +static int binlog_start_consistent_snapshot(handlerton *hton, THD *thd); knielsen> /** knielsen> Silence all errors and warnings reported when performing a write knielsen> @@ -155,9 +156,10 @@ class binlog_trx_data { knielsen> public: knielsen> binlog_trx_data() knielsen> : at_least_one_stmt_committed(0), incident(FALSE), m_pending(0), knielsen> - before_stmt_pos(MY_OFF_T_UNDEF), commit_bin_log_file_pos(0), using_xa(0) knielsen> + before_stmt_pos(MY_OFF_T_UNDEF), last_commit_pos_offset(0), using_xa(0) knielsen> { knielsen> trans_log.end_of_file= max_binlog_cache_size; knielsen> + strcpy(last_commit_pos_file, ""); strcpy() -> strmov() or even better last_commit_pos_file[0]= 0; (I want to get rid of all strcpy() and replace with strmov() as strcpy() is often used wrongly; I would like to have this thing changed to not get 'false positives' when searching for strcpy() to remove. knielsen> } knielsen> ~binlog_trx_data() knielsen> @@ -215,7 +217,8 @@ public: knielsen> incident= FALSE; knielsen> trans_log.end_of_file= max_binlog_cache_size; knielsen> using_xa= FALSE; knielsen> - commit_bin_log_file_pos= 0; knielsen> + strcpy(last_commit_pos_file, ""); strcpy -> strmov or last_commit_pos_file[0]= 0; knielsen> + last_commit_pos_offset= 0; knielsen> DBUG_ASSERT(empty()); knielsen> } knielsen> /* knielsen> Write a table map to the binary log. knielsen> @@ -4337,6 +4369,9 @@ MYSQL_BIN_LOG::flush_and_set_pending_row knielsen> rotate_and_purge(RP_LOCK_LOG_IS_ALREADY_LOCKED); knielsen> } Add a comment that this mutex is a protection for partial reads of last_commit_pos_offset on 32 bit systems. (at least I can't come up with any better reason for this mutex). Maybe we should introduce pthread_mutex_lock_for_32_bit() that would be void on 64 bit systems in cases like this. knielsen> + pthread_mutex_lock(&LOCK_commit_ordered); knielsen> + last_commit_pos_offset= my_b_tell(&log_file); knielsen> + pthread_mutex_unlock(&LOCK_commit_ordered); knielsen> pthread_mutex_unlock(&LOCK_log); knielsen> } knielsen> @@ -4526,7 +4561,12 @@ bool MYSQL_BIN_LOG::write(Log_event *eve knielsen> err_unlock: knielsen> if (file == &log_file) knielsen> + { knielsen> + pthread_mutex_lock(&LOCK_commit_ordered); knielsen> + last_commit_pos_offset= my_b_tell(&log_file); knielsen> + pthread_mutex_unlock(&LOCK_commit_ordered); knielsen> pthread_mutex_unlock(&LOCK_log); knielsen> + } Same comment needed for this. knielsen> err: knielsen> if (error) knielsen> @@ -4827,6 +4867,9 @@ bool MYSQL_BIN_LOG::write_incident(THD * knielsen> signal_update(); knielsen> rotate_and_purge(RP_LOCK_LOG_IS_ALREADY_LOCKED); knielsen> } knielsen> + pthread_mutex_lock(&LOCK_commit_ordered); knielsen> + last_commit_pos_offset= my_b_tell(&log_file); knielsen> + pthread_mutex_unlock(&LOCK_commit_ordered); knielsen> pthread_mutex_unlock(&LOCK_log); and here... knielsen> static ulonglong binlog_status_var_num_commits; knielsen> static ulonglong binlog_status_var_num_group_commits; knielsen> +static char binlog_trx_file[FN_REFLEN]; knielsen> +static ulonglong binlog_trx_position; knielsen> static SHOW_VAR binlog_status_vars_detail[]= knielsen> { knielsen> @@ -6562,12 +6609,16 @@ static SHOW_VAR binlog_status_vars_detai knielsen> (char *)&binlog_status_var_num_commits, SHOW_LONGLONG}, knielsen> {"group_commits", knielsen> (char *)&binlog_status_var_num_group_commits, SHOW_LONGLONG}, knielsen> + {"trx_file", knielsen> + (char *)&binlog_trx_file, SHOW_CHAR}, knielsen> + {"trx_position", knielsen> + (char *)&binlog_trx_position, SHOW_LONGLONG}, knielsen> {NullS, NullS, SHOW_LONG} knielsen> }; knielsen> static int show_binlog_vars(THD *thd, SHOW_VAR *var, char *buff) knielsen> { knielsen> - mysql_bin_log.set_status_variables(); knielsen> + mysql_bin_log.set_status_variables(thd); knielsen> var->type= SHOW_ARRAY; knielsen> var->value= (char *)&binlog_status_vars_detail; knielsen> return 0; knielsen> @@ -6606,17 +6657,31 @@ static struct st_mysql_sys_var *binlog_s knielsen> This is called only under LOCK_status, so we can fill in a static array. knielsen> */ knielsen> void knielsen> -TC_LOG_BINLOG::set_status_variables() knielsen> +TC_LOG_BINLOG::set_status_variables(THD *thd) knielsen> { knielsen> - ulonglong num_commits, num_group_commits; knielsen> + binlog_trx_data *trx_data; knielsen> + knielsen> + if (thd) knielsen> + trx_data= (binlog_trx_data*) thd_get_ha_data(thd, binlog_hton); knielsen> + else knielsen> + trx_data= NULL; knielsen> pthread_mutex_lock(&LOCK_commit_ordered); knielsen> - num_commits= this->num_commits; knielsen> - num_group_commits= this->num_group_commits; knielsen> + binlog_status_var_num_commits= this->num_commits; knielsen> + binlog_status_var_num_group_commits= this->num_group_commits; knielsen> + if (!trx_data || 0 == strcmp(trx_data->last_commit_pos_file, "")) -> if (!trx_data || trx_data->last_commit_pos_file[0] == 0) knielsen> + { knielsen> + strmake(binlog_trx_file, last_commit_pos_file, sizeof(binlog_trx_file)-1); knielsen> + binlog_trx_position= last_commit_pos_offset; knielsen> + } knielsen> pthread_mutex_unlock(&LOCK_commit_ordered); knielsen> + if (trx_data && 0 != strcmp(trx_data->last_commit_pos_file, "")) -> if (trx_data && trx_data->last_commit_pos_file[0] != 0) It was a bit tricky to see that this was a negative test for the earlier one. I would sugges to change first to: -> if ((binlog_pos_set= (!trx_data || trx_data->last_commit_pos_file[0] == 0)) and the later to if (!binlog_pos_set) knielsen> + { knielsen> + strmake(binlog_trx_file, trx_data->last_commit_pos_file, knielsen> + sizeof(binlog_trx_file)-1); knielsen> + binlog_trx_position= trx_data->last_commit_pos_offset; knielsen> + } knielsen> } <cut> Regards, Monty
Hi Monty! Thanks for review of MWL#136. I fixed the patch according to all your suggestions! - Kristian.
Kristian, this patch fails on Windows builds due to "grep" and "tail" used in the test cases. http://buildbot.askmonty.org/buildbot/builders/win2008r2-vs2010-amd64-debug/ builds/181 Perhaps it could be a good idea to rewrite the case in a portable way, for example using "perl; "sections ? Hopefully, grep and tail will only take a few lines of Perl. Thanks, Wlad
-----Original Message----- From: maria-developers- bounces+wlad=montyprogram.com@lists.launchpad.net [mailto:maria- developers-bounces+wlad=montyprogram.com@lists.launchpad.net] On Behalf Of Kristian Nielsen Sent: Montag, 4. April 2011 09:29 To: monty@askmonty.org Cc: maria-developers@lists.launchpad.net Subject: Re: [Maria-developers] [Commits] Rev 2864: MWL#136: Cross- engine consistency for START TRANSACTION WITH CONSISTENT SNAPSHOT in http://bazaar.launchpad.net/~maria-captains/maria/5.1
Hi Monty!
Thanks for review of MWL#136.
I fixed the patch according to all your suggestions!
- Kristian.
_______________________________________________ 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
Wlad, If I may, I'd like to point out that both grep and tail are very easily, and readily implemented in python as well, which is equally portable and increasingly common to have installed on windows workstations (I use it on servers professionally for admin type tasks, etc). Also it is packaged in a very standard, easy to use (and to be located during build time) manner that would be amazingly straightforward as a dependency. Furthermore you can easily make a portable, directory with a path (just set the PYTHON_PATH) type installation for all build environments which don't have it or don't want it for anything outside of this use. If we are making use of what is already in existence, why not try to make use of what may already even be installed. Jakob Lorberblatt
Kristian, this patch fails on Windows builds due to "grep" and "tail" used in the test cases. http://buildbot.askmonty.org/buildbot/builders/win2008r2-vs2010-amd64-debug/ builds/181
Perhaps it could be a good idea to rewrite the case in a portable way, for example using "perl; "sections ? Hopefully, grep and tail will only take a few lines of Perl.
Thanks, Wlad
-----Original Message----- From: maria-developers- bounces+wlad=montyprogram.com@lists.launchpad.net [mailto:maria- developers-bounces+wlad=montyprogram.com@lists.launchpad.net] On Behalf Of Kristian Nielsen Sent: Montag, 4. April 2011 09:29 To: monty@askmonty.org Cc: maria-developers@lists.launchpad.net Subject: Re: [Maria-developers] [Commits] Rev 2864: MWL#136: Cross- engine consistency for START TRANSACTION WITH CONSISTENT SNAPSHOT in http://bazaar.launchpad.net/~maria-captains/maria/5.1
Hi Monty!
Thanks for review of MWL#136.
I fixed the patch according to all your suggestions!
- Kristian.
_______________________________________________ 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
_______________________________________________ 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
-----Original Message----- From: maria-developers- bounces+wlad=montyprogram.com@lists.launchpad.net [mailto:maria- developers-bounces+wlad=montyprogram.com@lists.launchpad.net] On Behalf Of Jakob Lorberblatt Sent: Dienstag, 5. April 2011 04:01 To: maria-developers@lists.launchpad.net Subject: Re: [Maria-developers] [Commits] Rev 2864: MWL#136: Cross- engine consistency for START TRANSACTION WITH CONSISTENT SNAPSHOT in http://bazaar.launchpad.net/~maria-captains/maria/5.1
Wlad, If I may, I'd like to point out that both grep and tail are very easily, and readily implemented in python as well, which is equally portable and increasingly common to have installed on windows workstations (I use it on servers professionally for admin type tasks, etc). Also it is packaged in a very standard, easy to use (and to be located during build time) manner that would be amazingly straightforward as a dependency. Furthermore you can easily make a portable, directory with a path (just set the PYTHON_PATH) type installation for all build environments which don't have it or don't want it for anything outside of this use. If we are making use of what is already in existence, why not try to make use of what may already even be installed.
Thanks Jakob. Python is a fine language and I would have preferred it to Perl, for better portability/readability and other abilities. But, out test suite depends on Perl to run and it is not likely to change in the next decade, so we'll live with this dependency on developer systems (there is no dependency on it on customer machines). Either way, the test case in question is a simple grep file | tail -1 , which is rather trivial in Perl as well, but will be probably solved in a different manner (as http://askmonty.org/worklog/Server-RawIdeaBin/index.pl?tid=191 alludes). Wlad
participants (4)
-
Jakob Lorberblatt -
Kristian Nielsen -
Michael Widenius -
Vladislav Vaintroub