[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
Kirill Shcherbatov
kshcherbatov at tarantool.org
Wed Apr 18 10:31:02 MSK 2018
>From 0ed6b21e9e45974dcccfbcfe53675513ed731d0f Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov at 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.
>
More information about the Tarantool-patches
mailing list