txn_limbo: don't duplicate confirmations in WAL

When an ACK was received for an already confirmed transaction
whose CONFIRM WAL write is in progress, it produced a second
CONFIRM in WAL with the same LSN.

That was unnecessary work taking time and disk space for WAL
records. Although it didn't lead to any bugs. Just was very
inefficient.

This patch makes confirmation LSN monotonically grow. In case more
ACKs are received for an already confirmed LSN, its confirmation
is not written second time.

Closes #5144

src/box/txn_limbo.c

@@ -41,6 +41,7 @@ txn_limbo_create(struct txn_limbo *limbo)
 	limbo->instance_id = REPLICA_ID_NIL;
 	fiber_cond_create(&limbo->wait_cond);
 	vclock_create(&limbo->vclock);
+	limbo->confirmed_lsn = 0;
 	limbo->rollback_count = 0;
 	limbo->is_in_rollback = false;
 }
@@ -312,6 +313,8 @@ txn_limbo_write_confirm_rollback(struct txn_limbo *limbo, int64_t lsn,
 static int
 txn_limbo_write_confirm(struct txn_limbo *limbo, int64_t lsn)
 {
+	assert(lsn > limbo->confirmed_lsn);
+	limbo->confirmed_lsn = lsn;
 	return txn_limbo_write_confirm_rollback(limbo, lsn, true);
 }
 
@@ -361,6 +364,8 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
 static int
 txn_limbo_write_rollback(struct txn_limbo *limbo, int64_t lsn)
 {
+	assert(lsn > limbo->confirmed_lsn);
+	assert(!limbo->is_in_rollback);
 	limbo->is_in_rollback = true;
 	int rc = txn_limbo_write_confirm_rollback(limbo, lsn, false);
 	limbo->is_in_rollback = false;
@@ -438,7 +443,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn)
 			confirm_lsn = e->lsn;
 		}
 	}
-	if (confirm_lsn == -1)
+	if (confirm_lsn == -1 || confirm_lsn <= limbo->confirmed_lsn)
 		return;
 	if (txn_limbo_write_confirm(limbo, confirm_lsn) != 0) {
 		// TODO: what to do here?.
@@ -581,7 +586,7 @@ txn_limbo_on_parameters_change(struct txn_limbo *limbo)
 			assert(confirm_lsn > 0);
 		}
 	}
-	if (confirm_lsn > 0) {
+	if (confirm_lsn > limbo->confirmed_lsn) {
 		if (txn_limbo_write_confirm(limbo, confirm_lsn) != 0) {
 			panic("Couldn't write CONFIRM to WAL");
 			return;

src/box/txn_limbo.h

@@ -124,6 +124,14 @@ struct txn_limbo {
 	 * transactions, created on the limbo's owner node.
 	 */
 	struct vclock vclock;
+	/**
+	 * Maximal LSN gathered quorum and either already confirmed in WAL, or
+	 * whose confirmation is in progress right now. Any attempt to confirm
+	 * something smaller than this value can be safely ignored. Moreover,
+	 * any attempt to rollback something starting from <= this LSN is
+	 * illegal.
+	 */
+	int64_t confirmed_lsn;
 	/**
 	 * Total number of performed rollbacks. It used as a guard
 	 * to do some actions assuming all limbo transactions will

test/replication/gh-5144-qsync-dup-confirm.result

+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+--
+-- gh-5144: qsync should not write duplicate confirms for the same LSN. That
+-- could happen when first CONFIRM write is in progress, and more ACKs arrive.
+--
+box.schema.user.grant('guest', 'super')
+ | ---
+ | ...
+
+test_run:cmd('create server replica1 with rpl_master=default,                   \
+             script="replication/replica1.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica1 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('create server replica2 with rpl_master=default,                   \
+             script="replication/replica2.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica2 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+box.cfg{replication_synchro_quorum = 2, replication_synchro_timeout = 1000}
+ | ---
+ | ...
+
+_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
+ | ---
+ | ...
+_ = _:create_index('pk')
+ | ---
+ | ...
+
+-- Remember the current LSN. In the end, when the following synchronous
+-- transaction is committed, result LSN should be this value +2: for the
+-- transaction itself and for CONFIRM.
+lsn_before_txn = box.info.lsn
+ | ---
+ | ...
+
+box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 1)
+ | ---
+ | - ok
+ | ...
+ok, err = nil
+ | ---
+ | ...
+f = fiber.create(function()                                                     \
+    ok, err = pcall(box.space.sync.insert, box.space.sync, {1})                 \
+end)
+ | ---
+ | ...
+-- First replica's ACK is received. Quorum 2 is achieved. CONFIRM write is in
+-- progress.
+while not box.error.injection.get("ERRINJ_WAL_DELAY") do fiber.sleep(0.001) end
+ | ---
+ | ...
+
+-- Wait when both replicas confirm receipt of the sync transaction. It would
+-- mean the master knows both LSNs and either already tried to write CONFIRM
+-- twice, or didn't and will not.
+function wait_lsn_ack(id, lsn)                                                  \
+    local this_id = box.info.id                                                 \
+    test_run:wait_downstream(id, {status='follow'})                             \
+    test_run:wait_cond(function()                                               \
+        return box.info.replication[id].downstream.vclock[this_id] >= lsn       \
+    end)                                                                        \
+end
+ | ---
+ | ...
+replica1_id = test_run:get_server_id('replica1')
+ | ---
+ | ...
+replica2_id = test_run:get_server_id('replica2')
+ | ---
+ | ...
+lsn = box.info.lsn
+ | ---
+ | ...
+wait_lsn_ack(replica1_id, lsn)
+ | ---
+ | ...
+wait_lsn_ack(replica2_id, lsn)
+ | ---
+ | ...
+
+-- Decrease already achieved quorum to check that it won't generate the same
+-- CONFIRM on the parameter update.
+box.cfg{replication_synchro_quorum = 1}
+ | ---
+ | ...
+
+-- Let the transaction finish. CONFIRM should be in WAL now.
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+ | ---
+ | - ok
+ | ...
+test_run:wait_cond(function() return f:status() == 'dead' end)
+ | ---
+ | - true
+ | ...
+ok, err
+ | ---
+ | - true
+ | - [1]
+ | ...
+
+-- First +1 is the transaction itself. Second +1 is CONFIRM.
+assert(box.info.lsn - lsn_before_txn == 2)
+ | ---
+ | - true
+ | ...
+
+box.space.sync:drop()
+ | ---
+ | ...
+
+test_run:cmd('stop server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica2')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
+
+box.schema.user.revoke('guest', 'super')
+ | ---
+ | ...

test/replication/gh-5144-qsync-dup-confirm.test.lua

+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+fiber = require('fiber')
+--
+-- gh-5144: qsync should not write duplicate confirms for the same LSN. That
+-- could happen when first CONFIRM write is in progress, and more ACKs arrive.
+--
+box.schema.user.grant('guest', 'super')
+
+test_run:cmd('create server replica1 with rpl_master=default,                   \
+             script="replication/replica1.lua"')
+test_run:cmd('start server replica1 with wait=True, wait_load=True')
+
+test_run:cmd('create server replica2 with rpl_master=default,                   \
+             script="replication/replica2.lua"')
+test_run:cmd('start server replica2 with wait=True, wait_load=True')
+
+box.cfg{replication_synchro_quorum = 2, replication_synchro_timeout = 1000}
+
+_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
+_ = _:create_index('pk')
+
+-- Remember the current LSN. In the end, when the following synchronous
+-- transaction is committed, result LSN should be this value +2: for the
+-- transaction itself and for CONFIRM.
+lsn_before_txn = box.info.lsn
+
+box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 1)
+ok, err = nil
+f = fiber.create(function()                                                     \
+    ok, err = pcall(box.space.sync.insert, box.space.sync, {1})                 \
+end)
+-- First replica's ACK is received. Quorum 2 is achieved. CONFIRM write is in
+-- progress.
+while not box.error.injection.get("ERRINJ_WAL_DELAY") do fiber.sleep(0.001) end
+
+-- Wait when both replicas confirm receipt of the sync transaction. It would
+-- mean the master knows both LSNs and either already tried to write CONFIRM
+-- twice, or didn't and will not.
+function wait_lsn_ack(id, lsn)                                                  \
+    local this_id = box.info.id                                                 \
+    test_run:wait_downstream(id, {status='follow'})                             \
+    test_run:wait_cond(function()                                               \
+        return box.info.replication[id].downstream.vclock[this_id] >= lsn       \
+    end)                                                                        \
+end
+replica1_id = test_run:get_server_id('replica1')
+replica2_id = test_run:get_server_id('replica2')
+lsn = box.info.lsn
+wait_lsn_ack(replica1_id, lsn)
+wait_lsn_ack(replica2_id, lsn)
+
+-- Decrease already achieved quorum to check that it won't generate the same
+-- CONFIRM on the parameter update.
+box.cfg{replication_synchro_quorum = 1}
+
+-- Let the transaction finish. CONFIRM should be in WAL now.
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+test_run:wait_cond(function() return f:status() == 'dead' end)
+ok, err
+
+-- First +1 is the transaction itself. Second +1 is CONFIRM.
+assert(box.info.lsn - lsn_before_txn == 2)
+
+box.space.sync:drop()
+
+test_run:cmd('stop server replica1')
+test_run:cmd('delete server replica1')
+test_run:cmd('stop server replica2')
+test_run:cmd('delete server replica2')
+
+box.schema.user.revoke('guest', 'super')

test/replication/replica1.lua

+replica.lua
\ No newline at end of file

test/replication/replica2.lua

+replica.lua
\ No newline at end of file

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
+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
 config = suite.cfg
 lua_libs = lua/fast_replica.lua lua/rlimit.lua
 use_unix_sockets = True