[tarantool-patches] Re: [PATCH v4 5/7] sql: move names to server

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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.