<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hello! Thank you for review! New patch and diff between last two<br>
patches below.<br>
</p>
<br>
On 10/09/2018 05:15 PM, n.pettik wrote:<br>
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<pre wrap="">Attach please next time diff between two versions,
so as I can avoid to review whole patch again.
Also, check Travis before you send patch:
<a class="moz-txt-link-freetext" href="https://travis-ci.org/tarantool/tarantool/jobs/438801852">https://travis-ci.org/tarantool/tarantool/jobs/438801852</a>
<a class="moz-txt-link-freetext" href="https://travis-ci.org/tarantool/tarantool/jobs/438801839">https://travis-ci.org/tarantool/tarantool/jobs/438801839</a>
Build fails.</pre>
</blockquote>
It is better now:<br>
<a class="moz-txt-link-freetext" href="https://travis-ci.org/tarantool/tarantool/builds/439677737">https://travis-ci.org/tarantool/tarantool/builds/439677737</a><a
class="moz-txt-link-freetext"
href="https://travis-ci.org/tarantool/tarantool/builds/439289217"></a><br>
<br>
Test box-tap/feedback_daemon.test.lua failed but<br>
there is ticket:<br>
<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/3558">https://github.com/tarantool/tarantool/issues/3558</a><br>
<br>
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">
<pre wrap=""> void
sql_finish_coding(struct Parse *parse_context)
@@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
struct sqlite3 *db = parse_context->db;
struct Vdbe *v = sqlite3GetVdbe(parse_context);
sqlite3VdbeAddOp0(v, OP_Halt);
+ /*
+ * Destructors for all created in current statement
+ * spaces, indexes, sequences etc. There is no need to
+ * create destructor for last SInsert.
+ */
+ if (rlist_empty(&parse_context->record_list) == 0) {
</pre>
</blockquote>
<pre wrap="">Nit: if(! rlist_empty())
</pre>
</blockquote>
<pre wrap="">Fixed.
</pre>
</blockquote>
<pre wrap="">No, it is not fixed.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<blockquote type="cite">
<pre wrap="">diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a806fb4..6a97ff9 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
void
sql_finish_coding(struct Parse *parse_context)
{
@@ -62,6 +109,40 @@ sql_finish_coding(struct Parse *parse_context)
struct sqlite3 *db = parse_context->db;
struct Vdbe *v = sqlite3GetVdbe(parse_context);
sqlite3VdbeAddOp0(v, OP_Halt);
+ /*
+ * In case statement "CREATE TABLE ..." fails it can
+ * left some records in system spaces that shouldn't be
+ * there. To clean-up properly this code is added. Last
+ * record isn't deleted because if statement fails than
+ * it won't be created. This code works the same way for
+ * other "CREATE ..." statements but it won't delete
+ * anything as these statements create no more than one
+ * record.
+ */
+ if (rlist_empty(&parse_context->record_list) == 0) {
+ struct saved_record *record =
+ rlist_shift_entry(&parse_context->record_list,
+ struct saved_record, link);
+ /* Set P2 of SInsert. */
+ sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
+ char *comment = "Delete entry from %s if CREATE TABLE fails”;
</pre>
</blockquote>
<pre wrap="">Nit: const char *.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<blockquote type="cite">
<pre wrap="">+ rlist_foreach_entry(record, &parse_context->record_list, link) {
+ int record_reg = ++parse_context->nMem;
+ sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,
+ record->reg_key_count, record_reg);
+ sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id,
+ record_reg);
+ struct space *space = space_by_id(record->space_id);
+ VdbeComment((v, comment, space_name(space)));
+ /* Set P2 of SInsert. */
+ sqlite3VdbeChangeP2(v, record->insertion_opcode,
+ v->nOp);
+ }
+ sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
+ VdbeComment((v,
+ "Exit with an error if CREATE statement fails"));
+ }
+
if (db->mallocFailed || parse_context->nErr != 0) {
if (parse_context->rc == SQLITE_OK)
parse_context->rc = SQLITE_ERROR;
@@ -1101,13 +1182,14 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
sqlite3VdbeAddOp4(v, OP_Blob, index_parts_sz, entry_reg + 5,
SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg);
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, tuple_reg);
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);
/*
* Non-NULL value means that index has been created via
* separate CREATE INDEX statement.
*/
if (idx_def->opts.sql != NULL)
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
</pre>
</blockquote>
<pre wrap="">As I sad you don’t need to add to this list index entries when it comes
to CREATE INDEX. You can tell index from unique constraint by
existence of opts.sql string:
+++ b/src/box/sql/build.c
@@ -1185,11 +1185,15 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
sqlite3VdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);
/*
* Non-NULL value means that index has been created via
- * separate CREATE INDEX statement.
+ * separate CREATE INDEX statement. On the other hand,
+ * we need to register all indexes incoming in
+ * CREATE TABLE in order to remove them on table creation
+ * fail.
*/
if (idx_def->opts.sql != NULL)
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
- save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
+ else
+ save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
return;
However, in this case we need to jump to abort_due_to_error label
during processing of OP_SInsert. I don’t insist on this change tho,
it is up to you. </pre>
</blockquote>
Discussed and decided to left as it is.<br>
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<blockquote type="cite">
<pre wrap=""> return;
error:
parse->rc = SQL_TARANTOOL_ERROR;
@@ -1165,8 +1247,9 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
sqlite3VdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,
SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1);
return;
error:
pParse->nErr++;
@@ -1340,9 +1423,11 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
parent_links, P4_DYNAMIC);
sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,
constr_tuple_reg + 9);
- sqlite3VdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
+ sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
constr_tuple_reg + 9);
sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
+ save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
+ vdbe->nOp - 1);
sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
return;
error:
@@ -1487,14 +1572,18 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
int reg_seq_record =
emitNewSysSequenceRecord(pParse, reg_seq_id,
p->def->name);
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID,
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,
reg_seq_record);
+ save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
+ v->nOp - 1);
/* Do an insertion into _space_sequence. */
int reg_space_seq_record =
emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
reg_seq_id);
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, 0,
reg_space_seq_record);
+ save_record(pParse, BOX_SPACE_SEQUENCE_ID,
+ reg_space_seq_record + 1, 1, v->nOp - 1);
</pre>
</blockquote>
<pre wrap="">If we are creating FK constraint with ALTER TABLE ADD CONSTRAINT,
we don’t need to add it to list of things to be deleted (the same as for index).
@@ -1426,8 +1430,10 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
constr_tuple_reg + 9);
sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
- save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
- vdbe->nOp - 1);
+ if (parse_context->pNewTable != NULL) {
+ save_record(parse_context, BOX_FK_CONSTRAINT_ID,
+ constr_tuple_reg, 2, vdbe->nOp - 1);
+ }
</pre>
</blockquote>
Discussed and decided to left as it is.
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<blockquote type="cite">
<pre wrap="">diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..1bc8894 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,62 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+--
+-- Give user right to write in _index. Still have not enough
+-- rights to write in _sequence.
+--
+box.schema.user.grant('tmp', 'write', 'space', '_index')
+box.session.su('tmp')
+
+--
+-- Error: user do not have rights to write in _sequence.
+--
+box.sql.execute('create table t2 (id int primary key autoincrement, a unique, b unique, c unique, d unique)’)
</pre>
</blockquote>
<pre wrap="">Nit: for SQL statements please use uppercase.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org">
<blockquote type="cite">
<pre wrap="">+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+box.schema.user.drop('tmp’)
</pre>
</blockquote>
<pre wrap="">I see no tests involving FK constraints. Add them as well.
</pre>
</blockquote>
Added.<br>
<br>
<b> Diff between two last patches:</b><br>
<br>
diff --git a/6a97ff9..28b488c b/28b488c<br>
index 6a97ff9..28b488c 100644<br>
--- a/6a97ff9..28b488c<br>
+++ b/28b488c<br>
@@ -119,26 +119,28 @@ sql_finish_coding(struct Parse *parse_context)<br>
* anything as these statements create no more than one<br>
* record.<br>
*/<br>
- if (rlist_empty(&parse_context->record_list) == 0) {<br>
+ if (!rlist_empty(&parse_context->record_list)) {<br>
struct saved_record *record =<br>
rlist_shift_entry(&parse_context->record_list,<br>
struct saved_record, link);<br>
/* Set P2 of SInsert. */<br>
sqlite3VdbeChangeP2(v, record->insertion_opcode,
v->nOp);<br>
- char *comment = "Delete entry from %s if CREATE TABLE
fails";<br>
+ MAYBE_UNUSED const char *comment =<br>
+ "Delete entry from %s if CREATE TABLE fails";<br>
rlist_foreach_entry(record,
&parse_context->record_list, link) {<br>
int record_reg = ++parse_context->nMem;<br>
sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,<br>
record->reg_key_count, record_reg);<br>
sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id,<br>
record_reg);<br>
- struct space *space = space_by_id(record->space_id);<br>
+ MAYBE_UNUSED struct space *space =<br>
+ space_by_id(record->space_id);<br>
VdbeComment((v, comment, space_name(space)));<br>
/* Set P2 of SInsert. */<br>
sqlite3VdbeChangeP2(v, record->insertion_opcode,<br>
v->nOp);<br>
}<br>
- sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);<br>
+ sqlite3VdbeAddOp1(v, OP_Halt, SQL_TARANTOOL_ERROR);<br>
VdbeComment((v,<br>
"Exit with an error if CREATE statement fails"));<br>
}<br>
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result<br>
index 1659604..43da275 100644<br>
--- a/test/sql/drop-table.result<br>
+++ b/test/sql/drop-table.result<br>
@@ -42,7 +42,7 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111,
222, 'c3', 444)")<br>
box.schema.user.create('tmp')<br>
---<br>
...<br>
-box.schema.user.grant('tmp', 'create', 'universe')<br>
+box.schema.user.grant('tmp', 'create, read', 'universe')<br>
---<br>
...<br>
box.schema.user.grant('tmp', 'write', 'space', '_space')<br>
@@ -68,12 +68,12 @@ box.session.su('tmp')<br>
-- Error: user do not have rights to write in box.space._index.<br>
-- Space that was already created should be automatically dropped.<br>
--<br>
-box.sql.execute('create table t1 (id int primary key, a int)')<br>
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')<br>
---<br>
- error: Write access to space '_index' is denied for user 'tmp'<br>
...<br>
-- Error: no such table.<br>
-box.sql.execute('drop table t1')<br>
+box.sql.execute('DROP TABLE t1')<br>
---<br>
- error: 'no such table: T1'<br>
...<br>
@@ -109,7 +109,7 @@ box.session.su('tmp')<br>
--<br>
-- Error: user do not have rights to write in _sequence.<br>
--<br>
-box.sql.execute('create table t2 (id int primary key autoincrement,
a unique, b unique, c unique, d unique)')<br>
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY AUTOINCREMENT,
a UNIQUE, b UNIQUE, c UNIQUE, d UNIQUE)')<br>
---<br>
- error: Write access to space '_sequence' is denied for user 'tmp'<br>
...<br>
@@ -132,6 +132,48 @@ sequence_count == #box.space._sequence:select()<br>
---<br>
- true<br>
...<br>
+--<br>
+-- Give user right to write in _sequence. Still have not enough<br>
+-- rights to write in _fk_constraint.<br>
+--<br>
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')<br>
+---<br>
+...<br>
+box.session.su('tmp')<br>
+---<br>
+...<br>
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')<br>
+---<br>
+...<br>
+--<br>
+-- Error: user do not have rights to write in _fk_constraint.<br>
+--<br>
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES
t3);')<br>
+---<br>
+- error: Write access to space '_fk_constraint' is denied for user
'tmp'<br>
+...<br>
+box.sql.execute('DROP TABLE t3;')<br>
+---<br>
+...<br>
+box.session.su('admin')<br>
+---<br>
+...<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+---<br>
+- true<br>
+...<br>
+index_count == #box.space._index:select()<br>
+---<br>
+- true<br>
+...<br>
+sequence_count == #box.space._sequence:select()<br>
+---<br>
+- true<br>
+...<br>
box.schema.user.drop('tmp')<br>
---<br>
...<br>
diff --git a/test/sql/drop-table.test.lua
b/test/sql/drop-table.test.lua<br>
index 1bc8894..f86e3d8 100644<br>
--- a/test/sql/drop-table.test.lua<br>
+++ b/test/sql/drop-table.test.lua<br>
@@ -33,7 +33,7 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111,
222, 'c3', 444)")<br>
-- create index.<br>
--<br>
box.schema.user.create('tmp')<br>
-box.schema.user.grant('tmp', 'create', 'universe')<br>
+box.schema.user.grant('tmp', 'create, read', 'universe')<br>
box.schema.user.grant('tmp', 'write', 'space', '_space')<br>
box.schema.user.grant('tmp', 'write', 'space', '_schema')<br>
<br>
@@ -47,9 +47,9 @@ box.session.su('tmp')<br>
-- Error: user do not have rights to write in box.space._index.<br>
-- Space that was already created should be automatically dropped.<br>
--<br>
-box.sql.execute('create table t1 (id int primary key, a int)')<br>
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')<br>
-- Error: no such table.<br>
-box.sql.execute('drop table t1')<br>
+box.sql.execute('DROP TABLE t1')<br>
<br>
box.session.su('admin')<br>
<br>
@@ -71,7 +71,31 @@ box.session.su('tmp')<br>
--<br>
-- Error: user do not have rights to write in _sequence.<br>
--<br>
-box.sql.execute('create table t2 (id int primary key autoincrement,
a unique, b unique, c unique, d unique)')<br>
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY AUTOINCREMENT,
a UNIQUE, b UNIQUE, c UNIQUE, d UNIQUE)')<br>
+<br>
+box.session.su('admin')<br>
+<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+index_count == #box.space._index:select()<br>
+sequence_count == #box.space._sequence:select()<br>
+<br>
+--<br>
+-- Give user right to write in _sequence. Still have not enough<br>
+-- rights to write in _fk_constraint.<br>
+--<br>
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')<br>
+box.session.su('tmp')<br>
+<br>
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')<br>
+--<br>
+-- Error: user do not have rights to write in _fk_constraint.<br>
+--<br>
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES
t3);')<br>
+box.sql.execute('DROP TABLE t3;')<br>
<br>
box.session.su('admin')<br>
<br>
<br>
<b> New patch:</b><br>
<br>
commit 876ac107e293bd184e247d0e8e299765cb986f9f<br>
Author: Mergen Imeev <a class="moz-txt-link-rfc2396E" href="mailto:imeevma@gmail.com"><imeevma@gmail.com></a><br>
Date: Fri Aug 31 15:50:17 2018 +0300<br>
<br>
sql: clean-up on failed CREATE TABLE<br>
<br>
In case statement "CREATE TABLE ..." fails it can left some<br>
records in system spaces that shouldn't be there. These records<br>
won't be left behind after this patch.<br>
<br>
@TarantoolBot document<br>
Title: Clean up after failure of CREATE TABLE<br>
Usually CREATE TABLE creates no less than two objects which are<br>
space and index. If creation of index (or any object after
space)<br>
failed, created space (and other created objects) won't be
deleted<br>
though operation failed. Now these objects will be deleted<br>
properly.<br>
<br>
Closes #3592<br>
<br>
diff --git a/src/box/sql/build.c b/src/box/sql/build.c<br>
index a806fb4..28b488c 100644<br>
--- a/src/box/sql/build.c<br>
+++ b/src/box/sql/build.c<br>
@@ -55,6 +55,53 @@<br>
#include "box/tuple_format.h"<br>
#include "box/coll_id_cache.h"<br>
<br>
+/**<br>
+ * Structure that contains information about record that was<br>
+ * inserted into system space.<br>
+ */<br>
+struct saved_record<br>
+{<br>
+ /** A link in a record list. */<br>
+ struct rlist link;<br>
+ /** Id of space in which the record was inserted. */<br>
+ uint32_t space_id;<br>
+ /** First register of the key of the record. */<br>
+ int reg_key;<br>
+ /** Number of registers the key consists of. */<br>
+ int reg_key_count;<br>
+ /** The number of the OP_SInsert operation. */<br>
+ int insertion_opcode;<br>
+};<br>
+<br>
+/**<br>
+ * Save inserted in system space record in list.<br>
+ *<br>
+ * @param parser SQL Parser object.<br>
+ * @param space_id Id of table in which record is inserted.<br>
+ * @param reg_key Register that contains first field of the key.<br>
+ * @param reg_key_count Exact number of fields of the key.<br>
+ * @param insertion_opcode Number of OP_SInsert opcode.<br>
+ */<br>
+static inline void<br>
+save_record(struct Parse *parser, uint32_t space_id, int reg_key,<br>
+ int reg_key_count, int insertion_opcode)<br>
+{<br>
+ struct saved_record *record =<br>
+ region_alloc(&parser->region, sizeof(*record));<br>
+ if (record == NULL) {<br>
+ diag_set(OutOfMemory, sizeof(*record), "region_alloc",<br>
+ "record");<br>
+ parser->rc = SQL_TARANTOOL_ERROR;<br>
+ parser->nErr++;<br>
+ return;<br>
+ }<br>
+ record->space_id = space_id;<br>
+ record->reg_key = reg_key;<br>
+ record->reg_key_count = reg_key_count;<br>
+ record->insertion_opcode = insertion_opcode;<br>
+ rlist_add_entry(&parser->record_list, record, link);<br>
+}<br>
+<br>
void<br>
sql_finish_coding(struct Parse *parse_context)<br>
{<br>
@@ -62,6 +109,42 @@ sql_finish_coding(struct Parse *parse_context)<br>
struct sqlite3 *db = parse_context->db;<br>
struct Vdbe *v = sqlite3GetVdbe(parse_context);<br>
sqlite3VdbeAddOp0(v, OP_Halt);<br>
+ /*<br>
+ * In case statement "CREATE TABLE ..." fails it can<br>
+ * left some records in system spaces that shouldn't be<br>
+ * there. To clean-up properly this code is added. Last<br>
+ * record isn't deleted because if statement fails than<br>
+ * it won't be created. This code works the same way for<br>
+ * other "CREATE ..." statements but it won't delete<br>
+ * anything as these statements create no more than one<br>
+ * record.<br>
+ */<br>
+ if (!rlist_empty(&parse_context->record_list)) {<br>
+ struct saved_record *record =<br>
+ rlist_shift_entry(&parse_context->record_list,<br>
+ struct saved_record, link);<br>
+ /* Set P2 of SInsert. */<br>
+ sqlite3VdbeChangeP2(v, record->insertion_opcode,
v->nOp);<br>
+ MAYBE_UNUSED const char *comment =<br>
+ "Delete entry from %s if CREATE TABLE fails";<br>
+ rlist_foreach_entry(record,
&parse_context->record_list, link) {<br>
+ int record_reg = ++parse_context->nMem;<br>
+ sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,<br>
+ record->reg_key_count, record_reg);<br>
+ sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id,<br>
+ record_reg);<br>
+ MAYBE_UNUSED struct space *space =<br>
+ space_by_id(record->space_id);<br>
+ VdbeComment((v, comment, space_name(space)));<br>
+ /* Set P2 of SInsert. */<br>
+ sqlite3VdbeChangeP2(v, record->insertion_opcode,<br>
+ v->nOp);<br>
+ }<br>
+ sqlite3VdbeAddOp1(v, OP_Halt, SQL_TARANTOOL_ERROR);<br>
+ VdbeComment((v,<br>
+ "Exit with an error if CREATE statement fails"));<br>
+ }<br>
+<br>
if (db->mallocFailed || parse_context->nErr != 0) {<br>
if (parse_context->rc == SQLITE_OK)<br>
parse_context->rc = SQLITE_ERROR;<br>
@@ -1101,13 +1184,14 @@ vdbe_emit_create_index(struct Parse *parse,
struct space_def *def,<br>
sqlite3VdbeAddOp4(v, OP_Blob, index_parts_sz, entry_reg + 5,<br>
SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);<br>
sqlite3VdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg);<br>
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, tuple_reg);<br>
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);<br>
/*<br>
* Non-NULL value means that index has been created via<br>
* separate CREATE INDEX statement.<br>
*/<br>
if (idx_def->opts.sql != NULL)<br>
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);<br>
+ save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);<br>
return;<br>
error:<br>
parse->rc = SQL_TARANTOOL_ERROR;<br>
@@ -1165,8 +1249,9 @@ createSpace(Parse * pParse, int iSpaceId, char
*zStmt)<br>
sqlite3VdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,<br>
SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);<br>
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);<br>
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);<br>
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);<br>
sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);<br>
+ save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1);<br>
return;<br>
error:<br>
pParse->nErr++;<br>
@@ -1340,9 +1425,11 @@ vdbe_emit_fkey_create(struct Parse
*parse_context, const struct fkey_def *fk)<br>
parent_links, P4_DYNAMIC);<br>
sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,<br>
constr_tuple_reg + 9);<br>
- sqlite3VdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,<br>
+ sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,<br>
constr_tuple_reg + 9);<br>
sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);<br>
+ save_record(parse_context, BOX_FK_CONSTRAINT_ID,
constr_tuple_reg, 2,<br>
+ vdbe->nOp - 1);<br>
sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);<br>
return;<br>
error:<br>
@@ -1487,14 +1574,18 @@ sqlite3EndTable(Parse * pParse, /* Parse
context */<br>
int reg_seq_record =<br>
emitNewSysSequenceRecord(pParse, reg_seq_id,<br>
p->def->name);<br>
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID,<br>
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,<br>
reg_seq_record);<br>
+ save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,<br>
+ v->nOp - 1);<br>
/* Do an insertion into _space_sequence. */<br>
int reg_space_seq_record =<br>
emitNewSysSpaceSequenceRecord(pParse, reg_space_id,<br>
reg_seq_id);<br>
- sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,<br>
+ sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, 0,<br>
reg_space_seq_record);<br>
+ save_record(pParse, BOX_SPACE_SEQUENCE_ID,<br>
+ reg_space_seq_record + 1, 1, v->nOp - 1);<br>
}<br>
/* Code creation of FK constraints, if any. */<br>
struct fkey_parse *fk_parse;<br>
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c<br>
index e98e845..a4b65eb 100644<br>
--- a/src/box/sql/prepare.c<br>
+++ b/src/box/sql/prepare.c<br>
@@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3
*db)<br>
memset(parser, 0, sizeof(struct Parse));<br>
parser->db = db;<br>
rlist_create(&parser->new_fkey);<br>
+ rlist_create(&parser->record_list);<br>
region_create(&parser->region, &cord()->slabc);<br>
}<br>
<br>
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h<br>
index 744b660..d8eb516 100644<br>
--- a/src/box/sql/sqliteInt.h<br>
+++ b/src/box/sql/sqliteInt.h<br>
@@ -2765,6 +2765,11 @@ struct Parse {<br>
* Foreign key constraint appeared in CREATE TABLE stmt.<br>
*/<br>
struct rlist new_fkey;<br>
+ /**<br>
+ * List of all records that were inserted in system spaces<br>
+ * in current statement.<br>
+ */<br>
+ struct rlist record_list;<br>
bool initiateTTrans; /* Initiate Tarantool transaction */<br>
/** True, if table to be created has AUTOINCREMENT PK. */<br>
bool is_new_table_autoinc;<br>
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c<br>
index 7c1015c..1bec2fa 100644<br>
--- a/src/box/sql/vdbe.c<br>
+++ b/src/box/sql/vdbe.c<br>
@@ -1010,8 +1010,12 @@ case OP_Halt: {<br>
p->pc = pcx;<br>
if (p->rc) {<br>
if (p->rc == SQL_TARANTOOL_ERROR) {<br>
- assert(pOp->p4.z != NULL);<br>
- box_error_set(__FILE__, __LINE__, pOp->p5,
pOp->p4.z);<br>
+ if (pOp->p4.z == NULL) {<br>
+ assert(! diag_is_empty(diag_get()));<br>
+ } else {<br>
+ box_error_set(__FILE__, __LINE__, pOp->p5,<br>
+ pOp->p4.z);<br>
+ }<br>
} else if (pOp->p5 != 0) {<br>
static const char * const azType[] = { "NOT NULL",
"UNIQUE", "CHECK",<br>
"FOREIGN KEY" };<br>
@@ -4308,8 +4312,8 @@ case OP_IdxInsert: {<br>
break;<br>
}<br>
<br>
-/* Opcode: SInsert P1 P2 * * P5<br>
- * Synopsis: space id = P1, key = r[P2]<br>
+/* Opcode: SInsert P1 P2 P3 * P5<br>
+ * Synopsis: space id = P1, key = r[P3], on error goto P2<br>
*<br>
* This opcode is used only during DDL routine.<br>
* In contrast to ordinary insertion, insertion to system spaces<br>
@@ -4322,15 +4326,15 @@ case OP_IdxInsert: {<br>
*/<br>
case OP_SInsert: {<br>
assert(pOp->p1 > 0);<br>
- assert(pOp->p2 >= 0);<br>
+ assert(pOp->p2 > 0);<br>
+ assert(pOp->p3 >= 0);<br>
<br>
- pIn2 = &aMem[pOp->p2];<br>
+ pIn3 = &aMem[pOp->p3];<br>
struct space *space = space_by_id(pOp->p1);<br>
assert(space != NULL);<br>
assert(space_is_system(space));<br>
- rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z +
pIn2->n);<br>
- if (rc)<br>
- goto abort_due_to_error;<br>
+ if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z +
pIn3->n) != 0)<br>
+ goto jump_to_p2;<br>
if (pOp->p5 & OPFLAG_NCHANGE)<br>
p->nChange++;<br>
break;<br>
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result<br>
index 08f2496..43da275 100644<br>
--- a/test/sql/drop-table.result<br>
+++ b/test/sql/drop-table.result<br>
@@ -33,3 +33,147 @@ box.sql.execute("INSERT INTO zzzoobar VALUES
(111, 222, 'c3', 444)")<br>
-- DROP TABLE should do the job<br>
-- Debug<br>
-- require("console").start()<br>
+--<br>
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.<br>
+--<br>
+-- Let user have enough rights to create space, but not enough to<br>
+-- create index.<br>
+--<br>
+box.schema.user.create('tmp')<br>
+---<br>
+...<br>
+box.schema.user.grant('tmp', 'create, read', 'universe')<br>
+---<br>
+...<br>
+box.schema.user.grant('tmp', 'write', 'space', '_space')<br>
+---<br>
+...<br>
+box.schema.user.grant('tmp', 'write', 'space', '_schema')<br>
+---<br>
+...<br>
+-- Number of records in _space, _index, _sequence:<br>
+space_count = #box.space._space:select()<br>
+---<br>
+...<br>
+index_count = #box.space._index:select()<br>
+---<br>
+...<br>
+sequence_count = #box.space._sequence:select()<br>
+---<br>
+...<br>
+box.session.su('tmp')<br>
+---<br>
+...<br>
+--<br>
+-- Error: user do not have rights to write in box.space._index.<br>
+-- Space that was already created should be automatically dropped.<br>
+--<br>
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')<br>
+---<br>
+- error: Write access to space '_index' is denied for user 'tmp'<br>
+...<br>
+-- Error: no such table.<br>
+box.sql.execute('DROP TABLE t1')<br>
+---<br>
+- error: 'no such table: T1'<br>
+...<br>
+box.session.su('admin')<br>
+---<br>
+...<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+---<br>
+- true<br>
+...<br>
+index_count == #box.space._index:select()<br>
+---<br>
+- true<br>
+...<br>
+sequence_count == #box.space._sequence:select()<br>
+---<br>
+- true<br>
+...<br>
+--<br>
+-- Give user right to write in _index. Still have not enough<br>
+-- rights to write in _sequence.<br>
+--<br>
+box.schema.user.grant('tmp', 'write', 'space', '_index')<br>
+---<br>
+...<br>
+box.session.su('tmp')<br>
+---<br>
+...<br>
+--<br>
+-- Error: user do not have rights to write in _sequence.<br>
+--<br>
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY AUTOINCREMENT,
a UNIQUE, b UNIQUE, c UNIQUE, d UNIQUE)')<br>
+---<br>
+- error: Write access to space '_sequence' is denied for user 'tmp'<br>
+...<br>
+box.session.su('admin')<br>
+---<br>
+...<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+---<br>
+- true<br>
+...<br>
+index_count == #box.space._index:select()<br>
+---<br>
+- true<br>
+...<br>
+sequence_count == #box.space._sequence:select()<br>
+---<br>
+- true<br>
+...<br>
+--<br>
+-- Give user right to write in _sequence. Still have not enough<br>
+-- rights to write in _fk_constraint.<br>
+--<br>
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')<br>
+---<br>
+...<br>
+box.session.su('tmp')<br>
+---<br>
+...<br>
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')<br>
+---<br>
+...<br>
+--<br>
+-- Error: user do not have rights to write in _fk_constraint.<br>
+--<br>
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES
t3);')<br>
+---<br>
+- error: Write access to space '_fk_constraint' is denied for user
'tmp'<br>
+...<br>
+box.sql.execute('DROP TABLE t3;')<br>
+---<br>
+...<br>
+box.session.su('admin')<br>
+---<br>
+...<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+---<br>
+- true<br>
+...<br>
+index_count == #box.space._index:select()<br>
+---<br>
+- true<br>
+...<br>
+sequence_count == #box.space._sequence:select()<br>
+---<br>
+- true<br>
+...<br>
+box.schema.user.drop('tmp')<br>
+---<br>
+...<br>
diff --git a/test/sql/drop-table.test.lua
b/test/sql/drop-table.test.lua<br>
index 9663074..f86e3d8 100644<br>
--- a/test/sql/drop-table.test.lua<br>
+++ b/test/sql/drop-table.test.lua<br>
@@ -25,3 +25,86 @@ box.sql.execute("INSERT INTO zzzoobar VALUES
(111, 222, 'c3', 444)")<br>
<br>
-- Debug<br>
-- require("console").start()<br>
+<br>
+--<br>
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.<br>
+--<br>
+-- Let user have enough rights to create space, but not enough to<br>
+-- create index.<br>
+--<br>
+box.schema.user.create('tmp')<br>
+box.schema.user.grant('tmp', 'create, read', 'universe')<br>
+box.schema.user.grant('tmp', 'write', 'space', '_space')<br>
+box.schema.user.grant('tmp', 'write', 'space', '_schema')<br>
+<br>
+-- Number of records in _space, _index, _sequence:<br>
+space_count = #box.space._space:select()<br>
+index_count = #box.space._index:select()<br>
+sequence_count = #box.space._sequence:select()<br>
+<br>
+box.session.su('tmp')<br>
+--<br>
+-- Error: user do not have rights to write in box.space._index.<br>
+-- Space that was already created should be automatically dropped.<br>
+--<br>
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')<br>
+-- Error: no such table.<br>
+box.sql.execute('DROP TABLE t1')<br>
+<br>
+box.session.su('admin')<br>
+<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+index_count == #box.space._index:select()<br>
+sequence_count == #box.space._sequence:select()<br>
+<br>
+--<br>
+-- Give user right to write in _index. Still have not enough<br>
+-- rights to write in _sequence.<br>
+--<br>
+box.schema.user.grant('tmp', 'write', 'space', '_index')<br>
+box.session.su('tmp')<br>
+<br>
+--<br>
+-- Error: user do not have rights to write in _sequence.<br>
+--<br>
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY AUTOINCREMENT,
a UNIQUE, b UNIQUE, c UNIQUE, d UNIQUE)')<br>
+<br>
+box.session.su('admin')<br>
+<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+index_count == #box.space._index:select()<br>
+sequence_count == #box.space._sequence:select()<br>
+<br>
+--<br>
+-- Give user right to write in _sequence. Still have not enough<br>
+-- rights to write in _fk_constraint.<br>
+--<br>
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')<br>
+box.session.su('tmp')<br>
+<br>
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')<br>
+--<br>
+-- Error: user do not have rights to write in _fk_constraint.<br>
+--<br>
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES
t3);')<br>
+box.sql.execute('DROP TABLE t3;')<br>
+<br>
+box.session.su('admin')<br>
+<br>
+--<br>
+-- Check that _space, _index and _sequence have the same number of<br>
+-- records.<br>
+--<br>
+space_count == #box.space._space:select()<br>
+index_count == #box.space._index:select()<br>
+sequence_count == #box.space._sequence:select()<br>
+<br>
+box.schema.user.drop('tmp')<br>
<br>
</body>
</html>