From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 0B6F0469719 for ; Thu, 5 Nov 2020 01:14:40 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <46f1c499-b84b-7c4a-d8a1-189d3c5965d9@tarantool.org> Date: Wed, 4 Nov 2020 23:14:38 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org, sergos@tarantool.org Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 2 comments below. On 02.11.2020 10:49, imeevma@tarantool.org wrote: > Prior to this patch, region on fiber was reset during select(), get(), > count(), max(), or min(). This would result in an error if one of these > operations was used in a user-defined function in SQL. After this patch, > these functions truncate region instead of resetting it. > > Closes #5427 > --- > https://github.com/tarantool/tarantool/issues/5427 > https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result > > @ChangeLog > - Region truncated instead of resetting on select() (gh-5427). 1. I suggest to make the message more understandable for users. They have no idea what a region is. For instance, consider Memory corruption when SQL called Lua functions with box calls inside > src/box/box.cc | 5 +- > src/box/index.cc | 20 +++ > src/box/txn.h | 6 +- > .../gh-5427-lua-func-changes-result.result | 153 ++++++++++++++++++ > .../gh-5427-lua-func-changes-result.test.lua | 86 ++++++++++ > 5 files changed, 264 insertions(+), 6 deletions(-) > create mode 100644 test/sql/gh-5427-lua-func-changes-result.result > create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua > > diff --git a/src/box/box.cc b/src/box/box.cc > index 18568df3b..473ef52ad 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1388,7 +1388,8 @@ box_select(uint32_t space_id, uint32_t index_id, > struct port *port) > { > (void)key_end; > - > + struct region *region = &fiber()->gc; > + size_t used = region_used(region); > rmean_collect(rmean_box, IPROTO_SELECT, 1); 2. I must say I couldn't come up with a better idea so far. I remember I also proposed to start a transaction for each VDBE statement, but not sure it is a right thing to do. It means, we probably should go for this one, but a bit polished. In the current solution I don't like, that you basically just inlined txn_commit_ro_stmt(), but instead I would rather want you to patch it to make it work. So for example txn_begin_ro_stmt inside remembers the current region size, and returns it via an out parameter struct txn_ro_savepoint *svp. Which you create on the stack. Then, after you are done with the statement, you call txn_commit_ro_stmt, which takes const struct txn_ro_savepoint *svp, and does the region_truncate inside.