[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

From 0ed6b21e9e45974dcccfbcfe53675513ed731d0f Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Wed, 18 Apr 2018 10:26:51 +0300
Subject: [PATCH] Fixes

---
 src/box/sql/alter.c     |  2 +-
 src/box/sql/build.c     | 20 +++++++-------------
 src/box/sql/fkey.c      |  1 -
 src/box/sql/sqliteInt.h |  3 ++-
 src/box/sql/update.c    |  4 ----
 5 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 39ae070..747d20e 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -161,7 +161,7 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
 
 	zTab = &pNew->zName[16];	/* Skip the "sqlite_altertab_" prefix on the name */
 	pCol = &pNew->aCol[pNew->nCol - 1];
-	assert(pNew->def);
+	assert(pNew->def != NULL);
 	pDflt = pNew->def->fields[pNew->nCol - 1].default_value_expr;
 	pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);;
 	assert(pTab);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 20e9611..f3e9a7f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -397,7 +397,7 @@ deleteTable(sqlite3 * db, Table * pTable)
 	sqlite3SelectDelete(db, pTable->pSelect);
 	sqlite3ExprListDelete(db, pTable->pCheck);
 	if (pTable->def) {
-		/* fields has been allocated on separate region */
+		/* Fields has been allocated independently. */
 		struct field_def *fields = pTable->def->fields;
 		space_def_delete(pTable->def);
 		sqlite3DbFree(db, fields);
@@ -511,7 +511,6 @@ sql_table_new(Parse *pParse, char *zName)
 	struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
 					      &space_opts_default, NULL, 0);
 	if (pTable == NULL || def == NULL) {
-		assert(db->mallocFailed);
 		if (def != NULL)
 			space_def_delete(def);
 		sqlite3DbFree(db, pTable);
@@ -521,11 +520,6 @@ sql_table_new(Parse *pParse, char *zName)
 	}
 
 	pTable->def = def;
-	/* store allocated fields count */
-	def->exact_field_count = 0;
-	def->field_count = 0;
-	pTable->def->fields = NULL;
-
 	pTable->zName = zName;
 	pTable->iPKey = -1;
 	pTable->iAutoIncPKey = -1;
@@ -640,19 +634,19 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
 
 	if (id >= pTable->def->exact_field_count) {
 		uint32_t nCol = pTable->def->exact_field_count;
-		nCol = (nCol > 0) ? 2*nCol : 1;
+		nCol = (nCol > 0) ? 2 * nCol : 1;
 		field = sqlite3DbRealloc(db, pTable->def->fields,
 					 nCol * sizeof(pTable->def->fields[0]));
 		if (field == NULL) {
-			assert(db->mallocFailed);
 			pParse->rc = SQLITE_NOMEM_BKPT;
 			pParse->nErr++;
 			return NULL;
 		}
 
-		for (uint32_t i = nCol/2; i < nCol; i++)
-		     memcpy(&field[i], &field_def_default,
-			    sizeof(struct field_def));
+		for (uint32_t i = nCol / 2; i < nCol; i++) {
+			memcpy(&field[i], &field_def_default,
+			       sizeof(struct field_def));
+		}
 
 		pTable->def->fields = field;
 		pTable->def->exact_field_count = nCol;
@@ -893,7 +887,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
 			 * is required by pragma table_info.
 			 */
 			Expr x;
-			assert(p->def);
+			assert(p->def != NULL);
 			struct field_def *field =
 				&p->def->fields[p->nCol - 1];
 			sql_expr_free(db, field->default_value_expr, false);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index d196fd4..9b88b5f 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -36,7 +36,6 @@
 #include <box/coll.h>
 #include "sqliteInt.h"
 #include "box/session.h"
-#include "box/schema.h"
 #include "tarantoolInt.h"
 
 #ifndef SQLITE_OMIT_FOREIGN_KEY
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 90b7e08..19efbf3 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1969,7 +1969,8 @@ struct Table {
 	Trigger *pTrigger;	/* List of triggers stored in pSchema */
 	Schema *pSchema;	/* Schema that contains this table */
 	Table *pNextZombie;	/* Next on the Parse.pZombieTab list */
-	struct space_def *def;	/* Space definition with Tarantool metadata */
+	/* Space definition with Tarantool metadata. */
+	struct space_def *def;
 };
 
 /*
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 8d1cfdd..2743053 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -81,12 +81,8 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg)
 		Expr *expr = NULL;
 		struct space *space =
 			space_cache_find(SQLITE_PAGENO_TO_SPACEID(pTab->tnum));
-		/* FIXME: ephemeral spaces are not present in the cache now */
 		if (space != NULL && space->def->fields != NULL)
 			expr = space->def->fields[i].default_value_expr;
-		if (expr == NULL && pTab->def != NULL)
-			expr = pTab->def->fields[i].default_value_expr;
-
 		sqlite3ValueFromExpr(sqlite3VdbeDb(v),
 				     expr,
 				     pCol->affinity, &pValue);
-- 
2.7.4


On 17.04.2018 21:30, Vladislav Shpilevoy wrote:
> And please, rename the branch as I asked you - the
> issue 3051 (lost tuple format) is not linked with the
> problem, that we solve here.
> 
> On 17/04/2018 21:29, Vladislav Shpilevoy wrote:
>> Hello. Thanks for fixes! See my 11 new comments below.
>>
>>> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
>>> index 129ef82..39ae070 100644
>>> --- a/src/box/sql/alter.c
>>> +++ b/src/box/sql/alter.c
>>> @@ -161,7 +161,8 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token 
>>> * pColDef)
>>>
>>>      zTab = &pNew->zName[16];    /* Skip the "sqlite_altertab_" prefix 
>>> on the name */
>>>      pCol = &pNew->aCol[pNew->nCol - 1];
>>> -    pDflt = pCol->pDflt;
>>> +    assert(pNew->def);
>>
>> 1. Please, use explicit != NULL, when compare pointers. In other places
>> too.
>>
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index 92f3cb6..394ee3a 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -397,6 +396,12 @@ deleteTable(sqlite3 * db, Table * pTable)
>>>      sqlite3DbFree(db, pTable->zColAff);
>>>      sqlite3SelectDelete(db, pTable->pSelect);
>>>      sqlite3ExprListDelete(db, pTable->pCheck);
>>> +    if (pTable->def) {
>>> +        /* fields has been allocated on separate region */
>>> +        struct field_def *fields = pTable->def->fields;
>>> +        space_def_delete(pTable->def);
>>> +        sqlite3DbFree(db, fields);
>>> +    }
>>
>> 2. Please, do not use 'region' term here. In Tarantool region is
>> the allocator, and this comment slightly confuses, since the
>> fields array is allocated on heap instead of region.
>>
>> More style comments: start a comment with capital letter and put
>> a dot at the end of sentences.
>>
>>> @@ -490,6 +495,49 @@ sqlite3PrimaryKeyIndex(Table * pTab)
>>>      return p;
>>>  }
>>>
>>> +/**
>>> + * Create and initialize a new SQL Table object.
>>> + * @param pParse SQL Parser object.
>>> + * @param zName Table to create name.
>>> + * @retval NULL on memory allocation error, Parser state changed.
>>> + * @retval not NULL on success.
>>> + */
>>> +static Table *
>>> +sql_table_new(Parse *pParse, char *zName)
>>> +{
>>> +    sqlite3 *db = pParse->db;
>>> +
>>> +    Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
>>> +    struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
>>> +                          &space_opts_default,
>>> +                          &field_def_default, 0);
>>> +    if (pTable == NULL || def == NULL) {
>>> +        assert(db->mallocFailed);
>>
>> 3. This assertion fails, if space_def_new is failed, because
>> it does not set db flags.
>>
>>> +    pTable->def = def;
>>> +    /* store allocated fields count */
>>> +    def->exact_field_count = 0;
>>
>> 4. Same as 2 - comments style.
>>
>>> +    def->exact_field_count = 0;
>>> +    def->field_count = 0;
>>> +    pTable->def->fields = NULL;
>>
>> 5. These fields are initialized already in space_def_new.
>>
>>> +/**
>>> + * Get field by id. Allocate memory if needed.
>>> + * @param pParse SQL Parser object.
>>> + * @param pTable SQL Table object.
>>> + * @param id column identifier.
>>> + * @retval not NULL on success.
>>> + * @retval NULL on out of memory.
>>> + */
>>> +static struct field_def *
>>> +sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
>>> +{
>>> +    sqlite3 *db = pParse->db;
>>> +    struct field_def *field;
>>> +    assert(pTable->def);
>>> +    assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
>>> +    assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
>>> +
>>> +    if (id >= pTable->def->exact_field_count) {
>>> +        uint32_t nCol = pTable->def->exact_field_count;
>>> +        nCol = (nCol > 0) ? 2*nCol : 1;
>>
>> 6. Please, put spaces around arithmetic operators.
>>
>>> +        for (uint32_t i = nCol/2; i < nCol; i++)
>>> +             memcpy(&field[i], &field_def_default,
>>> +                sizeof(struct field_def));
>>
>> 7. Same as 6. And put a 'for' body into {}, when it consists of
>> multiple lines.
>>
>>> @@ -813,7 +894,11 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * 
>>> pSpan)
>>>               * is required by pragma table_info.
>>>               */
>>>              Expr x;
>>> -            sql_expr_free(db, pCol->pDflt, false);
>>> +            assert(p->def);
>>
>> 8. Same as 1.
>>
>>> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
>>> index f56b6d9..d196fd4 100644
>>> --- a/src/box/sql/fkey.c
>>> +++ b/src/box/sql/fkey.c
>>> @@ -36,6 +36,8 @@
>>>  #include <box/coll.h>
>>>  #include "sqliteInt.h"
>>>  #include "box/session.h"
>>> +#include "box/schema.h"
>>
>> 9. Unused header.
>>
>>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>>> index 59662cf..90b7e08 100644
>>> --- a/src/box/sql/sqliteInt.h
>>> +++ b/src/box/sql/sqliteInt.h
>>> @@ -1970,6 +1969,7 @@ struct Table {
>>>      Trigger *pTrigger;    /* List of triggers stored in pSchema */
>>>      Schema *pSchema;    /* Schema that contains this table */
>>>      Table *pNextZombie;    /* Next on the Parse.pZombieTab list */
>>> +    struct space_def *def;    /* Space definition with Tarantool 
>>> metadata */
>>
>> 10. For new members please use Tarantool code style - the
>> comment above the member.
>>
>>> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
>>> index 83c05ab..8d1cfdd 100644
>>> --- a/src/box/sql/update.c
>>> +++ b/src/box/sql/update.c
>>> @@ -35,6 +35,8 @@
>>>   */
>>>  #include "sqliteInt.h"
>>>  #include "box/session.h"
>>> +#include "tarantoolInt.h"
>>> +#include "box/schema.h"
>>>
>>>  /*
>>>   * The most recently coded instruction was an OP_Column to retrieve the
>>> @@ -75,7 +77,18 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, 
>>> int iReg)
>>>          Column *pCol = &pTab->aCol[i];
>>>          VdbeComment((v, "%s.%s", pTab->zName, pCol->zName));
>>>          assert(i < pTab->nCol);
>>> -        sqlite3ValueFromExpr(sqlite3VdbeDb(v), pCol->pDflt,
>>> +
>>> +        Expr *expr = NULL;
>>> +        struct space *space =
>>> +            space_cache_find(SQLITE_PAGENO_TO_SPACEID(pTab->tnum));
>>> +        /* FIXME: ephemeral spaces are not present in the cache now */
>>> +        if (space != NULL && space->def->fields != NULL)
>>> +            expr = space->def->fields[i].default_value_expr;
>>> +        if (expr == NULL && pTab->def != NULL)
>>> +            expr = pTab->def->fields[i].default_value_expr;
>>> +
>>
>> 11. You must not use default from pTab->def after DDL is finished. As I
>> remember we discussed it verbally, so lets just remove it.
>