From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 1B7496EC40; Mon, 16 Aug 2021 11:57:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1B7496EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629104278; bh=YD1lJZysuDlsuxHPibzaxNYB08PIiH71Nsf+/IuV4wo=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=fCYJGnkcpwyvuIXhFOOMAMC9P/xH9ciKyqYOU82QGN0jCnszT9Vc3nMqdMDSKsXxO t7qAtB0xO6MhZuoqfswxIivtfmIhf29FwBqN9a6iFIxASh6ATEq+hDPn7xGDdqJFwY p0tRYDXlzzxVmqLSm4gJr34cHTcJl9Vw/18B1F7Y= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [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 37B1B6EC40 for ; Mon, 16 Aug 2021 11:57:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 37B1B6EC40 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mFYRP-0001fT-84; Mon, 16 Aug 2021 11:57:55 +0300 Date: Mon, 16 Aug 2021 11:57:54 +0300 To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org, Mergen Imeev Message-ID: <20210816085754.tq3xe4gvlhnxxkns@esperanza> References: <20eb7c0115c8b3c90386da95a0ddf64dccbd6b0f.1628824097.git.imeevma@gmail.com> <20210813151229.dish2wtuyu32zc72@esperanza> <20210815132603.GA87243@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210815132603.GA87243@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9BCE6B93DE0C6C3914462CDB1732D383C182A05F538085040A8BBA8416CAB036DC4250884454B00A23931B82B2B271BB8B810A8132BF6DB60 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70F8483684B69468CB287FD4696A6DC2FA8DF7F3B2552694A4E2F5AFA99E116B42401471946AA11AF1661749BA6B9773522EA870BBF4AACFC8F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB553375665B73F950BC6E7FFBADC6ABA2C801C7018288968A5C299DF9A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE7328B01A8D746D8839FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3ED8438A78DFE0A9E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637B8F435DEDE9E76EBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5058C4EB07B2BE0EFD3C3E9997269EF7A37 X-C1DE0DAB: 0D63561A33F958A5873A3E74D1C1A78306C72C7AB7EF4012B4BC94F763535851D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FD9114F69CA58D1EE80138BB989FCB9EF850E68DF93F696ACFC3C381F38F381A957BF45C42CDE4661D7E09C32AA3244C06917DEA30D2943201F0CD99408AAC947101BF96129E4011927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojIrFL/N5KnVEGb/lRUMhdJA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D2969070F3E3184A248024D91926C9A52274CEFED1673C562683ABF942079399BFB559BB5D741EB966A65DFF43FF7BE03240331F90058701C67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: define ephemeral space fields X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladimir Davydov via Tarantool-patches Reply-To: Vladimir Davydov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Sun, Aug 15, 2021 at 04:26:03PM +0300, Mergen Imeev wrote: > On Fri, Aug 13, 2021 at 06:12:29PM +0300, Vladimir Davydov wrote: > > On Fri, Aug 13, 2021 at 06:09:19AM +0300, imeevma@tarantool.org wrote: > > > diff --git a/src/box/sql.c b/src/box/sql.c > > > index c805a1e5c..2b95aed8d 100644 > > > --- a/src/box/sql.c > > > +++ b/src/box/sql.c > > > @@ -243,114 +243,163 @@ tarantoolsqlCount(struct BtCursor *pCur) > > > return index_count(pCur->index, pCur->iter_type, NULL, 0); > > > } > > > > > > -struct space * > > > -sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > > +struct space_info * > > > +sql_space_info_new(uint32_t field_count, uint32_t part_count) > > > { > > > - struct key_def *def = NULL; > > > - uint32_t part_count = field_count; > > > - if (key_info != NULL) { > > > - def = sql_key_info_to_key_def(key_info); > > > - if (def == NULL) > > > - return NULL; > > > - /* > > > - * In case is_pk_rowid is true we can use rowid > > > - * as the only part of the key. > > > - */ > > > - if (key_info->is_pk_rowid) > > > - part_count = 1; > > > + assert(field_count > 0); > > > + uint32_t info_size = sizeof(struct space_info); > > > + uint32_t field_size = field_count * sizeof(enum field_type); > > > + uint32_t colls_size = field_count * sizeof(uint32_t); > > > + uint32_t parts_size = part_count * sizeof(uint32_t); > > > + uint32_t size = info_size + field_size + colls_size + parts_size; > > > + > > > + struct space_info *info = sqlDbMallocRawNN(sql_get(), size); > > > + if (info == NULL) { > > > + diag_set(OutOfMemory, size, "sqlDbMallocRawNN", "info"); > > > + return NULL; > > > } > > > + info->types = (enum field_type *)((char *)info + info_size); > > > + info->coll_ids = (uint32_t *)((char *)info->types + field_size); > > > + info->parts = part_count == 0 ? NULL : > > > + (uint32_t *)((char *)info->coll_ids + colls_size); > > > + info->field_count = field_count; > > > + info->part_count = part_count; > > > + info->sort_order = SORT_ORDER_ASC; > > > > > > - struct region *region = &fiber()->gc; > > > + for (uint32_t i = 0; i < field_count; ++i) { > > > + info->types[i] = FIELD_TYPE_SCALAR; > > > + info->coll_ids[i] = COLL_NONE; > > > + } > > > + for (uint32_t i = 0; i < part_count; ++i) > > > + info->parts[i] = i; > > > + return info; > > > +} > > > + > > > +void > > > +sql_space_info_from_space_def(struct space_info *info, > > > + const struct space_def *def) > > > > Separating allocation from initialization only complicates the protocol: > > - The caller must ensure that the arguments passed to alloc and init > > are compatible. > > - There's a window between alloc and init when the object is unusable. > > > > Please don't: > > > > struct sql_space_info * > > sql_space_info_new_from_space_def(...); > > > > struct sql_space_info * > > sql_space_info_new_from_index_def(...); > > > Fixed. There is still some places that cannot be fixed for now, since > sql_space_info may be modified during execution of some code. Please also rename sql_space_info_from_id_list to sql_space_info_new_from_id_list. > > struct space * > > sql_ephemeral_space_new(const struct sql_space_info *info); > > > I would like to do this, but we already have a function with such name. More > than that, this functions actually have no connection to ephemeral spaces, > the "spaces" it creates are only needed for creation of normal spaces, i.e. > it actually creates analogues of space_def. Not sure why space_def cannot be > used there. Then you should give those old functions adequate names. Please rename them in a separate patch and rebase. > > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > > > index b9107fccc..a3e551017 100644 > > > --- a/src/box/sql/select.c > > > +++ b/src/box/sql/select.c > > > @@ -5879,6 +6047,23 @@ sqlSelect(Parse * pParse, /* The parser context */ > > > sSort.reg_eph, > > > nCols, > > > 0, (char *)key_info, P4_KEYINFO); > > > + /* > > > + * We also should define space_info in case this opcode will not > > > + * be converted to SorterOpen. > > > + */ > > > + uint32_t part_count = sSort.pOrderBy->nExpr + 1; > > > + struct space_info *info = sql_space_info_new(nCols, part_count); > > > + if (info == NULL) { > > > + pParse->is_aborted = true; > > > + goto select_end; > > > + } > > > + key_info->info = info; > > > + if (space_info_for_sorting(info, pParse, sSort.pOrderBy, > > > + pEList) != 0) { > > > + pParse->is_aborted = true; > > > + goto select_end; > > > + } > > > + > > > > I don't understand why you can't pass sql_space_info instead of > > sql_key_info. AFAIU, you don't need key_info here at all anymore. > > Anyway, keeping a pointer to space_info in key_info looks bad. > > Can you avoid that? > > > The problem here is that OpenTEphemeral opcode can be replaced by SorterOpen > opcode. Also, its key_info could be moved to OP_Compare opcode. Actually, I > made a patch which also replaces sql_key_info by space_info for these two > opcodes, but space_info become too complicated. I decided to leave it for now. > I actually think that we should drop sorter and OP_Compare, but not sure that > we should do it now, before release. I will think of it a bit later, if I will > be given some time to rework this. I think you should convert space_info to key_info when changing the opcode then.