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 B00412303B for ; Thu, 3 May 2018 07:08:33 -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 NuPto8sQnHew for ; Thu, 3 May 2018 07:08:33 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 495CC232EE for ; Thu, 3 May 2018 07:08:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 5/7] sql: move names to server References: From: Vladislav Shpilevoy Message-ID: <6eb7f317-68b9-4258-962c-54010f415a54@tarantool.org> Date: Thu, 3 May 2018 14:08:30 +0300 MIME-Version: 1.0 In-Reply-To: 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: Kirill Shcherbatov , tarantool-patches@freelists.org "sql: move names to server" - which names? whose names? I see, that your code starts to use space name from space_def instead of name from struct Table - it is not obvious from the commit header. And it is not "move" - these names are in the server already. You just start to use them. See below 27 comments. On 28/04/2018 21:26, Kirill Shcherbatov wrote: > Part of #3272. > --- > src/box/sql.c | 38 ++++++++++++++++++++++------- > src/box/sql.h | 6 ++--- > src/box/sql/alter.c | 11 +++++---- > src/box/sql/analyze.c | 11 +++++---- > src/box/sql/build.c | 61 ++++++++++++++++++++++++++++++----------------- > src/box/sql/delete.c | 6 ++--- > src/box/sql/fkey.c | 11 +++++---- > src/box/sql/hash.c | 5 ++-- > src/box/sql/insert.c | 9 +++---- > src/box/sql/pragma.c | 4 ++-- > src/box/sql/resolve.c | 7 +++--- > src/box/sql/select.c | 63 ++++++++++++++++++++++++------------------------- > src/box/sql/sqliteInt.h | 1 - > src/box/sql/treeview.c | 2 +- > src/box/sql/trigger.c | 4 ++-- > src/box/sql/update.c | 3 ++- > src/box/sql/vdbe.c | 2 +- > src/box/sql/where.c | 5 ++-- > src/box/sql/wherecode.c | 3 ++- > src/box/sql/whereexpr.c | 2 +- > 20 files changed, 150 insertions(+), 104 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index ef11eb9..47f7cb1 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1699,12 +1699,13 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) > return space->def->fields[fieldno].default_value_expr; > } > > -struct space_def * > -sql_ephemeral_space_def_new(Parse *parser) > +static struct space_def * > +sql_ephemeral_space_def_new(Parse *parser, const char *name) > { > struct space_def *def = NULL; > struct region *region = &fiber()->gc; > - size_t size = sizeof(struct space_def) + 1; > + size_t name_len = name != NULL ? strlen(name) : 0; > + size_t size = sizeof(struct space_def) + name_len + 1; > def = (struct space_def *)region_alloc(region, size); > if (def != NULL) { > memset(def, 0, size); > @@ -1718,19 +1719,40 @@ sql_ephemeral_space_def_new(Parse *parser) > parser->nErr++; > return NULL; > } > + memcpy(def->name, name, name_len); > + def->name[name_len] = '\0'; > def->dict->refs = 1; > def->opts.temporary = true; > return def; > } > > +struct space_def * > +sql_ephemeral_space_def_clone(Parse *parser, struct space_def *old_def) > +{ > + struct space_def *new_def = NULL; > + new_def = sql_ephemeral_space_def_new(parser, old_def->name); > + if (new_def == NULL) { > + parser->rc = SQLITE_NOMEM_BKPT; > + parser->nErr++; > + return NULL; > + } > + new_def->opts = old_def->opts; > + new_def->opts.temporary = true; > + new_def->id = old_def->id; > + new_def->uid = old_def->uid; > + memcpy(new_def->engine_name, old_def->engine_name, > + strlen(old_def->engine_name)); 1. I see, that this code can be much simpler and shorter, if you will do just one memcpy(new_def, old_def, size). And now it becomes at least third place, where size of a space_def is needed. Lets declare space_def_sizeof function from space_def.c in space_def.h, make it non-static, and use here and in sql_ephemeral_space_def_new. > diff --git a/src/box/sql.h b/src/box/sql.h > index 410653b..d7cfd70 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -152,16 +152,16 @@ sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc); > * @retval not NULL on success. > */ > struct Table * > -sql_ephemeral_table_new(struct Parse *parser); > +sql_ephemeral_table_new(struct Parse *parser, const char *name); 2. Fix the comment as well. > > /** > - * Create and initialize a new ephemeric space_def object. > + * Create and initialize a new ephemeric space_def object copy. > * @param pParse SQL Parser object. > * @retval NULL on memory allocation error, Parser state changed. > * @retval not NULL on success. > */ > struct space_def * > -sql_ephemeral_space_def_new(struct Parse *parser); > +sql_ephemeral_space_def_clone(struct Parse *parser, struct space_def *old_def); 3. Same. And write a new comment for sql_ephemeral_space_def_new. > diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c > index f830a15..33a4f4d 100644 > --- a/src/box/sql/alter.c > +++ b/src/box/sql/alter.c > @@ -865,7 +865,8 @@ analyzeOneTable(Parse * pParse, /* Parser context */ > > /* Populate the register containing the index name. */ > sqlite3VdbeLoadString(v, regIdxname, zIdxName); > - VdbeComment((v, "Analysis for %s.%s", pTab->zName, zIdxName)); > + VdbeComment((v, "Analysis for %s.%s", > + pTab->def->name, zIdxName)); 4. Wrong alignment. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index e9c0686..4e0ae87 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -662,7 +660,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) > assert(p->def->opts.temporary == true); > #if SQLITE_MAX_COLUMN > if ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) { > - sqlite3ErrorMsg(pParse, "too many columns on %s", p->zName); > + sqlite3ErrorMsg(pParse, "too many columns on %s", > + p->def->name); 5. Leading white space. > @@ -1306,7 +1305,7 @@ createTableStmt(sqlite3 * db, Table * p) > } > sqlite3_snprintf(n, zStmt, "CREATE TABLE "); > k = sqlite3Strlen30(zStmt); > - identPut(zStmt, &k, p->zName); > + identPut(zStmt, &k, (char *)p->def->name); 6. Please, make identPut second argument be const char * to avoid this conversion. > @@ -1710,7 +1709,8 @@ parseTableSchemaRecord(Parse * pParse, int iSpaceId, char *zStmt) > > sqlite3VdbeAddOp4(v, > OP_String8, 0, iTop, 0, > - sqlite3DbStrDup(pParse->db, p->zName), P4_DYNAMIC); > + sqlite3DbStrDup(pParse->db, p->def->name), > + P4_DYNAMIC); 7. Wrong alignment. > @@ -1950,7 +1950,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ > > reg_seq_record = emitNewSysSequenceRecord(pParse, > reg_seq_id, > - p->zName); > + p->def->name); 8. Same. > @@ -2135,6 +2135,11 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > * a VIEW it holds the list of column names. > */ > sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable); > + struct space_def *old_def = pTable->def; > + old_def->opts.temporary = true; /* delete it manually */ 9. Comment is out of 66 symbols. Start a comment from a capital letter. Put a dot at the end of sentence. > @@ -2142,18 +2147,30 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > pSel); > } > } else if (pSelTab) { > - assert(pTable->def->opts.temporary == false); > /* CREATE VIEW name AS... without an argument list. Construct > * the column names from the SELECT statement that defines the view. > */ > assert(pTable->aCol == 0); > assert((int)pTable->def->field_count == -1); > - struct space_def *def = pSelTab->def; > - pSelTab->def = pTable->def; > - pSelTab->def->field_count = 0; > - pTable->def = def; > + assert(pSelTab->def->opts.temporary); > + > + struct space_def *old_def = pTable->def; > + struct space_def *new_def = > + sql_ephemeral_space_def_clone(pParse, old_def); > + if (new_def == NULL) { > + nErr++; > + } else { > + new_def->fields = pSelTab->def->fields; > + new_def->field_count = pSelTab->def->field_count; 10. Out of 80 symbols. > + pTable->def = new_def; > + if (sql_table_def_rebuild(db, pTable) != 0) 11. Why you can not instead of clone() + rebuild() just do one space_def_new() ? Or even space_def_dup, if old_def is not ephemeral. > diff --git a/src/box/sql/hash.c b/src/box/sql/hash.c > index cedcb7d..79f0840 100644 > --- a/src/box/sql/hash.c > +++ b/src/box/sql/hash.c > @@ -69,6 +69,7 @@ sqlite3HashClear(Hash * pH) > while (elem) { > HashElem *next_elem = elem->next; > sqlite3_free(elem); > + free((void *)elem->pKey); 12. Unnecessary cast. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 06635ee..bde0cc1 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1145,7 +1145,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ > case ON_CONFLICT_ACTION_ROLLBACK: > case ON_CONFLICT_ACTION_FAIL: { > char *zMsg = > - sqlite3MPrintf(db, "%s.%s", pTab->zName, > + sqlite3MPrintf(db, "%s.%s", 13. Leading white space. > @@ -1404,7 +1405,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ > x = pPk->aiColumn[i]; > sqlite3VdbeAddOp3(v, OP_Column, > iThisCur, x, regR + i); > - VdbeComment((v, "%s.%s", pTab->zName, > + VdbeComment((v, "%s.%s", pTab->def->name, > pTab->def->fields[ > pPk->aiColumn[i]].name)); 14. Wrong alignment. > diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > index 109c410..5d85ef7 100644 > --- a/src/box/sql/resolve.c > +++ b/src/box/sql/resolve.c > @@ -239,7 +239,8 @@ lookupName(Parse * pParse, /* The parsing context */ > for (i = 0, pItem = pSrcList->a; i < pSrcList->nSrc; > i++, pItem++) { > pTab = pItem->pTab; > - assert(pTab != 0 && pTab->zName != 0); > + assert(pTab != 0 && > + pTab->def->name != NULL); 15. Leading white space. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index fa1de9b..199caf0 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1765,7 +1765,8 @@ generateColumnNames(Parse * pParse, /* Parser context */ > } else if (fullNames) { > char *zName = 0; > zName = > - sqlite3MPrintf(db, "%s.%s", pTab->zName, > + sqlite3MPrintf(db, "%s.%s", 16. Same. > @@ -1972,6 +1962,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */ > /* > * Given a SELECT statement, generate a Table structure that describes > * the result set of that SELECT. > + * Return table with def is allocated on region. 17. Return'ed'. No 'is'. And please, use doxygen style: @param, @retval etc. > @@ -4691,12 +4681,14 @@ selectExpander(Walker * pWalker, Select * p) > for (i = 0, pFrom = pTabList->a; i < pTabList->nSrc; i++, pFrom++) { > Table *pTab; > assert(pFrom->fg.isRecursive == 0 || pFrom->pTab != 0); > - if (pFrom->fg.isRecursive) > + if (pFrom->fg.isRecursive) { > continue; > + } 18. Unnecessary diff. > assert(pFrom->pTab == 0); > #ifndef SQLITE_OMIT_CTE > - if (withExpand(pWalker, pFrom)) > + if (withExpand(pWalker, pFrom)) { > return WRC_Abort; > + } 19. Same. > @@ -4707,17 +4699,24 @@ selectExpander(Walker * pWalker, Select * p) > assert(pFrom->pTab == 0); > if (sqlite3WalkSelect(pWalker, pSel)) > return WRC_Abort; > + char *name = "sqlite_sq_DEADBEAFDEADBEAF"; 20. Write a comment why it is needed. > pFrom->pTab = pTab = > - sql_ephemeral_table_new(pParse); > + sql_ephemeral_table_new(pParse, name); > if (pTab == NULL) > return WRC_Abort; > + /* rewrite old name with correct pointer */ > + name = sqlite3MPrintf(db, "sqlite_sq_%p", (void *)pTab); > + sprintf(pTab->def->name, "%s", name); > + sqlite3DbFree(db, name); 21. Use tt_sprintf instead of sqlite3MPrintf + sqlite3DbFree. And you did not check if mprintf returned NULL. > + > pTab->nTabRef = 1; > - pTab->zName = > - sqlite3MPrintf(db, "sqlite_sq_%p", (void *)pTab); > while (pSel->pPrior) { > pSel = pSel->pPrior; > } > sqlite3ColumnsFromExprList(pParse, pSel->pEList, pTab); > + if (sql_table_def_rebuild(db, pTab) != 0) > + return WRC_Abort; 22. Why do you need rebuild? AFAIK ephemeral struct Table dies together with the parser. > @@ -4727,12 +4726,13 @@ selectExpander(Walker * pWalker, Select * p) > assert(pFrom->pTab == 0); > pFrom->pTab = pTab = > sqlite3LocateTable(pParse, 0, pFrom->zName); > - if (pTab == NULL) > + if (pTab == NULL) { > return WRC_Abort; > + } 23. Same as 18. > @@ -5390,14 +5390,13 @@ sqlite3Select(Parse * pParse, /* The parser context */ > Table *pTab = pItem->pTab; > if (pSub == 0) > continue; > - 24. Same. > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 464feee..ad00537 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -75,7 +75,8 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg) > if (!pTab->pSelect) { > sqlite3_value *pValue = 0; > Column *pCol = &pTab->aCol[i]; > - VdbeComment((v, "%s.%s", pTab->zName, pTab->def->fields[i].name)); > + VdbeComment((v, "%s.%s", pTab->def->name, > + pTab->def->fields[i].name)); 25. Same as 14. > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index fc0f84c..88f4c28 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -1639,7 +1639,8 @@ whereLoopPrint(WhereLoop * p, WhereClause * pWC) > sqlite3DebugPrintf("%c%2d.%0*llx.%0*llx", p->cId, > p->iTab, nb, p->maskSelf, nb, p->prereq & mAll); > sqlite3DebugPrintf(" %12s", > - pItem->zAlias ? pItem->zAlias : pTab->zName); > + pItem->zAlias ? pItem->zAlias : 26. Same as 15. > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index 233fde0..3da7cdb 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -1158,7 +1158,8 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t > pTabItem->addrFillSub); > pLevel->p2 = sqlite3VdbeAddOp2(v, OP_Yield, regYield, addrBrk); > VdbeCoverage(v); > - VdbeComment((v, "next row of \"%s\"", pTabItem->pTab->zName)); > + VdbeComment((v, "next row of \"%s\"", 27. Same.