Merge lp:~gl-az/percona-xtrabackup/bug1250375-2.1 into lp:percona-xtrabackup/2.1

Proposed by George Ormond Lorch III
Status: Merged
Approved by: Alexey Kopytov
Approved revision: no longer in the source branch.
Merged at revision: 716
Proposed branch: lp:~gl-az/percona-xtrabackup/bug1250375-2.1
Merge into: lp:percona-xtrabackup/2.1
Diff against target: 75 lines (+60/-0)
2 files modified
innobackupex.pl (+22/-0)
test/t/bug1250375.sh (+38/-0)
To merge this branch: bzr merge lp:~gl-az/percona-xtrabackup/bug1250375-2.1
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Review via email: mp+201526@code.launchpad.net

Description of the change

Fix for bug 1250375 : PXB 56 in galera mode should backup last binlog file.

When --galera-info is specified and Executed_Gtid_Set is valid (PXB 5.6 only or better), innobackupex will execute FLUSH BINARY LOGS and then carry the current binary log as indicated in SHOW MASTER STATUS into the backup set.

Created simple test case (bug1250375.sh) based on xb_galera_info.sh that is only valid for PXC 5.6 or greater to test that the correct bin-log file is present within the backup set.

To post a comment you must log in.
Revision history for this message
George Ormond Lorch III (gl-az) wrote :

http://jenkins.percona.com/view/PXB%202.1/job/percona-xtrabackup-2.1-param/517/

I'm not particularly thrilled with the test case. Please suggest some better ideas if you have any.

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

Reviewed the changes myself again after a break and re-orderd things a bit in write_galera_info...new jenkins here http://jenkins.percona.com/view/PXB%202.1/job/percona-xtrabackup-2.1-param/518/ but one host is now having issues. I will restart tests when the issue clears and post new comments.

Revision history for this message
Alexey Kopytov (akopytov) wrote :

George,

Are there any reasons to execute FLUSH BINARY LOGS unconditionally, i.e. even when GTID is disabled?

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

Well, yeah but it can be done differently. 'Executed_Gtid_Set' is retrieved via 'SHOW MASTER STATUS' as is 'File' (bin log file name). The alternative approach would be to 'SHOW MASTER STATUS' to test for the validity of 'Executed_Gtid_Set', then if valid, 'FLUSH BINARY LOGS' and 'SHOW MASTER STATUS' again to get the accurate/updated 'File'.

Revision history for this message
Alexey Kopytov (akopytov) wrote :

You already have SHOW MASTER STATUS values when write_galera_info() is called. It is executed from write_binlog_info() which is called before write_galera_info(). If Executed_Gtid_Set is defined, it will not change, since the server is read-only and thus, it can be re-used.

To be safe from future code changes, you can do something like:

if (!defined($con->{master_status}) {
  get_mysql_master_status($con);
}

if ($con->{master_status}->{Executed_Gtid_Set}) {
  mysql_query($con, "FLUSH BINARY LOGS");
  ...
}

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

Yup, that was precisely the case, I did not want to assume that SHOW MASTER STATUS had already been called. Your suggestion is a good one. I will get that coded up and resubmitted here shortly.

Revision history for this message
George Ormond Lorch III (gl-az) wrote :

OK, fixed up the FLUSH BINARY LOGS as mentioned above as well as some minor tab/space issues and long lines...

new jenkins here:
http://jenkins.percona.com/view/PXB%202.1/job/percona-xtrabackup-2.1-param/519/

Revision history for this message
Alexey Kopytov (akopytov) wrote :

> + if (!defined($log_bin_file) || !defined($log_bin_dir)) {
> + print STDERR "$now $prefix Failed to get master binlog " .
> + "directory and coordinates from SHOW MASTER STATUS\n";
> + print STDERR "$now $prefix This means that the server is not a " .
> + "member of a cluster nor a replication master. " .
> + "Ignoring the --galera-info option\n";
> +
>

The error message is wrong. Binary log must be enabled for GTID to work,
so failure to get binlog coordinates is an impossible condition. But it
certainly does not mean that the server is not a member of a
cluster. Nor does it mean that the server is not a replication master.

It does also not lead to the --galera-info option being ignored, because
we do write the galera info file in this case.

I would make it a fatal error. I.e.:

die “Failed to get binlog coordinates from SHOW MASTER STATUS”

[...]

> +if [[ -n ${WSREP_DEBUG:-} ]];then
> + start_server --log-bin=`hostname`-bin --binlog-format=ROW --wsrep-provider=${MYSQL_BASEDIR}/lib/libgalera_smm.so --wsrep_cluster_address=gcomm:// --wsrep-debug=1 --wsrep_provider_options="debug=1" --wsrep_node_address=$ADDR
> +else
> + start_server --log-bin=`hostname`-bin --binlog-format=ROW --wsrep-provider=${MYSQL_BASEDIR}/lib/libgalera_smm.so --wsrep_cluster_address=gcomm:// --wsrep_node_address=$ADDR
> +fi

gtid_mode=ON?

> +
> +
> +innobackupex --no-timestamp --galera-info $topdir/backup
> +backup_dir=$topdir/backup
> +vlog "Backup created in directory $backup_dir"
> +
> +count=0
> +master_file=
> +master_pos=
> +
> +while read line; do
> + if [ $count -eq 1 ] # File:
> + then
> + master_file=`echo "$line" | sed s/File://`
> + elif [ $count -eq 2 ] # Position:
> + then
> + master_pos=`echo "$line" | sed s/Position://`

master_pos is unused?

> + fi
> + count=$((count+1))
> +done <<< "`run_cmd $MYSQL $MYSQL_ARGS -Nse 'SHOW MASTER STATUS\G' mysql`"
> +
> +stop_server
> +
> +found_logs=`find $backup_dir -type f -name $master_file`

found_logs is unused? It should probably be used on the next line instead of $master_file, but why do you need to use 'find', i.e. recursive directory search? You could just use "if [ -f $backup_dir/$master_file ]" on the next line.

> +if [ -z "$master_file" ]
> +then
> + vlog "Could not find bin-log file in backup set, expecting to find $master_file"
> + exit 1
> +fi

We should be able to actualy run this test case in Jenkins. I pinged DS about adding galera56 to Jenkins matrix.

review: Needs Fixing
Revision history for this message
George Ormond Lorch III (gl-az) wrote :

> The error message is wrong. Binary log must be enabled for GTID to work,
> so failure to get binlog coordinates is an impossible condition. But it
> certainly does not mean that the server is not a member of a
> cluster. Nor does it mean that the server is not a replication master.
>
> It does also not lead to the --galera-info option being ignored, because
> we do write the galera info file in this case.
>
> I would make it a fatal error. I.e.:
>
> die “Failed to get binlog coordinates from SHOW MASTER STATUS”

True, I wasn't sure if this should be fatal or not since the original --galera-info wasn't fatal when wsrep state couldn't be obtained. Fixed.

> gtid_mode=ON?
Didn't think about that. Fixed.

> master_pos is unused?

Yup, I suppose I could 'break' once we have the master_file. This was copied out of the sync_slave_with_master function and not reduced to the bare essentials for the test.

> found_logs is unused? It should probably be used on the next line instead of
> $master_file, but why do you need to use 'find', i.e. recursive directory
> search? You could just use "if [ -f $backup_dir/$master_file ]" on the next
> line.

True, that was a typo, found_logs was intended to be used here. Originally I didn't use the SHOW MASTER STATUS and was just going to use something like 'find $backup_dir -name `hostname`-bin*' but decided that perhaps a more explicit test should be done for the specific log file name. With that in place the simple 'if [ -f $backup_dir/$master_File ]' is now a more appropriate way to go.

> We should be able to actualy run this test case in Jenkins. I pinged DS about
> adding galera56 to Jenkins matrix.

Agreed.

Revision history for this message
Alexey Kopytov (akopytov) wrote :

After running it through PXC 5.6 enabled Jenkins:

- log_slave_updates is also a pre-requisite for gtid_mode
- $con->{master_status}->{Executed_Gtid_Set} is always defined on a 5.6 server (but is empty, if GTID is disabled).

I will fix it in the merge revision.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'innobackupex.pl'
2--- innobackupex.pl 2014-01-03 10:46:10 +0000
3+++ innobackupex.pl 2014-01-15 18:24:12 +0000
4@@ -3120,6 +3120,28 @@
5
6 write_to_backup_file("$galera_info", "$state_uuid" .
7 ":" . "$last_committed" . "\n");
8+
9+ if (!defined($con->{master_status})) {
10+ get_mysql_master_status($con);
11+ }
12+
13+ if (defined($con->{master_status}->{Executed_Gtid_Set})) {
14+ my $log_bin_dir;
15+ my $log_bin_file;
16+
17+ mysql_query($con, "FLUSH BINARY LOGS");
18+ get_mysql_master_status($con);
19+
20+ $log_bin_file = $con->{master_status}->{File};
21+ $log_bin_dir = File::Basename::dirname($mysql{vars}->{log_bin_basename}->{Value});
22+
23+ if (!defined($log_bin_file) || !defined($log_bin_dir)) {
24+ die "Failed to get master binlog coordinates from SHOW MASTER STATUS";
25+ }
26+
27+ backup_file("$log_bin_dir", "$log_bin_file",
28+ "$backup_dir/$log_bin_dir/$log_bin_file")
29+ }
30 }
31
32
33
34=== added file 'test/t/bug1250375.sh'
35--- test/t/bug1250375.sh 1970-01-01 00:00:00 +0000
36+++ test/t/bug1250375.sh 2014-01-15 18:24:12 +0000
37@@ -0,0 +1,38 @@
38+. inc/common.sh
39+
40+require_galera
41+require_server_version_higher_than 5.6.0
42+
43+ADDR=127.0.0.1
44+
45+if [[ -n ${WSREP_DEBUG:-} ]];then
46+ start_server --log-bin=`hostname`-bin --binlog-format=ROW --wsrep-provider=${MYSQL_BASEDIR}/lib/libgalera_smm.so --wsrep_cluster_address=gcomm:// --wsrep-debug=1 --wsrep_provider_options="debug=1" --wsrep_node_address=$ADDR --gtid_mode=ON
47+else
48+ start_server --log-bin=`hostname`-bin --binlog-format=ROW --wsrep-provider=${MYSQL_BASEDIR}/lib/libgalera_smm.so --wsrep_cluster_address=gcomm:// --wsrep_node_address=$ADDR --gtid_mode=ON
49+
50+fi
51+
52+
53+innobackupex --no-timestamp --galera-info $topdir/backup
54+backup_dir=$topdir/backup
55+vlog "Backup created in directory $backup_dir"
56+
57+count=0
58+master_file=
59+
60+while read line; do
61+ if [ $count -eq 1 ] # File:
62+ then
63+ master_file=`echo "$line" | sed s/File://`
64+ break
65+ fi
66+ count=$((count+1))
67+done <<< "`run_cmd $MYSQL $MYSQL_ARGS -Nse 'SHOW MASTER STATUS\G' mysql`"
68+
69+stop_server
70+
71+if [ -f "$backup_dir/$master_file" ]
72+then
73+ vlog "Could not find bin-log file in backup set, expecting to find $master_file"
74+ exit 1
75+fi

Subscribers

People subscribed via source and target branches