[tarantool-patches] Re: [PATCH v6 2/4] sql: remove SQL fields from Table and Column
Kirill Shcherbatov
kshcherbatov at tarantool.org
Fri May 18 18:35:39 MSK 2018
> Where did you introduce this flag? AFAIR It has already been introduced
> in space opts several patches ago.
> Duplicate: in fact, it is fifth point.
Updated commit message.
> Sounds confusing to me. Mb better call it just ‘action_is_nullable’?
> It is just a suggestion.
+++ b/src/box/field_def.h
@@ -131,7 +131,7 @@ struct field_def {
};
static inline bool
-nullable_action_is_nullable(enum on_conflict_action nullable_action)
+action_is_nullable(enum on_conflict_action nullable_action)
etc.
> You don’t need carrying this line: it fits into one.
> Either this.
@@ -105,8 +105,7 @@ space_def_dup(const struct space_def *src)
- struct Expr *e =
- src->fields[i].default_value_expr;
+ struct Expr *e = src->fields[i].default_value_expr;
@@ -190,8 +189,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
- struct Expr *e =
- fields[i].default_value_expr;
+ struct Expr *e = fields[i].default_value_expr;
> And this one.
@@ -1448,8 +1448,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
for (i = 0; i < n; i++) {
const char *t;
- struct coll *coll =
- coll_by_id(pTable->def->fields[i].coll_id);
+ struct coll *coll = coll_by_id(pTable->def->fields[i].coll_id);
>
>> +
>> + struct field_def *field = &def->fields[i];
>> + const char *zToken = field->default_value;
>
> Lest use Tarantool naming conventions, i.e. don’t use Hungarian notation.
@@ -1448,15 +1448,14 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
for (i = 0; i < n; i++) {
const char *t;
- struct coll *coll =
- coll_by_id(pTable->def->fields[i].coll_id);
+ struct coll *coll = coll_by_id(pTable->def->fields[i].coll_id);
struct field_def *field = &def->fields[i];
- const char *zToken = field->default_value;
+ const char *default_str = field->default_value;
int base_len = 4;
if (coll != NULL)
base_len += 1;
- if (zToken != NULL)
+ if (default_str != NULL)
base_len += 1;
p = enc->encode_map(p, base_len);
p = enc->encode_str(p, "name", 4);
@@ -1487,9 +1486,9 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
p = enc->encode_str(p, "collation", strlen("collation"));
p = enc->encode_uint(p, coll->id);
}
- if (zToken != NULL) {
+ if (default_str != NULL) {
p = enc->encode_str(p, "default", strlen("default"));
- p = enc->encode_str(p, zToken, strlen(zToken));
+ p = enc->encode_str(p, default_str, strlen(default_str));
}
}
return (int)(p - base);
> Here smth wrong with indentations (and in other places where you use this assertion).
@@ -1463,8 +1463,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
assert(def->fields[i].is_nullable ==
- action_is_nullable(
- def->fields[i].nullable_action));
+ action_is_nullable(def->fields[i].nullable_action));
@@ -1558,8 +1557,7 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
assert(def->fields[col].is_nullable ==
- action_is_nullable(
- def->fields[col].nullable_action));
+ action_is_nullable(def->fields[col].nullable_action));
@@ -200,9 +200,8 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable ==
- action_is_nullable(
- pNew->def->fields[pNew->def->field_count -
- 1].nullable_action));
+ action_is_nullable(pNew->def->fields[
+ pNew->def->field_count - 1].nullable_action));
>
>> +
>> +struct space_def *
>> +sql_ephemeral_space_def_new(Parse *parser, const char *name)
>
> Use ‘struct’ prefix for Parse struct: you did it in declaration, tho. (*)
@@ -1696,7 +1696,7 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
struct space_def *
-sql_ephemeral_space_def_new(Parse *parser, const char *name)
+sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
@@ -1720,7 +1720,7 @@ sql_ephemeral_space_def_new(Parse *parser, const char *name)
-Table *
-sql_ephemeral_table_new(Parse *parser, const char *name)
+struct Table *
+sql_ephemeral_table_new(struct Parse *parser, const char *name)
>
> Please, provide explanation, why you call it ‘ephemeral’.
> I mean, this function is used both for real tables and those,
> which, for instance, represent result set of selects.
> Furthermore, there is some mess between sql_table_new and
> sql_ephemeral_table_new(). I strongly recommend you to write
> comments in code concerning these points.
@@ -146,6 +146,12 @@ sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
/**
* Create and initialize a new ephemeral SQL Table object.
+ * All memory allocation operations except Table object itself
+ * are performed in the region.
+ *
+ * The 'ephemeral' means that this memory is temporal,
+ * so the table should be rebuild with sql_table_def_rebuild for further use.
+ *
* @param parser SQL Parser object.
* @param name Table to create name.
* @retval NULL on memory allocation error, Parser state changed.
@@ -156,6 +162,11 @@ sql_ephemeral_table_new(struct Parse *parser, const char *name);
/**
* Create and initialize a new ephemeral space_def object.
+ * All memory allocation operations are performed on the region.
+ *
+ * The 'ephemeral' means that this memory is temporal,
+ * so the table should be rebuild with sql_table_def_rebuild for further use.
+ *
* @param parser SQL Parser object.
* @param name Table to create name.
* @retval NULL on memory allocation error, Parser state changed.
> Why do you need to declare it beforehand?
> You can write this way:
> struct space_def *def = (struct space_def *)region_alloc(region, size);
@@ -1698,12 +1698,11 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
struct space_def *
sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
{
- struct space_def *def = NULL;
struct region *region = &fiber()->gc;
size_t name_len = name != NULL ? strlen(name) : 0;
uint32_t dummy;
size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy, &dummy);
- def = (struct space_def *)region_alloc(region, size);
+ struct space_def *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");
> The same as (*).@@ -1740,12 +1739,12 @@ int
sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
{
struct space_def *old_def = pTable->def;
- struct space_def *new_def = NULL;
- new_def = space_def_new(old_def->id, old_def->uid,
- old_def->field_count, old_def->name,
- strlen(old_def->name), old_def->engine_name,
- strlen(old_def->engine_name), &old_def->opts,
- old_def->fields, old_def->field_count);
+ struct space_def *new_def =
+ space_def_new(old_def->id, old_def->uid,
+ old_def->field_count, old_def->name,
+ strlen(old_def->name), old_def->engine_name,
+ strlen(old_def->engine_name), &old_def->opts,
+ old_def->fields, old_def->field_count);
>
> Did you mean ’Name of table to be created’?
- * @param name Table to create name.
+ * @param name Name of table to be created.
> I may already mention, but don’t write comments
> 'just for comments’. I think it is a good place to explain,
> for example, difference between ephemeral (btw, I don’t like this name)
> table and ‘ordinary’ one.
Accounted above.
>> + if (!pTable->def->opts.temporary)
>
> You would better describe somewhere that you are using
> this field as a sign of def allocated on region.
> It can be misleading since it is used for ephemeral spaces
> which def if allocated on regular malloc.
+++ b/src/box/space_def.h
@@ -47,6 +47,8 @@ struct space_opts {
* - it is empty at server start
* - changes are not written to WAL
* - changes are not part of a snapshot
+ * - in SQL: space_def memory is allocated on region and
+ * does not require manual release.
*/
@@ -166,6 +167,7 @@ sql_ephemeral_table_new(struct Parse *parser, const char *name);
*
* The 'ephemeral' means that this memory is temporal,
* so the table should be rebuild with sql_table_def_rebuild for further use.
+ * (opts.temporary flag is set 'true' to indicate this property)
> The same is here: add comment explaining this assert.
@@ -646,7 +646,6 @@ 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 ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) {
sqlite3ErrorMsg(pParse, "too many columns on %s",
@@ -654,6 +653,12 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
return;
}
#endif
+ /*
+ * As sql_field_retrieve will allocate memory on region
+ * ensure that p->def is also temporal and would be rebuilded or
+ * dropped.
+ */
+ assert(p->def->opts.temporary);
if (sql_field_retrieve(pParse, p,
(uint32_t) p->def->field_count) == NULL)
return;
>
>> @@ -1937,7 +1920,9 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
>> if (pSelect) {
>> zStmt = createTableStmt(db, p);
>> } else {
>> - Token *pEnd2 = p->pSelect ? &pParse->sLastToken : pEnd;
>> + assert(p->def->opts.is_view == (p->pSelect != NULL));
>
> Too many same assertions in one function. Do you really need them all?
@@ -1895,7 +1895,6 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
if (pSelect) {
zStmt = createTableStmt(db, p);
} else {
- assert(p->def->opts.is_view == (p->pSelect != NULL));
@@ -1956,7 +1955,6 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
#ifndef SQLITE_OMIT_ALTERTABLE
- assert(p->def->opts.is_view == (p->pSelect != NULL));
>
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index 0c86761..119940c 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -48,7 +48,7 @@ static int exprCodeVector(Parse * pParse, Expr * p, int *piToFree);
>> char
>> sqlite3TableColumnAffinity(Table * pTab, int iCol)
>> {
>> - assert(iCol < pTab->nCol);
>> + assert(iCol < (int)pTab->def->field_count);
>> return iCol >= 0 ? pTab->aCol[iCol].affinity : SQLITE_AFF_INTEGER;
>> }
>>
>> @@ -2233,7 +2233,8 @@ isCandidateForInOpt(Expr * pX)
>> return 0; /* FROM is not a subquery or view */
>> pTab = pSrc->a[0].pTab;
>> assert(pTab != 0);
>> - assert(pTab->pSelect == 0); /* FROM clause is not a view */
>> + assert(pTab->def->opts.is_view == (pTab->pSelect != NULL));
>> + assert(!pTab->def->opts.is_view); /* FROM clause is not a view */
>
> Don’t put comment right to code: move it up.
@@ -2238,7 +2238,8 @@ isCandidateForInOpt(Expr * pX)
assert(pTab->def->opts.is_view == (pTab->pSelect != NULL));
- assert(!pTab->def->opts.is_view); /* FROM clause is not a view */
+ /* FROM clause is not a view */
+ assert(!pTab->def->opts.is_view);
> I would mention in commit message about the fact that
> now almost within parsing context is allocated on region,
> and at the end of parsing region is truncated. Or, at least,
> in comments to sql_parser_destroy()/sql_parser_create()
@@ -4157,6 +4157,8 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
/**
* Initialize a new parser object.
+ * A number of service allocations are performed on the region, which is also
+ * cleared in the destroy function.
* @param parser object to initialize.
>> + /* NULL nullable_action should math is_nullable flag. */
> Math? I can’t understand this comment. Rephrase it pls.
- /* NULL nullable_action should math is_nullable flag. */
+ /*
+ * Set is_nullable flag for fields with default NULL nullable_action to
+ * be consistent.
+ */
> Why do you declare it as uin32_t and do cast?
> region_alloc and memcpy take it as size_t arg, btw.
- uint32_t name_len = (uint32_t)strlen(zName);
+ size_t name_len = strlen(zName);
More information about the Tarantool-patches
mailing list