[tarantool-patches] Re: [PATCH 2/3] vinyl: move unique check optimization setup to the space level

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev at gmail.com> [19/07/23 18:49]:
> If any sub-set of fields indexed by a unique index is indexed by another
> unique index, we can skip the uniqueness check for it. We use this to
> optimize out unnecessary uniqueness checks in vinyl, where they may be
> pretty costly, especially if the bloom filter doesn't reflect them.
> 
> Currently, whether to check if an index is unique or not is determined
> in vinyl space constructor, which sets vy_lsm::check_is_unique flag for
> the space indexes. This looks ugly, because this means that no other
> engine can make use of this optimization without duplicating the code
> setting up the flags. True, no other engine needs it now, but still it
> doesn't feel right. Besides, the check_is_unique flag isn't actually an
> index property - it's rather a property of a space so storing it in
> vy_lsm looks wrong. Because of that we have to update the flag when an
> index is reassigned to another space by MoveIndex DDL operation, see
> vinyl_space_swap_index().
> 
> So let's store the flags indicating whether a uniqueness check is
> required for a particular index in a bitmap in the space struct and
> set it up in the generic space constructor, space_create().

LGTM
> +/** A trivial wrapper around space_build_index(). */
> +static void
> +alter_space_build_index(struct space *src_space, struct space *new_space,
> +			struct index *new_index)
> +{
> +	bool check_unique = space_needs_check_unique(new_space,
> +						     new_index->def->iid);

check_unique is not proper English, please use
check_unique_constraint or check_unique_ck.

> +	space_build_index_xc(src_space, new_index, new_space->format,
> +			     check_unique);

Why can't you move testing check_unique property inside space_build_index
or even the engine specific implementation of space_build_index?

> +	space->unique_index_bitmap = calloc(bitmap_size(index_id_max + 1), 1);

Let's use check_unique_constraint_map
> +/**
> + * Return true if the unique constraint must be checked for
> + * the index with the given id before inserting a tuple into
> + * the space.
> + */
> +static inline bool
> +space_needs_check_unique(struct space *space, uint32_t index_id)

space_needs_unique_constraint_check

-- 
Konstantin Osipov, Moscow, Russia