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 4191D20FFB for ; Fri, 18 May 2018 11:35:42 -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 z8-nhIXfbyFy for ; Fri, 18 May 2018 11:35:42 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C35EE20FF7 for ; Fri, 18 May 2018 11:35:41 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v6 2/4] sql: remove SQL fields from Table and Column References: <2cd3d86ce88d5115c5ec12611d40251fcc7968bd.1526403792.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: <4ede3bde-98d1-cc92-6659-891ef61e0917@tarantool.org> Date: Fri, 18 May 2018 18:35:39 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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, "n.pettik@corp.mail.ru" Cc: "v.shpilevoy@tarantool.org" > Where did you introduce this flag? AFAIR It has already been introduced > in space opts several patches ago. > Duplicate: in fact, it is fifth point. Updated commit message. > Sounds confusing to me. Mb better call it just ‘action_is_nullable’? > It is just a suggestion. +++ b/src/box/field_def.h @@ -131,7 +131,7 @@ struct field_def { }; static inline bool -nullable_action_is_nullable(enum on_conflict_action nullable_action) +action_is_nullable(enum on_conflict_action nullable_action) etc. > You don’t need carrying this line: it fits into one. > Either this. @@ -105,8 +105,7 @@ space_def_dup(const struct space_def *src) - struct Expr *e = - src->fields[i].default_value_expr; + struct Expr *e = src->fields[i].default_value_expr; @@ -190,8 +189,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, - struct Expr *e = - fields[i].default_value_expr; + struct Expr *e = fields[i].default_value_expr; > And this one. @@ -1448,8 +1448,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) for (i = 0; i < n; i++) { const char *t; - struct coll *coll = - coll_by_id(pTable->def->fields[i].coll_id); + struct coll *coll = coll_by_id(pTable->def->fields[i].coll_id); > >> + >> + struct field_def *field = &def->fields[i]; >> + const char *zToken = field->default_value; > > Lest use Tarantool naming conventions, i.e. don’t use Hungarian notation. @@ -1448,15 +1448,14 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) for (i = 0; i < n; i++) { const char *t; - struct coll *coll = - coll_by_id(pTable->def->fields[i].coll_id); + struct coll *coll = coll_by_id(pTable->def->fields[i].coll_id); struct field_def *field = &def->fields[i]; - const char *zToken = field->default_value; + const char *default_str = field->default_value; int base_len = 4; if (coll != NULL) base_len += 1; - if (zToken != NULL) + if (default_str != NULL) base_len += 1; p = enc->encode_map(p, base_len); p = enc->encode_str(p, "name", 4); @@ -1487,9 +1486,9 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) p = enc->encode_str(p, "collation", strlen("collation")); p = enc->encode_uint(p, coll->id); } - if (zToken != NULL) { + if (default_str != NULL) { p = enc->encode_str(p, "default", strlen("default")); - p = enc->encode_str(p, zToken, strlen(zToken)); + p = enc->encode_str(p, default_str, strlen(default_str)); } } return (int)(p - base); > Here smth wrong with indentations (and in other places where you use this assertion). @@ -1463,8 +1463,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) assert(def->fields[i].is_nullable == - action_is_nullable( - def->fields[i].nullable_action)); + action_is_nullable(def->fields[i].nullable_action)); @@ -1558,8 +1557,7 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf) assert(def->fields[col].is_nullable == - action_is_nullable( - def->fields[col].nullable_action)); + action_is_nullable(def->fields[col].nullable_action)); @@ -200,9 +200,8 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef) assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable == - action_is_nullable( - pNew->def->fields[pNew->def->field_count - - 1].nullable_action)); + action_is_nullable(pNew->def->fields[ + pNew->def->field_count - 1].nullable_action)); > >> + >> +struct space_def * >> +sql_ephemeral_space_def_new(Parse *parser, const char *name) > > Use ‘struct’ prefix for Parse struct: you did it in declaration, tho. (*) @@ -1696,7 +1696,7 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) struct space_def * -sql_ephemeral_space_def_new(Parse *parser, const char *name) +sql_ephemeral_space_def_new(struct Parse *parser, const char *name) @@ -1720,7 +1720,7 @@ sql_ephemeral_space_def_new(Parse *parser, const char *name) -Table * -sql_ephemeral_table_new(Parse *parser, const char *name) +struct Table * +sql_ephemeral_table_new(struct Parse *parser, const char *name) > > Please, provide explanation, why you call it ‘ephemeral’. > 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. @@ -146,6 +146,12 @@ sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc); /** * Create and initialize a new ephemeral SQL Table object. + * All memory allocation operations except Table object itself + * are performed in the region. + * + * The 'ephemeral' means that this memory is temporal, + * so the table should be rebuild with sql_table_def_rebuild for further use. + * * @param parser SQL Parser object. * @param name Table to create name. * @retval NULL on memory allocation error, Parser state changed. @@ -156,6 +162,11 @@ sql_ephemeral_table_new(struct Parse *parser, const char *name); /** * Create and initialize a new ephemeral space_def object. + * All memory allocation operations are performed on the region. + * + * The 'ephemeral' means that this memory is temporal, + * so the table should be rebuild with sql_table_def_rebuild for further use. + * * @param parser SQL Parser object. * @param name Table to create name. * @retval NULL on memory allocation error, Parser state changed. > Why do you need to declare it beforehand? > You can write this way: > struct space_def *def = (struct space_def *)region_alloc(region, size); @@ -1698,12 +1698,11 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) struct space_def * sql_ephemeral_space_def_new(struct Parse *parser, const char *name) { - struct space_def *def = NULL; struct region *region = &fiber()->gc; size_t name_len = name != NULL ? strlen(name) : 0; uint32_t dummy; size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy, &dummy); - def = (struct space_def *)region_alloc(region, size); + struct space_def *def = (struct space_def *)region_alloc(region, size); if (def == NULL) { diag_set(OutOfMemory, sizeof(struct tuple_dictionary), "region_alloc", "sql_ephemeral_space_def_new"); > The same as (*).@@ -1740,12 +1739,12 @@ int sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) { struct space_def *old_def = pTable->def; - struct space_def *new_def = NULL; - new_def = space_def_new(old_def->id, old_def->uid, - old_def->field_count, old_def->name, - strlen(old_def->name), old_def->engine_name, - strlen(old_def->engine_name), &old_def->opts, - old_def->fields, old_def->field_count); + struct space_def *new_def = + space_def_new(old_def->id, old_def->uid, + old_def->field_count, old_def->name, + strlen(old_def->name), old_def->engine_name, + strlen(old_def->engine_name), &old_def->opts, + old_def->fields, old_def->field_count); > > Did you mean ’Name of table to be created’? - * @param name Table to create name. + * @param name Name of table to be created. > I may already mention, but don’t write comments > 'just for comments’. I think it is a good place to explain, > for example, difference between ephemeral (btw, I don’t like this name) > table and ‘ordinary’ one. Accounted above. >> + 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. +++ b/src/box/space_def.h @@ -47,6 +47,8 @@ struct space_opts { * - it is empty at server start * - changes are not written to WAL * - changes are not part of a snapshot + * - in SQL: space_def memory is allocated on region and + * does not require manual release. */ @@ -166,6 +167,7 @@ sql_ephemeral_table_new(struct Parse *parser, const char *name); * * The 'ephemeral' means that this memory is temporal, * so the table should be rebuild with sql_table_def_rebuild for further use. + * (opts.temporary flag is set 'true' to indicate this property) > The same is here: add comment explaining this assert. @@ -646,7 +646,6 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) sqlite3 *db = pParse->db; if ((p = pParse->pNewTable) == 0) return; - assert(p->def->opts.temporary); #if SQLITE_MAX_COLUMN if ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) { sqlite3ErrorMsg(pParse, "too many columns on %s", @@ -654,6 +653,12 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) return; } #endif + /* + * As sql_field_retrieve will allocate memory on region + * ensure that p->def is also temporal and would be rebuilded or + * dropped. + */ + assert(p->def->opts.temporary); if (sql_field_retrieve(pParse, p, (uint32_t) p->def->field_count) == NULL) return; > >> @@ -1937,7 +1920,9 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ >> if (pSelect) { >> zStmt = createTableStmt(db, p); >> } else { >> - Token *pEnd2 = p->pSelect ? &pParse->sLastToken : pEnd; >> + assert(p->def->opts.is_view == (p->pSelect != NULL)); > > Too many same assertions in one function. Do you really need them all? @@ -1895,7 +1895,6 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ if (pSelect) { zStmt = createTableStmt(db, p); } else { - assert(p->def->opts.is_view == (p->pSelect != NULL)); @@ -1956,7 +1955,6 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ #ifndef SQLITE_OMIT_ALTERTABLE - assert(p->def->opts.is_view == (p->pSelect != NULL)); > >> 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 >= 0 ? pTab->aCol[iCol].affinity : SQLITE_AFF_INTEGER; >> } >> >> @@ -2233,7 +2233,8 @@ isCandidateForInOpt(Expr * pX) >> return 0; /* FROM is not a subquery or view */ >> pTab = pSrc->a[0].pTab; >> assert(pTab != 0); >> - assert(pTab->pSelect == 0); /* FROM clause is not a view */ >> + assert(pTab->def->opts.is_view == (pTab->pSelect != NULL)); >> + assert(!pTab->def->opts.is_view); /* FROM clause is not a view */ > > Don’t put comment right to code: move it up. @@ -2238,7 +2238,8 @@ isCandidateForInOpt(Expr * pX) assert(pTab->def->opts.is_view == (pTab->pSelect != NULL)); - assert(!pTab->def->opts.is_view); /* FROM clause is not a view */ + /* FROM clause is not a view */ + assert(!pTab->def->opts.is_view); > 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() @@ -4157,6 +4157,8 @@ table_column_nullable_action(struct Table *tab, uint32_t column); /** * Initialize a new parser object. + * A number of service allocations are performed on the region, which is also + * cleared in the destroy function. * @param parser object to initialize. >> + /* NULL nullable_action should math is_nullable flag. */ > Math? I can’t understand this comment. Rephrase it pls. - /* NULL nullable_action should math is_nullable flag. */ + /* + * Set is_nullable flag for fields with default NULL nullable_action to + * be consistent. + */ > Why do you declare it as uin32_t and do cast? > region_alloc and memcpy take it as size_t arg, btw. - uint32_t name_len = (uint32_t)strlen(zName); + size_t name_len = strlen(zName);