[Tarantool-patches] [PATCH 3/3] box: fix an assertion failure in box.ctl.promote()
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun May 23 15:18:26 MSK 2021
Hi! Thanks for the patch!
I see some of CI jobs have failed the new test:
https://github.com/tarantool/tarantool/runs/2620153809
See 4 comments below.
> diff --git a/src/box/box.cc b/src/box/box.cc
> index c10e0d8bf..1b1e7eec0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1442,17 +1442,22 @@ box_quorum_on_ack_f(struct trigger *trigger, void *event)
> }
>
> /**
> - * Wait until at least @a quorum of nodes confirm @a target_lsn from the node
> - * with id @a lead_id.
> + * Wait until at least @a quorum of nodes confirm the last available synchronous
> + * entry from the node with id @a lead_id.
> */
> static int
> -box_wait_quorum(uint32_t lead_id, int64_t target_lsn, int quorum,
> +box_wait_quorum(uint32_t lead_id, struct txn_limbo_entry **entry, int quorum,
> double timeout)
1. Maybe try to leave this function not depending on the limbo
and its entries? It was supposed to wait for replication of just
LSN, not necessarily a synchronous transaction.
> {
> struct box_quorum_trigger t;
> memset(&t, 0, sizeof(t));
> vclock_create(&t.vclock);
>
> + *entry = txn_limbo_wait_lsn_assigned(&txn_limbo);
> + if (*entry == NULL)
> + return -1;
> + int64_t target_lsn = (*entry)->lsn;
> +
> /* Take this node into account immediately. */
> int ack_count = vclock_get(box_vclock, lead_id) >= target_lsn;
> replicaset_foreach(replica) {
> @@ -1622,22 +1627,17 @@ box_promote(void)
> }
> }
>
> - /*
> - * promote() is a no-op on the limbo owner, so all the rows
> - * in the limbo must've come through the applier meaning they already
> - * have an lsn assigned, even if their WAL write hasn't finished yet.
> - */
> - wait_lsn = txn_limbo_last_synchro_entry(&txn_limbo)->lsn;
> - assert(wait_lsn > 0);
> -
> - rc = box_wait_quorum(former_leader_id, wait_lsn, quorum,
> + struct txn_limbo_entry *last_entry;
> + rc = box_wait_quorum(former_leader_id,&last_entry, quorum,
2. Missing whitespace after the first argument.
> replication_synchro_timeout);
> if (rc == 0) {
> + wait_lsn = last_entry->lsn;
> if (quorum < replication_synchro_quorum) {
> diag_set(ClientError, ER_QUORUM_WAIT, quorum,
> "quorum was increased while waiting");
> rc = -1;
> - } else if (wait_lsn < txn_limbo_last_synchro_entry(&txn_limbo)->lsn) {
> + } else if (last_entry !=
> + txn_limbo_last_synchro_entry(&txn_limbo)) {
> diag_set(ClientError, ER_QUORUM_WAIT, quorum,
> "new synchronous transactions appeared");
> rc = -1;
3. Could all the 3 commits be replaced with calling wal_sync() in the
beginning of the promote() if we see the last LSN is unknown? After
wal_sync() several outcomes are possible:
- All was rolled back, and the limbo is empty;
- The last transaction is different after sync - it means
it was added during promote() which is an error like in
the code above;
- The transaction in the end of the limbo is the same.
In the last case you work like before - box_wait_quorum() with the
known LSN. Will it work?
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index f287369a2..406f2de89 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -69,6 +69,48 @@ txn_limbo_last_synchro_entry(struct txn_limbo *limbo)
> return NULL;
> }
>
> +static int
> +txn_limbo_wait_lsn_assigned_f(struct trigger *trig, void *event)
> +{
> + (void)event;
> + struct fiber *fiber = trig->data;
> + fiber_wakeup(fiber);
> + return 0;
> +}
> +
> +struct txn_limbo_entry *
> +txn_limbo_wait_lsn_assigned(struct txn_limbo *limbo)
> +{
> + assert(!txn_limbo_is_empty(limbo));
> + struct txn_limbo_entry *entry = txn_limbo_last_synchro_entry(limbo);
> + if (entry->lsn >= 0)
> + return entry;
> +
> + struct trigger write_trigger, rollback_trigger;
> + trigger_create(&write_trigger, txn_limbo_wait_lsn_assigned_f, fiber(),
> + NULL);
> + trigger_create(&rollback_trigger, txn_limbo_wait_lsn_assigned_f,
> + fiber(), NULL);
> + txn_on_wal_write(entry->txn, &write_trigger);
> + txn_on_rollback(entry->txn, &rollback_trigger);
> + do {
> + fiber_yield();
> + if (fiber_is_cancelled()) {
> + diag_set(FiberIsCancelled);
> + entry = NULL;
> + break;
> + }
> + if (entry->txn->signature < 0) {
> + diag_set(ClientError, ER_SYNC_ROLLBACK);
> + entry = NULL;
> + break;
> + }
> + } while (entry->lsn == -1);
> + trigger_clear(&write_trigger);
> + trigger_clear(&rollback_trigger);
4. Why do you need the LSN assigned in the on_wal_write trigger in
the previous commit? I can't see where do you use it here.
More information about the Tarantool-patches
mailing list