[Tarantool-patches] [PATCH 09/14] WIP: module api: add box_key_def_new_ex()

[Alexander Turenko]

> 1. Why _ex()? Why not 2? What are we going to do when will need a new
> parameter? _ex_ex()? _ex2()? With a number at least we could just bump
> it.

_ex() and _v2() have certain meaning (extended, updated version), but
_2() may be interpreted differently: two parameters (like wait3() /
wait4()), second version (update of the first one), another variant (not
update, but just the same thing with some other properties).

'How to name the next version when we'll need to bump it' is the good
question. _ex() does not give us good advice. So it seems we should
prefer _v2() in the module API.

I'll change it.

> 
> On 23.09.2020 03:14, Alexander Turenko wrote:
> > Unlike box_key_def_new() it allows to set nullability, collation and
> > JSON path.
> > 
> > Note: JSON paths are not supported in the backported version of the
> > patch for 1.10.
> > 
> > Provided public non-opaque key part definition structure to create a key
> > definition. The next commit will also use this structure to dump a key
> > definition.
> 
> 2. Did you think of making a function which would create a key_def taking
> only part_count, and then exporting functions like
> 
>     box_key_def_set_part_type(...);
>     box_key_def_set_part_json_path(...);
>     box_key_def_set_part_collation(...);
>     etc.
> 
> to customize it after creation? Then we wouldn't need to export
> box_key_part_def_t and care about ABI shit.

Of course. I would even say that this variant have no strict blockers
considering my use case. However I would want to avoid the situation,
when an API design lead to performance restrictions in areas, where it
can be bypassed. And also I think that an opaque structure pointer + set
of accessor functions just to access fields is not how usual C API is
designed. Example: getaddrinfo() and <struct addrinfo>.

Sure, an ABI needs care, but I hope that when we have defined
modification rules, which should guarantee ABI compatibility, further
maintanence should not be hard.

If you don't have strict objections, I would go this way.

> 
> > There are several techinal points around the box_key_part_def_t
> 
> 3. techinal -> techincal.

Will fix.

> 
> > structure. They are mainly about providing stable ABI.
> > 
> > - Two uint32_t fields are placed first for better aligning of next
> >   fields (pointers, which are usually 64 bit wide).
> > 
> > - A padding is used to guarantee that the structure size will remain the
> >   same across tarantool versions. It allows to allocate an array of such
> >   structures.
> > 
> > - The padding array is not a field of the structure itself, but added as
> >   a union variant (see the code). It allows to get rid of manual
> >   calculation of cumulative fields size, which is hard to do in a
> >   platform independent way.
> > 
> > - A minimal size of the structure is guaranteed by the union with
> >   padding, but a static assert is required to guarantee that the size
> >   will not overrun the predefined value.
> > 
> > - PACKED is added as an extra remedy to make the structure layout
> >   predictable.
> 
> 4. Is pointer size predictable?

I'll add 'on given target architecture' here in the next patchset
version.

> 
> > - A bit flag is used for is_nullable. bool is considered as too
> >   expensive (it requires 8 bits). bitfields (int:1 and so on) do no
> >   guarantee certain data layout (it is compiler implementation detail),
> >   while a module is compiled outside of tarantool build and may use
> >   different toolchain. A bit flag is the only choice.
> > 
> > - A collation is identified using a string. Different IDs may be used on
> >   different tarantool instances for collations. The only 'real'
> >   identifier is a collation name, so using it as identifier in the API
> >   should be more convenient and less error-prone.
> > 
> > - A field type is also identified using a string instead of a number. We
> >   have <enum field_type> in the module API, but it cannot be used here,
> >   because IDs are changed across tarantool versions. Aside of this, size
> >   of a enum is compiler defined. Anyway, we can expose field types as
> >   numbers and implement number-to-name and name-to-number mapping
> >   functions, but IMHO it would just add extra complexity.
> 
> 5. For record - I still don't like the strings. Because it is not
> obvious what to specify here. There is no a set of values to use, and it
> makes the creation slower.
> 
> It simplifies API, but not module.h. It simplifies the Lua module you
> built on top of this. For pure C modules and for the module.h itself it
> complicates things.
> 
> But I do not like it not enough to insist.

(Just hold my thoughts.)

It looks very like 'natural key' vs 'surrogate key' debate. There are
cons and pros.

Anyway, we have the natural ID for collations and let's look why. A user
can define it's own collations and in fact nothing prevents the
situation when one surrogate ID is used for different collations on
different instances. We want to overcome the subtle problem, when a user
assumes one collation, but a different one (maybe very slightly
different) holds the same ID locally.

Okay. How field types are different? Can we (in theory) give a user
ability to register it's own mp_ext (from a specified range), provide
a comparator and name it somehow? If it'll occur (that does not look as
impossible), the surrogate IDs may similarly lead to a confusion, say,
just because of autoincremented / autogenerated numeric IDs. It is
harder to step into such situation with string IDs.

That's aside of extra complexity I already mentioned in the commit
message. I guess most of usages of the <field_type> field will be to
grab it from Lua (where it is identified using a string) or from a human
(which usually operates on strings) or to show it to a human (when
requested or in an error message).

I guess it is not enough to convince you, but I at least made attempt to
show how I think around it (if some future internet archeologist will
curious).

> 
> > XXX: Add a module API test.
> > 
> > Part of #5273
> > ---
> >  src/box/key_def_api.c | 146 ++++++++++++++++++++++++++++++++++++++++++
> >  src/box/key_def_api.h | 123 +++++++++++++++++++++++++++++++++++
> >  src/exports.h         |   2 +
> >  3 files changed, 271 insertions(+)
> > 
> > diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
> > index 7f6c0ac55..19590095d 100644
> > --- a/src/box/key_def_api.c
> > +++ b/src/box/key_def_api.c
> > @@ -55,6 +138,67 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
> >  	return key_def;
> >  }
> >  
> > +box_key_def_t *
> > +box_key_def_new_ex(box_key_part_def_t *parts, uint32_t part_count)
> > +{
> > +	struct region *region = &fiber()->gc;
> > +	size_t region_svp = region_used(region);
> > +	size_t internal_parts_size;
> > +	struct key_part_def *internal_parts =
> > +		region_alloc_array(region, typeof(internal_parts[0]),
> > +				   part_count, &internal_parts_size);
> > +	if (parts == NULL) {
> 
> 6. I assume you wanted to check for internal_parts == NULL.

Argh. Thanks!

> 
> > +		diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
> > +			 "parts");
> > +		return NULL;
> > +	}
> > +	if (part_count == 0) {
> 
> 7. internal_parts leaks here? I don't even know if region allocates
> anything when size is 0. I propose to move the check before the
> allocation.

Ugh. Thank you again.

> 
> > +		diag_set(IllegalParams, "At least one key part is required");
> > +		return NULL;
> > +	}
> > +
> > +	/*
> > +	 * It is possible to implement a function similar to
> > +	 * key_def_new() and eliminate <box_key_part_def_t> ->
> > +	 * <struct key_part_def> copying. However this would lead
> > +	 * to code duplication and would complicate maintanence,
> > +	 * so it worth to do so only if key_def creation will
> > +	 * appear on a hot path in some meaningful use case.
> > +	 */
> > +	uint32_t min_field_count = 0;
> > +	for (uint32_t i = 0; i < part_count; ++i) {
> > +		if (key_def_set_internal_part(&internal_parts[i], &parts[i],
> > +					      region) != 0) {
> > +			region_truncate(region, region_svp);
> > +			return NULL;
> > +		}
> > +		bool is_nullable =
> > +			(parts[i].flags & BOX_KEY_PART_DEF_IS_NULLABLE_MASK) ==
> > +			BOX_KEY_PART_DEF_IS_NULLABLE_MASK;
> 
> 8. You have internal_parts[i].is_nullable for that.

See, there is a logic: this function (and key_def_set_internal_part())
reads everything from the public key part defs and write to the internal
key part defs and the resulting key def.

So the data dependency graph is the following:

 | <public parts> +-> <private parts> ---+
 |                |                      |
 |                |                      +-> <key_def>
 |                |                      |
 |                +-> <min_field_count> -+

But it would be so if I'll implement your suggestion:

 | <public parts> +-> <private parts> +----------------------+
 |                |                   |                      |
 |                |                   |                      +-> <key_def>
 |                |                   |                      |
 |                +-------------------+-> <min_field_count> -+

Or, if I'll change fieldno accesses to read from an internal part too:

 | <public parts> -> <private parts> +----------------------+
 |                                   |                      |
 |                                   |                      +-> <key_def>
 |                                   |                      |
 |                                   +-> <min_field_count> -+

I found the first graph as the simpler one. At least within this code
part, where all accesses are like on the first graph. When we'll move
this code to key_def_new(), we'll follow data access pattern of the
surrounding code.

Acquiring a flag from `parts[i].flags` looks huge, but it just because
of using bit arithmetic / masks and because the public name is large. I
don't think it should be a reason to discourage acquiring a flag from
a public key part.

Anyway, I hope it'll be part of key_def_new() in a future (see the
response to your 9th comment).

> 
> > +		if (!is_nullable && parts[i].fieldno > min_field_count)
> > +			min_field_count = parts[i].fieldno;
> > +	}
> > +
> > +	struct key_def *key_def = key_def_new(internal_parts, part_count,
> > +					      false);
> > +	region_truncate(region, region_svp);
> > +	if (key_def == NULL)
> > +		return NULL;
> > +
> > +	/*
> > +	 * Update key_def->has_optional_parts and function
> > +	 * pointers.
> > +	 *
> > +	 * FIXME: It seems, this call should be part of
> > +	 * key_def_new(), because otherwise a callee function may
> > +	 * obtain an incorrect key_def. However I don't know any
> > +	 * case that would prove this guess.
> 
> 9. Just do that and if all works, then it is fine to change. Looks like
> a bug which didn't have a place to explode until now.

I don't want to do it in a hurry, without ability to poke the code and
to make an attempt to write a test case. It is in my backlog, alongside
with several other possible problems found during working on the
patchset. I'll file issues for them and, if time will permit, will
propose fixes.

> 
> > +	 */
> > +	key_def_update_optionality(key_def, min_field_count);
> > +
> > +	return key_def;
> > +}
> > +
> >  void
> >  box_key_def_delete(box_key_def_t *key_def)
> > diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> > index 5b1c861f5..328a58c70 100644
> > --- a/src/box/key_def_api.h
> > +++ b/src/box/key_def_api.h
> > @@ -44,11 +44,104 @@ typedef struct tuple box_tuple_t;
> >  
> >  typedef struct key_def box_key_def_t;
> >  
> > +/** Key part definition flags. */
> > +enum {
> > +	BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT = 0,
> > +	BOX_KEY_PART_DEF_IS_NULLABLE_MASK =
> > +		1 << BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT,
> 
> 10. Does not it seem a little overcomplicated to you? Why
> do you need BOX_KEY_PART_DEF_IS_NULLABLE_SHIFT? Why not
> call BOX_KEY_PART_DEF_IS_NULLABLE_MASK
> BOX_KEY_PART_DEF_IS_NULLABLE_FLAG? Or
> BOX_KEY_PART_DEF_IS_NULLABLE.

Let me think...

Check whether a flag is set:

 | bool is_nullable = (part_def.flags &
 |         BOX_KEY_PART_DEF_IS_NULLABLE_MASK) ==
 |         BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

Set a flag:

 | part_def.flags |= BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

Clear a flag:

 | part_def.flags &= ~BOX_KEY_PART_DEF_IS_NULLABLE_MASK;

In fact, _SHIFT is not needed. So we can simplify it to just
BOX_KEY_PART_DEF_IS_NULLABLE. I'll do.

> 
> > +};
> > +
> > +/**
> > + * It is recommended to verify size of <box_key_part_def_t>
> > + * against this constant on the module side at build time.
> > + * Example:
> > + *
> > + * | #if !defined(__cplusplus) && !defined(static_assert)
> > + * | #define static_assert _Static_assert
> > + * | #endif
> > + * |
> > + * | (slash)*
> > + * |  * Verify that <box_key_part_def_t> has the same size when
> > + * |  * compiled within tarantool and within the module.
> > + * |  *
> > + * |  * It is important, because the module allocates an array of key
> > + * |  * parts and passes it to <box_key_def_new_ex>() tarantool
> > + * |  * function.
> > + * |  *(slash)
> > + * | static_assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE,
> > + * |               "sizeof(box_key_part_def_t)");
> > + *
> > + * This snippet is not part of module.h, because portability of
> > + * static_assert() / _Static_assert() is dubious. It should be
> > + * decision of a module author how portable its code should be.
> > + */
> > +enum {
> > +	BOX_KEY_PART_DEF_T_SIZE = 64,
> > +};
> > +
> > +/**
> > + * Public representation of a key part definition.
> > + *
> > + * Usage: Allocate an array of such key parts, initialize each
> > + * key part (call <box_key_part_def_create>() and set necessary
> > + * fields), pass the array into <box_key_def_new_ex>() function.
> > + *
> > + * The idea of separation from internal <struct key_part_def> is
> > + * to provide stable API and ABI for modules.
> > + *
> > + * New fields may be added into the end of the structure in later
> > + * tarantool versions. Also new flags may be introduced within
> > + * <flags> field. <collation> cannot be changed to a union (to
> > + * reuse for some other value), because it is verified even for
> > + * a non-string key part by <box_key_def_new_ex>().
> > + *
> > + * Fields that are unknown at given tarantool version are ignored.
> > + */
> > +typedef union PACKED {
> > +	struct {
> > +		/** Index of a tuple field (zero based). */
> > +		uint32_t fieldno;
> > +		/** Flags, e.g. nullability. */
> > +		uint32_t flags;
> > +		/** Type of the tuple field. */
> > +		const char *field_type;
> > +		/** Collation name for string comparisons. */
> > +		const char *collation;
> > +		/**
> > +		 * JSON path to point a nested field.
> > +		 *
> > +		 * Example:
> > +		 *
> > +		 * tuple: [1, {"foo": "bar"}]
> > +		 * key parts: [
> > +		 *     {
> > +		 *         "fieldno": 2,
> > +		 *         "type": "string",
> > +		 *         "path": "foo"
> > +		 *     }
> > +		 * ]
> > +		 *
> > +		 * => key: ["bar"]
> > +		 *
> > +		 * Note: When the path is given, <field_type>
> > +		 * means type of the nested field.
> > +		 */
> > +		const char *path;
> > +	};
> > +	/**
> > +	 * Padding to guarantee certain size across different
> > +	 * tarantool versions.
> > +	 */
> > +	char padding[BOX_KEY_PART_DEF_T_SIZE];
> > +} box_key_part_def_t;
> 
> 11. You know, we could also export a function like box_key_def_part_size(),
> which would return sizeof(key_part_def). Then the module would need
> to call it to allocate the part. And then use functions box_key_def_set_part_*()
> to set the part def fields. So the struct would be opaque, could be
> allocated in an array, and would be ABI stable. However probably it is
> the same what I proposed in the beginning of this email but with extra
> steps.

I hope I made my wishes clear enough in the response to your 2nd comment.

> 
> > +
> >  /**
> >   * Create key definition with given field numbers and field types.
> >   *
> >   * May be used for tuple format creation and/or tuple comparison.
> >   *
> > + * \sa <box_key_def_new_ex>().
> > + *
> >   * \param fields array with key field identifiers
> >   * \param types array with key field types (see enum field_type)
> >   * \param part_count the number of key fields
> > @@ -57,6 +150,28 @@ typedef struct key_def box_key_def_t;
> >  API_EXPORT box_key_def_t *
> >  box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
> >  
> > +/**
> > + * Initialize a key part with default values.
> > + *
> > + * All trailing padding bytes are set to zero.
> 
> 12. Why does it need to touch the padding? How is it
> related to ABI?

In fact, I just had desire to specify certain behaviour for functions,
which read from or write to <box_key_part_def_t>. There are the
following variants what to do with unknown fields and flags:

 | #   | Create / dump | Read              |
 | --- | ------------- | ----------------- |
 | (1) | unspecified   | ignore            |
 | (2) | don't touch   | ignore            |
 | (3) | zeroing       | ignore            |
 | (4) | zeroing       | error if not zero |

First, I don't like (1) and (2), because I fear to leave garbage after
initialization, it looks as a bad pattern.

(3) is for the robustness principle and (4) is against. I don't know
what is better here. I can implement my module based on any API with any
desired behaviour (now it forbids to create a key definition with a JSON
path on tarantool versions, which do not support it).

I guess most of times a new field or flag will be added, it will affect
how some key def functions will work (on a new tarantool version), so
(3) assumes explicit check of supported features on the module side.

(4) looks like allowing a module to lean on tarantool to perform the
check. However I'm not sure a module actually can miss parameters
checking. Say, if a default value of an option that is not supported at
given tarantool version is passed, the module should give an error (it
is debatable, but looks better for me).

Does it mean that (4) is more error-prone? I don't know. That's complex
question.

Anyway, both ways work for me. (3) assumes that I can test supported
features in runtime by passing non-zero valid values to _new(), than do
_dump_parts(): supported fields are ones that I got back. With (4) I can
test them in several iterations: pass a field and look whether _new()
gives an error. (See the code for (3) in [1].)

To be honest, I don't know how to better design those APIs. I guess (3)
is a bit more flexible (considering all sides perfectly understand
guarantees and strictly follow the API), so I decided to stick here,
because I don't know a future. Maybe it will allow us to do some nice
thing around behaviour of old version of a module on a new version of
tarantool. But it is hard to imagine something certain.

Okay, let me rephrase. I didn't think so much until you asked and I
chose (3) (because it look more or less natural for me) with the thought
"I'll reconsider it if I'll find some problem while looking at it from
the module side". I didn't find any problem and left it as is.

BTW, I stated explicitly that box_key_def_dump_parts() set unknown
fields and flags to zero.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019997.html

> 
> > + *
> > + * All unknown <flags> bits are set to zero.
> > + */
> > +API_EXPORT void
> > +box_key_part_def_create(box_key_part_def_t *part);