* [tarantool-patches] [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces @ 2019-05-17 15:51 imeevma 2019-05-19 12:13 ` [tarantool-patches] " n.pettik 2019-06-06 14:13 ` Kirill Yukhin 0 siblings, 2 replies; 5+ messages in thread From: imeevma @ 2019-05-17 15:51 UTC (permalink / raw) To: korablev; +Cc: tarantool-patches At some point, it became possible to use SELECT on spaces created in Lua. Since it is allowed to create temporary spaces in Lua, this led to an error. To avoid this error, now not all temporary spaces are checked, but only ephemeral spaces. Close #4139 --- https://github.com/tarantool/tarantool/issues/4139 https://github.com/tarantool/tarantool/tree/imeevma/gh-4139-assertion-on-temporary-space src/box/sql.c | 1 + src/box/sql/build.c | 6 +++--- test/sql/misc.result | 22 ++++++++++++++++++++++ test/sql/misc.test.lua | 9 +++++++++ 4 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index fbfa599..3bf263d 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1281,6 +1281,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name) memcpy(def->name, name, name_len); def->name[name_len] = '\0'; def->opts.is_temporary = true; + def->opts.is_ephemeral = true; return def; } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 6051a25..33005c5 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2762,14 +2762,14 @@ sqlSrcListDelete(sql * db, SrcList * pList) if (pItem->fg.isTabFunc) sql_expr_list_delete(db, pItem->u1.pFuncArg); /* - * Space is either not temporary which means that - * it came from space cache; or space is temporary + * Space is either not ephemeral which means that + * it came from space cache; or space is ephemeral * 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->def->opts.is_ephemeral || (pItem->space->index == NULL && pItem->space->def->opts.checks == NULL)); sql_select_delete(db, pItem->pSelect); diff --git a/test/sql/misc.result b/test/sql/misc.result index b117e15..bc8b10e 100644 --- a/test/sql/misc.result +++ b/test/sql/misc.result @@ -106,3 +106,25 @@ box.execute('SELECT X\'4D6564766564\'') rows: - ['Medved'] ... +-- +-- gh-4139: assertion when reading a temporary space. +-- +format = {{name = 'id', type = 'integer'}} +--- +... +s = box.schema.space.create('s',{format=format, temporary=true}) +--- +... +i = s:create_index('i') +--- +... +box.execute('select * from "s"') +--- +- metadata: + - name: id + type: integer + rows: [] +... +s:drop() +--- +... diff --git a/test/sql/misc.test.lua b/test/sql/misc.test.lua index 0b1c34d..fdc19f3 100644 --- a/test/sql/misc.test.lua +++ b/test/sql/misc.test.lua @@ -26,3 +26,12 @@ box.execute('SELECT 1.5;') box.execute('SELECT 1.0;') box.execute('SELECT \'abc\';') box.execute('SELECT X\'4D6564766564\'') + +-- +-- gh-4139: assertion when reading a temporary space. +-- +format = {{name = 'id', type = 'integer'}} +s = box.schema.space.create('s',{format=format, temporary=true}) +i = s:create_index('i') +box.execute('select * from "s"') +s:drop() -- 2.7.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces 2019-05-17 15:51 [tarantool-patches] [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces imeevma @ 2019-05-19 12:13 ` n.pettik 2019-05-21 15:06 ` Mergen Imeev 2019-06-06 14:13 ` Kirill Yukhin 1 sibling, 1 reply; 5+ messages in thread From: n.pettik @ 2019-05-19 12:13 UTC (permalink / raw) To: tarantool-patches; +Cc: Imeev Mergen > On 17 May 2019, at 18:51, imeevma@tarantool.org wrote: > > At some point, it became possible to use SELECT on spaces created > in Lua. Since it is allowed to create temporary spaces in Lua, > this led to an error. > To avoid this error, now not all temporary > spaces are checked, but only ephemeral spaces. Checked on what condition? Please, provide complete explanation of the problem in commit message. Btw, they are not real ephemeral spaces, they are surrogate wrappers which have no underlying spaces in cache. It was our convention to avoid messing them with real (ephemeral, temporary or casual) spaces. > > Close #4139 > --- > https://github.com/tarantool/tarantool/issues/4139 > https://github.com/tarantool/tarantool/tree/imeevma/gh-4139-assertion-on-temporary-space > > src/box/sql.c | 1 + > src/box/sql/build.c | 6 +++--- > test/sql/misc.result | 22 ++++++++++++++++++++++ > test/sql/misc.test.lua | 9 +++++++++ > 4 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index fbfa599..3bf263d 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1281,6 +1281,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name) > memcpy(def->name, name, name_len); > def->name[name_len] = '\0'; > def->opts.is_temporary = true; > + def->opts.is_ephemeral = true; Let’s set only is_ephemeral flag - setting both doesn’t really make any sense. Also, fix assertions during building routine to check is_ephemeral flag, not is_temporary. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces 2019-05-19 12:13 ` [tarantool-patches] " n.pettik @ 2019-05-21 15:06 ` Mergen Imeev 2019-05-25 13:48 ` n.pettik 0 siblings, 1 reply; 5+ messages in thread From: Mergen Imeev @ 2019-05-21 15:06 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches Hi! Thank you for review. My answers and new patch below. On Sun, May 19, 2019 at 03:13:36PM +0300, n.pettik wrote: > > > > On 17 May 2019, at 18:51, imeevma@tarantool.org wrote: > > > > At some point, it became possible to use SELECT on spaces created > > in Lua. Since it is allowed to create temporary spaces in Lua, > > this led to an error. > > To avoid this error, now not all temporary > > spaces are checked, but only ephemeral spaces. > > Checked on what condition? Please, provide complete > explanation of the problem in commit message. > > Btw, they are not real ephemeral spaces, they are > surrogate wrappers which have no underlying spaces > in cache. It was our convention to avoid messing them > with real (ephemeral, temporary or casual) spaces. > Changed commit-message > > > > Close #4139 > > --- > > https://github.com/tarantool/tarantool/issues/4139 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-4139-assertion-on-temporary-space > > > > src/box/sql.c | 1 + > > src/box/sql/build.c | 6 +++--- > > test/sql/misc.result | 22 ++++++++++++++++++++++ > > test/sql/misc.test.lua | 9 +++++++++ > > 4 files changed, 35 insertions(+), 3 deletions(-) > > > > diff --git a/src/box/sql.c b/src/box/sql.c > > index fbfa599..3bf263d 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -1281,6 +1281,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name) > > memcpy(def->name, name, name_len); > > def->name[name_len] = '\0'; > > def->opts.is_temporary = true; > > + def->opts.is_ephemeral = true; > > Let’s set only is_ephemeral flag - setting both doesn’t > really make any sense. Also, fix assertions during > building routine to check is_ephemeral flag, not > is_temporary. > > Done. New patch: From 16309a0e955f32a866585eb71e429080f716d7bf Mon Sep 17 00:00:00 2001 From: Mergen Imeev <imeevma@gmail.com> Date: Fri, 17 May 2019 17:56:56 +0300 Subject: [PATCH] sql: do not set is_temporary for ephemeral spaces Up to this point, the is_temporary flag has been set in all ephemeral spaces. At some point, it became possible to use spaces created in Lua in SQL statements. Since it is allowed to create temporary spaces in Lua, not all temporary spaces in SQL are ephemeral. To separate the temporary and ephemeral spaces, is_ephemeral flag is now set in the ephemeral spaces and is_temporary flag is not set. Close #4139 diff --git a/src/box/space.c b/src/box/space.c index 243e7da..6380a0a 100644 --- a/src/box/space.c +++ b/src/box/space.c @@ -194,7 +194,6 @@ space_new(struct space_def *def, struct rlist *key_list) struct space * space_new_ephemeral(struct space_def *def, struct rlist *key_list) { - assert(def->opts.is_temporary); assert(def->opts.is_ephemeral); struct space *space = space_new(def, key_list); if (space == NULL) diff --git a/src/box/space_def.c b/src/box/space_def.c index d825def..0b9cc01 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -263,7 +263,6 @@ struct space_def* space_def_new_ephemeral(uint32_t field_count) { struct space_opts opts = space_opts_default; - opts.is_temporary = true; opts.is_ephemeral = true; struct space_def *space_def = space_def_new(0, 0, field_count, "ephemeral", diff --git a/src/box/sql.c b/src/box/sql.c index fbfa599..dd9c9bb 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1280,7 +1280,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name) memset(def, 0, size); memcpy(def->name, name, name_len); def->name[name_len] = '\0'; - def->opts.is_temporary = true; + def->opts.is_ephemeral = true; return def; } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 91b977d..29a4ef2 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -422,7 +422,7 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def) * As sql_field_retrieve will allocate memory on region * ensure that def is also temporal and would be dropped. */ - assert(def->opts.is_temporary); + assert(def->opts.is_ephemeral); if (sql_field_retrieve(pParse, def, def->field_count) == NULL) return; struct region *region = &pParse->region; @@ -493,7 +493,7 @@ sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) sql *db = pParse->db; struct space *p = pParse->create_table_def.new_space; if (p != NULL) { - assert(p->def->opts.is_temporary); + assert(p->def->opts.is_ephemeral); struct space_def *def = p->def; if (!sqlExprIsConstantOrFunction (pSpan->pExpr, db->init.busy)) { @@ -723,7 +723,7 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id) * In cases mentioned above collation is fetched by id. */ if (space == NULL) { - assert(def->opts.is_temporary); + assert(def->opts.is_ephemeral); assert(column < (uint32_t)def->field_count); *coll_id = def->fields[column].coll_id; struct coll_id *collation = coll_by_id(*coll_id); @@ -1260,7 +1260,7 @@ sql_create_view(struct Parse *parse_context) sqlSelectAddColumnTypeAndCollation(parse_context, space->def, view_def->select); } else { - assert(select_res_space->def->opts.is_temporary); + assert(select_res_space->def->opts.is_ephemeral); space->def->fields = select_res_space->def->fields; space->def->field_count = select_res_space->def->field_count; select_res_space->def->fields = NULL; @@ -2767,14 +2767,14 @@ sqlSrcListDelete(sql * db, SrcList * pList) if (pItem->fg.isTabFunc) sql_expr_list_delete(db, pItem->u1.pFuncArg); /* - * Space is either not temporary which means that - * it came from space cache; or space is temporary + * Space is either not ephemeral which means that + * it came from space cache; or space is ephemeral * 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->def->opts.is_ephemeral || (pItem->space->index == NULL && pItem->space->def->opts.checks == NULL)); sql_select_delete(db, pItem->pSelect); diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 8cc3532..bff50f4 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -431,7 +431,7 @@ parser_space_delete(struct sql *db, struct space *space) { if (space == NULL || db == NULL || db->pnBytesFreed == 0) return; - assert(space->def->opts.is_temporary); + assert(space->def->opts.is_ephemeral); 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); diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 9cdeb05..b5d55b2 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -711,7 +711,6 @@ tuple_format_reuse(struct tuple_format **p_format) * Make sure they're unset. */ assert(format->dict->name_count == 0); - assert(format->is_temporary); mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); if (key != mh_end(tuple_formats_hash)) { @@ -737,7 +736,6 @@ tuple_format_add_to_hash(struct tuple_format *format) if(!format->is_ephemeral) return 0; assert(format->dict->name_count == 0); - assert(format->is_temporary); mh_int_t key = mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, NULL, NULL); diff --git a/test/sql/misc.result b/test/sql/misc.result index b117e15..bc8b10e 100644 --- a/test/sql/misc.result +++ b/test/sql/misc.result @@ -106,3 +106,25 @@ box.execute('SELECT X\'4D6564766564\'') rows: - ['Medved'] ... +-- +-- gh-4139: assertion when reading a temporary space. +-- +format = {{name = 'id', type = 'integer'}} +--- +... +s = box.schema.space.create('s',{format=format, temporary=true}) +--- +... +i = s:create_index('i') +--- +... +box.execute('select * from "s"') +--- +- metadata: + - name: id + type: integer + rows: [] +... +s:drop() +--- +... diff --git a/test/sql/misc.test.lua b/test/sql/misc.test.lua index 0b1c34d..fdc19f3 100644 --- a/test/sql/misc.test.lua +++ b/test/sql/misc.test.lua @@ -26,3 +26,12 @@ box.execute('SELECT 1.5;') box.execute('SELECT 1.0;') box.execute('SELECT \'abc\';') box.execute('SELECT X\'4D6564766564\'') + +-- +-- gh-4139: assertion when reading a temporary space. +-- +format = {{name = 'id', type = 'integer'}} +s = box.schema.space.create('s',{format=format, temporary=true}) +i = s:create_index('i') +box.execute('select * from "s"') +s:drop() ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces 2019-05-21 15:06 ` Mergen Imeev @ 2019-05-25 13:48 ` n.pettik 0 siblings, 0 replies; 5+ messages in thread From: n.pettik @ 2019-05-25 13:48 UTC (permalink / raw) To: tarantool-patches; +Cc: Imeev Mergen > > From 16309a0e955f32a866585eb71e429080f716d7bf Mon Sep 17 00:00:00 2001 > From: Mergen Imeev <imeevma@gmail.com> > Date: Fri, 17 May 2019 17:56:56 +0300 > Subject: [PATCH] sql: do not set is_temporary for ephemeral spaces > > Up to this point, the is_temporary flag has been set in all > ephemeral spaces. > > At some point, it became possible to use spaces created in Lua in > SQL statements. Since it is allowed to create temporary spaces in > Lua, not all temporary spaces in SQL are ephemeral. To separate > the temporary and ephemeral spaces, is_ephemeral flag is now set > in the ephemeral spaces and is_temporary flag is not set. > > Close #4139 You slightly misunderstood my initial intention. There are two kinds of what we call ephemeral spaces: first ones are real ephemeral spaces which are created during VDBE runtime to hold materialised intermediate results of query execution. They are destroyed when query is finished. Second type consists of surrogate defs allocated using region and they are used to keep meta-information during compilation of DML queries. They are not real space object and I have no idea why we call them ‘ephemeral’, I guess it is a big mess. A while ago, we had to use some marked for such surrogate spaces to process special clean-up operations. And it was decided that we would use ‘is_temporary’ flag. I’ve force pushed my updates (commit message + code fixes). Message: sql: replace is_temprorary with is_ephemeral for surrogate defs Up to this point, the is_temporary flag has been set for surrogate space definitions used to transfer meta-information during compilation stage of DML queries (CREATE TABLE/INDEX etc). At some point, it became possible to use spaces created in Lua in SQL statements. Since it is allowed to create temporary spaces in Lua, not all temporary spaces in SQL are surrogate wrappers. To separate real temporary spaces (i.e. which came from space cache) from surrogate ones, is_ephemeral flag is now set in such spaces instead of is_temporary. Note that there can't be mess between these flags since ephemeral spaces can exist only during execution of VDBE bytecode. Also note that flag is required only for debugging facilities. Close #4139 Diff: diff --git a/src/box/space.c b/src/box/space.c index 6380a0af3..243e7da2f 100644 --- a/src/box/space.c +++ b/src/box/space.c @@ -194,6 +194,7 @@ space_new(struct space_def *def, struct rlist *key_list) struct space * space_new_ephemeral(struct space_def *def, struct rlist *key_list) { + assert(def->opts.is_temporary); assert(def->opts.is_ephemeral); struct space *space = space_new(def, key_list); if (space == NULL) diff --git a/src/box/space_def.c b/src/box/space_def.c index 0b9cc015c..d825def75 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -263,6 +263,7 @@ struct space_def* space_def_new_ephemeral(uint32_t field_count) { struct space_opts opts = space_opts_default; + opts.is_temporary = true; opts.is_ephemeral = true; struct space_def *space_def = space_def_new(0, 0, field_count, "ephemeral", diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index b5d55b2e4..9cdeb051b 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -711,6 +711,7 @@ tuple_format_reuse(struct tuple_format **p_format) * Make sure they're unset. */ assert(format->dict->name_count == 0); + assert(format->is_temporary); mh_int_t key = mh_tuple_format_find(tuple_formats_hash, format, NULL); if (key != mh_end(tuple_formats_hash)) { @@ -736,6 +737,7 @@ tuple_format_add_to_hash(struct tuple_format *format) if(!format->is_ephemeral) return 0; assert(format->dict->name_count == 0); + assert(format->is_temporary); mh_int_t key = mh_tuple_format_put(tuple_formats_hash, (const struct tuple_format **)&format, NULL, NULL); ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces 2019-05-17 15:51 [tarantool-patches] [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces imeevma 2019-05-19 12:13 ` [tarantool-patches] " n.pettik @ 2019-06-06 14:13 ` Kirill Yukhin 1 sibling, 0 replies; 5+ messages in thread From: Kirill Yukhin @ 2019-06-06 14:13 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev Hello, On 17 May 18:51, imeevma@tarantool.org wrote: > At some point, it became possible to use SELECT on spaces created > in Lua. Since it is allowed to create temporary spaces in Lua, > this led to an error. To avoid this error, now not all temporary > spaces are checked, but only ephemeral spaces. > > Close #4139 > --- > https://github.com/tarantool/tarantool/issues/4139 > https://github.com/tarantool/tarantool/tree/imeevma/gh-4139-assertion-on-temporary-space I've checked the patch into 2.1 and master. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-06 14:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-17 15:51 [tarantool-patches] [PATCH v1 1/1] sql: make assertion to check only ephemeral spaces imeevma 2019-05-19 12:13 ` [tarantool-patches] " n.pettik 2019-05-21 15:06 ` Mergen Imeev 2019-05-25 13:48 ` n.pettik 2019-06-06 14:13 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox