From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 4 Dec 2018 18:51:51 +0300 From: Vladimir Davydov Subject: Re: [PATCH v5 8/9] box: introduce offset slot cache in key_part Message-ID: <20181204155151.mfkugyfwukvdkcrs@esperanza> References: <20181203210418.bpjwulkewjnoyqoh@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181203210418.bpjwulkewjnoyqoh@esperanza> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org, kostja@tarantool.org List-ID: On Tue, Dec 04, 2018 at 12:04:18AM +0300, Vladimir Davydov wrote: > On Mon, Nov 26, 2018 at 01:49:42PM +0300, Kirill Shcherbatov wrote: > > diff --git a/src/box/alter.cc b/src/box/alter.cc > > index 029da02..6291159 100644 > > --- a/src/box/alter.cc > > +++ b/src/box/alter.cc > > @@ -856,7 +856,10 @@ alter_space_do(struct txn *txn, struct alter_space *alter) > > * Create a new (empty) space for the new definition. > > * Sic: the triggers are not moved over yet. > > */ > > - alter->new_space = space_new_xc(alter->space_def, &alter->key_list); > > + alter->new_space = > > + space_new_xc(alter->space_def, &alter->key_list, > > + alter->old_space->format != NULL ? > > + alter->old_space->format->epoch + 1 : 1); > > Passing the epoch around looks ugly. Let's introduce a global counter > and bump it right in tuple_format_new(). If you worry about disk_format > and mem_format having different epochs in vinyl, it's not really a > problem: make epoch an optional argument to tuple_format_new(); if the > caller passes an epoch, use it, otherwise generate a new one; in vinyl > reuse mem_format->epoch for disk_format. > > > diff --git a/src/box/key_def.h b/src/box/key_def.h > > index 7731e48..3e08eb4 100644 > > --- a/src/box/key_def.h > > +++ b/src/box/key_def.h > > @@ -95,6 +95,14 @@ struct key_part { > > char *path; > > /** The length of JSON path. */ > > uint32_t path_len; > > + /** > > + * Source format for offset_slot_cache hit validations. > > + * Cache is expected to use "the format with the newest > > + * epoch is most relevant" strategy. > > + */ > > + struct tuple_format *format_cache; > > Why did you decide to store tuple_format in key_part instead of epoch, > as you did originally? I liked that more. Discussed verbally, agreed that: - Storing a pointer to the format in key_part is potentially dangerous, because pointers may coincide after reallocation, resulting in invalid behavior. - We should make the epoch counter global rather than passing it around on space creation. That is, tuple_format_new() will bump a global int64_t counter and assign the new value to the new format. - In vinyl disk_format and mem_format must have different epochs, because they have different field maps so no optional argument argument is required for tuple_format_new() - it will always assign a new epoch to the new format. - 'Epoch' isn't a very good name. We should probably call it 'version' or 'format_version' or 'tuple_format_version', depending on the context.