[tarantool-patches] Re: [PATCH] sql: remove struct Table

["n.pettik" <korablev@tarantool.org>]

> On 1 Feb 2019, at 14:05, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
> 
> Thank you for the review! Sorry for all these code style errors.
> I consider review changes minor, so I put diff b/w first and
> second version of patch at the end of the letter.
> (second commit on the branch 'review fixes' would be squashed)

Don’t do it next time, pls. Instead, inline fixes as an answers to comments,
especially when it comes for huge diff ( 209 insertions(+), 259 deletions(-))
Otherwise, it takes a while to track fixed chunks of code in a whole diff.

>>> Completely removes struct Table. Also the
>>> patch simplifies memory management as in
>>> many cases struct space (which replaces
>>> struct Table) is allocated on region
>>> and shouldn't be explicitly freed.
>> Feel free to use up to 72 chars in commit message body.
>> 
>> sql: remove struct Table
>> 
>> Completely removes struct Table. Also the patch simplifies memory
>> management as in many cases struct space (which replaces struct Table)
>> is allocated on region and shouldn't be explicitly freed.
>> 
>> Note that after commit subject's prefix we use lower-case.
>> 
> Fixed.
>> Also, for some reason content of this letter looks very strange:
>> at least it contains unusual fonts… I am not complaining but
>> are you sure that you set up git-email correctly?
>> 

It still looks weird. Please, verify that text you send is plain.

>>> +
>>>  #if SQLITE_MAX_COLUMN
>>> -	if ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) {
>>> +	if ((int)space->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) {
>>>  		sqlite3ErrorMsg(pParse, "too many columns on %s",
>>> -				p->def->name);
>>> +				space->def->name);
>>>  		return;
>>>  	}
>>>  #endif
>>> -	/*
>>> +	/**

One more nit: we start comments from single star if they appear inside functions. 

>>> @@ -435,7 +428,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>>  }
>>>  
>>>  void
>>> -sql_generate_row_delete(struct Parse *parse, struct Table *table,
>>> +sql_generate_row_delete(struct Parse *parse, struct space *space,
>>>  			struct sql_trigger *trigger_list, int cursor,
>>>  			int reg_pk, short npk, bool need_update_count,
>>>  			enum on_conflict_action onconf, u8 mode,
>>> @@ -464,31 +457,31 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
>>>  	/* If there are any triggers to fire, allocate a range of registers to
>>>  	 * use for the old.* references in the triggers.
>>>  	 */
>>> -	if (table != NULL &&
>>> -	    (fkey_is_required(table->def->id, NULL) || trigger_list != NULL)) {
>>> +	if (space != NULL &&
>>> +		(fkey_is_required(space->def->id, NULL) ||
>>> +			trigger_list != NULL)) {
>>> 
>> Broken indentation.
> Fixed now.

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index e8354f7e4..1dbf244cc 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -458,8 +458,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
         * use for the old.* references in the triggers.
         */
        if (space != NULL &&
-           (fkey_is_required(space->def->id, NULL) ||
-            trigger_list != NULL)) {
+           (fkey_is_required(space->def->id, NULL) || trigger_list != NULL)) {
                /* Mask of OLD.* columns in use */
                /* TODO: Could use temporary registers here. */
                uint32_t mask =

>>> 
>>> +/**
>>>   * Add type and collation information to a column list based on
>>>   * a SELECT statement.
>>>   *
>>> - * The column list presumably came from selectColumnNamesFromExprList().
>>> - * The column list has only names, not types or collations.  This
>>> - * routine goes through and adds the types and collations.
>>> + * The column list presumably came from
>>> + * selectColumnNamesFromExprList(). The column list has only
>>> + * names, not types or collations.  This routine goes through
>>> + * and adds the types and collations.
>>> 
>> Why did you change this comment?
> Because the length of it's lines exceeded 65 chars.

But this fix is not related to patch: it doesn’t even contain words like ’table’.

>>> @@ -2000,23 +2006,20 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
>>>  	while (pSelect->pPrior)
>>>  		pSelect = pSelect->pPrior;
>>>  	user_session->sql_flags = savedFlags;
>>> -	Table *table = sql_ephemeral_table_new(pParse, NULL);
>>> -	if (table == NULL)
>>> +	struct space *space = sql_ephemeral_space_new(pParse, NULL);
>>> +
>>> +	if (space == NULL)
>>>  		return 0;
>>> -	/* The sqlite3ResultSetOfSelect() is only used n contexts where lookaside
>>> +	/* The sqlite3ResultSetOfSelect() is only used in contexts where lookaside
>>>  	 * is disabled
>>> 
>> Extra diff.
> Empty line is removed.

I mean not (only) empty line, but fix of the comment. Nevermind.

>>>  	 */
>>>  	assert(db->lookaside.bDisable);
>>> -	table->nTabRef = 1;
>>> -	table->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
>>> -	assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == DEFAULT_TUPLE_LOG_COUNT);
>>> -	sqlite3ColumnsFromExprList(pParse, pSelect->pEList, table);
>>> -	sqlite3SelectAddColumnTypeAndCollation(pParse, table, pSelect);
>>> +	sqlite3ColumnsFromExprList(pParse, pSelect->pEList, space->def);
>>> +	sqlite3SelectAddColumnTypeAndCollation(pParse, space, pSelect);
>>>  	if (db->mallocFailed) {
>>> -		sqlite3DeleteTable(db, table);
>>>  		return 0;

return NULL;

>>>  /**
>>>   * Generate code for a DELETE FROM statement.
>>> @@ -3603,10 +3581,11 @@ int sqlite3WhereOkOnePass(WhereInfo *, int *);
>>>  
>>>  /**
>>>   * Generate code that will extract the iColumn-th column from
>>> - * table pTab and store the column value in a register.
>>> + * table defined by space_def and store the column value in a
>>> + * register.
>>>   *
>>>   * An effort is made to store the column value in register iReg.
>>> - * This is not garanteeed for GetColumn() - the result can be
>>> + * This is not guaranteed for GetColumn() - the result can be
>>> 
>> Extra diff.
> This diff fixes typo 'garanteeed' -> ‘guaranteed'

It is not related to patch. Instead, it enlarges diff to be reviewed.

As for space_by_id() clean-up: you can make key_is_required() accept
struct space instead of space_id. You can remove space_checks_expr_list()
function.

Also this fix removes one space_by_id usage:

diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 61cfb6fd7..238ab7891 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -421,7 +421,7 @@ fkey_scan_children(struct Parse *parser, struct SrcList *src,
                VdbeCoverage(v);
        }
 
-       struct space *child_space = space_by_id(fkey->child_id);
+       struct space *child_space = src->a[0].space;

> diff --git a/src/box/
> schema.cc b/src/box/schema.cc
> 
> index 8625d92ea..2d12f01d0 100644
> --- a/src/box/
> schema.cc
> 
> +++ b/src/box/
> schema.cc
> 
> @@ -193,8 +193,8 @@ space_cache_replace(struct space *old_space, struct space *new_space)
>  			mh_strnptr_del(spaces_by_name, k, NULL);
>  		}
>  		/*
> -		 * Insert @new_space into @spaces cache, replacing
> -		 * @old_space if it's not NULL.
> +		 * Insert @updated_space into @spaces cache,
> +		 * replacing @old_space if it's not NULL.
>  		 */
>  		const struct mh_i32ptr_node_t node_p = { space_id(new_space),
>  							 new_space };
> @@ -209,7 +209,7 @@ space_cache_replace(struct space *old_space, struct space *new_space)
>  		assert(old_space_by_id == old_space);
>  		(void)old_space_by_id;
>  		/*
> -		 * Insert @new_space into @spaces_by_name cache.
> +		 * Insert @updated_space into @spaces_by_name cache.
>  		 */

Why these changes are here?

>  /**
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index cae0b3f6e..599ece6d5 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -247,7 +247,7 @@ sql_space_column_is_in_pk(struct space *space, uint32_t column)
>  void
>  sql_space_delete(struct sqlite3 *db, struct space *space)
>  {
> -	if (!space || !db || db->pnBytesFreed == 0)
> +	if (space  != NULL || db != NULL|| db->pnBytesFreed == 0)
>  		return;
>  
>  	if (space->def->opts.is_temporary) {
> @@ -310,35 +310,13 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName)
>  }
>  
>  struct index *
> -sql_table_primary_key(const struct space *space)
> +sql_space_primary_key(const struct space *space)

It is used only in build.c, you can make this function static.

The same is for sql_ephemeral_space_def_new() - lets make it static
or even inline it (since it used only once). If the latter, then you can
alloc space at once for space and def.

 void
-sqlite3DeleteTable(sqlite3 * db, Table * pTable)
+sql_space_delete(struct sqlite3 *db, struct space *space, bool is_list_del)
 {
-       /* Do not delete the table until the reference count reaches zero. */
-       if (!pTable)
-               return;
-       if (((!db || db->pnBytesFreed == 0) && (--pTable->nTabRef) > 0))
+       if (space  != NULL || db != NULL || db->pnBytesFreed == 0)
                return;

You refactored this if-clause in a wrong way.

if (!db) <=> if (db == NULL)
if (!space) <=> if (space == NULL)

Fix it since it leads to numerous leaks.

What is more, you don’t need to call this function in sqlite3SrcListDelete().
We must free index defs only during table creation. In this case, they are
stored in updated_space. Check out diff:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 599ece6d5..e6da4be97 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -232,35 +232,6 @@ sql_space_column_is_in_pk(struct space *space, uint32_t column)
        return false;
 }
 
-/**
- * Remove the memory data structures associated with the given
- * space. The only case when some parts of space must be
- * deleted explicitly is when it comes from building
- * routine (i.e. it was born during CREATE TABLE
- * processing). In this case only index defs and check
- * expressions are allocated using malloc; the rest - on region.
- * This case is identified by 'is_temporary' flag == TRUE.
- *
- * @param db Database handler.
- * @param space Space to be deleted.
- */
-void
-sql_space_delete(struct sqlite3 *db, struct space *space)
-{
-       if (space  != NULL || db != NULL|| db->pnBytesFreed == 0)
-               return;
-
-       if (space->def->opts.is_temporary) {
-               for (uint32_t i = 0; i < space->index_count; ++i)
-                       index_def_delete(space->index[i]->def);
-               /**
-                * Do not delete space and space->def allocated
-                * on region.
-                */
-               sql_expr_list_delete(db, space->def->opts.checks);
-       }
-}
-
 /*
  * Given a token, return a string that consists of the text of that
  * token.  Space to hold the returned string
@@ -2813,7 +2784,17 @@ sqlite3SrcListDelete(sqlite3 * db, SrcList * pList)
                        sqlite3DbFree(db, pItem->u1.zIndexedBy);
                if (pItem->fg.isTabFunc)
                        sql_expr_list_delete(db, pItem->u1.pFuncArg);
-               sql_space_delete(db, pItem->space);
+               /*
+                * Space is either not temporary which means that
+                * it came from space cache; or space is temporary
+                * but has no indexes and check constraints.
+                * The latter proves that it is not the space
+                * which might come from CREATE TABLE routines.
+                */
+               assert(pItem->space == NULL ||
+                      !pItem->space->def->opts.is_temporary ||
+                       (pItem->space->index == NULL &&
+                        pItem->space->def->opts.checks == NULL));
                sql_select_delete(db, pItem->pSelect);
                sql_expr_delete(db, pItem->pOn, false);
                sqlite3IdListDelete(db, pItem->pUsing);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index c9ec9b1ca..83b78507a 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3453,7 +3453,6 @@ sql_store_select(struct Parse *parse_context, struct Select *select);
 
 void
 sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
-void sql_space_delete(sqlite3 *, struct space *);
 void sqlite3Insert(Parse *, SrcList *, Select *, IdList *,
                   enum on_conflict_action);
 void *sqlite3ArrayAllocate(sqlite3 *, void *, int, int *, int *);
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 3102c75bd..99f87f81c 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -416,6 +416,27 @@ sql_token(const char *z, int *type, bool *is_reserved)
        return i;
 }
 
+/**
+ * This function is called to release parsing artifacts
+ * during table creation. The only objects allocated using
+ * malloc are index defs and check constraints.
+ * Note that this functions can't be called on ordinary
+ * space object. It is purpose to clean-up parser->updated_space.
+ *
+ * @param db Database handler.
+ * @param space Space to be deleted.
+ */
+static void
+parser_space_delete(struct sqlite3 *db, struct space *space)
+{
+       if (space == NULL || db == NULL || db->pnBytesFreed == 0)
+               return;
+       assert(space->def->opts.is_temporary);
+       for (uint32_t i = 0; i < space->index_count; ++i)
+               index_def_delete(space->index[i]->def);
+       sql_expr_list_delete(db, space->def->opts.checks);
+}
+
 /*
  * Run the parser on the given SQL string.  The parser structure is
  * passed in.  An SQLITE_ status code is returned.  If an error occurs
@@ -529,7 +550,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
                sqlite3VdbeDelete(pParse->pVdbe);
                pParse->pVdbe = 0;
        }
-       sql_space_delete(db, pParse->updated_space);
+       parser_space_delete(db, pParse->updated_space);

You can even try to move index defs and checks allocations
to the region. In this case, we won’t have to free anything at all.