[tarantool-patches] Re: [PATCH v5 4/4] box: introduce multikey indexes in memtx

[Vladimir Davydov]

On Mon, May 06, 2019 at 07:35:10PM +0300, Kirill Shcherbatov wrote:
> > 
> > How's it going to work in case the field map stored in a tuple is
> > greater than field_map_size?
> > 
> > I think we should calculate the real size of the field map here
> > in case the format allows multikey indexes.
> 
> Consider this diff (not on branch yet):
> 
> diff --git a/src/box/field_map.c b/src/box/field_map.c
> index 5d25e3032..a917d4a25 100644
> --- a/src/box/field_map.c
> +++ b/src/box/field_map.c
> @@ -105,12 +105,28 @@ field_map_build(struct field_map_builder *builder, char *buffer)
>  		struct field_map_builder_slot_extent *extent =
>  						builder->slots[i].extent;
>  		/** Retrive memory for the extent. */
> -		uint32_t sz = sizeof(struct field_map_builder_slot_extent) +
> -			      extent->size * sizeof(uint32_t);
>  		field_map[i] =
>  			(uint32_t)((char *)extent_wptr - (char *)field_map);
> -		memcpy(extent_wptr, extent, sz);
> -		extent_wptr += sz;
> +		*(uint32_t *)extent_wptr = extent->size;
> +		uint32_t extent_offset_sz = extent->size * sizeof(uint32_t);
> +		memcpy(&((uint32_t *) extent_wptr)[1], extent->offset,
> +			extent_offset_sz);
> +		extent_wptr += sizeof(uint32_t) + extent_offset_sz;
>  	}
>  	assert(extent_wptr == buffer + builder->extents_size);
>  }
> +
> +uint32_t
> +field_map_get_size(const uint32_t *field_map, uint32_t format_field_map_sz)
> +{
> +	uint32_t total = format_field_map_sz;
> +	int32_t field_map_items = format_field_map_sz / sizeof(uint32_t);
> +	for (int32_t slot = -1; slot >= -field_map_items; slot--) {
> +		if ((int32_t)field_map[slot] >= 0)
> +			continue;
> +		uint32_t *extent = (uint32_t *)((char *)field_map +
> +					(int32_t)field_map[slot]);
> +		total += (1 + extent[0]) * sizeof(uint32_t);
> +	}
> +	return total;
> +}
> diff --git a/src/box/field_map.h b/src/box/field_map.h
> index 0fd35c3e1..78c96e4f7 100644
> --- a/src/box/field_map.h
> +++ b/src/box/field_map.h
> @@ -163,6 +163,19 @@ field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,
>  	return offset;
>  }
>  
> +/**
> + * Get size of the tuple field_map using
> + * field_map pointer and format root field_map size.
> + *
> + * In case of multikey indexes the real field_map size may be
> + * greater than the size of format root field_map. To calculate
> + * the total size of the field_map extentions, routine relies
> + * on the fact that all field_map slots that has an extention
> + * use negative offset as a marker.
> + */
> +uint32_t
> +field_map_get_size(const uint32_t *field_map, uint32_t format_field_map_sz);
> +
>  /**
>   * Initialize field_map_builder.
>   *
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 65e8804a4..ab4913035 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -1180,6 +1180,8 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
>  	char *raw = (char *) tuple + tuple->data_offset;
>  	field_map_build(&builder, raw - field_map_size);
>  	memcpy(raw, data, tuple_len);
> +	assert(field_map_get_size((uint32_t *)raw,
> +				  format->field_map_size) == field_map_size);

Could you please move this assertion to field_map_build so that it works
for all engines, not only for memtx?

Also, we need to make sure that the key_def module doesn't allow to
create multikey key definitions, as its API doesn't allow to compare
multikey indexes. Please do it in the scope of this patch as well.
May be, we will extend the API later to allow that, but I think we
can live without it for now.

>  	say_debug("%s(%zu) = %p", __func__, tuple_len, memtx_tuple);
>  end:
>  	region_truncate(region, region_svp);
> @@ -1192,8 +1194,9 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple)
>  	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
>  	say_debug("%s(%p)", __func__, tuple);
>  	assert(tuple->refs == 0);
> -	size_t total = sizeof(struct memtx_tuple) + format->field_map_size +
> -		tuple->bsize;
> +	const uint32_t *field_map = tuple_field_map(tuple);
> +	size_t total = sizeof(struct memtx_tuple) + tuple->bsize +
> +		       field_map_get_size(field_map, format->field_map_size);
>  	tuple_format_unref(format);
>  	struct memtx_tuple *memtx_tuple =
>  		container_of(tuple, struct memtx_tuple, base);
>