Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 1/1] [tosquash] xrow: encode CONFIRM/ROLLBACK on txn region
@ 2020-07-07 23:32 Vladislav Shpilevoy
  0 siblings, 0 replies; only message in thread
From: Vladislav Shpilevoy @ 2020-07-07 23:32 UTC (permalink / raw)
  To: tarantool-patches

There were 2 bugs: one memory leak and one memory corruption.

The leak was in txn_limbo_write_confirm_rollback(). This function
used fiber->gc region to encode CONFIRM/ROLLBACK, but never freed
it.

The corruption was in applier.cc in process_confirm_rollback().
CONFIRM/ROLLBACK were stored on the applier's ibuf. As a result,
if applier experiences relatively intensive load, the ibuf will
be quickly recycled, right during a WAL write of CONFIRM/ROLLBACK
stored on it.

DML requests would have the same problem, but they were copied in
txn_add_redo() inside of xrow_encode_dml() call. This patch makes
synchro requests copied as well.

This commit should be squashed into the commits introducing
CONFIRM/ROLLBACK entries and their handling.

Closes #5138
---
Branch: http://github.com/tarantool/tarantool/tree/gh-4842-sync-replication
Issue 1: https://github.com/tarantool/tarantool/issues/4842
Issue 2: https://github.com/tarantool/tarantool/issues/5138

 src/box/txn.c                         |  2 ++
 src/box/txn_limbo.c                   | 15 +++++----
 src/box/xrow.c                        | 31 ++++++++++++++----
 src/box/xrow.h                        | 16 ++++++++--
 test/replication/qsync_basic.result   | 46 +++++++++++++++++++++++++++
 test/replication/qsync_basic.test.lua | 18 +++++++++++
 6 files changed, 113 insertions(+), 15 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 59145eef8..205e5e9f9 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -88,6 +88,8 @@ txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
 	 */
 	if (likely(!iproto_type_is_synchro_request(row->type)))
 		row->bodycnt = xrow_encode_dml(request, &txn->region, row->body);
+	else
+		row->bodycnt = xrow_header_dup_body(row, &txn->region);
 	if (row->bodycnt < 0)
 		return -1;
 	stmt->row = row;
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index f22ca1123..884c188b2 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -237,22 +237,25 @@ txn_limbo_write_confirm_rollback(struct txn_limbo *limbo, int64_t lsn,
 		.header = &row,
 	};
 
+	struct txn *txn = txn_begin();
+	if (txn == NULL)
+		return -1;
+
 	int res = 0;
 	if (is_confirm) {
-		res = xrow_encode_confirm(&row, limbo->instance_id, lsn);
+		res = xrow_encode_confirm(&row, &txn->region,
+					  limbo->instance_id, lsn);
 	} else {
 		/*
 		 * This LSN is the first to be rolled back, so
 		 * the last "safe" lsn is lsn - 1.
 		 */
-		res = xrow_encode_rollback(&row, limbo->instance_id, lsn - 1);
+		res = xrow_encode_rollback(&row, &txn->region,
+					   limbo->instance_id, lsn - 1);
 	}
 	if (res == -1)
-		return -1;
+		goto rollback;
 
-	struct txn *txn = txn_begin();
-	if (txn == NULL)
-		return -1;
 	/*
 	 * This is not really a transaction. It just uses txn API
 	 * to put the data into WAL. And obviously it should not
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 39d1814c4..0c797a9d5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -220,6 +220,21 @@ error:
 	return 0;
 }
 
+int
+xrow_header_dup_body(struct xrow_header *row, struct region *region)
+{
+	assert(row->bodycnt == 1);
+	size_t size = row->body[0].iov_len;
+	char *copy = (char *)region_alloc(region, size);
+	if (copy == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "copy");
+		return -1;
+	}
+	memcpy(copy, row->body[0].iov_base, size);
+	row->body[0].iov_base = copy;
+	return 1;
+}
+
 /**
  * @pre pos points at a valid msgpack
  */
@@ -879,13 +894,13 @@ xrow_encode_dml(const struct request *request, struct region *region,
 }
 
 static int
-xrow_encode_confirm_rollback(struct xrow_header *row, uint32_t replica_id,
-			     int64_t lsn, int type)
+xrow_encode_confirm_rollback(struct xrow_header *row, struct region *region,
+			     uint32_t replica_id, int64_t lsn, int type)
 {
 	size_t len = mp_sizeof_map(2) + mp_sizeof_uint(IPROTO_REPLICA_ID) +
 		     mp_sizeof_uint(replica_id) + mp_sizeof_uint(IPROTO_LSN) +
 		     mp_sizeof_uint(lsn);
-	char *buf = (char *)region_alloc(&fiber()->gc, len);
+	char *buf = (char *)region_alloc(region, len);
 	if (buf == NULL) {
 		diag_set(OutOfMemory, len, "region_alloc", "buf");
 		return -1;
@@ -910,16 +925,18 @@ xrow_encode_confirm_rollback(struct xrow_header *row, uint32_t replica_id,
 }
 
 int
-xrow_encode_confirm(struct xrow_header *row, uint32_t replica_id, int64_t lsn)
+xrow_encode_confirm(struct xrow_header *row, struct region *region,
+		    uint32_t replica_id, int64_t lsn)
 {
-	return xrow_encode_confirm_rollback(row, replica_id, lsn,
+	return xrow_encode_confirm_rollback(row, region, replica_id, lsn,
 					    IPROTO_CONFIRM);
 }
 
 int
-xrow_encode_rollback(struct xrow_header *row, uint32_t replica_id, int64_t lsn)
+xrow_encode_rollback(struct xrow_header *row, struct region *region,
+		     uint32_t replica_id, int64_t lsn)
 {
-	return xrow_encode_confirm_rollback(row, replica_id, lsn,
+	return xrow_encode_confirm_rollback(row, region, replica_id, lsn,
 					    IPROTO_ROLLBACK);
 }
 
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 1def394e7..7e6a4aceb 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -141,6 +141,14 @@ int
 xrow_header_decode(struct xrow_header *header, const char **pos,
 		   const char *end, bool end_is_exact);
 
+/**
+ * Duplicate the xrow's body onto the given region.
+ * @retval -1 Error.
+ * @retval >= 0 Iov count in the body.
+ */
+int
+xrow_header_dup_body(struct xrow_header *header, struct region *region);
+
 /**
  * DML request.
  */
@@ -211,13 +219,15 @@ xrow_encode_dml(const struct request *request, struct region *region,
  * Encode the CONFIRM to row body and set row type to
  * IPROTO_CONFIRM.
  * @param row xrow header.
+ * @param region Region to use to encode the confirmation body.
  * @param replica_id master's instance id.
  * @param lsn last confirmed lsn.
  * @retval -1 on error.
  * @retval 0 success.
  */
 int
-xrow_encode_confirm(struct xrow_header *row, uint32_t replica_id, int64_t lsn);
+xrow_encode_confirm(struct xrow_header *row, struct region *region,
+		    uint32_t replica_id, int64_t lsn);
 
 /**
  * Decode the CONFIRM request body.
@@ -234,13 +244,15 @@ xrow_decode_confirm(struct xrow_header *row, uint32_t *replica_id, int64_t *lsn)
  * Encode the ROLLBACK row body and set row type to
  * IPROTO_ROLLBACK.
  * @param row xrow header.
+ * @param region Region to use to encode the rollback body.
  * @param replica_id master's instance id.
  * @param lsn lsn to rollback to.
  * @retval -1  on error.
  * @retval 0 success.
  */
 int
-xrow_encode_rollback(struct xrow_header *row, uint32_t replica_id, int64_t lsn);
+xrow_encode_rollback(struct xrow_header *row, struct region *region,
+		     uint32_t replica_id, int64_t lsn);
 
 /**
  * Decode the ROLLBACK row body.
diff --git a/test/replication/qsync_basic.result b/test/replication/qsync_basic.result
index 59f5d9123..fa93b0263 100644
--- a/test/replication/qsync_basic.result
+++ b/test/replication/qsync_basic.result
@@ -565,6 +565,52 @@ box.space.sync:select{12}
  | - - [12]
  | ...
 
+--
+-- gh-5138: synchro rows were not saved onto txns region, and
+-- could get corrupted under load.
+--
+test_run:switch('default')
+ | ---
+ | - true
+ | ...
+box.cfg{replication_synchro_timeout = 1000}
+ | ---
+ | ...
+for i = 1, 100 do box.space.sync:replace{i} end
+ | ---
+ | ...
+test_run:cmd('switch replica')
+ | ---
+ | - true
+ | ...
+box.space.sync:count()
+ | ---
+ | - 100
+ | ...
+-- Rows could be corrupted during WAL writes. Restart should
+-- reveal the problem during recovery.
+test_run:cmd('restart server replica')
+ | 
+box.space.sync:count()
+ | ---
+ | - 100
+ | ...
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+for i = 1, 100 do box.space.sync:delete{i} end
+ | ---
+ | ...
+test_run:cmd('switch replica')
+ | ---
+ | - true
+ | ...
+box.space.sync:count()
+ | ---
+ | - 0
+ | ...
+
 -- Cleanup.
 test_run:cmd('switch default')
  | ---
diff --git a/test/replication/qsync_basic.test.lua b/test/replication/qsync_basic.test.lua
index 1bb3ba87d..043e6b4ba 100644
--- a/test/replication/qsync_basic.test.lua
+++ b/test/replication/qsync_basic.test.lua
@@ -230,6 +230,24 @@ box.space.sync:select{12}
 test_run:switch('replica')
 box.space.sync:select{12}
 
+--
+-- gh-5138: synchro rows were not saved onto txns region, and
+-- could get corrupted under load.
+--
+test_run:switch('default')
+box.cfg{replication_synchro_timeout = 1000}
+for i = 1, 100 do box.space.sync:replace{i} end
+test_run:cmd('switch replica')
+box.space.sync:count()
+-- Rows could be corrupted during WAL writes. Restart should
+-- reveal the problem during recovery.
+test_run:cmd('restart server replica')
+box.space.sync:count()
+test_run:cmd('switch default')
+for i = 1, 100 do box.space.sync:delete{i} end
+test_run:cmd('switch replica')
+box.space.sync:count()
+
 -- Cleanup.
 test_run:cmd('switch default')
 
-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2020-07-07 23:32 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 23:32 [Tarantool-patches] [PATCH 1/1] [tosquash] xrow: encode CONFIRM/ROLLBACK on txn region Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox