From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 CCB5446970F for ; Thu, 28 Nov 2019 11:10:09 +0300 (MSK) References: <156ce93b495648d6f3fd6c879b0d9aaf56754a1e.1574773773.git.lvasiliev@tarantool.org> <20191126210520.GE23422@atlas> <20191126211701.mhavpytwkemux3vm@tkn_work_nb> <20191127083123.GA2752@atlas> From: Leonid Vasiliev Message-ID: Date: Thu, 28 Nov 2019 11:10:07 +0300 MIME-Version: 1.0 In-Reply-To: <20191127083123.GA2752@atlas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] Move txn from shema to a separate module (use C API instead of FFI) List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , Alexander Turenko , tarantool-patches@dev.tarantool.org On 11/27/19 11:31 AM, Konstantin Osipov wrote: > * Alexander Turenko [19/11/27 00:17]: >>>> https://github.com/tarantool/tarantool/issues/4427 >>> >>>> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-some-stuff-from-ffi-to-c-api >>> >>> Please add a test case. And an explanation of how the fix solves >>> the issue. The patch is an overkill - the trace is going through >>> box_txn_rollback_to_savepoint, so moving it off ffi to C api >>> should be sufficient. >> >> I would add a bit more context here. The original patch was made by >> Sergey O. and I asked for extracting all related functions into its own >> 'module'. See links below. >> >> https://lists.tarantool.org/pipermail/tarantool-patches/2019-September/006747.html >> https://lists.tarantool.org/pipermail/tarantool-patches/2019-September/000734.html >> >> The moving of just one function should be sufficient, you're right. >> However it is not much more work then extracting all those related >> function into tnx.{c,h,lua}. So I think it worth to do just here. >> >> Are you agree that box/lua/txn.* is more proper place for those >> functions then box/lua/schema.lua? > > The move also converts these functions from ffi to plain C. > This is what I am objecting against. > Hi, thanks for your review. 1) About a test case - the bug has been detected on already exist test. 2) The problem was caused by sandwich stack Lua-FFI-> C-API-> Lua (a term of Vyacheslav Egorov). So, no FFI - no problem. We should not use FFI in box_txn_rollback_to_savepoint. 3) Alexandr point: Move all txn stuff from schema to a separate module, IMHO it's a good idea. Why not? 4) About converts others txn functions from FFI to C-API: I think it's a good practice, use one or the other (FFI or C-API) in module