Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts()
Date: Fri, 9 Oct 2020 12:33:35 +0300	[thread overview]
Message-ID: <20201009093335.62g5qcowisd746gr@tkn_work_nb> (raw)
In-Reply-To: <61c50889-7675-0586-0229-8f079814177e@tarantool.org>

> > The function dumps an opacue <box_key_def_t> structure into a non-opacue
> 
> 1. opacue -> opaque.

Will fix in the next version of the patchset.

> > array of <box_key_part_def_t> structures in order to allow an external
> > module to obtain information about the key definition.
> 
> 2. Why would a module need it, if it already had all the parameters used
> to create this key def?

When __serialize metamethod is called on cdata<struct key_def_key_def *>
(it is a pointer to <struct key_def>, see [^1]), we only have the
pointer to the opaque structure. Of course, we can create our own
structure around <struct key_def> and duplicate all information there,
but it would be...  I don't know... wasteful.

[^1]: Side note regarding <struct key_def_key_def> name:

       | Modification of cdata metatype is not allowed in LuaJIT, so we
       | should use another structure name for Lua (not
       | <struct key_def>). The reason is that built-in module is
       | already loaded and already calls its ffi.metatype() on
       | <struct key_def>.

      https://github.com/Totktonada/key_def/commit/645c8902c8242f0d1e5168b207676af6f8824b70

      It is key_def prefixed by the module name ('key_def'). May be
      changed after the module renaming to tuple-keydef.

Presence of box_key_def_dump_parts() also opens abitility to do a
runtime test, whether a particular feature is supported on given
tarantool version: say, we can set a JSON path and look whether it is
present in the dumped key parts. That's also why I always set all
unknown fields and flags to zero in box_key_part_def_create().

I think, this approach is good, because it works by construction: we
don't need to introduce a feature test function for any new key_def
feature: I guess it may be forgotten.

That's how runtime check whether JSON path is supported is performed in
the external module:

 | /**
 |  * Runtime check whether JSON path is supported.
 |  */
 | static bool
 | json_path_is_supported(void)
 | {
 |         bool res = false;
 |
 |         /* Create a key_def with JSON path. */
 |         box_key_part_def_t part;
 |         box_key_part_def_create(&part);
 |         part.fieldno = 0;
 |         part.field_type = "unsigned";
 |         JSON_PATH_SET(&part, "[1]");
 |         box_key_def_t *key_def = box_key_def_new_ex(&part, 1);
 |
 |         /* Dump parts and look whether JSON path is dumped. */
 |         size_t region_svp = box_region_used();
 |         uint32_t part_count = 0;
 |         box_key_part_def_t *parts = box_key_def_dump_parts(key_def,
 |                                                            &part_count);
 |         assert(parts != NULL);
 |         res = JSON_PATH(&parts[0]) != NULL;
 |         box_region_truncate(region_svp);
 |
 |         /* Delete the key_def. */
 |         box_key_def_delete(key_def);
 |
 |         return res;
 | }

https://github.com/Totktonada/key_def/commit/73dae9d26ad887ca38872a40962b958c2e4bbd84

This, however, was not the main purpose of the function, it is more like
the side effect. The main idea was to don't duplicate information from
<struct key_def> in a module level wrapping structure.

> > +box_key_part_def_t *
> > +box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
> > +{
> > +	struct region *region = &fiber()->gc;
> > +	size_t region_svp = region_used(region);
> > +	size_t size;
> > +	box_key_part_def_t *parts = region_alloc_array(
> > +		region, typeof(parts[0]), key_def->part_count, &size);
> > +	if (parts == NULL) {
> > +		diag_set(OutOfMemory, size, "region_alloc_array", "parts");
> > +		return NULL;
> > +	}
> > +
> > +	for (uint32_t i = 0; i < key_def->part_count; i++) {
> > +		const struct key_part *part = &key_def->parts[i];
> > +		box_key_part_def_t *part_def = &parts[i];
> > +		box_key_part_def_create(part_def);
> > +
> > +		/* Set part->{fieldno,flags,field_type}. */
> > +		part_def->fieldno = part->fieldno;
> > +		part_def->flags = 0;
> > +		if (key_part_is_nullable(part))
> > +			part_def->flags |= BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
> > +		assert(part->type >= 0 && part->type < field_type_MAX);
> > +		part_def->field_type = field_type_strs[part->type];
> > +
> > +		/* Set part->collation. */
> > +		if (part->coll_id != COLL_NONE) {
> > +			struct coll_id *coll_id = coll_by_id(part->coll_id);
> > +			/*
> > +			 * A collation may be removed after
> > +			 * key_def creation.
> > +			 */
> > +			if (coll_id == NULL) {
> > +				diag_set(CollationError,
> > +					 "key_def holds dead collation id %d",
> > +					 part->coll_id);
> > +				region_truncate(region, region_svp);
> > +				return NULL;
> > +			}
> > +			part_def->collation = coll_id->name;
> 
> 3. What if coll_id is removed after return from this function? For
> example, by deleting a tuple from _collation. The pointer would
> be dead. We need to copy the name.

Sure. I'll do.

  reply	other threads:[~2020-10-09  9:33 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23  1:14 [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 01/14] module api: get rid of typedef redefinitions Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 02/14] WIP: module api: expose box region Alexander Turenko
2020-09-24 22:31   ` Vladislav Shpilevoy
2020-10-08 19:21     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 03/14] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko
2020-09-24 22:32   ` Vladislav Shpilevoy
2020-10-08 21:46     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 04/14] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 05/14] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko
2020-09-24 22:32   ` Vladislav Shpilevoy
2020-10-12 19:06     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 06/14] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 07/14] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 08/14] WIP: refactoring: extract key_def module API functions Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-07 11:30     ` Alexander Turenko
2020-10-07 22:12       ` Vladislav Shpilevoy
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09 21:54     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09  9:33     ` Alexander Turenko [this message]
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 11/14] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 12/14] WIP: module api: expose box_key_def_merge() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09  1:46     ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko
2020-09-25 22:58   ` Vladislav Shpilevoy
2020-10-09  1:14     ` Alexander Turenko
2020-10-10  1:21       ` Alexander Turenko
2020-09-23  1:14 ` [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() Alexander Turenko
2020-09-25 22:59   ` Vladislav Shpilevoy
2020-10-09  1:22     ` Alexander Turenko
2020-09-23  1:40 ` [Tarantool-patches] [PATCH 1.10 00/16] RFC: module api: extend for external key_def Lua module Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 01/16] collation: allow to find a collation by a name Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 02/16] refactoring: adjust contract of luaT_tuple_new() Alexander Turenko
2020-09-28 21:26     ` Vladislav Shpilevoy
2020-10-05 11:58       ` Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 03/16] module api: get rid of typedef redefinitions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 04/16] WIP: module api: expose box region Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 05/16] WIP: module api/lua: add luaL_iscdata() function Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 06/16] WIP: module api/lua: expose luaT_tuple_new() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 07/16] WIP: module api/lua: add luaT_tuple_encode() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 08/16] WIP: refactoring: add API_EXPORT to lua/tuple functions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 09/16] WIP: refactoring: add API_EXPORT to key_def functions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 10/16] WIP: refactoring: extract key_def module API functions Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 11/16] WIP: module api: add box_key_def_new_ex() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 12/16] WIP: module api: add box_key_def_dump_parts() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 13/16] WIP: module api: expose box_tuple_validate_key_parts() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 14/16] WIP: module api: expose box_key_def_merge() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 15/16] WIP: module api: expose box_tuple_extract_key_ex() Alexander Turenko
2020-09-23  1:40   ` [Tarantool-patches] [PATCH 1.10 16/16] WIP: module api: add box_key_def_validate_key() Alexander Turenko
2020-10-05  7:26 ` [Tarantool-patches] [PATCH 00/14] RFC: module api: extend for external key_def Lua module Alexander Turenko

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=20201009093335.62g5qcowisd746gr@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts()' \
    /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