[tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon May 14 14:20:00 MSK 2018
> 1. Lets remove 'to': nullable_action_is_nullable. It is too long too, but I
> can not think up anotherstatic inline bool
-nullable_action_to_is_nullable(enum on_conflict_action nullable_action)
+nullable_action_is_nullable(enum on_conflict_action nullable_action)
> 4. Use space_def_sizeof for this please.
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 77c0e02..1fa3345 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -47,20 +47,7 @@ const struct opt_def space_opts_reg[] = {
OPT_END,
};
-/**
- * Size of the space_def.
- * @param name_len Length of the space name.
- * @param fields Fields array of space format.
- * @param field_count Space field count.
- * @param[out] names_offset Offset from the beginning of a def to
- * a field names memory.
- * @param[out] fields_offset Offset from the beginning of a def to
- * a fields array.
- * @param[out] def_expr_offset Offset from the beginning of a def
- * to a def_value_expr array.
- * @retval Size in bytes.
- */
-static inline size_t
+size_t
space_def_sizeof(uint32_t name_len, const struct field_def *fields,
uint32_t field_count, uint32_t *names_offset,
uint32_t *fields_offset, uint32_t *def_expr_offset)
diff --git a/src/box/space_def.h b/src/box/space_def.h
index 4add1e7..52447b6 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -157,6 +157,24 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
const struct space_opts *opts, const struct field_def *fields,
uint32_t field_count);
+/**
+ * Size of the space_def.
+ * @param name_len Length of the space name.
+ * @param fields Fields array of space format.
+ * @param field_count Space field count.
+ * @param[out] names_offset Offset from the beginning of a def to
+ * a field names memory.
+ * @param[out] fields_offset Offset from the beginning of a def to
+ * a fields array.
+ * @param[out] def_expr_offset Offset from the beginning of a def
+ * to a def_value_expr array.
+ * @retval Size in bytes.
+ */
+size_t
+space_def_sizeof(uint32_t name_len, const struct field_def *fields,
+ uint32_t field_count, uint32_t *names_offset,
+ uint32_t *fields_offset, uint32_t *def_expr_offset);
+
diff --git a/src/box/sql.c b/src/box/sql.c
index 7d48cdc..d8a3ef3 100644
@@ -1704,7 +1704,8 @@ sql_ephemeral_space_def_new(Parse *parser, const char *name)
struct space_def *def = NULL;
struct region *region = &fiber()->gc;
size_t name_len = name != NULL ? strlen(name) : 0;
- size_t size = sizeof(struct space_def) + name_len + 1;
+ uint32_t dummy;
+ size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy, &dummy);
def = (struct space_def *)region_alloc(region, size);
if (def == NULL) {
diag_set(OutOfMemory, sizeof(struct tuple_dictionary),
> 5. If you return on def == NULL, then either move all the other code
> under 'else' body, or remove the 'else'.
@@ -1712,9 +1713,9 @@ sql_ephemeral_space_def_new(Parse *parser, const char *name)
parser->rc = SQL_TARANTOOL_ERROR;
parser->nErr++;
return NULL;
- } else {
- memset(def, 0, size);
}
+
+ memset(def, 0, size);
> 6. Same as in the previous review - ephemeric -> ephemeral.
+++ b/src/box/sql.h
@@ -145,7 +145,7 @@ void
sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
/**
- * Create and initialize a new ephemeric SQL Table object.
+ * Create and initialize a new ephemeral SQL Table object.
> 7. Same.
@@ -155,7 +155,7 @@ struct Table *
sql_ephemeral_table_new(struct Parse *parser, const char *name);
/**
- * Create and initialize a new ephemeric space_def object.
+ * Create and initialize a new ephemeral space_def object.
> 8. In the function body: "/* All allocations are on region. */".
> Please, fix the comment.
@@ -1742,8 +1743,6 @@ int
sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
{
assert(pTable->def->opts.temporary);
-
- /* All allocations are on region. */
> 10. pDflt == NULL.
+++ b/src/box/sql/alter.c
@@ -200,11 +200,12 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
return;
}
assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable ==
- nullable_action_to_is_nullable(
- pNew->def->fields[pNew->def->field_count - 1].nullable_action));
+ nullable_action_is_nullable(
+ pNew->def->fields[pNew->def->field_count -
+ 1].nullable_action));
- if (pNew->def->fields[pNew->def->field_count - 1].nullable_action
- && !pDflt) {
+ if (!pNew->def->fields[pNew->def->field_count - 1].is_nullable
+ && pDflt == NULL) {
> 11. Forgot to set sqlite3OomFault(db). I know, that this code is
> unreachable, but in the future it will be not.
@@ -296,6 +297,7 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
pNew->def = space_def_dup(pTab->def);
if (pNew->def == NULL) {
sqlite3DbFree(db, pNew);
+ sqlite3OomFault(db);
> 12. Nice. Now this function consists of a single DbFree - lets just
> inline it and remove sqlite3DeleteColumnNames.
@@ -381,7 +370,7 @@ deleteTable(sqlite3 * db, Table * pTable)
/* Delete the Table structure itself.
*/
sqlite3HashClear(&pTable->idxHash);
- sqlite3DeleteColumnNames(db, pTable);
+ sqlite3DbFree(db, pTable->aCol);
@@ -2212,10 +2213,9 @@ sqliteViewResetAll(sqlite3 * db)
Table *pTab = sqliteHashData(i);
assert(pTab->def->opts.is_view == (pTab->pSelect != NULL));
if (pTab->def->opts.is_view) {
- sqlite3DeleteColumnNames(db, pTab);
+ sqlite3DbFree(db, pTab->aCol);
+++ b/src/box/sql/sqliteInt.h
@@ -3503,7 +3503,6 @@ int sqlite3InitCallback(void *, int, char **, char **);
void sqlite3CommitInternalChanges();
-void sqlite3DeleteColumnNames(sqlite3 *, Table *);
+++ b/src/box/sql/build.c
@@ -285,17 +285,6 @@ sqlite3CommitInternalChanges()
}
/*
- * Delete memory allocated for the column names of a table or view (the
- * Table.aCol[] array).
- */
-void
-sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable)
-{
- assert(pTable != 0);
- sqlite3DbFree(db, pTable->aCol);
-}
> 14. Out of 80 symbols.
> 15. Why you can not do one memcpy(field, table->def->fields,
> sizeof(*field) * table->def->exact_field_count); ?@@ -616,17 +605,15 @@ sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
field = region_alloc(region, columns_new *
sizeof(table->def->fields[0]));
if (field == NULL) {
- diag_set(OutOfMemory, columns_new * sizeof(table->def->fields[0]),
+ diag_set(OutOfMemory, columns_new *
+ sizeof(table->def->fields[0]),
"region_alloc", "sql_field_retrieve");
parser->rc = SQL_TARANTOOL_ERROR;
parser->nErr++;
return NULL;
}
-
- for (uint32_t i = 0; i < table->def->exact_field_count; i++) {
- memcpy(&field[i], &table->def->fields[i],
- sizeof(struct field_def));
- }
+ memcpy(field, table->def->fields,
+ sizeof(*field) * table->def->exact_field_count);
> 16. Out of 80 symbols.
> 17. Missed diag_set + pParse->nErr + pParse->rc.
@@ -667,12 +654,18 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
return;
}
#endif
- if (sql_field_retrieve(pParse, p, (uint32_t) p->def->field_count) == NULL)
+ if (sql_field_retrieve(pParse, p,
+ (uint32_t) p->def->field_count) == NULL)
return;
struct region *region = &fiber()->gc;
z = region_alloc(region, pName->n + 1);
- if (z == 0)
+ if (z == NULL) {
+ diag_set(OutOfMemory, pName->n + 1,
+ "region_alloc", "sqlite3AddColumn");
+ pParse->rc = SQL_TARANTOOL_ERROR;
+ pParse->nErr++;
return;
+ }
> 18. Now you have nullable_action_to_is_nullable.
@@ -745,7 +738,7 @@ sqlite3AddNotNull(Parse * pParse, int onError)
return;
p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError;
p->def->fields[p->def->field_count - 1].is_nullable =
- (onError == ON_CONFLICT_ACTION_NONE);
+ nullable_action_is_nullable(onError);
> 19. Lets use diag + SQL_TARANTOOL_ERROR.
> 20. Why do you case to char*? zStart is already const char* and strncpy takes it ok
@@ -876,11 +869,14 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
field->default_value = region_alloc(region,
default_length + 1);
if (field->default_value == NULL) {
- pParse->rc = SQLITE_NOMEM_BKPT;
+ diag_set(OutOfMemory, default_length + 1,
+ "region_alloc",
+ "sqlite3AddDefaultValue");
+ pParse->rc = SQL_TARANTOOL_ERROR;
pParse->nErr++;
return;
}
- strncpy(field->default_value, (char *)pSpan->zStart,
+ strncpy(field->default_value, pSpan->zStart,
default_length);
> 21. It is simpler and shorter to return coll_by_id() with no saving
> into the variable.
> 22. On error pTable->def is not reset, so here you would delete actually
> old pTable->def, that is on a region. Do this space_def_delete in 'else' branch.
@@ -2142,13 +2136,16 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
* normally holds CHECK constraints on an ordinary table, but for
* a VIEW it holds the list of column names.
*/
- sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable);
struct space_def *old_def = pTable->def;
- /* Delete it manually. */
+ /* Manage def memory manually. */
old_def->opts.temporary = true;
- if (sql_table_def_rebuild(db, pTable) != 0)
+ sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable);
+ if (sql_table_def_rebuild(db, pTable) != 0) {
+ old_def->opts.temporary = false;
nErr++;
- space_def_delete(old_def);
+ } else {
+ space_def_delete(old_def);
+ }
> 28. It is not guaranteed. You must check for def == NULL and set
> sqlite3OomFault(db). And do not reset pTab def, if you can not create a new one.
@@ -2223,8 +2223,12 @@ sqliteViewResetAll(sqlite3 * db)
strlen(old_def->engine_name),
&old_def->opts,
NULL, 0);
- assert(pTab->def);
- space_def_delete(old_def);
+ if (pTab->def == NULL) {
+ sqlite3OomFault(db);
+ pTab->def = old_def;
+ } else {
+ space_def_delete(old_def);
+ }
> 29. Please, do not wrap '->'. And it is out of 80 symbols anyway.
@@ -2997,8 +3001,8 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */
if (pList == 0) {
Token prevCol;
sqlite3TokenInit(&prevCol,
- pTab->def->
- fields[pTab->def->field_count - 1].name);
+ pTab->def->fields[
+ pTab->def->field_count - 1].name);
> 30. strdup can fail. See the sqlite3HashInsert what to do in such a case.
+++ b/src/box/sql/hash.c
@@ -293,7 +293,8 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data)
removeElementGivenHash(pH, elem, h);
} else {
elem->data = data;
- elem->pKey = strdup(pKey);
+ assert(elem->pKey != NULL);
+ assert(strcmp(elem->pKey, pKey) == 0);
}
return old_data;
}
@@ -303,6 +304,10 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data)
if (new_elem == 0)
return data;
new_elem->pKey = strdup(pKey);
+ if (new_elem->pKey == NULL) {
+ sqlite3_free(new_elem);
+ return data;
+ }
new_elem->data = data;
pH->count++;
>> +void
>> +sql_parser_free(Parse *parser)
>> +{
>> + if (parser == NULL)
>> +
> 34. It is never == NULL as I can see.
void
-sql_parser_free(Parse *parser)
+sql_parser_destroy(Parse *parser)
{
- if (parser == NULL)
- return;
+ assert(parser != NULL);
> 36. existent -> existing.
+++ b/src/box/sql/select.c
@@ -1825,12 +1825,16 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */
- * names for existent table.
+ * names for existing table.
> 37. Lets better check def->opts.is_temporary. field == NULL means nothing.
sqlite3ViewGetColumnNames calls sqlite3ColumnsFromExprList with non-temporal table in
pSel = sqlite3SelectDup(db, pTable->pSelect, 0);
if (pSel) {
branch.
It is ok, as we manually manage memory in this context.
> 38. region_alloc can fail - set
> sqlite3OomFault().
struct region *region = &fiber()->gc;
pTable->def->fields =
region_alloc(region, nCol * sizeof(pTable->def->fields[0]));
+ if (pTable->def->fields == NULL) {
+ sqlite3OomFault(db);
+ goto cleanup;
+ }
memset(pTable->def->fields, 0, nCol * sizeof(pTable->def->fields[0]));
/* NULL nullable_action should math is_nullable flag. */
for (int i = 0; i < nCol; i++)
@@ -1899,6 +1903,7 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */
pTable->def->fields[i].name[name_len] = '\0';
}
}
+cleanup:
sqlite3HashClear(&ht);
int rc = db->mallocFailed ? SQLITE_NOMEM_BKPT : SQLITE_OK;
> 40. Out of 66.
> 41. Will *be* overwritten.
> 42. Same in the previous review - please, start a sentence from a capital
> letter, and finish with dot.
- /* Will overwritten with pointer as unique identifier. */
+ /*
+ * Will be overwritten with pointer as unique
+ * identifier.
+ * */
const char *name = "sqlite_sq_DEADBEAFDEADBEAF";
pFrom->pTab = pTab =
sql_ephemeral_table_new(pParse, name);
if (pTab == NULL)
return WRC_Abort;
- /* rewrite old name with correct pointer */
+ /* Rewrite old name with correct pointer. */
> 43. Out convention is 'create/destroy'. Please, respect it.
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -671,6 +671,6 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result)
return -1;
}
*result = parser.parsed_expr;
- sql_parser_free(&parser);
+ sql_parser_destroy(&parser);
+++ b/src/box/sql/trigger.c
@@ -935,7 +935,7 @@ codeRowTrigger(Parse * pParse, /* Current parse context */
assert(!pSubParse->pZombieTab);
assert(!pSubParse->pTriggerPrg && !pSubParse->nMaxArg);
- sql_parser_free(pSubParse);
+ sql_parser_destroy(pSubParse);
+++ b/src/box/sql/sqliteInt.h
@@ -4160,6 +4159,6 @@ sql_parser_create(struct Parse *parser)
* @param parser object to release.
*/
void
-sql_parser_free(struct Parse *parser);
+sql_parser_destroy(struct Parse *parser);
More information about the Tarantool-patches
mailing list