From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 53515430407 for ; Sat, 15 Aug 2020 18:04:29 +0300 (MSK) References: <20200814211442.667099-1-gorcunov@gmail.com> <20200814211442.667099-5-gorcunov@gmail.com> From: Vladislav Shpilevoy Message-ID: <19c845c0-6cf9-288b-1cb6-942681d93ec3@tarantool.org> Date: Sat, 15 Aug 2020 17:04:27 +0200 MIME-Version: 1.0 In-Reply-To: <20200814211442.667099-5-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v7 4/8] qsync: direct write of CONFIRM/ROLLBACK into a journal List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov , tml Hi! Thanks for the patch! See 3 comments below. On 14.08.2020 23:14, Cyrill Gorcunov wrote: > When we need to write CONFIRM or ROLLBACK message (which is > a binary record in msgpack format) into a journal we use txn code > to allocate a new transaction, encode there a message and pass it > to walk the long txn path before it hit the journal. This is not > only resource wasting but also somehow strange from architectural > point of view. > > Instead lets encode a record on the stack and write it to the journal > directly. > > Part-of #5129 > > Signed-off-by: Cyrill Gorcunov > --- > src/box/txn_limbo.c | 64 ++++++++++++++++++++++----------------------- > 1 file changed, 32 insertions(+), 32 deletions(-) > > diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c > index ed8c10419..447630d23 100644 > --- a/src/box/txn_limbo.c > +++ b/src/box/txn_limbo.c > @@ -284,46 +296,34 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, uint32_t type, int64_t lsn) > > /* > * This is a synchronous commit so we can > - * use body and row allocated on a stack. > + * allocate everything on a stack. > */ 1. Unnecessary change. > struct synchro_body_bin body; > struct xrow_header row; > - struct request request = { > - .header = &row, > - }; > + char buf[sizeof(struct journal_entry) + > + sizeof(struct xrow_header *)]; 2. Is there a guarantee, that 'buf' will be aligned by at least 8 bytes? > > - struct txn *txn = txn_begin(); > - if (txn == NULL) > - goto rollback; > + struct journal_entry *entry = (struct journal_entry *)buf; > + entry->rows[0] = &row; > > xrow_encode_synchro(&row, &body, &req); > > - /* > - * This is not really a transaction. It just uses txn API > - * to put the data into WAL. And obviously it should not > - * go to the limbo and block on the very same sync > - * transaction which it tries to confirm now. > - */ > - txn_set_flag(txn, TXN_FORCE_ASYNC); > - > - if (txn_begin_stmt(txn, NULL) != 0) > - goto rollback; > - if (txn_commit_stmt(txn, &request) != 0) > - goto rollback; > - if (txn_commit(txn) != 0) > - goto rollback; > - return; > + journal_entry_create(entry, 1, xrow_approx_len(&row), > + txn_limbo_write_cb, fiber()); > > -rollback: > - /* > - * XXX: the stub is supposed to be removed once it is defined what to do > - * when a synchro request WAL write fails. One of the possible > - * solutions: log the error, keep the limbo queue as is and probably put > - * in rollback mode. Then provide a hook to call manually when WAL > - * problems are fixed. Or retry automatically with some period. > - */ > - panic("Could not write a synchro request to WAL: lsn = %lld, type = " > - "%s\n", lsn, iproto_type_name(type)); > + if (journal_write(entry) != 0 || entry->res < 0) { > + diag_set(ClientError, ER_WAL_IO); > + diag_log(); > + /* > + * XXX: the stub is supposed to be removed once it is defined what to do > + * when a synchro request WAL write fails. One of the possible > + * solutions: log the error, keep the limbo queue as is and probably put > + * in rollback mode. Then provide a hook to call manually when WAL > + * problems are fixed. Or retry automatically with some period. 3. Out of 80 symbols. > + */ > + panic("Could not write a synchro request to WAL: lsn = %lld, type = " > + "%s\n", lsn, type == IPROTO_CONFIRM ? "CONFIRM" : "ROLLBACK"); > + } > } > > /** >