From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 5BE2E469710 for ; Fri, 29 May 2020 14:42:16 +0300 (MSK) References: From: Serge Petrenko Message-ID: <266e9768-27ea-e924-e562-846e74df7760@tarantool.org> Date: Fri, 29 May 2020 14:42:14 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2 2/2] wal: reorder tx rows so that a tx ends on a global row List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 29.05.2020 01:54, Vladislav Shpilevoy пишет: > Thanks for the patch, > But I have a comment! > It is a nice crutch, > Yet we need one more moment. > > The patch basically sacrifices transaction rows order > and WAL correctness for the sake of replication. It > does not look right. Why can't we leave WAL as is, and > tweak all these things in relay? It looks really wrong > to change statements order. Especially taking into > account this is needed *only* for replication. For example, > consider FKs. > > A local space has a FOREIGN KEY reference to a global > space. To make it work, we need to insert into the global > space first, and then into the local space. When you change > the order, the local insert goes first, and violates the > foreign key. So if we will check FKs on recovery (when we > will have FKs in box), this patch will break them. > > Alternative to relay - append a dummy NOP statement in the > end of the transaction, which would be global. But this is > also a crutch. I think the TSNs figuring out should be done > in relay. It could keep track of the current transaction, > change TSNs and is_commit when necessary. Hi! Thanks for the review! I understand this is a crutch, but it's the best solution I could come up with.  Appending dummy  NOPs will increase instance LSN by one, which  also looks  ugly. The correct solution is, indeed, to collect  a tx in relay and mangle with it in any means  we need before sending, however, I faced some problems with this approach. See more in v1 of this patchset (letter [PATCH 2/2] replication: make relay send txs in batches). By the way, your txn_limbo implementation assigns the tx an lsn of the last tx row, which doesn't work in case the last row is a local one. So, looks like we either need to reorder the rows or insert a NOP at the tx end. Or just assign the tx an lsn of the last global row. Still, it's up to you which solution you find a better one, and I'll implement it. -- Serge Petrenko