Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [tarantool-patches] Re: [PATCH v5 4/4] box: introduce multikey indexes in memtx
Date: Tue, 7 May 2019 11:11:03 +0300	[thread overview]
Message-ID: <20190507081103.fnzqguaeev4hxrlt@esperanza> (raw)
In-Reply-To: <88b86a7c-686d-30ac-fe75-f892381e96a4@tarantool.org>

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);
> 

  reply	other threads:[~2019-05-07  8:11 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-06 11:57 [PATCH v5 0/4] " Kirill Shcherbatov
2019-05-06 11:57 ` [PATCH v5 1/4] box: introduce tuple_format_iterator class Kirill Shcherbatov
2019-05-06 13:55   ` Vladimir Davydov
2019-05-06 14:32     ` [tarantool-patches] " Kirill Shcherbatov
2019-05-06 11:57 ` [PATCH v5 2/4] box: introduce field_map_builder class Kirill Shcherbatov
2019-05-06 14:22   ` Vladimir Davydov
2019-05-06 11:57 ` [PATCH v5 3/4] salad: introduce bps_tree_delete_identical routine Kirill Shcherbatov
2019-05-06 14:34   ` Vladimir Davydov
2019-05-06 14:55     ` [tarantool-patches] " Kirill Shcherbatov
2019-05-06 11:57 ` [PATCH v5 4/4] box: introduce multikey indexes in memtx Kirill Shcherbatov
2019-05-06 15:46   ` Vladimir Davydov
2019-05-06 16:35     ` [tarantool-patches] " Kirill Shcherbatov
2019-05-07  8:11       ` Vladimir Davydov [this message]
2019-05-07  8:28         ` Kirill Shcherbatov
2019-05-07 11:30           ` Vladimir Davydov
2019-05-07 13:13     ` Vladimir Davydov
2019-05-06 15:52 ` [PATCH v5 0/4] " Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190507081103.fnzqguaeev4hxrlt@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] Re: [PATCH v5 4/4] box: introduce multikey indexes in memtx' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox