Merge lp:~akopytov/percona-xtrabackup/bug1095249-2.1 into lp:percona-xtrabackup/2.1

[Alexey Kopytov]

[George Ormond Lorch III (gl-az)]

Looks good except for tw things:
1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
2 - there is no call to ds_buffer_set_size to set the buffering size to 1M from the default 64K...does it make sense to make this a program option?

This brings up two issue in/with the 2.1 encryption work, small writes from compress->encrypt (encrypting extremely small buffers) and inefficient i/o. Also IIRC the encryption work implemented similar test case for basic encryption as I found it was missing. Should probably echo these comments on that MP and fix topology creation and destruction accordingly. This could be our first case of using multiple instances of a single data sink if we choose to go that route:
ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local

review: Approve (g2)

[Alexey Kopytov (akopytov)]

Hi George,

On Thu, 03 Jan 2013 17:47:22 -0000, George Ormond Lorch III wrote:
> Review: Approve g2
>
> Looks good except for tw things:
> 1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
> 2 - there is no call to ds_buffer_set_size to set the buffering size to 1M from the default 64K...does it make sense to make this a program option?
>

Good catch. Removed redundant includes and added a ds_buffer_set_size()
call.

Regarding making it a program option. We have
https://blueprints.launchpad.net/percona-xtrabackup/+spec/specify-io-block-size
which can make both read and write buffers configurable. So far they are
both hardcoded to 1 MB.

> This brings up two issue in/with the 2.1 encryption work, small writes from compress->encrypt (encrypting extremely small buffers) and inefficient i/o. Also IIRC the encryption work implemented similar test case for basic encryption as I found it was missing. Should probably echo these comments on that MP and fix topology creation and destruction accordingly. This could be our first case of using multiple instances of a single data sink if we choose to go that route:
> ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local
>

Yes, I see encryption also writes metadata in a separate write call, so
it will still be affected by the same problem even when compression will
use buffering (or no compression is used).

Thanks!

[Alexey Kopytov (akopytov)]

No idea why Launchpad decided to change the status to Merged. Reverting.

[George Ormond Lorch III (gl-az)]

> Hi George,
>
> On Thu, 03 Jan 2013 17:47:22 -0000, George Ormond Lorch III wrote:
> > Review: Approve g2
> >
> > Looks good except for tw things:
> > 1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really
> necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
> > 2 - there is no call to ds_buffer_set_size to set the buffering size to 1M
> from the default 64K...does it make sense to make this a program option?
> >
>
> Good catch. Removed redundant includes and added a ds_buffer_set_size()
> call.
>
> Regarding making it a program option. We have
> https://blueprints.launchpad.net/percona-xtrabackup/+spec/specify-io-block-
> size
> which can make both read and write buffers configurable. So far they are
> both hardcoded to 1 MB.
>
> > This brings up two issue in/with the 2.1 encryption work, small writes from
> compress->encrypt (encrypting extremely small buffers) and inefficient i/o.
> Also IIRC the encryption work implemented similar test case for basic
> encryption as I found it was missing. Should probably echo these comments on
> that MP and fix topology creation and destruction accordingly. This could be
> our first case of using multiple instances of a single data sink if we choose
> to go that route:
> > ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local
> >
>
> Yes, I see encryption also writes metadata in a separate write call, so
> it will still be affected by the same problem even when compression will
> use buffering (or no compression is used).
>
> Thanks!

Also FYI in case you really haven't had a chance to review much of the encryption work yet, I went ahead and exposed both compress and encrypt block sizes as program options which allow tuning of the individual compress and encrypt worker thread block sizes. In my testing these can actually have a pretty decent impact on overall backup speed when set properly (and the tuning ratios are not entirely intuitive). This is what lead me to propose the PLMCE 2013 tutorial on tuning XB options for best results. With this new i/o buffer, it may make the current results much more interesting. Once this change is merged into 2.1, I will rebase the encryption branch and also look into implementing the i/o block size program options ahead of it which should be simple enough. It would be nice to have all of these in the trunk by the time for PLMCE 2013 and included into my talk.

[Alexey Kopytov (akopytov)]

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/135/ (though compression tests are not currently executed by Jenkins anyway).

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

=== modified file 'src/Makefile'
--- src/Makefile	2012-05-25 11:38:15 +0000
+++ src/Makefile	2013-01-07 06:21:20 +0000
@@ -28,6 +28,7 @@
 	ds_local.o \
 	ds_compress.o \
 	ds_tmpfile.o \
+	ds_buffer.o \
 	datasink.o \
 	write_filt.o \
 	fil_cur.o \
@@ -185,7 +186,7 @@
 xbstream.o xbstream_read.o: %.o: %.c
 	$(CC) $(CFLAGS) $(INC) $(DEFS) -c $< -o $@
 
-xbstream: $(XBSTREAMOBJS) $(MYSQLOBJS) ds_local.o datasink.o
+xbstream: $(XBSTREAMOBJS) $(MYSQLOBJS) ds_local.o ds_buffer.o datasink.o
 	$(CC) $(CFLAGS) $^ $(INC) $(MYSQLOBJS) $(LIBS) -o $@
 
 xtrabackup.o: xtrabackup.c xb_regex.h write_filt.h fil_cur.h xtrabackup.h
 
=== modified file 'src/datasink.c'
--- src/datasink.c	2012-02-16 18:02:07 +0000
+++ src/datasink.c	2013-01-07 06:21:20 +0000
@@ -25,6 +25,7 @@
 #include "ds_stream.h"
 #include "ds_local.h"
 #include "ds_tmpfile.h"
+#include "ds_buffer.h"
 
 /************************************************************************
 Create a datasink of the specified type */
@@ -47,6 +48,9 @@
 	case DS_TYPE_TMPFILE:
 		ds = &datasink_tmpfile;
 		break;
+	case DS_TYPE_BUFFER:
+		ds = &datasink_buffer;
+		break;
 	default:
 		msg("Unknown datasink type: %d\n", type);
 		return NULL;
 
=== modified file 'src/datasink.h'
--- src/datasink.h	2012-02-16 18:02:07 +0000
+++ src/datasink.h	2013-01-07 06:21:20 +0000
@@ -53,7 +53,8 @@
 	DS_TYPE_LOCAL,
 	DS_TYPE_STREAM,
 	DS_TYPE_COMPRESS,
-	DS_TYPE_TMPFILE
+	DS_TYPE_TMPFILE,
+	DS_TYPE_BUFFER
 } ds_type_t;
 
 /************************************************************************
 
=== added file 'src/ds_buffer.c'
--- src/ds_buffer.c	1970-01-01 00:00:00 +0000
+++ src/ds_buffer.c	2013-01-07 06:21:20 +0000
@@ -0,0 +1,186 @@
+/******************************************************
+Copyright (c) 2012 Percona Ireland Ltd.
+
+buffer datasink for XtraBackup.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; version 2 of the License.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; if not, write to the Free Software
+Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+
+*******************************************************/
+
+/* Does buffered output to a destination datasink set with ds_set_pipe().
+Writes to the destination datasink are guaranteed to not be smaller than a
+specified buffer size (DS_DEFAULT_BUFFER_SIZE by default), with the only
+exception for the last write for a file. */
+
+#include <my_base.h>
+#include "common.h"
+#include "datasink.h"
+
+#define DS_DEFAULT_BUFFER_SIZE (64 * 1024)
+
+typedef struct {
+	ds_file_t	*dst_file;
+	char		*buf;
+	size_t		 pos;
+	size_t		size;
+} ds_buffer_file_t;
+
+typedef struct {
+	size_t	buffer_size;
+} ds_buffer_ctxt_t;
+
+static ds_ctxt_t *buffer_init(const char *root);
+static ds_file_t *buffer_open(ds_ctxt_t *ctxt, const char *path,
+			      MY_STAT *mystat);
+static int buffer_write(ds_file_t *file, const void *buf, size_t len);
+static int buffer_close(ds_file_t *file);
+static void buffer_deinit(ds_ctxt_t *ctxt);
+
+datasink_t datasink_buffer = {
+	&buffer_init,
+	&buffer_open,
+	&buffer_write,
+	&buffer_close,
+	&buffer_deinit
+};
+
+/* Change the default buffer size */
+void ds_buffer_set_size(ds_ctxt_t *ctxt, size_t size)
+{
+	ds_buffer_ctxt_t *buffer_ctxt = (ds_buffer_ctxt_t *) ctxt->ptr;
+
+	buffer_ctxt->buffer_size = size;
+}
+
+static ds_ctxt_t *
+buffer_init(const char *root)
+{
+	ds_ctxt_t		*ctxt;
+	ds_buffer_ctxt_t	*buffer_ctxt;
+
+	ctxt = my_malloc(sizeof(ds_ctxt_t) + sizeof(ds_buffer_ctxt_t),
+			 MYF(MY_FAE));
+	buffer_ctxt = (ds_buffer_ctxt_t *) (ctxt + 1);
+	buffer_ctxt->buffer_size = DS_DEFAULT_BUFFER_SIZE;
+
+	ctxt->ptr = buffer_ctxt;
+	ctxt->root = my_strdup(root, MYF(MY_FAE));
+
+	return ctxt;
+}
+
+static ds_file_t *
+buffer_open(ds_ctxt_t *ctxt, const char *path, MY_STAT *mystat)
+{
+	ds_buffer_ctxt_t	*buffer_ctxt;
+	ds_ctxt_t		*pipe_ctxt;
+	ds_file_t		*dst_file;
+	ds_file_t		*file;
+	ds_buffer_file_t	*buffer_file;
+
+	pipe_ctxt = ctxt->pipe_ctxt;
+	xb_a(pipe_ctxt != NULL);
+
+	dst_file = ds_open(pipe_ctxt, path, mystat);
+	if (dst_file == NULL) {
+		exit(EXIT_FAILURE);
+	}
+
+	buffer_ctxt = (ds_buffer_ctxt_t *) ctxt->ptr;
+
+	file = (ds_file_t *) my_malloc(sizeof(ds_file_t) +
+				       sizeof(ds_buffer_file_t) +
+				       buffer_ctxt->buffer_size,
+				       MYF(MY_FAE));
+
+	buffer_file = (ds_buffer_file_t *) (file + 1);
+	buffer_file->dst_file = dst_file;
+	buffer_file->buf = (char *) (buffer_file + 1);
+	buffer_file->size = buffer_ctxt->buffer_size;
+	buffer_file->pos = 0;
+
+	file->path = dst_file->path;
+	file->ptr = buffer_file;
+
+	return file;
+}
+
+static int
+buffer_write(ds_file_t *file, const void *buf, size_t len)
+{
+	ds_buffer_file_t	*buffer_file;
+
+	buffer_file = (ds_buffer_file_t *) file->ptr;
+
+	while (len > 0) {
+		if (buffer_file->pos + len > buffer_file->size) {
+			if (buffer_file->pos > 0) {
+				size_t bytes;
+
+				bytes = buffer_file->size - buffer_file->pos;
+				memcpy(buffer_file->buf + buffer_file->pos, buf,
+				       bytes);
+
+				if (ds_write(buffer_file->dst_file,
+					     buffer_file->buf,
+					     buffer_file->size)) {
+					return 1;
+				}
+
+				buffer_file->pos = 0;
+
+				buf = (const char *) buf + bytes;
+				len -= bytes;
+			} else {
+				/* We don't have any buffered bytes, just write
+				the entire source buffer */
+				if (ds_write(buffer_file->dst_file, buf, len)) {
+					return 1;
+				}
+				break;
+			}
+		} else {
+			memcpy(buffer_file->buf + buffer_file->pos, buf, len);
+			buffer_file->pos += len;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int
+buffer_close(ds_file_t *file)
+{
+	ds_buffer_file_t	*buffer_file;
+
+	buffer_file = (ds_buffer_file_t *) file->ptr;
+	if (buffer_file->pos > 0) {
+		ds_write(buffer_file->dst_file, buffer_file->buf,
+			 buffer_file->pos);
+	}
+
+	ds_close(buffer_file->dst_file);
+
+	MY_FREE(file);
+
+	return 0;
+}
+
+static void
+buffer_deinit(ds_ctxt_t *ctxt)
+{
+	MY_FREE(ctxt->root);
+	MY_FREE(ctxt);
+}
 
=== added file 'src/ds_buffer.h'
--- src/ds_buffer.h	1970-01-01 00:00:00 +0000
+++ src/ds_buffer.h	2013-01-07 06:21:20 +0000
@@ -0,0 +1,31 @@
+/******************************************************
+Copyright (c) 2012 Percona Ireland Ltd.
+
+buffer datasink for XtraBackup.
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; version 2 of the License.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program; if not, write to the Free Software
+Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+
+*******************************************************/
+
+#ifndef DS_BUFFER_H
+#define DS_BUFFER_H
+
+#include "datasink.h"
+
+extern datasink_t datasink_buffer;
+
+/* Change the default buffer size */
+void ds_buffer_set_size(ds_ctxt_t *ctxt, size_t size);
+
+#endif
 
=== modified file 'src/ds_compress.c'
--- src/ds_compress.c	2012-06-14 14:48:55 +0000
+++ src/ds_compress.c	2013-01-07 06:21:20 +0000
@@ -25,8 +25,6 @@
 #include <zlib.h>
 #include "common.h"
 #include "datasink.h"
-#include "ds_stream.h"
-#include "ds_local.h"
 
 #define COMPRESS_CHUNK_SIZE (64 * 1024UL)
 #define MY_QLZ_COMPRESS_OVERHEAD 400
 
=== modified file 'src/xtrabackup.c'
--- src/xtrabackup.c	2012-11-21 13:51:49 +0000
+++ src/xtrabackup.c	2013-01-07 06:21:20 +0000
@@ -87,6 +87,7 @@
 #include "fil_cur.h"
 #include "write_filt.h"
 #include "xtrabackup.h"
+#include "ds_buffer.h"
 
 my_bool innodb_inited= 0;
 
@@ -261,6 +262,7 @@
 static  ds_ctxt_t       *ds_compress = NULL;
 static  ds_ctxt_t       *ds_tmpfile  = NULL;
 static  ds_ctxt_t       *ds_stream   = NULL;
+static  ds_ctxt_t       *ds_buffer   = NULL;
 
 /* ======== Datafiles iterator ======== */
 typedef struct {
@@ -2124,21 +2126,28 @@
 		if (xtrabackup_compress) {
 			ds_compress = ds_create(xtrabackup_target_dir,
 						DS_TYPE_COMPRESS);
+			/* Use a 1 MB buffer for compressed output stream */
+			ds_buffer = ds_create(xtrabackup_target_dir,
+					      DS_TYPE_BUFFER);
+			ds_buffer_set_size(ds_buffer, 1024 * 1024);
+
 			ds_data = ds_compress;
 
 			if (xtrabackup_stream) {
 				/* Streaming compressed backup */
-				ds_set_pipe(ds_compress, ds_stream);
+				ds_set_pipe(ds_buffer, ds_stream);
 				/* Bypass compression for meta files */
 				ds_meta = ds_stream;
 			} else {
 				/* Local compressed backup */
 				ds_local = ds_create(xtrabackup_target_dir,
 						     DS_TYPE_LOCAL);
-				ds_set_pipe(ds_compress, ds_local);
+				ds_set_pipe(ds_buffer, ds_local);
 				/* Bypass compression for meta files */
 				ds_meta = ds_local;
 			}
+
+			ds_set_pipe(ds_compress, ds_buffer);
 		} else {
 			/* Streaming uncompressed backup */
 			ds_data = ds_stream;
@@ -2177,6 +2186,10 @@
 		ds_destroy(ds_compress);
 		ds_compress = NULL;
 	}
+	if (ds_buffer != NULL) {
+		ds_destroy(ds_buffer);
+		ds_buffer = NULL;
+	}
 	if (ds_stream != NULL) {
 		ds_destroy(ds_stream);
 		ds_stream = NULL;
 
=== added file 'test/t/ib_compress_basic.sh'
--- test/t/ib_compress_basic.sh	1970-01-01 00:00:00 +0000
+++ test/t/ib_compress_basic.sh	2013-01-07 06:21:20 +0000
@@ -0,0 +1,32 @@
+########################################################################
+# Basic test for local compressed backups
+########################################################################
+
+. inc/common.sh
+
+if ! which qpress > /dev/null 2>&1 ; then
+  echo "Requires qpress to be installed" > $SKIPPED_REASON
+  exit $SKIPPED_EXIT_CODE
+fi
+
+start_server --innodb_file_per_table
+load_sakila
+
+innobackupex --compress --no-timestamp $topdir/backup
+
+stop_server
+rm -rf ${MYSQLD_DATADIR}/*
+
+cd $topdir/backup
+
+for i in *.qp;  do qpress -d $i ./; done; \
+for i in sakila/*.qp; do qpress -d $i sakila/; done
+
+innobackupex --apply-log $topdir/backup
+
+innobackupex --copy-back $topdir/backup
+
+start_server
+
+# TODO: proper validation
+run_cmd ${MYSQL} ${MYSQL_ARGS} -e "SELECT count(*) from actor" sakila