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 3B8A46EC40; Thu, 12 Aug 2021 21:32:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3B8A46EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628793168; bh=0xz/Qm2AHjv96DOGFaIQBTSi4B7TDCd+qo4/MmxPlvk=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=X7LnGQLcQz/EnoaOeLHMxHtH+4k/pEEYz4xAObDUHJ3v666Y3g1CJmaTtJqqKccYY FvrXKf4x3Xoe5I9QErhZKWyXOuAPuj3IQ6bUjYCes2ZPE7zUaldsEzKngC95oUN1CI DnfPdGvGDgDNHq4Vqa4vSMh65/ePS5rXSThef6nA= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 D96666EC40 for ; Thu, 12 Aug 2021 21:32:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D96666EC40 Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1mEFVT-0006j6-O8; Thu, 12 Aug 2021 21:32:46 +0300 To: Mergen Imeev References: <30b6e5331a79e14a0fdd0e5782aa7ac29c4922bb.1626358375.git.imeevma@gmail.com> <41ee6127-c53c-c964-a71c-c4a264f59ab0@tarantool.org> <20210810143251.GA471410@tarantool.org> Message-ID: Date: Thu, 12 Aug 2021 21:32:38 +0300 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: <20210810143251.GA471410@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9D5AC6413C25DCF08CC98B8FCC5CD86F3182A05F5380850401F82771B273A6B1EBB65364A4322FD9001B8CBA42DAD8DAA70E5E8B6859B9F0D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71BF69A9C8C5AF260EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370E0E628E5A2670BA8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81F4177AA3331BCCCE4302C3F8C10D6ED117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE91ADC097FE2C3A081B0CC92B5A49C88ED8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3BFD98ABA943BD70B6136E347CC761E07C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637B8F435DEDE9E76EBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79D94463893BF8742D00D576F459B06BAB8 X-C1DE0DAB: 0D63561A33F958A515F5B8613209504E0222EF346D14A635F8AA11D353C70B4DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752DA3D96DA0CEF5C48E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341776B9FDE05FBA7A7AA399BBD8C678BB62F91E17794A4D5C5AAF67054F9EF44CD93584FE8E1068441D7E09C32AA3244CC31A29BB2DF0DFCC4141DBC353D1F97C3C6EB905E3A8056BFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKW4rnL99YhL7RRl36KQ7SQ== X-Mailru-Sender: DCB18673505F245B5D9D3B35E842BD3C32188A86A74A089E817A15D3A81166581186C5DEF162E43D841FB911095AA09146E8006E22572C39C920B61C43410E8717BDA556287159EE9437F6177E88F7363CDA0F3B3F5B9367 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 Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the fixes! See 7 comments below. > diff --git a/src/box/sql.c b/src/box/sql.c > index b87d236d1..d4b88d6e5 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c <...> > +void > +sql_space_info_from_space_def(struct space_info *info, > + const struct space_def *def) > +{ > + assert(info->field_count == def->field_count + 1); > + assert(info->part_count == 0); > + for (uint32_t i = 0; i < def->field_count; ++i) { > + info->types[i] = def->fields[i].type; > + info->coll_ids[i] = def->fields[i].coll_id; > + } > + info->types[def->field_count] = FIELD_TYPE_INTEGER; > + info->coll_ids[def->field_count] = COLL_NONE; 1. Please, explain in a comment what is this last special field. <...> > +struct space * > +sql_ephemeral_space_new_from_info(const struct space_info *info) > +{ > + uint32_t field_count = info->field_count; > + uint32_t part_count = info->parts == NULL ? field_count : > + info->part_count; > + uint32_t parts_indent = field_count * sizeof(struct field_def); > + uint32_t names_indent = part_count * sizeof(struct key_part_def) + > + parts_indent; > + uint32_t size = names_indent + field_count * NAME_LEN; > + > + struct region *region = &fiber()->gc; > + size_t svp = region_used(region); > struct field_def *fields = region_aligned_alloc(region, size, > alignof(fields[0])); > if (fields == NULL) { > diag_set(OutOfMemory, size, "region_aligned_alloc", "fields"); > return NULL; > } > - struct key_part_def *ephemer_key_parts = > - (void *)fields + field_count * sizeof(struct field_def); > - static_assert(alignof(*fields) == alignof(*ephemer_key_parts), > - "allocated in one block, and should have the same " > - "alignment"); > - char *names = (char *)ephemer_key_parts + > - part_count * sizeof(struct key_part_def); > - for (uint32_t i = 0; i < field_count; ++i) { > - struct field_def *field = &fields[i]; > - field->name = names; > - names += 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; > - if (def != NULL && i < def->part_count) { > - assert(def->parts[i].type < field_type_MAX); > - field->type = def->parts[i].type; > - field->coll_id = def->parts[i].coll_id; > - } else { > - field->coll_id = COLL_NONE; > - field->type = FIELD_TYPE_SCALAR; > - } > + struct key_part_def *parts = (void *)fields + parts_indent; 2. Are you sure void * + integer works bytewise? Shouldn't it be cast to char * to move the pointer? In other places you use char *. > + static_assert(alignof(*fields) == alignof(*parts), "allocated in one " > + "block, and should have the same alignment"); > + char *names = (char *)fields + names_indent; > + > + for (uint32_t i = 0; i < info->field_count; ++i) { > + fields[i].name = &names[i * NAME_LEN]; > + sprintf(fields[i].name, "_COLUMN_%d", i); > + fields[i].is_nullable = true; > + fields[i].nullable_action = ON_CONFLICT_ACTION_NONE; > + fields[i].default_value = NULL; > + fields[i].default_value_expr = NULL; > + fields[i].type = info->types[i]; > + fields[i].coll_id = info->coll_ids[i]; > + } > + if (info->parts == NULL) { > + for (uint32_t i = 0; i < part_count; ++i) > + parts[i].fieldno = i; > + } else { > + for (uint32_t i = 0; i < part_count; ++i) > + parts[i].fieldno = info->parts[i]; > } 3. It looks not so good for the cache that you walk the parts array more than once. > - > for (uint32_t i = 0; i < part_count; ++i) { > - struct key_part_def *part = &ephemer_key_parts[i]; > - /* > - * In case we need to save initial order of > - * inserted into ephemeral space rows we use rowid > - * as the only part of PK. If ephemeral space has > - * a rowid, it is always the last column. > - */ > - uint32_t j = i; > - if (key_info != NULL && key_info->is_pk_rowid) > - j = field_count - 1; > - part->fieldno = j; > - 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 = fields[j].type; > - part->coll_id = fields[j].coll_id; > + parts[i].nullable_action = ON_CONFLICT_ACTION_NONE; > + parts[i].is_nullable = true; > + parts[i].exclude_null = false; > + parts[i].sort_order = info->sort_order; > + parts[i].path = NULL; > + parts[i].type = info->types[parts[i].fieldno]; > + parts[i].coll_id = info->coll_ids[parts[i].fieldno]; > } > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 5226dd6ea..9d5a63fc5 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -224,12 +224,20 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > * is held in ephemeral table, there is no PK for > * it, so columns should be loaded manually. > */ > - struct sql_key_info *pk_info = NULL; > int reg_eph = ++parse->nMem; > int reg_pk = parse->nMem + 1; > - int pk_len; > + int pk_len = is_view ? space->def->field_count + 1 : > + space->index[0]->def->key_def->part_count; > int eph_cursor = parse->nTab++; > int addr_eph_open = sqlVdbeCurrentAddr(v); > + struct space_info *info = sql_space_info_new(pk_len, 0); 4. It is hard to understand when you calculate the space info field count, create the info, and later sql_space_info_from_space_def() assumes internally the field count you calculated manually. But I can't propose anything better now. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 21b4f2407..99b5ceee6 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -442,34 +442,44 @@ sqlInsert(Parse * pParse, /* Parser context */ > reg_eph = ++pParse->nMem; > regRec = sqlGetTempReg(pParse); > regCopy = sqlGetTempRange(pParse, nColumn + 1); > - sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph, > - nColumn + 1); > /* > - * This key_info is used to show that > - * rowid should be the first part of PK in > - * case we used AUTOINCREMENT feature. > - * This way we will save initial order of > - * the inserted values. The order is > - * important if we use the AUTOINCREMENT > - * feature, since changing the order can > - * change the number inserted instead of > - * NULL. > + * Order of inserted values is important since it is > + * possible, that NULL will be inserted in field with > + * AUTOINCREMENT. So, the first part of key should be > + * rowid. Since each rowid is unique, we do not need any > + * other parts. > */ > - if (space->sequence != NULL) { > - struct sql_key_info *key_info = > - sql_key_info_new(pParse->db, > - nColumn + 1); > - key_info->parts[nColumn].type = > - FIELD_TYPE_UNSIGNED; > - key_info->is_pk_rowid = true; > - sqlVdbeChangeP4(v, -1, (void *)key_info, > - P4_KEYINFO); > + struct space_info *info = > + sql_space_info_new(nColumn + 1, 1); > + if (info == NULL) { > + pParse->is_aborted = true; > + goto insert_cleanup; > } > + struct field_def *fields = space_def->fields; > + if (pColumn != NULL) { > + for (int i = 0; i < nColumn; ++i) { > + int j = pColumn->a[i].idx; > + info->types[i] = fields[j].type; > + info->coll_ids[i] = fields[j].coll_id; > + } > + } else { > + for (int i = 0; i < nColumn; ++i) { > + info->types[i] = fields[i].type; > + info->coll_ids[i] = fields[i].coll_id; > + } > + } > + info->types[nColumn] = FIELD_TYPE_INTEGER; > + info->parts[0] = nColumn; 5. Why is sql_space_info_from_space_def() extracted but this is not? It looks quite internal to space_info, since it changes its fields in some intricate way. The same in the other places where info is changed. For instance, in @@ -5879,6 +6027,39 @@ sqlSelect. > + > + sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph, > + nColumn + 1, 0, (char *)info, P4_DYNAMIC); > addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm); > VdbeCoverage(v); > sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph, > regCopy + nColumn); > sqlVdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1); > + sqlVdbeAddOp4(v, OP_ApplyType, regCopy, nColumn + 1, 0, > + (char *)info->types, P4_STATIC); > sqlVdbeAddOp3(v, OP_MakeRecord, regCopy, > nColumn + 1, regRec); > /* Set flag to save memory allocating one by malloc. */ > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index b9107fccc..a26ca54a3 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1044,13 +1132,28 @@ selectInnerLoop(Parse * pParse, /* The parser context */ > * space format. > */ > uint32_t excess_field_count = 0; > + struct VdbeOp *op = sqlVdbeGetOp(v, > + pSort->addrSortIndex); > + struct space_info *info; > + if (op->p4type == P4_KEYINFO) > + info = op->p4.key_info->info; > + else > + info = op->p4.space_info; > for (i = pSort->nOBSat; i < pSort->pOrderBy->nExpr; > i++) { > int j = pSort->pOrderBy->a[i].u.x.iOrderByCol; > - if (j > 0) { > - excess_field_count++; > - pEList->a[j - 1].u.x.iOrderByCol = > - (u16) (i + 1 - pSort->nOBSat); 6. No need for a whitespace after the cast. Also it could be turned to uint16_t since you changed the line anyway. > + if (j == 0) > + continue; > + assert(j > 0); > + excess_field_count++; > + pEList->a[j - 1].u.x.iOrderByCol = > + (u16) (i + 1 - pSort->nOBSat); > + --info->field_count; > + for (int k = j; k < pEList->nExpr; ++k) { > + int n = k + pSort->pOrderBy->nExpr + 1; > + info->types[n - 1] = info->types[n]; > + info->coll_ids[n - 1] = > + info->coll_ids[n]; > } > } > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 1f87e6823..60fa1678d 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -4006,6 +4006,8 @@ sql_analysis_load(struct sql *db); > */ > struct sql_key_info { > sql *db; > + /* Informations about all field types and key parts. */ 7. For out of comments we use /** opening usually.