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 96A7024F6B for ; Fri, 11 May 2018 07:22:48 -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 eRQd0jgQJYid for ; Fri, 11 May 2018 07:22:48 -0400 (EDT) Received: from smtp2.mail.ru (smtp2.mail.ru [94.100.179.91]) (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 D9D6324F67 for ; Fri, 11 May 2018 07:22:47 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: replace KeyInfo with key_def References: <877b5a2f7886137cda487933b7de9b1774a88421.1525765048.git.kyukhin@tarantool.org> <20180510125938.o6r3gxjfxmsusl5y@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 11 May 2018 14:22:44 +0300 MIME-Version: 1.0 In-Reply-To: <20180510125938.o6r3gxjfxmsusl5y@tarantool.org> 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, Kirill Yukhin Hello. Thanks for fixes. See travis: https://travis-ci.org/tarantool/tarantool/jobs/377267310. See 10 comments below. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 029c71e..bfaf3af 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1119,10 +1131,9 @@ sql_index_collation(Index *idx, uint32_t column) > return idx->coll_array[column]; > } > > - uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum); > - struct index *index = space_index(space, index_id); > - assert(index != NULL && index->def->key_def->part_count >= column); > - return index->def->key_def->parts[column].coll; > + struct key_def *key_def = sql_index_key_def(idx); 1. Sql_index_key_def makes dup, that is not needed to get the collation. 2. It leaks. > + assert(key_def != NULL && key_def->part_count >= column); 3. Assertion can fail on OOM. > @@ -1143,10 +1154,9 @@ sql_index_column_sort_order(Index *idx, uint32_t column) > return idx->sort_order[column]; > } > > - uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum); > - struct index *index = space_index(space, index_id); > - assert(index != NULL && index->def->key_def->part_count >= column); > - return index->def->key_def->parts[column].sort_order; > + struct key_def *key_def = sql_index_key_def(idx); > + assert(key_def != NULL && key_def->part_count >= column); > + return key_def->parts[column].sort_order; 4. All the same. Maybe it is better to do not dup key_def in sql_index_key_def and just return it as is. And do dup() in caller code. > @@ -2643,14 +2650,13 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) > } else { > tnum = pIndex->tnum; > } > - pKey = sqlite3KeyInfoOfIndex(pParse, db, pIndex); > - assert(pKey != 0 || db->mallocFailed || pParse->nErr); > + struct key_def *def = sql_index_key_def(pIndex); > + assert(def != NULL || db->mallocFailed || pParse->nErr); 5. Assertion can fail - when sql_index_key_def fails to do dup(), it does not set mallocFailed and nErr. > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 3f74b93..b6fc135 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -386,10 +386,10 @@ sqlite3DeleteFrom(Parse * pParse, /* The parser context */ > iPk = pParse->nMem + 1; > pParse->nMem += nPk; > iEphCur = pParse->nTab++; > - KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, nPk, 0); > + struct key_def *def = key_def_new(nPk); 6. Nobody sets pParse->rc. Maybe it is better to declare a function like parser_key_def_new, that takes all the same as key_def + struct Parse, and sets rc = SQL_TARANTOOL_ERROR to get OOM from diag. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index cc969ca..635357b 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -1505,7 +1505,7 @@ sqlite3ExprListDup(sqlite3 * db, ExprList * p, int flags) > } > pItem->zName = sqlite3DbStrDup(db, pOldItem->zName); > pItem->zSpan = sqlite3DbStrDup(db, pOldItem->zSpan); > - pItem->sortOrder = pOldItem->sortOrder; > + pItem->sort_order = pOldItem->sort_order; > pItem->done = 0; > pItem->bSpanIsTab = pOldItem->bSpanIsTab; > pItem->u = pOldItem->u; > @@ -1766,10 +1766,13 @@ sqlite3ExprListSetSortOrder(ExprList * p, int iSortOrder) > return; > assert(p->nExpr > 0); > if (iSortOrder == SORT_ORDER_UNDEF) { > - assert(p->a[p->nExpr - 1].sortOrder == SORT_ORDER_ASC); > + assert(p->a[p->nExpr - 1].sort_order == SORT_ORDER_ASC); > return; > } > - p->a[p->nExpr - 1].sortOrder = (u8) iSortOrder; > + if (iSortOrder == 0) > + p->a[p->nExpr - 1].sort_order = SORT_ORDER_ASC; > + else > + p->a[p->nExpr - 1].sort_order = SORT_ORDER_DESC; 7. Lets make iSortOrder be enum sort_order and remove this 'if's. > @@ -2761,7 +2763,7 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ > pExpr->is_ephemeral = 1; > addr = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, > pExpr->iTable, nVal); > - pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, nVal, 1); > + struct key_def *key_def = key_def_new(nVal); 8. pParse->rc is not set to error. > @@ -2787,29 +2789,34 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ > pSelect->iLimit = 0; > testcase(pSelect-> > selFlags & SF_Distinct); > - testcase(pKeyInfo == 0); /* Caused by OOM in sqlite3KeyInfoAlloc() */ > if (sqlite3Select > (pParse, pSelect, &dest)) { > sqlite3DbFree(pParse->db, > dest.zAffSdst); > - sqlite3KeyInfoUnref(pKeyInfo); > + if (key_def != NULL) > + free(key_def); > return 0; > } > sqlite3DbFree(pParse->db, > dest.zAffSdst); > - assert(pKeyInfo != 0); /* OOM will cause exit after sqlite3Select() */ > + assert(key_def != NULL); 9. Assertion fails, if key_def == NULL. Maybe it is better to check key_def == NULL once right after key_def_new, return on error, and remove this 'if (key_def != NULL)' everywhere. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 1a34f71..95a2610 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -565,9 +565,9 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > regRec = sqlite3GetTempReg(pParse); > regCopy = sqlite3GetTempRange(pParse, nColumn); > regTempId = sqlite3GetTempReg(pParse); > - KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, 1+nColumn, 0); > + struct key_def *def = key_def_new(nColumn + 1); 10. No error code is set. I will not repeat this comment below, but the error code is not set in more places.