From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4F8E7469719 for ; Fri, 9 Oct 2020 12:33:19 +0300 (MSK) Date: Fri, 9 Oct 2020 12:33:35 +0300 From: Alexander Turenko Message-ID: <20201009093335.62g5qcowisd746gr@tkn_work_nb> References: <94bccf8d2647debb1c1184af95713d48acf4c504.1600817803.git.alexander.turenko@tarantool.org> <61c50889-7675-0586-0229-8f079814177e@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <61c50889-7675-0586-0229-8f079814177e@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 10/14] WIP: module api: add box_key_def_dump_parts() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org > > The function dumps an opacue structure into a non-opacue > > 1. opacue -> opaque. Will fix in the next version of the patchset. > > array of 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 (it is a pointer to , see [^1]), we only have the pointer to the opaque structure. Of course, we can create our own structure around and duplicate all information there, but it would be... I don't know... wasteful. [^1]: Side note regarding name: | Modification of cdata metatype is not allowed in LuaJIT, so we | should use another structure name for Lua (not | ). The reason is that built-in module is | already loaded and already calls its ffi.metatype() on | . 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 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.