Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column
Date: Fri, 11 May 2018 23:59:45 +0300	[thread overview]
Message-ID: <70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org> (raw)
In-Reply-To: <b0e872bbbb844bf9527aaa481f7c76894f195332.1526028449.git.kshcherbatov@tarantool.org>

Hello. Thanks for the patch! See below 43 comments.

On 11/05/2018 11:49, Kirill Shcherbatov wrote:
> 1. Removed zName, is_nullable, collation, type
> from SQL Column.
> 2. Removed zColumns, zName from SQL Table.
> 3. Refactored Parser to use def_expression directly.
> 4. Introduced is_view flag.
> 4. Introduced sql_table_def_rebuild intended for collect
> fragmented with sql_field_retrieve space_def into memory
> located in one allocation.
> 
> Part of #3272, #3218.
> ---
>   src/box/field_def.h     |   6 +
>   src/box/space_def.c     |  29 +++--
>   src/box/sql.c           | 128 +++++++++++++++----
>   src/box/sql.h           |  33 +++++
>   src/box/sql/alter.c     |  55 +++++---
>   src/box/sql/analyze.c   |  16 ++-
>   src/box/sql/build.c     | 334 ++++++++++++++++++++++++++----------------------
>   src/box/sql/delete.c    |  21 +--
>   src/box/sql/expr.c      |  14 +-
>   src/box/sql/fkey.c      |  38 +++---
>   src/box/sql/hash.c      |   5 +-
>   src/box/sql/hash.h      |   2 +-
>   src/box/sql/insert.c    |  67 +++++-----
>   src/box/sql/pragma.c    |  34 +++--
>   src/box/sql/prepare.c   |  45 +++----
>   src/box/sql/resolve.c   |  22 ++--
>   src/box/sql/select.c    | 159 +++++++++++++----------
>   src/box/sql/sqliteInt.h |  37 ++++--
>   src/box/sql/tokenize.c  |   5 +-
>   src/box/sql/treeview.c  |   2 +-
>   src/box/sql/trigger.c   |   7 +-
>   src/box/sql/update.c    |  33 ++---
>   src/box/sql/util.c      |   9 --
>   src/box/sql/vdbe.c      |   2 +-
>   src/box/sql/vdbeaux.c   |   9 +-
>   src/box/sql/where.c     |  12 +-
>   src/box/sql/wherecode.c |   4 +-
>   src/box/sql/whereexpr.c |   6 +-
>   28 files changed, 675 insertions(+), 459 deletions(-)
> 
> diff --git a/src/box/field_def.h b/src/box/field_def.h
> index dfc1950..a42beab 100644
> --- a/src/box/field_def.h
> +++ b/src/box/field_def.h
> @@ -116,6 +116,12 @@ struct field_def {
>   	struct Expr *default_value_expr;
>   };
>   
> +static inline bool
> +nullable_action_to_is_nullable(enum on_conflict_action nullable_action)
> +{
> +	return nullable_action == ON_CONFLICT_ACTION_NONE;
> +}

1. Lets remove 'to': nullable_action_is_nullable. It is too long too, but I
can not think up another.
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 22bd3ca..77c0e02 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -116,12 +117,13 @@ space_def_dup(const struct space_def *src)
>   			if (src->fields[i].default_value != NULL) {
>   				ret->fields[i].default_value = strs_pos;
>   				strs_pos += strlen(strs_pos) + 1;
> -
> -				struct Expr *e =
> -					src->fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				src->fields[i].default_value_expr;
> +			if (e != NULL) {
>   				char *expr_pos_old = expr_pos;
> -				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> +				e = sql_expr_dup(sql_get(), e, 0,
> +						 &expr_pos);

2. Same as in the previous review: garbage diff.

> @@ -201,12 +203,13 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
>   				       fields[i].default_value, len);
>   				def->fields[i].default_value[len] = 0;
>   				strs_pos += len + 1;
> -
> -				struct Expr *e =
> -					fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				fields[i].default_value_expr;
> +			if (e != NULL) {
>   				char *expr_pos_old = expr_pos;
> -				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> +				e = sql_expr_dup(sql_get(), e, 0,
> +						 &expr_pos);

3. Same.
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 166bb71..7d48cdc 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1681,3 +1697,65 @@ 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, const char *name)
> +{
> +	struct space_def *def = NULL;
> +	struct region *region = &fiber()->gc;
> +	size_t name_len = name != NULL ? strlen(name) : 0;
> +	size_t size = sizeof(struct space_def) + name_len + 1;

4. Use space_def_sizeof for this please.

> +	def = (struct space_def *)region_alloc(region, size);
> +	if (def == NULL) {
> +		diag_set(OutOfMemory, sizeof(struct tuple_dictionary),
> +			"region_alloc", "sql_ephemeral_space_def_new");
> +		parser->rc = SQL_TARANTOOL_ERROR;
> +		parser->nErr++;
> +		return NULL;
> +	} else {

5. If you return on def == NULL, then either move all the other code
under 'else' body, or remove the 'else'.

> +		memset(def, 0, size);
> +	}
> +	memcpy(def->name, name, name_len);
> +	def->name[name_len] = '\0';
> +	def->opts.temporary = true;
> +	return def;
> +}
> +
> diff --git a/src/box/sql.h b/src/box/sql.h
> index db92d80..ac8d07a 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -65,6 +65,7 @@ sql_get();
>   struct Expr;
>   struct Parse;
>   struct Select;
> +struct Table;
>   
>   /**
>    * Perform parsing of provided expression. This is done by
> @@ -143,6 +144,38 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer);
>   void
>   sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
>   
> +/**
> + * Create and initialize a new ephemeric SQL Table object.

6. Same as in the previous review - ephemeric -> ephemeral.

> + * @param parser SQL Parser object.
> + * @param name Table to create name.
> + * @retval NULL on memory allocation error, Parser state changed.
> + * @retval not NULL on success.
> + */
> +struct Table *
> +sql_ephemeral_table_new(struct Parse *parser, const char *name);
> +
> +/**
> + * Create and initialize a new ephemeric space_def object.

7. Same.

> + * @param parser SQL Parser object.
> + * @param name Table to create name.
> + * @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, 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.

8. In the function body: "/* All allocations are on region. */".
Please, fix the comment.
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 24f0965..85404ac 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -195,7 +199,12 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>   				"Cannot add a REFERENCES column with non-NULL default value");
>   		return;
>   	}
> -	if (pCol->notNull && !pDflt) {
> +	assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable ==
> +	       nullable_action_to_is_nullable(
> +		pNew->def->fields[pNew->def->field_count - 1].nullable_action));
> +
> +	if (pNew->def->fields[pNew->def->field_count - 1].nullable_action

9. Please, do not use non-boolean variables as booleans.
Use (action != ON_CONFLICT_ACTION_NONE) instead of just (action).

> +	    && !pDflt) {

10. pDflt == NULL.
> @@ -281,25 +293,30 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
>   	pNew = (Table *) sqlite3DbMallocZero(db, sizeof(Table));
>   	if (!pNew)
>   		goto exit_begin_add_column;
> +	pNew->def = space_def_dup(pTab->def);
> +	if (pNew->def == NULL) {

11. Forgot to set sqlite3OomFault(db). I know, that this code is
unreachable, but in the future it will be not.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a2b712a..a02fe89 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -291,15 +291,8 @@ sqlite3CommitInternalChanges()
>   void
>   sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable)
>   {
> -	int i;
> -	Column *pCol;
>   	assert(pTable != 0);
> -	if ((pCol = pTable->aCol) != 0) {
> -		for (i = 0; i < pTable->nCol; i++, pCol++) {
> -			sqlite3DbFree(db, pCol->zName);
> -		}
> -		sqlite3DbFree(db, pTable->aCol);
> -	}
> +	sqlite3DbFree(db, pTable->aCol);

12. Nice. Now this function consists of a single DbFree - lets just
inline it and remove sqlite3DeleteColumnNames.

> @@ -494,31 +484,19 @@ sqlite3PrimaryKeyIndex(Table * pTab)
>   
>   /**
>    * Create and initialize a new SQL Table object.
> + * All memory is allocated on region.

13. Not all - the table itself is on the heap.

> @@ -626,30 +605,35 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>   static struct field_def *
>   sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
>   {
> -	sqlite3 *db = parser->db;
>   	struct field_def *field;
>   	assert(table->def != NULL);
> -	assert(table->def->exact_field_count >= (uint32_t)table->nCol);
>   	assert(id < SQLITE_MAX_COLUMN);
>   
>   	if (id >= table->def->exact_field_count) {
> -		uint32_t columns = table->def->exact_field_count;
> -		columns = (columns > 0) ? 2 * columns : 1;
> -		field = sqlite3DbRealloc(db, table->def->fields,
> -					 columns * sizeof(table->def->fields[0]));
> +		uint32_t columns_new = table->def->exact_field_count;
> +		columns_new = (columns_new > 0) ? 2 * columns_new : 1;
> +		struct region *region = &fiber()->gc;
> +		field = region_alloc(region, columns_new *
> +				     sizeof(table->def->fields[0]));
>   		if (field == NULL) {
> -			parser->rc = SQLITE_NOMEM_BKPT;
> +			diag_set(OutOfMemory, columns_new * sizeof(table->def->fields[0]),
> +				"region_alloc", "sql_field_retrieve");

14. Out of 80 symbols.

> +			parser->rc = SQL_TARANTOOL_ERROR;
>   			parser->nErr++;
>   			return NULL;
>   		}
>   
> -		for (uint32_t i = columns / 2; i < columns; i++) {
> +		for (uint32_t i = 0; i < table->def->exact_field_count; i++) {
> +			memcpy(&field[i], &table->def->fields[i],
> +			       sizeof(struct field_def));

15. Why you can not do one memcpy(field, table->def->fields,
				sizeof(*field) * table->def->exact_field_count); ?

> +		}
> +		for (uint32_t i = columns_new / 2; i < columns_new; i++) {
>   			memcpy(&field[i], &field_def_default,
>   			       sizeof(struct field_def));
> @@ -675,42 +659,44 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
>   	sqlite3 *db = pParse->db;
>   	if ((p = pParse->pNewTable) == 0)
>   		return;
> +	assert(p->def->opts.temporary);
>   #if SQLITE_MAX_COLUMN
> -	if (p->nCol + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) {
> -		sqlite3ErrorMsg(pParse, "too many columns on %s", p->zName);
> +	if ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) {
> +		sqlite3ErrorMsg(pParse, "too many columns on %s",
> +				p->def->name);
>   		return;
>   	}
>   #endif
> -	if (sql_field_retrieve(pParse, p, (uint32_t) p->nCol) == NULL)
> +	if (sql_field_retrieve(pParse, p, (uint32_t) p->def->field_count) == NULL)

16. Out of 80 symbols.

>   		return;
> -	z = sqlite3DbMallocRaw(db, pName->n + 1);
> +	struct region *region = &fiber()->gc;
> +	z = region_alloc(region, pName->n + 1);
>   	if (z == 0)

17. Missed diag_set + pParse->nErr + pParse->rc.

> @@ -756,9 +741,11 @@ sqlite3AddNotNull(Parse * pParse, int onError)
>   {
>   	Table *p;
>   	p = pParse->pNewTable;
> -	if (p == 0 || NEVER(p->nCol < 1))
> +	if (p == 0 || NEVER(p->def->field_count < 1))
>   		return;
> -	p->aCol[p->nCol - 1].notNull = (u8) onError;
> +	p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError;
> +	p->def->fields[p->def->field_count - 1].is_nullable =
> +		(onError == ON_CONFLICT_ACTION_NONE);

18. Now you have nullable_action_to_is_nullable.
> @@ -871,38 +858,31 @@ void
>   sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
>   {
>   	Table *p;
> -	Column *pCol;
>   	sqlite3 *db = pParse->db;
>   	p = pParse->pNewTable;
> +	assert(p->def->opts.temporary);
>   	if (p != 0) {
> -		pCol = &(p->aCol[p->nCol - 1]);
>   		if (!sqlite3ExprIsConstantOrFunction
>   		    (pSpan->pExpr, db->init.busy)) {
>   			sqlite3ErrorMsg(pParse,
>   					"default value of column [%s] is not constant",
> -					pCol->zName);
> +					p->def->fields[p->def->field_count - 1].name);
>   		} else {
> -			/* A copy of pExpr is used instead of the original, as pExpr contains
> -			 * tokens that point to volatile memory. The 'span' of the expression
> -			 * is required by pragma table_info.
> -			 */
> -			Expr x;
>   			assert(p->def != NULL);
>   			struct field_def *field =
> -				&p->def->fields[p->nCol - 1];
> -			sql_expr_free(db, field->default_value_expr, false);
> -
> -			memset(&x, 0, sizeof(x));
> -			x.op = TK_SPAN;
> -			x.u.zToken = sqlite3DbStrNDup(db, (char *)pSpan->zStart,
> -						      (int)(pSpan->zEnd -
> -							    pSpan->zStart));
> -			x.pLeft = pSpan->pExpr;
> -			x.flags = EP_Skip;
> -
> -			field->default_value_expr =
> -				sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
> -			sqlite3DbFree(db, x.u.zToken);
> +				&p->def->fields[p->def->field_count - 1];
> +			struct region *region = &fiber()->gc;
> +			uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
> +			field->default_value = region_alloc(region,
> +							    default_length + 1);
> +			if (field->default_value == NULL) {

19. Lets use diag + SQL_TARANTOOL_ERROR.

> +				pParse->rc = SQLITE_NOMEM_BKPT;
> +				pParse->nErr++;
> +				return;
> +			}
> +			strncpy(field->default_value, (char *)pSpan->zStart,

20. Why do you case to char*? zStart is already const char* and strncpy takes it ok.
> @@ -1087,8 +1067,10 @@ sql_column_collation(Table *table, uint32_t column)
>   	 * SQL specific structures.
>   	 */
>   	if (space == NULL || space_index(space, 0) == NULL) {
> -		assert(column < (uint32_t)table->nCol);
> -		return table->aCol[column].coll;
> +		assert(column < (uint32_t)table->def->field_count);
> +		struct coll *coll =
> +			coll_by_id(table->def->fields[column].coll_id);
> +		return coll;

21. It is simpler and shorter to return coll_by_id() with no saving
into the variable.
> @@ -2149,11 +2142,16 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
>   			 * normally holds CHECK constraints on an ordinary table, but for
>   			 * a VIEW it holds the list of column names.
>   			 */
> -			sqlite3ColumnsFromExprList(pParse, pTable->pCheck,
> -						   &pTable->nCol,
> -						   &pTable->aCol);
> +			sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable);
> +			struct space_def *old_def = pTable->def;
> +			/* Delete it manually. */
> +			old_def->opts.temporary = true;
> +			if (sql_table_def_rebuild(db, pTable) != 0)
> +				nErr++;
> +			space_def_delete(old_def);

22. On error pTable->def is not reset, so here you would delete actually
old pTable->def, that is on a region. Do this space_def_delete in 'else' branch.
> @@ -2163,12 +2161,31 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
>   			 * the column names from the SELECT statement that defines the view.
>   			 */
>   			assert(pTable->aCol == 0);
> -			pTable->nCol = pSelTab->nCol;
> +			assert((int)pTable->def->field_count == -1);
> +			assert(pSelTab->def->opts.temporary);
> +
> +			struct space_def *old_def = pTable->def;
> +			struct space_def *new_def = NULL;

23. Useless initialization - you reset this variable on the next line.

> +			new_def = sql_ephemeral_space_def_new(pParse, old_def->name);

24. Out of 80 symbols.

> +			if (new_def == NULL) {
> +				nErr++;
> +			} else {
> +				memcpy(new_def, old_def, sizeof(struct space_def));

25. Same.

> +				new_def->dict = NULL;
> +				new_def->opts.temporary = true;
> +				new_def->fields = pSelTab->def->fields;
> +				new_def->field_count = pSelTab->def->field_count;

26. Same.
> @@ -2193,10 +2210,22 @@ sqliteViewResetAll(sqlite3 * db)
>   	for (i = sqliteHashFirst(&db->pSchema->tblHash); i;
>   	     i = sqliteHashNext(i)) {
>   		Table *pTab = sqliteHashData(i);
> -		if (pTab->pSelect) {
> +		assert(pTab->def->opts.is_view == (pTab->pSelect != NULL));
> +		if (pTab->def->opts.is_view) {
>   			sqlite3DeleteColumnNames(db, pTab);
> +			struct space_def *old_def = pTab->def;
> +			assert(old_def->opts.temporary == false);
> +			/* Ignore fields allocated on region */

27. As I can see, here pTab is the normal view, that has normal
non-temporary space_def on malloc. Not on region.

> +			pTab->def = space_def_new(old_def->id, old_def->uid,
> +						  0, old_def->name,
> +						  strlen(old_def->name),
> +						  old_def->engine_name,
> +						  strlen(old_def->engine_name),
> +						  &old_def->opts,
> +						  NULL, 0);
> +			assert(pTab->def);

28. It is not guaranteed. You must check for def == NULL and set
sqlite3OomFault(db). And do not reset pTab def, if you can not create a new one.
> @@ -2966,7 +2996,9 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
>   	 */
>   	if (pList == 0) {
>   		Token prevCol;
> -		sqlite3TokenInit(&prevCol, pTab->aCol[pTab->nCol - 1].zName);
> +		sqlite3TokenInit(&prevCol,
> +				 pTab->def->
> +					 fields[pTab->def->field_count - 1].name);

29. Please, do not wrap '->'. And it is out of 80 symbols anyway.
> diff --git a/src/box/sql/hash.c b/src/box/sql/hash.c
> index cedcb7d..b109cca 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(elem->pKey);
>   		elem = next_elem;
>   	}
>   	pH->count = 0;
> @@ -292,7 +293,7 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data)
>   			removeElementGivenHash(pH, elem, h);
>   		} else {
>   			elem->data = data;
> -			elem->pKey = pKey;
> +			elem->pKey = strdup(pKey);

30. strdup can fail. See the sqlite3HashInsert what to do in such a case.
> @@ -301,7 +302,7 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data)
>   	new_elem = (HashElem *) sqlite3Malloc(sizeof(HashElem));
>   	if (new_elem == 0)
>   		return data;
> -	new_elem->pKey = pKey;
> +	new_elem->pKey = strdup(pKey);

31. Same.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 939b5e3..c272ae1 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -146,13 +146,14 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
>   	char *zColAff = pTab->zColAff;
>   	if (zColAff == 0) {
>   		sqlite3 *db = sqlite3VdbeDb(v);
> -		zColAff = (char *)sqlite3DbMallocRaw(0, pTab->nCol + 1);
> +		zColAff = (char *)sqlite3DbMallocRaw(0,
> +						     pTab->def->field_count + 1);

32. Out of 80.
> @@ -611,10 +612,10 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>   		ipkColumn = pTab->iPKey;
>   	}
>   
> -	if (pColumn == 0 && nColumn && nColumn != (pTab->nCol)) {
> +	if (pColumn == 0 && nColumn && nColumn != ((int)pTab->def->field_count)) {

33. Same.
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 2ab8751..926c3dd 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -456,3 +435,21 @@ sqlite3_prepare_v2(sqlite3 * db,	/* Database handle. */
>   	assert(rc == SQLITE_OK || ppStmt == 0 || *ppStmt == 0);	/* VERIFY: F13021 */
>   	return rc;
>   }
> +
> +void
> +sql_parser_free(Parse *parser)
> +{
> +	if (parser == NULL)
> +		return;

34. It is never == NULL as I can see.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 5a50413..32a8e08 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1661,14 +1661,15 @@ columnTypeImpl(NameContext * pNC, Expr * pExpr,
>   			} else if (pTab->pSchema) {
>   				/* A real table */
>   				assert(!pS);
> -				assert(iCol >= 0 && iCol < pTab->nCol);
> +				assert(iCol >= 0 && iCol <
> +						    (int)pTab->def->field_count);

35. Out of 80.
> @@ -1819,11 +1818,25 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
>   		testcase(aCol == 0);
>   	} else {
>   		nCol = 0;
> -		aCol = 0;
> +		aCol = NULL;
>   	}
>   	assert(nCol == (i16) nCol);
> -	*pnCol = nCol;
> -	*paCol = aCol;
> +
> +	/*
> +	 * This should be a table without resolved columns.
> +	 * sqlite3ViewGetColumnNames could use it to resolve
> +	 * names for existent table.

36. existent -> existing.

> +	 */
> +	assert(pTable->def->fields == NULL);

37. Lets better check def->opts.is_temporary. field == NULL means nothing.

> +	struct region *region = &fiber()->gc;
> +	pTable->def->fields =
> +		region_alloc(region, nCol * sizeof(pTable->def->fields[0]));

38. region_alloc can fail - set
diag + pParse->rc = SQL_TARANTOOL_ERROR + pParse->nErr++;

or

sqlite3OomFault().

Unfortunately we still have no standard way to handle OOM in SQL.
> @@ -1874,22 +1887,28 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
>   			if (cnt > 3)
>   				sqlite3_randomness(sizeof(cnt), &cnt);
>   		}
> -		pCol->zName = zName;
> -		if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) {
> +		uint32_t name_len = (uint32_t)strlen(zName);
> +		if (zName != NULL && sqlite3HashInsert(&ht, zName, pCol) == pCol)
>   			sqlite3OomFault(db);
> +		pTable->def->fields[i].name =
> +			region_alloc(region, name_len + 1);
> +		if (pTable->def->fields[i].name == NULL) {
> +			sqlite3OomFault(db);
> +		} else {
> +			memcpy(pTable->def->fields[i].name, zName, name_len);
> +			pTable->def->fields[i].name[name_len] = '\0';
>   		}
>   	}
>   	sqlite3HashClear(&ht);
> -	if (db->mallocFailed) {
> -		for (j = 0; j < i; j++) {
> -			sqlite3DbFree(db, aCol[j].zName);
> -		}
> +	int rc = db->mallocFailed ? SQLITE_NOMEM_BKPT : SQLITE_OK;
> +	if (rc != SQLITE_OK) {
>   		sqlite3DbFree(db, aCol);
> -		*paCol = 0;
> -		*pnCol = 0;
> -		return SQLITE_NOMEM_BKPT;
> +		pTable->def->fields = NULL;
> +		pTable->def->field_count = 0;

39. pTable->def is on region and is not deleted together with table. You
can skip this cleaning.
> @@ -4683,18 +4701,24 @@ selectExpander(Walker * pWalker, Select * p)
>   			assert(pFrom->pTab == 0);
>   			if (sqlite3WalkSelect(pWalker, pSel))
>   				return WRC_Abort;
> +			/* Will overwritten with pointer as unique identifier. */

40. Out of 66.

41. Will *be* overwritten.

> +			const char *name = "sqlite_sq_DEADBEAFDEADBEAF";
>   			pFrom->pTab = pTab =
> -			    sqlite3DbMallocZero(db, sizeof(Table));
> -			if (pTab == 0)
> +				sql_ephemeral_table_new(pParse, name);
> +			if (pTab == NULL)
>   				return WRC_Abort;
> +			/* rewrite old name with correct pointer */

42. Same in the previous review - please, start a sentence from a capital
letter, and finish with dot.
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 8bb45c9..4fba008 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1867,15 +1867,6 @@ struct Savepoint {
>    * of this structure.
>    */
>   struct Column {
> -	char *zName;		/* Name of this column */
> -	enum field_type type;	/* Column type. */
> -	/** Collating sequence. */
> -	struct coll *coll;
> -	/**
> -	 * An ON_CONFLICT_ACTION code for handling a NOT NULL
> -	 * constraint.
> -	 */
> -	enum on_conflict_action notNull;

Niceeee.
> @@ -4153,4 +4142,24 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
>   bool
>   table_column_is_nullable(struct Table *tab, uint32_t column);
>   
> +/**
> + * Initialize a new parser object.
> + * @param parser object to initialize.
> + */
> +static inline void
> +sql_parser_create(struct Parse *parser)
> +{
> +	memset(parser, 0, PARSE_HDR_SZ);
> +	memset(PARSE_TAIL(parser), 0, PARSE_TAIL_SZ);
> +	struct region *region = &fiber()->gc;
> +	parser->region_initial_size = region_used(region);
> +}
> +
> +/**
> + * Release the parser object resources.
> + * @param parser object to release.
> + */
> +void
> +sql_parser_free(struct Parse *parser);

43. Out convention is 'create/destroy'. Please, respect it.

  reply	other threads:[~2018-05-11 20:59 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-11  8:49 [tarantool-patches] [PATCH v5 0/3] sql: refactor SQL Parser structures Kirill Shcherbatov
2018-05-11  8:49 ` [tarantool-patches] [PATCH v5 1/3] sql: fix code style in sqlite3Pragma Kirill Shcherbatov
2018-05-11 20:59   ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-11  8:49 ` [tarantool-patches] [PATCH v5 2/3] sql: remove SQL fields from Table and Column Kirill Shcherbatov
2018-05-11 20:59   ` Vladislav Shpilevoy [this message]
2018-05-14 11:20     ` [tarantool-patches] " Kirill Shcherbatov
2018-05-14 13:39       ` Vladislav Shpilevoy
2018-05-15 15:56         ` Kirill Shcherbatov
2018-05-11  8:49 ` [tarantool-patches] [PATCH v5 3/3] sql: space_def* instead of Table* in Expr Kirill Shcherbatov
2018-05-11 20:59   ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-14 11:20     ` Kirill Shcherbatov
2018-05-11  8:58 ` [tarantool-patches] Re: [PATCH v5 0/3] sql: refactor SQL Parser structures Vladislav Shpilevoy
2018-05-11 19:40   ` [tarantool-patches] Re[2]: [tarantool-patches] " Kirill Shcherbatov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox