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 9D87324AA4 for ; Tue, 15 May 2018 11:56:16 -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 4OY2hdnhVtfn for ; Tue, 15 May 2018 11:56:16 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 09E3C24A81 for ; Tue, 15 May 2018 11:56:15 -0400 (EDT) Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1fIcJB-0003YS-S2 for tarantool-patches@freelists.org; Tue, 15 May 2018 18:56:14 +0300 Subject: [tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column References: <70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org> <7204bccd-3e12-f063-6717-3bbb95e2d570@tarantool.org> From: Kirill Shcherbatov Message-ID: <6f7cfd9a-422b-2cf3-11a1-1ef567aead9a@tarantool.org> Date: Tue, 15 May 2018 18:56:03 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 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 > 1. Wrong fix. As I can see sql_table_def_rebuild does not depend on allocation > way of its arguments. So the function declaration comment must be fixed too.@@ -166,8 +166,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name); /** * Rebuild struct def in Table with memory allocated on a single - * malloc. Fields and strings are expected to be allocated with - * sqlite3DbMalloc. + * malloc. > 2. OutOfMemory takes variable name in the last argument. Not caller function name. > 3. Same. @@ -661,7 +661,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) diag_set(OutOfMemory, pName->n + 1, - "region_alloc", "sqlite3AddColumn"); + "region_alloc", "z"); pParse->rc = SQL_TARANTOOL_ERROR; pParse->nErr++; return; @@ -871,7 +871,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan) diag_set(OutOfMemory, default_length + 1, "region_alloc", - "sqlite3AddDefaultValue"); + "field->default_value"); > 4. Looks like you removed sql_column_collation. It would be good to see the diff > under the comment. @@ -1053,45 +1030,13 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken) - if (pIdx->aiColumn[0] == i) - pIdx->coll_array[0] = sql_column_collation(p, i); + if (pIdx->aiColumn[0] == i) { + pIdx->coll_array[0] = + coll_by_id(p->def->fields[i].coll_id); + } } @@ -3066,7 +3074,7 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ - coll = sql_column_collation(pTab, j); + coll = coll_by_id(pTab->def->fields[j].coll_id); @@ -179,13 +182,14 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found) - coll = sql_column_collation(p->pTab, j); + coll = coll_by_id( + p->space_def->fields[j].coll_id); @@ -298,14 +299,14 @@ sqlite3FkLocateIndex(Parse * pParse, /* Parse context to store any error in */ - def_coll = sql_column_collation(pParent, - iCol); + def_coll = coll_by_id( + pParent->def->fields[iCol].coll_id); @@ -298,14 +299,14 @@ sqlite3FkLocateIndex(Parse * pParse, /* Parse context to store any error in */ - def_coll = sql_column_collation(pParent, - iCol); + def_coll = coll_by_id( + pParent->def->fields[iCol].coll_id); @@ -1784,26 +1795,29 @@ xferOptimization(Parse * pParse, /* Parser context */ - if (sql_column_collation(pDest, i) != - sql_column_collation(pSrc, i)) { + if (coll_by_id(pDest->def->fields[i].coll_id) != + coll_by_id(pSrc->def->fields[i].coll_id)) @@ -3529,8 +3499,6 @@ void sqlite3AddCollateType(Parse *, Token *); -struct coll * -sql_column_collation(Table *, uint32_t); > 5. Out of 80 symbols. Please, do self-review of a patch before sending. @@ -2110,7 +2110,8 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable); + sqlite3ColumnsFromExprList(pParse, pTable->pCheck, + pTable); > 6. I looked at sqlite3ColumnsFromExprList, and looks like it does not use > opts.temporary flag. So you can remove this opts.temporary flag manipulation, it is not? @@ -1742,7 +1742,6 @@ sql_ephemeral_table_new(Parse *parser, const char *name) int sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) { - assert(pTable->def->opts.temporary); struct space_def *old_def = pTable->def; @@ -2108,12 +2108,9 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) * a VIEW it holds the list of column names. */ struct space_def *old_def = pTable->def; - /* Manage def memory manually. */ - old_def->opts.temporary = true; sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable); if (sql_table_def_rebuild(db, pTable) != 0) { - old_def->opts.temporary = false; > > Here I meant this: @@ -2131,8 +2131,7 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) - struct space_def *new_def; - new_def = + struct space_def *new_def = sql_ephemeral_space_def_new(pParse, > 7. Apply this: @@ -2969,10 +2968,9 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ */ if (pList == 0) { Token prevCol; - sqlite3TokenInit(&prevCol, - pTab->def->fields[ - pTab->def->field_count - 1]. - name); + struct field_def *field = + &pTab->def->fields[pTab->def->field_count - 1]; + sqlite3TokenInit(&prevCol, field->name); > 8. When you added cleanup label, you can reuse it for another OOM on line 1904. @@ -1842,7 +1842,7 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ - for (i = 0, pCol = aCol; i < nCol && !db->mallocFailed; i++, pCol++) { + for (i = 0, pCol = aCol; i < nCol; i++, pCol++) { @@ -1902,6 +1902,7 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ if (pTable->def->fields[i].name == NULL) { sqlite3OomFault(db); + goto cleanup; } else { > 9. Please, do not ignore comments. > Looks like it can be on malloc sometimes, so you should write a comment about it - > why do you need this cleaning. @@ -1912,6 +1912,10 @@ cleanup: sqlite3HashClear(&ht); int rc = db->mallocFailed ? SQLITE_NOMEM_BKPT : SQLITE_OK; if (rc != SQLITE_OK) { + /* + * pTable->def could be not temporal in + * sqlite3ViewGetColumnNames so we need clean-up. + */ > 10. Still out of 66. And wrong end: '* */' must be '*/'.@@ -4714,9 +4718,9 @@ selectExpander(Walker * pWalker, Select * p) - * Will be overwritten with pointer as unique - * identifier. - * */ + * Will be overwritten with pointer as + * unique identifier. + */ > 11. Out of 66. @@ -4729,15 +4732,17 @@ selectExpander(Walker * pWalker, Select * p) - /* Rewrite old name with correct pointer. */ + /* + * Rewrite old name with correct pointer. + */