From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <imeevma@tarantool.org>
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 <tarantool-patches@dev.tarantool.org>;
 Fri, 10 Apr 2020 12:49:26 +0300 (MSK)
Date: Fri, 10 Apr 2020 12:49:24 +0300
From: Mergen Imeev <imeevma@tarantool.org>
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 <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
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?