* [tarantool-patches] [PATCH 0/2] SQL TXN DDL
@ 2019-07-19 22:49 Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 1/2] alter: do not access space in drop ck commit trigger Vladislav Shpilevoy
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-19 22:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev
The patchset uses recently introduced transactional DDL in SQL. Nothing more to
say, the goals are quite obvious.
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4086-sql-txn-ddl
Issue: https://github.com/tarantool/tarantool/issues/4086
Vladislav Shpilevoy (2):
alter: do not access space in drop ck commit trigger
sql: transactional DDL
src/box/alter.cc | 49 +---
src/box/space.c | 32 +++
src/box/space.h | 17 ++
src/box/sql/build.c | 159 ++-----------
src/box/sql/parse.y | 12 +-
src/box/sql/prepare.c | 1 -
src/box/sql/sqlInt.h | 7 +-
src/box/sql/trigger.c | 6 +-
src/box/sql/vdbe.c | 19 +-
test/sql/ddl.result | 356 +++++++++++++++++++++++++++++
test/sql/ddl.test.lua | 187 +++++++++++++++
test/sql/errinj.result | 50 ----
test/sql/errinj.test.lua | 22 --
test/sql/view_delayed_wal.result | 12 +-
test/sql/view_delayed_wal.test.lua | 8 +-
15 files changed, 654 insertions(+), 283 deletions(-)
create mode 100644 test/sql/ddl.result
create mode 100644 test/sql/ddl.test.lua
--
2.20.1 (Apple Git-117)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH 1/2] alter: do not access space in drop ck commit trigger
2019-07-19 22:49 [tarantool-patches] [PATCH 0/2] SQL TXN DDL Vladislav Shpilevoy
@ 2019-07-19 22:49 ` Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 2/2] sql: transactional DDL Vladislav Shpilevoy
2019-07-31 13:19 ` [tarantool-patches] Re: [PATCH 0/2] SQL TXN DDL Kirill Yukhin
2 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-19 22:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev
When transactional DDL is introduced, 'DROP TABLE' will remove a
space and its cks in one transaction. At the moment of that
transaction commit the space is already removed from _space and
from space cache, and can't be accessed from other triggers.
This patch makes all space-related actions in on_replace.
Part of #4086
---
src/box/alter.cc | 49 +++++++-----------------------------------------
src/box/space.c | 32 +++++++++++++++++++++++++++++++
src/box/space.h | 17 +++++++++++++++++
3 files changed, 56 insertions(+), 42 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index e8721fd16..b3244094e 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4526,17 +4526,10 @@ on_create_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
assert(ck != NULL);
struct space *space = space_by_id(ck->def->space_id);
assert(space != NULL);
- struct trigger *ck_trigger = space->ck_constraint_trigger;
- assert(ck_trigger != NULL);
assert(space_ck_constraint_by_name(space, ck->def->name,
strlen(ck->def->name)) != NULL);
- rlist_del_entry(ck, link);
+ space_remove_ck_constraint(space, ck);
ck_constraint_delete(ck);
- if (rlist_empty(&space->ck_constraint)) {
- trigger_clear(ck_trigger);
- ck_trigger->destroy(ck_trigger);
- space->ck_constraint_trigger = NULL;
- }
trigger_run_xc(&on_alter_space, space);
}
@@ -4546,15 +4539,7 @@ on_drop_ck_constraint_commit(struct trigger *trigger, void * /* event */)
{
struct ck_constraint *ck = (struct ck_constraint *)trigger->data;
assert(ck != NULL);
- struct space *space = space_by_id(ck->def->space_id);
- assert(space != NULL);
- struct trigger *ck_trigger = space->ck_constraint_trigger;
- assert(ck_trigger != NULL);
- if (rlist_empty(&space->ck_constraint)) {
- ck_trigger->destroy(ck_trigger);
- space->ck_constraint_trigger = NULL;
- ck_constraint_delete(ck);
- }
+ ck_constraint_delete(ck);
}
/** Rollback DELETE check constraint. */
@@ -4565,13 +4550,10 @@ on_drop_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
assert(ck != NULL);
struct space *space = space_by_id(ck->def->space_id);
assert(space != NULL);
- struct trigger *ck_trigger = space->ck_constraint_trigger;
- assert(ck_trigger != NULL);
assert(space_ck_constraint_by_name(space, ck->def->name,
strlen(ck->def->name)) == NULL);
- rlist_add_entry(&space->ck_constraint, ck, link);
- if (rlist_empty(&ck_trigger->link))
- trigger_add(&space->on_replace, ck_trigger);
+ if (space_add_ck_constraint(space, ck) != 0)
+ panic("Can't recover after CK constraint drop rollback");
trigger_run_xc(&on_alter_space, space);
}
@@ -4613,8 +4595,6 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
tuple_field_u32_xc(old_tuple != NULL ? old_tuple : new_tuple,
BOX_CK_CONSTRAINT_FIELD_SPACE_ID);
struct space *space = space_cache_find_xc(space_id);
- struct trigger *ck_trigger = space->ck_constraint_trigger;
- assert(ck_trigger == NULL || !rlist_empty(&ck_trigger->link));
struct trigger *on_rollback = txn_alter_trigger_new(NULL, NULL);
struct trigger *on_commit = txn_alter_trigger_new(NULL, NULL);
@@ -4655,20 +4635,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
space_ck_constraint_by_name(space, name, strlen(name));
if (old_ck_constraint != NULL)
rlist_del_entry(old_ck_constraint, link);
- rlist_add_entry(&space->ck_constraint, new_ck_constraint, link);
- if (ck_trigger == NULL) {
- ck_trigger =
- (struct trigger *)malloc(sizeof(*ck_trigger));
- if (ck_trigger == NULL) {
- tnt_raise(OutOfMemory, sizeof(*ck_trigger),
- "malloc", "ck_trigger");
- }
- trigger_create(ck_trigger,
- ck_constraint_on_replace_trigger, NULL,
- (trigger_f0) free);
- trigger_add(&space->on_replace, ck_trigger);
- space->ck_constraint_trigger = ck_trigger;
- }
+ if (space_add_ck_constraint(space, new_ck_constraint) != 0)
+ diag_raise();
ck_guard.is_active = false;
if (old_tuple != NULL) {
on_rollback->data = old_ck_constraint;
@@ -4680,7 +4648,6 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
on_commit->data = old_ck_constraint;
on_commit->run = on_replace_ck_constraint_commit;
} else {
- assert(ck_trigger != NULL);
assert(new_tuple == NULL && old_tuple != NULL);
/* Drop check constraint. */
uint32_t name_len;
@@ -4691,9 +4658,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
struct ck_constraint *old_ck_constraint =
space_ck_constraint_by_name(space, name, name_len);
assert(old_ck_constraint != NULL);
- rlist_del_entry(old_ck_constraint, link);
- if (rlist_empty(&space->ck_constraint))
- trigger_clear(ck_trigger);
+ space_remove_ck_constraint(space, old_ck_constraint);
on_commit->data = old_ck_constraint;
on_commit->run = on_drop_ck_constraint_commit;
on_rollback->data = old_ck_constraint;
diff --git a/src/box/space.c b/src/box/space.c
index e7babb2ae..d81948ff3 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -545,6 +545,38 @@ space_execute_dml(struct space *space, struct txn *txn,
return 0;
}
+int
+space_add_ck_constraint(struct space *space, struct ck_constraint *ck)
+{
+ rlist_add_entry(&space->ck_constraint, ck, link);
+ if (space->ck_constraint_trigger == NULL) {
+ struct trigger *ck_trigger =
+ (struct trigger *) malloc(sizeof(*ck_trigger));
+ if (ck_trigger == NULL) {
+ diag_set(OutOfMemory, sizeof(*ck_trigger), "malloc",
+ "ck_trigger");
+ return -1;
+ }
+ trigger_create(ck_trigger, ck_constraint_on_replace_trigger,
+ NULL, (trigger_f0) free);
+ trigger_add(&space->on_replace, ck_trigger);
+ space->ck_constraint_trigger = ck_trigger;
+ }
+ return 0;
+}
+
+void
+space_remove_ck_constraint(struct space *space, struct ck_constraint *ck)
+{
+ rlist_del_entry(ck, link);
+ if (rlist_empty(&space->ck_constraint)) {
+ struct trigger *ck_trigger = space->ck_constraint_trigger;
+ trigger_clear(ck_trigger);
+ ck_trigger->destroy(ck_trigger);
+ space->ck_constraint_trigger = NULL;
+ }
+}
+
/* {{{ Virtual method stubs */
size_t
diff --git a/src/box/space.h b/src/box/space.h
index 88db4ec52..708584968 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -50,6 +50,7 @@ struct request;
struct port;
struct tuple;
struct tuple_format;
+struct ck_constraint;
struct space_vtab {
/** Free a space instance. */
@@ -475,6 +476,22 @@ space_dump_def(const struct space *space, struct rlist *key_list);
void
space_fill_index_map(struct space *space);
+/**
+ * Add a new ck constraint to the space. A ck constraint check
+ * trigger is created, if this is a first ck in this space. The
+ * space takes ownership of this object.
+ */
+int
+space_add_ck_constraint(struct space *space, struct ck_constraint *ck);
+
+/**
+ * Remove a ck constraint from the space. A ck constraint check
+ * trigger is deleted, if this is a last ck in this space. This
+ * object may be deleted manually after the call.
+ */
+void
+space_remove_ck_constraint(struct space *space, struct ck_constraint *ck);
+
/*
* Virtual method stubs.
*/
--
2.20.1 (Apple Git-117)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH 2/2] sql: transactional DDL
2019-07-19 22:49 [tarantool-patches] [PATCH 0/2] SQL TXN DDL Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 1/2] alter: do not access space in drop ck commit trigger Vladislav Shpilevoy
@ 2019-07-19 22:49 ` Vladislav Shpilevoy
2019-07-20 7:42 ` [tarantool-patches] " Konstantin Osipov
2019-07-22 13:59 ` n.pettik
2019-07-31 13:19 ` [tarantool-patches] Re: [PATCH 0/2] SQL TXN DDL Kirill Yukhin
2 siblings, 2 replies; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-19 22:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev
Box recently added support of transactional DDL allowing to do
any number of non-yielding DDL operations atomically. This is
really a big relief of one of the biggest pains of SQL. Before
this patch each multirow SQL DDL statement needed to prepare its
own rollback procedure for a case if something would go wrong.
Now with box support SQL wraps each DDL statement into a
transaction, and doesn't need own escape-routes in a form of
'struct save_record' and others.
Closes #4086
---
src/box/sql/build.c | 159 ++-----------
src/box/sql/parse.y | 12 +-
src/box/sql/prepare.c | 1 -
src/box/sql/sqlInt.h | 7 +-
src/box/sql/trigger.c | 6 +-
src/box/sql/vdbe.c | 19 +-
test/sql/ddl.result | 356 +++++++++++++++++++++++++++++
test/sql/ddl.test.lua | 187 +++++++++++++++
test/sql/errinj.result | 50 ----
test/sql/errinj.test.lua | 22 --
test/sql/view_delayed_wal.result | 12 +-
test/sql/view_delayed_wal.test.lua | 8 +-
12 files changed, 598 insertions(+), 241 deletions(-)
create mode 100644 test/sql/ddl.result
create mode 100644 test/sql/ddl.test.lua
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 2aefa2a3f..4884a7855 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -57,64 +57,6 @@
#include "box/tuple_format.h"
#include "box/coll_id_cache.h"
-/**
- * Structure that contains information about record that was
- * inserted into system space.
- */
-struct saved_record
-{
- /** A link in a record list. */
- struct rlist link;
- /** Id of space in which the record was inserted. */
- uint32_t space_id;
- /** First register of the key of the record. */
- int reg_key;
- /** Number of registers the key consists of. */
- int reg_key_count;
- /** The address of the opcode. */
- int op_addr;
- /** Flag to show that operation is SInsert. */
- bool is_insert;
-};
-
-/**
- * Save inserted in system space record in list. This procedure is
- * called after generation of either OP_SInsert or OP_NoColflict +
- * OP_SetDiag. In the first case, record inserted to the system
- * space is supposed to be deleted on error; in the latter - jump
- * target specified in OP_SetDiag should be adjusted to the start
- * of clean-up routines (current entry isn't inserted to the space
- * yet, so there's no need to delete it).
- *
- * @param parser SQL Parser object.
- * @param space_id Id of table in which record is inserted.
- * @param reg_key Register that contains first field of the key.
- * @param reg_key_count Exact number of fields of the key.
- * @param op_addr Address of opcode (OP_SetDiag or OP_SInsert).
- * Used to fix jump target (see
- * sql_finish_coding()).
- * @param is_insert_op Whether opcode is OP_SInsert or not.
- */
-static inline void
-save_record(struct Parse *parser, uint32_t space_id, int reg_key,
- int reg_key_count, int op_addr, bool is_insert_op)
-{
- struct saved_record *record =
- region_alloc(&parser->region, sizeof(*record));
- if (record == NULL) {
- diag_set(OutOfMemory, sizeof(*record), "region_alloc",
- "record");
- parser->is_aborted = true;
- return;
- }
- record->space_id = space_id;
- record->reg_key = reg_key;
- record->reg_key_count = reg_key_count;
- record->op_addr = op_addr;
- record->is_insert = is_insert_op;
- rlist_add_entry(&parser->record_list, record, link);
-}
-
void
sql_finish_coding(struct Parse *parse_context)
{
@@ -122,52 +64,6 @@ sql_finish_coding(struct Parse *parse_context)
struct sql *db = parse_context->db;
struct Vdbe *v = sqlGetVdbe(parse_context);
sqlVdbeAddOp0(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. Hence for processed insertions we should remove
- * entries from corresponding system spaces alongside
- * with fixing jump address for OP_SInsert opcode in
- * case it fails during VDBE runtime; for OP_SetDiag only
- * adjust jump target to the start of clean-up program
- * for already inserted entries.
- */
- if (!rlist_empty(&parse_context->record_list)) {
- struct saved_record *record =
- rlist_shift_entry(&parse_context->record_list,
- struct saved_record, link);
- /*
- * Set jump target for OP_SetDiag and OP_SInsert.
- */
- sqlVdbeChangeP2(v, record->op_addr, v->nOp);
- MAYBE_UNUSED const char *comment =
- "Delete entry from %s if CREATE TABLE fails";
- rlist_foreach_entry(record, &parse_context->record_list, link) {
- if (record->is_insert) {
- int rec_reg = ++parse_context->nMem;
- sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key,
- record->reg_key_count, rec_reg);
- sqlVdbeAddOp2(v, OP_SDelete, record->space_id,
- rec_reg);
- MAYBE_UNUSED struct space *space =
- space_by_id(record->space_id);
- VdbeComment((v, comment, space_name(space)));
- }
- /*
- * Set jump target for OP_SetDiag and
- * OP_SInsert.
- */
- sqlVdbeChangeP2(v, record->op_addr, v->nOp);
- }
- sqlVdbeAddOp1(v, OP_Halt, -1);
- VdbeComment((v,
- "Exit with an error if CREATE statement fails"));
- }
if (db->mallocFailed)
parse_context->is_aborted = true;
@@ -933,8 +829,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
sqlVdbeAddOp4(v, OP_Blob, index_parts_sz, entry_reg + 5,
SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
sqlVdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg);
- sqlVdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);
- save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1, true);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, tuple_reg);
return;
error:
parse->is_aborted = true;
@@ -991,9 +886,8 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,
SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);
sqlVdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, tuple_reg);
- sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, tuple_reg);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, tuple_reg);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
- save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1, true);
return;
error:
pParse->is_aborted = true;
@@ -1102,8 +996,7 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
* Occupy registers for 5 fields: each member in
* _ck_constraint space plus one for final msgpack tuple.
*/
- int ck_constraint_reg = parser->nMem + 1;
- parser->nMem += 6;
+ int ck_constraint_reg = sqlGetTempRange(parser, 6);
sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg);
sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 1, 0,
sqlDbStrDup(db, ck_def->name), P4_DYNAMIC);
@@ -1120,13 +1013,12 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
ck_constraint_reg, 2,
ER_CONSTRAINT_EXISTS, error_msg,
- false, OP_NoConflict, true) != 0)
+ false, OP_NoConflict) != 0)
return;
- sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0,
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
ck_constraint_reg + 5);
- save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2,
- v->nOp - 1, true);
VdbeComment((v, "Create CK constraint %s", ck_def->name));
+ sqlReleaseTempRange(parser, ck_constraint_reg, 6);
}
/**
@@ -1148,8 +1040,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
* Occupy registers for 9 fields: each member in
* _fk_constraint space plus one for final msgpack tuple.
*/
- int constr_tuple_reg = parse_context->nMem + 1;
- parse_context->nMem += 10;
+ int constr_tuple_reg = sqlGetTempRange(parse_context, 10);
char *name_copy = sqlDbStrDup(parse_context->db, fk->name);
if (name_copy == NULL)
return;
@@ -1185,7 +1076,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
BOX_FK_CONSTRAINT_ID, 0,
constr_tuple_reg, 2,
ER_CONSTRAINT_EXISTS, error_msg,
- false, OP_NoConflict, true) != 0)
+ false, OP_NoConflict) != 0)
return;
sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
@@ -1228,14 +1119,13 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
parent_links, P4_DYNAMIC);
sqlVdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,
constr_tuple_reg + 9);
- sqlVdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
- constr_tuple_reg + 9);
+ sqlVdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
+ constr_tuple_reg + 9);
if (parse_context->create_table_def.new_space == NULL) {
sqlVdbeCountChanges(vdbe);
sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
}
- save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
- vdbe->nOp - 1, true);
+ sqlReleaseTempRange(parse_context, constr_tuple_reg, 10);
return;
error:
parse_context->is_aborted = true;
@@ -1331,7 +1221,7 @@ sqlEndTable(struct Parse *pParse)
if (vdbe_emit_halt_with_presence_test(pParse, BOX_SPACE_ID, 2,
name_reg, 1, ER_SPACE_EXISTS,
error_msg, (no_err != 0),
- OP_NoConflict, false) != 0)
+ OP_NoConflict) != 0)
return;
int reg_space_id = getNewSpaceId(pParse);
@@ -1354,18 +1244,13 @@ sqlEndTable(struct Parse *pParse)
int reg_seq_record =
emitNewSysSequenceRecord(pParse, reg_seq_id,
new_space->def->name);
- sqlVdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,
- reg_seq_record);
- save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
- v->nOp - 1, true);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
/* Do an insertion into _space_sequence. */
int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
reg_space_id, reg_seq_id,
new_space->index[0]->def);
- sqlVdbeAddOp3(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, true);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
+ reg_space_seq_record);
}
/* Code creation of FK constraints, if any. */
struct fk_constraint_parse *fk_parse;
@@ -1492,7 +1377,7 @@ sql_create_view(struct Parse *parse_context)
if (vdbe_emit_halt_with_presence_test(parse_context, BOX_SPACE_ID, 2,
name_reg, 1, ER_SPACE_EXISTS,
error_msg, (no_err != 0),
- OP_NoConflict, false) != 0)
+ OP_NoConflict) != 0)
goto create_view_fail;
vdbe_emit_space_create(parse_context, getNewSpaceId(parse_context),
@@ -1611,7 +1496,7 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
BOX_FK_CONSTRAINT_ID, 0,
key_reg, 2, ER_NO_SUCH_CONSTRAINT,
error_msg, false,
- OP_Found, false) != 0) {
+ OP_Found) != 0) {
sqlDbFree(parse_context->db, constraint_name);
return;
}
@@ -1643,8 +1528,7 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false,
- OP_Found, false) != 0)
+ error_msg, false, OP_Found) != 0)
return;
sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
@@ -3310,7 +3194,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int index_id, int key_reg, uint32_t key_len,
int tarantool_error_code,
const char *error_src, bool no_error,
- int cond_opcode, bool is_clean_needed)
+ int cond_opcode)
{
assert(cond_opcode == OP_NoConflict || cond_opcode == OP_Found);
struct Vdbe *v = sqlGetVdbe(parser);
@@ -3331,10 +3215,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
} else {
sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
P4_DYNAMIC);
- if (is_clean_needed)
- save_record(parser, 0, 0, 0, v->nOp - 1, false);
- else
- sqlVdbeAddOp1(v, OP_Halt, -1);
+ sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
}
sqlVdbeJumpHere(v, addr);
sqlVdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 2a60ad25b..06eab7f2d 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -172,6 +172,7 @@ cmd ::= create_table create_table_args.
create_table ::= CREATE TABLE ifnotexists(E) nm(Y). {
create_table_def_init(&pParse->create_table_def, &Y, E);
pParse->create_table_def.new_space = sqlStartTable(pParse, &Y);
+ pParse->initiateTTrans = true;
}
%type ifnotexists {int}
@@ -380,12 +381,14 @@ resolvetype(A) ::= REPLACE. {A = ON_CONFLICT_ACTION_REPLACE;}
cmd ::= DROP TABLE ifexists(E) fullname(X) . {
struct Token t = Token_nil;
drop_table_def_init(&pParse->drop_table_def, X, &t, E);
+ pParse->initiateTTrans = true;
sql_drop_table(pParse);
}
cmd ::= DROP VIEW ifexists(E) fullname(X) . {
struct Token t = Token_nil;
drop_view_def_init(&pParse->drop_view_def, X, &t, E);
+ pParse->initiateTTrans = true;
sql_drop_table(pParse);
}
@@ -399,6 +402,7 @@ cmd ::= CREATE(X) VIEW ifnotexists(E) nm(Y) eidlist_opt(C)
AS select(S). {
if (!pParse->parse_only) {
create_view_def_init(&pParse->create_view_def, &Y, &X, C, S, E);
+ pParse->initiateTTrans = true;
sql_create_view(pParse);
} else {
sql_store_select(pParse, S);
@@ -1404,6 +1408,7 @@ cmd ::= CREATE uniqueflag(U) INDEX ifnotexists(NE) nm(X)
}
create_index_def_init(&pParse->create_index_def, src_list, &X, Z, U,
SORT_ORDER_ASC, NE);
+ pParse->initiateTTrans = true;
sql_create_index(pParse);
}
@@ -1456,6 +1461,7 @@ eidlist(A) ::= nm(Y). {
//
cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). {
drop_index_def_init(&pParse->drop_index_def, Y, &X, E);
+ pParse->initiateTTrans = true;
sql_drop_index(pParse);
}
@@ -1502,7 +1508,7 @@ cmd ::= CREATE trigger_decl(A) BEGIN trigger_cmd_list(S) END(Z). {
Token all;
all.z = A.z;
all.n = (int)(Z.z - A.z) + Z.n;
- pParse->initiateTTrans = false;
+ pParse->initiateTTrans = true;
sql_trigger_finish(pParse, S, &all);
}
@@ -1652,6 +1658,7 @@ raisetype(A) ::= FAIL. {A = ON_CONFLICT_ACTION_FAIL;}
cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
struct Token t = Token_nil;
drop_trigger_def_init(&pParse->drop_trigger_def, X, &t, NOERR);
+ pParse->initiateTTrans = true;
sql_drop_trigger(pParse);
}
@@ -1671,6 +1678,7 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
A.table_name = T;
A.name = N;
+ pParse->initiateTTrans = true;
}
cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
@@ -1697,11 +1705,13 @@ unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; }
cmd ::= alter_table_start(A) RENAME TO nm(N). {
rename_entity_def_init(&pParse->rename_entity_def, A, &N);
+ pParse->initiateTTrans = true;
sql_alter_table_rename(pParse);
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ pParse->initiateTTrans = true;
sql_drop_foreign_key(pParse);
}
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 84fb31bcd..36c21a221 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -243,7 +243,6 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
memset(parser, 0, sizeof(struct Parse));
parser->db = db;
parser->sql_flags = sql_flags;
- rlist_create(&parser->record_list);
region_create(&parser->region, &cord()->slabc);
}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4f5bad287..99296b4b3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2329,11 +2329,6 @@ struct Parse {
* sqlEndTable() function).
*/
struct create_table_def create_table_def;
- /**
- * List of all records that were inserted in system spaces
- * in current statement.
- */
- struct rlist record_list;
bool initiateTTrans; /* Initiate Tarantool transaction */
/** If set - do not emit byte code at all, just parse. */
bool parse_only;
@@ -4542,7 +4537,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int index_id, int key_reg, uint32_t key_len,
int tarantool_error_code,
const char *error_src, bool no_error,
- int cond_opcode, bool is_clean_needed);
+ int cond_opcode);
/**
* Generate VDBE code to delete records from system _sql_stat1 or
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 562723959..d746ef893 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -112,8 +112,7 @@ sql_trigger_begin(struct Parse *parse)
name_reg, 1,
ER_TRIGGER_EXISTS,
error_msg, (no_err != 0),
- OP_NoConflict,
- false) != 0)
+ OP_NoConflict) != 0)
goto trigger_cleanup;
}
@@ -421,8 +420,7 @@ sql_drop_trigger(struct Parse *parser)
sqlVdbeAddOp4(v, OP_String8, 0, name_reg, 0, name_copy, P4_DYNAMIC);
if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0,
name_reg, 1, ER_NO_SUCH_TRIGGER,
- error_msg, no_err, OP_Found,
- false) != 0)
+ error_msg, no_err, OP_Found) != 0)
goto drop_trigger_cleanup;
vdbe_code_drop_trigger(parser, trigger_name, true);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6a4a303b9..a71b331f8 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4359,8 +4359,8 @@ case OP_Update: {
break;
}
-/* Opcode: SInsert P1 P2 P3 * P5
- * Synopsis: space id = P1, key = r[P3], on error goto P2
+/* Opcode: SInsert P1 P2 * * P5
+ * Synopsis: space id = P1, key = r[P2]
*
* This opcode is used only during DDL routine.
* In contrast to ordinary insertion, insertion to system spaces
@@ -4373,15 +4373,16 @@ case OP_Update: {
*/
case OP_SInsert: {
assert(pOp->p1 > 0);
- assert(pOp->p2 > 0);
- assert(pOp->p3 >= 0);
+ assert(pOp->p2 >= 0);
- pIn3 = &aMem[pOp->p3];
+ pIn2 = &aMem[pOp->p2];
struct space *space = space_by_id(pOp->p1);
assert(space != NULL);
assert(space_is_system(space));
- if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
- goto jump_to_p2;
+ if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) {
+ p->errorAction = ON_CONFLICT_ACTION_ABORT;
+ goto abort_due_to_error;
+ }
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
break;
@@ -4404,8 +4405,10 @@ case OP_SDelete: {
struct space *space = space_by_id(pOp->p1);
assert(space != NULL);
assert(space_is_system(space));
- if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0)
+ if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) {
+ p->errorAction = ON_CONFLICT_ACTION_ABORT;
goto abort_due_to_error;
+ }
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
break;
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
new file mode 100644
index 000000000..8f7f91151
--- /dev/null
+++ b/test/sql/ddl.result
@@ -0,0 +1,356 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+ | ---
+ | - row_count: 0
+ | ...
+
+--
+-- gh-4086: SQL transactional DDL.
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.space.T1 ~= nil
+ | ---
+ | - true
+ | ...
+box.space.T1.index[0] ~= nil
+ | ---
+ | - true
+ | ...
+box.space.T2 ~= nil
+ | ---
+ | - true
+ | ...
+box.space.T2.index[0] ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('DROP TABLE t1;')
+assert(box.space.T1 == nil)
+assert(box.space.T2 ~= nil)
+box.execute('DROP TABLE t2;')
+assert(box.space.T2 == nil)
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+--
+-- Use all the possible SQL DDL statements.
+--
+test_run:cmd("setopt delimiter '$'")
+ | ---
+ | - true
+ | ...
+function monster_ddl()
+ local _, err1, err2, err3
+ box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER);]])
+ box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER UNIQUE,
+ CONSTRAINT ck1 CHECK(b < 100));]])
+
+ box.execute('CREATE INDEX t1a ON t1(a);')
+ box.execute('CREATE INDEX t2a ON t2(a);')
+ box.execute('DROP INDEX t2a ON t2;')
+
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
+ box.space.T1.ck_constraint.CK1:drop()
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+ box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
+
+-- Try random errors inside this big batch of DDL to ensure, that
+-- they do not affect normal operation.
+ _, err1 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+
+ box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY
+ KEY AUTOINCREMENT);]])
+
+ box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err2 = pcall(box.execute, 'DROP TABLE t3;')
+
+ box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err3 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
+
+ box.execute('DROP TRIGGER t2t;')
+
+ return 'Finished ok, errors in the middle: ', err1, err2, err3
+end$
+ | ---
+ | ...
+function monster_ddl_is_clean()
+ assert(box.space.T1 == nil)
+ assert(box.space.T2 == nil)
+ assert(box.space._trigger:count() == 0)
+ assert(box.space._fk_constraint:count() == 0)
+ assert(box.space._ck_constraint:count() == 0)
+end$
+ | ---
+ | ...
+function monster_ddl_check()
+ local _, err1, err2, err3, err4, res
+ _, err1 = pcall(box.execute, 'INSERT INTO t2 VALUES (1, 1, 101)')
+ box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+ _, err2 = pcall(box.execute, 'INSERT INTO t2 VALUES(2, 2, 1)')
+ _, err3 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, 20, 1)')
+ _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)')
+ box.execute('INSERT INTO t1 VALUES (1, 1, 1)')
+ res = box.execute('SELECT * FROM trigger_catcher')
+ return 'Finished ok, errors and trigger catcher content: ', err1, err2,
+ err3, err4, res
+end$
+ | ---
+ | ...
+function monster_ddl_clear()
+ box.execute('DROP TRIGGER IF EXISTS t1t;')
+ box.execute('DROP TABLE IF EXISTS trigger_catcher;')
+ pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;')
+ box.execute('DROP TABLE IF EXISTS t2')
+ box.execute('DROP TABLE IF EXISTS t1')
+ monster_ddl_is_clean()
+end$
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''")$
+ | ---
+ | - true
+ | ...
+
+-- No txn.
+monster_ddl()
+ | ---
+ | - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+monster_ddl_check()
+ | ---
+ | - 'Finished ok, errors and trigger catcher content: '
+ | - 'Check constraint failed ''CK1'': b < 100'
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | - 'Check constraint failed ''CK2'': a > 0'
+ | - metadata:
+ | - name: ID
+ | type: integer
+ | rows:
+ | - [1]
+ | ...
+monster_ddl_clear()
+ | ---
+ | ...
+
+-- Both DDL and cleanup in one txn.
+res = nil
+ | ---
+ | ...
+box.begin() res = {monster_ddl()} monster_ddl_clear() box.commit()
+ | ---
+ | ...
+res
+ | ---
+ | - - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+
+-- DDL in txn, cleanup is not.
+box.begin() res = {monster_ddl()} box.commit()
+ | ---
+ | ...
+res
+ | ---
+ | - - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+monster_ddl_check()
+ | ---
+ | - 'Finished ok, errors and trigger catcher content: '
+ | - 'Check constraint failed ''CK1'': b < 100'
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | - 'Check constraint failed ''CK2'': a > 0'
+ | - metadata:
+ | - name: ID
+ | type: integer
+ | rows:
+ | - [1]
+ | ...
+monster_ddl_clear()
+ | ---
+ | ...
+
+-- DDL is not in txn, cleanup is.
+monster_ddl()
+ | ---
+ | - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+monster_ddl_check()
+ | ---
+ | - 'Finished ok, errors and trigger catcher content: '
+ | - 'Check constraint failed ''CK1'': b < 100'
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | - 'Check constraint failed ''CK2'': a > 0'
+ | - metadata:
+ | - name: ID
+ | type: integer
+ | rows:
+ | - [1]
+ | ...
+box.begin() monster_ddl_clear() box.commit()
+ | ---
+ | ...
+
+-- DDL and cleanup in separate txns.
+box.begin() monster_ddl() box.commit()
+ | ---
+ | ...
+monster_ddl_check()
+ | ---
+ | - 'Finished ok, errors and trigger catcher content: '
+ | - 'Check constraint failed ''CK1'': b < 100'
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | - 'Check constraint failed ''CK2'': a > 0'
+ | - metadata:
+ | - name: ID
+ | type: integer
+ | rows:
+ | - [1]
+ | ...
+box.begin() monster_ddl_clear() box.commit()
+ | ---
+ | ...
+
+--
+-- Voluntary rollback.
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+assert(box.space.T1 ~= nil)
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+assert(box.space.T2 ~= nil)
+box.rollback();
+ | ---
+ | ...
+
+box.space.T1 == nil and box.space.T2 == nil;
+ | ---
+ | - true
+ | ...
+
+box.begin()
+save1 = box.savepoint()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY)')
+save2 = box.savepoint()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER)')
+box.execute('CREATE INDEX t2a ON t2(a)')
+save3 = box.savepoint()
+assert(box.space.T1 ~= nil)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save3)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+save3 = box.savepoint()
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save2)
+assert(box.space.T2 == nil)
+assert(box.space.T1 ~= nil)
+box.rollback_to_savepoint(save1)
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.space.T1 == nil and box.space.T2 == nil
+ | ---
+ | - true
+ | ...
+
+--
+-- Unexpected rollback.
+--
+
+box.begin() res = {monster_ddl()} require('fiber').yield()
+ | ---
+ | ...
+res
+ | ---
+ | - - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+box.commit()
+ | ---
+ | - error: Transaction has been aborted by a fiber yield
+ | ...
+box.rollback()
+ | ---
+ | ...
+monster_ddl_clear()
+ | ---
+ | ...
diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
new file mode 100644
index 000000000..477158796
--- /dev/null
+++ b/test/sql/ddl.test.lua
@@ -0,0 +1,187 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+
+--
+-- gh-4086: SQL transactional DDL.
+--
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+box.space.T1 ~= nil
+box.space.T1.index[0] ~= nil
+box.space.T2 ~= nil
+box.space.T2.index[0] ~= nil
+
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('DROP TABLE t1;')
+assert(box.space.T1 == nil)
+assert(box.space.T2 ~= nil)
+box.execute('DROP TABLE t2;')
+assert(box.space.T2 == nil)
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+--
+-- Use all the possible SQL DDL statements.
+--
+test_run:cmd("setopt delimiter '$'")
+function monster_ddl()
+ local _, err1, err2, err3
+ box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER);]])
+ box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER UNIQUE,
+ CONSTRAINT ck1 CHECK(b < 100));]])
+
+ box.execute('CREATE INDEX t1a ON t1(a);')
+ box.execute('CREATE INDEX t2a ON t2(a);')
+ box.execute('DROP INDEX t2a ON t2;')
+
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
+ box.space.T1.ck_constraint.CK1:drop()
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+ box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
+
+-- Try random errors inside this big batch of DDL to ensure, that
+-- they do not affect normal operation.
+ _, err1 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+
+ box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY
+ KEY AUTOINCREMENT);]])
+
+ box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err2 = pcall(box.execute, 'DROP TABLE t3;')
+
+ box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err3 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
+
+ box.execute('DROP TRIGGER t2t;')
+
+ return 'Finished ok, errors in the middle: ', err1, err2, err3
+end$
+function monster_ddl_is_clean()
+ assert(box.space.T1 == nil)
+ assert(box.space.T2 == nil)
+ assert(box.space._trigger:count() == 0)
+ assert(box.space._fk_constraint:count() == 0)
+ assert(box.space._ck_constraint:count() == 0)
+end$
+function monster_ddl_check()
+ local _, err1, err2, err3, err4, res
+ _, err1 = pcall(box.execute, 'INSERT INTO t2 VALUES (1, 1, 101)')
+ box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+ _, err2 = pcall(box.execute, 'INSERT INTO t2 VALUES(2, 2, 1)')
+ _, err3 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, 20, 1)')
+ _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)')
+ box.execute('INSERT INTO t1 VALUES (1, 1, 1)')
+ res = box.execute('SELECT * FROM trigger_catcher')
+ return 'Finished ok, errors and trigger catcher content: ', err1, err2,
+ err3, err4, res
+end$
+function monster_ddl_clear()
+ box.execute('DROP TRIGGER IF EXISTS t1t;')
+ box.execute('DROP TABLE IF EXISTS trigger_catcher;')
+ pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;')
+ box.execute('DROP TABLE IF EXISTS t2')
+ box.execute('DROP TABLE IF EXISTS t1')
+ monster_ddl_is_clean()
+end$
+test_run:cmd("setopt delimiter ''")$
+
+-- No txn.
+monster_ddl()
+monster_ddl_check()
+monster_ddl_clear()
+
+-- Both DDL and cleanup in one txn.
+res = nil
+box.begin() res = {monster_ddl()} monster_ddl_clear() box.commit()
+res
+
+-- DDL in txn, cleanup is not.
+box.begin() res = {monster_ddl()} box.commit()
+res
+monster_ddl_check()
+monster_ddl_clear()
+
+-- DDL is not in txn, cleanup is.
+monster_ddl()
+monster_ddl_check()
+box.begin() monster_ddl_clear() box.commit()
+
+-- DDL and cleanup in separate txns.
+box.begin() monster_ddl() box.commit()
+monster_ddl_check()
+box.begin() monster_ddl_clear() box.commit()
+
+--
+-- Voluntary rollback.
+--
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+assert(box.space.T1 ~= nil)
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+assert(box.space.T2 ~= nil)
+box.rollback();
+
+box.space.T1 == nil and box.space.T2 == nil;
+
+box.begin()
+save1 = box.savepoint()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY)')
+save2 = box.savepoint()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER)')
+box.execute('CREATE INDEX t2a ON t2(a)')
+save3 = box.savepoint()
+assert(box.space.T1 ~= nil)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save3)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+save3 = box.savepoint()
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save2)
+assert(box.space.T2 == nil)
+assert(box.space.T1 ~= nil)
+box.rollback_to_savepoint(save1)
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+box.space.T1 == nil and box.space.T2 == nil
+
+--
+-- Unexpected rollback.
+--
+
+box.begin() res = {monster_ddl()} require('fiber').yield()
+res
+box.commit()
+box.rollback()
+monster_ddl_clear()
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index 8846e5ee8..257dbafde 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -388,56 +388,6 @@ box.execute("DROP TABLE t3;")
---
- row_count: 1
...
--- gh-3780: space without PK raises error if
--- it is used in SQL queries.
---
-errinj = box.error.injection
----
-...
-fiber = require('fiber')
----
-...
-box.execute("CREATE TABLE t (id INT PRIMARY KEY);")
----
-- row_count: 1
-...
-box.execute("INSERT INTO t VALUES (1);")
----
-- row_count: 1
-...
-errinj.set("ERRINJ_WAL_DELAY", true)
----
-- ok
-...
--- DROP TABLE consists of several steps: firstly indexes
--- are deleted, then space itself. Lets make sure that if
--- first part of drop is successfully finished, but resulted
--- in yield, all operations on space will be blocked due to
--- absence of primary key.
---
-function drop_table_yield() box.execute("DROP TABLE t;") end
----
-...
-f = fiber.create(drop_table_yield)
----
-...
-box.execute("SELECT * FROM t;")
----
-- error: SQL does not support spaces without primary key
-...
-box.execute("INSERT INTO t VALUES (2);")
----
-- error: SQL does not support spaces without primary key
-...
-box.execute("UPDATE t SET id = 2;")
----
-- error: SQL does not support spaces without primary key
-...
--- Finish drop space.
-errinj.set("ERRINJ_WAL_DELAY", false)
----
-- ok
-...
--
-- gh-3931: Store regular identifiers in case-normal form
--
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 48b80a443..3bc1b684d 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -118,28 +118,6 @@ box.execute("INSERT INTO t3 VALUES(1, 1, 3);")
errinj.set("ERRINJ_WAL_IO", false)
box.execute("DROP TABLE t3;")
--- gh-3780: space without PK raises error if
--- it is used in SQL queries.
---
-errinj = box.error.injection
-fiber = require('fiber')
-box.execute("CREATE TABLE t (id INT PRIMARY KEY);")
-box.execute("INSERT INTO t VALUES (1);")
-errinj.set("ERRINJ_WAL_DELAY", true)
--- DROP TABLE consists of several steps: firstly indexes
--- are deleted, then space itself. Lets make sure that if
--- first part of drop is successfully finished, but resulted
--- in yield, all operations on space will be blocked due to
--- absence of primary key.
---
-function drop_table_yield() box.execute("DROP TABLE t;") end
-f = fiber.create(drop_table_yield)
-box.execute("SELECT * FROM t;")
-box.execute("INSERT INTO t VALUES (2);")
-box.execute("UPDATE t SET id = 2;")
--- Finish drop space.
-errinj.set("ERRINJ_WAL_DELAY", false)
-
--
-- gh-3931: Store regular identifiers in case-normal form
--
diff --git a/test/sql/view_delayed_wal.result b/test/sql/view_delayed_wal.result
index 519794931..3faaf07b8 100644
--- a/test/sql/view_delayed_wal.result
+++ b/test/sql/view_delayed_wal.result
@@ -13,8 +13,8 @@ fiber = require('fiber')
...
-- View reference counters are incremented before firing
-- on_commit triggers (i.e. before being written into WAL), so
--- it is impossible to create view on dropped (but not written
--- into WAL) space.
+-- it is impossible to drop a space referenced by a created, but
+-- no committed view.
--
box.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
---
@@ -49,13 +49,13 @@ box.error.injection.set("ERRINJ_WAL_DELAY", false)
fiber.sleep(0.1)
---
...
-box.space.T1
+box.space.T1 ~= nil
---
-- null
+- true
...
-box.space.V1
+box.space.V1 ~= nil
---
-- null
+- true
...
--
-- In the same way, we have to drop all referenced spaces before
diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
index 8e73b03f3..0a10d121b 100644
--- a/test/sql/view_delayed_wal.test.lua
+++ b/test/sql/view_delayed_wal.test.lua
@@ -5,8 +5,8 @@ fiber = require('fiber')
-- View reference counters are incremented before firing
-- on_commit triggers (i.e. before being written into WAL), so
--- it is impossible to create view on dropped (but not written
--- into WAL) space.
+-- it is impossible to drop a space referenced by a created, but
+-- no committed view.
--
box.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
function create_view() box.execute('CREATE VIEW v1 AS SELECT * FROM t1') end
@@ -18,8 +18,8 @@ f2 = fiber.create(drop_index_t1)
f3 = fiber.create(drop_space_t1)
box.error.injection.set("ERRINJ_WAL_DELAY", false)
fiber.sleep(0.1)
-box.space.T1
-box.space.V1
+box.space.T1 ~= nil
+box.space.V1 ~= nil
--
-- In the same way, we have to drop all referenced spaces before
--
2.20.1 (Apple Git-117)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-19 22:49 ` [tarantool-patches] [PATCH 2/2] sql: transactional DDL Vladislav Shpilevoy
@ 2019-07-20 7:42 ` Konstantin Osipov
2019-07-20 14:02 ` Vladislav Shpilevoy
2019-07-22 13:59 ` n.pettik
1 sibling, 1 reply; 12+ messages in thread
From: Konstantin Osipov @ 2019-07-20 7:42 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/07/20 01:52]:
> index 000000000..8f7f91151
> --- /dev/null
> +++ b/test/sql/ddl.result
> @@ -0,0 +1,356 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
Just curious why did you choose Lua test format and not SQL?
Now that SQL format is in place my personal preference would be is
that it is used for SQL tests whenever possible, to improve
readability (SQL syntax highlighting, autocompletion).
What do you miss in SQL tests format?
I personally only miss anonymous sql/psm blocks and loops.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-20 7:42 ` [tarantool-patches] " Konstantin Osipov
@ 2019-07-20 14:02 ` Vladislav Shpilevoy
2019-07-20 15:16 ` Konstantin Osipov
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-20 14:02 UTC (permalink / raw)
To: tarantool-patches, Konstantin Osipov; +Cc: korablev
On 20/07/2019 09:42, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/07/20 01:52]:
>> index 000000000..8f7f91151
>> --- /dev/null
>> +++ b/test/sql/ddl.result
>> @@ -0,0 +1,356 @@
>> +-- test-run result file version 2
>> +test_run = require('test_run').new()
>> + | ---
>> + | ...
>> +engine = test_run:get_cfg('engine')
>> + | ---
>
> Just curious why did you choose Lua test format and not SQL?
>
> Now that SQL format is in place my personal preference would be is
> that it is used for SQL tests whenever possible, to improve
> readability (SQL syntax highlighting, autocompletion).
>
> What do you miss in SQL tests format?
>
> I personally only miss anonymous sql/psm blocks and loops.
>
>
Firstly, Lua tests still are easier to copy paste into console
as is. Secondly, I just used to write Lua tests. Thirdly, I
don't like tap tests, because you need compare results
manually instead of just printing them.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-20 14:02 ` Vladislav Shpilevoy
@ 2019-07-20 15:16 ` Konstantin Osipov
0 siblings, 0 replies; 12+ messages in thread
From: Konstantin Osipov @ 2019-07-20 15:16 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, korablev
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/07/20 17:02]:
> Firstly, Lua tests still are easier to copy paste into console
> as is. Secondly, I just used to write Lua tests. Thirdly, I
> don't like tap tests, because you need compare results
> manually instead of just printing them.
SQL is not tap and there is SQL console you can use.
So it seems only a habit, inertia.
Lua + SQL tests are harder to maintain that plain SQL. If there is
no limitation of SLQ testsing system that you have hit, please use
SQL tests for all new SQL test files.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-19 22:49 ` [tarantool-patches] [PATCH 2/2] sql: transactional DDL Vladislav Shpilevoy
2019-07-20 7:42 ` [tarantool-patches] " Konstantin Osipov
@ 2019-07-22 13:59 ` n.pettik
2019-07-22 21:35 ` Vladislav Shpilevoy
1 sibling, 1 reply; 12+ messages in thread
From: n.pettik @ 2019-07-22 13:59 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
Previous patch seems to be OK to me.
> Box recently added support of transactional DDL allowing to do
> any number of non-yielding DDL operations atomically.
So, in SQL the only yielding operation (except for registered by
user custom function) is CREATE INDEX involving non-empty space.
Is this statement correct? If so, I guess we should add a few examples
demonstrating new functionality to our SQL manual.
> This is
> really a big relief of one of the biggest pains of SQL. Before
> this patch each multirow SQL DDL statement needed to prepare its
> own rollback procedure for a case if something would go wrong.
>
> Now with box support SQL wraps each DDL statement into a
> transaction, and doesn't need own escape-routes in a form of
> 'struct save_record' and others.
>
> Closes #4086
> ---
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 2aefa2a3f..4884a7855 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
>
> @@ -3331,10 +3215,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
> } else {
> sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
> P4_DYNAMIC);
> - if (is_clean_needed)
> - save_record(parser, 0, 0, 0, v->nOp - 1, false);
> - else
> - sqlVdbeAddOp1(v, OP_Halt, -1);
> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
ABORT is set by default, isn’t it? See similar questions below.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 6a4a303b9..a71b331f8 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4373,15 +4373,16 @@ case OP_Update: {
> */
> case OP_SInsert: {
> assert(pOp->p1 > 0);
> - assert(pOp->p2 > 0);
> - assert(pOp->p3 >= 0);
> + assert(pOp->p2 >= 0);
>
> - pIn3 = &aMem[pOp->p3];
> + pIn2 = &aMem[pOp->p2];
> struct space *space = space_by_id(pOp->p1);
> assert(space != NULL);
> assert(space_is_system(space));
> - if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
> - goto jump_to_p2;
> + if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) {
> + p->errorAction = ON_CONFLICT_ACTION_ABORT;
> + goto abort_due_to_error;
Why do we need set errorAction here? As I see, it is set
to _ABORT by default:
sqlVdbeRewind() -> sqlVdbeMakeReady() -> sql_finish_coding()
> @@ -4404,8 +4405,10 @@ case OP_SDelete: {
> struct space *space = space_by_id(pOp->p1);
> assert(space != NULL);
> assert(space_is_system(space));
> - if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0)
> + if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) {
> + p->errorAction = ON_CONFLICT_ACTION_ABORT;
Same question concerning errorAction.
> goto abort_due_to_error;
> + }
> if (pOp->p5 & OPFLAG_NCHANGE)
> p->nChange++;
> break;
> diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
> new file mode 100644
> index 000000000..477158796
> --- /dev/null
> +++ b/test/sql/ddl.test.lua
> @@ -0,0 +1,187 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +
> +--
> +-- gh-4086: SQL transactional DDL.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +box.begin()
> +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
> +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
> +box.commit();
> +test_run:cmd("setopt delimiter ''");
> +
> +box.space.T1 ~= nil
> +box.space.T1.index[0] ~= nil
> +box.space.T2 ~= nil
> +box.space.T2.index[0] ~= nil
> +
> +test_run:cmd("setopt delimiter ';'")
> +box.begin()
> +box.execute('DROP TABLE t1;')
> +assert(box.space.T1 == nil)
> +assert(box.space.T2 ~= nil)
> +box.execute('DROP TABLE t2;')
> +assert(box.space.T2 == nil)
> +box.commit();
> +test_run:cmd("setopt delimiter ''");
> +
> +--
> +-- Use all the possible SQL DDL statements.
> +--
> +test_run:cmd("setopt delimiter '$'")
> +function monster_ddl()
Still you have missed ALTER RENAME statement :)
Also, AFAIR TRUNCATE is also considered to be DDL
operation. Anyway, great tests, I like them. Personally,
I’d write them in Lua format as well. IMHO *.sql.test would
look a bit weird taking into account number of involved Lua
functions.
> + local _, err1, err2, err3
> + box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
> + a INTEGER,
> + b INTEGER);]])
> + box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
> + a INTEGER,
> + b INTEGER UNIQUE,
> + CONSTRAINT ck1 CHECK(b < 100));]])
> +
> + box.execute('CREATE INDEX t1a ON t1(a);')
> + box.execute('CREATE INDEX t2a ON t2(a);')
> + box.execute('DROP INDEX t2a ON t2;')
> +
> + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
> + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
> + box.space.T1.ck_constraint.CK1:drop()
> +
> + box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
> + (a) REFERENCES t2(b);]])
> + box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
> +
> +-- Try random errors inside this big batch of DDL to ensure, that
> +-- they do not affect normal operation.
Nit: a bit broken comment's indentation..?
> diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
> index 8e73b03f3..0a10d121b 100644
> --- a/test/sql/view_delayed_wal.test.lua
> +++ b/test/sql/view_delayed_wal.test.lua
> @@ -5,8 +5,8 @@ fiber = require('fiber')
>
> -- View reference counters are incremented before firing
> -- on_commit triggers (i.e. before being written into WAL), so
> --- it is impossible to create view on dropped (but not written
> --- into WAL) space.
> +-- it is impossible to drop a space referenced by a created, but
> +-- no committed view.
Nit: I guess ’*still* not yet committed view’ would sound better.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-22 13:59 ` n.pettik
@ 2019-07-22 21:35 ` Vladislav Shpilevoy
2019-07-24 13:23 ` n.pettik
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-22 21:35 UTC (permalink / raw)
To: n.pettik, tarantool-patches
Hi! Thanks for the comments.
On 22/07/2019 15:59, n.pettik wrote:
> Previous patch seems to be OK to me.
>
>> Box recently added support of transactional DDL allowing to do
>> any number of non-yielding DDL operations atomically.
>
> So, in SQL the only yielding operation (except for registered by
> user custom function) is CREATE INDEX involving non-empty space.
> Is this statement correct? If so, I guess we should add a few examples
> demonstrating new functionality to our SQL manual.
Yes, CREATE INDEX should be the only one. I forgot to test it, and here
is diff. Note, that I omitted *.test.lua file fixes, because otherwise
the email gets too big.
================================================================================
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
index 8f7f91151..f96f7de9e 100644
--- a/test/sql/ddl.result
+++ b/test/sql/ddl.result
@@ -63,6 +63,93 @@ test_run:cmd("setopt delimiter ''");
| - true
| ...
+--
+-- Try to build an index transactionally.
+--
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER)')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T1:replace{1, 1, 1}
+ | ---
+ | - [1, 1, 1]
+ | ...
+box.space.T1:replace{2, 2, 2}
+ | ---
+ | - [2, 2, 2]
+ | ...
+box.space.T1:replace{3, 3, 3}
+ | ---
+ | - [3, 3, 3]
+ | ...
+box.space.T1:replace{4, 4, 4}
+ | ---
+ | - [4, 4, 4]
+ | ...
+box.space.T1:replace{5, 5, 5}
+ | ---
+ | - [5, 5, 5]
+ | ...
+-- Snapshot to dump Vinyl memory level, and force reading from a
+-- disk, if someday create index will support transactions.
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)')
+box.execute('CREATE INDEX t1a ON t1(a)')
+box.execute('CREATE INDEX t1b ON t1(b)')
+box.commit();
+ | ---
+ | - error: Can not perform index build in a multi-statement transaction
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+box.rollback()
+ | ---
+ | ...
+
+--
+-- Index drop does not yield, and is being done in background
+-- later. So it is transactional.
+--
+box.execute('CREATE INDEX t1a ON t1(a)')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('CREATE INDEX t1b ON t1(b)')
+ | ---
+ | - row_count: 1
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)')
+box.execute('DROP INDEX t1a ON t1')
+box.execute('DROP INDEX t1b ON t1')
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+box.execute('DROP TABLE t1')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP TABLE t2')
+ | ---
+ | - row_count: 1
+ | ...
+
--
-- Use all the possible SQL DDL statements.
--
================================================================================
I also added a couple of tests on SQL transactions:
================================================================================
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
index f96f7de9e..73abc0c77 100644
--- a/test/sql/ddl.result
+++ b/test/sql/ddl.result
@@ -361,6 +361,45 @@ box.begin() monster_ddl_clear() box.commit()
| ---
| ...
+-- Try SQL transactions.
+box.execute('START TRANSACTION') res = {monster_ddl()} box.execute('COMMIT')
+ | ---
+ | ...
+res
+ | ---
+ | - - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+monster_ddl_check()
+ | ---
+ | - 'Finished ok, errors and trigger catcher content: '
+ | - 'Check constraint failed ''CK1'': b < 100'
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | - 'Check constraint failed ''CK2'': a > 0'
+ | - metadata:
+ | - name: ID
+ | type: integer
+ | rows:
+ | - [1]
+ | ...
+box.execute('START TRANSACTION') monster_ddl_clear() box.execute('COMMIT')
+ | ---
+ | ...
+
+box.execute('START TRANSACTION') res = {monster_ddl()} box.execute('ROLLBACK')
+ | ---
+ | ...
+res
+ | ---
+ | - - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | ...
+
--
-- Voluntary rollback.
--
================================================================================
Talking of documentation - I added a docbot request. See at the end of the
email for the whole patch and the commit message.
>
>> This is
>> really a big relief of one of the biggest pains of SQL. Before
>> this patch each multirow SQL DDL statement needed to prepare its
>> own rollback procedure for a case if something would go wrong.
>>
>> Now with box support SQL wraps each DDL statement into a
>> transaction, and doesn't need own escape-routes in a form of
>> 'struct save_record' and others.
>>
>> Closes #4086
>> ---
>>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 2aefa2a3f..4884a7855 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>>
>> @@ -3331,10 +3215,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
>> } else {
>> sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
>> P4_DYNAMIC);
>> - if (is_clean_needed)
>> - save_record(parser, 0, 0, 0, v->nOp - 1, false);
>> - else
>> - sqlVdbeAddOp1(v, OP_Halt, -1);
>> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
>
> ABORT is set by default, isn’t it? See similar questions below.
In this place it was not set. OP_Halt takes an error action from p2, which
was 0 here. 0 means rollback of the whole transaction, but we need a
rollback of the current statement only - this is ON_CONFLICT_ACTION_ABORT.
>
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 6a4a303b9..a71b331f8 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -4373,15 +4373,16 @@ case OP_Update: {
>> */
>> case OP_SInsert: {
>> assert(pOp->p1 > 0);
>> - assert(pOp->p2 > 0);
>> - assert(pOp->p3 >= 0);
>> + assert(pOp->p2 >= 0);
>>
>> - pIn3 = &aMem[pOp->p3];
>> + pIn2 = &aMem[pOp->p2];
>> struct space *space = space_by_id(pOp->p1);
>> assert(space != NULL);
>> assert(space_is_system(space));
>> - if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
>> - goto jump_to_p2;
>> + if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) {
>> + p->errorAction = ON_CONFLICT_ACTION_ABORT;
>> + goto abort_due_to_error;
>
> Why do we need set errorAction here? As I see, it is set
> to _ABORT by default:
>
> sqlVdbeRewind() -> sqlVdbeMakeReady() -> sql_finish_coding()
>
Yes, here you are right. These gotos bypass OP_Halt, and terminate
the execution directly, so p2 here does not matter, and p->errorAction
is already set correctly. Here is diff:
================================================================================
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a71b331f8..e63d50382 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4379,10 +4379,9 @@ case OP_SInsert: {
struct space *space = space_by_id(pOp->p1);
assert(space != NULL);
assert(space_is_system(space));
- if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) {
- p->errorAction = ON_CONFLICT_ACTION_ABORT;
+ assert(p->errorAction == ON_CONFLICT_ACTION_ABORT);
+ if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0)
goto abort_due_to_error;
- }
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
break;
@@ -4405,10 +4404,9 @@ case OP_SDelete: {
struct space *space = space_by_id(pOp->p1);
assert(space != NULL);
assert(space_is_system(space));
- if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) {
- p->errorAction = ON_CONFLICT_ACTION_ABORT;
+ assert(p->errorAction == ON_CONFLICT_ACTION_ABORT);
+ if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0)
goto abort_due_to_error;
- }
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
break;
================================================================================
>
>> @@ -4404,8 +4405,10 @@ case OP_SDelete: {
>> struct space *space = space_by_id(pOp->p1);
>> assert(space != NULL);
>> assert(space_is_system(space));
>> - if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0)
>> + if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) {
>> + p->errorAction = ON_CONFLICT_ACTION_ABORT;
>
> Same question concerning errorAction.
The same, you are right. See diff and the explanation above.
>
>> goto abort_due_to_error;
>> + }
>> if (pOp->p5 & OPFLAG_NCHANGE)
>> p->nChange++;
>> break;
>> diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
>> new file mode 100644
>> index 000000000..477158796
>> --- /dev/null
>> +++ b/test/sql/ddl.test.lua
>> @@ -0,0 +1,187 @@
>> +test_run = require('test_run').new()
>> +engine = test_run:get_cfg('engine')
>> +box.execute('pragma sql_default_engine=\''..engine..'\'')
>> +
>> +--
>> +-- gh-4086: SQL transactional DDL.
>> +--
>> +test_run:cmd("setopt delimiter ';'")
>> +box.begin()
>> +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
>> +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
>> +box.commit();
>> +test_run:cmd("setopt delimiter ''");
>> +
>> +box.space.T1 ~= nil
>> +box.space.T1.index[0] ~= nil
>> +box.space.T2 ~= nil
>> +box.space.T2.index[0] ~= nil
>> +
>> +test_run:cmd("setopt delimiter ';'")
>> +box.begin()
>> +box.execute('DROP TABLE t1;')
>> +assert(box.space.T1 == nil)
>> +assert(box.space.T2 ~= nil)
>> +box.execute('DROP TABLE t2;')
>> +assert(box.space.T2 == nil)
>> +box.commit();
>> +test_run:cmd("setopt delimiter ''");
>> +
>> +--
>> +-- Use all the possible SQL DDL statements.
>> +--
>> +test_run:cmd("setopt delimiter '$'")
>> +function monster_ddl()
>
> Still you have missed ALTER RENAME statement :)
Indeed. Here it is:
================================================================================
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
index 73abc0c77..f233470f2 100644
--- a/test/sql/ddl.result
+++ b/test/sql/ddl.result
@@ -158,7 +158,9 @@ test_run:cmd("setopt delimiter '$'")
| - true
| ...
function monster_ddl()
- local _, err1, err2, err3
+-- Try random errors inside this big batch of DDL to ensure, that
+-- they do not affect normal operation.
+ local _, err1, err2, err3, err4
box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
a INTEGER,
b INTEGER);]])
@@ -169,9 +171,17 @@ function monster_ddl()
box.execute('CREATE INDEX t1a ON t1(a);')
box.execute('CREATE INDEX t2a ON t2(a);')
+
+ box.execute('CREATE TABLE t_to_rename(id INTEGER PRIMARY KEY, a INTEGER);')
+
box.execute('DROP INDEX t2a ON t2;')
+ box.execute('CREATE INDEX t_to_rename_a ON t_to_rename(a);')
+
box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
+
+ _, err1 = pcall(box.execute, 'ALTER TABLE t_to_rename RENAME TO t1;')
+
box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
box.space.T1.ck_constraint.CK1:drop()
@@ -179,9 +189,7 @@ function monster_ddl()
(a) REFERENCES t2(b);]])
box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
--- Try random errors inside this big batch of DDL to ensure, that
--- they do not affect normal operation.
- _, err1 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+ _, err2 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);')
box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
(a) REFERENCES t2(b);]])
@@ -189,23 +197,27 @@ function monster_ddl()
box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY
KEY AUTOINCREMENT);]])
+ box.execute('ALTER TABLE t_to_rename RENAME TO t_renamed;')
+
+ box.execute('DROP INDEX t_to_rename_a ON t_renamed;')
+
box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
BEGIN
INSERT INTO trigger_catcher VALUES(1);
END; ]])
- _, err2 = pcall(box.execute, 'DROP TABLE t3;')
+ _, err3 = pcall(box.execute, 'DROP TABLE t3;')
box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW
BEGIN
INSERT INTO trigger_catcher VALUES(1);
END; ]])
- _, err3 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
+ _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
box.execute('DROP TRIGGER t2t;')
- return 'Finished ok, errors in the middle: ', err1, err2, err3
+ return 'Finished ok, errors in the middle: ', err1, err2, err3, err4
end$
| ---
| ...
@@ -215,6 +227,8 @@ function monster_ddl_is_clean()
assert(box.space._trigger:count() == 0)
assert(box.space._fk_constraint:count() == 0)
assert(box.space._ck_constraint:count() == 0)
+ assert(box.space.T_RENAMED == nil)
+ assert(box.space.T_TO_RENAME == nil)
end$
| ---
| ...
@@ -227,6 +241,9 @@ function monster_ddl_check()
_, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)')
box.execute('INSERT INTO t1 VALUES (1, 1, 1)')
res = box.execute('SELECT * FROM trigger_catcher')
+ assert(box.space.T_RENAMED ~= nil)
+ assert(box.space.T_RENAMED.index.T_TO_RENAME_A == nil)
+ assert(box.space.T_TO_RENAME == nil)
return 'Finished ok, errors and trigger catcher content: ', err1, err2,
err3, err4, res
end$
@@ -238,6 +255,7 @@ function monster_ddl_clear()
pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;')
box.execute('DROP TABLE IF EXISTS t2')
box.execute('DROP TABLE IF EXISTS t1')
+ box.execute('DROP TABLE IF EXISTS t_renamed')
monster_ddl_is_clean()
end$
| ---
@@ -252,6 +270,7 @@ monster_ddl()
| ---
| - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
@@ -283,6 +302,7 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
@@ -295,6 +315,7 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
@@ -320,6 +341,7 @@ monster_ddl()
| ---
| - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
@@ -369,6 +391,7 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
@@ -396,6 +419,7 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
@@ -467,6 +491,7 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
+ | - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
| ...
================================================================================
> Also, AFAIR TRUNCATE is also considered to be DDL
> operation.
Fair, added:
================================================================================
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
index 73abc0c77..f1f72e9d5 100644
--- a/test/sql/ddl.result
+++ b/test/sql/ddl.result
@@ -141,6 +141,47 @@ test_run:cmd("setopt delimiter ''");
| ---
| ...
+--
+-- Truncate should not be different from index drop in terms of
+-- yields and atomicity.
+--
+box.space.T2:replace{1}
+ | ---
+ | - [1]
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function truncate_both()
+ box.execute('TRUNCATE TABLE t1;')
+ box.execute('TRUNCATE TABLE t2;')
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.begin() truncate_both() box.rollback()
+ | ---
+ | ...
+
+box.space.T1:count() > 0 and box.space.T2:count() > 0
+ | ---
+ | - true
+ | ...
+
+box.begin() truncate_both() box.commit()
+ | ---
+ | ...
+
+box.space.T1:count() == 0 and box.space.T2:count() == 0
+ | ---
+ | - true
+ | ...
+
box.execute('DROP TABLE t1')
| ---
| - row_count: 1
@@ -158,7 +199,9 @@ test_run:cmd("setopt delimiter '$'")
| - true
| ...
function monster_ddl()
-- Try random errors inside this big batch of DDL to ensure, that
-- they do not affect normal operation.
- local _, err1, err2, err3, err4
+ local _, err1, err2, err3, err4, err5, err6
box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
a INTEGER,
b INTEGER);]])
@@ -189,23 +238,32 @@ function monster_ddl()
_, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
+
+ box.execute('TRUNCATE TABLE t1;')
+ _, err5 = pcall(box.execute, 'TRUNCATE TABLE t2;')
+ _, err6 = pcall(box.execute, 'TRUNCATE TABLE t_does_not_exist;')
box.execute('DROP TRIGGER t2t;')
- return 'Finished ok, errors in the middle: ', err1, err2, err3, err4
+ return 'Finished ok, errors in the middle: ', err1, err2, err3, err4,
+ err5, err6
end$
| ---
| ...
@@ -252,8 +316,12 @@ monster_ddl()
| ---
| - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other objects
+ | depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
monster_ddl_check()
| ---
@@ -283,8 +351,12 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other
+ | objects depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
-- DDL in txn, cleanup is not.
@@ -295,8 +367,12 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other
+ | objects depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
monster_ddl_check()
| ---
@@ -320,8 +396,12 @@ monster_ddl()
| ---
| - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other objects
+ | depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
monster_ddl_check()
| ---
@@ -369,8 +449,12 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other
+ | objects depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
monster_ddl_check()
| ---
@@ -396,8 +480,12 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other
+ | objects depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
--
@@ -467,8 +555,12 @@ res
| ---
| - - 'Finished ok, errors in the middle: '
| - Space 'T1' already exists
| - Space 'T1' already exists
| - Space 'T3' does not exist
| - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other
+ | objects depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
| ...
box.commit()
| ---
================================================================================
> Anyway, great tests, I like them. Personally,
> I’d write them in Lua format as well. IMHO *.sql.test would
> look a bit weird taking into account number of involved Lua
> functions.
>
>> + local _, err1, err2, err3
>> + box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
>> + a INTEGER,
>> + b INTEGER);]])
>> + box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
>> + a INTEGER,
>> + b INTEGER UNIQUE,
>> + CONSTRAINT ck1 CHECK(b < 100));]])
>> +
>> + box.execute('CREATE INDEX t1a ON t1(a);')
>> + box.execute('CREATE INDEX t2a ON t2(a);')
>> + box.execute('DROP INDEX t2a ON t2;')
>> +
>> + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
>> + box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
>> + box.space.T1.ck_constraint.CK1:drop()
>> +
>> + box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
>> + (a) REFERENCES t2(b);]])
>> + box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
>> +
>> +-- Try random errors inside this big batch of DDL to ensure, that
>> +-- they do not affect normal operation.
>
> Nit: a bit broken comment's indentation..?
No, it is a problem with test-run (perhaps, or probably with the
console - don't know). If you set a delimiter, then comments can't
have indentation. Otherwise you will get weird errors.
>
>> diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
>> index 8e73b03f3..0a10d121b 100644
>> --- a/test/sql/view_delayed_wal.test.lua
>> +++ b/test/sql/view_delayed_wal.test.lua
>> @@ -5,8 +5,8 @@ fiber = require('fiber')
>>
>> -- View reference counters are incremented before firing
>> -- on_commit triggers (i.e. before being written into WAL), so
>> --- it is impossible to create view on dropped (but not written
>> --- into WAL) space.
>> +-- it is impossible to drop a space referenced by a created, but
>> +-- no committed view.
>
> Nit: I guess ’*still* not yet committed view’ would sound better.
>
Perhaps.
================================================================================
diff --git a/test/sql/view_delayed_wal.result b/test/sql/view_delayed_wal.result
index 3faaf07b8..d518e7d8c 100644
--- a/test/sql/view_delayed_wal.result
+++ b/test/sql/view_delayed_wal.result
@@ -14,7 +14,7 @@ fiber = require('fiber')
-- View reference counters are incremented before firing
-- on_commit triggers (i.e. before being written into WAL), so
-- it is impossible to drop a space referenced by a created, but
--- no committed view.
+-- *still* not yet committed view.
--
box.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
---
================================================================================
Additionally, after the fixes there are too many repeating results
of monster_ddl() and monster_ddl_check() calls. I moved their comparison
into a function monster_ddl_cmp_res(). See the new patch:
================================================================================
sql: transactional DDL
Box recently added support of transactional DDL allowing to do
any number of non-yielding DDL operations atomically. This is
really a big relief of one of the biggest pains of SQL. Before
this patch each multirow SQL DDL statement needed to prepare its
own rollback procedure for a case if something would go wrong.
Now with box support SQL wraps each DDL statement into a
transaction, and doesn't need own escape-routes in a form of
'struct save_record' and others.
Closes #4086
@TarantoolBot document
Title: SQL DDL is transactional
SQL DDL operations are atomic now. For example, if a CREATE TABLE
request fails somewhere in the middle, it won't leave any
garbage. Like a space without indexes, or unused sequences. Even
if the instance is powered off during the request.
Also, SQL DDL can be manually included into transactions, with
certain limitations - such a transaction can't yield.
For example, this is legal:
START TRANSACTION;
CREATE TABLE test(a INTEGER PRIMARY KEY, b INTEGER);
CREATE INDEX test_a ON test(a);
COMMIT;
If you want to test it in the console, then wrap it into a
function to do not get a rollback by yield, because the console
yields after each command:
function create()
box.execute('START TRANSACTION;')
box.execute('CREATE TABLE test(a INTEGER PRIMARY KEY, b INTEGER);')
box.execute('CREATE INDEX test_a ON test(a);')
box.execute('COMMIT;')
end
create()
But the following example is illegal and you will get an error:
box.execute('CREATE TABLE test(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);')
box.execute('INSERT INTO test VALUES (1, 1, 1), (2, 2, 2), (3, 3, 3);')
function index()
box.execute('START TRANSACTION;')
box.execute('CREATE INDEX test_b ON test(b);')
box.execute('CREATE INDEX test_c ON test(c);')
box.execute('COMMIT;')
end
tarantool> index()
---
- error: Can not perform index build in a multi-statement transaction
...
The error is because an attempt to build an index on a non-empty
space leads to immediate yield.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 2aefa2a3f..4884a7855 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -57,64 +57,6 @@
#include "box/tuple_format.h"
#include "box/coll_id_cache.h"
-/**
- * Structure that contains information about record that was
- * inserted into system space.
- */
-struct saved_record
-{
- /** A link in a record list. */
- struct rlist link;
- /** Id of space in which the record was inserted. */
- uint32_t space_id;
- /** First register of the key of the record. */
- int reg_key;
- /** Number of registers the key consists of. */
- int reg_key_count;
- /** The address of the opcode. */
- int op_addr;
- /** Flag to show that operation is SInsert. */
- bool is_insert;
-};
-
-/**
- * Save inserted in system space record in list. This procedure is
- * called after generation of either OP_SInsert or OP_NoColflict +
- * OP_SetDiag. In the first case, record inserted to the system
- * space is supposed to be deleted on error; in the latter - jump
- * target specified in OP_SetDiag should be adjusted to the start
- * of clean-up routines (current entry isn't inserted to the space
- * yet, so there's no need to delete it).
- *
- * @param parser SQL Parser object.
- * @param space_id Id of table in which record is inserted.
- * @param reg_key Register that contains first field of the key.
- * @param reg_key_count Exact number of fields of the key.
- * @param op_addr Address of opcode (OP_SetDiag or OP_SInsert).
- * Used to fix jump target (see
- * sql_finish_coding()).
- * @param is_insert_op Whether opcode is OP_SInsert or not.
- */
-static inline void
-save_record(struct Parse *parser, uint32_t space_id, int reg_key,
- int reg_key_count, int op_addr, bool is_insert_op)
-{
- struct saved_record *record =
- region_alloc(&parser->region, sizeof(*record));
- if (record == NULL) {
- diag_set(OutOfMemory, sizeof(*record), "region_alloc",
- "record");
- parser->is_aborted = true;
- return;
- }
- record->space_id = space_id;
- record->reg_key = reg_key;
- record->reg_key_count = reg_key_count;
- record->op_addr = op_addr;
- record->is_insert = is_insert_op;
- rlist_add_entry(&parser->record_list, record, link);
-}
-
void
sql_finish_coding(struct Parse *parse_context)
{
@@ -122,52 +64,6 @@ sql_finish_coding(struct Parse *parse_context)
struct sql *db = parse_context->db;
struct Vdbe *v = sqlGetVdbe(parse_context);
sqlVdbeAddOp0(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. Hence for processed insertions we should remove
- * entries from corresponding system spaces alongside
- * with fixing jump address for OP_SInsert opcode in
- * case it fails during VDBE runtime; for OP_SetDiag only
- * adjust jump target to the start of clean-up program
- * for already inserted entries.
- */
- if (!rlist_empty(&parse_context->record_list)) {
- struct saved_record *record =
- rlist_shift_entry(&parse_context->record_list,
- struct saved_record, link);
- /*
- * Set jump target for OP_SetDiag and OP_SInsert.
- */
- sqlVdbeChangeP2(v, record->op_addr, v->nOp);
- MAYBE_UNUSED const char *comment =
- "Delete entry from %s if CREATE TABLE fails";
- rlist_foreach_entry(record, &parse_context->record_list, link) {
- if (record->is_insert) {
- int rec_reg = ++parse_context->nMem;
- sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key,
- record->reg_key_count, rec_reg);
- sqlVdbeAddOp2(v, OP_SDelete, record->space_id,
- rec_reg);
- MAYBE_UNUSED struct space *space =
- space_by_id(record->space_id);
- VdbeComment((v, comment, space_name(space)));
- }
- /*
- * Set jump target for OP_SetDiag and
- * OP_SInsert.
- */
- sqlVdbeChangeP2(v, record->op_addr, v->nOp);
- }
- sqlVdbeAddOp1(v, OP_Halt, -1);
- VdbeComment((v,
- "Exit with an error if CREATE statement fails"));
- }
if (db->mallocFailed)
parse_context->is_aborted = true;
@@ -933,8 +829,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
sqlVdbeAddOp4(v, OP_Blob, index_parts_sz, entry_reg + 5,
SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
sqlVdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg);
- sqlVdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);
- save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1, true);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, tuple_reg);
return;
error:
parse->is_aborted = true;
@@ -991,9 +886,8 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6,
SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC);
sqlVdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, tuple_reg);
- sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, tuple_reg);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, tuple_reg);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
- save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1, true);
return;
error:
pParse->is_aborted = true;
@@ -1102,8 +996,7 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
* Occupy registers for 5 fields: each member in
* _ck_constraint space plus one for final msgpack tuple.
*/
- int ck_constraint_reg = parser->nMem + 1;
- parser->nMem += 6;
+ int ck_constraint_reg = sqlGetTempRange(parser, 6);
sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg);
sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 1, 0,
sqlDbStrDup(db, ck_def->name), P4_DYNAMIC);
@@ -1120,13 +1013,12 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
ck_constraint_reg, 2,
ER_CONSTRAINT_EXISTS, error_msg,
- false, OP_NoConflict, true) != 0)
+ false, OP_NoConflict) != 0)
return;
- sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0,
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
ck_constraint_reg + 5);
- save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2,
- v->nOp - 1, true);
VdbeComment((v, "Create CK constraint %s", ck_def->name));
+ sqlReleaseTempRange(parser, ck_constraint_reg, 6);
}
/**
@@ -1148,8 +1040,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
* Occupy registers for 9 fields: each member in
* _fk_constraint space plus one for final msgpack tuple.
*/
- int constr_tuple_reg = parse_context->nMem + 1;
- parse_context->nMem += 10;
+ int constr_tuple_reg = sqlGetTempRange(parse_context, 10);
char *name_copy = sqlDbStrDup(parse_context->db, fk->name);
if (name_copy == NULL)
return;
@@ -1185,7 +1076,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
BOX_FK_CONSTRAINT_ID, 0,
constr_tuple_reg, 2,
ER_CONSTRAINT_EXISTS, error_msg,
- false, OP_NoConflict, true) != 0)
+ false, OP_NoConflict) != 0)
return;
sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
@@ -1228,14 +1119,13 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
parent_links, P4_DYNAMIC);
sqlVdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9,
constr_tuple_reg + 9);
- sqlVdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
- constr_tuple_reg + 9);
+ sqlVdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
+ constr_tuple_reg + 9);
if (parse_context->create_table_def.new_space == NULL) {
sqlVdbeCountChanges(vdbe);
sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
}
- save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
- vdbe->nOp - 1, true);
+ sqlReleaseTempRange(parse_context, constr_tuple_reg, 10);
return;
error:
parse_context->is_aborted = true;
@@ -1331,7 +1221,7 @@ sqlEndTable(struct Parse *pParse)
if (vdbe_emit_halt_with_presence_test(pParse, BOX_SPACE_ID, 2,
name_reg, 1, ER_SPACE_EXISTS,
error_msg, (no_err != 0),
- OP_NoConflict, false) != 0)
+ OP_NoConflict) != 0)
return;
int reg_space_id = getNewSpaceId(pParse);
@@ -1354,18 +1244,13 @@ sqlEndTable(struct Parse *pParse)
int reg_seq_record =
emitNewSysSequenceRecord(pParse, reg_seq_id,
new_space->def->name);
- sqlVdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,
- reg_seq_record);
- save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
- v->nOp - 1, true);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
/* Do an insertion into _space_sequence. */
int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
reg_space_id, reg_seq_id,
new_space->index[0]->def);
- sqlVdbeAddOp3(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, true);
+ sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
+ reg_space_seq_record);
}
/* Code creation of FK constraints, if any. */
struct fk_constraint_parse *fk_parse;
@@ -1492,7 +1377,7 @@ sql_create_view(struct Parse *parse_context)
if (vdbe_emit_halt_with_presence_test(parse_context, BOX_SPACE_ID, 2,
name_reg, 1, ER_SPACE_EXISTS,
error_msg, (no_err != 0),
- OP_NoConflict, false) != 0)
+ OP_NoConflict) != 0)
goto create_view_fail;
vdbe_emit_space_create(parse_context, getNewSpaceId(parse_context),
@@ -1611,7 +1496,7 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
BOX_FK_CONSTRAINT_ID, 0,
key_reg, 2, ER_NO_SUCH_CONSTRAINT,
error_msg, false,
- OP_Found, false) != 0) {
+ OP_Found) != 0) {
sqlDbFree(parse_context->db, constraint_name);
return;
}
@@ -1643,8 +1528,7 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false,
- OP_Found, false) != 0)
+ error_msg, false, OP_Found) != 0)
return;
sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
@@ -3310,7 +3194,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int index_id, int key_reg, uint32_t key_len,
int tarantool_error_code,
const char *error_src, bool no_error,
- int cond_opcode, bool is_clean_needed)
+ int cond_opcode)
{
assert(cond_opcode == OP_NoConflict || cond_opcode == OP_Found);
struct Vdbe *v = sqlGetVdbe(parser);
@@ -3331,10 +3215,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
} else {
sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
P4_DYNAMIC);
- if (is_clean_needed)
- save_record(parser, 0, 0, 0, v->nOp - 1, false);
- else
- sqlVdbeAddOp1(v, OP_Halt, -1);
+ sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
}
sqlVdbeJumpHere(v, addr);
sqlVdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 2a60ad25b..06eab7f2d 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -172,6 +172,7 @@ cmd ::= create_table create_table_args.
create_table ::= CREATE TABLE ifnotexists(E) nm(Y). {
create_table_def_init(&pParse->create_table_def, &Y, E);
pParse->create_table_def.new_space = sqlStartTable(pParse, &Y);
+ pParse->initiateTTrans = true;
}
%type ifnotexists {int}
@@ -380,12 +381,14 @@ resolvetype(A) ::= REPLACE. {A = ON_CONFLICT_ACTION_REPLACE;}
cmd ::= DROP TABLE ifexists(E) fullname(X) . {
struct Token t = Token_nil;
drop_table_def_init(&pParse->drop_table_def, X, &t, E);
+ pParse->initiateTTrans = true;
sql_drop_table(pParse);
}
cmd ::= DROP VIEW ifexists(E) fullname(X) . {
struct Token t = Token_nil;
drop_view_def_init(&pParse->drop_view_def, X, &t, E);
+ pParse->initiateTTrans = true;
sql_drop_table(pParse);
}
@@ -399,6 +402,7 @@ cmd ::= CREATE(X) VIEW ifnotexists(E) nm(Y) eidlist_opt(C)
AS select(S). {
if (!pParse->parse_only) {
create_view_def_init(&pParse->create_view_def, &Y, &X, C, S, E);
+ pParse->initiateTTrans = true;
sql_create_view(pParse);
} else {
sql_store_select(pParse, S);
@@ -1404,6 +1408,7 @@ cmd ::= CREATE uniqueflag(U) INDEX ifnotexists(NE) nm(X)
}
create_index_def_init(&pParse->create_index_def, src_list, &X, Z, U,
SORT_ORDER_ASC, NE);
+ pParse->initiateTTrans = true;
sql_create_index(pParse);
}
@@ -1456,6 +1461,7 @@ eidlist(A) ::= nm(Y). {
//
cmd ::= DROP INDEX ifexists(E) nm(X) ON fullname(Y). {
drop_index_def_init(&pParse->drop_index_def, Y, &X, E);
+ pParse->initiateTTrans = true;
sql_drop_index(pParse);
}
@@ -1502,7 +1508,7 @@ cmd ::= CREATE trigger_decl(A) BEGIN trigger_cmd_list(S) END(Z). {
Token all;
all.z = A.z;
all.n = (int)(Z.z - A.z) + Z.n;
- pParse->initiateTTrans = false;
+ pParse->initiateTTrans = true;
sql_trigger_finish(pParse, S, &all);
}
@@ -1652,6 +1658,7 @@ raisetype(A) ::= FAIL. {A = ON_CONFLICT_ACTION_FAIL;}
cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
struct Token t = Token_nil;
drop_trigger_def_init(&pParse->drop_trigger_def, X, &t, NOERR);
+ pParse->initiateTTrans = true;
sql_drop_trigger(pParse);
}
@@ -1671,6 +1678,7 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
A.table_name = T;
A.name = N;
+ pParse->initiateTTrans = true;
}
cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
@@ -1697,11 +1705,13 @@ unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; }
cmd ::= alter_table_start(A) RENAME TO nm(N). {
rename_entity_def_init(&pParse->rename_entity_def, A, &N);
+ pParse->initiateTTrans = true;
sql_alter_table_rename(pParse);
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ pParse->initiateTTrans = true;
sql_drop_foreign_key(pParse);
}
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 84fb31bcd..36c21a221 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -243,7 +243,6 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
memset(parser, 0, sizeof(struct Parse));
parser->db = db;
parser->sql_flags = sql_flags;
- rlist_create(&parser->record_list);
region_create(&parser->region, &cord()->slabc);
}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4f5bad287..99296b4b3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2329,11 +2329,6 @@ struct Parse {
* sqlEndTable() function).
*/
struct create_table_def create_table_def;
- /**
- * List of all records that were inserted in system spaces
- * in current statement.
- */
- struct rlist record_list;
bool initiateTTrans; /* Initiate Tarantool transaction */
/** If set - do not emit byte code at all, just parse. */
bool parse_only;
@@ -4542,7 +4537,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int index_id, int key_reg, uint32_t key_len,
int tarantool_error_code,
const char *error_src, bool no_error,
- int cond_opcode, bool is_clean_needed);
+ int cond_opcode);
/**
* Generate VDBE code to delete records from system _sql_stat1 or
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 562723959..d746ef893 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -112,8 +112,7 @@ sql_trigger_begin(struct Parse *parse)
name_reg, 1,
ER_TRIGGER_EXISTS,
error_msg, (no_err != 0),
- OP_NoConflict,
- false) != 0)
+ OP_NoConflict) != 0)
goto trigger_cleanup;
}
@@ -421,8 +420,7 @@ sql_drop_trigger(struct Parse *parser)
sqlVdbeAddOp4(v, OP_String8, 0, name_reg, 0, name_copy, P4_DYNAMIC);
if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0,
name_reg, 1, ER_NO_SUCH_TRIGGER,
- error_msg, no_err, OP_Found,
- false) != 0)
+ error_msg, no_err, OP_Found) != 0)
goto drop_trigger_cleanup;
vdbe_code_drop_trigger(parser, trigger_name, true);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6a4a303b9..e63d50382 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4359,8 +4359,8 @@ case OP_Update: {
break;
}
-/* Opcode: SInsert P1 P2 P3 * P5
- * Synopsis: space id = P1, key = r[P3], on error goto P2
+/* Opcode: SInsert P1 P2 * * P5
+ * Synopsis: space id = P1, key = r[P2]
*
* This opcode is used only during DDL routine.
* In contrast to ordinary insertion, insertion to system spaces
@@ -4373,15 +4373,15 @@ case OP_Update: {
*/
case OP_SInsert: {
assert(pOp->p1 > 0);
- assert(pOp->p2 > 0);
- assert(pOp->p3 >= 0);
+ assert(pOp->p2 >= 0);
- pIn3 = &aMem[pOp->p3];
+ pIn2 = &aMem[pOp->p2];
struct space *space = space_by_id(pOp->p1);
assert(space != NULL);
assert(space_is_system(space));
- if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
- goto jump_to_p2;
+ assert(p->errorAction == ON_CONFLICT_ACTION_ABORT);
+ if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0)
+ goto abort_due_to_error;
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
break;
@@ -4404,6 +4404,7 @@ case OP_SDelete: {
struct space *space = space_by_id(pOp->p1);
assert(space != NULL);
assert(space_is_system(space));
+ assert(p->errorAction == ON_CONFLICT_ACTION_ABORT);
if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0)
goto abort_due_to_error;
if (pOp->p5 & OPFLAG_NCHANGE)
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
new file mode 100644
index 000000000..70fb2222e
--- /dev/null
+++ b/test/sql/ddl.result
@@ -0,0 +1,542 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+json = require('json')
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+ | ---
+ | - row_count: 0
+ | ...
+
+--
+-- gh-4086: SQL transactional DDL.
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.space.T1 ~= nil
+ | ---
+ | - true
+ | ...
+box.space.T1.index[0] ~= nil
+ | ---
+ | - true
+ | ...
+box.space.T2 ~= nil
+ | ---
+ | - true
+ | ...
+box.space.T2.index[0] ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('DROP TABLE t1;')
+assert(box.space.T1 == nil)
+assert(box.space.T2 ~= nil)
+box.execute('DROP TABLE t2;')
+assert(box.space.T2 == nil)
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+--
+-- Try to build an index transactionally.
+--
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER)')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T1:replace{1, 1, 1}
+ | ---
+ | - [1, 1, 1]
+ | ...
+box.space.T1:replace{2, 2, 2}
+ | ---
+ | - [2, 2, 2]
+ | ...
+box.space.T1:replace{3, 3, 3}
+ | ---
+ | - [3, 3, 3]
+ | ...
+box.space.T1:replace{4, 4, 4}
+ | ---
+ | - [4, 4, 4]
+ | ...
+box.space.T1:replace{5, 5, 5}
+ | ---
+ | - [5, 5, 5]
+ | ...
+-- Snapshot to dump Vinyl memory level, and force reading from a
+-- disk, if someday create index will support transactions.
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)')
+box.execute('CREATE INDEX t1a ON t1(a)')
+box.execute('CREATE INDEX t1b ON t1(b)')
+box.commit();
+ | ---
+ | - error: Can not perform index build in a multi-statement transaction
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+box.rollback()
+ | ---
+ | ...
+
+--
+-- Index drop does not yield, and is being done in background
+-- later. So it is transactional.
+--
+box.execute('CREATE INDEX t1a ON t1(a)')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('CREATE INDEX t1b ON t1(b)')
+ | ---
+ | - row_count: 1
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)')
+box.execute('DROP INDEX t1a ON t1')
+box.execute('DROP INDEX t1b ON t1')
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+--
+-- Truncate should not be different from index drop in terms of
+-- yields and atomicity.
+--
+box.space.T2:replace{1}
+ | ---
+ | - [1]
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function truncate_both()
+ box.execute('TRUNCATE TABLE t1;')
+ box.execute('TRUNCATE TABLE t2;')
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.begin() truncate_both() box.rollback()
+ | ---
+ | ...
+
+box.space.T1:count() > 0 and box.space.T2:count() > 0
+ | ---
+ | - true
+ | ...
+
+box.begin() truncate_both() box.commit()
+ | ---
+ | ...
+
+box.space.T1:count() == 0 and box.space.T2:count() == 0
+ | ---
+ | - true
+ | ...
+
+box.execute('DROP TABLE t1')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP TABLE t2')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Use all the possible SQL DDL statements.
+--
+test_run:cmd("setopt delimiter '$'")
+ | ---
+ | - true
+ | ...
+function monster_ddl()
+-- Try random errors inside this big batch of DDL to ensure, that
+-- they do not affect normal operation.
+ local _, err1, err2, err3, err4, err5, err6
+ box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER);]])
+ box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER UNIQUE,
+ CONSTRAINT ck1 CHECK(b < 100));]])
+
+ box.execute('CREATE INDEX t1a ON t1(a);')
+ box.execute('CREATE INDEX t2a ON t2(a);')
+
+ box.execute('CREATE TABLE t_to_rename(id INTEGER PRIMARY KEY, a INTEGER);')
+
+ box.execute('DROP INDEX t2a ON t2;')
+
+ box.execute('CREATE INDEX t_to_rename_a ON t_to_rename(a);')
+
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
+
+ _, err1 = pcall(box.execute, 'ALTER TABLE t_to_rename RENAME TO t1;')
+
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
+ box.space.T1.ck_constraint.CK1:drop()
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+ box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
+
+ _, err2 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+
+ box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY
+ KEY AUTOINCREMENT);]])
+
+ box.execute('ALTER TABLE t_to_rename RENAME TO t_renamed;')
+
+ box.execute('DROP INDEX t_to_rename_a ON t_renamed;')
+
+ box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err3 = pcall(box.execute, 'DROP TABLE t3;')
+
+ box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
+
+ box.execute('TRUNCATE TABLE t1;')
+ _, err5 = pcall(box.execute, 'TRUNCATE TABLE t2;')
+ _, err6 = pcall(box.execute, 'TRUNCATE TABLE t_does_not_exist;')
+
+ box.execute('DROP TRIGGER t2t;')
+
+ return {'Finished ok, errors in the middle: ', err1, err2, err3, err4,
+ err5, err6}
+end$
+ | ---
+ | ...
+function monster_ddl_cmp_res(res1, res2)
+ if json.encode(res1) == json.encode(res2) then
+ return true
+ end
+ return res1, res2
+end$
+ | ---
+ | ...
+function monster_ddl_is_clean()
+ assert(box.space.T1 == nil)
+ assert(box.space.T2 == nil)
+ assert(box.space._trigger:count() == 0)
+ assert(box.space._fk_constraint:count() == 0)
+ assert(box.space._ck_constraint:count() == 0)
+ assert(box.space.T_RENAMED == nil)
+ assert(box.space.T_TO_RENAME == nil)
+end$
+ | ---
+ | ...
+function monster_ddl_check()
+ local _, err1, err2, err3, err4, res
+ _, err1 = pcall(box.execute, 'INSERT INTO t2 VALUES (1, 1, 101)')
+ box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+ _, err2 = pcall(box.execute, 'INSERT INTO t2 VALUES(2, 2, 1)')
+ _, err3 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, 20, 1)')
+ _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)')
+ box.execute('INSERT INTO t1 VALUES (1, 1, 1)')
+ res = box.execute('SELECT * FROM trigger_catcher')
+ assert(box.space.T_RENAMED ~= nil)
+ assert(box.space.T_RENAMED.index.T_TO_RENAME_A == nil)
+ assert(box.space.T_TO_RENAME == nil)
+ return {'Finished ok, errors and trigger catcher content: ', err1, err2,
+ err3, err4, res}
+end$
+ | ---
+ | ...
+function monster_ddl_clear()
+ box.execute('DROP TRIGGER IF EXISTS t1t;')
+ box.execute('DROP TABLE IF EXISTS trigger_catcher;')
+ pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;')
+ box.execute('DROP TABLE IF EXISTS t2')
+ box.execute('DROP TABLE IF EXISTS t1')
+ box.execute('DROP TABLE IF EXISTS t_renamed')
+ monster_ddl_is_clean()
+end$
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''")$
+ | ---
+ | - true
+ | ...
+
+-- No txn.
+true_ddl_res = monster_ddl()
+ | ---
+ | ...
+true_ddl_res
+ | ---
+ | - - 'Finished ok, errors in the middle: '
+ | - Space 'T1' already exists
+ | - Space 'T1' already exists
+ | - Space 'T3' does not exist
+ | - Index 'T1A' already exists in space 'T1'
+ | - 'Failed to execute SQL statement: can not truncate space ''T2'' because other
+ | objects depend on it'
+ | - Space 'T_DOES_NOT_EXIST' does not exist
+ | ...
+
+true_check_res = monster_ddl_check()
+ | ---
+ | ...
+true_check_res
+ | ---
+ | - - 'Finished ok, errors and trigger catcher content: '
+ | - 'Check constraint failed ''CK1'': b < 100'
+ | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in space 'T2'
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | - 'Check constraint failed ''CK2'': a > 0'
+ | - metadata:
+ | - name: ID
+ | type: integer
+ | rows:
+ | - [1]
+ | ...
+
+monster_ddl_clear()
+ | ---
+ | ...
+
+-- Both DDL and cleanup in one txn.
+ddl_res = nil
+ | ---
+ | ...
+box.begin() ddl_res = monster_ddl() monster_ddl_clear() box.commit()
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+
+-- DDL in txn, cleanup is not.
+box.begin() ddl_res = monster_ddl() box.commit()
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+
+check_res = monster_ddl_check()
+ | ---
+ | ...
+monster_ddl_cmp_res(check_res, true_check_res)
+ | ---
+ | - true
+ | ...
+
+monster_ddl_clear()
+ | ---
+ | ...
+
+-- DDL is not in txn, cleanup is.
+ddl_res = monster_ddl()
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+
+check_res = monster_ddl_check()
+ | ---
+ | ...
+monster_ddl_cmp_res(check_res, true_check_res)
+ | ---
+ | - true
+ | ...
+
+box.begin() monster_ddl_clear() box.commit()
+ | ---
+ | ...
+
+-- DDL and cleanup in separate txns.
+box.begin() ddl_res = monster_ddl() box.commit()
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+
+check_res = monster_ddl_check()
+ | ---
+ | ...
+monster_ddl_cmp_res(check_res, true_check_res)
+ | ---
+ | - true
+ | ...
+
+box.begin() monster_ddl_clear() box.commit()
+ | ---
+ | ...
+
+-- Try SQL transactions.
+box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('COMMIT')
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+
+check_res = monster_ddl_check()
+ | ---
+ | ...
+monster_ddl_cmp_res(check_res, true_check_res)
+ | ---
+ | - true
+ | ...
+
+box.execute('START TRANSACTION') monster_ddl_clear() box.execute('COMMIT')
+ | ---
+ | ...
+
+box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('ROLLBACK')
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+
+--
+-- Voluntary rollback.
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+assert(box.space.T1 ~= nil)
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+assert(box.space.T2 ~= nil)
+box.rollback();
+ | ---
+ | ...
+
+box.space.T1 == nil and box.space.T2 == nil;
+ | ---
+ | - true
+ | ...
+
+box.begin()
+save1 = box.savepoint()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY)')
+save2 = box.savepoint()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER)')
+box.execute('CREATE INDEX t2a ON t2(a)')
+save3 = box.savepoint()
+assert(box.space.T1 ~= nil)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save3)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+save3 = box.savepoint()
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save2)
+assert(box.space.T2 == nil)
+assert(box.space.T1 ~= nil)
+box.rollback_to_savepoint(save1)
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+box.space.T1 == nil and box.space.T2 == nil
+ | ---
+ | - true
+ | ...
+
+--
+-- Unexpected rollback.
+--
+
+box.begin() ddl_res = monster_ddl() require('fiber').yield()
+ | ---
+ | ...
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+ | ---
+ | - true
+ | ...
+box.commit()
+ | ---
+ | - error: Transaction has been aborted by a fiber yield
+ | ...
+box.rollback()
+ | ---
+ | ...
+monster_ddl_clear()
+ | ---
+ | ...
diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
new file mode 100644
index 000000000..74fc20ea9
--- /dev/null
+++ b/test/sql/ddl.test.lua
@@ -0,0 +1,302 @@
+test_run = require('test_run').new()
+json = require('json')
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+
+--
+-- gh-4086: SQL transactional DDL.
+--
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+box.space.T1 ~= nil
+box.space.T1.index[0] ~= nil
+box.space.T2 ~= nil
+box.space.T2.index[0] ~= nil
+
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('DROP TABLE t1;')
+assert(box.space.T1 == nil)
+assert(box.space.T2 ~= nil)
+box.execute('DROP TABLE t2;')
+assert(box.space.T2 == nil)
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+--
+-- Try to build an index transactionally.
+--
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER, b INTEGER)')
+box.space.T1:replace{1, 1, 1}
+box.space.T1:replace{2, 2, 2}
+box.space.T1:replace{3, 3, 3}
+box.space.T1:replace{4, 4, 4}
+box.space.T1:replace{5, 5, 5}
+-- Snapshot to dump Vinyl memory level, and force reading from a
+-- disk, if someday create index will support transactions.
+box.snapshot()
+
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)')
+box.execute('CREATE INDEX t1a ON t1(a)')
+box.execute('CREATE INDEX t1b ON t1(b)')
+box.commit();
+test_run:cmd("setopt delimiter ''");
+box.rollback()
+
+--
+-- Index drop does not yield, and is being done in background
+-- later. So it is transactional.
+--
+box.execute('CREATE INDEX t1a ON t1(a)')
+box.execute('CREATE INDEX t1b ON t1(b)')
+
+test_run:cmd("setopt delimiter ';'")
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY)')
+box.execute('DROP INDEX t1a ON t1')
+box.execute('DROP INDEX t1b ON t1')
+test_run:cmd("setopt delimiter ''");
+
+--
+-- Truncate should not be different from index drop in terms of
+-- yields and atomicity.
+--
+box.space.T2:replace{1}
+test_run:cmd("setopt delimiter ';'")
+function truncate_both()
+ box.execute('TRUNCATE TABLE t1;')
+ box.execute('TRUNCATE TABLE t2;')
+end;
+test_run:cmd("setopt delimiter ''");
+
+box.begin() truncate_both() box.rollback()
+
+box.space.T1:count() > 0 and box.space.T2:count() > 0
+
+box.begin() truncate_both() box.commit()
+
+box.space.T1:count() == 0 and box.space.T2:count() == 0
+
+box.execute('DROP TABLE t1')
+box.execute('DROP TABLE t2')
+
+--
+-- Use all the possible SQL DDL statements.
+--
+test_run:cmd("setopt delimiter '$'")
+function monster_ddl()
+-- Try random errors inside this big batch of DDL to ensure, that
+-- they do not affect normal operation.
+ local _, err1, err2, err3, err4, err5, err6
+ box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER);]])
+ box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
+ a INTEGER,
+ b INTEGER UNIQUE,
+ CONSTRAINT ck1 CHECK(b < 100));]])
+
+ box.execute('CREATE INDEX t1a ON t1(a);')
+ box.execute('CREATE INDEX t2a ON t2(a);')
+
+ box.execute('CREATE TABLE t_to_rename(id INTEGER PRIMARY KEY, a INTEGER);')
+
+ box.execute('DROP INDEX t2a ON t2;')
+
+ box.execute('CREATE INDEX t_to_rename_a ON t_to_rename(a);')
+
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
+
+ _, err1 = pcall(box.execute, 'ALTER TABLE t_to_rename RENAME TO t1;')
+
+ box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
+ box.space.T1.ck_constraint.CK1:drop()
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+ box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
+
+ _, err2 = pcall(box.execute, 'CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+
+ box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
+ (a) REFERENCES t2(b);]])
+
+ box.execute([[CREATE TABLE trigger_catcher(id INTEGER PRIMARY
+ KEY AUTOINCREMENT);]])
+
+ box.execute('ALTER TABLE t_to_rename RENAME TO t_renamed;')
+
+ box.execute('DROP INDEX t_to_rename_a ON t_renamed;')
+
+ box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err3 = pcall(box.execute, 'DROP TABLE t3;')
+
+ box.execute([[CREATE TRIGGER t2t AFTER INSERT ON t2 FOR EACH ROW
+ BEGIN
+ INSERT INTO trigger_catcher VALUES(1);
+ END; ]])
+
+ _, err4 = pcall(box.execute, 'CREATE INDEX t1a ON t1(a, b);')
+
+ box.execute('TRUNCATE TABLE t1;')
+ _, err5 = pcall(box.execute, 'TRUNCATE TABLE t2;')
+ _, err6 = pcall(box.execute, 'TRUNCATE TABLE t_does_not_exist;')
+
+ box.execute('DROP TRIGGER t2t;')
+
+ return {'Finished ok, errors in the middle: ', err1, err2, err3, err4,
+ err5, err6}
+end$
+function monster_ddl_cmp_res(res1, res2)
+ if json.encode(res1) == json.encode(res2) then
+ return true
+ end
+ return res1, res2
+end$
+function monster_ddl_is_clean()
+ assert(box.space.T1 == nil)
+ assert(box.space.T2 == nil)
+ assert(box.space._trigger:count() == 0)
+ assert(box.space._fk_constraint:count() == 0)
+ assert(box.space._ck_constraint:count() == 0)
+ assert(box.space.T_RENAMED == nil)
+ assert(box.space.T_TO_RENAME == nil)
+end$
+function monster_ddl_check()
+ local _, err1, err2, err3, err4, res
+ _, err1 = pcall(box.execute, 'INSERT INTO t2 VALUES (1, 1, 101)')
+ box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
+ _, err2 = pcall(box.execute, 'INSERT INTO t2 VALUES(2, 2, 1)')
+ _, err3 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, 20, 1)')
+ _, err4 = pcall(box.execute, 'INSERT INTO t1 VALUES(1, -1, 1)')
+ box.execute('INSERT INTO t1 VALUES (1, 1, 1)')
+ res = box.execute('SELECT * FROM trigger_catcher')
+ assert(box.space.T_RENAMED ~= nil)
+ assert(box.space.T_RENAMED.index.T_TO_RENAME_A == nil)
+ assert(box.space.T_TO_RENAME == nil)
+ return {'Finished ok, errors and trigger catcher content: ', err1, err2,
+ err3, err4, res}
+end$
+function monster_ddl_clear()
+ box.execute('DROP TRIGGER IF EXISTS t1t;')
+ box.execute('DROP TABLE IF EXISTS trigger_catcher;')
+ pcall(box.execute, 'ALTER TABLE t1 DROP CONSTRAINT fk1;')
+ box.execute('DROP TABLE IF EXISTS t2')
+ box.execute('DROP TABLE IF EXISTS t1')
+ box.execute('DROP TABLE IF EXISTS t_renamed')
+ monster_ddl_is_clean()
+end$
+test_run:cmd("setopt delimiter ''")$
+
+-- No txn.
+true_ddl_res = monster_ddl()
+true_ddl_res
+
+true_check_res = monster_ddl_check()
+true_check_res
+
+monster_ddl_clear()
+
+-- Both DDL and cleanup in one txn.
+ddl_res = nil
+box.begin() ddl_res = monster_ddl() monster_ddl_clear() box.commit()
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+
+-- DDL in txn, cleanup is not.
+box.begin() ddl_res = monster_ddl() box.commit()
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+
+check_res = monster_ddl_check()
+monster_ddl_cmp_res(check_res, true_check_res)
+
+monster_ddl_clear()
+
+-- DDL is not in txn, cleanup is.
+ddl_res = monster_ddl()
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+
+check_res = monster_ddl_check()
+monster_ddl_cmp_res(check_res, true_check_res)
+
+box.begin() monster_ddl_clear() box.commit()
+
+-- DDL and cleanup in separate txns.
+box.begin() ddl_res = monster_ddl() box.commit()
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+
+check_res = monster_ddl_check()
+monster_ddl_cmp_res(check_res, true_check_res)
+
+box.begin() monster_ddl_clear() box.commit()
+
+-- Try SQL transactions.
+box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('COMMIT')
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+
+check_res = monster_ddl_check()
+monster_ddl_cmp_res(check_res, true_check_res)
+
+box.execute('START TRANSACTION') monster_ddl_clear() box.execute('COMMIT')
+
+box.execute('START TRANSACTION') ddl_res = monster_ddl() box.execute('ROLLBACK')
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+
+--
+-- Voluntary rollback.
+--
+test_run:cmd("setopt delimiter ';'")
+box.begin()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
+assert(box.space.T1 ~= nil)
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
+assert(box.space.T2 ~= nil)
+box.rollback();
+
+box.space.T1 == nil and box.space.T2 == nil;
+
+box.begin()
+save1 = box.savepoint()
+box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY)')
+save2 = box.savepoint()
+box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER)')
+box.execute('CREATE INDEX t2a ON t2(a)')
+save3 = box.savepoint()
+assert(box.space.T1 ~= nil)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save3)
+assert(box.space.T2 ~= nil)
+assert(box.space.T2.index.T2A ~= nil)
+save3 = box.savepoint()
+box.execute('DROP TABLE t2')
+assert(box.space.T2 == nil)
+box.rollback_to_savepoint(save2)
+assert(box.space.T2 == nil)
+assert(box.space.T1 ~= nil)
+box.rollback_to_savepoint(save1)
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+box.space.T1 == nil and box.space.T2 == nil
+
+--
+-- Unexpected rollback.
+--
+
+box.begin() ddl_res = monster_ddl() require('fiber').yield()
+monster_ddl_cmp_res(ddl_res, true_ddl_res)
+box.commit()
+box.rollback()
+monster_ddl_clear()
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index 8846e5ee8..257dbafde 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -388,56 +388,6 @@ box.execute("DROP TABLE t3;")
---
- row_count: 1
...
--- gh-3780: space without PK raises error if
--- it is used in SQL queries.
---
-errinj = box.error.injection
----
-...
-fiber = require('fiber')
----
-...
-box.execute("CREATE TABLE t (id INT PRIMARY KEY);")
----
-- row_count: 1
-...
-box.execute("INSERT INTO t VALUES (1);")
----
-- row_count: 1
-...
-errinj.set("ERRINJ_WAL_DELAY", true)
----
-- ok
-...
--- DROP TABLE consists of several steps: firstly indexes
--- are deleted, then space itself. Lets make sure that if
--- first part of drop is successfully finished, but resulted
--- in yield, all operations on space will be blocked due to
--- absence of primary key.
---
-function drop_table_yield() box.execute("DROP TABLE t;") end
----
-...
-f = fiber.create(drop_table_yield)
----
-...
-box.execute("SELECT * FROM t;")
----
-- error: SQL does not support spaces without primary key
-...
-box.execute("INSERT INTO t VALUES (2);")
----
-- error: SQL does not support spaces without primary key
-...
-box.execute("UPDATE t SET id = 2;")
----
-- error: SQL does not support spaces without primary key
-...
--- Finish drop space.
-errinj.set("ERRINJ_WAL_DELAY", false)
----
-- ok
-...
--
-- gh-3931: Store regular identifiers in case-normal form
--
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 48b80a443..3bc1b684d 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -118,28 +118,6 @@ box.execute("INSERT INTO t3 VALUES(1, 1, 3);")
errinj.set("ERRINJ_WAL_IO", false)
box.execute("DROP TABLE t3;")
--- gh-3780: space without PK raises error if
--- it is used in SQL queries.
---
-errinj = box.error.injection
-fiber = require('fiber')
-box.execute("CREATE TABLE t (id INT PRIMARY KEY);")
-box.execute("INSERT INTO t VALUES (1);")
-errinj.set("ERRINJ_WAL_DELAY", true)
--- DROP TABLE consists of several steps: firstly indexes
--- are deleted, then space itself. Lets make sure that if
--- first part of drop is successfully finished, but resulted
--- in yield, all operations on space will be blocked due to
--- absence of primary key.
---
-function drop_table_yield() box.execute("DROP TABLE t;") end
-f = fiber.create(drop_table_yield)
-box.execute("SELECT * FROM t;")
-box.execute("INSERT INTO t VALUES (2);")
-box.execute("UPDATE t SET id = 2;")
--- Finish drop space.
-errinj.set("ERRINJ_WAL_DELAY", false)
-
--
-- gh-3931: Store regular identifiers in case-normal form
--
diff --git a/test/sql/view_delayed_wal.result b/test/sql/view_delayed_wal.result
index 519794931..d518e7d8c 100644
--- a/test/sql/view_delayed_wal.result
+++ b/test/sql/view_delayed_wal.result
@@ -13,8 +13,8 @@ fiber = require('fiber')
...
-- View reference counters are incremented before firing
-- on_commit triggers (i.e. before being written into WAL), so
--- it is impossible to create view on dropped (but not written
--- into WAL) space.
+-- it is impossible to drop a space referenced by a created, but
+-- *still* not yet committed view.
--
box.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
---
@@ -49,13 +49,13 @@ box.error.injection.set("ERRINJ_WAL_DELAY", false)
fiber.sleep(0.1)
---
...
-box.space.T1
+box.space.T1 ~= nil
---
-- null
+- true
...
-box.space.V1
+box.space.V1 ~= nil
---
-- null
+- true
...
--
-- In the same way, we have to drop all referenced spaces before
diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
index 8e73b03f3..7e6fce6ca 100644
--- a/test/sql/view_delayed_wal.test.lua
+++ b/test/sql/view_delayed_wal.test.lua
@@ -5,8 +5,8 @@ fiber = require('fiber')
-- View reference counters are incremented before firing
-- on_commit triggers (i.e. before being written into WAL), so
--- it is impossible to create view on dropped (but not written
--- into WAL) space.
+-- it is impossible to drop a space referenced by a created, but
+-- *still* not yet committed view.
--
box.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
function create_view() box.execute('CREATE VIEW v1 AS SELECT * FROM t1') end
@@ -18,8 +18,8 @@ f2 = fiber.create(drop_index_t1)
f3 = fiber.create(drop_space_t1)
box.error.injection.set("ERRINJ_WAL_DELAY", false)
fiber.sleep(0.1)
-box.space.T1
-box.space.V1
+box.space.T1 ~= nil
+box.space.V1 ~= nil
--
-- In the same way, we have to drop all referenced spaces before
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-22 21:35 ` Vladislav Shpilevoy
@ 2019-07-24 13:23 ` n.pettik
2019-07-24 20:58 ` Vladislav Shpilevoy
0 siblings, 1 reply; 12+ messages in thread
From: n.pettik @ 2019-07-24 13:23 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
Thanks, the only dubious nit I can spot: should we
set initiateTTrans for DDL operations which consist
of one box operations? Like: create index, drop index,
create trigger, drop trigger, drop constraint etc
For instance, we don’t set it for TRUNCATE stmt.
The only two operations which require starting transactions
are CREATE TABLE and RENAME. Btw, when checking
results of RENAME I’d also check that operation (update of
‘CREATE TRIGGER …' string) performed on _trigger has
been committed/rollbacked.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-24 13:23 ` n.pettik
@ 2019-07-24 20:58 ` Vladislav Shpilevoy
2019-07-25 12:04 ` n.pettik
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-24 20:58 UTC (permalink / raw)
To: n.pettik, tarantool-patches
Hi! Thanks for the comments!
On 24/07/2019 15:23, n.pettik wrote:
>
> Thanks, the only dubious nit I can spot: should we
> set initiateTTrans for DDL operations which consist
> of one box operations?
Yeah, I thought about it, and even made it and reverted back
several times having some doubts 'code consistency vs
transaction necessity'.
After all I decided, that each DDL op should be transactional,
even if consists of one statement. Because 1) it is consistent
with other SQL DDL, 2) it protects us from the case when SQL
DDL will complicate. I think, the latter matters the most, taking
into account incoming information schema, when even a simple
index creation will insert into _index, into something like
_constraint_name to ensure name uniqueness, etc.
> Like: create index, drop index,
> create trigger, drop trigger, drop constraint etc
> For instance, we don’t set it for TRUNCATE stmt.
Good catch, now I will.
----------------------------------------------------------------
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 06eab7f2d..c5e41cbf3 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -779,6 +779,7 @@ cmd ::= with(C) DELETE FROM fullname(X) indexed_opt(I) where_opt(W). {
/////////////////////////// The TRUNCATE statement /////////////////////////////
//
cmd ::= TRUNCATE TABLE fullname(X). {
+ pParse->initiateTTrans = true;
sql_table_truncate(pParse, X);
}
----------------------------------------------------------------
> The only two operations which require starting transactions
> are CREATE TABLE and RENAME.
Do not forget about DROP TABLE.
> Btw, when checking
> results of RENAME I’d also check that operation (update of
> ‘CREATE TRIGGER …' string) performed on _trigger has
> been committed/rollbacked.
>
Agree.
----------------------------------------------------------------
diff --git a/test/sql/ddl.result b/test/sql/ddl.result
index 834d9757f..a47e15bb4 100644
--- a/test/sql/ddl.result
+++ b/test/sql/ddl.result
@@ -187,7 +187,58 @@ box.space.T1:count() == 0 and box.space.T2:count() == 0
| - true
| ...
-box.execute('DROP TABLE t1')
+--
+-- Rename transactionally changes name of the table and its
+-- mentioning in trigger bodies.
+--
+_trigger_index = box.space._trigger.index.space_id
+ | ---
+ | ...
+test_run:cmd("setopt delimiter '$'")
+ | ---
+ | - true
+ | ...
+box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+ BEGIN
+ INSERT INTO t2 VALUES(1);
+ END; ]])$
+ | ---
+ | - row_count: 1
+ | ...
+
+box.begin()
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+assert(sql:find('T1_NEW'))
+box.rollback()$
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''")$
+ | ---
+ | - true
+ | ...
+
+sql = _trigger_index:select(box.space.T1.id)[1].opts.sql
+ | ---
+ | ...
+not sql:find('T1_NEW') and sql:find('t1') ~= nil
+ | ---
+ | - true
+ | ...
+
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+ | ---
+ | - row_count: 0
+ | ...
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+ | ---
+ | ...
+sql:find('T1_NEW') ~= nil
+ | ---
+ | - true
+ | ...
+
+box.execute('DROP TABLE t1_new')
| ---
| - row_count: 1
| ...
diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
index 300f8b20c..371f0339e 100644
--- a/test/sql/ddl.test.lua
+++ b/test/sql/ddl.test.lua
@@ -85,7 +85,32 @@ box.begin() truncate_both() box.commit()
box.space.T1:count() == 0 and box.space.T2:count() == 0
-box.execute('DROP TABLE t1')
+--
+-- Rename transactionally changes name of the table and its
+-- mentioning in trigger bodies.
+--
+_trigger_index = box.space._trigger.index.space_id
+test_run:cmd("setopt delimiter '$'")
+box.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 FOR EACH ROW
+ BEGIN
+ INSERT INTO t2 VALUES(1);
+ END; ]])$
+
+box.begin()
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+assert(sql:find('T1_NEW'))
+box.rollback()$
+test_run:cmd("setopt delimiter ''")$
+
+sql = _trigger_index:select(box.space.T1.id)[1].opts.sql
+not sql:find('T1_NEW') and sql:find('t1') ~= nil
+
+box.execute('ALTER TABLE t1 RENAME TO t1_new;')
+sql = _trigger_index:select(box.space.T1_NEW.id)[1].opts.sql
+sql:find('T1_NEW') ~= nil
+
+box.execute('DROP TABLE t1_new')
box.execute('DROP TABLE t2')
--
----------------------------------------------------------------
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL
2019-07-24 20:58 ` Vladislav Shpilevoy
@ 2019-07-25 12:04 ` n.pettik
0 siblings, 0 replies; 12+ messages in thread
From: n.pettik @ 2019-07-25 12:04 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 24 Jul 2019, at 23:58, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> On 24/07/2019 15:23, n.pettik wrote:
>>
>> Thanks, the only dubious nit I can spot: should we
>> set initiateTTrans for DDL operations which consist
>> of one box operations?
>
> Yeah, I thought about it, and even made it and reverted back
> several times having some doubts 'code consistency vs
> transaction necessity'.
>
> After all I decided, that each DDL op should be transactional,
> even if consists of one statement. Because 1) it is consistent
> with other SQL DDL, 2) it protects us from the case when SQL
> DDL will complicate. I think, the latter matters the most, taking
> into account incoming information schema, when even a simple
> index creation will insert into _index, into something like
> _constraint_name to ensure name uniqueness, etc.
Ok, I was driven by the same thoughts.
Thanks, now LGTM.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH 0/2] SQL TXN DDL
2019-07-19 22:49 [tarantool-patches] [PATCH 0/2] SQL TXN DDL Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 1/2] alter: do not access space in drop ck commit trigger Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 2/2] sql: transactional DDL Vladislav Shpilevoy
@ 2019-07-31 13:19 ` Kirill Yukhin
2 siblings, 0 replies; 12+ messages in thread
From: Kirill Yukhin @ 2019-07-31 13:19 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev
Hello,
On 20 Jul 00:49, Vladislav Shpilevoy wrote:
> The patchset uses recently introduced transactional DDL in SQL. Nothing more to
> say, the goals are quite obvious.
>
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4086-sql-txn-ddl
> Issue: https://github.com/tarantool/tarantool/issues/4086
I've checked the patchset into master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-07-31 13:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-19 22:49 [tarantool-patches] [PATCH 0/2] SQL TXN DDL Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 1/2] alter: do not access space in drop ck commit trigger Vladislav Shpilevoy
2019-07-19 22:49 ` [tarantool-patches] [PATCH 2/2] sql: transactional DDL Vladislav Shpilevoy
2019-07-20 7:42 ` [tarantool-patches] " Konstantin Osipov
2019-07-20 14:02 ` Vladislav Shpilevoy
2019-07-20 15:16 ` Konstantin Osipov
2019-07-22 13:59 ` n.pettik
2019-07-22 21:35 ` Vladislav Shpilevoy
2019-07-24 13:23 ` n.pettik
2019-07-24 20:58 ` Vladislav Shpilevoy
2019-07-25 12:04 ` n.pettik
2019-07-31 13:19 ` [tarantool-patches] Re: [PATCH 0/2] SQL TXN DDL Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox