[tarantool-patches] Re: [PATCH v6 2/4] sql: remove SQL fields from Table and Column

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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);