[Tarantool-patches] [PATCH v2 2/2] wal: reorder tx rows so that a tx ends on a global row
Serge Petrenko
sergepetrenko at tarantool.org
Tue May 26 12:41:38 MSK 2020
25.05.2020 19:34, Konstantin Osipov пишет:
> * Serge Petrenko <sergepetrenko at tarantool.org> [20/05/25 14:00]:
>
> While I think it's an acceptable strategy for a bugfix, I still
> think relaying has to be fixed to be transactional, the current
> xstream api is a huge legacy we're stuck with since Tarantool 1.3!
>
> It saddens me a great deal the relay patch may be shuffled into
> the desk once the problem is not as urgent.
Hi! Thanks for your answer.
I understand your concern with transactional relay rework. However, this
approach faces some non-trivial problems we discussed verbally. That's why
I chose a more simple solution.
I have a question regarding this patch. Do you think I should reorder
the tx rows
so that the last row is a global one? Maybe it'd be better to just set
is_commit
flag on the last global row? This breaks tx boundaries in local WAL, but
preserves
them as is in replication and in other instances' WALs. I don't think
we'll need tx
boundariesin local WAL anyway, but still breaking the invariant that the
last tx
row has an is_commit flagset to true is scary.
>
>> Since we stopped sending local space operations in replication, the last
>> tx row has to be global in order to preserve tx boundaries on replica.
>> If the last row happens to be a local one, replica will never receive
>> the tx end marker, yielding the following errors:
>> `ER_UNSUPPORTED: replication does not support interleaving
>> transactions`.
>>
>> In order to fix the problem reorder the rows before writing them to WAL
>> so that the last tx row is a global one.
>> Do nothing if the transaction is fully local. It isn't replicated so the
>> tx borders aren't an issue here.
>>
>> Such reordering doesn't break anything, since only the relative positions
>> between some local rows and one global row are changed. Local and global
>> rows do not share a common lsn counter: local rows use a zero instance
>> id. So each row gets the same lsn it would get without the reordering,
>> the transaction looks exactly the same when replicated, and recovery
>> also isn't an issue, since local and global space operations can be
>> reordered freely.
>>
>> Follow-up #4114
>> Closes #4928
>> ---
>> src/box/wal.c | 19 ++-
>> test/replication/gh-4928-tx-boundaries.result | 138 ++++++++++++++++++
>> .../gh-4928-tx-boundaries.test.lua | 61 ++++++++
>> test/replication/suite.cfg | 1 +
>> 4 files changed, 218 insertions(+), 1 deletion(-)
>> create mode 100644 test/replication/gh-4928-tx-boundaries.result
>> create mode 100644 test/replication/gh-4928-tx-boundaries.test.lua
>>
>> diff --git a/src/box/wal.c b/src/box/wal.c
>> index ef4d84920..0921c7261 100644
>> --- a/src/box/wal.c
>> +++ b/src/box/wal.c
>> @@ -958,6 +958,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>> int64_t tsn = 0;
>> struct xrow_header **start = row;
>> struct xrow_header **first_glob_row = row;
>> + struct xrow_header **last_glob_row = end - 1;
>> /** Assign LSN to all local rows. */
>> for ( ; row < end; row++) {
>> if ((*row)->replica_id == 0) {
>> @@ -971,6 +972,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>> */
>> if ((*row)->group_id != GROUP_LOCAL) {
>> (*row)->replica_id = instance_id;
>> + last_glob_row = row;
>> }
>>
>> (*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) +
>> @@ -987,7 +989,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>> first_glob_row = row;
>> }
>> (*row)->tsn = tsn == 0 ? (*start)->lsn : tsn;
>> - (*row)->is_commit = row == end - 1;
>> + (*row)->is_commit = false;
>> } else {
>> int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id);
>> if (diff <= vclock_get(vclock_diff,
>> @@ -1013,6 +1015,21 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>> */
>> for (row = start; row < first_glob_row; row++)
>> (*row)->tsn = tsn;
>> +
>> + /*
>> + * If a mixed transaction ends on a local row, float up
>> + * the last global row, so that the upper tx boundary is
>> + * delivered to the replica.
>> + */
>> + if (last_glob_row < end - 1) {
>> + struct xrow_header *tmp = *last_glob_row;
>> + memmove(last_glob_row, last_glob_row + 1,
>> + (end - 1 - last_glob_row) *
>> + sizeof(struct xrow_header *));
>> + *(end - 1) = tmp;
>> + }
>> +
>> + (*(end - 1))->is_commit = true;
>> }
>>
>> static void
>> diff --git a/test/replication/gh-4928-tx-boundaries.result b/test/replication/gh-4928-tx-boundaries.result
>> new file mode 100644
>> index 000000000..969bd8438
>> --- /dev/null
>> +++ b/test/replication/gh-4928-tx-boundaries.result
>> @@ -0,0 +1,138 @@
>> +-- test-run result file version 2
>> +-- gh-4928. Test that transactions mixing local and global
>> +-- space operations are replicated correctly.
>> +env = require('test_run')
>> + | ---
>> + | ...
>> +test_run = env.new()
>> + | ---
>> + | ...
>> +bit = require('bit')
>> + | ---
>> + | ...
>> +
>> +-- Init.
>> +box.schema.user.grant('guest', 'replication')
>> + | ---
>> + | ...
>> +_ = box.schema.space.create('glob')
>> + | ---
>> + | ...
>> +_ = box.schema.space.create('loc', {is_local=true})
>> + | ---
>> + | ...
>> +_ = box.space.glob:create_index('pk')
>> + | ---
>> + | ...
>> +_ = box.space.loc:create_index('pk')
>> + | ---
>> + | ...
>> +
>> +function gen_mixed_tx(i)\
>> + box.begin()\
>> + if bit.band(i, 1) ~= 0 then\
>> + box.space.glob:insert{10 * i + 1}\
>> + else\
>> + box.space.loc:insert{10 * i + 1}\
>> + end\
>> + if bit.band(i, 2) ~= 0 then\
>> + box.space.glob:insert{10 * i + 2}\
>> + else\
>> + box.space.loc:insert{10 * i + 2}\
>> + end\
>> + if bit.band(i, 4) ~= 0 then\
>> + box.space.glob:insert{10 * i + 3}\
>> + else\
>> + box.space.loc:insert{10 * i + 3}\
>> + end\
>> + box.commit()\
>> +end
>> + | ---
>> + | ...
>> +
>> +test_run:cmd("create server replica with rpl_master=default,\
>> + script='replication/replica.lua'")
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('start server replica')
>> + | ---
>> + | - true
>> + | ...
>> +test_run:wait_downstream(2, {status='follow'})
>> + | ---
>> + | - true
>> + | ...
>> +
>> +for i = 0, 7 do gen_mixed_tx(i) end
>> + | ---
>> + | ...
>> +
>> +box.info.replication[2].status
>> + | ---
>> + | - null
>> + | ...
>> +
>> +vclock = box.info.vclock
>> + | ---
>> + | ...
>> +vclock[0] = nil
>> + | ---
>> + | ...
>> +test_run:wait_vclock("replica", vclock)
>> + | ---
>> + | ...
>> +
>> +test_run:cmd('switch replica')
>> + | ---
>> + | - true
>> + | ...
>> +
>> +box.info.status
>> + | ---
>> + | - running
>> + | ...
>> +box.info.replication[1].upstream.status
>> + | ---
>> + | - follow
>> + | ...
>> +
>> +box.space.glob:select{}
>> + | ---
>> + | - - [11]
>> + | - [22]
>> + | - [31]
>> + | - [32]
>> + | - [43]
>> + | - [51]
>> + | - [53]
>> + | - [62]
>> + | - [63]
>> + | - [71]
>> + | - [72]
>> + | - [73]
>> + | ...
>> +
>> +test_run:cmd('switch default')
>> + | ---
>> + | - true
>> + | ...
>> +
>> +-- Cleanup.
>> +test_run:cmd('stop server replica')
>> + | ---
>> + | - true
>> + | ...
>> +test_run:cmd('delete server replica')
>> + | ---
>> + | - true
>> + | ...
>> +box.schema.user.revoke('guest', 'replication')
>> + | ---
>> + | ...
>> +box.space.loc:drop()
>> + | ---
>> + | ...
>> +box.space.glob:drop()
>> + | ---
>> + | ...
>> diff --git a/test/replication/gh-4928-tx-boundaries.test.lua b/test/replication/gh-4928-tx-boundaries.test.lua
>> new file mode 100644
>> index 000000000..92526fc51
>> --- /dev/null
>> +++ b/test/replication/gh-4928-tx-boundaries.test.lua
>> @@ -0,0 +1,61 @@
>> +-- gh-4928. Test that transactions mixing local and global
>> +-- space operations are replicated correctly.
>> +env = require('test_run')
>> +test_run = env.new()
>> +bit = require('bit')
>> +
>> +-- Init.
>> +box.schema.user.grant('guest', 'replication')
>> +_ = box.schema.space.create('glob')
>> +_ = box.schema.space.create('loc', {is_local=true})
>> +_ = box.space.glob:create_index('pk')
>> +_ = box.space.loc:create_index('pk')
>> +
>> +function gen_mixed_tx(i)\
>> + box.begin()\
>> + if bit.band(i, 1) ~= 0 then\
>> + box.space.glob:insert{10 * i + 1}\
>> + else\
>> + box.space.loc:insert{10 * i + 1}\
>> + end\
>> + if bit.band(i, 2) ~= 0 then\
>> + box.space.glob:insert{10 * i + 2}\
>> + else\
>> + box.space.loc:insert{10 * i + 2}\
>> + end\
>> + if bit.band(i, 4) ~= 0 then\
>> + box.space.glob:insert{10 * i + 3}\
>> + else\
>> + box.space.loc:insert{10 * i + 3}\
>> + end\
>> + box.commit()\
>> +end
>> +
>> +test_run:cmd("create server replica with rpl_master=default,\
>> + script='replication/replica.lua'")
>> +test_run:cmd('start server replica')
>> +test_run:wait_downstream(2, {status='follow'})
>> +
>> +for i = 0, 7 do gen_mixed_tx(i) end
>> +
>> +box.info.replication[2].status
>> +
>> +vclock = box.info.vclock
>> +vclock[0] = nil
>> +test_run:wait_vclock("replica", vclock)
>> +
>> +test_run:cmd('switch replica')
>> +
>> +box.info.status
>> +box.info.replication[1].upstream.status
>> +
>> +box.space.glob:select{}
>> +
>> +test_run:cmd('switch default')
>> +
>> +-- Cleanup.
>> +test_run:cmd('stop server replica')
>> +test_run:cmd('delete server replica')
>> +box.schema.user.revoke('guest', 'replication')
>> +box.space.loc:drop()
>> +box.space.glob:drop()
>> diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
>> index d2743b5ed..f357b07da 100644
>> --- a/test/replication/suite.cfg
>> +++ b/test/replication/suite.cfg
>> @@ -18,6 +18,7 @@
>> "gh-4606-admin-creds.test.lua": {},
>> "gh-4739-vclock-assert.test.lua": {},
>> "gh-4730-applier-rollback.test.lua": {},
>> + "gh-4928-tx-boundaries.test.lua": {},
>> "*": {
>> "memtx": {"engine": "memtx"},
>> "vinyl": {"engine": "vinyl"}
>> --
>> 2.24.2 (Apple Git-127)
>>
--
Serge Petrenko
More information about the Tarantool-patches
mailing list