[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
Kirill Shcherbatov
kshcherbatov at tarantool.org
Thu Apr 19 17:07:53 MSK 2018
Have taken into account everything except general realloc/free.
sqlite3 Delete Table is mentioned in various contexts with data allocated with SQL allocator; stdlib free in the list of various SQL free is also looks strange.
>From e426855d25770f7a2e5ea20f054da971092b7c3f Mon Sep 17 00:00:00 2001
Message-Id: <e426855d25770f7a2e5ea20f054da971092b7c3f.1524146521.git.kshcherbatov at tarantool.org>
From: Kirill Shcherbatov <kshcherbatov at tarantool.org>
Date: Thu, 19 Apr 2018 17:01:53 +0300
Subject: [PATCH] Accounted Nikita's comments
---
src/box/sql/build.c | 74 ++++++++++++++++++++++++++--------------------------
src/box/sql/insert.c | 7 ++---
2 files changed, 41 insertions(+), 40 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ab6df46..3346a5b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -497,39 +497,39 @@ sqlite3PrimaryKeyIndex(Table * pTab)
/**
* Create and initialize a new SQL Table object.
- * @param pParse SQL Parser object.
- * @param zName Table to create name.
+ * @param parser SQL Parser object.
+ * @param name Table to create name.
* @retval NULL on memory allocation error, Parser state is
* changed.
* @retval not NULL on success.
*/
static Table *
-sql_table_new(Parse *pParse, char *zName)
+sql_table_new(Parse *parser, char *name)
{
- sqlite3 *db = pParse->db;
+ sqlite3 *db = parser->db;
- Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
+ Table *table = sqlite3DbMallocZero(db, sizeof(Table));
struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
&space_opts_default, NULL, 0);
- if (pTable == NULL || def == NULL) {
+ if (table == NULL || def == NULL) {
if (def != NULL)
space_def_delete(def);
- sqlite3DbFree(db, pTable);
- pParse->rc = SQLITE_NOMEM_BKPT;
- pParse->nErr++;
+ sqlite3DbFree(db, table);
+ parser->rc = SQLITE_NOMEM_BKPT;
+ parser->nErr++;
return NULL;
}
- pTable->def = def;
- pTable->zName = zName;
- pTable->iPKey = -1;
- pTable->iAutoIncPKey = -1;
- pTable->pSchema = db->pSchema;
- sqlite3HashInit(&pTable->idxHash);
- pTable->nTabRef = 1;
- pTable->nRowLogEst = 200;
+ table->def = def;
+ table->zName = name;
+ table->iPKey = -1;
+ table->iAutoIncPKey = -1;
+ table->pSchema = db->pSchema;
+ sqlite3HashInit(&table->idxHash);
+ table->nTabRef = 1;
+ table->nRowLogEst = 200;
assert(200 == sqlite3LogEst(1048576));
- return pTable;
+ return table;
}
/*
@@ -618,42 +618,42 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
/**
* Get field by id. Allocate memory if needed.
- * @param pParse SQL Parser object.
- * @param pTable SQL Table object.
+ * @param parser SQL Parser object.
+ * @param table 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)
+sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
{
- sqlite3 *db = pParse->db;
+ sqlite3 *db = parser->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;
- field = sqlite3DbRealloc(db, pTable->def->fields,
- nCol * sizeof(pTable->def->fields[0]));
+ 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]));
if (field == NULL) {
- pParse->rc = SQLITE_NOMEM_BKPT;
- pParse->nErr++;
+ parser->rc = SQLITE_NOMEM_BKPT;
+ parser->nErr++;
return NULL;
}
- for (uint32_t i = nCol / 2; i < nCol; i++) {
+ for (uint32_t i = columns / 2; i < columns; i++) {
memcpy(&field[i], &field_def_default,
sizeof(struct field_def));
}
- pTable->def->fields = field;
- pTable->def->exact_field_count = nCol;
+ table->def->fields = field;
+ table->def->exact_field_count = columns;
}
- field = &pTable->def->fields[id];
+ field = &table->def->fields[id];
return field;
}
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 067f50a..adc752b 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1809,13 +1809,14 @@ xferOptimization(Parse * pParse, /* Parser context */
SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
struct space *dest_space =
space_cache_find(dest_space_id);
+ assert(src_space != NULL && dest_space != NULL);
char *src_expr_str =
src_space->def->fields[i].default_value;
char *dest_expr_str =
dest_space->def->fields[i].default_value;
- if ((dest_expr_str == NULL) != (src_expr_str == NULL)
- || (dest_expr_str
- && strcmp(src_expr_str, dest_expr_str) != 0)
+ if ((dest_expr_str == NULL) != (src_expr_str == NULL) ||
+ (dest_expr_str &&
+ strcmp(src_expr_str, dest_expr_str) != 0)
) {
return 0; /* Default values must be the same for all columns */
}
--
2.7.4
On 19.04.2018 15:58, n.pettik wrote:
> Hello. Several minor comments. Overall, seems OK to me.
>
>> +/**
>> + * 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 is
>> + * changed.
>> + * @retval not NULL on success.
>> + */
>> +static Table *
>> +sql_table_new(Parse *pParse, char *zName)
>> +{
>
> I would use Tarantool codestyle as well for args and vars
> inside function. It is not mandatory, but would look better.
>
>> +/**
>> + * 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)
>> +{
>
> The same is here: you can use Tarantool codestyle for vars/args.
> Also, I strongly recommend you to add some explanation
> comments, in particular concerning allocating strategy.
> It doesn’t seem to be obvious (at least for independent reviewer).
>
>> + sqlite3 *db = pParse->db;
>> + struct field_def *field;
>> + assert(pTable->def);
>
> Use explicit != NULL check.
>
>> + assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
>> + assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
>
> I have grepped through code, and it seems that this 'limit'
> is valid only under SQLITE_MAX_COLUMN macros.
> What about removing this macros (i.e. we will make this feature be always enabled),
> or surrounding your code with #ifdef directives?
>
>> +
>> + if (id >= pTable->def->exact_field_count) {
>> + uint32_t nCol = pTable->def->exact_field_count;
>> + nCol = (nCol > 0) ? 2 * nCol : 1;
>> + field = sqlite3DbRealloc(db, pTable->def->fields,
>> + nCol * sizeof(pTable->def->fields[0]));
>
> What about using simple malloc/realloc? It would allow you
> to get rid of pParse argument.
>
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index b24d55b07..067f50a0a 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -1801,14 +1801,21 @@ xferOptimization(Parse * pParse, /* Parser context */
>> }
>> /* Default values for second and subsequent columns need to match. */
>> if (i > 0) {
>> - assert(pDestCol->pDflt == 0
>> - || pDestCol->pDflt->op == TK_SPAN);
>> - assert(pSrcCol->pDflt == 0
>> - || pSrcCol->pDflt->op == TK_SPAN);
>> - if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
>> - || (pDestCol->pDflt
>> - && strcmp(pDestCol->pDflt->u.zToken,
>> - pSrcCol->pDflt->u.zToken) != 0)
>> + uint32_t src_space_id =
>> + SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
>> + struct space *src_space =
>> + space_cache_find(src_space_id);
>
> space_cache_find() can return NULL pointer. I would check it on nullability somehow.
>
>> + uint32_t dest_space_id =
>> + SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
>> + struct space *dest_space =
>> + space_cache_find(dest_space_id);
>> + char *src_expr_str =
>> + src_space->def->fields[i].default_value;
>> + char *dest_expr_str =
>> + dest_space->def->fields[i].default_value;
>> + if ((dest_expr_str == NULL) != (src_expr_str == NULL)
>> + || (dest_expr_str
>
> Codestyle nitpicking: put binary operator on previous line:
> … A &&
> B
>
>
>
More information about the Tarantool-patches
mailing list