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 A9FAA2557C for ; Thu, 17 May 2018 13:25:41 -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 VxmA4ryWI6ay for ; Thu, 17 May 2018 13:25:41 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 DBD3E25448 for ; Thu, 17 May 2018 13:25:40 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v6 2/4] sql: remove SQL fields from Table and Column From: "n.pettik" In-Reply-To: <2cd3d86ce88d5115c5ec12611d40251fcc7968bd.1526403792.git.kshcherbatov@tarantool.org> Date: Thu, 17 May 2018 20:25:30 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <2cd3d86ce88d5115c5ec12611d40251fcc7968bd.1526403792.git.kshcherbatov@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: Kirill Shcherbatov , Vladislav Shpilevoy Disclaimer: Most of comments don=E2=80=99t seem to be vital. You may skip them, if you disagree with them. =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 1. Removed zName, is_nullable, collation, type > from SQL Column. > 2. Removed zColumns, zName from SQL Table. > 3. Refactored Parser to use def_expression directly. > 4. Introduced is_view flag. Where did you introduce this flag? AFAIR It has already been introduced in space opts several patches ago. > 4. Introduced sql_table_def_rebuild intended for collect > fragmented with sql_field_retrieve space_def into memory > located in one allocation. Duplicate: in fact, it is fifth point. > +static inline bool > +nullable_action_is_nullable(enum on_conflict_action nullable_action) Sounds confusing to me. Mb better call it just =E2=80=98action_is_nullable= =E2=80=99? It is just a suggestion. > @@ -116,10 +104,10 @@ space_def_dup(const struct space_def *src) > if (src->fields[i].default_value !=3D NULL) { > ret->fields[i].default_value =3D = strs_pos; > strs_pos +=3D strlen(strs_pos) + 1; > - > - struct Expr *e =3D > - = src->fields[i].default_value_expr; > - assert(e !=3D NULL); > + } > + struct Expr *e =3D > + src->fields[i].default_value_expr; You don=E2=80=99t need carrying this line: it fits into one. > + if (e !=3D NULL) { > char *expr_pos_old =3D expr_pos; > e =3D sql_expr_dup(sql_get(), e, 0, = &expr_pos); > assert(e !=3D NULL); > @@ -201,10 +189,10 @@ space_def_new(uint32_t id, uint32_t uid, = uint32_t exact_field_count, > fields[i].default_value, len); > def->fields[i].default_value[len] =3D 0; > strs_pos +=3D len + 1; > - > - struct Expr *e =3D > - fields[i].default_value_expr; > - assert(e !=3D NULL); > + } > + struct Expr *e =3D > + fields[i].default_value_expr; Either this. > @@ -1441,47 +1443,53 @@ int tarantoolSqlite3MakeTableFormat(Table = *pTable, void *buf) > * treat it as strict type, not affinity. */ > if (pk_idx && pk_idx->nColumn =3D=3D 1) { > int pk =3D pk_idx->aiColumn[0]; > - if (pTable->aCol[pk].type =3D=3D FIELD_TYPE_INTEGER) > + if (def->fields[pk].type =3D=3D FIELD_TYPE_INTEGER) > pk_forced_int =3D pk; > } >=20 > for (i =3D 0; i < n; i++) { > const char *t; > - struct coll *coll =3D aCol[i].coll; > - struct field_def *field =3D &pTable->def->fields[i]; > - struct Expr *def =3D field->default_value_expr; > + struct coll *coll =3D > + coll_by_id(pTable->def->fields[i].coll_id); And this one. > + > + struct field_def *field =3D &def->fields[i]; > + const char *zToken =3D field->default_value; Lest use Tarantool naming conventions, i.e. don=E2=80=99t use Hungarian = notation. > int base_len =3D 4; > if (coll !=3D NULL) > base_len +=3D 1; > - if (def !=3D NULL) > + if (zToken !=3D NULL) > base_len +=3D 1; > p =3D enc->encode_map(p, base_len); > p =3D enc->encode_str(p, "name", 4); > - p =3D enc->encode_str(p, aCol[i].zName, = strlen(aCol[i].zName)); > + p =3D enc->encode_str(p, field->name, = strlen(field->name)); > p =3D enc->encode_str(p, "type", 4); > + > + assert(def->fields[i].is_nullable =3D=3D > + nullable_action_is_nullable( > + def->fields[i].nullable_action)); Here smth wrong with indentations (and in other places where you use = this assertion). > @@ -1496,13 +1504,12 @@ int tarantoolSqlite3MakeTableFormat(Table = *pTable, void *buf) > */ > int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, = void *buf) > { > - (void)pTable; > const struct Enc *enc =3D get_enc(buf); > char *base =3D buf, *p; >=20 > bool is_view =3D false; > if (pTable !=3D NULL) > - is_view =3D pTable->pSelect !=3D NULL; > + is_view =3D pTable->def->opts.is_view; > p =3D enc->encode_map(base, is_view ? 2 : 1); > p =3D enc->encode_str(p, "sql", 3); > p =3D enc->encode_str(p, zSql, strlen(zSql)); > @@ -1523,6 +1530,9 @@ int tarantoolSqlite3MakeTableOpts(Table *pTable, = const char *zSql, void *buf) > int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf) > { > struct Column *aCol =3D pIndex->pTable->aCol; > + struct space_def *def =3D pIndex->pTable->def; In tarantoolSqlite3MakeTableFormat() you declare such var as const. Lets use everywhere const, or don=E2=80=99t use it at all. > + > +struct space_def * > +sql_ephemeral_space_def_new(Parse *parser, const char *name) Use =E2=80=98struct=E2=80=99 prefix for Parse struct: you did it in = declaration, tho. (*) Please, provide explanation, why you call it =E2=80=98ephemeral=E2=80=99. 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. > +{ > + struct space_def *def =3D NULL; Why do you need to declare it beforehand? You can write this way: struct space_def *def =3D (struct space_def *)region_alloc(region, = size); > + struct region *region =3D &fiber()->gc; > + size_t name_len =3D name !=3D NULL ? strlen(name) : 0; > + uint32_t dummy; > + size_t size =3D space_def_sizeof(name_len, NULL, 0, &dummy, = &dummy, &dummy); > + def =3D (struct space_def *)region_alloc(region, size); > + if (def =3D=3D NULL) { > + diag_set(OutOfMemory, sizeof(struct tuple_dictionary), > + "region_alloc", "sql_ephemeral_space_def_new"); > + parser->rc =3D SQL_TARANTOOL_ERROR; > + parser->nErr++; > + return NULL; You can avoid passing parse context as an argument, and check return value. Wouldn=E2=80=99t it be better? > + } > + > + memset(def, 0, size); > + memcpy(def->name, name, name_len); > + def->name[name_len] =3D '\0'; > + def->opts.temporary =3D true; > + return def; > +} > + > +Table * > +sql_ephemeral_table_new(Parse *parser, const char *name) The same as (*). > diff --git a/src/box/sql.h b/src/box/sql.h > index db92d80..3c26492 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -65,6 +65,7 @@ sql_get(); > struct Expr; > struct Parse; > struct Select; > +struct Table; >=20 > /** > * Perform parsing of provided expression. This is done by > @@ -143,6 +144,37 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, = int flags, char **buffer); > void > sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool = extern_alloc); >=20 > +/** > + * Create and initialize a new ephemeral SQL Table object. > + * @param parser SQL Parser object. > + * @param name Table to create name. Did you mean =E2=80=99Name of table to be created=E2=80=99? > + * @retval NULL on memory allocation error, Parser state changed. > + * @retval not NULL on success. > + */ I may already mention, but don=E2=80=99t write comments 'just for comments=E2=80=99. I think it is a good place to explain, for example, difference between ephemeral (btw, I don=E2=80=99t like = this name) table and =E2=80=98ordinary=E2=80=99 one. > +struct Table * > +sql_ephemeral_table_new(struct Parse *parser, const char *name); > + > +/** > + * Create and initialize a new ephemeral space_def object. > + * @param parser SQL Parser object. > + * @param name Table to create name. The same. > @@ -388,17 +370,14 @@ deleteTable(sqlite3 * db, Table * pTable) > /* Delete the Table structure itself. > */ > sqlite3HashClear(&pTable->idxHash); > - sqlite3DeleteColumnNames(db, pTable); > - sqlite3DbFree(db, pTable->zName); > + sqlite3DbFree(db, pTable->aCol); > sqlite3DbFree(db, pTable->zColAff); > sqlite3SelectDelete(db, pTable->pSelect); > sqlite3ExprListDelete(db, pTable->pCheck); > - if (pTable->def !=3D NULL) { > - /* Fields has been allocated independently. */ > - struct field_def *fields =3D pTable->def->fields; > + /* Do not delete pTable->def allocated not on region. */ > + assert(pTable->def !=3D NULL); > + 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. > @@ -675,42 +646,50 @@ sqlite3AddColumn(Parse * pParse, Token * pName, = Token * pType) > sqlite3 *db =3D pParse->db; > if ((p =3D pParse->pNewTable) =3D=3D 0) > return; > + assert(p->def->opts.temporary); The same is here: add comment explaining this assert. > @@ -1937,7 +1920,9 @@ sqlite3EndTable(Parse * pParse, /* Parse context = */ > if (pSelect) { > zStmt =3D createTableStmt(db, p); > } else { > - Token *pEnd2 =3D p->pSelect ? = &pParse->sLastToken : pEnd; > + assert(p->def->opts.is_view =3D=3D (p->pSelect = !=3D NULL)); Too many same assertions in one function. Do you really need them all? > 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 >=3D 0 ? pTab->aCol[iCol].affinity : = SQLITE_AFF_INTEGER; > } >=20 > @@ -2233,7 +2233,8 @@ isCandidateForInOpt(Expr * pX) > return 0; /* FROM is not a subquery or view */ > pTab =3D pSrc->a[0].pTab; > assert(pTab !=3D 0); > - assert(pTab->pSelect =3D=3D 0); /* FROM clause is not a view */ > + assert(pTab->def->opts.is_view =3D=3D (pTab->pSelect !=3D = NULL)); > + assert(!pTab->def->opts.is_view); /* FROM clause is not a = view */ Don=E2=80=99t put comment right to code: move it up. >=20 > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index 2ab8751..f949709 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -204,26 +204,6 @@ sqlite3InitDatabase(sqlite3 * db) > return rc; > } >=20 > - > -/* > - * Free all memory allocations in the pParse object > - */ > -void > -sqlite3ParserReset(Parse * pParse) > -{ > - if (pParse) { > - sqlite3 *db =3D pParse->db; > - sqlite3DbFree(db, pParse->aLabel); > - sqlite3ExprListDelete(db, pParse->pConstExpr); > - if (db) { > - assert(db->lookaside.bDisable >=3D > - pParse->disableLookaside); > - db->lookaside.bDisable -=3D = pParse->disableLookaside; > - } > - pParse->disableLookaside =3D 0; > - } > -} 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() > - > /* > * Compile the UTF-8 encoded SQL statement zSql into a statement = handle. > */ > @@ -241,9 +221,7 @@ sqlite3Prepare(sqlite3 * db, /* Database = handle. */ > int rc =3D SQLITE_OK; /* Result code */ > int i; /* Loop counter */ > Parse sParse; /* Parsing context */ > - > - memset(&sParse, 0, PARSE_HDR_SZ); > - memset(PARSE_TAIL(&sParse), 0, PARSE_TAIL_SZ); > + sql_parser_create(&sParse); > sParse.pReprepare =3D pReprepare; > assert(ppStmt && *ppStmt =3D=3D 0); > /* assert( !db->mallocFailed ); // not true with = SQLITE_USE_ALLOCA */ > @@ -265,6 +243,7 @@ sqlite3Prepare(sqlite3 * db, /* Database = handle. */ > * works even if READ_UNCOMMITTED is set. > */ > sParse.db =3D db; > + Redundant empty line. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 5a50413..e08d709 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -318,9 +318,9 @@ sqlite3JoinType(Parse * pParse, Token * pA, Token = * pB, Token * pC) > static int > columnIndex(Table * pTab, const char *zCol) > { > - int i; > - for (i =3D 0; i < pTab->nCol; i++) { > - if (strcmp(pTab->aCol[i].zName, zCol) =3D=3D 0) > + uint32_t i; > + for (i =3D 0; i < pTab->def->field_count; i++) { You can move definition of i inside =E2=80=98for' cycle: for (uint32_t i =3D 0; ...) > @@ -1819,13 +1818,31 @@ sqlite3ColumnsFromExprList(Parse * pParse, = /* Parsing context */ > testcase(aCol =3D=3D 0); > } else { > nCol =3D 0; > - aCol =3D 0; > + aCol =3D NULL; > } > assert(nCol =3D=3D (i16) nCol); > - *pnCol =3D nCol; > - *paCol =3D aCol; >=20 > - for (i =3D 0, pCol =3D aCol; i < nCol && !db->mallocFailed; i++, = pCol++) { > + /* > + * This should be a table without resolved columns. > + * sqlite3ViewGetColumnNames could use it to resolve > + * names for existing table. > + */ > + assert(pTable->def->fields =3D=3D NULL); > + struct region *region =3D &fiber()->gc; > + pTable->def->fields =3D > + region_alloc(region, nCol * = sizeof(pTable->def->fields[0])); > + if (pTable->def->fields =3D=3D NULL) { > + sqlite3OomFault(db); > + goto cleanup; > + } > + memset(pTable->def->fields, 0, nCol * = sizeof(pTable->def->fields[0])); > + /* NULL nullable_action should math is_nullable flag. */ Math? I can=E2=80=99t understand this comment. Rephrase it pls. > + for (int i =3D 0; i < nCol; i++) > + pTable->def->fields[i].is_nullable =3D true; > + pTable->def->field_count =3D (uint32_t)nCol; > + pTable->aCol =3D aCol; > + > + for (i =3D 0, pCol =3D aCol; i < nCol; i++, pCol++) { > /* Get an appropriate name for the column > */ > p =3D sqlite3ExprSkipCollate(pEList->a[i].pExpr); > @@ -1845,7 +1862,7 @@ sqlite3ColumnsFromExprList(Parse * pParse, = /* Parsing context */ > pTab =3D pColExpr->pTab; > if (iCol < 0) > iCol =3D pTab->iPKey; > - zName =3D pTab->aCol[iCol].zName; > + zName =3D pTab->def->fields[iCol].name; > } else if (pColExpr->op =3D=3D TK_ID) { > assert(!ExprHasProperty(pColExpr, = EP_IntValue)); > zName =3D pColExpr->u.zToken; > @@ -1874,22 +1891,34 @@ sqlite3ColumnsFromExprList(Parse * pParse, = /* Parsing context */ > if (cnt > 3) > sqlite3_randomness(sizeof(cnt), &cnt); > } > - pCol->zName =3D zName; > - if (zName && sqlite3HashInsert(&ht, zName, pCol) =3D=3D = pCol) { > + uint32_t name_len =3D (uint32_t)strlen(zName); Why do you declare it as uin32_t and do cast? region_alloc and memcpy take it as size_t arg, btw. > @@ -4683,18 +4711,29 @@ selectExpander(Walker * pWalker, Select * p) > assert(pFrom->pTab =3D=3D 0); > if (sqlite3WalkSelect(pWalker, pSel)) > return WRC_Abort; > + /* > + * Will be overwritten with pointer as > + * unique identifier. > + */ > + const char *name =3D = "sqlite_sq_DEADBEAFDEADBEAF=E2=80=9D; Lest don=E2=80=99t use sqlite prefixes. Just call it =E2=80=98unused=E2=80= =99. > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index c77aa9b..279c3af 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -661,15 +661,16 @@ sql_expr_compile(sqlite3 *db, const char *expr, = struct Expr **result) > sprintf(stmt, "%s%s", outer, expr); >=20 > struct Parse parser; > - memset(&parser, 0, sizeof(parser)); > + sql_parser_create(&parser); > parser.db =3D db; > parser.parse_only =3D true; > + Redundant empty line.