From: Mergen Imeev <imeevma@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 1/1] sql: do not change order of inserted values Date: Fri, 10 Apr 2020 12:49:24 +0300 [thread overview] Message-ID: <20200410094923.GA6005@tarantool.org> (raw) In-Reply-To: <81e5efe8-1627-6294-73bf-77db32e68e5a@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?
next prev parent reply other threads:[~2020-04-10 9:49 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-02 10:45 imeevma 2020-04-06 21:42 ` Vladislav Shpilevoy 2020-04-10 9:49 ` Mergen Imeev [this message] 2020-04-11 14:34 ` Vladislav Shpilevoy 2020-04-12 10:21 ` Imeev Mergen 2020-04-12 14:02 ` Vladislav Shpilevoy 2020-04-12 15:21 ` Imeev Mergen 2020-04-12 16:00 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200410094923.GA6005@tarantool.org \ --to=imeevma@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 1/1] sql: do not change order of inserted values' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox