From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1C055445320 for ; Wed, 22 Jul 2020 01:42:47 +0300 (MSK) From: Vladislav Shpilevoy Date: Wed, 22 Jul 2020 00:42:43 +0200 Message-Id: In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH 1/2] txn_limbo: single function to confirm transactions List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com Before this patch there were some structural problems with the limbo code (still are now, but less). It wasn't clear when lifetime of a transaction ends. When it it is committed, when rolled back, when completion is called. With rollback things are more or less bearable, but not with commit. Transaction's completion could be done at the same time with setting limbo_entry.is_commit flag. Or with setting the flag + yield + completion. This led to having a weird crutch in txn_limbo_wait_complete() to make async transactions explicitly wait until the previous sync transactions are written to WAL. The commit flag could be set in 3!!! different places - parameters update handler, ACK handler, and confirmation handler. In these 3 places there were various assumptions making it really hard to understand what is happening here and there, what is the difference. Not counting how much code was duplicated. This patch makes the commit path always come through confirmation reader, as the most logical place for that, and already covering almost all the tricky cases. Now there is a guarantee that a transaction is completed if it is removed from the limbo queue. No need to check TXN_IS_DONE, wait for anything, whatsoever. Also the patch is a preparation for removal of TXN_IS_DONE check from the main path of txn_commit(). txn_limbo_wait_complete() shouldn't ever return a not finished transaction for that. Part of #5143 --- src/box/txn_limbo.c | 89 ++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 53 deletions(-) diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c index d5b887d36..a3936c569 100644 --- a/src/box/txn_limbo.c +++ b/src/box/txn_limbo.c @@ -221,6 +221,12 @@ do_rollback: complete: assert(txn_limbo_entry_is_complete(entry)); + /* + * Entry is *always* removed from the limbo by the same fiber, which + * installed the commit/rollback flag. + */ + assert(rlist_empty(&entry->in_queue)); + assert(txn_has_flag(txn, TXN_IS_DONE)); /* * The first tx to be rolled back already performed all * the necessary cleanups for us. @@ -229,28 +235,6 @@ complete: diag_set(ClientError, ER_SYNC_ROLLBACK); return -1; } - /* - * The entry might be not the first in the limbo. It - * happens when there was a sync transaction and async - * transaction. The sync and async went to WAL. After sync - * WAL write is done, it may be already ACKed by the - * needed replica count. Now it marks self as committed - * and does the same for the next async txn. Then it - * starts writing CONFIRM. During that the async - * transaction finishes its WAL write, sees it is - * committed and ends up here. Not being the first - * transaction in the limbo. - */ - while (!rlist_empty(&entry->in_queue) && - txn_limbo_first_entry(limbo) != entry) { - bool cancellable = fiber_set_cancellable(false); - fiber_yield(); - fiber_set_cancellable(cancellable); - } - if (!rlist_empty(&entry->in_queue)) - txn_limbo_remove(limbo, entry); - txn_clear_flag(txn, TXN_WAIT_SYNC); - txn_clear_flag(txn, TXN_WAIT_ACK); return 0; } @@ -319,16 +303,22 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn) struct txn_limbo_entry *e, *tmp; rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) { /* - * Confirm a transaction if - * - it is a sync transaction covered by the - * confirmation LSN; - * - it is an async transaction, and it is the - * last in the queue. So it does not depend on - * a not finished sync transaction anymore and - * can be confirmed too. + * Check if it is an async transaction last in the queue. When + * it is last, it does not depend on a not finished sync + * transaction anymore and can be confirmed right away. */ - if (e->lsn > lsn && txn_has_flag(e->txn, TXN_WAIT_ACK)) - break; + if (txn_has_flag(e->txn, TXN_WAIT_ACK)) { + /* Sync transaction not covered by the confirmation. */ + if (e->lsn > lsn) + break; + /* + * Sync transaction not yet received an LSN. Happens + * only to local master transactions whose WAL write is + * in progress. + */ + if (e->lsn == -1) + break; + } e->is_commit = true; txn_limbo_remove(limbo, e); txn_clear_flag(e->txn, TXN_WAIT_SYNC); @@ -403,7 +393,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn) assert(limbo->instance_id != REPLICA_ID_NIL); int64_t prev_lsn = vclock_get(&limbo->vclock, replica_id); vclock_follow(&limbo->vclock, replica_id, lsn); - struct txn_limbo_entry *e, *last_quorum = NULL; + struct txn_limbo_entry *e; int64_t confirm_lsn = -1; rlist_foreach_entry(e, &limbo->queue, in_queue) { assert(e->ack_count <= VCLOCK_MAX); @@ -416,7 +406,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn) */ if (!txn_has_flag(e->txn, TXN_WAIT_ACK)) { assert(e->lsn == -1); - if (last_quorum == NULL) + if (confirm_lsn == -1) continue; } else if (e->lsn <= prev_lsn) { continue; @@ -425,10 +415,8 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn) } else { confirm_lsn = e->lsn; } - e->is_commit = true; - last_quorum = e; } - if (last_quorum == NULL) + if (confirm_lsn == -1) return; if (txn_limbo_write_confirm(limbo, confirm_lsn) != 0) { // TODO: what to do here?. @@ -437,16 +425,7 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn) // able to write ROLLBACK? return; } - /* - * Wakeup all the entries in direct order as soon - * as confirmation message is written to WAL. - */ - rlist_foreach_entry(e, &limbo->queue, in_queue) { - if (e->txn->fiber != fiber()) - fiber_wakeup(e->txn->fiber); - if (e == last_quorum) - break; - } + txn_limbo_read_confirm(limbo, confirm_lsn); } /** @@ -579,16 +558,20 @@ txn_limbo_on_parameters_change(struct txn_limbo *limbo) confirm_lsn = e->lsn; assert(confirm_lsn > 0); } - e->is_commit = true; } - if (confirm_lsn > 0 && - txn_limbo_write_confirm(limbo, confirm_lsn) != 0) { - panic("Couldn't write CONFIRM to WAL"); - return; + if (confirm_lsn > 0) { + if (txn_limbo_write_confirm(limbo, confirm_lsn) != 0) { + panic("Couldn't write CONFIRM to WAL"); + return; + } + txn_limbo_read_confirm(limbo, confirm_lsn); } /* - * Wakeup all. Confirmed will be committed. Timed out will - * rollback. + * Wakeup all the others - timed out will rollback. Also + * there can be non-transactional waiters, such as CONFIRM + * waiters. They are bound to a transaction, but if they + * wait on replica, they won't see timeout update. Because + * sync transactions can live on replica infinitely. */ fiber_cond_broadcast(&limbo->wait_cond); } -- 2.21.1 (Apple Git-122.3)