From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C160324886 for ; Wed, 10 Apr 2019 06:11:52 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Up6cTwR35ikg for ; Wed, 10 Apr 2019 06:11:52 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 11339227FA for ; Wed, 10 Apr 2019 06:11:51 -0400 (EDT) Date: Wed, 10 Apr 2019 13:11:48 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 1/2] Introduce a txn memory region Message-ID: <20190410101148.GL8268@chai> References: <75597e590276ca833b38aec0bee1f008f081271d.1554880565.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <75597e590276ca833b38aec0bee1f008f081271d.1554880565.git.georgy@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Georgy Kirichenko * Georgy Kirichenko [19/04/10 10:26]: > Attach a separate memory region for each txn structure in order to store > all txn internal data until the transaction finished. This patch is a > preparation to detach a txn from a fiber and a fiber gc storage. The patch is OK as the first patch on the stack, I hope most of the allocations will begin using txn region in subsequent patches. A few minor comments: - let's use a single linked list, unless there is a good reason to use a double linked list - let's preserve THRASH, or maybe there is already thrash in region_free? - it's good we use sizeof(txn) in region_truncate(), but let's assert that region_alloc() does use sizeof(txn) and doesn't align it in any way. - I would not add txn_new/txn_free as separate methods, they are not useful on their own - I thought we already have ER_NO_ACTIVE_TRANSACTION error, but found we don't. We have ER_SAVEPOINT_NO_TRANSACTION though. Perhaps we could make it more generic? Well, your choice is good enough, but then again, maybe make *it* generic? E.g., could we perhaps make the message like this: Can not %s when there is no active transaction. We could even rename ER_SAVEPOINT_NO_TRANSACTION, change the error message to one with a placeholder and reuse it. What do you think? -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov