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 6F51D253D1 for ; Tue, 29 Jan 2019 09:59:35 -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 MbjexVCOO_lz for ; Tue, 29 Jan 2019 09:59:35 -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 BB3F125216 for ; Tue, 29 Jan 2019 09:59:34 -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: <4c2b87b5-9e83-f96f-4350-edf7a12e5593@tarantool.org> Date: Tue, 29 Jan 2019 17:59:31 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <4c2b87b5-9e83-f96f-4350-edf7a12e5593@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 > 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 Feel free to use up to 72 chars in commit message body. sql: remove struct Table 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. Note that after commit subject's prefix we use lower-case. 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? > Closes #3235 > --- > Branch:=20 > = https://github.com/tarantool/tarantool/tree/sudobobo/gh-3235-repl-Table-w-= space >=20 > Issue:=20 > https://github.com/tarantool/tarantool/issues/3235 >=20 >=20 > src/box/sql.c | 64 +++----- > src/box/sql.h | 25 +-- > src/box/sql/analyze.c | 8 +- > src/box/sql/build.c | 390 = +++++++++++++++++++++------------------------ > src/box/sql/delete.c | 75 ++++----- > src/box/sql/expr.c | 28 ++-- > src/box/sql/fkey.c | 67 ++++---- > src/box/sql/insert.c | 97 ++++++----- > src/box/sql/resolve.c | 47 +++--- > src/box/sql/select.c | 297 ++++++++++++++++------------------ > src/box/sql/sqliteInt.h | 129 +++++++-------- > src/box/sql/tarantoolInt.h | 8 +- > src/box/sql/tokenize.c | 10 +- > src/box/sql/treeview.c | 4 +- > src/box/sql/trigger.c | 36 ++--- > src/box/sql/update.c | 52 +++--- > src/box/sql/where.c | 59 ++++--- > src/box/sql/wherecode.c | 8 +- > src/box/sql/whereexpr.c | 13 +- > 19 files changed, 635 insertions(+), 782 deletions(-) >=20 > diff --git a/src/box/sql.c b/src/box/sql.c > index 387da7b3d..978ad8310 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -972,7 +972,7 @@ cursor_advance(BtCursor *pCur, int *pRes) > */ > =20 > char * > -sql_encode_table(struct region *region, struct Table *table, uint32_t = *size) > +sql_encode_table(struct region *region, struct space *space, uint32_t = *size) In this function we use only space_def, so lets pass directly space_def instead of whole space. > @@ -1033,7 +1033,7 @@ sql_encode_table(struct region *region, struct = Table *table, uint32_t *size) > } > =20 > char * > -sql_encode_table_opts(struct region *region, struct Table *table, > +sql_encode_table_opts(struct region *region, struct space *space, > const char *sql, uint32_t *size) The same is here. > @@ -1258,49 +1258,26 @@ sql_ephemeral_space_def_new(struct Parse = *parser, const char *name) > return def; > } > =20 > -Table * > -sql_ephemeral_table_new(Parse *parser, const char *name) > +struct space * > +sql_ephemeral_space_new(Parse *parser, const char *name) > { > - sqlite3 *db =3D parser->db; > - struct space_def *def =3D NULL; > - Table *table =3D sqlite3DbMallocZero(db, sizeof(Table)); > - if (table !=3D NULL) > - def =3D sql_ephemeral_space_def_new(parser, name); > - if (def =3D=3D NULL) { > - sqlite3DbFree(db, table); > - return NULL; > - } > - table->space =3D (struct space *) region_alloc(&parser->region, > - sizeof(struct = space)); > - if (table->space =3D=3D NULL) { > + struct space *space =3D > + (struct space *) region_alloc(&parser->region, > + sizeof(struct space)); Smth wrong with indentation here: we use as much tabs as we can and fill the rest with spaces. NIt: consider this variant: diff --git a/src/box/sql.c b/src/box/sql.c index 978ad8310..933dfe059 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1261,22 +1261,18 @@ sql_ephemeral_space_def_new(struct Parse = *parser, const char *name) struct space * sql_ephemeral_space_new(Parse *parser, const char *name) { - struct space *space =3D - (struct space *) region_alloc(&parser->region, - sizeof(struct space)); + size_t sz =3D sizeof(struct space); + struct space *space =3D (struct space *) = region_alloc(&parser->region, sz); if (space =3D=3D NULL) { - diag_set(OutOfMemory, sizeof(struct space), "region", = "space"); + diag_set(OutOfMemory, sz, "region", "space"); parser->rc =3D SQL_TARANTOOL_ERROR; parser->nErr++; return NULL; } - - memset(space, 0, sizeof(struct space)); + memset(space, 0, sz); space->def =3D sql_ephemeral_space_def_new(parser, name); - if (space->def =3D=3D NULL) { + if (space->def =3D=3D NULL) return NULL; - } - return space; > + if (space =3D=3D NULL) { > diag_set(OutOfMemory, sizeof(struct space), "region", = "space"); > parser->rc =3D SQL_TARANTOOL_ERROR; > parser->nErr++; > - sqlite3DbFree(db, table); > return NULL; > } > - memset(table->space, 0, sizeof(struct space)); > - table->def =3D def; > - return table; > -} > =20 > -int > -sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) > -{ > - struct space_def *old_def =3D pTable->def; > - struct space_def *new_def =3D NULL; > - new_def =3D 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); > - if (new_def =3D=3D NULL) { > - sqlite3OomFault(db); > - return -1; > + memset(space, 0, sizeof(struct space)); > + space->def =3D sql_ephemeral_space_def_new(parser, name); > + if (space->def =3D=3D NULL) { > + return NULL; > } We don=E2=80=99t wrap one-line if body in brackets. > @@ -1357,11 +1334,10 @@ = sql_checks_resolve_space_def_reference(ExprList *expr_list, > sql_parser_create(&parser, sql_get()); > parser.parse_only =3D true; > =20 > - Table dummy_table; > - memset(&dummy_table, 0, sizeof(dummy_table)); > - dummy_table.def =3D def; > + struct space dummy_space; Objects created on stack are not supposed to be zeroed. This time we get lucky and it is filled with 0, but generally speaking - it is not true. You can use memset. Another way of initialisation with zeroes is: struct space dummy_space =3D { 0 }; I guess both memset and {0} are represented in the same way in terms of machine codes, so choose any option. > + dummy_space.def =3D def; > =20 > - sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, = NULL, > + sql_resolve_self_reference(&parser, &dummy_space, NC_IsCheck, = NULL, In both cases where this function appears you pass space allocated on stack. Hence, you can pass here space_def and inside this function wrap it in dummy space. > diff --git a/src/box/sql.h b/src/box/sql.h > index 028a15245..0d9ecd0ce 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -225,36 +225,25 @@ void > sql_expr_delete(struct sqlite3 *db, struct Expr *expr, bool = extern_alloc); > =20 > /** > - * Create and initialize a new ephemeral SQL Table object. > + * Create and initialize a new ephemeral space object. > * @param parser SQL Parser object. > - * @param name Table to create name. > + * @param name Space to create name. Rephrase comment pls, can=E2=80=99t parse. Guess it should be =E2=80=9CName of space to be created=E2=80=9D. =20 > /** > * Create and initialize a new ephemeral space_def object. > * @param parser SQL Parser object. > - * @param name Table to create name. > + * @param name Space to create name. The same (ofc it is initially not your comment but anyway). > * @retval NULL on memory allocation error, Parser state changed. > * @retval not NULL on success. > */ > struct space_def * > sql_ephemeral_space_def_new(struct Parse *parser, const char *name); > =20 > void > -sqlite3DeleteTable(sqlite3 * db, Table * pTable) > +sql_space_delete(struct sqlite3 *db, struct space *space) > { > - /* 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 || !db || db->pnBytesFreed =3D=3D 0) We use explicit !=3D NULL comparison. > return; > - table_delete(db, pTable); > + > + 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); > + } > } > =20 > /* > @@ -343,15 +310,15 @@ sqlite3CheckIdentifierName(Parse *pParse, char = *zName) > } > =20 > struct index * > -sql_table_primary_key(const struct Table *tab) > +sql_table_primary_key(const struct space *space) Its not longer table. > /** > - * Create and initialize a new SQL Table object. > + * Create and initialize a new SQL space object. > * All memory except table object itself is allocated on region. > * @param parser SQL Parser object. > * @param name Table to create name. > @@ -359,18 +326,17 @@ sql_table_primary_key(const struct Table *tab) > * changed. > * @retval not NULL on success. > */ > -static Table * > +static struct space * > sql_table_new(Parse *parser, char *name) Inline this function, it seems to be too insignificant. > { > - struct Table *table =3D sql_ephemeral_table_new(parser, name); > - if (table =3D=3D NULL) > + struct space *space =3D sql_ephemeral_space_new(parser, name); > + if (space =3D=3D NULL) > return NULL; > =20 > - strcpy(table->def->engine_name, > + strcpy(space->def->engine_name, > = sql_storage_engine_strs[current_session()->sql_default_engine]); > =20 > - table->nTabRef =3D 1; > - return table; > + return space; > } > =20 > /* > @@ -396,7 +362,6 @@ sql_table_new(Parse *parser, char *name) > void > sqlite3StartTable(Parse *pParse, Token *pName, int noErr) Update comment to this function: there is no pNewTable anymore. > { > - Table *pTable; > char *zName =3D 0; /* The name of the new table */ > sqlite3 *db =3D pParse->db; > struct Vdbe *v =3D sqlite3GetVdbe(pParse); > @@ -423,12 +388,12 @@ sqlite3StartTable(Parse *pParse, Token *pName, = int noErr) > goto cleanup; > } > =20 > - pTable =3D sql_table_new(pParse, zName); > - if (pTable =3D=3D NULL) > + struct space *new_space =3D sql_table_new(pParse, zName); > + if (new_space =3D=3D NULL) > goto cleanup; > =20 > - assert(pParse->pNewTable =3D=3D 0); > - pParse->pNewTable =3D pTable; > + assert(pParse->new_space =3D=3D 0); =3D=3D NULL >=20 > @@ -513,27 +478,28 @@ void > sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def = *type_def) > { > assert(type_def !=3D NULL); > - Table *p; > int i; > char *z; > sqlite3 *db =3D pParse->db; > - if ((p =3D pParse->pNewTable) =3D=3D 0) > + if ((pParse->new_space) =3D=3D 0) =3D=3D NULL > return; > + struct space *space =3D pParse->new_space; You use only def in this function, so fetch space_def instead of whole space. > + > #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 > - /* > + /** > * As sql_field_retrieve will allocate memory on region > - * ensure that p->def is also temporal and would be rebuilded or > + * ensure that p->space->def is also temporal and would be = rebuilded or > * dropped. You have removed rebuild function, so I guess there is no opportunities to rebuild anymore. > */ > - assert(p->def->opts.is_temporary); > - if (sql_field_retrieve(pParse, p, > - (uint32_t) p->def->field_count) =3D=3D = NULL) > + assert(space->def->opts.is_temporary); > + if (sql_field_retrieve(pParse, space->def, > + (uint32_t) space->def->field_count) =3D=3D = NULL) field_count is already of type uint32_t. > return; > struct region *region =3D &pParse->region; > z =3D region_alloc(region, pName->n + 1); > @@ -547,13 +513,14 @@ sqlite3AddColumn(Parse * pParse, Token * pName, = struct type_def *type_def) > memcpy(z, pName->z, pName->n); > z[pName->n] =3D 0; > sqlite3NormalizeName(z); > - for (i =3D 0; i < (int)p->def->field_count; i++) { > - if (strcmp(z, p->def->fields[i].name) =3D=3D 0) { > + for (i =3D 0; i < (int)space->def->field_count; i++) { for (uint32_t i =3D 0; space->def->field_count; ...) > + if (strcmp(z, space->def->fields[i].name) =3D=3D 0) { > sqlite3ErrorMsg(pParse, "duplicate column name: = %s", z); > return; > } > } >=20 > =20 > @@ -573,17 +540,18 @@ void > sql_column_add_nullable_action(struct Parse *parser, > enum on_conflict_action nullable_action) > { > - struct Table *p =3D parser->pNewTable; > - if (p =3D=3D NULL || NEVER(p->def->field_count < 1)) > + if (parser->new_space =3D=3D NULL || > + NEVER(parser->new_space->def->field_count < 1)) Wrong indentation. > return; > - struct field_def *field =3D &p->def->fields[p->def->field_count = - 1]; > + struct space *space =3D parser->new_space; struct space_def *def =3D parser->new_space->def; > @@ -608,20 +576,20 @@ sql_column_add_nullable_action(struct Parse = *parser, > void > sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan) > { > - Table *p; > sqlite3 *db =3D pParse->db; > - p =3D pParse->pNewTable; > - assert(p->def->opts.is_temporary); > - if (p !=3D 0) { > + assert(pParse->new_space->def->opts.is_temporary); > + if (pParse->new_space !=3D 0) { !=3D NULL (please, fix the rest of similar places). > + struct space_def *space_def =3D pParse->new_space->def; Call it simply =E2=80=9Cdef=E2=80=9D and you won=E2=80=99t have to carry = line below. > if (!sqlite3ExprIsConstantOrFunction > (pSpan->pExpr, db->init.busy)) { > sqlite3ErrorMsg(pParse, > "default value of column [%s] is = not constant", > - = p->def->fields[p->def->field_count - 1].name); > + space_def-> > + fields[space_def->field_count - = 1].name); >=20 >=20 > @@ -714,14 +682,14 @@ sqlite3AddPrimaryKey(Parse * pParse, /* = Parsing context */ > } > } > if (nTerm =3D=3D 1 && iCol !=3D -1 && > - pTab->def->fields[iCol].type =3D=3D FIELD_TYPE_INTEGER && > - sortOrder !=3D SORT_ORDER_DESC) { Wrong indentation. > + space->def->fields[iCol].type =3D=3D FIELD_TYPE_INTEGER = && > + sortOrder !=3D SORT_ORDER_DESC) { >=20 >=20 >=20 > @@ -1229,7 +1197,7 @@ resolve_link(struct Parse *parse_context, const = struct space_def *def, > * This routine is called to report the final ")" that terminates > * a CREATE TABLE statement. > * > - * The table structure that other action routines have been building > + * The space structure that other action routines have been building > * is added to the internal hash tables, assuming no errors have > * occurred. There is no internal hash of tables anymore. Fix comment pls. > * > @@ -1238,12 +1206,12 @@ resolve_link(struct Parse *parse_context, = const struct space_def *def, > * space and all necessary Tarantool indexes is emitted > * 2. When db->init.busy =3D=3D 1. This means that byte code for = creation > * of new table is executing right now, and it's time to add new = entry > - * for the table into SQL memory represenation > + * for the table into SQL memory representation The same. > sqlite3EndTable(Parse * pParse, /* Parse context */ > @@ -1251,24 +1219,24 @@ sqlite3EndTable(Parse * pParse, /* Parse = context */ > Select * pSelect /* Select from a "CREATE ... AS = SELECT" */ > ) > { > - Table *p; /* The new table */ > sqlite3 *db =3D pParse->db; /* The database connection */ > =20 > if (pEnd =3D=3D 0 && pSelect =3D=3D 0) { > return; > } > assert(!db->mallocFailed); > - p =3D pParse->pNewTable; > - if (p =3D=3D 0) > + if (pParse->new_space =3D=3D 0) > return; > =20 > + struct space *new_space =3D pParse->new_space; > + Don=E2=80=99t abuse new lines. > @@ -1820,13 +1788,15 @@ sql_create_foreign_key(struct Parse = *parse_context, struct SrcList *child, > char *parent_name =3D NULL; > char *constraint_name =3D NULL; > bool is_self_referenced =3D false; > - /* > - * Table under construction during CREATE TABLE > + /** /** -> /* > + * Space under construction during CREATE TABLE > * processing. NULL for ALTER TABLE statement handling. > */ > - struct Table *new_tab =3D parse_context->pNewTable; > - /* Whether we are processing ALTER TABLE or CREATE TABLE. */ > - bool is_alter =3D new_tab =3D=3D NULL; > + bool is_alter =3D (parse_context->new_space =3D=3D NULL); > + struct space *space; > + if (!is_alter) > + space =3D parse_context->new_space; > + diff --git a/src/box/sql/build.c b/src/box/sql/build.c index cae0b3f6e..4b7eef988 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1792,11 +1792,8 @@ sql_create_foreign_key(struct Parse = *parse_context, struct SrcList *child, * Space under construction during CREATE TABLE * processing. NULL for ALTER TABLE statement handling. */ - bool is_alter =3D (parse_context->new_space =3D=3D NULL); - struct space *space; - if (!is_alter) - space =3D parse_context->new_space; - + struct space *space =3D parse_context->new_space; + bool is_alter =3D space =3D=3D NULL; > @@ -2249,7 +2219,6 @@ sql_create_index(struct Parse *parse, struct = Token *token, > * Return early if not found. > */ > struct space *space =3D NULL; > - struct space_def *def =3D NULL; Do you really need this diff? It results only in other extra diffs. def makes handling shorter. > if (tbl_name !=3D NULL) { > assert(token !=3D NULL && token->z !=3D NULL); > const char *name =3D tbl_name->a[0].zName; > @@ -2262,17 +2231,16 @@ sql_create_index(struct Parse *parse, struct = Token *token, > } > goto exit_create_index; > } > - def =3D space->def; > } else { > - if (parse->pNewTable =3D=3D NULL) > + if (parse->new_space =3D=3D NULL) > goto exit_create_index; > assert(token =3D=3D NULL); > assert(start =3D=3D NULL); > - space =3D parse->pNewTable->space; > - def =3D parse->pNewTable->def; > + space =3D parse->new_space; > + space->def =3D parse->new_space->def; Why do you assign parse->new_space->def to parse->new_space->def ? > @@ -2883,7 +2851,7 @@ sqlite3SrcListDelete(sqlite3 * db, SrcList * = pList) > sqlite3DbFree(db, pItem->u1.zIndexedBy); > if (pItem->fg.isTabFunc) > sql_expr_list_delete(db, pItem->u1.pFuncArg); > - sqlite3DeleteTable(db, pItem->pTab); > + sql_space_delete(db, pItem->space); > sql_select_delete(db, pItem->pSelect); > sql_expr_delete(db, pItem->pOn, false); > sqlite3IdListDelete(db, pItem->pUsing); > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index f9c42fdec..b687ff8f4 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -53,16 +53,10 @@ sql_lookup_table(struct Parse *parse, struct = SrcList_item *tbl_name) > parse->nErr++; > return NULL; > } > - struct Table *table =3D sqlite3DbMallocZero(parse->db, = sizeof(*table)); > - if (table =3D=3D NULL) > - return NULL; > - table->def =3D space->def; > - table->space =3D space; > - table->nTabRef =3D 1; > - tbl_name->pTab =3D table; > - if (sqlite3IndexedByLookup(parse, tbl_name) !=3D 0) > - table =3D NULL; > - return table; > + space_name->space =3D space; > + if (sqlite3IndexedByLookup(parse, space_name) !=3D 0) > + space =3D NULL; > + return space; > } > =20 >=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)) { Broken indentation. > diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c > index 3033cfcbb..93f98b9db 100644 > --- a/src/box/sql/fkey.c > +++ b/src/box/sql/fkey.c > @@ -302,20 +302,23 @@ fkey_lookup_parent(struct Parse *parse_context, = struct space *parent, > sqlite3VdbeAddOp1(v, OP_Close, cursor); > } > =20 > -/* > - * Return an Expr object that refers to a memory register = corresponding > - * to column iCol of table pTab. > +/** > + * Return an Expr object that refers to a memory register > + * corresponding to column iCol of given space. > * > - * regBase is the first of an array of register that contains the = data > - * for pTab. regBase+1 holds the first column. > + * regBase is the first of an array of register that contains > + * the data for given space. regBase+1 holds the first column. > * regBase+2 holds the second column, and so forth. > + * > + * @param pParse Parsing and code generating context. > + * @param space The space whose content is at r[regBase]... > + * @param regBase Contents of table pTab. Not pTab. > + * @param iCol Which column of pTab is desired. Same. > + * @return an Expr object that refers to a memory register > + * corresponding to column iCol of given space. > */ > static Expr * > -exprTableRegister(Parse * pParse, /* Parsing and code generating = context */ > - Table * pTab, /* The table whose content is at = r[regBase]... */ > - int regBase, /* Contents of table pTab */ > - i16 iCol /* Which column of pTab is desired */ > - ) > +exprTableRegister(Parse * pParse, struct space * space, int regBase, = i16 iCol) Lets rename it to space_field_register() or space_field_get_register() (not sure if we use _get_ prefix for getters). You also can pass here just def. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index f147f6a50..c45bcc21b 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -117,14 +117,14 @@ sql_space_autoinc_fieldno(struct space *space) > * SELECT. > * > * @param parser Parse context. > - * @param table Table AST object. > - * @retval true if the table table in database or any of its > + * @param space_def Space definition. > + * @retval true if the space (given by space_id) in database or any = of its Nit: out of 66 chars. > @@ -311,32 +310,32 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ >=20 > /* Figure out if we have any triggers and if the table being > * inserted into is a view > */ > - trigger =3D sql_triggers_exist(pTab, TK_INSERT, NULL, &tmask); > - bool is_view =3D pTab->def->opts.is_view; > + trigger =3D sql_triggers_exist(space->def, TK_INSERT, NULL, = &tmask); > + struct space_def *space_def =3D space->def; Swap this and previous lines pls, so that we pass space_def to sql_triggers_exits (not space->def). > + space_id =3D space->def->id; Here you need space id only to pass it to space_column_default_expr(), which in turn again fetches space from cache using space_by_id(). Lets remove this redundant function and simply get default value by space->def->fields=E2=80=A6 > @@ -218,7 +218,7 @@ lookupName(Parse * pParse, /* The parsing = context */ > struct SrcList_item *pMatch =3D 0; /* The matching pSrcList = item */ > NameContext *pTopNC =3D pNC; /* First namecontext in the list = */ > int isTrigger =3D 0; /* True if resolved to a trigger column = */ > - Table *pTab =3D 0; /* Table hold the row */ > + struct space_def *space_def; > =20 > assert(pNC); /* the name context cannot be NULL. */ > assert(zCol); /* The Z in X.Y.Z cannot be NULL */ > @@ -237,9 +237,10 @@ lookupName(Parse * pParse, /* The parsing = context */ > if (pSrcList) { > for (i =3D 0, pItem =3D pSrcList->a; i < = pSrcList->nSrc; > i++, pItem++) { > - pTab =3D pItem->pTab; > - assert(pTab !=3D 0 && pTab->def->name !=3D= NULL); > - assert(pTab->def->field_count > 0); > + assert(pItem->space !=3D NULL && > + pItem->space->def->name !=3D = NULL); Broken indentation a bit. > @@ -311,32 +312,32 @@ lookupName(Parse * pParse, /* The parsing = context */ > * it is a new.* or old.* trigger argument reference > */ > if (zTab !=3D 0 && cntTab =3D=3D 0 > - && pParse->pTriggerTab !=3D 0) { > + && pParse->trigger_space !=3D 0) { > int op =3D pParse->eTriggerOp; > assert(op =3D=3D TK_DELETE || op =3D=3D = TK_UPDATE > || op =3D=3D TK_INSERT); > if (op !=3D TK_DELETE && sqlite3StrICmp("new", = zTab) =3D=3D 0) { > pExpr->iTable =3D 1; > - pTab =3D pParse->pTriggerTab; > + space_def =3D = pParse->trigger_space->def; > } else if (op !=3D TK_INSERT > && sqlite3StrICmp("old", zTab) =3D=3D = 0) { > pExpr->iTable =3D 0; > - pTab =3D pParse->pTriggerTab; > + space_def =3D = pParse->trigger_space->def; > } else { > - pTab =3D 0; > + space_def =3D 0; NULL (and in other places too). >=20 > @@ -1934,20 +1932,28 @@ cleanup: > } > =20 > /* > + > + */ > + Garbage diff. > +/** > * 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. Why did you change this comment? > * > * This routine requires that all identifiers in the SELECT > * statement be resolved. > + * > + * @param pParse Parsing contexts. > + * @param space Add column type information to this table. To this space. > + * @param pSelect SELECT used to determine types and collations. > */ > void > -sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, = /* Parsing contexts */ > - Table * pTab, /* Add = column type information to this table */ > - Select * pSelect) /* = SELECT used to determine types and collations */ > +sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, struct space * = space,=20 Extra space after asterisk. > @@ -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 Extra diff. > */ > 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; > } Remove extra brackets. > @@ -4803,39 +4781,36 @@ selectExpander(Walker * pWalker, Select * p) > * unique identifier. > */ > const char *name =3D = "sqlite_sq_DEADBEAFDEADBEAF"; > - pFrom->pTab =3D pTab =3D > - sql_ephemeral_table_new(pParse, name); > - if (pTab =3D=3D NULL) > + struct space *space; > + pFrom->space =3D space =3D Avoid using double assignment at one time. > @@ -5673,28 +5649,29 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > pItem->regReturn); > pItem->addrFillSub =3D topAddr + 1; > if (pItem->fg.isCorrelated =3D=3D 0) { > - /* If the subquery is not correlated and = if we are not inside of > - * a trigger, then we only need to = compute the value of the subquery > - * once. > + /* If the subquery is not > + * correlated and if we are not > + * inside of a trigger, then > + * we only need to compute the > + * value of the subquery once. It is not necessary to fix this comment. > /* > * Each foreign key constraint is an instance of the following = structure. >=20 > @@ -2733,7 +2712,7 @@ struct Parse { > int nSelect; /* Number of SELECT statements seen */ > int nSelectIndent; /* How far to indent SELECTTRACE() = output */ > Parse *pToplevel; /* Parse structure for main program (or = NULL) */ > - Table *pTriggerTab; /* Table triggers are being coded for */ > + struct space *trigger_space; /* Space triggers are being = coded for */ > u32 nQueryLoop; /* Est number of iterations of a query = (10*log2(N)) */ > u32 oldmask; /* Mask of old.* columns referenced */ > u32 newmask; /* Mask of new.* columns referenced */ > @@ -2776,8 +2755,7 @@ struct Parse { > VList *pVList; /* Mapping between variable names and = numbers */ > Vdbe *pReprepare; /* VM being reprepared = (sqlite3Reprepare()) */ > const char *zTail; /* All SQL text past the last semicolon = parsed */ > - Table *pNewTable; /* A table being constructed by CREATE = TABLE */ > - Table *pZombieTab; /* List of Table objects to delete after = code gen */ > + struct space *new_space; /* A space being constructed by = CREATE TABLE */ Now we have trigger_space and new_space members. And it seems that we can=E2=80=99t use both of the at the same time. So, lets throw = away one member. If you want to save names, mb it is worth to use union. Moreover, I still see several pNewTable references in comments. =20 > /** > * 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 Extra diff. > @@ -3985,19 +3964,21 @@ vdbe_code_drop_trigger(struct Parse *parser, = const char *trigger_name, > bool account_changes); > =20 > /** > - * Return a list of all triggers on table pTab if there exists at > - * least one trigger that must be fired when an operation of type > - * 'op' is performed on the table, and, if that operation is an > - * UPDATE, if at least one of the columns in changes_list is being > - * modified. > - * > - * @param table The table the contains the triggers. > + * Return a list of all triggers on space (represented with > + * space_def) if there exists at least one trigger that must be > + * fired when an operation of type 'op' is performed on the > + * table, and, if that operation is an UPDATE, if at least one > + * of the columns in changes_list is being modified. > + * > + * @param space_def The definition of the space that contains > + * the triggers. > * @param op operation one of TK_DELETE, TK_INSERT, TK_UPDATE. > * @param changes_list Columns that change in an UPDATE statement. > * @param[out] pMask Mask of TRIGGER_BEFORE|TRIGGER_AFTER > */ > struct sql_trigger * > -sql_triggers_exist(struct Table *table, int op, struct ExprList = *changes_list, > +sql_triggers_exist(struct space_def *space_def, int op, > + struct ExprList *changes_list, > int *mask_ptr); You can fit two args in one line. Finally, I see a lot of space_by_id() usage through code. Could you please validate them and remove redundant ones. You may move this part to a separate patch, if you want to.