Re: [PATCH v4 4/4] box: introduce functional indexes

[Konstantin Osipov <kostja@tarantool.org>]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/07/24 10:38]:
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -250,6 +250,7 @@ struct errcode_record {
>  	/*195 */_(ER_CREATE_CK_CONSTRAINT,	"Failed to create check constraint '%s': %s") \
>  	/*196 */_(ER_CK_CONSTRAINT_FAILED,	"Check constraint failed '%s': %s") \
>  	/*197 */_(ER_SQL_COLUMN_COUNT,		"Unequal number of entries in row expression: left side has %u, but right side - %u") \
> +	/*198 */_(ER_FUNCTIONAL_INDEX_FUNC_ERROR,"Functional index function '%s' error: %s") \
>  

There is ER_ prefix, no need to add ERROR suffix.

The error code should reference index name and table name, not function name,
or all of this. 

Imagine getting this error for a Lua procedure, it should not
require too many extra steps to nail down the cause.

There should be different error codes for different errors.

Resetting the original error is also a very unfriendly, please add
a ticket and a follow up patch to your todo list for stacked
errors. I mean, it's the last drop, how many times shall we step
on lack of stacked diagnostics before we add them?

> +/**
> + * Update func pointer for functional index key definitions.
> + * @param def Index def, containing key definitions to update.
> + * @param func The functional index function pointer.
> + */
> +static inline void
> +index_def_update_func(struct index_def *def, struct func *func)
> +{
> +	def->key_def->func_index_func = func;
> +	def->cmp_def->func_index_func = func;
> +}

should be set_func, not update_func.

>  	uint32_t unique_part_count;
> +	/**
> +	 * Count of parts in functional index defintion.
> +	 * All functional_part_count key_part(s) of an
> +	 * initialized key def instance have func != NULL pointer.
> +	 * != 0 iff it is functional index definition.
> +	*/
> +	uint32_t functional_part_count;

I don't get this comment, we don't store func pointer in key_part.
Why do you need this at all? part_count should be enough.

> +	/** A pointer to functional index function. */
> +	struct func *func_index_func;

Please explain the life cycle of this member (it is not
immediately initialized, when is it assigned then?, when is it
safe to rely on it?) and how it is
used.
> +	/** Space id of _func_index. */
> +	BOX_FUNC_INDEX_ID = 372,

Please explain how this space is used. Please document why we
decided to have a separate space.

> +	*key = it->data;
> +	if (!it->validate) {

    Please document circumstances when validate is false.

> +	if (mp_typeof(*it->data) != MP_ARRAY) {
> +		diag_set(ClientError, ER_FUNCTIONAL_INDEX_FUNC_ERROR,
> +	if (part_count != functional_part_count) {
> +		const char *error_msg =
> +		diag_set(ClientError, ER_FUNCTIONAL_INDEX_FUNC_ERROR,
> +	if (key_validate_parts(it->key_def, rptr, functional_part_count, true,
> +			       &key_end) != 0) {
> +		diag_set(ClientError, ER_FUNCTIONAL_INDEX_FUNC_ERROR,

These three places  should use its own error code, not
ER_FUNC_INDEX_FUNC, ER_FUNC_INDEX_FUNC_FORMAT I guess.

> @@ -251,8 +253,15 @@ index_def_to_key_def(struct rlist *index_defs, int *size)
>  {
>  	int key_count = 0;
>  	struct index_def *index_def;
> -	rlist_foreach_entry(index_def, index_defs, link)
> +	rlist_foreach_entry(index_def, index_defs, link) {
> +		/*
> +		 * Don't use functional index key definition
> +		 * to build a space format.
> +		 */
> +		if (key_def_is_functional(index_def->key_def))
> +			continue;
>  		key_count++;

Why not skip such keys when building a space format? What is the
benefit of depriving the format module of these key defs?

> @@ -262,8 +271,11 @@ index_def_to_key_def(struct rlist *index_defs, int *size)
>  	}
>  	*size = key_count;
>  	key_count = 0;
> -	rlist_foreach_entry(index_def, index_defs, link)
> +	rlist_foreach_entry(index_def, index_defs, link) {
> +		if (key_def_is_functional(index_def->key_def))
> +			continue;
>  		keys[key_count++] = index_def->key_def;
> +	}
>  	return keys;
Same here. I'm sure space format module can figure out that it
deals with a functional key definition and skip it if necessary.

> +	if (index_def->iid == 0 && key_def_is_functional(index_def->key_def)) {
> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
> +			space_name, "primary key cannot be functional");

primary key can not use a function

>  struct key_def *
> -key_def_new(const struct key_part_def *parts, uint32_t part_count)
> +key_def_new(const struct key_part_def *parts, uint32_t part_count,
> +	    bool is_functional)

I don't like it that key_def_new has become overly complicated
with collations, multikey and functional indexes. Perhaps we need
to introduce key_def_is_valid, similar to index_def_is_valid(),
but I need to think.
>  key_def_can_merge(const struct key_def *key_def,
>  		  const struct key_part *to_merge)
>  {
> +	if (key_def_is_functional(key_def))
> +		return true;

Please explain what's going on here in a comment. 
>  struct key_def *
>  key_def_merge(const struct key_def *first, const struct key_def *second)
>  {
> +	assert(!key_def_is_functional(second));

Same here.

> -	struct key_def *key_def = key_def_new(parts, part_count);
> +	struct key_def *key_def = key_def_new(parts, part_count, false);

I don't get this, there should be two flags (is_multikey,
is_functional). How did this compile?

>  
> +		if (index_opts->functional_fid > 0) {
> +			lua_pushnumber(L, index_opts->functional_fid);
> +			lua_setfield(L, -2, "func_id");
> +		}

Why not function name?

> +template<bool is_nullable>
> +static inline int
> +functional_compare(struct tuple *tuple_a, hint_t tuple_a_hint,
> +		   struct tuple *tuple_b, hint_t tuple_b_hint,
> +		   struct key_def *key_def)
> +{

    Please solicit Vlad's review and explain the changes here in a
    changeset comment.

> +static char *
> +tuple_extract_key_stub(struct tuple *tuple, struct key_def *key_def,
> +			     int multikey_idx, uint32_t *key_size)
> +{
> +	(void)tuple; (void)key_def; (void)multikey_idx; (void)key_size;
> +	unreachable();
> +	return NULL;
> +}
> +
> +static char *
> +tuple_extract_key_raw_stub(const char *data, const char *data_end,
> +			   struct key_def *key_def, int multikey_idx,
> +			   uint32_t *key_size)
> +{
> +	(void)data; (void)data_end;
> +	(void)key_def; (void)multikey_idx; (void)key_size;
> +	unreachable();
> +	return NULL;
> +}

Why do you need these?


-- 
Konstantin Osipov, Moscow, Russia