From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 E38EC4696C3 for ; Fri, 10 Apr 2020 12:49:26 +0300 (MSK) Date: Fri, 10 Apr 2020 12:49:24 +0300 From: Mergen Imeev Message-ID: <20200410094923.GA6005@tarantool.org> References: <81e5cba334c04467e28948a491bd95d38f0d08ae.1585824116.git.imeevma@gmail.com> <81e5efe8-1627-6294-73bf-77db32e68e5a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <81e5efe8-1627-6294-73bf-77db32e68e5a@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 1/1] sql: do not change order of inserted values List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for review. My answers below. On Mon, Apr 06, 2020 at 11:42:39PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 2 questions below. > > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 27089bd..7c1eba2 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -324,10 +324,17 @@ struct space * > > sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > { > > 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; > > 1. On one hand this can really speed up ephemeral space comparators, > because we have special templates for single indexed unsigned field > if it is in 1, 2, or 3 column. > > On the other hand this won't help, because rowid is the last column > somewhy. And these fast comparators won't help. > > Can we make rowid first column? > I think we can, but that sould be quite confusing since not all ephemeral spaces have rowid. I am not sure that we really need this. > > } > > > > struct region *region = &fiber()->gc; > > @@ -364,17 +371,20 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > } > > } > > > > - for (uint32_t i = 0; i < field_count; ++i) { > > + for (uint32_t i = 0; i < part_count; ++i) { > > struct key_part_def *part = &parts[i]; > > - part->fieldno = i; > > + uint32_t j = i; > > + if (key_info != NULL && key_info->is_pk_rowid) > > + j = field_count - 1; > > 2. Does it make sense to use part count > 1, if we have rowid? > It is unique anyway. True, but not all ephemeral spaces have rowid, so we cannot do this all the time. Still, we can do this in all cases an ephemeral space have rowid. This may affect performance. Should I do it in this patch? Or may I just create an issue on github?