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 1FAD721ABA for ; Thu, 26 Apr 2018 07:47:27 -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 4nKnuq5De1Pt for ; Thu, 26 Apr 2018 07:47:27 -0400 (EDT) Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 AC07421AB7 for ; Thu, 26 Apr 2018 07:47:26 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 4/4] sql: Region-based allocations. References: From: Vladislav Shpilevoy Message-ID: <4a62ec38-3764-28a1-3de1-4e93e1f51508@tarantool.org> Date: Thu, 26 Apr 2018 14:47:23 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. Thanks for contributing! See below 16 comments. On 25/04/2018 19:52, Kirill Shcherbatov wrote: > Function sql_field_retrieve could leak names > in error cases. Let allocate all memory with > region allocator. > > Needed for #3272. 1. It is not only 3272. It is a part of the separate region allocations issue. I do not remember the number, please, find and write here 'Part of #nnnn#' on new line. > diff --git a/src/box/sql.c b/src/box/sql.c > index 6d4255e..2893d70 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1685,22 +1685,43 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) > return space->def->fields[fieldno].default_value_expr; > } > > +struct space_def * > +sql_ephemeral_space_def_new(Parse *parser) > +{ > + struct space_def *def = NULL; > + struct region *region = &fiber()->gc; > + size_t size = sizeof(struct space_def) + 1; > + def = (struct space_def *)region_alloc(region, size); > + if (def != NULL) { > + memset(def, 0, size); > + def->dict = > + (struct tuple_dictionary *) > + region_alloc(region, > + sizeof(struct tuple_dictionary)); 2. Why do you need a dictionary? It must not be used in SQL anywhere. And please, do not mess checks on allocations error - try to check it right after allocation. It is hard to review code, where after alloc its result is checked multiple times on ==/!= NULL. If you have multiple allocations, and after fail of each one you must destroy some results of the previous ones, or set some error codes, you can use this way: if (alloc1_failed) goto error1; ... if (alloc2_failed) goto error2; ... error3: do_clean3... error2: do_clean2... error1: do_clean1... return -1; See the good example in vy_lsm_new() function. > + } > + if (def == NULL || def->dict == NULL) { > + parser->rc = SQLITE_NOMEM_BKPT; > + parser->nErr++; > + return NULL; > + } > + def->dict->refs = 1; > + def->opts.temporary = true; > + return def; > +} > + > Table * > sql_ephemeral_table_new(Parse *parser) > { > sqlite3 *db = parser->db; > struct space_def *def = NULL; > Table *table = sqlite3DbMallocZero(db, sizeof(Table)); > - if (table != NULL) { > - def = space_def_new(0, 0, 0, NULL, 0, NULL, 0, > - &space_opts_default, NULL, 0); > - } > + if (table != NULL) > + def = sql_ephemeral_space_def_new(parser); > if (def == NULL) { > sqlite3DbFree(db, table); > - parser->rc = SQLITE_NOMEM_BKPT; > - parser->nErr++; 3. Why did you remove it? Region allocation error is the same, as malloc allocation error. See details in 6. > return NULL; > } > + 4. Garbage diff. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 584e6b1..4db4356 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -386,7 +386,9 @@ deleteTable(sqlite3 * db, Table * pTable) > sqlite3DbFree(db, pTable->zColAff); > sqlite3SelectDelete(db, pTable->pSelect); > sqlite3ExprListDelete(db, pTable->pCheck); > - if (pTable->def != NULL) > + /* Do not delete pTable->def allocated not on region. */ > + assert(pTable->def != NULL); > + if (pTable->def->opts.temporary == false) 5. Boolean variables checking on true/false can be omitted. > @@ -603,29 +606,33 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr) > static struct field_def * > sql_field_retrieve(Parse *parser, Table *table, uint32_t id) > { > - sqlite3 *db = parser->db; > struct field_def *field; > assert(table->def != NULL); > 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])); > + uint32_t columns_new = table->def->exact_field_count; > + columns_new = (columns_new > 0) ? 2 * columns_new : 1; > + struct region *region = &fiber()->gc; > + field = region_alloc(region, > + columns_new * sizeof(table->def->fields[0])) 6. On region error please do diag_set error. Region error is Tarantool error, not sqlite. And on such error you must return not SQLITE_NOMEM_BKPT, but SQL_TARANTOOL_ERROR. It leads to returning to a user an error, that is set in diag. > if (field == NULL) { > parser->rc = SQLITE_NOMEM_BKPT; > parser->nErr++; > return NULL; > } > > - for (uint32_t i = columns / 2; i < columns; i++) { > + for (uint32_t i = 0; i < table->def->exact_field_count; i++) { > + memcpy(&field[i], &table->def->fields[i], > + sizeof(struct field_def)); > + } 7. It would be good to note here, that field names are not on the same region allocation, and it is ok to memcpy pointers on them. > + for (uint32_t i = columns_new / 2; i < columns_new; i++) { > memcpy(&field[i], &field_def_default, > sizeof(struct field_def)); > } > > table->def->fields = field; > - table->def->exact_field_count = columns; > + table->def->exact_field_count = columns_new; 8. This diff, and other columns->columns_new diff are garbage. Why did you rename it? The previous name was ok. > @@ -677,10 +685,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) > aNew = > sqlite3DbRealloc(db, p->aCol, > (p->def->field_count + 8) * sizeof(p->aCol[0])); > - if (aNew == 0) { > - sqlite3DbFree(db, z); > + if (aNew == 0) > return; 9. Please, use == NULL for pointers. I see, that it is left from the original code, but if you modify it, then use Tarantool code style in the new code please. > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index 2ab8751..7dc14f6 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -211,17 +211,19 @@ sqlite3InitDatabase(sqlite3 * db) > void > sqlite3ParserReset(Parse * pParse) > { > - if (pParse) { > - sqlite3 *db = pParse->db; > - sqlite3DbFree(db, pParse->aLabel); > - sqlite3ExprListDelete(db, pParse->pConstExpr); > - if (db) { > - assert(db->lookaside.bDisable >= > - pParse->disableLookaside); > - db->lookaside.bDisable -= pParse->disableLookaside; > - } > - pParse->disableLookaside = 0; > + if (pParse == NULL) > + return; > + sqlite3 *db = pParse->db; > + sqlite3DbFree(db, pParse->aLabel); > + sqlite3ExprListDelete(db, pParse->pConstExpr); > + if (db) { 10. Same as 9. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 9eeff8e..03bfcf9 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1822,9 +1822,20 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ > aCol = 0; > } > assert(nCol == (i16) nCol); > + > + struct region *region = &fiber()->gc; > assert(pTable->def->fields == NULL); > + if (pTable->def->opts.temporary == false) { 11. At first, same as 5. At second, why? I can not see this in the original code. > @@ -1877,9 +1888,16 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ > if (cnt > 3) > sqlite3_randomness(sizeof(cnt), &cnt); > } > - pTable->def->fields[i].name = zName; > - if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) { > + uint32_t zNameLen = (uint32_t)strlen(zName); 12. For new code please use Tarantool code style - no Camel case. zNameLen -> name_len. > + if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) 13. Same as 9. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 94d5c80..520b74c 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2933,6 +2933,8 @@ struct Parse { > u8 eTriggerOp; /* TK_UPDATE, TK_INSERT or TK_DELETE */ > u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */ > u8 disableTriggers; /* True to disable triggers */ > + /** Region size at the Parser launch */ 14. Please, put a dot at the end of the comment. > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index c77aa9b..9055bd0 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -664,6 +664,9 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result) > memset(&parser, 0, sizeof(parser)); > parser.db = db; > parser.parse_only = true; > + struct region *region = &fiber()->gc; > + parser.region_initial_size = region_used(region); 15. It is a second place, where a parser is created. How about creating a function sql_parser_create(struct Parser *parser), that will do this? 16. I do not see, that you removed def rebuilding from sqlite3ColumnsFromExprList - it is not needed anymore.