[Tarantool-patches] [PATCH v19 0/3] qsync: implement packet filtering (part 1)

[Tarantool-patches] [PATCH v19 0/3] qsync: implement packet filtering (part 1)

[Cyrill Gorcunov via Tarantool-patches]

Please take a look once time permit.

v19 (by Vlad):
 - do not modify box_issue_promote and demote (while they are
   still simply utter code duplication but whatever)
 - make txn_limbo_process being void
 - make txn_limbo_process_begin/commit/rollback being void
 - the real processing of request under the lock named as
   txn_limbo_process_core
 - testcase completely reworked (kudos to SergeP)
 - note that if we import test to the master branch without
   ordering pass it will fire assertion
 - dropped off debug info from box.info interface

Guys, also I noted that the test while being passed pretty fine locally
sometimes doesn't pass on CI due to timeout in waiting record {2} propagation,
so any advices on the test is quite welcome!

branch gorcunov/gh-6036-rollback-confirm-19
issue https://github.com/tarantool/tarantool/issues/6036
previous series https://lists.tarantool.org/tarantool-patches/20210922130535.79479-1-gorcunov@gmail.com/

Cyrill Gorcunov (3):
  latch: add latch_is_locked helper
  qsync: order access to the limbo terms
  test: add gh-6036-qsync-order test

 src/box/applier.cc                            |  12 +-
 src/box/box.cc                                |  15 +-
 src/box/txn_limbo.c                           |  17 +-
 src/box/txn_limbo.h                           |  53 ++++-
 src/lib/core/latch.h                          |  11 +
 test/replication/gh-6036-qsync-master.lua     |   1 +
 test/replication/gh-6036-qsync-node.lua       |  35 +++
 test/replication/gh-6036-qsync-order.result   | 207 ++++++++++++++++++
 test/replication/gh-6036-qsync-order.test.lua |  95 ++++++++
 test/replication/gh-6036-qsync-replica1.lua   |   1 +
 test/replication/gh-6036-qsync-replica2.lua   |   1 +
 test/replication/suite.cfg                    |   1 +
 test/replication/suite.ini                    |   2 +-
 13 files changed, 433 insertions(+), 18 deletions(-)
 create mode 120000 test/replication/gh-6036-qsync-master.lua
 create mode 100644 test/replication/gh-6036-qsync-node.lua
 create mode 100644 test/replication/gh-6036-qsync-order.result
 create mode 100644 test/replication/gh-6036-qsync-order.test.lua
 create mode 120000 test/replication/gh-6036-qsync-replica1.lua
 create mode 120000 test/replication/gh-6036-qsync-replica2.lua


base-commit: 33830978870bbe8cdc3c23fa06ba0f141a960124
-- 
2.31.1

[Tarantool-patches] [PATCH v19 1/3] latch: add latch_is_locked helper

[Cyrill Gorcunov via Tarantool-patches]

To test if latch is locked.

Part-of #6036

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/latch.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/lib/core/latch.h b/src/lib/core/latch.h
index 49c59cf63..0aaa8b634 100644
--- a/src/lib/core/latch.h
+++ b/src/lib/core/latch.h
@@ -95,6 +95,17 @@ latch_owner(struct latch *l)
 	return l->owner;
 }
 
+/**
+ * Return true if the latch is locked.
+ *
+ * @param l - latch to be tested.
+ */
+static inline bool
+latch_is_locked(const struct latch *l)
+{
+	return l->owner != NULL;
+}
+
 /**
  * Lock a latch. If the latch is already locked by another fiber,
  * waits for timeout.
-- 
2.31.1

[Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Cyrill Gorcunov via Tarantool-patches]

Limbo terms tracking is shared between appliers and when
one of appliers is waiting for write to complete inside
journal_write() routine, an other may need to access read
term value to figure out if promote request is valid to
apply. Due to cooperative multitasking access to the terms
is not consistent so we need to be sure that other fibers
read up to date terms (ie written to the WAL).

For this sake we use a latching mechanism, when one fiber
takes a lock for updating other readers are waiting until
the operation is complete.

For example here is a call graph of two appliers

applier 1
---------
applier_apply_tx
  (promote term = 3
   current max term = 2)
  applier_synchro_filter_tx
  apply_synchro_row
    journal_write
      (sleeping)

at this moment another applier comes in with obsolete
data and term 2

                              applier 2
                              ---------
                              applier_apply_tx
                                (term 2)
                                applier_synchro_filter_tx
                                  txn_limbo_is_replica_outdated -> false
                                journal_write (sleep)

applier 1
---------
journal wakes up
  apply_synchro_row_cb
    set max term to 3

So the applier 2 didn't notice that term 3 is already seen
and wrote obsolete data. With locking the applier 2 will
wait until applier 1 has finished its write.

We introduce the following helpers:

1) txn_limbo_process_begin: which takes a lock
2) txn_limbo_process_commit and txn_limbo_process_rollback
   which simply release the lock but have different names
   for better semantics
3) txn_limbo_process is a general function which uses x_begin
   and x_commit helper internally
4) txn_limbo_process_core to do a real job over processing the
   request, it implies that txn_limbo_process_begin been called

Part-of #6036

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/applier.cc  | 12 +++++++---
 src/box/box.cc      | 15 ++++++++-----
 src/box/txn_limbo.c | 17 +++++++++++++--
 src/box/txn_limbo.h | 53 ++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index b981bd436..46c36e259 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -857,7 +857,7 @@ apply_synchro_row_cb(struct journal_entry *entry)
 		applier_rollback_by_wal_io(entry->res);
 	} else {
 		replica_txn_wal_write_cb(synchro_entry->rcb);
-		txn_limbo_process(&txn_limbo, synchro_entry->req);
+		txn_limbo_process_core(&txn_limbo, synchro_entry->req);
 		trigger_run(&replicaset.applier.on_wal_write, NULL);
 	}
 	fiber_wakeup(synchro_entry->owner);
@@ -873,6 +873,8 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
 	if (xrow_decode_synchro(row, &req) != 0)
 		goto err;
 
+	txn_limbo_process_begin(&txn_limbo);
+
 	struct replica_cb_data rcb_data;
 	struct synchro_entry entry;
 	/*
@@ -910,12 +912,16 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
 	 * transactions side, including the async ones.
 	 */
 	if (journal_write(&entry.base) != 0)
-		goto err;
+		goto err_rollback;
 	if (entry.base.res < 0) {
 		diag_set_journal_res(entry.base.res);
-		goto err;
+		goto err_rollback;
 	}
+	txn_limbo_process_commit(&txn_limbo);
 	return 0;
+
+err_rollback:
+	txn_limbo_process_rollback(&txn_limbo);
 err:
 	diag_log();
 	return -1;
diff --git a/src/box/box.cc b/src/box/box.cc
index e082e1a3d..6a9be745a 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1677,8 +1677,6 @@ box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
 	struct raft *raft = box_raft();
 	assert(raft->volatile_term == raft->term);
 	assert(promote_lsn >= 0);
-	txn_limbo_write_promote(&txn_limbo, promote_lsn,
-				raft->term);
 	struct synchro_request req = {
 		.type = IPROTO_RAFT_PROMOTE,
 		.replica_id = prev_leader_id,
@@ -1686,8 +1684,11 @@ box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
 		.lsn = promote_lsn,
 		.term = raft->term,
 	};
-	txn_limbo_process(&txn_limbo, &req);
+	txn_limbo_process_begin(&txn_limbo);
+	txn_limbo_write_promote(&txn_limbo, req.lsn, req.term);
+	txn_limbo_process_core(&txn_limbo, &req);
 	assert(txn_limbo_is_empty(&txn_limbo));
+	txn_limbo_process_commit(&txn_limbo);
 }
 
 /** A guard to block multiple simultaneous promote()/demote() invocations. */
@@ -1699,8 +1700,6 @@ box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn)
 {
 	assert(box_raft()->volatile_term == box_raft()->term);
 	assert(promote_lsn >= 0);
-	txn_limbo_write_demote(&txn_limbo, promote_lsn,
-				box_raft()->term);
 	struct synchro_request req = {
 		.type = IPROTO_RAFT_DEMOTE,
 		.replica_id = prev_leader_id,
@@ -1708,8 +1707,12 @@ box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn)
 		.lsn = promote_lsn,
 		.term = box_raft()->term,
 	};
-	txn_limbo_process(&txn_limbo, &req);
+	txn_limbo_process_begin(&txn_limbo);
+	txn_limbo_write_demote(&txn_limbo, promote_lsn,
+				box_raft()->term);
+	txn_limbo_process_core(&txn_limbo, &req);
 	assert(txn_limbo_is_empty(&txn_limbo));
+	txn_limbo_process_commit(&txn_limbo);
 }
 
 int
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 70447caaf..855c98c98 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -47,6 +47,7 @@ txn_limbo_create(struct txn_limbo *limbo)
 	vclock_create(&limbo->vclock);
 	vclock_create(&limbo->promote_term_map);
 	limbo->promote_greatest_term = 0;
+	latch_create(&limbo->promote_latch);
 	limbo->confirmed_lsn = 0;
 	limbo->rollback_count = 0;
 	limbo->is_in_rollback = false;
@@ -724,11 +725,14 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double timeout)
 }
 
 void
-txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
+txn_limbo_process_core(struct txn_limbo *limbo,
+		       const struct synchro_request *req)
 {
+	assert(latch_is_locked(&limbo->promote_latch));
+
 	uint64_t term = req->term;
 	uint32_t origin = req->origin_id;
-	if (txn_limbo_replica_term(limbo, origin) < term) {
+	if (vclock_get(&limbo->promote_term_map, origin) < (int64_t)term) {
 		vclock_follow(&limbo->promote_term_map, origin, term);
 		if (term > limbo->promote_greatest_term)
 			limbo->promote_greatest_term = term;
@@ -786,6 +790,15 @@ txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
 	return;
 }
 
+void
+txn_limbo_process(struct txn_limbo *limbo,
+		  const struct synchro_request *req)
+{
+	txn_limbo_process_begin(limbo);
+	txn_limbo_process_core(limbo, req);
+	txn_limbo_process_commit(limbo);
+}
+
 void
 txn_limbo_on_parameters_change(struct txn_limbo *limbo)
 {
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index 53e52f676..fdb214711 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -31,6 +31,7 @@
  */
 #include "small/rlist.h"
 #include "vclock/vclock.h"
+#include "latch.h"
 
 #include <stdint.h>
 
@@ -147,6 +148,10 @@ struct txn_limbo {
 	 * limbo and raft are in sync and the terms are the same.
 	 */
 	uint64_t promote_greatest_term;
+	/**
+	 * To order access to the promote data.
+	 */
+	struct latch promote_latch;
 	/**
 	 * Maximal LSN gathered quorum and either already confirmed in WAL, or
 	 * whose confirmation is in progress right now. Any attempt to confirm
@@ -216,9 +221,12 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
  * @a replica_id.
  */
 static inline uint64_t
-txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
+txn_limbo_replica_term(struct txn_limbo *limbo, uint32_t replica_id)
 {
-	return vclock_get(&limbo->promote_term_map, replica_id);
+	latch_lock(&limbo->promote_latch);
+	uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);
+	latch_unlock(&limbo->promote_latch);
+	return v;
 }
 
 /**
@@ -226,11 +234,14 @@ txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
  * data from it. The check is only valid when elections are enabled.
  */
 static inline bool
-txn_limbo_is_replica_outdated(const struct txn_limbo *limbo,
+txn_limbo_is_replica_outdated(struct txn_limbo *limbo,
 			      uint32_t replica_id)
 {
-	return txn_limbo_replica_term(limbo, replica_id) <
-	       limbo->promote_greatest_term;
+	latch_lock(&limbo->promote_latch);
+	uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);
+	bool res = v < limbo->promote_greatest_term;
+	latch_unlock(&limbo->promote_latch);
+	return res;
 }
 
 /**
@@ -300,7 +311,37 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn);
 int
 txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry);
 
-/** Execute a synchronous replication request. */
+/**
+ * Initiate execution of a synchronous replication request.
+ */
+static inline void
+txn_limbo_process_begin(struct txn_limbo *limbo)
+{
+	latch_lock(&limbo->promote_latch);
+}
+
+/** Commit a synchronous replication request. */
+static inline void
+txn_limbo_process_commit(struct txn_limbo *limbo)
+{
+	assert(latch_is_locked(&limbo->promote_latch));
+	latch_unlock(&limbo->promote_latch);
+}
+
+/** Rollback a synchronous replication request. */
+static inline void
+txn_limbo_process_rollback(struct txn_limbo *limbo)
+{
+	assert(latch_is_locked(&limbo->promote_latch));
+	latch_unlock(&limbo->promote_latch);
+}
+
+/** Core of processing synchronous replication request. */
+void
+txn_limbo_process_core(struct txn_limbo *limbo,
+		       const struct synchro_request *req);
+
+/** Process a synchronous replication request. */
 void
 txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req);
 
-- 
2.31.1

[Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Cyrill Gorcunov via Tarantool-patches]

To test that promotion requests are handled only when appropriate
write to WAL completes, because we update memory data before the
write finishes.

Note that without the patch this test fires assertion

> tarantool: src/box/txn_limbo.c:481: txn_limbo_read_rollback: Assertion `e->txn->signature >= 0' failed.

Part-of #6036

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/replication/gh-6036-qsync-master.lua     |   1 +
 test/replication/gh-6036-qsync-node.lua       |  35 +++
 test/replication/gh-6036-qsync-order.result   | 207 ++++++++++++++++++
 test/replication/gh-6036-qsync-order.test.lua |  95 ++++++++
 test/replication/gh-6036-qsync-replica1.lua   |   1 +
 test/replication/gh-6036-qsync-replica2.lua   |   1 +
 test/replication/suite.cfg                    |   1 +
 test/replication/suite.ini                    |   2 +-
 8 files changed, 342 insertions(+), 1 deletion(-)
 create mode 120000 test/replication/gh-6036-qsync-master.lua
 create mode 100644 test/replication/gh-6036-qsync-node.lua
 create mode 100644 test/replication/gh-6036-qsync-order.result
 create mode 100644 test/replication/gh-6036-qsync-order.test.lua
 create mode 120000 test/replication/gh-6036-qsync-replica1.lua
 create mode 120000 test/replication/gh-6036-qsync-replica2.lua

diff --git a/test/replication/gh-6036-qsync-master.lua b/test/replication/gh-6036-qsync-master.lua
new file mode 120000
index 000000000..87bdb46ef
--- /dev/null
+++ b/test/replication/gh-6036-qsync-master.lua
@@ -0,0 +1 @@
+gh-6036-qsync-node.lua
\ No newline at end of file
diff --git a/test/replication/gh-6036-qsync-node.lua b/test/replication/gh-6036-qsync-node.lua
new file mode 100644
index 000000000..ba6213255
--- /dev/null
+++ b/test/replication/gh-6036-qsync-node.lua
@@ -0,0 +1,35 @@
+local INSTANCE_ID = string.match(arg[0], "gh%-6036%-qsync%-(.+)%.lua")
+
+local function unix_socket(name)
+    return "unix/:./" .. name .. '.sock';
+end
+
+require('console').listen(os.getenv('ADMIN'))
+
+local box_cfg_common = {
+        listen                      = unix_socket(INSTANCE_ID),
+        replication                 = {
+            unix_socket("master"),
+            unix_socket("replica1"),
+            unix_socket("replica2"),
+        },
+        replication_connect_quorum  = 1,
+        replication_synchro_quorum  = 1,
+        replication_synchro_timeout = 10000,
+}
+
+if INSTANCE_ID == "master" then
+    box_cfg_common['election_mode'] = "manual"
+    box.cfg(box_cfg_common)
+elseif INSTANCE_ID == "replica1" then
+    box_cfg_common['election_mode'] = "manual"
+    box.cfg(box_cfg_common)
+else
+    assert(INSTANCE_ID == "replica2")
+    box_cfg_common['election_mode'] = "manual"
+    box.cfg(box_cfg_common)
+end
+
+box.once("bootstrap", function()
+    box.schema.user.grant('guest', 'super')
+end)
diff --git a/test/replication/gh-6036-qsync-order.result b/test/replication/gh-6036-qsync-order.result
new file mode 100644
index 000000000..378ce917a
--- /dev/null
+++ b/test/replication/gh-6036-qsync-order.result
@@ -0,0 +1,207 @@
+-- test-run result file version 2
+--
+-- gh-6036: verify that terms are locked when we're inside journal
+-- write routine, because parallel appliers may ignore the fact that
+-- the term is updated already but not yet written leading to data
+-- inconsistency.
+--
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd('create server master with script="replication/gh-6036-qsync-master.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('create server replica1 with script="replication/gh-6036-qsync-replica1.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('create server replica2 with script="replication/gh-6036-qsync-replica2.lua"')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('start server master with wait=False')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica1 with wait=False')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica2 with wait=False')
+ | ---
+ | - true
+ | ...
+
+test_run:wait_fullmesh({"master", "replica1", "replica2"})
+ | ---
+ | ...
+
+--
+-- Create a synchro space on the master node and make
+-- sure the write processed just fine.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+box.ctl.promote()
+ | ---
+ | ...
+s = box.schema.create_space('test', {is_sync = true})
+ | ---
+ | ...
+_ = s:create_index('pk')
+ | ---
+ | ...
+s:insert{1}
+ | ---
+ | - [1]
+ | ...
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+ | ---
+ | - true
+ | ...
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+ | ---
+ | - true
+ | ...
+
+--
+-- Drop connection between master and replica1.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./master.sock",              \
+        "unix/:./replica2.sock",            \
+    },                                      \
+})
+ | ---
+ | ...
+--
+-- Drop connection between replica1 and master.
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+ | ---
+ | - true
+ | ...
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./replica1.sock",            \
+        "unix/:./replica2.sock",            \
+    },                                      \
+})
+ | ---
+ | ...
+
+--
+-- Here we have the following scheme
+--
+--              replica2 (will be delayed)
+--              /     \
+--          master    replica1
+
+--
+-- Initiate disk delay and remember the max term seen so far.
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_WAL_DELAY', true)
+ | ---
+ | - ok
+ | ...
+
+--
+-- Make replica1 been a leader and start writting data,
+-- the PROMOTE request get queued on replica2 and not
+-- yet processed, same time INSERT won't complete either
+-- waiting for PROMOTE completion first.
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+box.ctl.promote()
+ | ---
+ | ...
+_ = require('fiber').create(function() box.space.test:insert{2} end)
+ | ---
+ | ...
+
+--
+-- The master node has no clue that there is a new leader
+-- and continue writting data with obsolete term. Since replica2
+-- is delayed now the INSERT won't proceed yet but get queued.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+_ = require('fiber').create(function() box.space.test:insert{3} end)
+ | ---
+ | ...
+
+--
+-- Finally enable replica2 back. Make sure the data from new replica1
+-- leader get writting while old leader's data ignored.
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_WAL_DELAY', false)
+ | ---
+ | - ok
+ | ...
+test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
+ | ---
+ | - true
+ | ...
+box.space.test:select{}
+ | ---
+ | - - [1]
+ |   - [2]
+ | ...
+
+test_run:switch("default")
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica2')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('delete server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
diff --git a/test/replication/gh-6036-qsync-order.test.lua b/test/replication/gh-6036-qsync-order.test.lua
new file mode 100644
index 000000000..ac1dc5d91
--- /dev/null
+++ b/test/replication/gh-6036-qsync-order.test.lua
@@ -0,0 +1,95 @@
+--
+-- gh-6036: verify that terms are locked when we're inside journal
+-- write routine, because parallel appliers may ignore the fact that
+-- the term is updated already but not yet written leading to data
+-- inconsistency.
+--
+test_run = require('test_run').new()
+
+test_run:cmd('create server master with script="replication/gh-6036-qsync-master.lua"')
+test_run:cmd('create server replica1 with script="replication/gh-6036-qsync-replica1.lua"')
+test_run:cmd('create server replica2 with script="replication/gh-6036-qsync-replica2.lua"')
+
+test_run:cmd('start server master with wait=False')
+test_run:cmd('start server replica1 with wait=False')
+test_run:cmd('start server replica2 with wait=False')
+
+test_run:wait_fullmesh({"master", "replica1", "replica2"})
+
+--
+-- Create a synchro space on the master node and make
+-- sure the write processed just fine.
+test_run:switch("master")
+box.ctl.promote()
+s = box.schema.create_space('test', {is_sync = true})
+_ = s:create_index('pk')
+s:insert{1}
+test_run:switch("replica1")
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+test_run:switch("replica2")
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+
+--
+-- Drop connection between master and replica1.
+test_run:switch("master")
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./master.sock",              \
+        "unix/:./replica2.sock",            \
+    },                                      \
+})
+--
+-- Drop connection between replica1 and master.
+test_run:switch("replica1")
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./replica1.sock",            \
+        "unix/:./replica2.sock",            \
+    },                                      \
+})
+
+--
+-- Here we have the following scheme
+--
+--              replica2 (will be delayed)
+--              /     \
+--          master    replica1
+
+--
+-- Initiate disk delay and remember the max term seen so far.
+test_run:switch("replica2")
+box.error.injection.set('ERRINJ_WAL_DELAY', true)
+
+--
+-- Make replica1 been a leader and start writting data,
+-- the PROMOTE request get queued on replica2 and not
+-- yet processed, same time INSERT won't complete either
+-- waiting for PROMOTE completion first.
+test_run:switch("replica1")
+box.ctl.promote()
+_ = require('fiber').create(function() box.space.test:insert{2} end)
+
+--
+-- The master node has no clue that there is a new leader
+-- and continue writting data with obsolete term. Since replica2
+-- is delayed now the INSERT won't proceed yet but get queued.
+test_run:switch("master")
+_ = require('fiber').create(function() box.space.test:insert{3} end)
+
+--
+-- Finally enable replica2 back. Make sure the data from new replica1
+-- leader get writting while old leader's data ignored.
+test_run:switch("replica2")
+box.error.injection.set('ERRINJ_WAL_DELAY', false)
+test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
+box.space.test:select{}
+
+test_run:switch("default")
+test_run:cmd('stop server master')
+test_run:cmd('stop server replica1')
+test_run:cmd('stop server replica2')
+
+test_run:cmd('delete server master')
+test_run:cmd('delete server replica1')
+test_run:cmd('delete server replica2')
diff --git a/test/replication/gh-6036-qsync-replica1.lua b/test/replication/gh-6036-qsync-replica1.lua
new file mode 120000
index 000000000..87bdb46ef
--- /dev/null
+++ b/test/replication/gh-6036-qsync-replica1.lua
@@ -0,0 +1 @@
+gh-6036-qsync-node.lua
\ No newline at end of file
diff --git a/test/replication/gh-6036-qsync-replica2.lua b/test/replication/gh-6036-qsync-replica2.lua
new file mode 120000
index 000000000..87bdb46ef
--- /dev/null
+++ b/test/replication/gh-6036-qsync-replica2.lua
@@ -0,0 +1 @@
+gh-6036-qsync-node.lua
\ No newline at end of file
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index 3eee0803c..ed09b2087 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -59,6 +59,7 @@
     "gh-6094-rs-uuid-mismatch.test.lua": {},
     "gh-6127-election-join-new.test.lua": {},
     "gh-6035-applier-filter.test.lua": {},
+    "gh-6036-qsync-order.test.lua": {},
     "election-candidate-promote.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
diff --git a/test/replication/suite.ini b/test/replication/suite.ini
index 77eb95f49..080e4fbf4 100644
--- a/test/replication/suite.ini
+++ b/test/replication/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
 script =  master.lua
 description = tarantool/box, replication
 disabled = consistent.test.lua
-release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua
+release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua gh-6036-qsync-order.test.lua
 config = suite.cfg
 lua_libs = lua/fast_replica.lua lua/rlimit.lua
 use_unix_sockets = True
-- 
2.31.1

Re: [Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Serge Petrenko via Tarantool-patches]

30.09.2021 12:44, Cyrill Gorcunov пишет:
> Limbo terms tracking is shared between appliers and when
> one of appliers is waiting for write to complete inside
> journal_write() routine, an other may need to access read
> term value to figure out if promote request is valid to
> apply. Due to cooperative multitasking access to the terms
> is not consistent so we need to be sure that other fibers
> read up to date terms (ie written to the WAL).
>
> For this sake we use a latching mechanism, when one fiber
> takes a lock for updating other readers are waiting until
> the operation is complete.
>
> For example here is a call graph of two appliers
>
> applier 1
> ---------
> applier_apply_tx
>    (promote term = 3
>     current max term = 2)
>    applier_synchro_filter_tx
>    apply_synchro_row
>      journal_write
>        (sleeping)
>
> at this moment another applier comes in with obsolete
> data and term 2
>
>                                applier 2
>                                ---------
>                                applier_apply_tx
>                                  (term 2)
>                                  applier_synchro_filter_tx
>                                    txn_limbo_is_replica_outdated -> false
>                                  journal_write (sleep)
>
> applier 1
> ---------
> journal wakes up
>    apply_synchro_row_cb
>      set max term to 3
>
> So the applier 2 didn't notice that term 3 is already seen
> and wrote obsolete data. With locking the applier 2 will
> wait until applier 1 has finished its write.
>
> We introduce the following helpers:
>
> 1) txn_limbo_process_begin: which takes a lock
> 2) txn_limbo_process_commit and txn_limbo_process_rollback
>     which simply release the lock but have different names
>     for better semantics
> 3) txn_limbo_process is a general function which uses x_begin
>     and x_commit helper internally
> 4) txn_limbo_process_core to do a real job over processing the
>     request, it implies that txn_limbo_process_begin been called
>
> Part-of #6036
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   src/box/applier.cc  | 12 +++++++---
>   src/box/box.cc      | 15 ++++++++-----
>   src/box/txn_limbo.c | 17 +++++++++++++--
>   src/box/txn_limbo.h | 53 ++++++++++++++++++++++++++++++++++++++++-----
>   4 files changed, 80 insertions(+), 17 deletions(-)
>
> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index b981bd436..46c36e259 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -857,7 +857,7 @@ apply_synchro_row_cb(struct journal_entry *entry)
>   		applier_rollback_by_wal_io(entry->res);
>   	} else {
>   		replica_txn_wal_write_cb(synchro_entry->rcb);
> -		txn_limbo_process(&txn_limbo, synchro_entry->req);
> +		txn_limbo_process_core(&txn_limbo, synchro_entry->req);
>   		trigger_run(&replicaset.applier.on_wal_write, NULL);
>   	}
>   	fiber_wakeup(synchro_entry->owner);
> @@ -873,6 +873,8 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
>   	if (xrow_decode_synchro(row, &req) != 0)
>   		goto err;
>   
> +	txn_limbo_process_begin(&txn_limbo);
> +
>   	struct replica_cb_data rcb_data;
>   	struct synchro_entry entry;
>   	/*
> @@ -910,12 +912,16 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
>   	 * transactions side, including the async ones.
>   	 */
>   	if (journal_write(&entry.base) != 0)
> -		goto err;
> +		goto err_rollback;
>   	if (entry.base.res < 0) {
>   		diag_set_journal_res(entry.base.res);
> -		goto err;
> +		goto err_rollback;
>   	}
> +	txn_limbo_process_commit(&txn_limbo);
>   	return 0;
> +
> +err_rollback:
> +	txn_limbo_process_rollback(&txn_limbo);
>   err:
>   	diag_log();
>   	return -1;
> diff --git a/src/box/box.cc b/src/box/box.cc
> index e082e1a3d..6a9be745a 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1677,8 +1677,6 @@ box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
>   	struct raft *raft = box_raft();
>   	assert(raft->volatile_term == raft->term);
>   	assert(promote_lsn >= 0);
> -	txn_limbo_write_promote(&txn_limbo, promote_lsn,
> -				raft->term);
>   	struct synchro_request req = {
>   		.type = IPROTO_RAFT_PROMOTE,
>   		.replica_id = prev_leader_id,
> @@ -1686,8 +1684,11 @@ box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
>   		.lsn = promote_lsn,
>   		.term = raft->term,
>   	};
> -	txn_limbo_process(&txn_limbo, &req);
> +	txn_limbo_process_begin(&txn_limbo);
> +	txn_limbo_write_promote(&txn_limbo, req.lsn, req.term);
> +	txn_limbo_process_core(&txn_limbo, &req);
>   	assert(txn_limbo_is_empty(&txn_limbo));
> +	txn_limbo_process_commit(&txn_limbo);
>   }
>   
>   /** A guard to block multiple simultaneous promote()/demote() invocations. */
> @@ -1699,8 +1700,6 @@ box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn)
>   {
>   	assert(box_raft()->volatile_term == box_raft()->term);
>   	assert(promote_lsn >= 0);
> -	txn_limbo_write_demote(&txn_limbo, promote_lsn,
> -				box_raft()->term);
>   	struct synchro_request req = {
>   		.type = IPROTO_RAFT_DEMOTE,
>   		.replica_id = prev_leader_id,
> @@ -1708,8 +1707,12 @@ box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn)
>   		.lsn = promote_lsn,
>   		.term = box_raft()->term,
>   	};
> -	txn_limbo_process(&txn_limbo, &req);
> +	txn_limbo_process_begin(&txn_limbo);
> +	txn_limbo_write_demote(&txn_limbo, promote_lsn,
> +				box_raft()->term);
> +	txn_limbo_process_core(&txn_limbo, &req);
>   	assert(txn_limbo_is_empty(&txn_limbo));
> +	txn_limbo_process_commit(&txn_limbo);
>   }
>   
>   int
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 70447caaf..855c98c98 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -47,6 +47,7 @@ txn_limbo_create(struct txn_limbo *limbo)
>   	vclock_create(&limbo->vclock);
>   	vclock_create(&limbo->promote_term_map);
>   	limbo->promote_greatest_term = 0;
> +	latch_create(&limbo->promote_latch);
>   	limbo->confirmed_lsn = 0;
>   	limbo->rollback_count = 0;
>   	limbo->is_in_rollback = false;
> @@ -724,11 +725,14 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double timeout)
>   }
>   
>   void
> -txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
> +txn_limbo_process_core(struct txn_limbo *limbo,
> +		       const struct synchro_request *req)
>   {
> +	assert(latch_is_locked(&limbo->promote_latch));
> +
>   	uint64_t term = req->term;
>   	uint32_t origin = req->origin_id;
> -	if (txn_limbo_replica_term(limbo, origin) < term) {
> +	if (vclock_get(&limbo->promote_term_map, origin) < (int64_t)term) {
>   		vclock_follow(&limbo->promote_term_map, origin, term);
>   		if (term > limbo->promote_greatest_term)
>   			limbo->promote_greatest_term = term;
> @@ -786,6 +790,15 @@ txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
>   	return;
>   }
>   
> +void
> +txn_limbo_process(struct txn_limbo *limbo,
> +		  const struct synchro_request *req)
> +{
> +	txn_limbo_process_begin(limbo);
> +	txn_limbo_process_core(limbo, req);
> +	txn_limbo_process_commit(limbo);
> +}
> +
>   void
>   txn_limbo_on_parameters_change(struct txn_limbo *limbo)
>   {
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index 53e52f676..fdb214711 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -31,6 +31,7 @@
>    */
>   #include "small/rlist.h"
>   #include "vclock/vclock.h"
> +#include "latch.h"
>   
>   #include <stdint.h>
>   
> @@ -147,6 +148,10 @@ struct txn_limbo {
>   	 * limbo and raft are in sync and the terms are the same.
>   	 */
>   	uint64_t promote_greatest_term;
> +	/**
> +	 * To order access to the promote data.
> +	 */
> +	struct latch promote_latch;
>   	/**
>   	 * Maximal LSN gathered quorum and either already confirmed in WAL, or
>   	 * whose confirmation is in progress right now. Any attempt to confirm
> @@ -216,9 +221,12 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
>    * @a replica_id.
>    */
>   static inline uint64_t
> -txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
> +txn_limbo_replica_term(struct txn_limbo *limbo, uint32_t replica_id)
>   {
> -	return vclock_get(&limbo->promote_term_map, replica_id);
> +	latch_lock(&limbo->promote_latch);
> +	uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);
> +	latch_unlock(&limbo->promote_latch);
> +	return v;
>   }
>   
>   /**
> @@ -226,11 +234,14 @@ txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
>    * data from it. The check is only valid when elections are enabled.
>    */
>   static inline bool
> -txn_limbo_is_replica_outdated(const struct txn_limbo *limbo,
> +txn_limbo_is_replica_outdated(struct txn_limbo *limbo,
>   			      uint32_t replica_id)
>   {
> -	return txn_limbo_replica_term(limbo, replica_id) <
> -	       limbo->promote_greatest_term;
> +	latch_lock(&limbo->promote_latch);
> +	uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);
> +	bool res = v < limbo->promote_greatest_term;
> +	latch_unlock(&limbo->promote_latch);
> +	return res;
>   }
>   
>   /**
> @@ -300,7 +311,37 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn);
>   int
>   txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry);
>   
> -/** Execute a synchronous replication request. */
> +/**
> + * Initiate execution of a synchronous replication request.
> + */
> +static inline void
> +txn_limbo_process_begin(struct txn_limbo *limbo)
> +{
> +	latch_lock(&limbo->promote_latch);
> +}
> +
> +/** Commit a synchronous replication request. */
> +static inline void
> +txn_limbo_process_commit(struct txn_limbo *limbo)
> +{
> +	assert(latch_is_locked(&limbo->promote_latch));
> +	latch_unlock(&limbo->promote_latch);
> +}
> +
> +/** Rollback a synchronous replication request. */
> +static inline void
> +txn_limbo_process_rollback(struct txn_limbo *limbo)
> +{
> +	assert(latch_is_locked(&limbo->promote_latch));
> +	latch_unlock(&limbo->promote_latch);
> +}
> +
> +/** Core of processing synchronous replication request. */
> +void
> +txn_limbo_process_core(struct txn_limbo *limbo,
> +		       const struct synchro_request *req);
> +
> +/** Process a synchronous replication request. */
>   void
>   txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req);

Thanks for the patch!

Mostly ok with one question:

What about txn_limbo_write_confirm/txn_limbo_read_confirm pairs issued
inside txn_limbo_ack() and txn_limbo_on_parameters_change() ?

Shouldn't they take the latch as well? I mean, txn_limbo_ack() and
txn_limbo_on_parameters_change() as a whole.
>   

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Serge Petrenko via Tarantool-patches]

30.09.2021 12:44, Cyrill Gorcunov пишет:
> To test that promotion requests are handled only when appropriate
> write to WAL completes, because we update memory data before the
> write finishes.
>
> Note that without the patch this test fires assertion
>

Thanks for the test!
Please, find a couple of comments below.
I think the test won't be flaky anymore once you fix my comments.

>> tarantool: src/box/txn_limbo.c:481: txn_limbo_read_rollback: Assertion `e->txn->signature >= 0' failed.
> Part-of #6036
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   test/replication/gh-6036-qsync-master.lua     |   1 +
>   test/replication/gh-6036-qsync-node.lua       |  35 +++
>   test/replication/gh-6036-qsync-order.result   | 207 ++++++++++++++++++
>   test/replication/gh-6036-qsync-order.test.lua |  95 ++++++++
>   test/replication/gh-6036-qsync-replica1.lua   |   1 +
>   test/replication/gh-6036-qsync-replica2.lua   |   1 +
>   test/replication/suite.cfg                    |   1 +
>   test/replication/suite.ini                    |   2 +-
>   8 files changed, 342 insertions(+), 1 deletion(-)
>   create mode 120000 test/replication/gh-6036-qsync-master.lua
>   create mode 100644 test/replication/gh-6036-qsync-node.lua
>   create mode 100644 test/replication/gh-6036-qsync-order.result
>   create mode 100644 test/replication/gh-6036-qsync-order.test.lua
>   create mode 120000 test/replication/gh-6036-qsync-replica1.lua
>   create mode 120000 test/replication/gh-6036-qsync-replica2.lua
>
> diff --git a/test/replication/gh-6036-qsync-master.lua b/test/replication/gh-6036-qsync-master.lua
> new file mode 120000
> index 000000000..87bdb46ef
> --- /dev/null
> +++ b/test/replication/gh-6036-qsync-master.lua
> @@ -0,0 +1 @@
> +gh-6036-qsync-node.lua
> \ No newline at end of file
> diff --git a/test/replication/gh-6036-qsync-node.lua b/test/replication/gh-6036-qsync-node.lua
> new file mode 100644
> index 000000000..ba6213255
> --- /dev/null
> +++ b/test/replication/gh-6036-qsync-node.lua
> @@ -0,0 +1,35 @@
> +local INSTANCE_ID = string.match(arg[0], "gh%-6036%-qsync%-(.+)%.lua")
> +
> +local function unix_socket(name)
> +    return "unix/:./" .. name .. '.sock';
> +end
> +
> +require('console').listen(os.getenv('ADMIN'))
> +
> +local box_cfg_common = {
> +        listen                      = unix_socket(INSTANCE_ID),
> +        replication                 = {
> +            unix_socket("master"),
> +            unix_socket("replica1"),
> +            unix_socket("replica2"),
> +        },
> +        replication_connect_quorum  = 1,
> +        replication_synchro_quorum  = 1,
> +        replication_synchro_timeout = 10000,
> +}
> +
> +if INSTANCE_ID == "master" then
> +    box_cfg_common['election_mode'] = "manual"
> +    box.cfg(box_cfg_common)
> +elseif INSTANCE_ID == "replica1" then
> +    box_cfg_common['election_mode'] = "manual"
> +    box.cfg(box_cfg_common)
> +else
> +    assert(INSTANCE_ID == "replica2")
> +    box_cfg_common['election_mode'] = "manual"
> +    box.cfg(box_cfg_common)
> +end
> +
> +box.once("bootstrap", function()
> +    box.schema.user.grant('guest', 'super')
> +end)

Looks like "election_replica.lua" suits our needs perfectly now.
No need to introduce a new instance file.

> diff --git a/test/replication/gh-6036-qsync-order.result b/test/replication/gh-6036-qsync-order.result
> new file mode 100644
> index 000000000..378ce917a
> --- /dev/null
> +++ b/test/replication/gh-6036-qsync-order.result
> @@ -0,0 +1,207 @@
> +-- test-run result file version 2
> +--
> +-- gh-6036: verify that terms are locked when we're inside journal
> +-- write routine, because parallel appliers may ignore the fact that
> +-- the term is updated already but not yet written leading to data
> +-- inconsistency.
> +--
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +
> +test_run:cmd('create server master with script="replication/gh-6036-qsync-master.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('create server replica1 with script="replication/gh-6036-qsync-replica1.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('create server replica2 with script="replication/gh-6036-qsync-replica2.lua"')
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('start server master with wait=False')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica1 with wait=False')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica2 with wait=False')
> + | ---
> + | - true
> + | ...
> +
> +test_run:wait_fullmesh({"master", "replica1", "replica2"})
> + | ---
> + | ...
> +
> +--
> +-- Create a synchro space on the master node and make
> +-- sure the write processed just fine.
> +test_run:switch("master")
> + | ---
> + | - true
> + | ...
> +box.ctl.promote()
> + | ---
> + | ...
> +s = box.schema.create_space('test', {is_sync = true})
> + | ---
> + | ...
> +_ = s:create_index('pk')
> + | ---
> + | ...
> +s:insert{1}
> + | ---
> + | - [1]
> + | ...
> +test_run:switch("replica1")
> + | ---
> + | - true
> + | ...
> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> + | ---
> + | - true
> + | ...
> +test_run:switch("replica2")
> + | ---
> + | - true
> + | ...
> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> + | ---
> + | - true
> + | ...
> +


You may replace both calls with test_run:wait_lsn('master', 'replica')
Even without switching.

> +--
> +-- Drop connection between master and replica1.
> +test_run:switch("master")
> + | ---
> + | - true
> + | ...
> +box.cfg({                                   \
> +    replication = {                         \
> +        "unix/:./master.sock",              \
> +        "unix/:./replica2.sock",            \
> +    },                                      \
> +})
> + | ---
> + | ...
> +--
> +-- Drop connection between replica1 and master.
> +test_run:switch("replica1")
> + | ---
> + | - true
> + | ...
> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> + | ---
> + | - true
> + | ...
> +box.cfg({                                   \
> +    replication = {                         \
> +        "unix/:./replica1.sock",            \
> +        "unix/:./replica2.sock",            \
> +    },                                      \
> +})
> + | ---
> + | ...
> +
> +--
> +-- Here we have the following scheme
> +--
> +--              replica2 (will be delayed)
> +--              /     \
> +--          master    replica1
> +
> +--
> +-- Initiate disk delay and remember the max term seen so far.
> +test_run:switch("replica2")
> + | ---
> + | - true
> + | ...
> +box.error.injection.set('ERRINJ_WAL_DELAY', true)
> + | ---
> + | - ok
> + | ...
> +
> +--
> +-- Make replica1 been a leader and start writting data,
> +-- the PROMOTE request get queued on replica2 and not
> +-- yet processed, same time INSERT won't complete either
> +-- waiting for PROMOTE completion first.
> +test_run:switch("replica1")
> + | ---
> + | - true
> + | ...
> +box.ctl.promote()
> + | ---
> + | ...
> +_ = require('fiber').create(function() box.space.test:insert{2} end)
> + | ---
> + | ...
> +
> +--

Prior to doing something on master, you should make sure
replica2 has received the promote.
"wait_lsn" won't work here, because WAL is disabled. You may try
test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)

> +-- The master node has no clue that there is a new leader
> +-- and continue writting data with obsolete term. Since replica2
> +-- is delayed now the INSERT won't proceed yet but get queued.
> +test_run:switch("master")
> + | ---
> + | - true
> + | ...
> +_ = require('fiber').create(function() box.space.test:insert{3} end)
> + | ---
> + | ...
> +
Just like above, try to wait until replica2 receives insert{3}.
> +--
> +-- Finally enable replica2 back. Make sure the data from new replica1
> +-- leader get writting while old leader's data ignored.
> +test_run:switch("replica2")
> + | ---
> + | - true
> + | ...
> +box.error.injection.set('ERRINJ_WAL_DELAY', false)
> + | ---
> + | - ok
> + | ...
> +test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
> + | ---
> + | - true
> + | ...
> +box.space.test:select{}
> + | ---
> + | - - [1]
> + |   - [2]
> + | ...
> +
> +test_run:switch("default")
> + | ---
> + | - true
> + | ...
> +test_run:cmd('stop server master')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('stop server replica1')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('stop server replica2')
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('delete server master')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica1')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica2')
> + | ---
> + | - true
> + | ...
> diff --git a/test/replication/gh-6036-qsync-order.test.lua b/test/replication/gh-6036-qsync-order.test.lua
> new file mode 100644
> index 000000000..ac1dc5d91
> --- /dev/null
> +++ b/test/replication/gh-6036-qsync-order.test.lua
> @@ -0,0 +1,95 @@
> +--
> +-- gh-6036: verify that terms are locked when we're inside journal
> +-- write routine, because parallel appliers may ignore the fact that
> +-- the term is updated already but not yet written leading to data
> +-- inconsistency.
> +--
> +test_run = require('test_run').new()
> +
> +test_run:cmd('create server master with script="replication/gh-6036-qsync-master.lua"')
> +test_run:cmd('create server replica1 with script="replication/gh-6036-qsync-replica1.lua"')
> +test_run:cmd('create server replica2 with script="replication/gh-6036-qsync-replica2.lua"')
> +
> +test_run:cmd('start server master with wait=False')
> +test_run:cmd('start server replica1 with wait=False')
> +test_run:cmd('start server replica2 with wait=False')
> +
> +test_run:wait_fullmesh({"master", "replica1", "replica2"})
> +
> +--
> +-- Create a synchro space on the master node and make
> +-- sure the write processed just fine.
> +test_run:switch("master")
> +box.ctl.promote()
> +s = box.schema.create_space('test', {is_sync = true})
> +_ = s:create_index('pk')
> +s:insert{1}
> +test_run:switch("replica1")
> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> +test_run:switch("replica2")
> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> +
> +--
> +-- Drop connection between master and replica1.
> +test_run:switch("master")
> +box.cfg({                                   \
> +    replication = {                         \
> +        "unix/:./master.sock",              \
> +        "unix/:./replica2.sock",            \
> +    },                                      \
> +})
> +--
> +-- Drop connection between replica1 and master.
> +test_run:switch("replica1")
> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> +box.cfg({                                   \
> +    replication = {                         \
> +        "unix/:./replica1.sock",            \
> +        "unix/:./replica2.sock",            \
> +    },                                      \
> +})
> +
> +--
> +-- Here we have the following scheme
> +--
> +--              replica2 (will be delayed)
> +--              /     \
> +--          master    replica1
> +
> +--
> +-- Initiate disk delay and remember the max term seen so far.
> +test_run:switch("replica2")
> +box.error.injection.set('ERRINJ_WAL_DELAY', true)
> +
> +--
> +-- Make replica1 been a leader and start writting data,
> +-- the PROMOTE request get queued on replica2 and not
> +-- yet processed, same time INSERT won't complete either
> +-- waiting for PROMOTE completion first.
> +test_run:switch("replica1")
> +box.ctl.promote()
> +_ = require('fiber').create(function() box.space.test:insert{2} end)
> +
> +--
> +-- The master node has no clue that there is a new leader
> +-- and continue writting data with obsolete term. Since replica2
> +-- is delayed now the INSERT won't proceed yet but get queued.
> +test_run:switch("master")
> +_ = require('fiber').create(function() box.space.test:insert{3} end)
> +
> +--
> +-- Finally enable replica2 back. Make sure the data from new replica1
> +-- leader get writting while old leader's data ignored.
> +test_run:switch("replica2")
> +box.error.injection.set('ERRINJ_WAL_DELAY', false)
> +test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
> +box.space.test:select{}
> +
> +test_run:switch("default")
> +test_run:cmd('stop server master')
> +test_run:cmd('stop server replica1')
> +test_run:cmd('stop server replica2')
> +
> +test_run:cmd('delete server master')
> +test_run:cmd('delete server replica1')
> +test_run:cmd('delete server replica2')
> diff --git a/test/replication/gh-6036-qsync-replica1.lua b/test/replication/gh-6036-qsync-replica1.lua
> new file mode 120000
> index 000000000..87bdb46ef
> --- /dev/null
> +++ b/test/replication/gh-6036-qsync-replica1.lua
> @@ -0,0 +1 @@
> +gh-6036-qsync-node.lua
> \ No newline at end of file
> diff --git a/test/replication/gh-6036-qsync-replica2.lua b/test/replication/gh-6036-qsync-replica2.lua
> new file mode 120000
> index 000000000..87bdb46ef
> --- /dev/null
> +++ b/test/replication/gh-6036-qsync-replica2.lua
> @@ -0,0 +1 @@
> +gh-6036-qsync-node.lua
> \ No newline at end of file
> diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
> index 3eee0803c..ed09b2087 100644
> --- a/test/replication/suite.cfg
> +++ b/test/replication/suite.cfg
> @@ -59,6 +59,7 @@
>       "gh-6094-rs-uuid-mismatch.test.lua": {},
>       "gh-6127-election-join-new.test.lua": {},
>       "gh-6035-applier-filter.test.lua": {},
> +    "gh-6036-qsync-order.test.lua": {},
>       "election-candidate-promote.test.lua": {},
>       "*": {
>           "memtx": {"engine": "memtx"},
> diff --git a/test/replication/suite.ini b/test/replication/suite.ini
> index 77eb95f49..080e4fbf4 100644
> --- a/test/replication/suite.ini
> +++ b/test/replication/suite.ini
> @@ -3,7 +3,7 @@ core = tarantool
>   script =  master.lua
>   description = tarantool/box, replication
>   disabled = consistent.test.lua
> -release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua
> +release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua gh-6036-qsync-order.test.lua
>   config = suite.cfg
>   lua_libs = lua/fast_replica.lua lua/rlimit.lua
>   use_unix_sockets = True

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Oct 01, 2021 at 03:14:52PM +0300, Serge Petrenko wrote:
...
> > +/** Process a synchronous replication request. */
> >   void
> >   txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req);
> 
> Thanks for the patch!
> 
> Mostly ok with one question:
> 
> What about txn_limbo_write_confirm/txn_limbo_read_confirm pairs issued
> inside txn_limbo_ack() and txn_limbo_on_parameters_change() ?
> 
> Shouldn't they take the latch as well? I mean, txn_limbo_ack() and
> txn_limbo_on_parameters_change() as a whole.

Wait, Serge, currently we guard promote_map/max_term, so it won't be
read while there is its update on the fly. Thus If only I'm not missing
something obvious txn_limbo_on_parameters_change() can't interfere with
promote data or race with it anyhow. If you mean some other race then
it seems I don't see it yet, but I suspect I might be simply wrong :)

Re: [Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Serge Petrenko via Tarantool-patches]

01.10.2021 15:31, Cyrill Gorcunov пишет:
> On Fri, Oct 01, 2021 at 03:14:52PM +0300, Serge Petrenko wrote:
> ...
>>> +/** Process a synchronous replication request. */
>>>    void
>>>    txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req);
>> Thanks for the patch!
>>
>> Mostly ok with one question:
>>
>> What about txn_limbo_write_confirm/txn_limbo_read_confirm pairs issued
>> inside txn_limbo_ack() and txn_limbo_on_parameters_change() ?
>>
>> Shouldn't they take the latch as well? I mean, txn_limbo_ack() and
>> txn_limbo_on_parameters_change() as a whole.
> Wait, Serge, currently we guard promote_map/max_term, so it won't be
> read while there is its update on the fly. Thus If only I'm not missing
> something obvious txn_limbo_on_parameters_change() can't interfere with
> promote data or race with it anyhow. If you mean some other race then
> it seems I don't see it yet, but I suspect I might be simply wrong :)
Shouldn't we guard limbo->owner as well?
Otherwise you may start writing confirm for an old leader once promote 
for a new
one is already in progress.

I don't remember us discussing this before, so, maybe I'm just confused.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Oct 01, 2021 at 03:30:41PM +0300, Serge Petrenko wrote:
> 
> Thanks for the test!
> Please, find a couple of comments below.
> I think the test won't be flaky anymore once you fix my comments.

Thanks for comments, Serge!

> > +
> > +box.once("bootstrap", function()
> > +    box.schema.user.grant('guest', 'super')
> > +end)
> 
> Looks like "election_replica.lua" suits our needs perfectly now.
> No need to introduce a new instance file.

yup, I updated the test, thanks!

> > +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
> > + | ---
> > + | - true
> > + | ...
> > +
> 
> 
> You may replace both calls with test_run:wait_lsn('master', 'replica')
> Even without switching.

Actually I need switch, otherwise i got stuck, so I use

test_run:switch("master")
box.ctl.promote()
s = box.schema.create_space('test', {is_sync = true})
_ = s:create_index('pk')
s:insert{1}

test_run:switch("replica1")
test_run:wait_lsn('replica1', 'master')

test_run:switch("replica2")
test_run:wait_lsn('replica2', 'master')


which works just fine

> > +
> > +--
> > +-- Make replica1 been a leader and start writting data,
> > +-- the PROMOTE request get queued on replica2 and not
> > +-- yet processed, same time INSERT won't complete either
> > +-- waiting for PROMOTE completion first.
> > +test_run:switch("replica1")
> > + | ---
> > + | - true
> > + | ...
> > +box.ctl.promote()
> > + | ---
> > + | ...
> > +_ = require('fiber').create(function() box.space.test:insert{2} end)
> > + | ---
> > + | ...
> > +
> > +--
> 
> Prior to doing something on master, you should make sure
> replica2 has received the promote.
> "wait_lsn" won't work here, because WAL is disabled. You may try
> test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)

Wait, this moment is dubious. Look, once we issue promote it get
stuck inside journal write procedure so the next "insert" won't
proceed until the promote finished. I understand that you point
to the potential race here because even promote() command may get
finished but slowed down on nework level and simply reach replica2
out of other calls. I think without aditional debug output (such
as promote term exposed via box.info I did in previous series) we
can't be sure about timings and it seems that i've to return back
the box.info patch. I mean currently the command

test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)

stucks forever because promote yet not finished and next 'insert' simply
has not been applied.

Re: [Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Oct 01, 2021 at 03:37:18PM +0300, Serge Petrenko wrote:
> Shouldn't we guard limbo->owner as well?
> Otherwise you may start writing confirm for an old leader once promote for a
> new one is already in progress.
> 
> I don't remember us discussing this before, so, maybe I'm just confused.

Seems I still don't follow. Look, currently limbo::owner_id is assigned
from the one place:

txn_limbo_process_core
  txn_limbo_process_begin(limbo)
    txn_limbo_read_demote | txn_limbo_read_promote
      ...
      limbo->owner_id = replica_id;

The txn_limbo_process_core is called from console (via box.ctl.promote|demote
commands) or via applier. When it comes from console then we still take the
lock before write into the journal

static void
box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
{
	struct raft *raft = box_raft();
	assert(raft->volatile_term == raft->term);
	assert(promote_lsn >= 0);
	struct synchro_request req = {
		.type = IPROTO_RAFT_PROMOTE,
		.replica_id = prev_leader_id,
		.origin_id = instance_id,
		.lsn = promote_lsn,
		.term = raft->term,
	};
-->	txn_limbo_process_begin(&txn_limbo);
-->	txn_limbo_write_promote(&txn_limbo, req.lsn, req.term);
	txn_limbo_process_core(&txn_limbo, &req);
	assert(txn_limbo_is_empty(&txn_limbo));
	txn_limbo_process_commit(&txn_limbo);
}

thus console commands and applier are serialized. Or I miss something
obvious?

Re: [Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Serge Petrenko via Tarantool-patches]

05.10.2021 00:53, Cyrill Gorcunov пишет:
> On Fri, Oct 01, 2021 at 03:37:18PM +0300, Serge Petrenko wrote:
>> Shouldn't we guard limbo->owner as well?
>> Otherwise you may start writing confirm for an old leader once promote for a
>> new one is already in progress.
>>
>> I don't remember us discussing this before, so, maybe I'm just confused.
> Seems I still don't follow. Look, currently limbo::owner_id is assigned
> from the one place:
>
> txn_limbo_process_core
>    txn_limbo_process_begin(limbo)
>      txn_limbo_read_demote | txn_limbo_read_promote
>        ...
>        limbo->owner_id = replica_id;
>
> The txn_limbo_process_core is called from console (via box.ctl.promote|demote
> commands) or via applier. When it comes from console then we still take the
> lock before write into the journal
>
> static void
> box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
> {
> 	struct raft *raft = box_raft();
> 	assert(raft->volatile_term == raft->term);
> 	assert(promote_lsn >= 0);
> 	struct synchro_request req = {
> 		.type = IPROTO_RAFT_PROMOTE,
> 		.replica_id = prev_leader_id,
> 		.origin_id = instance_id,
> 		.lsn = promote_lsn,
> 		.term = raft->term,
> 	};
> -->	txn_limbo_process_begin(&txn_limbo);
> -->	txn_limbo_write_promote(&txn_limbo, req.lsn, req.term);
> 	txn_limbo_process_core(&txn_limbo, &req);
> 	assert(txn_limbo_is_empty(&txn_limbo));
> 	txn_limbo_process_commit(&txn_limbo);
> }
>
> thus console commands and applier are serialized. Or I miss something
> obvious?

Here's what can happen. You're the old leader, and you've received a
promote from someone else already, it's being written now.
Now some replica sends you an ack. You enter txn_limbo_ack
(it doesn't check the latch). txn_limbo_ack() may result in writing a 
CONFIRM.

The PROMOTE is already in progress, so you'll end up writing CONFIRM for
a wrong leader id (your own) after PROMOTE is written.

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Serge Petrenko via Tarantool-patches]

05.10.2021 00:16, Cyrill Gorcunov пишет:
> On Fri, Oct 01, 2021 at 03:30:41PM +0300, Serge Petrenko wrote:
>> Thanks for the test!
>> Please, find a couple of comments below.
>> I think the test won't be flaky anymore once you fix my comments.
> Thanks for comments, Serge!
>
>>> +
>>> +box.once("bootstrap", function()
>>> +    box.schema.user.grant('guest', 'super')
>>> +end)
>> Looks like "election_replica.lua" suits our needs perfectly now.
>> No need to introduce a new instance file.
> yup, I updated the test, thanks!
>
>>> +test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
>>> + | ---
>>> + | - true
>>> + | ...
>>> +
>>
>> You may replace both calls with test_run:wait_lsn('master', 'replica')
>> Even without switching.
> Actually I need switch, otherwise i got stuck, so I use
>
> test_run:switch("master")
> box.ctl.promote()
> s = box.schema.create_space('test', {is_sync = true})
> _ = s:create_index('pk')
> s:insert{1}
>
> test_run:switch("replica1")
> test_run:wait_lsn('replica1', 'master')
>
> test_run:switch("replica2")
> test_run:wait_lsn('replica2', 'master')
>
>
> which works just fine

Yep, that's what I meant.

>
>>> +
>>> +--
>>> +-- Make replica1 been a leader and start writting data,
>>> +-- the PROMOTE request get queued on replica2 and not
>>> +-- yet processed, same time INSERT won't complete either
>>> +-- waiting for PROMOTE completion first.
>>> +test_run:switch("replica1")
>>> + | ---
>>> + | - true
>>> + | ...
>>> +box.ctl.promote()
>>> + | ---
>>> + | ...
>>> +_ = require('fiber').create(function() box.space.test:insert{2} end)
>>> + | ---
>>> + | ...
>>> +
>>> +--
>> Prior to doing something on master, you should make sure
>> replica2 has received the promote.
>> "wait_lsn" won't work here, because WAL is disabled. You may try
>> test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
> Wait, this moment is dubious. Look, once we issue promote it get
> stuck inside journal write procedure so the next "insert" won't
> proceed until the promote finished. I understand that you point
> to the potential race here because even promote() command may get
> finished but slowed down on nework level and simply reach replica2
> out of other calls. I think without aditional debug output (such
> as promote term exposed via box.info I did in previous series) we
> can't be sure about timings and it seems that i've to return back
> the box.info patch. I mean currently the command
>
> test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
>
> stucks forever because promote yet not finished and next 'insert' simply
> has not been applied.

Ok, I see. I didn't think of that at first.
Look, your `box.info` patch won't help here as well.
Since promote is blocked on its way to WAL, it isn't applied yet,
so we won't see the term increase.

There is a way to dect a blocked promote:
There's ERRINJ_WAL_WRITE_COUNT.
It's incremented each time you call wal_write. Even before the write is 
blocked.

So you need to save ERRINJ_WAL_WRITE_COUNT, then do promote on replica1, 
then
return to replica2 and wait until ERRINJ_WAL_WRITE_COUNT gets bigger 
than your saved
value.


-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v19 2/3] qsync: order access to the limbo terms

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Oct 05, 2021 at 04:25:21PM +0300, Serge Petrenko wrote:
> 
> Here's what can happen. You're the old leader, and you've received a
> promote from someone else already, it's being written now.
> Now some replica sends you an ack. You enter txn_limbo_ack
> (it doesn't check the latch). txn_limbo_ack() may result in writing a
> CONFIRM.
> 
> The PROMOTE is already in progress, so you'll end up writing CONFIRM for
> a wrong leader id (your own) after PROMOTE is written.

This sounds like we need more general lock for limbo operations. And
as we have been discussing already for box change params as well. I'm
starting to doubt that promote latch is a good name. Anyway I'll prepare
a draft version and send it out.

Re: [Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Cyrill Gorcunov via Tarantool-patches]

On Tue, Oct 05, 2021 at 04:55:24PM +0300, Serge Petrenko wrote:
> There is a way to dect a blocked promote:
> There's ERRINJ_WAL_WRITE_COUNT.
> It's incremented each time you call wal_write. Even before the write is
> blocked.
> 
> So you need to save ERRINJ_WAL_WRITE_COUNT, then do promote on replica1,
> then
> return to replica2 and wait until ERRINJ_WAL_WRITE_COUNT gets bigger than
> your saved
> value.

Here is a final version (which I didn't pushed outside yet, since I
started a new branch v20), anyway to share data early.
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] test: add gh-6036-qsync-order test

To test that promotion requests are handled only when appropriate
write to WAL completes, because we update memory data before the
write finishes.

Note that without the patch this test fires assertion

> tarantool: src/box/txn_limbo.c:481: txn_limbo_read_rollback: Assertion `e->txn->signature >= 0' failed.

Part-of #6036

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/replication/election_replica.lua         |   3 +-
 test/replication/gh-6036-qsync-order.result   | 222 ++++++++++++++++++
 test/replication/gh-6036-qsync-order.test.lua | 103 ++++++++
 test/replication/suite.cfg                    |   1 +
 test/replication/suite.ini                    |   2 +-
 5 files changed, 329 insertions(+), 2 deletions(-)
 create mode 100644 test/replication/gh-6036-qsync-order.result
 create mode 100644 test/replication/gh-6036-qsync-order.test.lua

diff --git a/test/replication/election_replica.lua b/test/replication/election_replica.lua
index 3b4d9a123..1dbfa96dc 100644
--- a/test/replication/election_replica.lua
+++ b/test/replication/election_replica.lua
@@ -6,6 +6,7 @@ local SYNCHRO_QUORUM = arg[1] and tonumber(arg[1]) or 3
 local ELECTION_TIMEOUT = arg[2] and tonumber(arg[2]) or 0.1
 local ELECTION_MODE = arg[3] or 'candidate'
 local CONNECT_QUORUM = arg[4] and tonumber(arg[4]) or 3
+local SYNCHRO_TIMEOUT = arg[5] and tonumber(arg[5]) or 0.1
 
 local function instance_uri(instance_id)
     return SOCKET_DIR..'/election_replica'..instance_id..'.sock';
@@ -25,7 +26,7 @@ box.cfg({
     election_mode = ELECTION_MODE,
     election_timeout = ELECTION_TIMEOUT,
     replication_synchro_quorum = SYNCHRO_QUORUM,
-    replication_synchro_timeout = 0.1,
+    replication_synchro_timeout = SYNCHRO_TIMEOUT,
     -- To reveal more election logs.
     log_level = 6,
 })
diff --git a/test/replication/gh-6036-qsync-order.result b/test/replication/gh-6036-qsync-order.result
new file mode 100644
index 000000000..b0d8c126e
--- /dev/null
+++ b/test/replication/gh-6036-qsync-order.result
@@ -0,0 +1,222 @@
+-- test-run result file version 2
+--
+-- gh-6036: verify that terms are locked when we're inside journal
+-- write routine, because parallel appliers may ignore the fact that
+-- the term is updated already but not yet written leading to data
+-- inconsistency.
+--
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd('create server master with script="replication/election_replica1.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('create server replica1 with script="replication/election_replica2.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('create server replica2 with script="replication/election_replica3.lua"')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd("start server master with wait=False, args='1 nil manual 1 10000'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server replica1 with wait=False, args='1 nil manual 1 10000'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server replica2 with wait=False, args='1 nil manual 1 10000'")
+ | ---
+ | - true
+ | ...
+
+test_run:wait_fullmesh({"master", "replica1", "replica2"})
+ | ---
+ | ...
+
+--
+-- Create a synchro space on the master node and make
+-- sure the write processed just fine.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+box.ctl.promote()
+ | ---
+ | ...
+s = box.schema.create_space('test', {is_sync = true})
+ | ---
+ | ...
+_ = s:create_index('pk')
+ | ---
+ | ...
+s:insert{1}
+ | ---
+ | - [1]
+ | ...
+
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+test_run:wait_lsn('replica1', 'master')
+ | ---
+ | ...
+
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+test_run:wait_lsn('replica2', 'master')
+ | ---
+ | ...
+
+--
+-- Drop connection between master and replica1.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica1.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+ | ---
+ | ...
+--
+-- Drop connection between replica1 and master.
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+ | ---
+ | - true
+ | ...
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica2.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+ | ---
+ | ...
+
+--
+-- Here we have the following scheme
+--
+--              replica2 (will be delayed)
+--              /     \
+--          master    replica1
+
+--
+-- Initiate disk delay in a bit tricky way: the next write will
+-- fall into forever sleep.
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 1)
+ | ---
+ | - ok
+ | ...
+
+--
+-- Make replica1 been a leader and start writting data,
+-- the PROMOTE request get queued on replica2 and not
+-- yet processed, same time INSERT won't complete either
+-- waiting for PROMOTE completion first. Note that we
+-- enter replica2 as well just to be sure the PROMOTE
+-- reached it.
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+box.ctl.promote()
+ | ---
+ | ...
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.error.injection.get('ERRINJ_WAL_DELAY_COUNTDOWN') ~= 0 end)
+ | ---
+ | - true
+ | ...
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+_ = require('fiber').create(function() box.space.test:insert{2} end)
+ | ---
+ | ...
+
+--
+-- The master node has no clue that there is a new leader
+-- and continue writing data with obsolete term. Since replica2
+-- is delayed now the INSERT won't proceed yet but get queued.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+_ = require('fiber').create(function() box.space.test:insert{3} end)
+ | ---
+ | ...
+
+--
+-- Finally enable replica2 back. Make sure the data from new replica1
+-- leader get writing while old leader's data ignored.
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_WAL_DELAY', false)
+ | ---
+ | - ok
+ | ...
+test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
+ | ---
+ | - true
+ | ...
+box.space.test:select{}
+ | ---
+ | - - [1]
+ |   - [2]
+ | ...
+
+test_run:switch("default")
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica2')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('delete server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
diff --git a/test/replication/gh-6036-qsync-order.test.lua b/test/replication/gh-6036-qsync-order.test.lua
new file mode 100644
index 000000000..4dac9c59a
--- /dev/null
+++ b/test/replication/gh-6036-qsync-order.test.lua
@@ -0,0 +1,103 @@
+--
+-- gh-6036: verify that terms are locked when we're inside journal
+-- write routine, because parallel appliers may ignore the fact that
+-- the term is updated already but not yet written leading to data
+-- inconsistency.
+--
+test_run = require('test_run').new()
+
+test_run:cmd('create server master with script="replication/election_replica1.lua"')
+test_run:cmd('create server replica1 with script="replication/election_replica2.lua"')
+test_run:cmd('create server replica2 with script="replication/election_replica3.lua"')
+
+test_run:cmd("start server master with wait=False, args='1 nil manual 1 10000'")
+test_run:cmd("start server replica1 with wait=False, args='1 nil manual 1 10000'")
+test_run:cmd("start server replica2 with wait=False, args='1 nil manual 1 10000'")
+
+test_run:wait_fullmesh({"master", "replica1", "replica2"})
+
+--
+-- Create a synchro space on the master node and make
+-- sure the write processed just fine.
+test_run:switch("master")
+box.ctl.promote()
+s = box.schema.create_space('test', {is_sync = true})
+_ = s:create_index('pk')
+s:insert{1}
+
+test_run:switch("replica1")
+test_run:wait_lsn('replica1', 'master')
+
+test_run:switch("replica2")
+test_run:wait_lsn('replica2', 'master')
+
+--
+-- Drop connection between master and replica1.
+test_run:switch("master")
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica1.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+--
+-- Drop connection between replica1 and master.
+test_run:switch("replica1")
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica2.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+
+--
+-- Here we have the following scheme
+--
+--              replica2 (will be delayed)
+--              /     \
+--          master    replica1
+
+--
+-- Initiate disk delay in a bit tricky way: the next write will
+-- fall into forever sleep.
+test_run:switch("replica2")
+box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 1)
+
+--
+-- Make replica1 been a leader and start writting data,
+-- the PROMOTE request get queued on replica2 and not
+-- yet processed, same time INSERT won't complete either
+-- waiting for PROMOTE completion first. Note that we
+-- enter replica2 as well just to be sure the PROMOTE
+-- reached it.
+test_run:switch("replica1")
+box.ctl.promote()
+test_run:switch("replica2")
+test_run:wait_cond(function() return box.error.injection.get('ERRINJ_WAL_DELAY_COUNTDOWN') ~= 0 end)
+test_run:switch("replica1")
+_ = require('fiber').create(function() box.space.test:insert{2} end)
+
+--
+-- The master node has no clue that there is a new leader
+-- and continue writing data with obsolete term. Since replica2
+-- is delayed now the INSERT won't proceed yet but get queued.
+test_run:switch("master")
+_ = require('fiber').create(function() box.space.test:insert{3} end)
+
+--
+-- Finally enable replica2 back. Make sure the data from new replica1
+-- leader get writing while old leader's data ignored.
+test_run:switch("replica2")
+box.error.injection.set('ERRINJ_WAL_DELAY', false)
+test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
+box.space.test:select{}
+
+test_run:switch("default")
+test_run:cmd('stop server master')
+test_run:cmd('stop server replica1')
+test_run:cmd('stop server replica2')
+
+test_run:cmd('delete server master')
+test_run:cmd('delete server replica1')
+test_run:cmd('delete server replica2')
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index 3eee0803c..ed09b2087 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -59,6 +59,7 @@
     "gh-6094-rs-uuid-mismatch.test.lua": {},
     "gh-6127-election-join-new.test.lua": {},
     "gh-6035-applier-filter.test.lua": {},
+    "gh-6036-qsync-order.test.lua": {},
     "election-candidate-promote.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
diff --git a/test/replication/suite.ini b/test/replication/suite.ini
index 77eb95f49..080e4fbf4 100644
--- a/test/replication/suite.ini
+++ b/test/replication/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
 script =  master.lua
 description = tarantool/box, replication
 disabled = consistent.test.lua
-release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua
+release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua gh-6036-qsync-order.test.lua
 config = suite.cfg
 lua_libs = lua/fast_replica.lua lua/rlimit.lua
 use_unix_sockets = True
-- 
2.31.1

Re: [Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Oct 06, 2021 at 01:26:16AM +0300, Cyrill Gorcunov wrote:
> On Tue, Oct 05, 2021 at 04:55:24PM +0300, Serge Petrenko wrote:
> > There is a way to dect a blocked promote:
> > There's ERRINJ_WAL_WRITE_COUNT.
> > It's incremented each time you call wal_write. Even before the write is
> > blocked.
> > 
> > So you need to save ERRINJ_WAL_WRITE_COUNT, then do promote on replica1,
> > then
> > return to replica2 and wait until ERRINJ_WAL_WRITE_COUNT gets bigger than
> > your saved
> > value.
> 
> Here is a final version (which I didn't pushed outside yet, since I
> started a new branch v20), anyway to share data early.
> ---
> From: Cyrill Gorcunov <gorcunov@gmail.com>
> Subject: [PATCH] test: add gh-6036-qsync-order test
> 
> To test that promotion requests are handled only when appropriate
> write to WAL completes, because we update memory data before the
> write finishes.

Drop it please, need to rework :/

Re: [Tarantool-patches] [PATCH v19 3/3] test: add gh-6036-qsync-order test

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Oct 06, 2021 at 01:32:20AM +0300, Cyrill Gorcunov wrote:
> 
> Drop it please, need to rework :/

Here is the last one, take a look please.
---
From ca074681354099a79105bee14ab804492964184a Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Mon, 20 Sep 2021 17:22:38 +0300
Subject: [PATCH] test: add gh-6036-qsync-order test

To test that promotion requests are handled only when appropriate
write to WAL completes, because we update memory data before the
write finishes.

Note that without the patch this test fires assertion

> tarantool: src/box/txn_limbo.c:481: txn_limbo_read_rollback: Assertion `e->txn->signature >= 0' failed.

Part-of #6036

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/replication/election_replica.lua         |   3 +-
 test/replication/gh-6036-qsync-order.result   | 224 ++++++++++++++++++
 test/replication/gh-6036-qsync-order.test.lua | 103 ++++++++
 test/replication/suite.cfg                    |   1 +
 test/replication/suite.ini                    |   2 +-
 5 files changed, 331 insertions(+), 2 deletions(-)
 create mode 100644 test/replication/gh-6036-qsync-order.result
 create mode 100644 test/replication/gh-6036-qsync-order.test.lua

diff --git a/test/replication/election_replica.lua b/test/replication/election_replica.lua
index 3b4d9a123..1dbfa96dc 100644
--- a/test/replication/election_replica.lua
+++ b/test/replication/election_replica.lua
@@ -6,6 +6,7 @@ local SYNCHRO_QUORUM = arg[1] and tonumber(arg[1]) or 3
 local ELECTION_TIMEOUT = arg[2] and tonumber(arg[2]) or 0.1
 local ELECTION_MODE = arg[3] or 'candidate'
 local CONNECT_QUORUM = arg[4] and tonumber(arg[4]) or 3
+local SYNCHRO_TIMEOUT = arg[5] and tonumber(arg[5]) or 0.1
 
 local function instance_uri(instance_id)
     return SOCKET_DIR..'/election_replica'..instance_id..'.sock';
@@ -25,7 +26,7 @@ box.cfg({
     election_mode = ELECTION_MODE,
     election_timeout = ELECTION_TIMEOUT,
     replication_synchro_quorum = SYNCHRO_QUORUM,
-    replication_synchro_timeout = 0.1,
+    replication_synchro_timeout = SYNCHRO_TIMEOUT,
     -- To reveal more election logs.
     log_level = 6,
 })
diff --git a/test/replication/gh-6036-qsync-order.result b/test/replication/gh-6036-qsync-order.result
new file mode 100644
index 000000000..34a7e7803
--- /dev/null
+++ b/test/replication/gh-6036-qsync-order.result
@@ -0,0 +1,224 @@
+-- test-run result file version 2
+--
+-- gh-6036: verify that terms are locked when we're inside journal
+-- write routine, because parallel appliers may ignore the fact that
+-- the term is updated already but not yet written leading to data
+-- inconsistency.
+--
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd('create server master with script="replication/election_replica1.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('create server replica1 with script="replication/election_replica2.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('create server replica2 with script="replication/election_replica3.lua"')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd("start server master with wait=False, args='1 nil manual 1 10000'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server replica1 with wait=False, args='1 nil manual 1 10000'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server replica2 with wait=False, args='1 nil manual 1 10000'")
+ | ---
+ | - true
+ | ...
+
+test_run:wait_fullmesh({"master", "replica1", "replica2"})
+ | ---
+ | ...
+
+--
+-- Create a synchro space on the master node and make
+-- sure the write processed just fine.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+box.ctl.promote()
+ | ---
+ | ...
+s = box.schema.create_space('test', {is_sync = true})
+ | ---
+ | ...
+_ = s:create_index('pk')
+ | ---
+ | ...
+s:insert{1}
+ | ---
+ | - [1]
+ | ...
+
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+test_run:wait_lsn('replica1', 'master')
+ | ---
+ | ...
+
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+test_run:wait_lsn('replica2', 'master')
+ | ---
+ | ...
+
+--
+-- Drop connection between master and replica1.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica1.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+ | ---
+ | ...
+--
+-- Drop connection between replica1 and master.
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+ | ---
+ | - true
+ | ...
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica2.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+ | ---
+ | ...
+
+--
+-- Here we have the following scheme
+--
+--              replica2 (will be delayed)
+--              /     \
+--          master    replica1
+
+--
+-- Initiate disk delay in a bit tricky way: the next write will
+-- fall into forever sleep.
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 1)
+ | ---
+ | - ok
+ | ...
+cnt_before = box.error.injection.get('ERRINJ_WAL_DELAY_COUNTDOWN')
+ | ---
+ | ...
+--
+-- Make replica1 been a leader and start writting data,
+-- the PROMOTE request get queued on replica2 and not
+-- yet processed, same time INSERT won't complete either
+-- waiting for PROMOTE completion first. Note that we
+-- enter replica2 as well just to be sure the PROMOTE
+-- reached it.
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+box.ctl.promote()
+ | ---
+ | ...
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+test_run:wait_cond(function() return box.error.injection.get('ERRINJ_WAL_DELAY_COUNTDOWN') < cnt_before end)
+ | ---
+ | - true
+ | ...
+test_run:switch("replica1")
+ | ---
+ | - true
+ | ...
+_ = require('fiber').create(function() box.space.test:insert{2} end)
+ | ---
+ | ...
+
+--
+-- The master node has no clue that there is a new leader
+-- and continue writing data with obsolete term. Since replica2
+-- is delayed now the INSERT won't proceed yet but get queued.
+test_run:switch("master")
+ | ---
+ | - true
+ | ...
+_ = require('fiber').create(function() box.space.test:insert{3} end)
+ | ---
+ | ...
+
+--
+-- Finally enable replica2 back. Make sure the data from new replica1
+-- leader get writing while old leader's data ignored.
+test_run:switch("replica2")
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_WAL_DELAY', false)
+ | ---
+ | - ok
+ | ...
+test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
+ | ---
+ | - true
+ | ...
+box.space.test:select{}
+ | ---
+ | - - [1]
+ |   - [2]
+ | ...
+
+test_run:switch("default")
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica2')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('delete server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
diff --git a/test/replication/gh-6036-qsync-order.test.lua b/test/replication/gh-6036-qsync-order.test.lua
new file mode 100644
index 000000000..47996998d
--- /dev/null
+++ b/test/replication/gh-6036-qsync-order.test.lua
@@ -0,0 +1,103 @@
+--
+-- gh-6036: verify that terms are locked when we're inside journal
+-- write routine, because parallel appliers may ignore the fact that
+-- the term is updated already but not yet written leading to data
+-- inconsistency.
+--
+test_run = require('test_run').new()
+
+test_run:cmd('create server master with script="replication/election_replica1.lua"')
+test_run:cmd('create server replica1 with script="replication/election_replica2.lua"')
+test_run:cmd('create server replica2 with script="replication/election_replica3.lua"')
+
+test_run:cmd("start server master with wait=False, args='1 nil manual 1 10000'")
+test_run:cmd("start server replica1 with wait=False, args='1 nil manual 1 10000'")
+test_run:cmd("start server replica2 with wait=False, args='1 nil manual 1 10000'")
+
+test_run:wait_fullmesh({"master", "replica1", "replica2"})
+
+--
+-- Create a synchro space on the master node and make
+-- sure the write processed just fine.
+test_run:switch("master")
+box.ctl.promote()
+s = box.schema.create_space('test', {is_sync = true})
+_ = s:create_index('pk')
+s:insert{1}
+
+test_run:switch("replica1")
+test_run:wait_lsn('replica1', 'master')
+
+test_run:switch("replica2")
+test_run:wait_lsn('replica2', 'master')
+
+--
+-- Drop connection between master and replica1.
+test_run:switch("master")
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica1.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+--
+-- Drop connection between replica1 and master.
+test_run:switch("replica1")
+test_run:wait_cond(function() return box.space.test:get{1} ~= nil end)
+box.cfg({                                   \
+    replication = {                         \
+        "unix/:./election_replica2.sock",   \
+        "unix/:./election_replica3.sock",   \
+    },                                      \
+})
+
+--
+-- Here we have the following scheme
+--
+--              replica2 (will be delayed)
+--              /     \
+--          master    replica1
+
+--
+-- Initiate disk delay in a bit tricky way: the next write will
+-- fall into forever sleep.
+test_run:switch("replica2")
+box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 1)
+cnt_before = box.error.injection.get('ERRINJ_WAL_DELAY_COUNTDOWN')
+--
+-- Make replica1 been a leader and start writting data,
+-- the PROMOTE request get queued on replica2 and not
+-- yet processed, same time INSERT won't complete either
+-- waiting for PROMOTE completion first. Note that we
+-- enter replica2 as well just to be sure the PROMOTE
+-- reached it.
+test_run:switch("replica1")
+box.ctl.promote()
+test_run:switch("replica2")
+test_run:wait_cond(function() return box.error.injection.get('ERRINJ_WAL_DELAY_COUNTDOWN') < cnt_before end)
+test_run:switch("replica1")
+_ = require('fiber').create(function() box.space.test:insert{2} end)
+
+--
+-- The master node has no clue that there is a new leader
+-- and continue writing data with obsolete term. Since replica2
+-- is delayed now the INSERT won't proceed yet but get queued.
+test_run:switch("master")
+_ = require('fiber').create(function() box.space.test:insert{3} end)
+
+--
+-- Finally enable replica2 back. Make sure the data from new replica1
+-- leader get writing while old leader's data ignored.
+test_run:switch("replica2")
+box.error.injection.set('ERRINJ_WAL_DELAY', false)
+test_run:wait_cond(function() return box.space.test:get{2} ~= nil end)
+box.space.test:select{}
+
+test_run:switch("default")
+test_run:cmd('stop server master')
+test_run:cmd('stop server replica1')
+test_run:cmd('stop server replica2')
+
+test_run:cmd('delete server master')
+test_run:cmd('delete server replica1')
+test_run:cmd('delete server replica2')
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index 3eee0803c..ed09b2087 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -59,6 +59,7 @@
     "gh-6094-rs-uuid-mismatch.test.lua": {},
     "gh-6127-election-join-new.test.lua": {},
     "gh-6035-applier-filter.test.lua": {},
+    "gh-6036-qsync-order.test.lua": {},
     "election-candidate-promote.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
diff --git a/test/replication/suite.ini b/test/replication/suite.ini
index 77eb95f49..080e4fbf4 100644
--- a/test/replication/suite.ini
+++ b/test/replication/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
 script =  master.lua
 description = tarantool/box, replication
 disabled = consistent.test.lua
-release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua
+release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5430-qsync-promote-crash.test.lua gh-5430-cluster-mvcc.test.lua  gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua gh-5447-downstream-lag.test.lua gh-4040-invalid-msgpack.test.lua gh-6036-qsync-order.test.lua
 config = suite.cfg
 lua_libs = lua/fast_replica.lua lua/rlimit.lua
 use_unix_sockets = True
-- 
2.31.1