* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-11-30 9:35 [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty Serge Petrenko
@ 2020-11-30 9:41 ` Serge Petrenko
2020-11-30 21:22 ` Vladislav Shpilevoy
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Serge Petrenko @ 2020-11-30 9:41 UTC (permalink / raw)
To: v.shpilevoy, gorcunov; +Cc: tarantool-patches
fixed a typo: instace -> instance.
30.11.2020 12:35, Serge Petrenko пишет:
> Users usually use box.ctl.wait_rw() to determine the moment when the
> instance becomes writeable.
> Since the synchronous replication introduction, this function became
> pointless, because even when an instance is writeable, it may fail at
> writing something because its limbo is not empty.
> To fix the problem introduce a new helper, txn_limbo_is_ro() and start
> using it in box_update_ro_summary().
> Call bax_update_ro_summary() every time the limbo gets emptied out or
> changes an owner.
>
> Closes #5440
> ---
> https://github.com/tarantool/tarantool/issues/5440
> sp/gh-5440-on-state-update
>
> Changes in v2:
> - instead of updating is_ro on raft side, make
> sure is_ro is updated every time limbo is
> filled and emptied out.
>
> src/box/box.cc | 3 ++-
> src/box/raft.c | 9 +++----
> src/box/txn_limbo.c | 25 +++++++++++++++++++
> src/box/txn_limbo.h | 3 +++
> test/replication/election_qsync.result | 10 ++++++--
> test/replication/election_qsync.test.lua | 6 +++--
> test/replication/election_qsync_stress.result | 4 +++
> .../election_qsync_stress.test.lua | 2 ++
> 8 files changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index b315635dc..4070cbeab 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -157,7 +157,8 @@ void
> box_update_ro_summary(void)
> {
> bool old_is_ro_summary = is_ro_summary;
> - is_ro_summary = is_ro || is_orphan || raft_is_ro(box_raft());
> + is_ro_summary = is_ro || is_orphan || raft_is_ro(box_raft()) ||
> + txn_limbo_is_ro(&txn_limbo);
> /* In 99% nothing changes. Filter this out first. */
> if (is_ro_summary == old_is_ro_summary)
> return;
> diff --git a/src/box/raft.c b/src/box/raft.c
> index 6d98f5645..8f32a9662 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -155,11 +155,10 @@ box_raft_on_update_f(struct trigger *trigger, void *event)
> struct raft *raft = (struct raft *)event;
> assert(raft == box_raft());
> /*
> - * XXX: in case the instance became a leader, RO must be updated only
> - * after clearing the synchro queue.
> - *
> - * When the instance became a follower, then on the contrary - make it
> - * read-only ASAP, this is good.
> + * When the instance becomes a follower, it's good to make it read-only
> + * ASAP. This way we make sure followers don't write anything.
> + * However, if the instance is transitioning to leader it'll become
> + * writable only after it clears its synchro queue.
> */
> box_update_ro_summary();
> if (raft->state != RAFT_STATE_LEADER)
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 2c35cd785..c406ed4c8 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -33,6 +33,7 @@
> #include "replication.h"
> #include "iproto_constants.h"
> #include "journal.h"
> +#include "box.h"
>
> struct txn_limbo txn_limbo;
>
> @@ -48,10 +49,17 @@ txn_limbo_create(struct txn_limbo *limbo)
> limbo->is_in_rollback = false;
> }
>
> +bool
> +txn_limbo_is_ro(struct txn_limbo *limbo)
> +{
> + return limbo->owner_id != instance_id && !txn_limbo_is_empty(limbo);
> +}
> +
> struct txn_limbo_entry *
> txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> {
> assert(txn_has_flag(txn, TXN_WAIT_SYNC));
> + assert(limbo == &txn_limbo);
> /*
> * Transactions should be added to the limbo before WAL write. Limbo
> * needs that to be able rollback transactions, whose WAL write is in
> @@ -72,11 +80,14 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> }
> if (id == 0)
> id = instance_id;
> + bool make_ro = false;
> if (limbo->owner_id != id) {
> if (limbo->owner_id == REPLICA_ID_NIL ||
> rlist_empty(&limbo->queue)) {
> limbo->owner_id = id;
> limbo->confirmed_lsn = 0;
> + if (id != instance_id)
> + make_ro = true;
> } else {
> diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
> limbo->owner_id);
> @@ -96,6 +107,12 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> e->is_commit = false;
> e->is_rollback = false;
> rlist_add_tail_entry(&limbo->queue, e, in_queue);
> + /*
> + * We added new entries from a remote instance to an empty limbo.
> + * Time to make this instance read-only.
> + */
> + if (make_ro)
> + box_update_ro_summary();
> return e;
> }
>
> @@ -349,6 +366,7 @@ static void
> txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
> {
> assert(limbo->owner_id != REPLICA_ID_NIL);
> + assert(limbo == &txn_limbo);
> struct txn_limbo_entry *e, *tmp;
> rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) {
> /*
> @@ -388,6 +406,9 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
> if (e->txn->signature >= 0)
> txn_complete_success(e->txn);
> }
> + /* Update is_ro once the limbo is clear. */
> + if (txn_limbo_is_empty(limbo))
> + box_update_ro_summary();
> }
>
> /**
> @@ -410,6 +431,7 @@ static void
> txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
> {
> assert(limbo->owner_id != REPLICA_ID_NIL);
> + assert(limbo == &txn_limbo);
> struct txn_limbo_entry *e, *tmp;
> struct txn_limbo_entry *last_rollback = NULL;
> rlist_foreach_entry_reverse(e, &limbo->queue, in_queue) {
> @@ -452,6 +474,9 @@ txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
> if (e == last_rollback)
> break;
> }
> + /* Update is_ro once the limbo is clear. */
> + if (txn_limbo_is_empty(limbo))
> + box_update_ro_summary();
> }
>
> void
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index c7e70ba64..f7d67a826 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -172,6 +172,9 @@ txn_limbo_is_empty(struct txn_limbo *limbo)
> return rlist_empty(&limbo->queue);
> }
>
> +bool
> +txn_limbo_is_ro(struct txn_limbo *limbo);
> +
> static inline struct txn_limbo_entry *
> txn_limbo_first_entry(struct txn_limbo *limbo)
> {
> diff --git a/test/replication/election_qsync.result b/test/replication/election_qsync.result
> index cb349efcc..c06400b38 100644
> --- a/test/replication/election_qsync.result
> +++ b/test/replication/election_qsync.result
> @@ -75,7 +75,10 @@ box.cfg{
> | ---
> | ...
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> + | ---
> + | ...
> +assert(box.info.election.state == 'leader')
> | ---
> | - true
> | ...
> @@ -130,7 +133,10 @@ box.cfg{
> | ---
> | ...
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> + | ---
> + | ...
> +assert(box.info.election.state == 'leader')
> | ---
> | - true
> | ...
> diff --git a/test/replication/election_qsync.test.lua b/test/replication/election_qsync.test.lua
> index eb89e5b79..ea6fc4a61 100644
> --- a/test/replication/election_qsync.test.lua
> +++ b/test/replication/election_qsync.test.lua
> @@ -39,7 +39,8 @@ box.cfg{
> replication_timeout = 0.1, \
> }
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> +assert(box.info.election.state == 'leader')
> lsn = box.info.lsn
> _ = fiber.create(function() \
> ok, err = pcall(box.space.test.replace, box.space.test, {1}) \
> @@ -69,7 +70,8 @@ box.cfg{
> replication_timeout = 0.01, \
> }
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> +assert(box.info.election.state == 'leader')
> _ = box.space.test:replace{2}
> box.space.test:select{}
> box.space.test:drop()
> diff --git a/test/replication/election_qsync_stress.result b/test/replication/election_qsync_stress.result
> index 9fab2f1d7..1380c910b 100644
> --- a/test/replication/election_qsync_stress.result
> +++ b/test/replication/election_qsync_stress.result
> @@ -69,6 +69,9 @@ leader_port = test_run:eval(old_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> | ---
> | ...
> +c:eval('box.ctl.wait_rw()')
> + | ---
> + | ...
>
> _ = c:eval('box.schema.space.create("test", {is_sync=true})')
> | ---
> @@ -94,6 +97,7 @@ for i = 1,10 do
> leader_port = test_run:eval(new_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> c:eval('box.cfg{replication_synchro_timeout=1000}')
> + c:eval('box.ctl.wait_rw()')
> c.space._schema:replace{'smth'}
> c.space.test:get{i}
> test_run:cmd('start server '..old_leader..' with wait=True, wait_load=True, args="2 0.4"')
> diff --git a/test/replication/election_qsync_stress.test.lua b/test/replication/election_qsync_stress.test.lua
> index 0ba15eef7..d70601cc5 100644
> --- a/test/replication/election_qsync_stress.test.lua
> +++ b/test/replication/election_qsync_stress.test.lua
> @@ -40,6 +40,7 @@ old_leader_nr = get_leader(nrs)
> old_leader = 'election_replica'..old_leader_nr
> leader_port = test_run:eval(old_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> +c:eval('box.ctl.wait_rw()')
>
> _ = c:eval('box.schema.space.create("test", {is_sync=true})')
> _ = c:eval('box.space.test:create_index("pk")')
> @@ -58,6 +59,7 @@ for i = 1,10 do
> leader_port = test_run:eval(new_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> c:eval('box.cfg{replication_synchro_timeout=1000}')
> + c:eval('box.ctl.wait_rw()')
> c.space._schema:replace{'smth'}
> c.space.test:get{i}
> test_run:cmd('start server '..old_leader..' with wait=True, wait_load=True, args="2 0.4"')
--
Serge Petrenko
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-11-30 9:35 [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty Serge Petrenko
2020-11-30 9:41 ` Serge Petrenko
@ 2020-11-30 21:22 ` Vladislav Shpilevoy
2020-12-01 8:46 ` Serge Petrenko
2020-12-01 11:06 ` Alexander V. Tikhonov
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-30 21:22 UTC (permalink / raw)
To: Serge Petrenko, gorcunov, Alexander V. Tikhonov; +Cc: tarantool-patches
Hi! Thanks for the patch!
On 30.11.2020 10:35, Serge Petrenko via Tarantool-patches wrote:
> Users usually use box.ctl.wait_rw() to determine the moment when the
> instance becomes writeable.
> Since the synchronous replication introduction, this function became
> pointless, because even when an instance is writeable, it may fail at
> writing something because its limbo is not empty.
> To fix the problem introduce a new helper, txn_limbo_is_ro() and start
> using it in box_update_ro_summary().
> Call bax_update_ro_summary() every time the limbo gets emptied out or
bax -> box.
> changes an owner.
>
> Closes #5440
Sasha (Tikh.), please, validate the branch is good to push.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-11-30 21:22 ` Vladislav Shpilevoy
@ 2020-12-01 8:46 ` Serge Petrenko
0 siblings, 0 replies; 11+ messages in thread
From: Serge Petrenko @ 2020-12-01 8:46 UTC (permalink / raw)
To: Vladislav Shpilevoy, gorcunov, Alexander V. Tikhonov; +Cc: tarantool-patches
01.12.2020 00:22, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
> On 30.11.2020 10:35, Serge Petrenko via Tarantool-patches wrote:
>> Users usually use box.ctl.wait_rw() to determine the moment when the
>> instance becomes writeable.
>> Since the synchronous replication introduction, this function became
>> pointless, because even when an instance is writeable, it may fail at
>> writing something because its limbo is not empty.
>> To fix the problem introduce a new helper, txn_limbo_is_ro() and start
>> using it in box_update_ro_summary().
>> Call bax_update_ro_summary() every time the limbo gets emptied out or
> bax -> box.
Thanks! Fixed.
>
>> changes an owner.
>>
>> Closes #5440
> Sasha (Tikh.), please, validate the branch is good to push.
--
Serge Petrenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-11-30 9:35 [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty Serge Petrenko
2020-11-30 9:41 ` Serge Petrenko
2020-11-30 21:22 ` Vladislav Shpilevoy
@ 2020-12-01 11:06 ` Alexander V. Tikhonov
2020-12-01 22:25 ` Vladislav Shpilevoy
2020-12-03 12:44 ` Alexander V. Tikhonov
4 siblings, 0 replies; 11+ messages in thread
From: Alexander V. Tikhonov @ 2020-12-01 11:06 UTC (permalink / raw)
To: Serge Petrenko; +Cc: tarantool-patches
Hi Sergey, , thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.
[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/223561823
On Mon, Nov 30, 2020 at 12:35:51PM +0300, Serge Petrenko via Tarantool-patches wrote:
> Users usually use box.ctl.wait_rw() to determine the moment when the
> instance becomes writeable.
> Since the synchronous replication introduction, this function became
> pointless, because even when an instance is writeable, it may fail at
> writing something because its limbo is not empty.
> To fix the problem introduce a new helper, txn_limbo_is_ro() and start
> using it in box_update_ro_summary().
> Call bax_update_ro_summary() every time the limbo gets emptied out or
> changes an owner.
>
> Closes #5440
> ---
> https://github.com/tarantool/tarantool/issues/5440
> sp/gh-5440-on-state-update
>
> Changes in v2:
> - instead of updating is_ro on raft side, make
> sure is_ro is updated every time limbo is
> filled and emptied out.
>
> src/box/box.cc | 3 ++-
> src/box/raft.c | 9 +++----
> src/box/txn_limbo.c | 25 +++++++++++++++++++
> src/box/txn_limbo.h | 3 +++
> test/replication/election_qsync.result | 10 ++++++--
> test/replication/election_qsync.test.lua | 6 +++--
> test/replication/election_qsync_stress.result | 4 +++
> .../election_qsync_stress.test.lua | 2 ++
> 8 files changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index b315635dc..4070cbeab 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -157,7 +157,8 @@ void
> box_update_ro_summary(void)
> {
> bool old_is_ro_summary = is_ro_summary;
> - is_ro_summary = is_ro || is_orphan || raft_is_ro(box_raft());
> + is_ro_summary = is_ro || is_orphan || raft_is_ro(box_raft()) ||
> + txn_limbo_is_ro(&txn_limbo);
> /* In 99% nothing changes. Filter this out first. */
> if (is_ro_summary == old_is_ro_summary)
> return;
> diff --git a/src/box/raft.c b/src/box/raft.c
> index 6d98f5645..8f32a9662 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -155,11 +155,10 @@ box_raft_on_update_f(struct trigger *trigger, void *event)
> struct raft *raft = (struct raft *)event;
> assert(raft == box_raft());
> /*
> - * XXX: in case the instance became a leader, RO must be updated only
> - * after clearing the synchro queue.
> - *
> - * When the instance became a follower, then on the contrary - make it
> - * read-only ASAP, this is good.
> + * When the instance becomes a follower, it's good to make it read-only
> + * ASAP. This way we make sure followers don't write anything.
> + * However, if the instance is transitioning to leader it'll become
> + * writable only after it clears its synchro queue.
> */
> box_update_ro_summary();
> if (raft->state != RAFT_STATE_LEADER)
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 2c35cd785..c406ed4c8 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -33,6 +33,7 @@
> #include "replication.h"
> #include "iproto_constants.h"
> #include "journal.h"
> +#include "box.h"
>
> struct txn_limbo txn_limbo;
>
> @@ -48,10 +49,17 @@ txn_limbo_create(struct txn_limbo *limbo)
> limbo->is_in_rollback = false;
> }
>
> +bool
> +txn_limbo_is_ro(struct txn_limbo *limbo)
> +{
> + return limbo->owner_id != instance_id && !txn_limbo_is_empty(limbo);
> +}
> +
> struct txn_limbo_entry *
> txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> {
> assert(txn_has_flag(txn, TXN_WAIT_SYNC));
> + assert(limbo == &txn_limbo);
> /*
> * Transactions should be added to the limbo before WAL write. Limbo
> * needs that to be able rollback transactions, whose WAL write is in
> @@ -72,11 +80,14 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> }
> if (id == 0)
> id = instance_id;
> + bool make_ro = false;
> if (limbo->owner_id != id) {
> if (limbo->owner_id == REPLICA_ID_NIL ||
> rlist_empty(&limbo->queue)) {
> limbo->owner_id = id;
> limbo->confirmed_lsn = 0;
> + if (id != instance_id)
> + make_ro = true;
> } else {
> diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
> limbo->owner_id);
> @@ -96,6 +107,12 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> e->is_commit = false;
> e->is_rollback = false;
> rlist_add_tail_entry(&limbo->queue, e, in_queue);
> + /*
> + * We added new entries from a remote instance to an empty limbo.
> + * Time to make this instance read-only.
> + */
> + if (make_ro)
> + box_update_ro_summary();
> return e;
> }
>
> @@ -349,6 +366,7 @@ static void
> txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
> {
> assert(limbo->owner_id != REPLICA_ID_NIL);
> + assert(limbo == &txn_limbo);
> struct txn_limbo_entry *e, *tmp;
> rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) {
> /*
> @@ -388,6 +406,9 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
> if (e->txn->signature >= 0)
> txn_complete_success(e->txn);
> }
> + /* Update is_ro once the limbo is clear. */
> + if (txn_limbo_is_empty(limbo))
> + box_update_ro_summary();
> }
>
> /**
> @@ -410,6 +431,7 @@ static void
> txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
> {
> assert(limbo->owner_id != REPLICA_ID_NIL);
> + assert(limbo == &txn_limbo);
> struct txn_limbo_entry *e, *tmp;
> struct txn_limbo_entry *last_rollback = NULL;
> rlist_foreach_entry_reverse(e, &limbo->queue, in_queue) {
> @@ -452,6 +474,9 @@ txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
> if (e == last_rollback)
> break;
> }
> + /* Update is_ro once the limbo is clear. */
> + if (txn_limbo_is_empty(limbo))
> + box_update_ro_summary();
> }
>
> void
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index c7e70ba64..f7d67a826 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -172,6 +172,9 @@ txn_limbo_is_empty(struct txn_limbo *limbo)
> return rlist_empty(&limbo->queue);
> }
>
> +bool
> +txn_limbo_is_ro(struct txn_limbo *limbo);
> +
> static inline struct txn_limbo_entry *
> txn_limbo_first_entry(struct txn_limbo *limbo)
> {
> diff --git a/test/replication/election_qsync.result b/test/replication/election_qsync.result
> index cb349efcc..c06400b38 100644
> --- a/test/replication/election_qsync.result
> +++ b/test/replication/election_qsync.result
> @@ -75,7 +75,10 @@ box.cfg{
> | ---
> | ...
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> + | ---
> + | ...
> +assert(box.info.election.state == 'leader')
> | ---
> | - true
> | ...
> @@ -130,7 +133,10 @@ box.cfg{
> | ---
> | ...
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> + | ---
> + | ...
> +assert(box.info.election.state == 'leader')
> | ---
> | - true
> | ...
> diff --git a/test/replication/election_qsync.test.lua b/test/replication/election_qsync.test.lua
> index eb89e5b79..ea6fc4a61 100644
> --- a/test/replication/election_qsync.test.lua
> +++ b/test/replication/election_qsync.test.lua
> @@ -39,7 +39,8 @@ box.cfg{
> replication_timeout = 0.1, \
> }
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> +assert(box.info.election.state == 'leader')
> lsn = box.info.lsn
> _ = fiber.create(function() \
> ok, err = pcall(box.space.test.replace, box.space.test, {1}) \
> @@ -69,7 +70,8 @@ box.cfg{
> replication_timeout = 0.01, \
> }
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> +assert(box.info.election.state == 'leader')
> _ = box.space.test:replace{2}
> box.space.test:select{}
> box.space.test:drop()
> diff --git a/test/replication/election_qsync_stress.result b/test/replication/election_qsync_stress.result
> index 9fab2f1d7..1380c910b 100644
> --- a/test/replication/election_qsync_stress.result
> +++ b/test/replication/election_qsync_stress.result
> @@ -69,6 +69,9 @@ leader_port = test_run:eval(old_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> | ---
> | ...
> +c:eval('box.ctl.wait_rw()')
> + | ---
> + | ...
>
> _ = c:eval('box.schema.space.create("test", {is_sync=true})')
> | ---
> @@ -94,6 +97,7 @@ for i = 1,10 do
> leader_port = test_run:eval(new_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> c:eval('box.cfg{replication_synchro_timeout=1000}')
> + c:eval('box.ctl.wait_rw()')
> c.space._schema:replace{'smth'}
> c.space.test:get{i}
> test_run:cmd('start server '..old_leader..' with wait=True, wait_load=True, args="2 0.4"')
> diff --git a/test/replication/election_qsync_stress.test.lua b/test/replication/election_qsync_stress.test.lua
> index 0ba15eef7..d70601cc5 100644
> --- a/test/replication/election_qsync_stress.test.lua
> +++ b/test/replication/election_qsync_stress.test.lua
> @@ -40,6 +40,7 @@ old_leader_nr = get_leader(nrs)
> old_leader = 'election_replica'..old_leader_nr
> leader_port = test_run:eval(old_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> +c:eval('box.ctl.wait_rw()')
>
> _ = c:eval('box.schema.space.create("test", {is_sync=true})')
> _ = c:eval('box.space.test:create_index("pk")')
> @@ -58,6 +59,7 @@ for i = 1,10 do
> leader_port = test_run:eval(new_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> c:eval('box.cfg{replication_synchro_timeout=1000}')
> + c:eval('box.ctl.wait_rw()')
> c.space._schema:replace{'smth'}
> c.space.test:get{i}
> test_run:cmd('start server '..old_leader..' with wait=True, wait_load=True, args="2 0.4"')
> --
> 2.24.3 (Apple Git-128)
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-11-30 9:35 [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty Serge Petrenko
` (2 preceding siblings ...)
2020-12-01 11:06 ` Alexander V. Tikhonov
@ 2020-12-01 22:25 ` Vladislav Shpilevoy
2020-12-02 8:10 ` Serge Petrenko
2020-12-03 12:44 ` Alexander V. Tikhonov
4 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2020-12-01 22:25 UTC (permalink / raw)
To: Serge Petrenko, gorcunov; +Cc: tarantool-patches
Pushed to master and 2.6.
Could you please backport it to 2.5 in a new branch? It seems
we need a test which wouldn't depend on elections anyhow.
Because we don't have them in 2.5.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-12-01 22:25 ` Vladislav Shpilevoy
@ 2020-12-02 8:10 ` Serge Petrenko
2020-12-02 21:38 ` Vladislav Shpilevoy
2020-12-03 21:21 ` Vladislav Shpilevoy
0 siblings, 2 replies; 11+ messages in thread
From: Serge Petrenko @ 2020-12-02 8:10 UTC (permalink / raw)
To: Vladislav Shpilevoy, gorcunov; +Cc: tarantool-patches
02.12.2020 01:25, Vladislav Shpilevoy пишет:
> Pushed to master and 2.6.
>
> Could you please backport it to 2.5 in a new branch? It seems
> we need a test which wouldn't depend on elections anyhow.
> Because we don't have them in 2.5.
No problem. Here's the branch: sp/gh-5440-2.5
I had to cherry pick your commit introducing is_ro_summary, since it
wasn't in 2.5
I've also made a test for read-only limbo without elections. Please,
push it to 2.6
and master as well. I think a proper test won't hurt there.
The test's also on the branch. Pasting it here just in case.
============================================================
commit 9d14706bacbd62afd474af7bb6d00f675241010e
Author: Serge Petrenko <sergepetrenko@tarantool.org>
Date: Wed Dec 2 11:02:07 2020 +0300
test: add tests for ro on non-empty limbo
Follow-up #5440
diff --git a/test/replication/gh-5440-qsync-ro.result
b/test/replication/gh-5440-qsync-ro.result
new file mode 100644
index 000000000..1ece26a42
--- /dev/null
+++ b/test/replication/gh-5440-qsync-ro.result
@@ -0,0 +1,133 @@
+-- test-run result file version 2
+--
+-- gh-5440 everyone but the limbo owner is read-only on non-empty limbo.
+--
+env = require('test_run')
+ | ---
+ | ...
+test_run = env.new()
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'replication')
+ | ---
+ | ...
+test_run:cmd('create server replica with rpl_master=default,
script="replication/replica.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+_ = box.schema.space.create('test', {is_sync=true})
+ | ---
+ | ...
+_ = box.space.test:create_index('pk')
+ | ---
+ | ...
+
+old_synchro_quorum = box.cfg.replication_synchro_quorum
+ | ---
+ | ...
+old_synchro_timeout = box.cfg.replication_synchro_timeout
+ | ---
+ | ...
+
+-- Make sure that the master stalls on commit leaving the limbo non-empty.
+box.cfg{replication_synchro_quorum=3, replication_synchro_timeout=1000}
+ | ---
+ | ...
+
+f = fiber.new(function() box.space.test:insert{1} end)
+ | ---
+ | ...
+f:status()
+ | ---
+ | - suspended
+ | ...
+
+-- Wait till replica's limbo is non-empty.
+test_run:wait_lsn('replica', 'default')
+ | ---
+ | ...
+test_run:cmd('switch replica')
+ | ---
+ | - true
+ | ...
+
+box.info.ro
+ | ---
+ | - true
+ | ...
+box.space.test:insert{2}
+ | ---
+ | - error: Can't modify data because this instance is in read-only mode.
+ | ...
+success = false
+ | ---
+ | ...
+f = require('fiber').new(function() box.ctl.wait_rw() success = true end)
+ | ---
+ | ...
+f:status()
+ | ---
+ | - suspended
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+
+-- Empty the limbo.
+box.cfg{replication_synchro_quorum=2}
+ | ---
+ | ...
+
+test_run:cmd('switch replica')
+ | ---
+ | - true
+ | ...
+
+test_run:wait_cond(function() return success end)
+ | ---
+ | - true
+ | ...
+box.info.ro
+ | ---
+ | - false
+ | ...
+-- Should succeed now.
+box.space.test:insert{2}
+ | ---
+ | - [2]
+ | ...
+
+-- Cleanup.
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+box.cfg{replication_synchro_quorum=old_synchro_quorum,\
+ replication_synchro_timeout=old_synchro_timeout}
+ | ---
+ | ...
+box.space.test:drop()
+ | ---
+ | ...
+test_run:cmd('stop server replica')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica')
+ | ---
+ | - true
+ | ...
+box.schema.user.revoke('guest', 'replication')
+ | ---
+ | ...
diff --git a/test/replication/gh-5440-qsync-ro.test.lua
b/test/replication/gh-5440-qsync-ro.test.lua
new file mode 100644
index 000000000..d63ec9c1e
--- /dev/null
+++ b/test/replication/gh-5440-qsync-ro.test.lua
@@ -0,0 +1,53 @@
+--
+-- gh-5440 everyone but the limbo owner is read-only on non-empty limbo.
+--
+env = require('test_run')
+test_run = env.new()
+fiber = require('fiber')
+
+box.schema.user.grant('guest', 'replication')
+test_run:cmd('create server replica with rpl_master=default,
script="replication/replica.lua"')
+test_run:cmd('start server replica with wait=True, wait_load=True')
+
+_ = box.schema.space.create('test', {is_sync=true})
+_ = box.space.test:create_index('pk')
+
+old_synchro_quorum = box.cfg.replication_synchro_quorum
+old_synchro_timeout = box.cfg.replication_synchro_timeout
+
+-- Make sure that the master stalls on commit leaving the limbo non-empty.
+box.cfg{replication_synchro_quorum=3, replication_synchro_timeout=1000}
+
+f = fiber.new(function() box.space.test:insert{1} end)
+f:status()
+
+-- Wait till replica's limbo is non-empty.
+test_run:wait_lsn('replica', 'default')
+test_run:cmd('switch replica')
+
+box.info.ro
+box.space.test:insert{2}
+success = false
+f = require('fiber').new(function() box.ctl.wait_rw() success = true end)
+f:status()
+
+test_run:cmd('switch default')
+
+-- Empty the limbo.
+box.cfg{replication_synchro_quorum=2}
+
+test_run:cmd('switch replica')
+
+test_run:wait_cond(function() return success end)
+box.info.ro
+-- Should succeed now.
+box.space.test:insert{2}
+
+-- Cleanup.
+test_run:cmd('switch default')
+box.cfg{replication_synchro_quorum=old_synchro_quorum,\
+ replication_synchro_timeout=old_synchro_timeout}
+box.space.test:drop()
+test_run:cmd('stop server replica')
+test_run:cmd('delete server replica')
+box.schema.user.revoke('guest', 'replication')
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index a862f5a97..d01f0023b 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -32,6 +32,7 @@
"gh-4739-vclock-assert.test.lua": {},
"gh-4730-applier-rollback.test.lua": {},
"gh-4928-tx-boundaries.test.lua": {},
+ "gh-5440-qsync-ro.test.lua": {},
"*": {
"memtx": {"engine": "memtx"},
"vinyl": {"engine": "vinyl"}
--
Serge Petrenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty
2020-11-30 9:35 [Tarantool-patches] [PATCH v2] box: make instace ro while limbo is not empty Serge Petrenko
` (3 preceding siblings ...)
2020-12-01 22:25 ` Vladislav Shpilevoy
@ 2020-12-03 12:44 ` Alexander V. Tikhonov
4 siblings, 0 replies; 11+ messages in thread
From: Alexander V. Tikhonov @ 2020-12-03 12:44 UTC (permalink / raw)
To: Serge Petrenko; +Cc: tarantool-patches
Hi Sergey, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.
[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/223561823
On Mon, Nov 30, 2020 at 12:35:51PM +0300, Serge Petrenko via Tarantool-patches wrote:
> Users usually use box.ctl.wait_rw() to determine the moment when the
> instance becomes writeable.
> Since the synchronous replication introduction, this function became
> pointless, because even when an instance is writeable, it may fail at
> writing something because its limbo is not empty.
> To fix the problem introduce a new helper, txn_limbo_is_ro() and start
> using it in box_update_ro_summary().
> Call bax_update_ro_summary() every time the limbo gets emptied out or
> changes an owner.
>
> Closes #5440
> ---
> https://github.com/tarantool/tarantool/issues/5440
> sp/gh-5440-on-state-update
>
> Changes in v2:
> - instead of updating is_ro on raft side, make
> sure is_ro is updated every time limbo is
> filled and emptied out.
>
> src/box/box.cc | 3 ++-
> src/box/raft.c | 9 +++----
> src/box/txn_limbo.c | 25 +++++++++++++++++++
> src/box/txn_limbo.h | 3 +++
> test/replication/election_qsync.result | 10 ++++++--
> test/replication/election_qsync.test.lua | 6 +++--
> test/replication/election_qsync_stress.result | 4 +++
> .../election_qsync_stress.test.lua | 2 ++
> 8 files changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index b315635dc..4070cbeab 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -157,7 +157,8 @@ void
> box_update_ro_summary(void)
> {
> bool old_is_ro_summary = is_ro_summary;
> - is_ro_summary = is_ro || is_orphan || raft_is_ro(box_raft());
> + is_ro_summary = is_ro || is_orphan || raft_is_ro(box_raft()) ||
> + txn_limbo_is_ro(&txn_limbo);
> /* In 99% nothing changes. Filter this out first. */
> if (is_ro_summary == old_is_ro_summary)
> return;
> diff --git a/src/box/raft.c b/src/box/raft.c
> index 6d98f5645..8f32a9662 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -155,11 +155,10 @@ box_raft_on_update_f(struct trigger *trigger, void *event)
> struct raft *raft = (struct raft *)event;
> assert(raft == box_raft());
> /*
> - * XXX: in case the instance became a leader, RO must be updated only
> - * after clearing the synchro queue.
> - *
> - * When the instance became a follower, then on the contrary - make it
> - * read-only ASAP, this is good.
> + * When the instance becomes a follower, it's good to make it read-only
> + * ASAP. This way we make sure followers don't write anything.
> + * However, if the instance is transitioning to leader it'll become
> + * writable only after it clears its synchro queue.
> */
> box_update_ro_summary();
> if (raft->state != RAFT_STATE_LEADER)
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 2c35cd785..c406ed4c8 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -33,6 +33,7 @@
> #include "replication.h"
> #include "iproto_constants.h"
> #include "journal.h"
> +#include "box.h"
>
> struct txn_limbo txn_limbo;
>
> @@ -48,10 +49,17 @@ txn_limbo_create(struct txn_limbo *limbo)
> limbo->is_in_rollback = false;
> }
>
> +bool
> +txn_limbo_is_ro(struct txn_limbo *limbo)
> +{
> + return limbo->owner_id != instance_id && !txn_limbo_is_empty(limbo);
> +}
> +
> struct txn_limbo_entry *
> txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> {
> assert(txn_has_flag(txn, TXN_WAIT_SYNC));
> + assert(limbo == &txn_limbo);
> /*
> * Transactions should be added to the limbo before WAL write. Limbo
> * needs that to be able rollback transactions, whose WAL write is in
> @@ -72,11 +80,14 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> }
> if (id == 0)
> id = instance_id;
> + bool make_ro = false;
> if (limbo->owner_id != id) {
> if (limbo->owner_id == REPLICA_ID_NIL ||
> rlist_empty(&limbo->queue)) {
> limbo->owner_id = id;
> limbo->confirmed_lsn = 0;
> + if (id != instance_id)
> + make_ro = true;
> } else {
> diag_set(ClientError, ER_UNCOMMITTED_FOREIGN_SYNC_TXNS,
> limbo->owner_id);
> @@ -96,6 +107,12 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
> e->is_commit = false;
> e->is_rollback = false;
> rlist_add_tail_entry(&limbo->queue, e, in_queue);
> + /*
> + * We added new entries from a remote instance to an empty limbo.
> + * Time to make this instance read-only.
> + */
> + if (make_ro)
> + box_update_ro_summary();
> return e;
> }
>
> @@ -349,6 +366,7 @@ static void
> txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
> {
> assert(limbo->owner_id != REPLICA_ID_NIL);
> + assert(limbo == &txn_limbo);
> struct txn_limbo_entry *e, *tmp;
> rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) {
> /*
> @@ -388,6 +406,9 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
> if (e->txn->signature >= 0)
> txn_complete_success(e->txn);
> }
> + /* Update is_ro once the limbo is clear. */
> + if (txn_limbo_is_empty(limbo))
> + box_update_ro_summary();
> }
>
> /**
> @@ -410,6 +431,7 @@ static void
> txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
> {
> assert(limbo->owner_id != REPLICA_ID_NIL);
> + assert(limbo == &txn_limbo);
> struct txn_limbo_entry *e, *tmp;
> struct txn_limbo_entry *last_rollback = NULL;
> rlist_foreach_entry_reverse(e, &limbo->queue, in_queue) {
> @@ -452,6 +474,9 @@ txn_limbo_read_rollback(struct txn_limbo *limbo, int64_t lsn)
> if (e == last_rollback)
> break;
> }
> + /* Update is_ro once the limbo is clear. */
> + if (txn_limbo_is_empty(limbo))
> + box_update_ro_summary();
> }
>
> void
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index c7e70ba64..f7d67a826 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -172,6 +172,9 @@ txn_limbo_is_empty(struct txn_limbo *limbo)
> return rlist_empty(&limbo->queue);
> }
>
> +bool
> +txn_limbo_is_ro(struct txn_limbo *limbo);
> +
> static inline struct txn_limbo_entry *
> txn_limbo_first_entry(struct txn_limbo *limbo)
> {
> diff --git a/test/replication/election_qsync.result b/test/replication/election_qsync.result
> index cb349efcc..c06400b38 100644
> --- a/test/replication/election_qsync.result
> +++ b/test/replication/election_qsync.result
> @@ -75,7 +75,10 @@ box.cfg{
> | ---
> | ...
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> + | ---
> + | ...
> +assert(box.info.election.state == 'leader')
> | ---
> | - true
> | ...
> @@ -130,7 +133,10 @@ box.cfg{
> | ---
> | ...
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> + | ---
> + | ...
> +assert(box.info.election.state == 'leader')
> | ---
> | - true
> | ...
> diff --git a/test/replication/election_qsync.test.lua b/test/replication/election_qsync.test.lua
> index eb89e5b79..ea6fc4a61 100644
> --- a/test/replication/election_qsync.test.lua
> +++ b/test/replication/election_qsync.test.lua
> @@ -39,7 +39,8 @@ box.cfg{
> replication_timeout = 0.1, \
> }
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> +assert(box.info.election.state == 'leader')
> lsn = box.info.lsn
> _ = fiber.create(function() \
> ok, err = pcall(box.space.test.replace, box.space.test, {1}) \
> @@ -69,7 +70,8 @@ box.cfg{
> replication_timeout = 0.01, \
> }
>
> -test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> +box.ctl.wait_rw()
> +assert(box.info.election.state == 'leader')
> _ = box.space.test:replace{2}
> box.space.test:select{}
> box.space.test:drop()
> diff --git a/test/replication/election_qsync_stress.result b/test/replication/election_qsync_stress.result
> index 9fab2f1d7..1380c910b 100644
> --- a/test/replication/election_qsync_stress.result
> +++ b/test/replication/election_qsync_stress.result
> @@ -69,6 +69,9 @@ leader_port = test_run:eval(old_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> | ---
> | ...
> +c:eval('box.ctl.wait_rw()')
> + | ---
> + | ...
>
> _ = c:eval('box.schema.space.create("test", {is_sync=true})')
> | ---
> @@ -94,6 +97,7 @@ for i = 1,10 do
> leader_port = test_run:eval(new_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> c:eval('box.cfg{replication_synchro_timeout=1000}')
> + c:eval('box.ctl.wait_rw()')
> c.space._schema:replace{'smth'}
> c.space.test:get{i}
> test_run:cmd('start server '..old_leader..' with wait=True, wait_load=True, args="2 0.4"')
> diff --git a/test/replication/election_qsync_stress.test.lua b/test/replication/election_qsync_stress.test.lua
> index 0ba15eef7..d70601cc5 100644
> --- a/test/replication/election_qsync_stress.test.lua
> +++ b/test/replication/election_qsync_stress.test.lua
> @@ -40,6 +40,7 @@ old_leader_nr = get_leader(nrs)
> old_leader = 'election_replica'..old_leader_nr
> leader_port = test_run:eval(old_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> +c:eval('box.ctl.wait_rw()')
>
> _ = c:eval('box.schema.space.create("test", {is_sync=true})')
> _ = c:eval('box.space.test:create_index("pk")')
> @@ -58,6 +59,7 @@ for i = 1,10 do
> leader_port = test_run:eval(new_leader, 'box.cfg.listen')[1]
> c = netbox.connect(leader_port)
> c:eval('box.cfg{replication_synchro_timeout=1000}')
> + c:eval('box.ctl.wait_rw()')
> c.space._schema:replace{'smth'}
> c.space.test:get{i}
> test_run:cmd('start server '..old_leader..' with wait=True, wait_load=True, args="2 0.4"')
> --
> 2.24.3 (Apple Git-128)
>
^ permalink raw reply [flat|nested] 11+ messages in thread