From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2C55F21959 for ; Mon, 28 May 2018 08:10:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x6YRaE3AsgXA for ; Mon, 28 May 2018 08:10:50 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B13A0218F1 for ; Mon, 28 May 2018 08:10:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: add index_def to struct Index (fixed version) References: <1527497514.998361944@f317.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <81e89dc5-a4f9-4dad-85d8-4b27692aaad6@tarantool.org> Date: Mon, 28 May 2018 15:10:46 +0300 MIME-Version: 1.0 In-Reply-To: <1527497514.998361944@f317.i.mail.ru> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Ivan Koptelov Hello. Thanks for the patch! See my 35 comments below. 1. The patch is not so big to do not fit in a letter. Please, send a patch as a file only when it is really huge. 2. There is a problem with your GibHub account. Your profile name in commits on github must be a link to the profile. But your name is just a text. Please, cope with the problem. 3. Now you can remove sql_index_collation() function. It is useless two line wrapper. And its second out parameter is not needed in some places. 4. Same about index_is_unique(). 5. get_new_iid() is unused. > +void > +append(struct region *r, const char *str, char **sql_arr, int idx, int total_sql_str_size) > +{ > + sql_arr[idx] = region_alloc(r, strlen(str)); > + strcpy(sql_arr[idx], str); > + total_sql_str_size += strlen(str); > +} 6. How does it work? total_sql_str_size is not out parameter here. In the caller context it is still unchanged. > + > +char * > +create_sql(const char *idx_name, struct space_def *space_def, ExprList *expr_list) > +{ > + struct region *r = &fiber()->gc; > + /* > + * CREATE_INDEX_ + INDEX_NAME + _ON_ + TABLE_NAME + _( > + */ > + uint32_t sql_parts_count = 5; 7. You do not need sql parts array, part count. You just do region_alloc + memcpy and region_join at the end. It is not required to store each part anywhere. > + Expr *column_expr; > + for (uint32_t i = 0; i < (uint32_t) expr_list->nExpr; i++){ > + column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr); > + sql_parts_count += expr_list->a[i].pExpr == column_expr ? 2 : 4; > + } > + > + size_t total_sql_str_size = 0; > + > + char **sql = region_alloc(r, sizeof(char*) * sql_parts_count); > + > + char *CREATE_INDEX = "CREATE INDEX "; 8. Do not store const string as non-const. 9. Why do you need this variable? Why can not you just inline it one line below?. > + append(r, CREATE_INDEX, sql, 0, total_sql_str_size); > + append(r, idx_name, sql, 1, total_sql_str_size); > + > + char *ON = " ON "; 10. Same. > + append(r, ON, sql, 2, total_sql_str_size); > + > + char *name = space_def->name; > + append(r, name, sql, 3, total_sql_str_size); > + append(r, " (", sql, 4, total_sql_str_size); > + > + Expr *collation_expr; > + uint32_t sql_idx = 5; > + for (int i = 0; i < expr_list->nExpr; i++){ > + column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr); > + collation_expr = expr_list->a[i].pExpr == column_expr ? > + NULL : expr_list->a[i].pExpr; 11. Please, do like sql_create_index(): use pExpr->op == TK_COLLATE to detect that it is a collated column. 12. Why did not you call sqlite3ResolveSelfReference() ? 13. Why your patch did not delete Index.coll_array, Index.coll_id_array, Index.aiColumn? Your patch must remove them. Index_def initialization must replace code in build.c on lines 3158:3202. > + // fix last ", " with ")\0" > + strcpy(sql[sql_idx - 1], ")\0"); > + char *res = region_join(r, total_sql_str_size); > + return res; 14. Do not use // comments. 15. Why do you need separate create_sql() function? Can you generate an expression during parts initialization? > +void > +set_index_def(Parse *parse, Index *index, Table *table, > + const char *name, > + uint32_t name_len, int on_error, > + ExprList *expr_list, u8 idx_type) 16. Big problems with indentation. > +{ > + struct space_def *space_def = table->def; > + uint32_t iid = SQLITE_PAGENO_TO_INDEXID(index->tnum); 17. You do not know iid here. The parser just parses and does not generate identifiers. I checked this code in a debugger, and in this example: box.sql.execute("CREATE TABLE t1(x integer primary key, a integer unique);") It creates two struct Index with the same def->iid == 0. It works only thanks to that Index.def->iid is not used anywhere now. > + struct index_opts opts; > + index_opts_create(&opts); > + opts.stat = NULL; 18. Already done by index_opts_create. > + opts.is_unique = on_error != ON_CONFLICT_ACTION_NONE; > + > + struct key_def *key_def = key_def_new(expr_list->nExpr); 19. Please, check each function result on error. Key_def_new can fail. > + > + if (idx_type == SQLITE_IDXTYPE_APPDEF) { > + opts.sql = create_sql(name, table->def, expr_list); > + } 20. Please, do not put 'if' body in {} when it consists of a single line. > + > + int i = 0; 21. Why can not you put 'int i = 0' into 'for (...'? > + Expr *column_expr; > + Expr *collation_expr; 22. Same. These variables are unused out of 'for'. And can be declared + initialized inside. > + for (i = 0; i < expr_list->nExpr; i++) { > + column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr); > + collation_expr = expr_list->a[i].pExpr == column_expr ? > + NULL : expr_list->a[i].pExpr; 23. Same as 11. > + > + uint32_t fieldno = column_expr->iColumn; > + > + uint32_t coll_id; > + struct coll *coll; > + if (collation_expr != NULL) > + coll = sql_get_coll_seq(parse, collation_expr->u.zToken,&coll_id); > + else > + coll = sql_column_collation(space_def, fieldno, &coll_id); > + > + key_def_set_part(key_def, i, fieldno, > + space_def->fields[fieldno].type, > + space_def->fields[fieldno].nullable_action, > + coll, coll_id, SORT_ORDER_ASC); > + } > + > + struct key_def *pk_key_def; > + if (idx_type == SQLITE_IDXTYPE_APPDEF) { > + pk_key_def = table->pIndex->def->key_def; > + } else { > + pk_key_def = NULL; > + } > + > + index->def = index_def_new(space_def->id, iid, name, name_len, > + TREE, &opts, key_def, pk_key_def); 24. Same as 16. > @@ -3068,6 +3200,11 @@ sql_create_index(struct Parse *parse, struct Token *token, > */ > pIndex->sort_order[i] = SORT_ORDER_ASC; > } > + > + set_index_def(parse, pIndex, pTab, zName, > + nName, on_error, col_list, > + idx_type); > + 25. It fits in 2 lines. > @@ -3133,6 +3270,7 @@ sql_create_index(struct Parse *parse, struct Token *token, > } > if (idx_type == SQLITE_IDXTYPE_PRIMARYKEY) > pIdx->idxType = idx_type; > + > goto exit_create_index; > } > } 26. Garbage diff. > @@ -3141,6 +3279,7 @@ sql_create_index(struct Parse *parse, struct Token *token, > /* Link the new Index structure to its table and to the other > * in-memory database structures. > */ > + > assert(parse->nErr == 0); > if (db->init.busy) { > Index *p; 27. Same. 28. index_column_count() is useless now. 29. index_is_unique_not_null() - same. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 5fbcbaffc..34ac846cf 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -4298,7 +4298,7 @@ sqlite3IndexedByLookup(Parse * pParse, struct SrcList_item *pFrom) > char *zIndexedBy = pFrom->u1.zIndexedBy; > Index *pIdx; > for (pIdx = pTab->pIndex; > - pIdx && strcmp(pIdx->zName, zIndexedBy); > + pIdx && strcmp(pIdx->def->name, zIndexedBy); > pIdx = pIdx->pNext) ; > if (!pIdx) { > sqlite3ErrorMsg(pParse, "no such index: %s", zIndexedBy, 30. If you use def->name instead of zName, then remove zName from struct Index. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 950409d93..fe61dacfe 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2124,6 +2124,7 @@ struct Index { > * or _NONE > */ > unsigned idxType:2; /* 1==UNIQUE, 2==PRIMARY KEY, 0==CREATE INDEX */ > + struct index_def *def; > }; 31. 4 spaces + 1 tab is bad indentation. Here 1 tab is enough. > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index 496b41725..6d95185b3 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -470,7 +470,7 @@ findIndexCol(Parse * pParse, /* Parse context */ > for (int i = 0; i < pList->nExpr; i++) { > Expr *p = sqlite3ExprSkipCollate(pList->a[i].pExpr); > if (p->op == TK_COLUMN && > - p->iColumn == pIdx->aiColumn[iCol] && > + p->iColumn == (int) pIdx->def->key_def->parts[iCol].fieldno && > p->iTable == iBase) { > bool is_found; > uint32_t id; 32. Out of 80. > @@ -2859,6 +2859,19 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */ > sPk.aiRowLogEst = aiRowEstPk; > sPk.onError = ON_CONFLICT_ACTION_REPLACE; > sPk.pTable = pTab; > + > + struct key_def *key_def = key_def_new(1); 33. Check on error. > + key_def_set_part(key_def, 0, 0, pTab->def->fields[0].type, > + ON_CONFLICT_ACTION_ABORT, > + NULL, COLL_NONE, SORT_ORDER_ASC); > + > + struct index_opts index_opts = index_opts_default; > + > + sPk.def = > + index_def_new(pTab->def->id, 0, "primary", > + sizeof("primary") - 1, TREE, > + &index_opts, key_def, NULL); 34. Same as 33. And it fits in 3 lines. > diff --git a/test/box/suite.ini b/test/box/suite.ini > index ddc71326b..94bc39c57 100644 > --- a/test/box/suite.ini > +++ b/test/box/suite.ini > @@ -2,7 +2,7 @@ > core = tarantool > description = Database tests > script = box.lua > -disabled = rtree_errinj.test.lua tuple_bench.test.lua > +disabled = rtree_errinj.test.lua tuple_bench.test.lua errinj.test.lua > release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua > lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua > use_unix_sockets = True 35. Why? On 28/05/2018 11:51, Ivan Koptelov wrote: