* [tarantool-patches] [PATCH] sql: remove struct Table
@ 2019-01-28 9:54 Ivan Koptelov
2019-01-29 14:59 ` [tarantool-patches] " n.pettik
2019-02-15 14:47 ` Kirill Yukhin
0 siblings, 2 replies; 10+ messages in thread
From: Ivan Koptelov @ 2019-01-28 9:54 UTC (permalink / raw)
To: tarantool-patches
[-- Attachment #1: Type: text/html, Size: 156648 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-01-28 9:54 [tarantool-patches] [PATCH] sql: remove struct Table Ivan Koptelov
@ 2019-01-29 14:59 ` n.pettik
2019-02-01 11:05 ` Ivan Koptelov
2019-02-15 14:47 ` Kirill Yukhin
1 sibling, 1 reply; 10+ messages in thread
From: n.pettik @ 2019-01-29 14:59 UTC (permalink / raw)
To: tarantool-patches; +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.
>
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… I am not complaining but
are you sure that you set up git-email correctly?
> Closes #3235
> ---
> Branch:
> https://github.com/tarantool/tarantool/tree/sudobobo/gh-3235-repl-Table-w-space
>
> Issue:
> https://github.com/tarantool/tarantool/issues/3235
>
>
> 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(-)
>
> 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)
> */
>
> 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)
> }
>
> 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;
> }
>
> -Table *
> -sql_ephemeral_table_new(Parse *parser, const char *name)
> +struct space *
> +sql_ephemeral_space_new(Parse *parser, const char *name)
> {
> - sqlite3 *db = parser->db;
> - struct space_def *def = NULL;
> - Table *table = sqlite3DbMallocZero(db, sizeof(Table));
> - if (table != NULL)
> - def = sql_ephemeral_space_def_new(parser, name);
> - if (def == NULL) {
> - sqlite3DbFree(db, table);
> - return NULL;
> - }
> - table->space = (struct space *) region_alloc(&parser->region,
> - sizeof(struct space));
> - if (table->space == NULL) {
> + struct space *space =
> + (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 =
- (struct space *) region_alloc(&parser->region,
- sizeof(struct space));
+ size_t sz = sizeof(struct space);
+ struct space *space = (struct space *) region_alloc(&parser->region, sz);
if (space == NULL) {
- diag_set(OutOfMemory, sizeof(struct space), "region", "space");
+ diag_set(OutOfMemory, sz, "region", "space");
parser->rc = SQL_TARANTOOL_ERROR;
parser->nErr++;
return NULL;
}
-
- memset(space, 0, sizeof(struct space));
+ memset(space, 0, sz);
space->def = sql_ephemeral_space_def_new(parser, name);
- if (space->def == NULL) {
+ if (space->def == NULL)
return NULL;
- }
-
return space;
> + if (space == NULL) {
> diag_set(OutOfMemory, sizeof(struct space), "region", "space");
> parser->rc = SQL_TARANTOOL_ERROR;
> parser->nErr++;
> - sqlite3DbFree(db, table);
> return NULL;
> }
> - memset(table->space, 0, sizeof(struct space));
> - table->def = def;
> - return table;
> -}
>
> -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);
> - if (new_def == NULL) {
> - sqlite3OomFault(db);
> - return -1;
> + memset(space, 0, sizeof(struct space));
> + space->def = sql_ephemeral_space_def_new(parser, name);
> + if (space->def == NULL) {
> + return NULL;
> }
We don’t 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 = true;
>
> - Table dummy_table;
> - memset(&dummy_table, 0, sizeof(dummy_table));
> - dummy_table.def = 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 = { 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 = def;
>
> - 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);
>
> /**
> - * 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’t parse.
Guess it should be “Name of space to be created”.
> /**
> * 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);
>
> 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 == 0) && (--pTable->nTabRef) > 0))
> + if (!space || !db || db->pnBytesFreed == 0)
We use explicit != NULL comparison.
> return;
> - table_delete(db, pTable);
> +
> + if (space->def->opts.is_temporary) {
> + for (uint32_t i = 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);
> + }
> }
>
> /*
> @@ -343,15 +310,15 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName)
> }
>
> 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 = sql_ephemeral_table_new(parser, name);
> - if (table == NULL)
> + struct space *space = sql_ephemeral_space_new(parser, name);
> + if (space == NULL)
> return NULL;
>
> - strcpy(table->def->engine_name,
> + strcpy(space->def->engine_name,
> sql_storage_engine_strs[current_session()->sql_default_engine]);
>
> - table->nTabRef = 1;
> - return table;
> + return space;
> }
>
> /*
> @@ -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 = 0; /* The name of the new table */
> sqlite3 *db = pParse->db;
> struct Vdbe *v = sqlite3GetVdbe(pParse);
> @@ -423,12 +388,12 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
> goto cleanup;
> }
>
> - pTable = sql_table_new(pParse, zName);
> - if (pTable == NULL)
> + struct space *new_space = sql_table_new(pParse, zName);
> + if (new_space == NULL)
> goto cleanup;
>
> - assert(pParse->pNewTable == 0);
> - pParse->pNewTable = pTable;
> + assert(pParse->new_space == 0);
== NULL
>
> @@ -513,27 +478,28 @@ void
> sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
> {
> assert(type_def != NULL);
> - Table *p;
> int i;
> char *z;
> sqlite3 *db = pParse->db;
> - if ((p = pParse->pNewTable) == 0)
> + if ((pParse->new_space) == 0)
== NULL
> return;
> + struct space *space = 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) == NULL)
> + assert(space->def->opts.is_temporary);
> + if (sql_field_retrieve(pParse, space->def,
> + (uint32_t) space->def->field_count) == NULL)
field_count is already of type uint32_t.
> return;
> struct region *region = &pParse->region;
> z = 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] = 0;
> sqlite3NormalizeName(z);
> - for (i = 0; i < (int)p->def->field_count; i++) {
> - if (strcmp(z, p->def->fields[i].name) == 0) {
> + for (i = 0; i < (int)space->def->field_count; i++) {
for (uint32_t i = 0; space->def->field_count; ...)
> + if (strcmp(z, space->def->fields[i].name) == 0) {
> sqlite3ErrorMsg(pParse, "duplicate column name: %s", z);
> return;
> }
> }
>
>
> @@ -573,17 +540,18 @@ void
> sql_column_add_nullable_action(struct Parse *parser,
> enum on_conflict_action nullable_action)
> {
> - struct Table *p = parser->pNewTable;
> - if (p == NULL || NEVER(p->def->field_count < 1))
> + if (parser->new_space == NULL ||
> + NEVER(parser->new_space->def->field_count < 1))
Wrong indentation.
> return;
> - struct field_def *field = &p->def->fields[p->def->field_count - 1];
> + struct space *space = parser->new_space;
struct space_def *def = 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 = pParse->db;
> - p = pParse->pNewTable;
> - assert(p->def->opts.is_temporary);
> - if (p != 0) {
> + assert(pParse->new_space->def->opts.is_temporary);
> + if (pParse->new_space != 0) {
!= NULL (please, fix the rest of similar places).
> + struct space_def *space_def = pParse->new_space->def;
Call it simply “def” and you won’t 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);
>
>
> @@ -714,14 +682,14 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */
> }
> }
> if (nTerm == 1 && iCol != -1 &&
> - pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER &&
> - sortOrder != SORT_ORDER_DESC) {
Wrong indentation.
> + space->def->fields[iCol].type == FIELD_TYPE_INTEGER &&
> + sortOrder != SORT_ORDER_DESC) {
>
>
>
> @@ -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 == 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 = pParse->db; /* The database connection */
>
> if (pEnd == 0 && pSelect == 0) {
> return;
> }
> assert(!db->mallocFailed);
> - p = pParse->pNewTable;
> - if (p == 0)
> + if (pParse->new_space == 0)
> return;
>
> + struct space *new_space = pParse->new_space;
> +
Don’t abuse new lines.
> @@ -1820,13 +1788,15 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> char *parent_name = NULL;
> char *constraint_name = NULL;
> bool is_self_referenced = false;
> - /*
> - * Table under construction during CREATE TABLE
> + /**
/** -> /*
> + * Space under construction during CREATE TABLE
> * processing. NULL for ALTER TABLE statement handling.
> */
> - struct Table *new_tab = parse_context->pNewTable;
> - /* Whether we are processing ALTER TABLE or CREATE TABLE. */
> - bool is_alter = new_tab == NULL;
> + bool is_alter = (parse_context->new_space == NULL);
> + struct space *space;
> + if (!is_alter)
> + space = 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 = (parse_context->new_space == NULL);
- struct space *space;
- if (!is_alter)
- space = parse_context->new_space;
-
+ struct space *space = parse_context->new_space;
+ bool is_alter = space == NULL;
> @@ -2249,7 +2219,6 @@ sql_create_index(struct Parse *parse, struct Token *token,
> * Return early if not found.
> */
> struct space *space = NULL;
> - struct space_def *def = NULL;
Do you really need this diff? It results only in other extra diffs.
def makes handling shorter.
> if (tbl_name != NULL) {
> assert(token != NULL && token->z != NULL);
> const char *name = tbl_name->a[0].zName;
> @@ -2262,17 +2231,16 @@ sql_create_index(struct Parse *parse, struct Token *token,
> }
> goto exit_create_index;
> }
> - def = space->def;
> } else {
> - if (parse->pNewTable == NULL)
> + if (parse->new_space == NULL)
> goto exit_create_index;
> assert(token == NULL);
> assert(start == NULL);
> - space = parse->pNewTable->space;
> - def = parse->pNewTable->def;
> + space = parse->new_space;
> + space->def = 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 = sqlite3DbMallocZero(parse->db, sizeof(*table));
> - if (table == NULL)
> - return NULL;
> - table->def = space->def;
> - table->space = space;
> - table->nTabRef = 1;
> - tbl_name->pTab = table;
> - if (sqlite3IndexedByLookup(parse, tbl_name) != 0)
> - table = NULL;
> - return table;
> + space_name->space = space;
> + if (sqlite3IndexedByLookup(parse, space_name) != 0)
> + space = NULL;
> + return space;
> }
>
>
> @@ -435,7 +428,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> }
>
> 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 != NULL &&
> - (fkey_is_required(table->def->id, NULL) || trigger_list != NULL)) {
> + if (space != NULL &&
> + (fkey_is_required(space->def->id, NULL) ||
> + trigger_list != 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);
> }
>
> -/*
> - * 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 */
>
> /* Figure out if we have any triggers and if the table being
> * inserted into is a view
> */
> - trigger = sql_triggers_exist(pTab, TK_INSERT, NULL, &tmask);
> - bool is_view = pTab->def->opts.is_view;
> + trigger = sql_triggers_exist(space->def, TK_INSERT, NULL, &tmask);
> + struct space_def *space_def = space->def;
Swap this and previous lines pls, so that we pass
space_def to sql_triggers_exits (not space->def).
> + space_id = 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…
> @@ -218,7 +218,7 @@ lookupName(Parse * pParse, /* The parsing context */
> struct SrcList_item *pMatch = 0; /* The matching pSrcList item */
> NameContext *pTopNC = pNC; /* First namecontext in the list */
> int isTrigger = 0; /* True if resolved to a trigger column */
> - Table *pTab = 0; /* Table hold the row */
> + struct space_def *space_def;
>
> 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 = 0, pItem = pSrcList->a; i < pSrcList->nSrc;
> i++, pItem++) {
> - pTab = pItem->pTab;
> - assert(pTab != 0 && pTab->def->name != NULL);
> - assert(pTab->def->field_count > 0);
> + assert(pItem->space != NULL &&
> + pItem->space->def->name != 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 != 0 && cntTab == 0
> - && pParse->pTriggerTab != 0) {
> + && pParse->trigger_space != 0) {
> int op = pParse->eTriggerOp;
> assert(op == TK_DELETE || op == TK_UPDATE
> || op == TK_INSERT);
> if (op != TK_DELETE && sqlite3StrICmp("new", zTab) == 0) {
> pExpr->iTable = 1;
> - pTab = pParse->pTriggerTab;
> + space_def = pParse->trigger_space->def;
> } else if (op != TK_INSERT
> && sqlite3StrICmp("old", zTab) == 0) {
> pExpr->iTable = 0;
> - pTab = pParse->pTriggerTab;
> + space_def = pParse->trigger_space->def;
> } else {
> - pTab = 0;
> + space_def = 0;
NULL (and in other places too).
>
> @@ -1934,20 +1932,28 @@ cleanup:
> }
>
> /*
> +
> + */
> +
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,
Extra space after asterisk.
> @@ -2000,23 +2006,20 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
> while (pSelect->pPrior)
> pSelect = pSelect->pPrior;
> user_session->sql_flags = savedFlags;
> - Table *table = sql_ephemeral_table_new(pParse, NULL);
> - if (table == NULL)
> + struct space *space = sql_ephemeral_space_new(pParse, NULL);
> +
> + if (space == 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 = 1;
> - table->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
> - assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == 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 = "sqlite_sq_DEADBEAFDEADBEAF";
> - pFrom->pTab = pTab =
> - sql_ephemeral_table_new(pParse, name);
> - if (pTab == NULL)
> + struct space *space;
> + pFrom->space = space =
Avoid using double assignment at one time.
> @@ -5673,28 +5649,29 @@ sqlite3Select(Parse * pParse, /* The parser context */
> pItem->regReturn);
> pItem->addrFillSub = topAddr + 1;
> if (pItem->fg.isCorrelated == 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.
>
> @@ -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’t 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.
> /**
> * Generate code for a DELETE FROM statement.
> @@ -3603,10 +3581,11 @@ int sqlite3WhereOkOnePass(WhereInfo *, int *);
>
> /**
> * 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);
>
> /**
> - * 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-01-29 14:59 ` [tarantool-patches] " n.pettik
@ 2019-02-01 11:05 ` Ivan Koptelov
2019-02-04 19:22 ` n.pettik
0 siblings, 1 reply; 10+ messages in thread
From: Ivan Koptelov @ 2019-02-01 11:05 UTC (permalink / raw)
To: n.pettik, tarantool-patches
[-- Attachment #1: Type: text/html, Size: 107668 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-02-01 11:05 ` Ivan Koptelov
@ 2019-02-04 19:22 ` n.pettik
2019-02-06 17:17 ` [tarantool-patches] " i.koptelov
0 siblings, 1 reply; 10+ messages in thread
From: n.pettik @ 2019-02-04 19:22 UTC (permalink / raw)
To: tarantool-patches; +Cc: Ivan Koptelov
> On 1 Feb 2019, at 14:05, Ivan Koptelov <ivan.koptelov@tarantool.org> wrote:
>
> 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’t 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.
>>
>> 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.
>>
> Fixed.
>> Also, for some reason content of this letter looks very strange:
>> at least it contains unusual fonts… I am not complaining but
>> are you sure that you set up git-email correctly?
>>
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.
>>> @@ -435,7 +428,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>> }
>>>
>>> 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 != NULL &&
>>> - (fkey_is_required(table->def->id, NULL) || trigger_list != NULL)) {
>>> + if (space != NULL &&
>>> + (fkey_is_required(space->def->id, NULL) ||
>>> + trigger_list != NULL)) {
>>>
>> 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 != NULL &&
- (fkey_is_required(space->def->id, NULL) ||
- trigger_list != NULL)) {
+ (fkey_is_required(space->def->id, NULL) || trigger_list != NULL)) {
/* Mask of OLD.* columns in use */
/* TODO: Could use temporary registers here. */
uint32_t mask =
>>>
>>> +/**
>>> * 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?
> Because the length of it's lines exceeded 65 chars.
But this fix is not related to patch: it doesn’t even contain words like ’table’.
>>> @@ -2000,23 +2006,20 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
>>> while (pSelect->pPrior)
>>> pSelect = pSelect->pPrior;
>>> user_session->sql_flags = savedFlags;
>>> - Table *table = sql_ephemeral_table_new(pParse, NULL);
>>> - if (table == NULL)
>>> + struct space *space = sql_ephemeral_space_new(pParse, NULL);
>>> +
>>> + if (space == 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.
> Empty line is removed.
I mean not (only) empty line, but fix of the comment. Nevermind.
>>> */
>>> assert(db->lookaside.bDisable);
>>> - table->nTabRef = 1;
>>> - table->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
>>> - assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == 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 *);
>>>
>>> /**
>>> * 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.
> This diff fixes typo 'garanteeed' -> ‘guaranteed'
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);
}
- struct space *child_space = space_by_id(fkey->child_id);
+ struct space *child_space = src->a[0].space;
> diff --git a/src/box/
> schema.cc b/src/box/schema.cc
>
> index 8625d92ea..2d12f01d0 100644
> --- a/src/box/
> schema.cc
>
> +++ b/src/box/
> schema.cc
>
> @@ -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 = { 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 == 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 == 0)
> + if (space != NULL || db != NULL|| db->pnBytesFreed == 0)
> return;
>
> if (space->def->opts.is_temporary) {
> @@ -310,35 +310,13 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName)
> }
>
> 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 == 0) && (--pTable->nTabRef) > 0))
+ if (space != NULL || db != NULL || db->pnBytesFreed == 0)
return;
You refactored this if-clause in a wrong way.
if (!db) <=> if (db == NULL)
if (!space) <=> if (space == NULL)
Fix it since it leads to numerous leaks.
What is more, you don’t 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;
}
-/**
- * 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 == TRUE.
- *
- * @param db Database handler.
- * @param space Space to be deleted.
- */
-void
-sql_space_delete(struct sqlite3 *db, struct space *space)
-{
- if (space != NULL || db != NULL|| db->pnBytesFreed == 0)
- return;
-
- if (space->def->opts.is_temporary) {
- for (uint32_t i = 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 == NULL ||
+ !pItem->space->def->opts.is_temporary ||
+ (pItem->space->index == NULL &&
+ pItem->space->def->opts.checks == 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);
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;
}
+/**
+ * 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 == NULL || db == NULL || db->pnBytesFreed == 0)
+ return;
+ assert(space->def->opts.is_temporary);
+ for (uint32_t i = 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 = 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’t have to free anything at all.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] [PATCH] sql: remove struct Table
2019-02-04 19:22 ` n.pettik
@ 2019-02-06 17:17 ` i.koptelov
2019-02-11 17:58 ` [tarantool-patches] " n.pettik
0 siblings, 1 reply; 10+ messages in thread
From: i.koptelov @ 2019-02-06 17:17 UTC (permalink / raw)
To: n.pettik; +Cc: tarantool-patches
>> On 1 Feb 2019, at 14:05, Ivan Koptelov<ivan.koptelov@tarantool.org> wrote:
>>
>> 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’t 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.
Sorry for this. All review fixes below is inlined.
>>>> 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.
>>>
>>> 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.
>>>
>> Fixed.
>>> Also, for some reason content of this letter looks very strange:
>>> at least it contains unusual fonts… I am not complaining but
>>> are you sure that you set up git-email correctly?
>>>
> 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.
Fixed:
@@ -471,7 +445,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
return;
}
#endif
- /**
+ /*
* As sql_field_retrieve will allocate memory on region
* ensure that p->space->def is also temporal and would be
* dropped.
>>>> @@ -435,7 +428,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>>> }
>>>> 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 != NULL &&
>>>> - (fkey_is_required(table->def->id, NULL) || trigger_list != NULL)) {
>>>> + if (space != NULL &&
>>>> + (fkey_is_required(space->def->id, NULL) ||
>>>> + trigger_list != NULL)) {
>>>>
>>> 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 != NULL &&
> - (fkey_is_required(space->def->id, NULL) ||
> - trigger_list != NULL)) {
> + (fkey_is_required(space->def->id, NULL) || trigger_list != NULL)) {
> /* Mask of OLD.* columns in use */
> /* TODO: Could use temporary registers here. */
> uint32_t mask =
Sorry.
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index e8354f7e4..143803f9d 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 != NULL &&
- (fkey_is_required(space->def->id, NULL) ||
- trigger_list != NULL)) {
+ (fkey_is_required(space, NULL) || trigger_list != NULL)) {
>>>> +/**
>>>> * 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?
>> Because the length of it's lines exceeded 65 chars.
> But this fix is not related to patch: it doesn’t even contain words like ’table’.
Ok, let's keep things to the point. Reverted changes.
>>>> @@ -2000,23 +2006,20 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
>>>> while (pSelect->pPrior)
>>>> pSelect = pSelect->pPrior;
>>>> user_session->sql_flags = savedFlags;
>>>> - Table *table = sql_ephemeral_table_new(pParse, NULL);
>>>> - if (table == NULL)
>>>> + struct space *space = sql_ephemeral_space_new(pParse, NULL);
>>>> +
>>>> + if (space == 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.
>> Empty line is removed.
> I mean not (only) empty line, but fix of the comment. Nevermind.
>
>>>> */
>>>> assert(db->lookaside.bDisable);
>>>> - table->nTabRef = 1;
>>>> - table->tuple_log_count = DEFAULT_TUPLE_LOG_COUNT;
>>>> - assert(sqlite3LogEst(DEFAULT_TUPLE_COUNT) == 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;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 79b8eafca..b30aa6200 100644
@@ -1998,13 +1993,13 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
user_session->sql_flags &= SQLITE_ShortColNames;
sqlite3SelectPrep(pParse, pSelect, 0);
if (pParse->nErr)
- return 0;
+ return NULL;
while (pSelect->pPrior)
pSelect = pSelect->pPrior;
user_session->sql_flags = savedFlags;
struct space *space = sql_ephemeral_space_new(pParse, NULL);
if (space == NULL)
- return 0;
+ return NULL;
/* The sqlite3ResultSetOfSelect() is only used in contexts where lookaside
* is disabled
*/
@@ -2012,7 +2007,7 @@ sqlite3ResultSetOfSelect(Parse * pParse, Select * pSelect)
sqlite3ColumnsFromExprList(pParse, pSelect->pEList, space->def);
sqlite3SelectAddColumnTypeAndCollation(pParse, space->def, pSelect);
if (db->mallocFailed)
- return 0;
+ return NULL;
return space;
}
>
>>>> /**
>>>> * Generate code for a DELETE FROM statement.
>>>> @@ -3603,10 +3581,11 @@ int sqlite3WhereOkOnePass(WhereInfo *, int *);
>>>> /**
>>>> * 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.
>> This diff fixes typo 'garanteeed' -> ‘guaranteed'
> It is not related to patch. Instead, it enlarges diff to be reviewed.
Ok, changes reverted.
> 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.
Ok, done.
diff --git a/src/box/sql.h b/src/box/sql.h
index d0b654e9c..e7b3933b9 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -172,14 +172,6 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select);
struct Expr*
space_column_default_expr(uint32_t space_id, uint32_t fieldno);
-/**
- * Get server checks list by space_id.
- * @param space_id Space ID.
- * @retval Checks list.
- */
-struct ExprList *
-space_checks_expr_list(uint32_t space_id);
-
/**
* Return the number of bytes required to create a duplicate of the
* expression passed as the first argument. The second argument is a
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 599ece6d5..4b055529c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -789,16 +763,6 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
return field->coll;
}
-struct ExprList *
-space_checks_expr_list(uint32_t space_id)
-{
- struct space *space;
- space = space_by_id(space_id);
- assert(space != NULL);
- assert(space->def != NULL);
- return space->def->opts.checks;
-}
-
int
vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
struct space *space)
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index e8354f7e4..143803f9d 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -149,7 +149,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
assert(space != NULL);
trigger_list = sql_triggers_exist(space->def, TK_DELETE, NULL, NULL);
bool is_complex = trigger_list != NULL ||
- fkey_is_required(space->def->id, NULL);
+ fkey_is_required(space, NULL);
bool is_view = space->def->opts.is_view;
/* If table is really a view, make sure it has been
@@ -458,8 +458,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
* use for the old.* references in the triggers.
*/
if (space != NULL &&
- (fkey_is_required(space->def->id, NULL) ||
- trigger_list != NULL)) {
+ (fkey_is_required(space, NULL) || trigger_list != NULL)) {
/* Mask of OLD.* columns in use */
/* TODO: Could use temporary registers here. */
uint32_t mask =
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 61cfb6fd7..28b53e7fd 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);
}
- struct space *child_space = space_by_id(fkey->child_id);
+ struct space *child_space = src->a[0].space;
assert(child_space != NULL);
/*
* Create an Expr object representing an SQL expression
@@ -672,9 +672,8 @@ fkey_emit_check(struct Parse *parser, struct space *space, int reg_old,
}
bool
-fkey_is_required(uint32_t space_id, const int *changes)
+fkey_is_required(struct space *space, const int *changes)
{
- struct space *space = space_by_id(space_id);
if (changes == NULL) {
/*
* A DELETE operation. FK processing is required
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index f28567dd1..6bd716767 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -929,7 +929,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
if (on_conflict == ON_CONFLICT_ACTION_DEFAULT)
on_conflict = ON_CONFLICT_ACTION_ABORT;
/* Test all CHECK constraints. */
- struct ExprList *checks = space_checks_expr_list(def->id);
+ struct ExprList *checks = def->opts.checks;
enum on_conflict_action on_conflict_check = on_conflict;
if (on_conflict == ON_CONFLICT_ACTION_REPLACE)
on_conflict_check = ON_CONFLICT_ACTION_ABORT;
@@ -1244,8 +1244,8 @@ xferOptimization(Parse * pParse, /* Parser context */
return 0;
}
/* Get server checks. */
- ExprList *pCheck_src = space_checks_expr_list(src->def->id);
- ExprList *pCheck_dest = space_checks_expr_list(dest->def->id);
+ ExprList *pCheck_src = src->def->opts.checks;
+ ExprList *pCheck_dest = dest->def->opts.checks;
if (pCheck_dest != NULL &&
sqlite3ExprListCompare(pCheck_src, pCheck_dest, -1) != 0) {
/* Tables have different CHECK constraints. Ticket #2252 */
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index c9ec9b1ca..c4fdccec0 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4750,12 +4743,12 @@ fkey_emit_actions(struct Parse *parser, struct space *space, int reg_old,
* changes[] array is set to -1. If the column is modified,
* the value is 0 or greater.
*
- * @param space_id Id of space to be modified.
+ * @param space Space to be modified.
* @param changes Array of modified fields for UPDATE.
* @retval True, if any foreign key processing will be required.
*/
bool
-fkey_is_required(uint32_t space_id, const int *changes);
+fkey_is_required(struct space *space, const int *changes);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index f7b7dc10d..a7affa954 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -201,7 +201,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
*/
pTabList->a[0].colUsed = 0;
- hasFK = fkey_is_required(space->def->id, aXRef);
+ hasFK = fkey_is_required(space, aXRef);
/* Begin generating code. */
v = sqlite3GetVdbe(pParse);
> 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);
> }
> - struct space *child_space = space_by_id(fkey->child_id);
> + struct space *child_space = src->a[0].space;
Done.
>> diff --git a/src/box/
>> schema.cc b/src/box/schema.cc
>>
>> index 8625d92ea..2d12f01d0 100644
>> --- a/src/box/
>> schema.cc
>>
>> +++ b/src/box/
>> schema.cc
>>
>> @@ -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 = { 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 == 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?
Sorry, they should not be there (auto-refactor error)
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2d12f01d0..8625d92ea 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -193,8 +193,8 @@ space_cache_replace(struct space *old_space, struct space *new_space)
mh_strnptr_del(spaces_by_name, k, NULL);
}
/*
- * Insert @updated_space into @spaces cache,
- * replacing @old_space if it's not NULL.
+ * Insert @new_space into @spaces cache, replacing
+ * @old_space if it's not NULL.
*/
const struct mh_i32ptr_node_t node_p = { 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 == old_space);
(void)old_space_by_id;
/*
- * Insert @updated_space into @spaces_by_name cache.
+ * Insert @new_space into @spaces_by_name cache.
*/
const char *name = space_name(new_space);
uint32_t name_len = strlen(name);
>> /**
>> 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 == 0)
>> + if (space != NULL || db != NULL|| db->pnBytesFreed == 0)
>> return;
>> if (space->def->opts.is_temporary) {
>> @@ -310,35 +310,13 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName)
>> }
>> 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.
Done
@@ -309,7 +280,10 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName)
return SQLITE_OK;
}
-struct index *
+/**
+ * Return the PRIMARY KEY index of a table.
+ */
+static struct index *
sql_space_primary_key(const struct space *space)
{
if (space->index_count == 0 || space->index[0]->def->iid != 0)
@@ -3331,12 +3331,6 @@ void
sqlite3SelectAddColumnTypeAndCollation(Parse *, struct space_def *, Select *);
struct space *sqlite3ResultSetOfSelect(Parse *, Select *);
-/**
- * Return the PRIMARY KEY index of a table.
- */
-struct index *
-sql_space_primary_key(const struct space *space);
-
void sqlite3StartTable(Parse *, Token *, int);
void sqlite3AddColumn(Parse *, Token *, struct type_def *);
> 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.
Done.
@@ -234,16 +226,6 @@ sql_expr_delete(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
struct space *
sql_ephemeral_space_new(struct Parse *parser, const char *name);
-/**
- * Create and initialize a new ephemeral space_def object.
- * @param parser SQL Parser object.
- * @param name Name of space to be created.
- * @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);
-
/**
* Duplicate Expr list.
* The flags parameter contains a combination of the EXPRDUP_XXX
@@ -1233,7 +1233,14 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
return space->def->fields[fieldno].default_value_expr;
}
-struct space_def *
+/**
+ * Create and initialize a new ephemeral space_def object.
+ * @param parser SQL Parser object.
+ * @param name Name of space to be created.
+ * @retval NULL on memory allocation error, Parser state changed.
+ * @retval not NULL on success.
+ */
+static struct space_def *
sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
{
struct space_def *def = NULL;
> 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 == 0) && (--pTable->nTabRef) > 0))
> + if (space != NULL || db != NULL || db->pnBytesFreed == 0)
> return;
>
> You refactored this if-clause in a wrong way.
>
> if (!db) <=> if (db == NULL)
> if (!space) <=> if (space == NULL)
>
> Fix it since it leads to numerous leaks.
Sorry, fixed.
> What is more, you don’t 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;
> }
> -/**
> - * 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 == TRUE.
> - *
> - * @param db Database handler.
> - * @param space Space to be deleted.
> - */
> -void
> -sql_space_delete(struct sqlite3 *db, struct space *space)
> -{
> - if (space != NULL || db != NULL|| db->pnBytesFreed == 0)
> - return;
> -
> - if (space->def->opts.is_temporary) {
> - for (uint32_t i = 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 == NULL ||
> + !pItem->space->def->opts.is_temporary ||
> + (pItem->space->index == NULL &&
> + pItem->space->def->opts.checks == 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);
> 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;
> }
> +/**
> + * 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 == NULL || db == NULL || db->pnBytesFreed == 0)
> + return;x
> + assert(space->def->opts.is_temporary);
> + for (uint32_t i = 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 = 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’t have to free anything at all.
>
>
>
Done. The diff is identical to yours.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-02-06 17:17 ` [tarantool-patches] " i.koptelov
@ 2019-02-11 17:58 ` n.pettik
2019-02-12 13:55 ` i.koptelov
0 siblings, 1 reply; 10+ messages in thread
From: n.pettik @ 2019-02-11 17:58 UTC (permalink / raw)
To: tarantool-patches; +Cc: Ivan Koptelov
> On 6 Feb 2019, at 20:17, i.koptelov <ivan.koptelov@tarantool.org> wrote:
>
>>> On 1 Feb 2019, at 14:05, Ivan Koptelov<ivan.koptelov@tarantool.org> wrote:
>>>
>>> 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’t 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.
> Sorry for this. All review fixes below is inlined.
Well, thank you. But don’t put fixes in a separate commit, at least when
you push your branch to the remote repository. You may *accidentally*
fail during final commit squashing and no-one will notice that.
Travis status was entirely negative: It can’t be compiled.
You forgot to add forward declaration of space_def.
I fixed that and a couple of minor issues more. Here is a diff
(I’ve squashed your commits and pushed on top of your branch).
Please, check it out. If it is OK, squash them and then patch LGTM.
diff --git a/src/box/sql.h b/src/box/sql.h
index e7b3933b9..aef4cd3b0 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -67,6 +67,7 @@ struct Parse;
struct Select;
struct Table;
struct sql_trigger;
+struct space_def;
/**
* Perform parsing of provided expression. This is done by
@@ -278,8 +279,9 @@ sql_expr_list_append(struct sqlite3 *db, struct ExprList *expr_list,
* @param expr_list Expression list to resolve. May be NUL.
*/
void
-sql_resolve_self_reference(struct Parse *parser, struct space_def *def, int type,
- struct Expr *expr, struct ExprList *expr_list);
+sql_resolve_self_reference(struct Parse *parser, struct space_def *def,
+ int type, struct Expr *expr,
+ struct ExprList *expr_list);
/**
* Initialize check_list_item.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 4b055529c..7f9e5f6e6 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -282,6 +282,17 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName)
/**
* Return the PRIMARY KEY index of a table.
+ *
+ * Note that during parsing routines this function is not equal
+ * to space_index(space, 0); call since primary key can be added
+ * after seconary keys:
+ *
+ * CREATE TABLE t (a INT UNIQUE, b PRIMARY KEY);
+ *
+ * In this particular case, after secondary index processing
+ * space still lacks PK, but index[0] != NULL since index array
+ * is filled in a straightforward way. Hence, function must
+ * return NULL.
*/
static struct index *
sql_space_primary_key(const struct space *space)
@@ -447,8 +458,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
#endif
/*
* As sql_field_retrieve will allocate memory on region
- * ensure that p->space->def is also temporal and would be
- * dropped.
+ * ensure that def is also temporal and would be dropped.
*/
assert(def->opts.is_temporary);
if (sql_field_retrieve(pParse, def, def->field_count) == NULL)
@@ -598,9 +608,9 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */
{
int iCol = -1, i;
int nTerm;
- if (pParse->updated_space == NULL)
- goto primary_key_exit;
struct space *space = pParse->updated_space;
+ if (space == NULL)
+ goto primary_key_exit;
if (sql_space_primary_key(space) != NULL) {
sqlite3ErrorMsg(pParse,
"table \"%s\" has more than one primary key",
@@ -742,17 +752,14 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
struct space *space = space_by_id(def->id);
/*
* It is not always possible to fetch collation directly
- * from struct space. To be more precise when:
- * 1. space is ephemeral. Thus, its id is zero and
- * it can't be found in space cache.
- * 2. space is a view. Hence, it lacks any functional
- * parts such as indexes or fields.
- * 3. space is under construction. So, the same as p.1
- * it can't be found in space cache.
- * In cases mentioned above collation is fetched from
- * SQL specific structures.
+ * from struct space due to its absence in space cache.
+ * To be more precise when space is ephemeral or it is
+ * under construction.
+ *
+ * In cases mentioned above collation is fetched by id.
*/
- if (space == NULL || space_index(space, 0) == NULL) {
+ if (space == NULL) {
+ assert(def->opts.is_temporary);
assert(column < (uint32_t)def->field_count);
*coll_id = def->fields[column].coll_id;
struct coll_id *collation = coll_by_id(*coll_id);
@@ -1157,10 +1164,9 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
return;
}
assert(!db->mallocFailed);
- if (pParse->updated_space == NULL)
- return;
-
struct space *new_space = pParse->updated_space;
+ if (new_space == NULL)
+ return;
assert(!db->init.busy);
if (!new_space->def->opts.is_view) {
@@ -1290,9 +1296,9 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
goto create_view_fail;
}
sqlite3StartTable(parse_context, name, if_exists);
- if (parse_context->updated_space == NULL || parse_context->nErr != 0)
- goto create_view_fail;
struct space *space = parse_context->updated_space;
+ if (space == NULL || parse_context->nErr != 0)
+ goto create_view_fail;
struct space *select_res_space =
sqlite3ResultSetOfSelect(parse_context, select);
@@ -2047,7 +2053,6 @@ index_fill_def(struct Parse *parse, struct index *index,
"region", "key parts");
goto tnt_error;
}
-
for (int i = 0; i < expr_list->nExpr; i++) {
struct Expr *expr = expr_list->a[i].pExpr;
sql_resolve_self_reference(parse, space_def, NC_IdxExpr,
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 143803f9d..8c4ad3146 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -146,10 +146,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct space *space = sql_lookup_space(parse, tab_list->a);
if (space == NULL)
goto delete_from_cleanup;
- assert(space != NULL);
trigger_list = sql_triggers_exist(space->def, TK_DELETE, NULL, NULL);
- bool is_complex = trigger_list != NULL ||
- fkey_is_required(space, NULL);
+ bool is_complex = trigger_list != NULL || fkey_is_required(space, NULL);
bool is_view = space->def->opts.is_view;
/* If table is really a view, make sure it has been
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 6bd716767..d630524b3 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -114,7 +114,7 @@ sql_space_autoinc_fieldno(struct space *space)
/**
* This routine is used to see if a statement of the form
* "INSERT INTO <table> SELECT ..." can run for the results of the
- * SELECT.
+ * SELECT. Otherwise, it may fall into infinite loop.
*
* @param parser Parse context.
* @param space_def Space definition.
@@ -124,7 +124,7 @@ sql_space_autoinc_fieldno(struct space *space)
* @retval false else.
*/
static bool
-vdbe_has_table_read(struct Parse *parser, const struct space_def *space_def)
+vdbe_has_space_read(struct Parse *parser, const struct space_def *space_def)
{
struct Vdbe *v = sqlite3GetVdbe(parser);
int last_instr = sqlite3VdbeCurrentAddr(v);
@@ -456,7 +456,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
* the SELECT statement. Also use a temp table in
* the case of row triggers.
*/
- if (trigger != NULL || vdbe_has_table_read(pParse, space_def))
+ if (trigger != NULL || vdbe_has_space_read(pParse, space_def))
useTempTable = 1;
if (useTempTable) {
@@ -1210,9 +1210,7 @@ xferOptimization(Parse * pParse, /* Parser context */
/* Affinity must be the same on all columns. */
if (dest_affinity != src_affinity)
return 0;
- uint32_t id;
- if (sql_column_collation(dest->def, i, &id) !=
- sql_column_collation(src->def, i, &id))
+ if (dest->def->fields[i].coll_id != src->def->fields[i].coll_id)
return 0;
if (!dest->def->fields[i].is_nullable &&
src->def->fields[i].is_nullable)
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 86d51280d..d3618d207 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -218,7 +218,6 @@ lookupName(Parse * pParse, /* The parsing context */
struct SrcList_item *pMatch = 0; /* The matching pSrcList item */
NameContext *pTopNC = pNC; /* First namecontext in the list */
int isTrigger = 0; /* True if resolved to a trigger column */
- struct space_def *space_def;
assert(pNC); /* the name context cannot be NULL. */
assert(zCol); /* The Z in X.Y.Z cannot be NULL */
@@ -311,11 +310,12 @@ lookupName(Parse * pParse, /* The parsing context */
/* If we have not already resolved the name, then maybe
* it is a new.* or old.* trigger argument reference
*/
- if (zTab != NULL && cntTab == 0
- && pParse->updated_space != NULL) {
+ if (zTab != NULL && cntTab == 0 &&
+ pParse->updated_space != NULL) {
int op = pParse->eTriggerOp;
assert(op == TK_DELETE || op == TK_UPDATE
|| op == TK_INSERT);
+ struct space_def *space_def = NULL;
if (op != TK_DELETE && sqlite3StrICmp("new", zTab) == 0) {
pExpr->iTable = 1;
space_def = pParse->updated_space->def;
@@ -323,8 +323,6 @@ lookupName(Parse * pParse, /* The parsing context */
&& sqlite3StrICmp("old", zTab) == 0) {
pExpr->iTable = 0;
space_def = pParse->updated_space->def;
- } else {
- space_def = NULL;
}
if (space_def != NULL) {
@@ -1614,8 +1612,9 @@ sqlite3ResolveSelectNames(Parse * pParse, /* The parser context */
}
void
-sql_resolve_self_reference(struct Parse *parser, struct space_def *def, int type,
- struct Expr *expr, struct ExprList *expr_list)
+sql_resolve_self_reference(struct Parse *parser, struct space_def *def,
+ int type, struct Expr *expr,
+ struct ExprList *expr_list)
{
/* Fake SrcList for parser->updated_space */
SrcList sSrc;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index b30aa6200..da602a9c0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -421,8 +421,7 @@ sqlite3JoinType(Parse * pParse, Token * pA, Token * pB, Token * pC)
static int
columnIndex(struct space_def *def, const char *zCol)
{
- int i;
- for (i = 0; i < (int)def->field_count; i++) {
+ for (uint32_t i = 0; i < def->field_count; i++) {
if (strcmp(def->fields[i].name, zCol) == 0)
return i;
}
@@ -1943,8 +1942,9 @@ cleanup:
* statement be resolved.
*/
void
-sqlite3SelectAddColumnTypeAndCollation(Parse *pParse, struct space_def *def,
- Select *pSelect)
+sqlite3SelectAddColumnTypeAndCollation(struct Parse *pParse,
+ struct space_def *def,
+ struct Select *pSelect)
{
sqlite3 *db = pParse->db;
NameContext sNC;
@@ -4021,7 +4021,8 @@ flattenSubquery(Parse * pParse, /* Parsing context */
return 1;
}
- /* Begin flattening the iFrom-th entry of the FROM clause
+ /*
+ * Begin flattening the iFrom-th entry of the FROM clause
* in the outer query.
*/
pSub = pSub1 = pSubitem->pSelect;
@@ -4599,7 +4600,7 @@ withExpand(Walker * pWalker, struct SrcList_item *pFrom)
/* Check if this is a recursive CTE. */
pSel = pFrom->pSelect;
bMayRecursive = (pSel->op == TK_ALL || pSel->op == TK_UNION);
- int ref_counter = 0;
+ uint32_t ref_counter = 0;
if (bMayRecursive) {
int i;
SrcList *pSrc = pFrom->pSelect->pSrc;
@@ -4770,13 +4771,12 @@ selectExpander(Walker * pWalker, Select * p)
* unique identifier.
*/
const char *name = "sqlite_sq_DEADBEAFDEADBEAF";
- pFrom->space =
- sql_ephemeral_space_new(sqlite3ParseToplevel(pParse),
- name);
- struct space *space = pFrom->space;
-
+ struct space *space =
+ sql_ephemeral_space_new(sqlite3ParseToplevel(pParse),
+ name);
if (space == NULL)
return WRC_Abort;
+ pFrom->space = space;
/*
* Rewrite old name with correct pointer.
*/
@@ -4786,7 +4786,7 @@ selectExpander(Walker * pWalker, Select * p)
pSel = pSel->pPrior;
}
sqlite3ColumnsFromExprList(pParse, pSel->pEList,
- space->def);
+ space->def);
} else {
/*
* An ordinary table or view name in the
@@ -4808,10 +4808,7 @@ selectExpander(Walker * pWalker, Select * p)
pFrom->pSelect = select;
sqlite3SelectSetName(pFrom->pSelect,
space->def->name);
- int columns = space->def->field_count;
- space->def->field_count = -1;
sqlite3WalkSelect(pWalker, pFrom->pSelect);
- space->def->field_count = columns;
}
}
/* Locate the index named by the INDEXED BY clause, if any. */
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 874fd682f..1352f6efe 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2313,7 +2313,8 @@ struct SrcList {
struct SrcList_item {
char *zName; /* Name of the table */
char *zAlias; /* The "B" part of a "A AS B" phrase. zName is the "A" */
- struct space *space; /* A space corresponding to zName */
+ /** A space corresponding to zName */
+ struct space *space;
Select *pSelect; /* A SELECT statement used in place of a table name */
int addrFillSub; /* Address of subroutine to manifest a subquery */
int regReturn; /* Register holding return address of addrFillSub */
@@ -2759,6 +2760,9 @@ struct Parse {
TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
With *pWith; /* Current WITH clause, or NULL */
With *pWithToFree; /* Free this WITH object at the end of the parse */
+ /** Space triggers are being coded for. */
+ struct space *triggered_space;
+ /** A space being constructed by CREATE TABLE */
/**
* Number of FK constraints declared within
* CREATE TABLE statement.
@@ -3480,7 +3484,7 @@ void sqlite3IdListDelete(sqlite3 *, IdList *);
*
* @param parse All information about this parse.
* @param token Index name. May be NULL.
- * @param tbl_name Table to index. Use pParse->updated_space ifNULL.
+ * @param tbl_name Table to index. Use pParse->updated_space if NULL.
* @param col_list A list of columns to be indexed.
* @param start The CREATE token that begins this statement.
* @param sort_order Sort order of primary key when pList==NULL.
@@ -3510,7 +3514,7 @@ Select *sqlite3SelectNew(Parse *, ExprList *, SrcList *, Expr *, ExprList *,
Expr *, ExprList *, u32, Expr *, Expr *);
/**
- * While a SrcList can in general represent multiple tables and
+ * While a SrcList can in general represent multiple spaces and
* subqueries (as in the FROM clause of a SELECT statement) in
* this case it contains the name of a single table, as one might
* find in an INSERT, DELETE, or UPDATE statement. Look up that
What is more, I returned back separation into new_space and trigger_space.
The reason is that I am working on another patch where I replace new_space
with struct create_table_def - structure assembling all required arguments for
parsing routines. So, we won’t be able to store new_space and trigger_space
in one struct. Here is diff:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 7f9e5f6e6..1cae2bb2c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -312,7 +312,7 @@ sql_space_primary_key(const struct space *space)
* when the "TEMP" or "TEMPORARY" keyword occurs in between
* CREATE and TABLE.
*
- * The new table record is initialized and put in pParse->updated_space.
+ * The new table record is initialized and put in pParse->new_space.
* As more of the CREATE TABLE statement is parsed, additional action
* routines will be called to add more information to this record.
* At the end of the CREATE TABLE statement, the sqlite3EndTable() routine
@@ -358,8 +358,8 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
strcpy(new_space->def->engine_name,
sql_storage_engine_strs[current_session()->sql_default_engine]);
- assert(pParse->updated_space == NULL);
- pParse->updated_space = new_space;
+ assert(pParse->new_space == NULL);
+ pParse->new_space = new_space;
if (!db->init.busy && (v = sqlite3GetVdbe(pParse)) != 0)
sql_set_multi_write(pParse, true);
@@ -446,9 +446,9 @@ sqlite3AddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
assert(type_def != NULL);
char *z;
sqlite3 *db = pParse->db;
- if (pParse->updated_space == NULL)
+ if (pParse->new_space == NULL)
return;
- struct space_def *def = pParse->updated_space->def;
+ struct space_def *def = pParse->new_space->def;
#if SQLITE_MAX_COLUMN
if ((int)def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) {
@@ -501,10 +501,10 @@ void
sql_column_add_nullable_action(struct Parse *parser,
enum on_conflict_action nullable_action)
{
- if (parser->updated_space == NULL ||
- NEVER(parser->updated_space->def->field_count < 1))
+ if (parser->new_space == NULL ||
+ NEVER(parser->new_space->def->field_count < 1))
return;
- struct space_def *def = parser->updated_space->def;
+ struct space_def *def = parser->new_space->def;
struct field_def *field = &def->fields[def->field_count - 1];
if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
nullable_action != field->nullable_action) {
@@ -538,9 +538,9 @@ void
sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
{
sqlite3 *db = pParse->db;
- assert(pParse->updated_space->def->opts.is_temporary);
- if (pParse->updated_space != NULL) {
- struct space_def *def = pParse->updated_space->def;
+ assert(pParse->new_space->def->opts.is_temporary);
+ if (pParse->new_space != NULL) {
+ struct space_def *def = pParse->new_space->def;
if (!sqlite3ExprIsConstantOrFunction
(pSpan->pExpr, db->init.busy)) {
sqlite3ErrorMsg(pParse,
@@ -608,7 +608,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */
{
int iCol = -1, i;
int nTerm;
- struct space *space = pParse->updated_space;
+ struct space *space = pParse->new_space;
if (space == NULL)
goto primary_key_exit;
if (sql_space_primary_key(space) != NULL) {
@@ -688,8 +688,8 @@ void
sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span)
{
struct Expr *expr = span->pExpr;
- if (parser->updated_space != NULL) {
- struct space *space = parser->updated_space;
+ if (parser->new_space != NULL) {
+ struct space *space = parser->new_space;
expr->u.zToken =
sqlite3DbStrNDup(parser->db, (char *)span->zStart,
(int)(span->zEnd - span->zStart));
@@ -719,9 +719,9 @@ release_expr:
void
sqlite3AddCollateType(Parse * pParse, Token * pToken)
{
- if (pParse->updated_space == NULL)
+ if (pParse->new_space == NULL)
return;
- struct space *space = pParse->updated_space;
+ struct space *space = pParse->new_space;
uint32_t i = space->def->field_count - 1;
sqlite3 *db = pParse->db;
char *zColl = sqlite3NameFromToken(db, pToken);
@@ -839,7 +839,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
memcpy(raw, index_parts, index_parts_sz);
index_parts = raw;
- if (parse->updated_space != NULL) {
+ if (parse->new_space != NULL) {
sqlite3VdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg);
sqlite3VdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + 1);
} else {
@@ -884,7 +884,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
int iRecord = (pParse->nMem += 7);
struct region *region = &pParse->region;
uint32_t table_opts_stmt_sz = 0;
- struct space *space = pParse->updated_space;
+ struct space *space = pParse->new_space;
char *table_opts_stmt = sql_encode_table_opts(region, space->def, zStmt,
&table_opts_stmt_sz);
if (table_opts_stmt == NULL)
@@ -1029,14 +1029,14 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
* of <CREATE TABLE ...> statement, we don't have child
* id, but we know register where it will be stored.
*/
- if (parse_context->updated_space != NULL) {
+ if (parse_context->new_space != NULL) {
sqlite3VdbeAddOp2(vdbe, OP_SCopy, fk->child_id,
constr_tuple_reg + 1);
} else {
sqlite3VdbeAddOp2(vdbe, OP_Integer, fk->child_id,
constr_tuple_reg + 1);
}
- if (parse_context->updated_space != NULL && fkey_is_self_referenced(fk)) {
+ if (parse_context->new_space != NULL && fkey_is_self_referenced(fk)) {
sqlite3VdbeAddOp2(vdbe, OP_SCopy, fk->parent_id,
constr_tuple_reg + 2);
} else {
@@ -1099,7 +1099,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
constr_tuple_reg + 9);
sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
constr_tuple_reg + 9);
- if (parse_context->updated_space == NULL)
+ if (parse_context->new_space == NULL)
sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
vdbe->nOp - 1);
@@ -1164,7 +1164,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
return;
}
assert(!db->mallocFailed);
- struct space *new_space = pParse->updated_space;
+ struct space *new_space = pParse->new_space;
if (new_space == NULL)
return;
assert(!db->init.busy);
@@ -1296,7 +1296,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
goto create_view_fail;
}
sqlite3StartTable(parse_context, name, if_exists);
- struct space *space = parse_context->updated_space;
+ struct space *space = parse_context->new_space;
if (space == NULL || parse_context->nErr != 0)
goto create_view_fail;
@@ -1729,7 +1729,7 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
* Space under construction during CREATE TABLE
* processing. NULL for ALTER TABLE statement handling.
*/
- struct space *space = parse_context->updated_space;
+ struct space *space = parse_context->new_space;
bool is_alter = space == NULL;
uint32_t child_cols_count;
@@ -2164,11 +2164,11 @@ sql_create_index(struct Parse *parse, struct Token *token,
goto exit_create_index;
}
} else {
- if (parse->updated_space == NULL)
+ if (parse->new_space == NULL)
goto exit_create_index;
assert(token == NULL);
assert(start == NULL);
- space = parse->updated_space;
+ space = parse->new_space;
}
struct space_def *def = space->def;
@@ -2354,7 +2354,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
* constraint, but has different onError (behavior on
* constraint violation), then an error is raised.
*/
- if (parse->updated_space != NULL) {
+ if (parse->new_space != NULL) {
for (uint32_t i = 0; i < space->index_count; ++i) {
struct index *existing_idx = space->index[i];
uint32_t iid = existing_idx->def->iid;
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 8c4ad3146..d7070b70b 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -413,7 +413,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Return the number of rows that were deleted. */
if ((user_session->sql_flags & SQLITE_CountRows) != 0 &&
- parse->updated_space != NULL) {
+ parse->triggered_space != NULL) {
sqlite3VdbeAddOp2(v, OP_ResultRow, reg_count, 1);
sqlite3VdbeSetNumCols(v, 1);
sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "rows deleted",
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index b23f36731..d0ac87f14 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4335,7 +4335,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
break;
}
case TK_RAISE:
- if (pParse->updated_space == NULL) {
+ if (pParse->triggered_space == NULL) {
sqlite3ErrorMsg(pParse, "RAISE() may only be used "
"within a trigger-program");
return 0;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d630524b3..50204da4a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -793,7 +793,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
/* Return the number of rows inserted. */
if ((user_session->sql_flags & SQLITE_CountRows) != 0 &&
- pParse->updated_space == NULL) {
+ pParse->triggered_space == NULL) {
sqlite3VdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
sqlite3VdbeSetNumCols(v, 1);
sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "rows inserted",
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index d3618d207..6e13151e6 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -311,18 +311,18 @@ lookupName(Parse * pParse, /* The parsing context */
* it is a new.* or old.* trigger argument reference
*/
if (zTab != NULL && cntTab == 0 &&
- pParse->updated_space != NULL) {
+ pParse->triggered_space != NULL) {
int op = pParse->eTriggerOp;
assert(op == TK_DELETE || op == TK_UPDATE
|| op == TK_INSERT);
struct space_def *space_def = NULL;
if (op != TK_DELETE && sqlite3StrICmp("new", zTab) == 0) {
pExpr->iTable = 1;
- space_def = pParse->updated_space->def;
+ space_def = pParse->triggered_space->def;
} else if (op != TK_INSERT
&& sqlite3StrICmp("old", zTab) == 0) {
pExpr->iTable = 0;
- space_def = pParse->updated_space->def;
+ space_def = pParse->triggered_space->def;
}
if (space_def != NULL) {
@@ -1616,9 +1616,9 @@ sql_resolve_self_reference(struct Parse *parser, struct space_def *def,
int type, struct Expr *expr,
struct ExprList *expr_list)
{
- /* Fake SrcList for parser->updated_space */
+ /* Fake SrcList for parser->new_space */
SrcList sSrc;
- /* Name context for parser->updated_space */
+ /* Name context for parser->new_space */
NameContext sNC;
assert(type == NC_IsCheck || type == NC_IdxExpr);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1352f6efe..5d01b91b2 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2755,7 +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 */
- struct space *updated_space; /* A space being constructed by CREATE TABLE
+ struct space *new_space; /* A space being constructed by CREATE TABLE
or space we are coding triggers for */
TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
With *pWith; /* Current WITH clause, or NULL */
@@ -3474,8 +3474,8 @@ void sqlite3IdListDelete(sqlite3 *, IdList *);
* index and tbl_name is the name of the table that is to be
* indexed. Both will be NULL for a primary key or an index that
* is created to satisfy a UNIQUE constraint. If tbl_name and
- * name are NULL, use parse->updated_space as the table to be indexed.
- * parse->updated_space is a space that is currently being
+ * name are NULL, use parse->new_space as the table to be indexed.
+ * parse->new_space is a space that is currently being
* constructed by a CREATE TABLE statement.
*
* col_list is a list of columns to be indexed. col_list will be
@@ -3484,7 +3484,7 @@ void sqlite3IdListDelete(sqlite3 *, IdList *);
*
* @param parse All information about this parse.
* @param token Index name. May be NULL.
- * @param tbl_name Table to index. Use pParse->updated_space if NULL.
+ * @param tbl_name Table to index. Use pParse->new_space if NULL.
* @param col_list A list of columns to be indexed.
* @param start The CREATE token that begins this statement.
* @param sort_order Sort order of primary key when pList==NULL.
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 9b0f2cf49..fd45413b9 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -421,7 +421,7 @@ sql_token(const char *z, int *type, bool *is_reserved)
* 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's purpose is to clean-up parser->updated_space.
+ * space object. It's purpose is to clean-up parser->new_space.
*
* @param db Database handler.
* @param space Space to be deleted.
@@ -470,7 +470,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
sqlite3OomFault(db);
return SQLITE_NOMEM_BKPT;
}
- assert(pParse->updated_space == NULL);
+ assert(pParse->new_space == NULL);
assert(pParse->parsed_ast.trigger == NULL);
assert(pParse->nVar == 0);
assert(pParse->pVList == 0);
@@ -550,7 +550,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
sqlite3VdbeDelete(pParse->pVdbe);
pParse->pVdbe = 0;
}
- parser_space_delete(db, pParse->updated_space);
+ parser_space_delete(db, pParse->new_space);
if (pParse->pWithToFree)
sqlite3WithDelete(db, pParse->pWithToFree);
sqlite3DbFree(db, pParse->pVList);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index e3c87c422..46523edd9 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -614,7 +614,7 @@ codeTriggerProgram(Parse * pParse, /* The parser context */
Vdbe *v = pParse->pVdbe;
sqlite3 *db = pParse->db;
- assert(pParse->updated_space != NULL && pParse->pToplevel != NULL);
+ assert(pParse->triggered_space != NULL && pParse->pToplevel != NULL);
assert(pStepList);
assert(v != 0);
@@ -799,7 +799,7 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
sql_parser_create(pSubParse, db);
memset(&sNC, 0, sizeof(sNC));
sNC.pParse = pSubParse;
- pSubParse->updated_space = space;
+ pSubParse->triggered_space = space;
pSubParse->pToplevel = pTop;
pSubParse->eTriggerOp = trigger->op;
pSubParse->nQueryLoop = parser->nQueryLoop;
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index a7affa954..5eb1437ab 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -295,7 +295,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
/* Initialize the count of updated rows
*/
if ((user_session->sql_flags & SQLITE_CountRows)
- && pParse->updated_space == NULL) {
+ && pParse->triggered_space == NULL) {
regRowCount = ++pParse->nMem;
sqlite3VdbeAddOp2(v, OP_Integer, 0, regRowCount);
}
@@ -503,7 +503,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
/* Increment the row counter
*/
if ((user_session->sql_flags & SQLITE_CountRows)
- && pParse->updated_space == NULL) {
+ && pParse->triggered_space == NULL) {
sqlite3VdbeAddOp2(v, OP_AddImm, regRowCount, 1);
}
@@ -525,7 +525,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
/* Return the number of rows that were changed. */
if (user_session->sql_flags & SQLITE_CountRows &&
- pParse->updated_space == NULL) {
+ pParse->triggered_space == NULL) {
sqlite3VdbeAddOp2(v, OP_ResultRow, regRowCount, 1);
sqlite3VdbeSetNumCols(v, 1);
sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "rows updated",
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-02-11 17:58 ` [tarantool-patches] " n.pettik
@ 2019-02-12 13:55 ` i.koptelov
2019-02-12 14:56 ` n.pettik
0 siblings, 1 reply; 10+ messages in thread
From: i.koptelov @ 2019-02-12 13:55 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
> On 11 Feb 2019, at 20:58, n.pettik <korablev@tarantool.org> wrote:
>
>
>> On 6 Feb 2019, at 20:17, i.koptelov <ivan.koptelov@tarantool.org> wrote:
>>
>>>> On 1 Feb 2019, at 14:05, Ivan Koptelov<ivan.koptelov@tarantool.org> wrote:
>>>>
>>>> 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’t 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.
>> Sorry for this. All review fixes below is inlined.
>
> Well, thank you. But don’t put fixes in a separate commit, at least when
> you push your branch to the remote repository. You may *accidentally*
> fail during final commit squashing and no-one will notice that.
>
> Travis status was entirely negative: It can’t be compiled.
> You forgot to add forward declaration of space_def.
> I fixed that and a couple of minor issues more. Here is a diff
> (I’ve squashed your commits and pushed on top of your branch).
>
> Please, check it out. If it is OK, squash them and then patch LGTM.
Thank you for the fixes! Ok, I would not put my fixes in separate commit in future.
I checked out the diff, it’s ok for me except one small issue. I’ve marked
it below.
>
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 874fd682f..1352f6efe 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2759,6 +2760,9 @@ struct Parse {
> TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
> With *pWith; /* Current WITH clause, or NULL */
> With *pWithToFree; /* Free this WITH object at the end of the parse */
> + /** Space triggers are being coded for. */
> + struct space *triggered_space;
> + /** A space being constructed by CREATE TABLE */
Are you sure that this comment (last line) should be here? Is it about all lines below?
> /**
> * Number of FK constraints declared within
> * CREATE TABLE statement.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-02-12 13:55 ` i.koptelov
@ 2019-02-12 14:56 ` n.pettik
2019-02-12 15:07 ` i.koptelov
0 siblings, 1 reply; 10+ messages in thread
From: n.pettik @ 2019-02-12 14:56 UTC (permalink / raw)
To: tarantool-patches; +Cc: Ivan Koptelov
> On 12 Feb 2019, at 16:55, i.koptelov <ivan.koptelov@tarantool.org> wrote:
>> On 11 Feb 2019, at 20:58, n.pettik <korablev@tarantool.org> wrote:
>>> On 6 Feb 2019, at 20:17, i.koptelov <ivan.koptelov@tarantool.org> wrote:
>>>
>>>>> On 1 Feb 2019, at 14:05, Ivan Koptelov<ivan.koptelov@tarantool.org> wrote:
>>>>>
>>>>> 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’t 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.
>>> Sorry for this. All review fixes below is inlined.
>>
>> Well, thank you. But don’t put fixes in a separate commit, at least when
>> you push your branch to the remote repository. You may *accidentally*
>> fail during final commit squashing and no-one will notice that.
>>
>> Travis status was entirely negative: It can’t be compiled.
>> You forgot to add forward declaration of space_def.
>> I fixed that and a couple of minor issues more. Here is a diff
>> (I’ve squashed your commits and pushed on top of your branch).
>>
>> Please, check it out. If it is OK, squash them and then patch LGTM.
> Thank you for the fixes! Ok, I would not put my fixes in separate commit in future.
> I checked out the diff, it’s ok for me except one small issue. I’ve marked
> it below.
>>
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 874fd682f..1352f6efe 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -2759,6 +2760,9 @@ struct Parse {
>> TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
>> With *pWith; /* Current WITH clause, or NULL */
>> With *pWithToFree; /* Free this WITH object at the end of the parse */
>> + /** Space triggers are being coded for. */
>> + struct space *triggered_space;
>> + /** A space being constructed by CREATE TABLE */
> Are you sure that this comment (last line) should be here? Is it about all lines below?
I forgot to move struct space *new_space; below.
Thanks for cross-check. Could you please fix this?
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-02-12 14:56 ` n.pettik
@ 2019-02-12 15:07 ` i.koptelov
0 siblings, 0 replies; 10+ messages in thread
From: i.koptelov @ 2019-02-12 15:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
> On 12 Feb 2019, at 17:56, n.pettik <korablev@tarantool.org> wrote:
>
>
>
>> On 12 Feb 2019, at 16:55, i.koptelov <ivan.koptelov@tarantool.org> wrote:
>>> On 11 Feb 2019, at 20:58, n.pettik <korablev@tarantool.org> wrote:
>>>> On 6 Feb 2019, at 20:17, i.koptelov <ivan.koptelov@tarantool.org> wrote:
>>>>
>>>>>> On 1 Feb 2019, at 14:05, Ivan Koptelov<ivan.koptelov@tarantool.org> wrote:
>>>>>>
>>>>>> 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’t 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.
>>>> Sorry for this. All review fixes below is inlined.
>>>
>>> Well, thank you. But don’t put fixes in a separate commit, at least when
>>> you push your branch to the remote repository. You may *accidentally*
>>> fail during final commit squashing and no-one will notice that.
>>>
>>> Travis status was entirely negative: It can’t be compiled.
>>> You forgot to add forward declaration of space_def.
>>> I fixed that and a couple of minor issues more. Here is a diff
>>> (I’ve squashed your commits and pushed on top of your branch).
>>>
>>> Please, check it out. If it is OK, squash them and then patch LGTM.
>> Thank you for the fixes! Ok, I would not put my fixes in separate commit in future.
>> I checked out the diff, it’s ok for me except one small issue. I’ve marked
>> it below.
>>>
>>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>>> index 874fd682f..1352f6efe 100644
>>> --- a/src/box/sql/sqliteInt.h
>>> +++ b/src/box/sql/sqliteInt.h
>>> @@ -2759,6 +2760,9 @@ struct Parse {
>>> TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
>>> With *pWith; /* Current WITH clause, or NULL */
>>> With *pWithToFree; /* Free this WITH object at the end of the parse */
>>> + /** Space triggers are being coded for. */
>>> + struct space *triggered_space;
>>> + /** A space being constructed by CREATE TABLE */
>> Are you sure that this comment (last line) should be here? Is it about all lines below?
>
> I forgot to move struct space *new_space; below.
> Thanks for cross-check. Could you please fix this?
>
Fixed and pushed on branch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: remove struct Table
2019-01-28 9:54 [tarantool-patches] [PATCH] sql: remove struct Table Ivan Koptelov
2019-01-29 14:59 ` [tarantool-patches] " n.pettik
@ 2019-02-15 14:47 ` Kirill Yukhin
1 sibling, 0 replies; 10+ messages in thread
From: Kirill Yukhin @ 2019-02-15 14:47 UTC (permalink / raw)
To: tarantool-patches
Hello,
On 28 Jan 12:54, Ivan Koptelov wrote:
> 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.
>
> Closes #3235
> ---
> Branch: [1]https://github.com/tarantool/tarantool/tree/sudobobo/gh-3235-repl-Table-w-space
> Issue: [2]https://github.com/tarantool/tarantool/issues/3235
I've checked your patch into 2.1 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-15 14:47 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-28 9:54 [tarantool-patches] [PATCH] sql: remove struct Table Ivan Koptelov
2019-01-29 14:59 ` [tarantool-patches] " n.pettik
2019-02-01 11:05 ` Ivan Koptelov
2019-02-04 19:22 ` n.pettik
2019-02-06 17:17 ` [tarantool-patches] " i.koptelov
2019-02-11 17:58 ` [tarantool-patches] " n.pettik
2019-02-12 13:55 ` i.koptelov
2019-02-12 14:56 ` n.pettik
2019-02-12 15:07 ` i.koptelov
2019-02-15 14:47 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox