From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 24 Jan 2019 11:53:14 +0300 From: Kirill Yukhin Subject: Re: [PATCH 2/4] Set is_temporary flag for formats of ephemeral spaces Message-ID: <20190124085314.4mx67wr3itudefcb@tarantool.org> References: <648713a318a3ac1181db921b0894881e25cf810c.1548165435.git.kyukhin@tarantool.org> <20190123080001.c7tlwx3npjea3nju@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190123080001.c7tlwx3npjea3nju@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: Hello Vladimir, Thanks for review. My answers are inlined. On 23 Jan 11:00, Vladimir Davydov wrote: > On Tue, Jan 22, 2019 at 05:07:18PM +0300, Kirill Yukhin wrote: > > diff --git a/src/box/space_def.c b/src/box/space_def.c > > index 3516bdd..1d2bfcb 100644 > > --- a/src/box/space_def.c > > +++ b/src/box/space_def.c > > @@ -257,6 +257,19 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > > return def; > > } > > > > +struct space_def* > > +space_def_new_ephemeral(uint32_t field_count) > > +{ > > + struct space_def *space_def = space_def_new(0, 0 , field_count, > > + "ephemeral", > > + strlen("ephemeral"), > > + "memtx", strlen("memtx"), > > + &space_opts_default, > > + &field_def_default, 0); > > + space_def->opts.is_temporary = true; > > Nit: I'd rather you didn't override is_temporary of a newly created > space: > > struct space_opts opts = space_opts_default; > opts.is_temporary = true; > space_def_new(opts); > > > + return space_def; > > +} Fixed. > > /** Free a default value's syntax trees of @a defs. */ > > void > > space_def_destroy_fields(struct field_def *fields, uint32_t field_count, > > diff --git a/src/box/space_def.h b/src/box/space_def.h > > index 8044f88..e8d46c5 100644 > > --- a/src/box/space_def.h > > +++ b/src/box/space_def.h > > @@ -169,6 +169,15 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > > const struct space_opts *opts, const struct field_def *fields, > > uint32_t field_count); > > > > +/** > > + * Create a new ephemeral space. > > Nit: space definition, not space. Added. > > diff --git a/src/box/sql.c b/src/box/sql.c > > index 081a038..387da7b 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -408,11 +408,7 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > > rlist_add_entry(&key_list, ephemer_index_def, link); > > > > struct space_def *ephemer_space_def = > > - space_def_new(0 /* space id */, 0 /* user id */, field_count, > > - "ephemeral", strlen("ephemeral"), > > - "memtx", strlen("memtx"), > > - &space_opts_default, &field_def_default, > > - 0 /* length of field_def */); > > + space_def_new_ephemeral(field_count); > > if (ephemer_space_def == NULL) { > > index_def_delete(ephemer_index_def); > > return NULL; > > This patch should also replace > > space->def->opts.is_temporary = true; > > with the corresponding assertion in space_new_ephemeral(). Done. -- Regards, Kirill Yukhin