From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 223FC23CE9 for ; Mon, 14 May 2018 07:20:21 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 9k_kaPzTCQIu for ; Mon, 14 May 2018 07:20:21 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 25C3521022 for ; Mon, 14 May 2018 07:20:19 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column References: <70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org> From: Kirill Shcherbatov Message-ID: <7204bccd-3e12-f063-6717-3bbb95e2d570@tarantool.org> Date: Mon, 14 May 2018 14:20:00 +0300 MIME-Version: 1.0 In-Reply-To: <70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" > 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);