Merge lp:~sergei.glushchenko/percona-xtrabackup/bug740254-memleak-stats into lp:percona-xtrabackup/2.0

[Sergei Glushchenko]

Bug740254
Memory leak in xtrabackup_stats_level when calling rec_get_offsets inside
loop. It happens when table has more then REC_OFFS_NORMAL_SIZE columns.
Fix include testcase which do xtrabackup --stats on backup which contains
table with 1000 columns.

[Sergei Glushchenko (sergei.glushchenko)]

Jenkins:
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-param/160/

[Laurynas Biveinis (laurynas-biveinis)]

For the reference, here is diff -b of the relevant part:

@@ -4829,9 +4576,14 @@
 #endif
        }

-loop:
        mem_heap_empty(heap);
- offsets = NULL;
+
+ ulint offsets_array[REC_OFFS_NORMAL_SIZE];
+ ulint* local_offsets = offsets_array;
+ *offsets_array = (sizeof offsets_array) / sizeof *offsets_array;
+
+ for (;;) {
+
 #if (MYSQL_VERSION_ID < 50100)
        mtr_x_lock(&(index->tree->lock), &mtr);
 #else /* MYSQL_VERSION_ID < 51000 */
@@ -4848,16 +4600,10 @@
        sum_data += page_get_data_size(page);
        n_recs += page_get_n_recs(page);

-
        if (level == 0) {
                page_cur_t cur;
                ulint n_fields;
                ulint i;
- mem_heap_t* local_heap = NULL;
- ulint offsets_[REC_OFFS_NORMAL_SIZE];
- ulint* local_offsets = offsets_;
-
- *offsets_ = (sizeof offsets_) / sizeof *offsets_;

 #ifndef INNODB_VERSION_SHORT
                page_cur_set_before_first(page, &cur);
@@ -4872,7 +4618,7 @@
                        }

                        local_offsets = rec_get_offsets(cur.rec, index, local_offsets,
- ULINT_UNDEFINED, &local_heap);
+ ULINT_UNDEFINED, &heap);
                        n_fields = rec_offs_n_fields(local_offsets);

                        for (i = 0; i < n_fields; i++) {
@@ -4955,7 +4701,8 @@

        mtr_commit(&mtr);
- if (right_page_no != FIL_NULL) {
+ if (right_page_no == FIL_NULL)
+ break;
                mtr_start(&mtr);
 #ifndef INNODB_VERSION_SHORT
                page = btr_page_get(space, right_page_no, RW_X_LATCH, &mtr);
@@ -4969,8 +4716,8 @@
 #endif
                page = buf_block_get_frame(block);
 #endif
- goto loop;
        }
+
        mem_heap_free(heap);

 #ifndef INNODB_VERSION_SHORT

[Laurynas Biveinis (laurynas-biveinis)]

   Since you are touching the code anyway, in diff lines 127,141
   s/record/records.

   Please move all local var decls to the block start (in this case
   function start).

   The extra indentation makes a lot of lines to go over the line
   length limit. I am not sure if and how we'd want to fix this
   (especially for 2.1), please get a second opinion. IMVHO, we should
   leave it as-is for 2.0 and extract new function in 2.1.

review: Needs Fixing

Unmerged revisions

392. By Sergei Glushchenko on 2012-03-02

Bug740254
Memory leak in xtrabackup_stats_level when calling rec_get_offsets inside
loop. It happens when table has more then REC_OFFS_NORMAL_SIZE columns.
Fix include testcase which do xtrabackup --stats on backup which contains
table with 1000 columns.

=== modified file 'src/xtrabackup.c'
--- src/xtrabackup.c	2012-02-20 04:38:44 +0000
+++ src/xtrabackup.c	2012-03-07 05:03:18 +0000
@@ -4579,133 +4579,133 @@
 #endif
 	}
 
-loop:
 	mem_heap_empty(heap);
-	offsets = NULL;
+
+	ulint	offsets_array[REC_OFFS_NORMAL_SIZE];
+	ulint*	local_offsets	= offsets_array;
+	*offsets_array = (sizeof offsets_array) / sizeof *offsets_array;
+
+	for (;;) {
+
 #if (MYSQL_VERSION_ID < 50100)
-	mtr_x_lock(&(index->tree->lock), &mtr);
+		mtr_x_lock(&(index->tree->lock), &mtr);
 #else /* MYSQL_VERSION_ID < 51000 */
-	mtr_x_lock(&(index->lock), &mtr);
+		mtr_x_lock(&(index->lock), &mtr);
 #endif
 
-	right_page_no = btr_page_get_next(page, &mtr);
-
-
-	/*=================================*/
-	//fprintf(stdout, "%lu ", (ulint) buf_frame_get_page_no(page));
-
-	n_pages++;
-	sum_data += page_get_data_size(page);
-	n_recs += page_get_n_recs(page);
-
-
-	if (level == 0) {
-		page_cur_t	cur;
-		ulint	n_fields;
-		ulint	i;
-		mem_heap_t*	local_heap	= NULL;
-		ulint	offsets_[REC_OFFS_NORMAL_SIZE];
-		ulint*	local_offsets	= offsets_;
-
-		*offsets_ = (sizeof offsets_) / sizeof *offsets_;
+		right_page_no = btr_page_get_next(page, &mtr);
+
+
+		/*=================================*/
+		//fprintf(stdout, "%lu ", (ulint) buf_frame_get_page_no(page));
+
+		n_pages++;
+		sum_data += page_get_data_size(page);
+		n_recs += page_get_n_recs(page);
+
+		if (level == 0) {
+			page_cur_t	cur;
+			ulint	n_fields;
+			ulint	i;
 
 #ifndef INNODB_VERSION_SHORT
-		page_cur_set_before_first(page, &cur);
+			page_cur_set_before_first(page, &cur);
 #else
-		page_cur_set_before_first(block, &cur);
+			page_cur_set_before_first(block, &cur);
 #endif
-		page_cur_move_to_next(&cur);
-
-		for (;;) {
-			if (page_cur_is_after_last(&cur)) {
-				break;
-			}
-
-			local_offsets = rec_get_offsets(cur.rec, index, local_offsets,
-						ULINT_UNDEFINED, &local_heap);
-			n_fields = rec_offs_n_fields(local_offsets);
-
-			for (i = 0; i < n_fields; i++) {
-				if (rec_offs_nth_extern(local_offsets, i)) {
-					page_t*	local_page;
-					ulint	space_id;
-					ulint	page_no;
-					ulint	offset;
-					byte*	blob_header;
-					ulint	part_len;
-					mtr_t	local_mtr;
-					ulint	local_len;
-					byte*	data;
+			page_cur_move_to_next(&cur);
+
+			for (;;) {
+				if (page_cur_is_after_last(&cur)) {
+					break;
+				}
+
+				local_offsets = rec_get_offsets(cur.rec, index, local_offsets,
+							ULINT_UNDEFINED, &heap);
+				n_fields = rec_offs_n_fields(local_offsets);
+
+				for (i = 0; i < n_fields; i++) {
+					if (rec_offs_nth_extern(local_offsets, i)) {
+						page_t*	local_page;
+						ulint	space_id;
+						ulint	page_no;
+						ulint	offset;
+						byte*	blob_header;
+						ulint	part_len;
+						mtr_t	local_mtr;
+						ulint	local_len;
+						byte*	data;
 #ifdef INNODB_VERSION_SHORT
-					buf_block_t*	local_block;
+						buf_block_t*	local_block;
 #endif
 
-					data = rec_get_nth_field(cur.rec, local_offsets, i, &local_len);
-
-					ut_a(local_len >= BTR_EXTERN_FIELD_REF_SIZE);
-					local_len -= BTR_EXTERN_FIELD_REF_SIZE;
-
-					space_id = mach_read_from_4(data + local_len + BTR_EXTERN_SPACE_ID);
-					page_no = mach_read_from_4(data + local_len + BTR_EXTERN_PAGE_NO);
-					offset = mach_read_from_4(data + local_len + BTR_EXTERN_OFFSET);
-
-					if (offset != FIL_PAGE_DATA)
-						msg("\nWarning: several record may share same external page.\n");
-
-					for (;;) {
-						mtr_start(&local_mtr);
+						data = rec_get_nth_field(cur.rec, local_offsets, i, &local_len);
+
+						ut_a(local_len >= BTR_EXTERN_FIELD_REF_SIZE);
+						local_len -= BTR_EXTERN_FIELD_REF_SIZE;
+
+						space_id = mach_read_from_4(data + local_len + BTR_EXTERN_SPACE_ID);
+						page_no = mach_read_from_4(data + local_len + BTR_EXTERN_PAGE_NO);
+						offset = mach_read_from_4(data + local_len + BTR_EXTERN_OFFSET);
+
+						if (offset != FIL_PAGE_DATA)
+							msg("\nWarning: several record may share same external page.\n");
+
+						for (;;) {
+							mtr_start(&local_mtr);
 
 #ifndef INNODB_VERSION_SHORT
-						local_page = buf_page_get(space_id, page_no, RW_S_LATCH, &local_mtr);
+							local_page = buf_page_get(space_id, page_no, RW_S_LATCH, &local_mtr);
 #ifdef UNIV_SYNC_DEBUG
-						buf_page_dbg_add_level(local_page, SYNC_NO_ORDER_CHECK);
+							buf_page_dbg_add_level(local_page, SYNC_NO_ORDER_CHECK);
 #endif
 #else
 #if (MYSQL_VERSION_ID < 50517)
-						local_block = btr_block_get(space_id, zip_size, page_no, RW_S_LATCH, &local_mtr);
+							local_block = btr_block_get(space_id, zip_size, page_no, RW_S_LATCH, &local_mtr);
 #else
-						local_block = btr_block_get(space_id, zip_size, page_no, RW_S_LATCH, index, &local_mtr);
-#endif
-						local_page = buf_block_get_frame(local_block);
-#endif
-						blob_header = local_page + offset;
+							local_block = btr_block_get(space_id, zip_size, page_no, RW_S_LATCH, index, &local_mtr);
+#endif
+							local_page = buf_block_get_frame(local_block);
+#endif
+							blob_header = local_page + offset;
 #define BTR_BLOB_HDR_PART_LEN		0
 #define BTR_BLOB_HDR_NEXT_PAGE_NO	4
-						//part_len = btr_blob_get_part_len(blob_header);
-						part_len = mach_read_from_4(blob_header + BTR_BLOB_HDR_PART_LEN);
-
-						//page_no = btr_blob_get_next_page_no(blob_header);
-						page_no = mach_read_from_4(blob_header + BTR_BLOB_HDR_NEXT_PAGE_NO);
-
-						offset = FIL_PAGE_DATA;
-
-
-
-
-						/*=================================*/
-						//fprintf(stdout, "[%lu] ", (ulint) buf_frame_get_page_no(page));
-
-						n_pages_extern++;
-						sum_data_extern += part_len;
-
-
-						mtr_commit(&local_mtr);
-
-						if (page_no == FIL_NULL)
-							break;
+							//part_len = btr_blob_get_part_len(blob_header);
+							part_len = mach_read_from_4(blob_header + BTR_BLOB_HDR_PART_LEN);
+
+							//page_no = btr_blob_get_next_page_no(blob_header);
+							page_no = mach_read_from_4(blob_header + BTR_BLOB_HDR_NEXT_PAGE_NO);
+
+							offset = FIL_PAGE_DATA;
+
+
+
+
+							/*=================================*/
+							//fprintf(stdout, "[%lu] ", (ulint) buf_frame_get_page_no(page));
+
+							n_pages_extern++;
+							sum_data_extern += part_len;
+
+
+							mtr_commit(&local_mtr);
+
+							if (page_no == FIL_NULL)
+								break;
+						}
 					}
 				}
+
+				page_cur_move_to_next(&cur);
 			}
-
-			page_cur_move_to_next(&cur);
 		}
-	}
-
-
-
-
-	mtr_commit(&mtr);
-	if (right_page_no != FIL_NULL) {
+
+
+
+
+		mtr_commit(&mtr);
+		if (right_page_no == FIL_NULL)
+			break;
 		mtr_start(&mtr);
 #ifndef INNODB_VERSION_SHORT
 		page = btr_page_get(space, right_page_no, RW_X_LATCH, &mtr);
@@ -4719,8 +4719,8 @@
 #endif
 		page = buf_block_get_frame(block);
 #endif
-		goto loop;
 	}
+
 	mem_heap_free(heap);
 
 #ifndef INNODB_VERSION_SHORT

=== added file 'test/t/bug740254.sh'
--- test/t/bug740254.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug740254.sh	2012-03-07 05:03:18 +0000
@@ -0,0 +1,42 @@
+##################################################################
+# Bug #740254: Memory leak in xtrabackup --stats                 #
+##################################################################
+
+. inc/common.sh
+
+init
+run_mysqld
+
+# create table with 1000 columns
+
+CREATE_TABLE="CREATE TABLE t1 ("
+for i in {1..999}; do
+	CREATE_TABLE="${CREATE_TABLE}f${i} int,"
+done
+CREATE_TABLE="${CREATE_TABLE}f1000 int)"
+
+echo $CREATE_TABLE | run_cmd $MYSQL $MYSQL_ARGS test
+
+INS="INSERT INTO t1 values ("
+for i in {1..999}; do
+	INS="${INS}${i},"
+done
+INS="${INS}1000)"
+
+INS="$INS;$INS;$INS;$INS;"
+
+for i in {1..5} ; do
+	echo $INS | run_cmd $MYSQL $MYSQL_ARGS test
+done
+
+run_cmd $MYSQL $MYSQL_ARGS test -e "select * from t1"
+
+innobackupex  --no-timestamp $topdir/backup
+
+stop_mysqld
+
+vlog "Applying log"
+innobackupex --apply-log $topdir/backup
+
+# memmory leak might be here
+xtrabackup --stats --datadir=$topdir/backup