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 0498225764 for ; Mon, 4 Feb 2019 14:22:13 -0500 (EST) 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 UFHIXaND8lbo for ; Mon, 4 Feb 2019 14:22:12 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 835642502B for ; Mon, 4 Feb 2019 14:22:12 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH] sql: remove struct Table From: "n.pettik" In-Reply-To: <9f612f00-6222-e3cb-642c-04bfec116fd4@tarantool.org> Date: Mon, 4 Feb 2019 22:22:09 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <3438F900-EF6D-4475-B3F1-BD4971F78186@tarantool.org> References: <4c2b87b5-9e83-f96f-4350-edf7a12e5593@tarantool.org> <9f612f00-6222-e3cb-642c-04bfec116fd4@tarantool.org> 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: Ivan Koptelov > On 1 Feb 2019, at 14:05, Ivan Koptelov = wrote: >=20 > Thank you for the review! Sorry for all these code style errors. > I consider review changes minor, so I put diff b/w first and > second version of patch at the end of the letter. > (second commit on the branch 'review fixes' would be squashed) Don=E2=80=99t do it next time, pls. Instead, inline fixes as an answers = to comments, especially when it comes for huge diff ( 209 insertions(+), 259 = deletions(-)) Otherwise, it takes a while to track fixed chunks of code in a whole = diff. >>> Completely removes struct Table. Also the >>> patch simplifies memory management as in >>> many cases struct space (which replaces >>> struct Table) is allocated on region >>> and shouldn't be explicitly freed. >> Feel free to use up to 72 chars in commit message body. >>=20 >> sql: remove struct Table >>=20 >> Completely removes struct Table. Also the patch simplifies memory >> management as in many cases struct space (which replaces struct = Table) >> is allocated on region and shouldn't be explicitly freed. >>=20 >> Note that after commit subject's prefix we use lower-case. >>=20 > Fixed. >> Also, for some reason content of this letter looks very strange: >> at least it contains unusual fonts=E2=80=A6 I am not complaining but >> are you sure that you set up git-email correctly? >>=20 It still looks weird. Please, verify that text you send is plain. >>> + >>> #if SQLITE_MAX_COLUMN >>> - if ((int)p->def->field_count + 1 > = db->aLimit[SQLITE_LIMIT_COLUMN]) { >>> + if ((int)space->def->field_count + 1 > = db->aLimit[SQLITE_LIMIT_COLUMN]) { >>> sqlite3ErrorMsg(pParse, "too many columns on %s", >>> - p->def->name); >>> + space->def->name); >>> return; >>> } >>> #endif >>> - /* >>> + /** One more nit: we start comments from single star if they appear inside = functions.=20 >>> @@ -435,7 +428,7 @@ sql_table_delete_from(struct Parse *parse, = struct SrcList *tab_list, >>> } >>> =20 >>> void >>> -sql_generate_row_delete(struct Parse *parse, struct Table *table, >>> +sql_generate_row_delete(struct Parse *parse, struct space *space, >>> struct sql_trigger *trigger_list, int cursor, >>> int reg_pk, short npk, bool need_update_count, >>> enum on_conflict_action onconf, u8 mode, >>> @@ -464,31 +457,31 @@ sql_generate_row_delete(struct Parse *parse, = struct Table *table, >>> /* If there are any triggers to fire, allocate a range of = registers to >>> * use for the old.* references in the triggers. >>> */ >>> - if (table !=3D NULL && >>> - (fkey_is_required(table->def->id, NULL) || trigger_list !=3D = NULL)) { >>> + if (space !=3D NULL && >>> + (fkey_is_required(space->def->id, NULL) || >>> + trigger_list !=3D NULL)) { >>>=20 >> Broken indentation. > Fixed now. diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index e8354f7e4..1dbf244cc 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -458,8 +458,7 @@ sql_generate_row_delete(struct Parse *parse, struct = space *space, * use for the old.* references in the triggers. */ if (space !=3D NULL && - (fkey_is_required(space->def->id, NULL) || - trigger_list !=3D NULL)) { + (fkey_is_required(space->def->id, NULL) || trigger_list !=3D = NULL)) { /* Mask of OLD.* columns in use */ /* TODO: Could use temporary registers here. */ uint32_t mask =3D >>>=20 >>> +/** >>> * Add type and collation information to a column list based on >>> * a SELECT statement. >>> * >>> - * The column list presumably came from = selectColumnNamesFromExprList(). >>> - * The column list has only names, not types or collations. This >>> - * routine goes through and adds the types and collations. >>> + * The column list presumably came from >>> + * selectColumnNamesFromExprList(). The column list has only >>> + * names, not types or collations. This routine goes through >>> + * and adds the types and collations. >>>=20 >> Why did you change this comment? > Because the length of it's lines exceeded 65 chars. But this fix is not related to patch: it doesn=E2=80=99t even contain = words like =E2=80=99table=E2=80=99. >>> @@ -2000,23 +2006,20 @@ sqlite3ResultSetOfSelect(Parse * pParse, = Select * pSelect) >>> while (pSelect->pPrior) >>> pSelect =3D pSelect->pPrior; >>> user_session->sql_flags =3D savedFlags; >>> - Table *table =3D sql_ephemeral_table_new(pParse, NULL); >>> - if (table =3D=3D NULL) >>> + struct space *space =3D sql_ephemeral_space_new(pParse, NULL); >>> + >>> + if (space =3D=3D NULL) >>> return 0; >>> - /* The sqlite3ResultSetOfSelect() is only used n contexts where = lookaside >>> + /* The sqlite3ResultSetOfSelect() is only used in contexts where = lookaside >>> * is disabled >>>=20 >> Extra diff. > Empty line is removed. I mean not (only) empty line, but fix of the comment. Nevermind. >>> */ >>> assert(db->lookaside.bDisable); >>> - table->nTabRef =3D 1; >>> - table->tuple_log_count =3D DEFAULT_TUPLE_LOG_COUNT; >>> - assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) =3D=3D = DEFAULT_TUPLE_LOG_COUNT); >>> - sqlite3ColumnsFromExprList(pParse, pSelect->pEList, table); >>> - sqlite3SelectAddColumnTypeAndCollation(pParse, table, pSelect); >>> + sqlite3ColumnsFromExprList(pParse, pSelect->pEList, space->def); >>> + sqlite3SelectAddColumnTypeAndCollation(pParse, space, pSelect); >>> if (db->mallocFailed) { >>> - sqlite3DeleteTable(db, table); >>> return 0; return NULL; >>> /** >>> * Generate code for a DELETE FROM statement. >>> @@ -3603,10 +3581,11 @@ int sqlite3WhereOkOnePass(WhereInfo *, int = *); >>> =20 >>> /** >>> * Generate code that will extract the iColumn-th column from >>> - * table pTab and store the column value in a register. >>> + * table defined by space_def and store the column value in a >>> + * register. >>> * >>> * An effort is made to store the column value in register iReg. >>> - * This is not garanteeed for GetColumn() - the result can be >>> + * This is not guaranteed for GetColumn() - the result can be >>>=20 >> Extra diff. > This diff fixes typo 'garanteeed' -> =E2=80=98guaranteed' It is not related to patch. Instead, it enlarges diff to be reviewed. As for space_by_id() clean-up: you can make key_is_required() accept struct space instead of space_id. You can remove = space_checks_expr_list() function. Also this fix removes one space_by_id usage: diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 61cfb6fd7..238ab7891 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -421,7 +421,7 @@ fkey_scan_children(struct Parse *parser, struct = SrcList *src, VdbeCoverage(v); } =20 - struct space *child_space =3D space_by_id(fkey->child_id); + struct space *child_space =3D src->a[0].space; > diff --git a/src/box/ > schema.cc b/src/box/schema.cc >=20 > index 8625d92ea..2d12f01d0 100644 > --- a/src/box/ > schema.cc >=20 > +++ b/src/box/ > schema.cc >=20 > @@ -193,8 +193,8 @@ space_cache_replace(struct space *old_space, = struct space *new_space) > mh_strnptr_del(spaces_by_name, k, NULL); > } > /* > - * Insert @new_space into @spaces cache, replacing > - * @old_space if it's not NULL. > + * Insert @updated_space into @spaces cache, > + * replacing @old_space if it's not NULL. > */ > const struct mh_i32ptr_node_t node_p =3D { = space_id(new_space), > new_space }; > @@ -209,7 +209,7 @@ space_cache_replace(struct space *old_space, = struct space *new_space) > assert(old_space_by_id =3D=3D old_space); > (void)old_space_by_id; > /* > - * Insert @new_space into @spaces_by_name cache. > + * Insert @updated_space into @spaces_by_name cache. > */ Why these changes are here? > /** > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index cae0b3f6e..599ece6d5 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -247,7 +247,7 @@ sql_space_column_is_in_pk(struct space *space, = uint32_t column) > void > sql_space_delete(struct sqlite3 *db, struct space *space) > { > - if (!space || !db || db->pnBytesFreed =3D=3D 0) > + if (space !=3D NULL || db !=3D NULL|| db->pnBytesFreed =3D=3D = 0) > return; > =20 > if (space->def->opts.is_temporary) { > @@ -310,35 +310,13 @@ sqlite3CheckIdentifierName(Parse *pParse, char = *zName) > } > =20 > struct index * > -sql_table_primary_key(const struct space *space) > +sql_space_primary_key(const struct space *space) It is used only in build.c, you can make this function static. The same is for sql_ephemeral_space_def_new() - lets make it static or even inline it (since it used only once). If the latter, then you can alloc space at once for space and def. void -sqlite3DeleteTable(sqlite3 * db, Table * pTable) +sql_space_delete(struct sqlite3 *db, struct space *space, bool = is_list_del) { - /* Do not delete the table until the reference count reaches = zero. */ - if (!pTable) - return; - if (((!db || db->pnBytesFreed =3D=3D 0) && (--pTable->nTabRef) > = 0)) + if (space !=3D NULL || db !=3D NULL || db->pnBytesFreed =3D=3D = 0) return; You refactored this if-clause in a wrong way. if (!db) <=3D> if (db =3D=3D NULL) if (!space) <=3D> if (space =3D=3D NULL) Fix it since it leads to numerous leaks. What is more, you don=E2=80=99t need to call this function in = sqlite3SrcListDelete(). We must free index defs only during table creation. In this case, they = are stored in updated_space. Check out diff: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 599ece6d5..e6da4be97 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -232,35 +232,6 @@ sql_space_column_is_in_pk(struct space *space, = uint32_t column) return false; } =20 -/** - * Remove the memory data structures associated with the given - * space. The only case when some parts of space must be - * deleted explicitly is when it comes from building - * routine (i.e. it was born during CREATE TABLE - * processing). In this case only index defs and check - * expressions are allocated using malloc; the rest - on region. - * This case is identified by 'is_temporary' flag =3D=3D TRUE. - * - * @param db Database handler. - * @param space Space to be deleted. - */ -void -sql_space_delete(struct sqlite3 *db, struct space *space) -{ - if (space !=3D NULL || db !=3D NULL|| db->pnBytesFreed =3D=3D = 0) - return; - - if (space->def->opts.is_temporary) { - for (uint32_t i =3D 0; i < space->index_count; ++i) - index_def_delete(space->index[i]->def); - /** - * Do not delete space and space->def allocated - * on region. - */ - sql_expr_list_delete(db, space->def->opts.checks); - } -} - /* * Given a token, return a string that consists of the text of that * token. Space to hold the returned string @@ -2813,7 +2784,17 @@ sqlite3SrcListDelete(sqlite3 * db, SrcList * = pList) sqlite3DbFree(db, pItem->u1.zIndexedBy); if (pItem->fg.isTabFunc) sql_expr_list_delete(db, pItem->u1.pFuncArg); - sql_space_delete(db, pItem->space); + /* + * Space is either not temporary which means that + * it came from space cache; or space is temporary + * but has no indexes and check constraints. + * The latter proves that it is not the space + * which might come from CREATE TABLE routines. + */ + assert(pItem->space =3D=3D NULL || + !pItem->space->def->opts.is_temporary || + (pItem->space->index =3D=3D NULL && + pItem->space->def->opts.checks =3D=3D NULL)); sql_select_delete(db, pItem->pSelect); sql_expr_delete(db, pItem->pOn, false); sqlite3IdListDelete(db, pItem->pUsing); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index c9ec9b1ca..83b78507a 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3453,7 +3453,6 @@ sql_store_select(struct Parse *parse_context, = struct Select *select); =20 void sql_drop_table(struct Parse *, struct SrcList *, bool, bool); -void sql_space_delete(sqlite3 *, struct space *); void sqlite3Insert(Parse *, SrcList *, Select *, IdList *, enum on_conflict_action); void *sqlite3ArrayAllocate(sqlite3 *, void *, int, int *, int *); diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 3102c75bd..99f87f81c 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -416,6 +416,27 @@ sql_token(const char *z, int *type, bool = *is_reserved) return i; } =20 +/** + * This function is called to release parsing artifacts + * during table creation. The only objects allocated using + * malloc are index defs and check constraints. + * Note that this functions can't be called on ordinary + * space object. It is purpose to clean-up parser->updated_space. + * + * @param db Database handler. + * @param space Space to be deleted. + */ +static void +parser_space_delete(struct sqlite3 *db, struct space *space) +{ + if (space =3D=3D NULL || db =3D=3D NULL || db->pnBytesFreed =3D=3D= 0) + return; + assert(space->def->opts.is_temporary); + for (uint32_t i =3D 0; i < space->index_count; ++i) + index_def_delete(space->index[i]->def); + sql_expr_list_delete(db, space->def->opts.checks); +} + /* * Run the parser on the given SQL string. The parser structure is * passed in. An SQLITE_ status code is returned. If an error occurs @@ -529,7 +550,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, = char **pzErrMsg) sqlite3VdbeDelete(pParse->pVdbe); pParse->pVdbe =3D 0; } - sql_space_delete(db, pParse->updated_space); + parser_space_delete(db, pParse->updated_space); You can even try to move index defs and checks allocations to the region. In this case, we won=E2=80=99t have to free anything at = all.