From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 859846EC55; Sat, 24 Jul 2021 02:06:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 859846EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627081594; bh=8V8ehTzPziMzVJY0jsYKC/keYHasTJ8cPKc4sUBJ710=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=bRLY7Q6F8WtVVAu0LPn9a2OyadId/3l+dZVjSnxIkX3KJPibAxarBXG+zMQd1vYB9 y/GFHzXhk7t/5hicrdYU77euyKKvmxw2AfDkQKHQY4j3kOHMkvQHGSfFNigXT27r2Q aNkPKUnLfme6ZwFPnmTL2ryjtjqohE1j6X6sPhVI= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 364B86EC55 for ; Sat, 24 Jul 2021 02:06:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 364B86EC55 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1m74FT-0000yy-GI; Sat, 24 Jul 2021 02:06:32 +0300 To: imeevma@tarantool.org, kyukhin@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <30b6e5331a79e14a0fdd0e5782aa7ac29c4922bb.1626358375.git.imeevma@gmail.com> Message-ID: <41ee6127-c53c-c964-a71c-c4a264f59ab0@tarantool.org> Date: Sat, 24 Jul 2021 01:06:30 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <30b6e5331a79e14a0fdd0e5782aa7ac29c4922bb.1626358375.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3FDAB68B812060C77E621B90589399EB5182A05F5380850406B56B25109519CCA732231F5D7B8DF1E791E1CE789D0B8D660FA47053494E26B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE768BD42809A772457EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B28E90C11C329EF18638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A264761594415C12902409A28F5FAB73117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6A45692FFBBD75A6A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5056AD8125B970CCCCABD0896727B78CAAE X-C1DE0DAB: 0D63561A33F958A5038B176CD492135B91123B15CF89DF75FED51FAFB33FCC21D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34BF5454112BD5BFD7E38CE2A50A3CD8D98838321011E8C6D45F4E079F805B75A07451F9B9177F88281D7E09C32AA3244C05C4349A0985A4B2243CAE995E430B8581560E2432555DBBFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojtKXY6NTIXk19Nqll9uRzpw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D1F3B0DD29538DCCDB4F17652EA9CB4373841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: define format of ephemeral spaces X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! See 9 comments below. > diff --git a/src/box/sql.c b/src/box/sql.c > index 790ca7f70..f5d0d6ce6 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -298,7 +298,8 @@ tarantoolsqlCount(struct BtCursor *pCur) > } > > struct space * > -sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > +sql_ephemeral_space_create_key_info(uint32_t field_count, > + struct sql_key_info *key_info) 1. The function creates not key_info. It creates a space by key_info. Maybe call it sql_ephemeral_space_new_from_key_info()? > { > struct key_def *def = NULL; > uint32_t part_count = field_count; > @@ -407,6 +408,122 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > return ephemer_new_space; > } > > +enum { > + /* > + * Name of the fields will be "_COLUMN_1", "_COLUMN_2" and so on. Due to > + * this, length of each name is no more than strlen("_COLUMN_") plus > + * length of UINT32_MAX turned to string, which is 10 and plus 1 for \0. > + */ > + NAME_LEN = 19, > +}; > + > +static struct space_def * > +sql_space_def_new_ephemeral(uint32_t field_count, enum field_type *types, > + uint32_t *coll_ids) 2. The last 2 arguments can be made const. The same in the other functions below. > +{ > + struct region *region = &fiber()->gc; > + size_t svp = region_used(region); > + uint32_t size = field_count * sizeof(struct field_def); > + struct field_def *fields = region_aligned_alloc(region, size, > + alignof(fields[0])); 3. There is region_alloc_array(). The same in the next function. > + if (fields == NULL) { > + diag_set(OutOfMemory, size, "region_aligned_alloc", "fields"); > + return NULL; > + } > + char *names = region_alloc(region, field_count * NAME_LEN); > + if (names == NULL) { > + diag_set(OutOfMemory, size, "region_alloc", "names");> + return NULL; > + } > + for (uint32_t i = 0; i < field_count; ++i) { > + struct field_def *field = &fields[i]; > + field->name = &names[i * NAME_LEN]; > + sprintf(field->name, "_COLUMN_%d", i); > + field->is_nullable = true; > + field->nullable_action = ON_CONFLICT_ACTION_NONE; > + field->default_value = NULL; > + field->default_value_expr = NULL; > + field->type = types[i]; > + field->coll_id = coll_ids[i]; > + } > + struct space_def *def = space_def_new_ephemeral(field_count, fields); > + region_truncate(region, svp); > + return def; > +} > + > +static struct index_def * > +sql_index_def_new_ephemeral(uint32_t field_count, enum field_type *types, > + uint32_t *coll_ids, bool is_pk_rowid) > +{ > + uint32_t part_count = is_pk_rowid ? 1 : field_count; > + struct region *region = &fiber()->gc; > + size_t svp = region_used(region); > + uint32_t size = part_count * sizeof(struct key_part_def); > + struct key_part_def *parts = region_aligned_alloc(region, size, > + alignof(parts[0])); > + if (parts == NULL) { > + diag_set(OutOfMemory, size, "region_aligned_alloc", "parts"); > + return NULL; > + } > + if (is_pk_rowid) { > + uint32_t j = field_count - 1; > + parts[0].fieldno = j; > + parts[0].nullable_action = ON_CONFLICT_ACTION_NONE; > + parts[0].is_nullable = true; > + parts[0].exclude_null = false; > + parts[0].sort_order = SORT_ORDER_ASC; > + parts[0].path = NULL; > + parts[0].type = types[j]; > + parts[0].coll_id = coll_ids[j]; > + } else { > + for (uint32_t i = 0; i < part_count; ++i) { > + struct key_part_def *part = &parts[i]; > + part->fieldno = i; > + part->nullable_action = ON_CONFLICT_ACTION_NONE; > + part->is_nullable = true; > + part->exclude_null = false; > + part->sort_order = SORT_ORDER_ASC; > + part->path = NULL; > + part->type = types[i]; > + part->coll_id = coll_ids[i]; > + } > + } > + struct key_def *key_def = key_def_new(parts, part_count, false); > + if (key_def == NULL) > + return NULL; > + const char *name = "ephemer_idx"; > + struct index_def *def = index_def_new(0, 0, name, strlen(name), TREE, > + &index_opts_default, key_def, > + NULL); > + key_def_delete(key_def); > + region_truncate(region, svp); > + return def; > +} > + > +struct space * > +sql_ephemeral_space_create(uint32_t field_count, enum field_type *types, > + uint32_t *coll_ids, bool is_pk_rowid) 4. It is strange how there are functions sql_space_def_new_ephemeral sql_index_def_new_ephemeral and at the same time these: sql_ephemeral_space_create_key_info sql_ephemeral_space_create First ones use ephemeral in the end, the last ones in the beginning. I would propose to unify them while you are here. One of the options: sql_ephemeral_space_def_new sql_ephemeral_index_def_new sql_ephemeral_space_new_from_key_info sql_ephemeral_space_new > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 62a726fdd..b35545296 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -249,22 +254,20 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > * account that id field as well. That's why pk_len > * has one field more than view format. > */ > - pk_len = space->def->field_count + 1; > - parse->nMem += pk_len; > - sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph, > - pk_len); > + uint32_t count = space->def->field_count; > + fields_info_from_space_def(info->types, info->coll_ids, > + count, space->def); 5. In all usages of fields_info_from_space_def you pass 2 fields of field_info. I propose to make it take field_info pointer instead of these 2 arrays. Also for individual fields I propose to add field_info_set_field(type, coll_id). So as not to miss anything. Currently it looks too much boilerplate code. > + info->types[count] = FIELD_TYPE_UNSIGNED; > + info->coll_ids[count] = COLL_NONE; > } else { > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index b9107fccc..c59d7b4b4 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -99,6 +99,103 @@ struct SortCtx { > #define SORTFLAG_UseSorter 0x01 /* Use SorterOpen instead of OpenEphemeral */ > #define SORTFLAG_DESC 0xF0 > > +static inline uint32_t > +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n); > + > +struct fields_info * > +fields_info_new(uint32_t len) > +{ > + uint32_t size_info = sizeof(struct fields_info); > + uint32_t size_types = len * sizeof(enum field_type); > + uint32_t size = size_info + size_types + len * (sizeof(uint32_t)); > + char *buf = sqlDbMallocRawNN(sql_get(), size); > + struct fields_info *info = (struct fields_info *)buf; > + if (info == NULL) > + return NULL; > + info->key_info = NULL; > + buf += size_info; > + info->types = (enum field_type *)buf; > + buf += size_types; > + info->coll_ids = (uint32_t *)buf; > + return info; > +} > + > +void > +fields_info_delete(struct fields_info *info) > +{ > + if (info->key_info != NULL) > + sql_key_info_unref(info->key_info); > + return sqlDbFree(sql_get(), info); 6. You do not need this explicit 'return'. > +} > + > +void > +fields_info_from_space_def(enum field_type *types, uint32_t *coll_ids, > + uint32_t count, struct space_def *def) 7. def can be made const. The same for some other arguments below. > +{ > + for (uint32_t i = 0; i < count; ++i) { > + types[i] = def->fields[i].type; > + coll_ids[i] = def->fields[i].coll_id; > + } > +} > + > +void > +fields_info_from_index_def(enum field_type *types, uint32_t *coll_ids, > + uint32_t count, struct index_def *def) > +{ > + for (uint32_t i = 0; i < count; ++i) { > + types[i] = def->key_def->parts[i].type; > + coll_ids[i] = def->key_def->parts[i].coll_id; > + } > +} > + > +static int > +fields_info_from_expr_list(enum field_type *types, uint32_t *coll_ids, > + struct Parse *parser, struct ExprList *list) > +{ > + for (int i = 0; i < list->nExpr; ++i) { > + bool b; > + struct coll *coll; > + struct Expr *expr = list->a[i].pExpr; > + types[i] = sql_expr_type(expr); > + if (types[i] == FIELD_TYPE_ANY) > + types[i] = FIELD_TYPE_SCALAR; > + if (sql_expr_coll(parser, expr, &b, &coll_ids[i], &coll) != 0) > + return -1; > + } > + return 0; > +} > + > +static int > +fields_info_from_order_by(enum field_type *types, uint32_t *coll_ids, > + struct Parse *parser, struct Select *select, > + struct ExprList *order_by) > +{ > + for (int i = 0; i < order_by->nExpr; ++i) { > + bool b; > + struct Expr *expr = order_by->a[i].pExpr; > + types[i] = sql_expr_type(expr); > + if (types[i] == FIELD_TYPE_ANY) > + types[i] = FIELD_TYPE_SCALAR; > + uint32_t id = COLL_NONE; > + if ((expr->flags & EP_Collate) != 0) { > + struct coll *coll; > + if (sql_expr_coll(parser, expr, &b, &id, &coll) != 0) > + return -1; > + } else { > + uint32_t fieldno = order_by->a[i].u.x.iOrderByCol - 1; > + id = multi_select_coll_seq(parser, select, fieldno); > + if (id != COLL_NONE) { > + const char *name = coll_by_id(id)->name; > + order_by->a[i].pExpr = > + sqlExprAddCollateString(parser, expr, > + name); > + } > + } > + coll_ids[i] = id; > + } > + return 0; > +} 8. It sligtly cuts the ears that a struct called fields_info also defines a key via key_info. It is like specifying indexes inside of space format. Maybe it is worth renaming it to ephemeral_space_def? Or space_info? Or shorten ephemeral to ephmen in all the new code and use ephem_space_def? Or sql_space_info? sql_ephemeral_space_def? And make sql_ephemeral_space_create() take this struct instead of 2 arrays. Besides, you could hide sql_ephemeral_space_create_key_info() inside of it because you could check key_info != null in sql_ephemeral_space_create(). It just does not seem like vdbe business to check these things. > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index ef8dcd693..167e80057 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -2248,6 +2248,11 @@ struct Parse { > #define OPFLAG_SYSTEMSP 0x20 /* OP_Open**: set if space pointer > * points to system space. > */ > +/** > + * If this flag is set, than in OP_OpenTEphemeral we should use rowid as the 9. than -> then.