[tarantool-patches] Re: [PATCH v4 5/7] sql: move names to server
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 3 14:08:30 MSK 2018
"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.
More information about the Tarantool-patches
mailing list