From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 10 Aug 2018 11:26:47 +0300 From: Vladimir Davydov Subject: Re: [RFC PATCH 08/23] vinyl: check key uniqueness before modifying tx write set Message-ID: <20180810082647.gm4v2gbrlzsrf7lm@esperanza> References: <34aef16e5a3dfab28c9d7e9edbd7a66d91279388.1531065648.git.vdavydov.dev@gmail.com> <20180809202616.GB16449@chai> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180809202616.GB16449@chai> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Thu, Aug 09, 2018 at 11:26:16PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [18/07/08 22:52]: > > + if (found != NULL && vy_tuple_compare(stmt, found, > > + lsm->pk->key_def) == 0) { > > + /* > > + * If the old and new tuples are the same in > > + * terms of the primary key definition, the > > + * statement doesn't modify the secondary key > > + * and so there's actually no conflict. > > + */ > > + tuple_unref(found); > > + return 0; > > + } > > In memtx, we pass old_tuple in txn_stmt around so that we can > check that found == old_tuple and ignore the duplicate (please > take a look at replace_check_dup). Why not do the same here, it > would save us a compare? The problem is vinyl doesn't necessarily return the same tuple for lookups by the same key - the resulting tuple is reallocated if it is read from disk or upserts are applied. Besides, the old tuple will be unavailable for REPLACE (once secondary keys are reworked) and for certain UPDATEs (if we make UPDATEs that don't touch secondary keys read-less). That said, we can pass the old tuple, but we still have to fall back on tuple comparison if pointers don't match. > > > + > > + /* > > + * For secondary indexes, uniqueness must be checked on both > > + * INSERT and REPLACE. > > + */ > > + for (uint32_t i = 1; i < space->index_count; i++) { > > + struct vy_lsm *lsm = vy_lsm(space->index[i]); > > + if (vy_check_is_unique_secondary(env, tx, rv, space_name(space), > > + index_name_by_id(space, i), > > + lsm, stmt) != 0) > > + return -1; > > + } > > + return 0; > > This code calls vy_get(), which in turns makes an unnecessary > lookup in the primary key. This is a preparation for new secondary keys - after this patch set is applied, secondary index lookup won't be enough for checking duplicates, because a tuple read from a secondary index may be stale (overwritten in the primary index without DELETE). There's no way to check that other than reading the matching tuple from the primary index. Also, always getting the full tuple is useful for shared cache, which is introduced later in the series. Currently, we store partial tuples in secondary index cache thus wasting memory. Reading the full tuple will allow us to avoid that.