* [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints
@ 2020-03-03 10:12 Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message Roman Khabibov
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:12 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
@ChangeLog
- Add ability to drop any table constraint with
<ALTER TABLE ... DROP CONSTRAINT ...>.
- Display table name in "no such constraint" error message.
Roman Khabibov (3):
sql: improve "no such constraint" error message
sql: don't select from _index during parsing
sql: support constraint drop
src/box/constraint_id.h | 1 +
src/box/errcode.h | 2 +-
src/box/space.h | 15 ++++
src/box/sql/alter.c | 2 +-
src/box/sql/build.c | 145 ++++++++++++++++++++++-------------
src/box/sql/parse.y | 4 +-
src/box/sql/parse_def.h | 11 +--
src/box/sql/pragma.c | 8 +-
src/box/sql/sqlInt.h | 7 +-
src/box/sql/vdbe.c | 10 ++-
test/sql-tap/alter2.test.lua | 2 +-
test/sql/checks.result | 2 +-
test/sql/constraint.result | 81 +++++++++++++++++++
test/sql/constraint.test.lua | 28 +++++++
14 files changed, 241 insertions(+), 77 deletions(-)
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message
2020-03-03 10:12 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
@ 2020-03-03 10:12 ` Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:12 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Clarify the error message for better user handling. Add the name
of space where the constraint under dropping wasn't founded.
Part of #4120
---
src/box/errcode.h | 2 +-
src/box/sql/alter.c | 2 +-
src/box/sql/build.c | 24 +++++++++++++-----------
test/sql-tap/alter2.test.lua | 2 +-
test/sql/checks.result | 2 +-
5 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index cac8447e2..d7ec97e8c 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -221,7 +221,7 @@ struct errcode_record {
/*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
/*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
/*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
- /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
+ /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
/*170 */_(ER_CONSTRAINT_EXISTS, "%s constraint '%s' already exists in space '%s'") \
/*171 */_(ER_SQL_TYPE_MISMATCH, "Type mismatch: can not convert %s to %s") \
/*172 */_(ER_ROWID_OVERFLOW, "Rowid is overflowed: too many entries in ephemeral space") \
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 973b420cf..14f6c1a97 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -121,7 +121,7 @@ sql_alter_ck_constraint_enable(struct Parse *parse)
int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2);
sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0,
sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name), P4_DYNAMIC);
+ constraint_name, tbl_name), P4_DYNAMIC);
sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
sqlVdbeJumpHere(v, addr);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bc50ecbfa..d9bf8de91 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1476,21 +1476,21 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
* @param constraint_name Name of FK constraint to be dropped.
* Must be allocated on head by sqlDbMalloc().
* It will be freed in VDBE.
- * @param child_id Id of table which constraint belongs to.
+ * @param child_def Def of table which constraint belongs to.
*/
static void
vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
- uint32_t child_id)
+ struct space_def *child_def)
{
struct Vdbe *vdbe = sqlGetVdbe(parse_context);
assert(vdbe != NULL);
int key_reg = sqlGetTempRange(parse_context, 3);
sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
P4_DYNAMIC);
- sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
+ sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
const char *error_msg =
tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name);
+ constraint_name, child_def->name);
if (vdbe_emit_halt_with_presence_test(parse_context,
BOX_FK_CONSTRAINT_ID, 0,
key_reg, 2, ER_NO_SUCH_CONSTRAINT,
@@ -1510,21 +1510,22 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
*
* @param parser Parsing context.
* @param ck_name Name of CK constraint to be dropped.
- * @param space_id Id of table which constraint belongs to.
+ * @param space_def Def of table which constraint belongs to.
*/
static void
vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
- uint32_t space_id)
+ struct space_def *space_def)
{
struct Vdbe *v = sqlGetVdbe(parser);
struct sql *db = v->db;
assert(v != NULL);
int key_reg = sqlGetTempRange(parser, 3);
- sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
+ sqlVdbeAddOp2(v, OP_Integer, space_def->id, key_reg);
sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
sqlDbStrDup(db, ck_name), P4_DYNAMIC);
const char *error_msg =
- tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
+ tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name,
+ space_def->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) != 0)
@@ -1658,14 +1659,15 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
char *fk_name_dup = sqlDbStrDup(v->db, child_fk->def->name);
if (fk_name_dup == NULL)
return;
- vdbe_emit_fk_constraint_drop(parse_context, fk_name_dup, space_id);
+ vdbe_emit_fk_constraint_drop(parse_context, fk_name_dup,
+ space->def);
}
/* Delete all CK constraints. */
struct ck_constraint *ck_constraint;
rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) {
vdbe_emit_ck_constraint_drop(parse_context,
ck_constraint->def->name,
- space_id);
+ space->def);
}
/*
* Drop all _space and _index entries that refer to the
@@ -2071,7 +2073,7 @@ sql_drop_foreign_key(struct Parse *parse_context)
return;
}
vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def->id);
+ child->def);
/*
* We account changes to row count only if drop of
* foreign keys take place in a separate
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index 3e21a5f40..759acc9c3 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -256,7 +256,7 @@ test:do_catchsql_test(
ALTER TABLE child DROP CONSTRAINT fake;
]], {
-- <alter2-5.2>
- 1, "Constraint FAKE does not exist"
+ 1, "Constraint 'FAKE' does not exist in space 'CHILD'"
-- </alter2-5.2>
})
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 4ae0b4c10..7b18e5d6b 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -801,7 +801,7 @@ box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);')
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"")
---
- null
-- Constraint falsch does not exist
+- Constraint 'falsch' does not exist in space 'TEST'
...
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"")
---
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing
2020-03-03 10:12 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message Roman Khabibov
@ 2020-03-03 10:12 ` Roman Khabibov
2020-03-03 12:47 ` Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop Roman Khabibov
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:12 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Remove function box_index_by_name() from parser to avoid selects
during parsing. Add the ability to choose index during VDBE code
compilation which will be used to find the tuple to drop from a
system space.
Needed for #4120
---
src/box/space.h | 15 +++++++++++
src/box/sql/build.c | 59 +++++++++++++++++++++++++++-----------------
src/box/sql/pragma.c | 8 ++----
src/box/sql/vdbe.c | 10 +++++---
4 files changed, 59 insertions(+), 33 deletions(-)
diff --git a/src/box/space.h b/src/box/space.h
index 9aea4e5be..28885d037 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -287,6 +287,21 @@ space_index(struct space *space, uint32_t id)
return NULL;
}
+/**
+ * Get index by index name.
+ * @return NULL if the index is not found.
+ */
+static inline struct index *
+space_index_by_name(struct space *space, const char *index_name)
+{
+ for(uint32_t i = 0; i < space->index_count; i++) {
+ struct index *index = space->index[i];
+ if (strcmp(index_name, index->def->name) == 0)
+ return index;
+ }
+ return NULL;
+}
+
/**
* Return true if the unique constraint must be checked for
* the index with the given id before inserting a tuple into
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d9bf8de91..e0f690fc2 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -47,7 +47,6 @@
#include "sqlInt.h"
#include "vdbeInt.h"
#include "tarantoolInt.h"
-#include "box/box.h"
#include "box/ck_constraint.h"
#include "box/fk_constraint.h"
#include "box/sequence.h"
@@ -1469,6 +1468,40 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
sql_table_delete_from(parse, src_list, where);
}
+/**
+ * Generate VDBE program to remove entry from _index space.
+ *
+ * @param parse_context Parsing context.
+ * @param name Index name.
+ * @param space_def Def of table which index belongs to.
+ * @param errcode Type of printing error: "no such index" or
+ * "no such constraint".
+ * @param if_exist True if there was <IF EXISTS> in the query.
+ */
+static void
+vdbe_emit_index_drop(struct Parse *parse_context, const char *name,
+ struct space_def *space_def, int errcode, bool if_exist)
+{
+ assert(errcode == ER_NO_SUCH_INDEX_NAME ||
+ errcode == ER_NO_SUCH_CONSTRAINT);
+ struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+ assert(vdbe != NULL);
+ assert(parse_context->db != NULL);
+ int key_reg = sqlGetTempRange(parse_context, 3);
+ sqlVdbeAddOp2(vdbe, OP_Integer, space_def->id, key_reg);
+ sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg + 1, 0,
+ sqlDbStrDup(parse_context->db, name), P4_DYNAMIC);
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(errcode), name, space_def->name);
+ if (vdbe_emit_halt_with_presence_test(parse_context, BOX_INDEX_ID, 2,
+ key_reg, 2, errcode, error_msg,
+ if_exist, OP_Found) != 0)
+ return;
+ sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
+ sqlVdbeAddOp3(vdbe, OP_SDelete, BOX_INDEX_ID, key_reg + 2, 2);
+ sqlReleaseTempRange(parse_context, key_reg, 3);
+}
+
/**
* Generate VDBE program to remove entry from _fk_constraint space.
*
@@ -2695,29 +2728,9 @@ sql_drop_index(struct Parse *parse_context)
parse_context->is_aborted = true;
goto exit_drop_index;
}
- uint32_t index_id = box_index_id_by_name(space->def->id, index_name,
- strlen(index_name));
- if (index_id == BOX_ID_NIL) {
- if (!if_exists) {
- diag_set(ClientError, ER_NO_SUCH_INDEX_NAME,
- index_name, table_name);
- parse_context->is_aborted = true;
- }
- goto exit_drop_index;
- }
- /*
- * Generate code to remove entry from _index space
- * But firstly, delete statistics since schema
- * changes after DDL.
- */
- int record_reg = ++parse_context->nMem;
- int space_id_reg = ++parse_context->nMem;
- int index_id_reg = ++parse_context->nMem;
- sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
- sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ vdbe_emit_index_drop(parse_context, index_name, space->def,
+ ER_NO_SUCH_INDEX_NAME, if_exists);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
exit_drop_index:
sqlSrcListDelete(db, table_list);
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index a77bf4b16..c15f2e0d1 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -33,7 +33,6 @@
* This file contains code used to implement the PRAGMA command.
*/
#include "box/index.h"
-#include "box/box.h"
#include "box/tuple.h"
#include "box/fk_constraint.h"
#include "box/schema.h"
@@ -192,12 +191,9 @@ sql_pragma_index_info(struct Parse *parse,
struct space *space = space_by_name(tbl_name);
if (space == NULL)
return;
- uint32_t iid = box_index_id_by_name(space->def->id, idx_name,
- strlen(idx_name));
- if (iid == BOX_ID_NIL)
+ struct index *idx = space_index_by_name(space, idx_name);
+ if (idx == NULL)
return;
- struct index *idx = space_index(space, iid);
- assert(idx != NULL);
parse->nMem = 6;
struct Vdbe *v = sqlGetVdbe(parse);
assert(v != NULL);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 620d74e66..912a10153 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4460,11 +4460,12 @@ case OP_SInsert: {
break;
}
-/* Opcode: SDelete P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SDelete P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P2], searching index id = P3
*
* This opcode is used only during DDL routine.
- * Delete entry with given key from system space.
+ * Delete entry with given key from system space. P3 is the index
+ * number by which to search for the key.
*
* If P5 is set to OPFLAG_NCHANGE, account overall changes
* made to database.
@@ -4472,13 +4473,14 @@ case OP_SInsert: {
case OP_SDelete: {
assert(pOp->p1 > 0);
assert(pOp->p2 >= 0);
+ assert(pOp->p3 >= 0);
pIn2 = &aMem[pOp->p2];
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)
+ if (sql_delete_by_key(space, pOp->p3, pIn2->z, pIn2->n) != 0)
goto abort_due_to_error;
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
@ 2020-03-03 12:47 ` Roman Khabibov
0 siblings, 0 replies; 11+ messages in thread
From: Roman Khabibov @ 2020-03-03 12:47 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Don’t see the previous mail of this commit.
sql: don't select from _index during parsing
Remove function box_index_by_name() from parser to avoid selects
during parsing. Add the ability to choose index during VDBE code
compilation which will be used to find the tuple to drop from a
system space.
Needed for #4120
---
src/box/space.h | 19 +++++++++++
src/box/sql/build.c | 79 +++++++++++++++++++++-----------------------
src/box/sql/pragma.c | 8 ++---
src/box/sql/vdbe.c | 10 +++---
4 files changed, 64 insertions(+), 52 deletions(-)
diff --git a/src/box/space.h b/src/box/space.h
index 9aea4e5be..bbdd3ef70 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -287,6 +287,25 @@ space_index(struct space *space, uint32_t id)
return NULL;
}
+/**
+ * Get index by index name.
+ *
+ * @param space Space index belongs to.
+ * @param index_name Name of index to be found.
+ *
+ * @retval NULL if the index is not found.
+ */
+static inline struct index *
+space_index_by_name(struct space *space, const char *index_name)
+{
+ for(uint32_t i = 0; i < space->index_count; i++) {
+ struct index *index = space->index[i];
+ if (strcmp(index_name, index->def->name) == 0)
+ return index;
+ }
+ return NULL;
+}
+
/**
* Return true if the unique constraint must be checked for
* the index with the given id before inserting a tuple into
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d9bf8de91..fef069640 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -47,7 +47,6 @@
#include "sqlInt.h"
#include "vdbeInt.h"
#include "tarantoolInt.h"
-#include "box/box.h"
#include "box/ck_constraint.h"
#include "box/fk_constraint.h"
#include "box/sequence.h"
@@ -116,24 +115,6 @@ sql_finish_coding(struct Parse *parse_context)
parse_context->is_aborted = true;
}
}
-/**
- * Find index by its name.
- *
- * @param space Space index belongs to.
- * @param name Name of index to be found.
- *
- * @retval NULL in case index doesn't exist.
- */
-static struct index *
-sql_space_index_by_name(struct space *space, const char *name)
-{
- for (uint32_t i = 0; i < space->index_count; ++i) {
- struct index *idx = space->index[i];
- if (strcmp(name, idx->def->name) == 0)
- return idx;
- }
- return NULL;
-}
bool
sql_space_column_is_in_pk(struct space *space, uint32_t column)
@@ -1469,6 +1450,40 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
sql_table_delete_from(parse, src_list, where);
}
+/**
+ * Generate VDBE program to remove entry from _index space.
+ *
+ * @param parse_context Parsing context.
+ * @param name Index name.
+ * @param space_def Def of table which index belongs to.
+ * @param errcode Type of printing error: "no such index" or
+ * "no such constraint".
+ * @param if_exist True if there was <IF EXISTS> in the query.
+ */
+static void
+vdbe_emit_index_drop(struct Parse *parse_context, const char *name,
+ struct space_def *space_def, int errcode, bool if_exist)
+{
+ assert(errcode == ER_NO_SUCH_INDEX_NAME ||
+ errcode == ER_NO_SUCH_CONSTRAINT);
+ struct Vdbe *vdbe = sqlGetVdbe(parse_context);
+ assert(vdbe != NULL);
+ assert(parse_context->db != NULL);
+ int key_reg = sqlGetTempRange(parse_context, 3);
+ sqlVdbeAddOp2(vdbe, OP_Integer, space_def->id, key_reg);
+ sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg + 1, 0,
+ sqlDbStrDup(parse_context->db, name), P4_DYNAMIC);
+ const char *error_msg =
+ tt_sprintf(tnt_errcode_desc(errcode), name, space_def->name);
+ if (vdbe_emit_halt_with_presence_test(parse_context, BOX_INDEX_ID, 2,
+ key_reg, 2, errcode, error_msg,
+ if_exist, OP_Found) != 0)
+ return;
+ sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
+ sqlVdbeAddOp3(vdbe, OP_SDelete, BOX_INDEX_ID, key_reg + 2, 2);
+ sqlReleaseTempRange(parse_context, key_reg, 3);
+}
+
/**
* Generate VDBE program to remove entry from _fk_constraint space.
*
@@ -2401,7 +2416,7 @@ sql_create_index(struct Parse *parse) {
parse->is_aborted = true;
goto exit_create_index;
}
- if (sql_space_index_by_name(space, name) != NULL) {
+ if (space_index_by_name(space, name) != NULL) {
if (! create_entity_def->if_not_exist) {
diag_set(ClientError, ER_INDEX_EXISTS_IN_SPACE,
name, def->name);
@@ -2695,29 +2710,9 @@ sql_drop_index(struct Parse *parse_context)
parse_context->is_aborted = true;
goto exit_drop_index;
}
- uint32_t index_id = box_index_id_by_name(space->def->id, index_name,
- strlen(index_name));
- if (index_id == BOX_ID_NIL) {
- if (!if_exists) {
- diag_set(ClientError, ER_NO_SUCH_INDEX_NAME,
- index_name, table_name);
- parse_context->is_aborted = true;
- }
- goto exit_drop_index;
- }
- /*
- * Generate code to remove entry from _index space
- * But firstly, delete statistics since schema
- * changes after DDL.
- */
- int record_reg = ++parse_context->nMem;
- int space_id_reg = ++parse_context->nMem;
- int index_id_reg = ++parse_context->nMem;
- sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
- sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ vdbe_emit_index_drop(parse_context, index_name, space->def,
+ ER_NO_SUCH_INDEX_NAME, if_exists);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
exit_drop_index:
sqlSrcListDelete(db, table_list);
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index a77bf4b16..c15f2e0d1 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -33,7 +33,6 @@
* This file contains code used to implement the PRAGMA command.
*/
#include "box/index.h"
-#include "box/box.h"
#include "box/tuple.h"
#include "box/fk_constraint.h"
#include "box/schema.h"
@@ -192,12 +191,9 @@ sql_pragma_index_info(struct Parse *parse,
struct space *space = space_by_name(tbl_name);
if (space == NULL)
return;
- uint32_t iid = box_index_id_by_name(space->def->id, idx_name,
- strlen(idx_name));
- if (iid == BOX_ID_NIL)
+ struct index *idx = space_index_by_name(space, idx_name);
+ if (idx == NULL)
return;
- struct index *idx = space_index(space, iid);
- assert(idx != NULL);
parse->nMem = 6;
struct Vdbe *v = sqlGetVdbe(parse);
assert(v != NULL);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 620d74e66..912a10153 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4460,11 +4460,12 @@ case OP_SInsert: {
break;
}
-/* Opcode: SDelete P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SDelete P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P2], searching index id = P3
*
* This opcode is used only during DDL routine.
- * Delete entry with given key from system space.
+ * Delete entry with given key from system space. P3 is the index
+ * number by which to search for the key.
*
* If P5 is set to OPFLAG_NCHANGE, account overall changes
* made to database.
@@ -4472,13 +4473,14 @@ case OP_SInsert: {
case OP_SDelete: {
assert(pOp->p1 > 0);
assert(pOp->p2 >= 0);
+ assert(pOp->p3 >= 0);
pIn2 = &aMem[pOp->p2];
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)
+ if (sql_delete_by_key(space, pOp->p3, pIn2->z, pIn2->n) != 0)
goto abort_due_to_error;
if (pOp->p5 & OPFLAG_NCHANGE)
p->nChange++;
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-03-03 10:12 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
@ 2020-03-03 10:12 ` Roman Khabibov
2020-03-03 22:13 ` [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Vladislav Shpilevoy
2020-03-04 21:09 ` Nikita Pettik
4 siblings, 0 replies; 11+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:12 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
---
src/box/constraint_id.h | 1 +
src/box/sql/build.c | 66 +++++++++++++++++++----------
src/box/sql/parse.y | 4 +-
src/box/sql/parse_def.h | 11 ++---
src/box/sql/sqlInt.h | 7 +++-
test/sql/constraint.result | 81 ++++++++++++++++++++++++++++++++++++
test/sql/constraint.test.lua | 28 +++++++++++++
7 files changed, 167 insertions(+), 31 deletions(-)
diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h
index 21f067cdc..ed4f37426 100755
--- a/src/box/constraint_id.h
+++ b/src/box/constraint_id.h
@@ -40,6 +40,7 @@ enum constraint_type {
CONSTRAINT_TYPE_UNIQUE,
CONSTRAINT_TYPE_FK,
CONSTRAINT_TYPE_CK,
+ constraint_type_MAX,
};
extern const char *constraint_type_strs[];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e0f690fc2..6fe8fbcdf 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1507,18 +1507,18 @@ vdbe_emit_index_drop(struct Parse *parse_context, const char *name,
*
* @param parse_context Parsing context.
* @param constraint_name Name of FK constraint to be dropped.
- * Must be allocated on head by sqlDbMalloc().
- * It will be freed in VDBE.
* @param child_def Def of table which constraint belongs to.
*/
static void
-vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
+vdbe_emit_fk_constraint_drop(struct Parse *parse_context,
+ const char *constraint_name,
struct space_def *child_def)
{
struct Vdbe *vdbe = sqlGetVdbe(parse_context);
assert(vdbe != NULL);
int key_reg = sqlGetTempRange(parse_context, 3);
- sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
+ const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
+ sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
P4_DYNAMIC);
sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
const char *error_msg =
@@ -1528,10 +1528,8 @@ 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) != 0) {
- sqlDbFree(parse_context->db, constraint_name);
+ OP_Found) != 0)
return;
- }
sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
@@ -1688,11 +1686,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
struct fk_constraint *child_fk;
rlist_foreach_entry(child_fk, &space->child_fk_constraint,
in_child_space) {
-
- char *fk_name_dup = sqlDbStrDup(v->db, child_fk->def->name);
- if (fk_name_dup == NULL)
- return;
- vdbe_emit_fk_constraint_drop(parse_context, fk_name_dup,
+ vdbe_emit_fk_constraint_drop(parse_context, child_fk->def->name,
space->def);
}
/* Delete all CK constraints. */
@@ -2085,28 +2079,38 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_normalized_name_region_new(&parse_context->region,
+ drop_def->name.z,
+ drop_def->name.n);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def);
/*
* We account changes to row count only if drop of
* foreign keys take place in a separate
@@ -2114,6 +2118,24 @@ sql_drop_foreign_key(struct Parse *parse_context)
* DROP TABLE always returns 1 (one) as a row count.
*/
struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ vdbe_emit_index_drop(parse_context, name, space->def,
+ ER_NO_SUCH_CONSTRAINT, false);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
+ break;
+ default:
+ unreachable();
+ }
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..cb0ecd2fc 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -265,7 +265,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +408,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..1579cc92e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3741,13 +3741,16 @@ void
sql_create_foreign_key(struct Parse *parse_context);
/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ *
* Function called from parser to handle
* <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement.
*
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..bcdc46a1c 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,87 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+ | ---
+ | - null
+ | - Constraint 'NON_CONSTRAINT' does not exist in space 'T2'
+ | ...
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+ | ---
+ | - row_count: 1
+ | ...
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;')
+ | ---
+ | - null
+ | - Constraint 'NON_EXISTING_CONSTRAINT' does not exist in space 'T2'
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..a9fa4ccc9 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,34 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;')
+
--
-- Cleanup.
--
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints
2020-03-03 10:12 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
` (2 preceding siblings ...)
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop Roman Khabibov
@ 2020-03-03 22:13 ` Vladislav Shpilevoy
2020-03-05 6:32 ` Kirill Yukhin
2020-03-04 21:09 ` Nikita Pettik
4 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-03 22:13 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
Hi! Thanks for the patchset!
On 03/03/2020 11:12, Roman Khabibov wrote:
> @ChangeLog
> - Add ability to drop any table constraint with
> <ALTER TABLE ... DROP CONSTRAINT ...>.
> - Display table name in "no such constraint" error message.
Almost ok. But template is different:
@ChangeLog
- Text description,
probably multiline (gh-####).
Also you seem to be very consistent with forgetting to put issue
and branch links to your patchsets. Please, create a text document
with a list of things to check before sending a patch (issue and
branch links, changelog, check that the branch is pushed, etc),
save it somewhere, and look at it before a send. I have such
document, and it helps sometimes.
LGTM.
Nikita, your turn.
> Roman Khabibov (3):
> sql: improve "no such constraint" error message
> sql: don't select from _index during parsing
> sql: support constraint drop
>
> src/box/constraint_id.h | 1 +
> src/box/errcode.h | 2 +-
> src/box/space.h | 15 ++++
> src/box/sql/alter.c | 2 +-
> src/box/sql/build.c | 145 ++++++++++++++++++++++-------------
> src/box/sql/parse.y | 4 +-
> src/box/sql/parse_def.h | 11 +--
> src/box/sql/pragma.c | 8 +-
> src/box/sql/sqlInt.h | 7 +-
> src/box/sql/vdbe.c | 10 ++-
> test/sql-tap/alter2.test.lua | 2 +-
> test/sql/checks.result | 2 +-
> test/sql/constraint.result | 81 +++++++++++++++++++
> test/sql/constraint.test.lua | 28 +++++++
> 14 files changed, 241 insertions(+), 77 deletions(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints
2020-03-03 22:13 ` [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Vladislav Shpilevoy
@ 2020-03-05 6:32 ` Kirill Yukhin
0 siblings, 0 replies; 11+ messages in thread
From: Kirill Yukhin @ 2020-03-05 6:32 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hello,
On 03 мар 23:13, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
>
> On 03/03/2020 11:12, Roman Khabibov wrote:
> > @ChangeLog
> > - Add ability to drop any table constraint with
> > <ALTER TABLE ... DROP CONSTRAINT ...>.
> > - Display table name in "no such constraint" error message.
>
> Almost ok. But template is different:
>
> @ChangeLog
> - Text description,
> probably multiline (gh-####).
And please, no line breaks.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints
2020-03-03 10:12 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
` (3 preceding siblings ...)
2020-03-03 22:13 ` [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Vladislav Shpilevoy
@ 2020-03-04 21:09 ` Nikita Pettik
4 siblings, 0 replies; 11+ messages in thread
From: Nikita Pettik @ 2020-03-04 21:09 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches, v.shpilevoy
On 03 Mar 13:12, Roman Khabibov wrote:
> @ChangeLog
> - Add ability to drop any table constraint with
> <ALTER TABLE ... DROP CONSTRAINT ...>.
> - Display table name in "no such constraint" error message.
Pushed to master, updated changelog. Thanks.
> Roman Khabibov (3):
> sql: improve "no such constraint" error message
> sql: don't select from _index during parsing
> sql: support constraint drop
>
> src/box/constraint_id.h | 1 +
> src/box/errcode.h | 2 +-
> src/box/space.h | 15 ++++
> src/box/sql/alter.c | 2 +-
> src/box/sql/build.c | 145 ++++++++++++++++++++++-------------
> src/box/sql/parse.y | 4 +-
> src/box/sql/parse_def.h | 11 +--
> src/box/sql/pragma.c | 8 +-
> src/box/sql/sqlInt.h | 7 +-
> src/box/sql/vdbe.c | 10 ++-
> test/sql-tap/alter2.test.lua | 2 +-
> test/sql/checks.result | 2 +-
> test/sql/constraint.result | 81 +++++++++++++++++++
> test/sql/constraint.test.lua | 28 +++++++
> 14 files changed, 241 insertions(+), 77 deletions(-)
>
> --
> 2.21.0 (Apple Git-122)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Tarantool-patches] [PATCH] sql: support constraint drop
@ 2020-01-09 10:15 Roman Khabibov
2020-02-20 23:09 ` Vladislav Shpilevoy
0 siblings, 1 reply; 11+ messages in thread
From: Roman Khabibov @ 2020-01-09 10:15 UTC (permalink / raw)
To: tarantool-patches
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
---
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4120-drop-constraint
Issue: https://github.com/tarantool/tarantool/issues/4120
src/box/errcode.h | 2 +-
src/box/sql/alter.c | 2 +-
src/box/sql/build.c | 127 ++++++++++++++++++++---------------
src/box/sql/parse.y | 4 +-
src/box/sql/parse_def.h | 15 ++---
src/box/sql/sqlInt.h | 4 +-
test/sql-tap/alter2.test.lua | 2 +-
test/sql/checks.result | 2 +-
test/sql/constraint.result | 74 ++++++++++++++++++++
test/sql/constraint.test.lua | 37 ++++++++++
10 files changed, 197 insertions(+), 72 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 3065b1948..9ba42dfb4 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -221,7 +221,7 @@ struct errcode_record {
/*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
/*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
/*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
- /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
+ /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
/*170 */_(ER_CONSTRAINT_EXISTS, "%s constraint '%s' already exists in space '%s'") \
/*171 */_(ER_SQL_TYPE_MISMATCH, "Type mismatch: can not convert %s to %s") \
/*172 */_(ER_ROWID_OVERFLOW, "Rowid is overflowed: too many entries in ephemeral space") \
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 973b420cf..14f6c1a97 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -121,7 +121,7 @@ sql_alter_ck_constraint_enable(struct Parse *parse)
int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2);
sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0,
sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name), P4_DYNAMIC);
+ constraint_name, tbl_name), P4_DYNAMIC);
sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
sqlVdbeJumpHere(v, addr);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bc50ecbfa..7c4b5760e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
sql_table_delete_from(parse, src_list, where);
}
+/**
+ * Generate VDBE program to remove entry with @a index_id and @a
+ * space_id from _index space.
+ */
+static void
+vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
+ uint32_t space_id)
+{
+ struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ int record_reg = ++parse_context->nMem;
+ int space_id_reg = ++parse_context->nMem;
+ int index_id_reg = ++parse_context->nMem;
+ sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
+ sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
+ sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
+ sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ if (index_id == 0)
+ VdbeComment((v, "Remove primary key", index_id));
+ else
+ VdbeComment((v, "Remove secondary index iid = %u", index_id));
+}
+
/**
* Generate VDBE program to remove entry from _fk_constraint space.
*
@@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
P4_DYNAMIC);
sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
- const char *error_msg =
- tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
- constraint_name);
- if (vdbe_emit_halt_with_presence_test(parse_context,
- BOX_FK_CONSTRAINT_ID, 0,
- key_reg, 2, ER_NO_SUCH_CONSTRAINT,
- error_msg, false,
- OP_Found) != 0) {
- sqlDbFree(parse_context->db, constraint_name);
- return;
- }
sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
@@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
sqlDbStrDup(db, ck_name), P4_DYNAMIC);
- const char *error_msg =
- 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) != 0)
- return;
sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((v, "Delete CK constraint %s", ck_name));
@@ -1617,7 +1623,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
*/
int idx_rec_reg = ++parse_context->nMem;
int space_id_reg = ++parse_context->nMem;
- int index_id_reg = ++parse_context->nMem;
int space_id = space->def->id;
sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
@@ -1680,23 +1685,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
* secondary exist.
*/
for (uint32_t i = 1; i < index_count; ++i) {
- sqlVdbeAddOp2(v, OP_Integer,
- space->index[i]->def->iid,
- index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord,
- space_id_reg, 2, idx_rec_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID,
- idx_rec_reg);
- VdbeComment((v,
- "Remove secondary index iid = %u",
- space->index[i]->def->iid));
+ vdbe_emit_index_drop(parse_context,
+ space->index[i]->def->iid,
+ space_id);
}
}
- sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2,
- idx_rec_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg);
- VdbeComment((v, "Remove primary index"));
+ vdbe_emit_index_drop(parse_context, 0, space_id);
}
/* Delete records about the space from the _truncate. */
sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg);
@@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_name_from_token(parse_context->db, &drop_def->name);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def->id);
+ uint32_t space_id = space->def->id;
+ assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
+ CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
+ id->type == CONSTRAINT_TYPE_CK);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ uint32_t index_id = box_index_id_by_name(space_id, name,
+ strlen(name));
+ /*
+ * We have already sure, that this index exists,
+ * so we don't check index_id for BOX_ID_NIL.
+ */
+ vdbe_emit_index_drop(parse_context, index_id, space_id);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space_id);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
+ break;
+ default:
+ unreachable();
+ }
/*
* We account changes to row count only if drop of
* foreign keys take place in a separate
@@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context)
goto exit_drop_index;
}
- /*
- * Generate code to remove entry from _index space
- * But firstly, delete statistics since schema
- * changes after DDL.
- */
- int record_reg = ++parse_context->nMem;
- int space_id_reg = ++parse_context->nMem;
- int index_id_reg = ++parse_context->nMem;
- sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
- sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
- sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
- sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+ vdbe_emit_index_drop(parse_context, index_id, space->def->id);
+ /* Delete statistics since schema changes after DDL. */
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
exit_drop_index:
sqlSrcListDelete(db, table_list);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..e6d45b256 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -159,10 +159,6 @@ enum entity_type {
ENTITY_TYPE_TRIGGER,
ENTITY_TYPE_CK,
ENTITY_TYPE_FK,
- /**
- * For assertion checks that constraint definition is
- * created before initialization of a term constraint.
- */
ENTITY_TYPE_CONSTRAINT,
};
@@ -265,7 +261,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +404,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..2ea653c7d 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3747,7 +3747,7 @@ sql_create_foreign_key(struct Parse *parse_context);
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index 3e21a5f40..759acc9c3 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -256,7 +256,7 @@ test:do_catchsql_test(
ALTER TABLE child DROP CONSTRAINT fake;
]], {
-- <alter2-5.2>
- 1, "Constraint FAKE does not exist"
+ 1, "Constraint 'FAKE' does not exist in space 'CHILD'"
-- </alter2-5.2>
})
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 4ae0b4c10..7b18e5d6b 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -801,7 +801,7 @@ box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);')
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"")
---
- null
-- Constraint falsch does not exist
+- Constraint 'falsch' does not exist in space 'TEST'
...
box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"")
---
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..73bdec45a 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,80 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop PRIMARY KEY constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - null
+ | - Can't drop primary key in space 'T2' while secondary keys exist
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+-- Drop UNIQUE constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - null
+ | - Constraint 'E' does not exist in space 'T2'
+ | ...
+
+--
+-- Ensure that constraint name dropped from/added into the
+-- internal space hash table during a transaction.
+--
+box.begin() \
+box.execute('CREATE UNIQUE INDEX e ON t2(i);') \
+-- Drop UNIQUE constraint. \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop CHECK constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop FOREIGN KEY constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+box.commit()
+ | ---
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..0cada5f09 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,43 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop PRIMARY KEY constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C ~= nil
+-- Drop UNIQUE constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.ck_constraint.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.ck_constraint.E == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+-- Check the error message.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+
+--
+-- Ensure that constraint name dropped from/added into the
+-- internal space hash table during a transaction.
+--
+box.begin() \
+box.execute('CREATE UNIQUE INDEX e ON t2(i);') \
+-- Drop UNIQUE constraint. \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop CHECK constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+-- Drop FOREIGN KEY constraint. \
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);') \
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
+box.commit()
+
--
-- Cleanup.
--
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: support constraint drop
2020-01-09 10:15 [Tarantool-patches] [PATCH] sql: support constraint drop Roman Khabibov
@ 2020-02-20 23:09 ` Vladislav Shpilevoy
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-20 23:09 UTC (permalink / raw)
To: Roman Khabibov, tarantool-patches, n.pettik
Hi! Thanks for the patch!
> Extend <ALTER TABLE> statement to drop table constraints by their
> names.
>
> Closes #4120
>
> @TarantoolBot document
> Title: Drop table constraints in SQL
> Now, it is possible to drop table constraints (PRIMARY KEY,
> UNIQUE, FOREIGN KEY, CHECK) using
> <ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
> by their names.
>
> For example:
>
> tarantool> box.execute([[CREATE TABLE test (
> a INTEGER PRIMARY KEY,
> b INTEGER,
> CONSTRAINT cnstr CHECK (a >= 0)
> );]])
> ---
> - row_count: 1
> ...
>
> tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
> ---
> - row_count: 1
> ...
>
> The same for all the other constraints.
>
1. Please, add a @ChangeLog record. Not to the commit message, but
in a separate mail. I guess, Kirill will search for '@ChangeLog'
in the mail history.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d9bf8de91..76ad79350 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
> is_deferred;
> }
>
> +/**
> + * Emit code to drop the entry from _index or _ck_contstraint or
> + * _fk_constraint space corresponding with the constraint type.
> + */
> void
> -sql_drop_foreign_key(struct Parse *parse_context)
> +sql_drop_constraint(struct Parse *parse_context)
> {
> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
> + struct drop_entity_def *drop_def =
> + &parse_context->drop_constraint_def.base;
> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
> const char *table_name = drop_def->base.entity_name->a[0].zName;
> assert(table_name != NULL);
> - struct space *child = space_by_name(table_name);
> - if (child == NULL) {
> + struct space *space = space_by_name(table_name);
> + if (space == NULL) {
> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - char *constraint_name =
> - sql_name_from_token(parse_context->db, &drop_def->name);
> - if (constraint_name == NULL) {
> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
> + if (name == NULL) {
> + parse_context->is_aborted = true;
> + return;
> + }
> + struct constraint_id *id = space_find_constraint_id(space, name);
> + if (id == NULL) {
2. We perhaps need to do that at runtime - search in fk, ck, and index
spaces, or somehow in this hash table. Because otherwise we rely on
having struct space object, which in theory won't always be the case.
However, not in scope of this patch maybe. Because anyway we have
struct space used in lots of other places. And I don't know a general
solution how to get rid of all of them. Yet.
> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> - child->def);
> - /*
> - * We account changes to row count only if drop of
> - * foreign keys take place in a separate
> - * ALTER TABLE DROP CONSTRAINT statement, since whole
> - * DROP TABLE always returns 1 (one) as a row count.
> - */
> struct Vdbe *v = sqlGetVdbe(parse_context);
> + assert(v != NULL);
> + assert(id->type < constraint_type_MAX);
> + switch (id->type) {
> + case CONSTRAINT_TYPE_PK:
> + case CONSTRAINT_TYPE_UNIQUE: {
> + uint32_t index_id = box_index_id_by_name(space->def->id, name,
> + strlen(name));
3. This is definitely not ok. It is not just looking at space *, it is a
select during parsing. _index has a unique index by space id and name, so
you can emit a deletion opcode without learning an index id. It shouldn't
be hard.
> + /*
> + * We have already verified, that this index
> + * exists, so we don't check index_id for
> + * BOX_ID_NIL.
> + */
4. You are going to need to do that when you will emit deletion opcode
by name. You can't check whether the index exists during compilation.
> + assert(index_id != BOX_ID_NIL);
> + int record_reg = ++parse_context->nMem;
> + int space_id_reg = ++parse_context->nMem;
> + int index_id_reg = ++parse_context->nMem;
> + sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> + const char *error_msg =
> + tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> + id->name, space->def->name);
> + if (vdbe_emit_halt_with_presence_test(parse_context,
> + BOX_INDEX_ID, 0,
> + space_id_reg, 2,
> + ER_NO_SUCH_CONSTRAINT,
> + error_msg, false,
> + OP_Found) != 0)
> + return;
> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + break;
> + }> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
> index 163f4309c..12f673dac 100755
> --- a/test/sql/constraint.test.lua
> +++ b/test/sql/constraint.test.lua
> @@ -131,6 +131,28 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
> -- uniqueness, index names should be unique in a space.
> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>
> +--
> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
> +-- correctly for each type of constraint.
> +--
> +-- Drop UNIQUE constraint.
> +box.space.T2.index.E ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.index.E == nil
> +-- Drop PRIMARY KEY constraint named "C".
> +box.execute('DROP INDEX d ON t2;')
> +box.space.T2.index.C ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
> +box.space.T2.index.C == nil
> +-- Drop CHECK constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
> +box.space.T2.ck_constraint.E ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.ck_constraint.E == nil
> +-- Drop FOREIGN KEY constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +
5. Please, add tests for deletion of not existing
constraints; for deletion of non-constraint objects
(non-unique index).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-02-20 23:09 ` Vladislav Shpilevoy
@ 2020-02-29 12:47 ` Roman Khabibov
2020-02-29 15:32 ` Vladislav Shpilevoy
0 siblings, 1 reply; 11+ messages in thread
From: Roman Khabibov @ 2020-02-29 12:47 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Feb 21, 2020, at 02:09, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Hi! Thanks for the patch!
>
>> Extend <ALTER TABLE> statement to drop table constraints by their
>> names.
>>
>> Closes #4120
>>
>> @TarantoolBot document
>> Title: Drop table constraints in SQL
>> Now, it is possible to drop table constraints (PRIMARY KEY,
>> UNIQUE, FOREIGN KEY, CHECK) using
>> <ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
>> by their names.
>>
>> For example:
>>
>> tarantool> box.execute([[CREATE TABLE test (
>> a INTEGER PRIMARY KEY,
>> b INTEGER,
>> CONSTRAINT cnstr CHECK (a >= 0)
>> );]])
>> ---
>> - row_count: 1
>> ...
>>
>> tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
>> ---
>> - row_count: 1
>> ...
>>
>> The same for all the other constraints.
>>
>
> 1. Please, add a @ChangeLog record. Not to the commit message, but
> in a separate mail. I guess, Kirill will search for '@ChangeLog'
> in the mail history.
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index d9bf8de91..76ad79350 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>> is_deferred;
>> }
>>
>> +/**
>> + * Emit code to drop the entry from _index or _ck_contstraint or
>> + * _fk_constraint space corresponding with the constraint type.
>> + */
>> void
>> -sql_drop_foreign_key(struct Parse *parse_context)
>> +sql_drop_constraint(struct Parse *parse_context)
>> {
>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>> + struct drop_entity_def *drop_def =
>> + &parse_context->drop_constraint_def.base;
>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>> assert(table_name != NULL);
>> - struct space *child = space_by_name(table_name);
>> - if (child == NULL) {
>> + struct space *space = space_by_name(table_name);
>> + if (space == NULL) {
>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>> parse_context->is_aborted = true;
>> return;
>> }
>> - char *constraint_name =
>> - sql_name_from_token(parse_context->db, &drop_def->name);
>> - if (constraint_name == NULL) {
>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>> + if (name == NULL) {
>> + parse_context->is_aborted = true;
>> + return;
>> + }
>> + struct constraint_id *id = space_find_constraint_id(space, name);
>> + if (id == NULL) {
>
> 2. We perhaps need to do that at runtime - search in fk, ck, and index
> spaces, or somehow in this hash table. Because otherwise we rely on
> having struct space object, which in theory won't always be the case.
> which in theory won't always be the case.
Why? Can you, please, explain?
> However, not in scope of this patch maybe. Because anyway we have
> struct space used in lots of other places. And I don't know a general
> solution how to get rid of all of them. Yet.
Do you mean to avoid struct space usage within build.c?
>> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
>> parse_context->is_aborted = true;
>> return;
>> }
>> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
>> - child->def);
>> - /*
>> - * We account changes to row count only if drop of
>> - * foreign keys take place in a separate
>> - * ALTER TABLE DROP CONSTRAINT statement, since whole
>> - * DROP TABLE always returns 1 (one) as a row count.
>> - */
>> struct Vdbe *v = sqlGetVdbe(parse_context);
>> + assert(v != NULL);
>> + assert(id->type < constraint_type_MAX);
>> + switch (id->type) {
>> + case CONSTRAINT_TYPE_PK:
>> + case CONSTRAINT_TYPE_UNIQUE: {
>> + uint32_t index_id = box_index_id_by_name(space->def->id, name,
>> + strlen(name));
>
> 3. This is definitely not ok. It is not just looking at space *, it is a
> select during parsing. _index has a unique index by space id and name, so
> you can emit a deletion opcode without learning an index id. It shouldn't
> be hard.
>
>> + /*
>> + * We have already verified, that this index
>> + * exists, so we don't check index_id for
>> + * BOX_ID_NIL.
>> + */
>
> 4. You are going to need to do that when you will emit deletion opcode
> by name. You can't check whether the index exists during compilation.
See the patch in the patch set.
>> + assert(index_id != BOX_ID_NIL);
>> + int record_reg = ++parse_context->nMem;
>> + int space_id_reg = ++parse_context->nMem;
>> + int index_id_reg = ++parse_context->nMem;
>> + sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
>> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
>> + const char *error_msg =
>> + tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
>> + id->name, space->def->name);
>> + if (vdbe_emit_halt_with_presence_test(parse_context,
>> + BOX_INDEX_ID, 0,
>> + space_id_reg, 2,
>> + ER_NO_SUCH_CONSTRAINT,
>> + error_msg, false,
>> + OP_Found) != 0)
>> + return;
>> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
>> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
>> + break;
>> + }> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
>> index 163f4309c..12f673dac 100755
>> --- a/test/sql/constraint.test.lua
>> +++ b/test/sql/constraint.test.lua
>> @@ -131,6 +131,28 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
>> -- uniqueness, index names should be unique in a space.
>> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>>
>> +--
>> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
>> +-- correctly for each type of constraint.
>> +--
>> +-- Drop UNIQUE constraint.
>> +box.space.T2.index.E ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +box.space.T2.index.E == nil
>> +-- Drop PRIMARY KEY constraint named "C".
>> +box.execute('DROP INDEX d ON t2;')
>> +box.space.T2.index.C ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
>> +box.space.T2.index.C == nil
>> +-- Drop CHECK constraint.
>> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
>> +box.space.T2.ck_constraint.E ~= nil
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +box.space.T2.ck_constraint.E == nil
>> +-- Drop FOREIGN KEY constraint.
>> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
>> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
>> +
>
> 5. Please, add tests for deletion of not existing
> constraints; for deletion of non-constraint objects
> (non-unique index).
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..a9fa4ccc9 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,34 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;’)
+
sql: support constraint drop
Extend <ALTER TABLE> statement to drop table constraints by their
names.
Closes #4120
@TarantoolBot document
Title: Drop table constraints in SQL
Now, it is possible to drop table constraints (PRIMARY KEY,
UNIQUE, FOREIGN KEY, CHECK) using
<ALTER TABLE table_name DROP CONSTRAINT constraint_name> statement
by their names.
For example:
tarantool> box.execute([[CREATE TABLE test (
a INTEGER PRIMARY KEY,
b INTEGER,
CONSTRAINT cnstr CHECK (a >= 0)
);]])
---
- row_count: 1
...
tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;')
---
- row_count: 1
...
The same for all the other constraints.
---
src/box/constraint_id.h | 1 +
src/box/sql/build.c | 63 ++++++++++++++++++----------
src/box/sql/parse.y | 4 +-
src/box/sql/parse_def.h | 11 ++---
src/box/sql/sqlInt.h | 7 +++-
test/sql/constraint.result | 81 ++++++++++++++++++++++++++++++++++++
test/sql/constraint.test.lua | 28 +++++++++++++
7 files changed, 165 insertions(+), 30 deletions(-)
diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h
index 21f067cdc..ed4f37426 100755
--- a/src/box/constraint_id.h
+++ b/src/box/constraint_id.h
@@ -40,6 +40,7 @@ enum constraint_type {
CONSTRAINT_TYPE_UNIQUE,
CONSTRAINT_TYPE_FK,
CONSTRAINT_TYPE_CK,
+ constraint_type_MAX,
};
extern const char *constraint_type_strs[];
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 00877b7d8..17fd07f78 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1508,8 +1508,6 @@ vdbe_emit_index_drop(struct Parse *parse_context, const char *name,
*
* @param parse_context Parsing context.
* @param constraint_name Name of FK constraint to be dropped.
- * Must be allocated on head by sqlDbMalloc().
- * It will be freed in VDBE.
* @param child_def Def of table which constraint belongs to.
*/
static void
@@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
struct Vdbe *vdbe = sqlGetVdbe(parse_context);
assert(vdbe != NULL);
int key_reg = sqlGetTempRange(parse_context, 3);
- sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
+ const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
+ sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
P4_DYNAMIC);
sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
const char *error_msg =
@@ -1529,10 +1528,8 @@ 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) != 0) {
- sqlDbFree(parse_context->db, constraint_name);
+ OP_Found) != 0)
return;
- }
sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
@@ -1689,11 +1686,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
struct fk_constraint *child_fk;
rlist_foreach_entry(child_fk, &space->child_fk_constraint,
in_child_space) {
-
- char *fk_name_dup = sqlDbStrDup(v->db, child_fk->def->name);
- if (fk_name_dup == NULL)
- return;
- vdbe_emit_fk_constraint_drop(parse_context, fk_name_dup,
+ vdbe_emit_fk_constraint_drop(parse_context, child_fk->def->name,
space->def);
}
/* Delete all CK constraints. */
@@ -2086,28 +2079,38 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
is_deferred;
}
+/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ */
void
-sql_drop_foreign_key(struct Parse *parse_context)
+sql_drop_constraint(struct Parse *parse_context)
{
- struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
- assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
+ struct drop_entity_def *drop_def =
+ &parse_context->drop_constraint_def.base;
+ assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
const char *table_name = drop_def->base.entity_name->a[0].zName;
assert(table_name != NULL);
- struct space *child = space_by_name(table_name);
- if (child == NULL) {
+ struct space *space = space_by_name(table_name);
+ if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
parse_context->is_aborted = true;
return;
}
- char *constraint_name =
- sql_name_from_token(parse_context->db, &drop_def->name);
- if (constraint_name == NULL) {
+ char *name = sql_normalized_name_region_new(&parse_context->region,
+ drop_def->name.z,
+ drop_def->name.n);
+ if (name == NULL) {
+ parse_context->is_aborted = true;
+ return;
+ }
+ struct constraint_id *id = space_find_constraint_id(space, name);
+ if (id == NULL) {
+ diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
parse_context->is_aborted = true;
return;
}
- vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
- child->def);
/*
* We account changes to row count only if drop of
* foreign keys take place in a separate
@@ -2115,6 +2118,24 @@ sql_drop_foreign_key(struct Parse *parse_context)
* DROP TABLE always returns 1 (one) as a row count.
*/
struct Vdbe *v = sqlGetVdbe(parse_context);
+ assert(v != NULL);
+ assert(id->type < constraint_type_MAX);
+ switch (id->type) {
+ case CONSTRAINT_TYPE_PK:
+ case CONSTRAINT_TYPE_UNIQUE: {
+ vdbe_emit_index_drop(parse_context, name, space->def,
+ ER_NO_SUCH_CONSTRAINT, false);
+ break;
+ }
+ case CONSTRAINT_TYPE_FK:
+ vdbe_emit_fk_constraint_drop(parse_context, name, space->def);
+ break;
+ case CONSTRAINT_TYPE_CK:
+ vdbe_emit_ck_constraint_drop(parse_context, name, space->def);
+ break;
+ default:
+ unreachable();
+ }
sqlVdbeCountChanges(v);
sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cfe1c0012..1a0e89703 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1763,9 +1763,9 @@ cmd ::= alter_table_start(A) RENAME TO nm(N). {
}
cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
- drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false);
+ drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false);
pParse->initiateTTrans = true;
- sql_drop_foreign_key(pParse);
+ sql_drop_constraint(pParse);
}
cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 2f433e4c0..cb0ecd2fc 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -265,7 +265,7 @@ struct drop_trigger_def {
struct drop_entity_def base;
};
-struct drop_fk_def {
+struct drop_constraint_def {
struct drop_entity_def base;
};
@@ -408,11 +408,12 @@ drop_trigger_def_init(struct drop_trigger_def *drop_trigger_def,
}
static inline void
-drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList *parent_name,
- struct Token *name, bool if_exist)
+drop_constraint_def_init(struct drop_constraint_def *drop_constraint_def,
+ struct SrcList *parent_name, struct Token *name,
+ bool if_exist)
{
- drop_entity_def_init(&drop_fk_def->base, parent_name, name, if_exist,
- ENTITY_TYPE_FK);
+ drop_entity_def_init(&drop_constraint_def->base, parent_name, name,
+ if_exist, ENTITY_TYPE_CONSTRAINT);
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d1fcf4761..1579cc92e 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2237,7 +2237,7 @@ struct Parse {
struct create_trigger_def create_trigger_def;
struct create_view_def create_view_def;
struct rename_entity_def rename_entity_def;
- struct drop_fk_def drop_fk_def;
+ struct drop_constraint_def drop_constraint_def;
struct drop_index_def drop_index_def;
struct drop_table_def drop_table_def;
struct drop_trigger_def drop_trigger_def;
@@ -3741,13 +3741,16 @@ void
sql_create_foreign_key(struct Parse *parse_context);
/**
+ * Emit code to drop the entry from _index or _ck_contstraint or
+ * _fk_constraint space corresponding with the constraint type.
+ *
* Function called from parser to handle
* <ALTER TABLE table DROP CONSTRAINT constraint> SQL statement.
*
* @param parse_context Parsing context.
*/
void
-sql_drop_foreign_key(struct Parse *parse_context);
+sql_drop_constraint(struct Parse *parse_context);
/**
* Now our SQL implementation can't operate on spaces which
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index 1585c2327..bcdc46a1c 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -291,6 +291,87 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);')
| - Index 'D' already exists in space 'T2'
| ...
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+ | ---
+ | - null
+ | - Constraint 'NON_CONSTRAINT' does not exist in space 'T2'
+ | ...
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E == nil
+ | ---
+ | - true
+ | ...
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.C == nil
+ | ---
+ | - true
+ | ...
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+ | ---
+ | - true
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+ | ---
+ | - true
+ | ...
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+ | ---
+ | - row_count: 1
+ | ...
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;')
+ | ---
+ | - null
+ | - Constraint 'NON_EXISTING_CONSTRAINT' does not exist in space 'T2'
+ | ...
+
--
-- Cleanup.
--
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
index 163f4309c..a9fa4ccc9 100755
--- a/test/sql/constraint.test.lua
+++ b/test/sql/constraint.test.lua
@@ -131,6 +131,34 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
-- uniqueness, index names should be unique in a space.
box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+--
+-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
+-- correctly for each type of constraint.
+--
+-- Drop non-constraint object (non-unique index).
+box.execute('CREATE INDEX non_constraint ON t2(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_constraint;')
+-- Drop UNIQUE constraint.
+box.space.T2.index.E ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
+box.space.T2.index.E == nil
+-- Drop PRIMARY KEY constraint named "C".
+box.execute('DROP INDEX non_constraint ON t2;')
+box.execute('DROP INDEX d ON t2;')
+box.space.T2.index.C ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
+box.space.T2.index.C == nil
+-- Drop CHECK constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT ck_constraint CHECK(i > 0);')
+box.space.T2.ck_constraint.CK_CONSTRAINT ~= nil
+box.execute('ALTER TABLE t2 DROP CONSTRAINT ck_constraint;')
+box.space.T2.ck_constraint.CK_CONSTRAINT == nil
+-- Drop FOREIGN KEY constraint.
+box.execute('ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT fk;')
+-- Drop non-existing constraint.
+box.execute('ALTER TABLE t2 DROP CONSTRAINT non_existing_constraint;')
+
--
-- Cleanup.
--
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
@ 2020-02-29 15:32 ` Vladislav Shpilevoy
2020-03-03 10:13 ` Roman Khabibov
0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-29 15:32 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
Thanks for the patch!
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index d9bf8de91..76ad79350 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>>> is_deferred;
>>> }
>>>
>>> +/**
>>> + * Emit code to drop the entry from _index or _ck_contstraint or
>>> + * _fk_constraint space corresponding with the constraint type.
>>> + */
>>> void
>>> -sql_drop_foreign_key(struct Parse *parse_context)
>>> +sql_drop_constraint(struct Parse *parse_context)
>>> {
>>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>>> + struct drop_entity_def *drop_def =
>>> + &parse_context->drop_constraint_def.base;
>>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>>> assert(table_name != NULL);
>>> - struct space *child = space_by_name(table_name);
>>> - if (child == NULL) {
>>> + struct space *space = space_by_name(table_name);
>>> + if (space == NULL) {
>>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>>> parse_context->is_aborted = true;
>>> return;
>>> }
>>> - char *constraint_name =
>>> - sql_name_from_token(parse_context->db, &drop_def->name);
>>> - if (constraint_name == NULL) {
>>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>>> + if (name == NULL) {
>>> + parse_context->is_aborted = true;
>>> + return;
>>> + }
>>> + struct constraint_id *id = space_find_constraint_id(space, name);
>>> + if (id == NULL) {
>>
>> 2. We perhaps need to do that at runtime - search in fk, ck, and index
>> spaces, or somehow in this hash table. Because otherwise we rely on
>> having struct space object, which in theory won't always be the case.
>> which in theory won't always be the case.
> Why? Can you, please, explain?
Remote client nodes do not have any schema objects. If a request
requires struct space *, it can't be compiled on the client. And
this is where we are going to.
>> However, not in scope of this patch maybe. Because anyway we have
>> struct space used in lots of other places. And I don't know a general
>> solution how to get rid of all of them. Yet.
> Do you mean to avoid struct space usage within build.c?
Yes, I want us to avoid any schema objects usage where possible.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 00877b7d8..17fd07f78 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
constraint_name can become 'const' now.
> struct Vdbe *vdbe = sqlGetVdbe(parse_context);
> assert(vdbe != NULL);
> int key_reg = sqlGetTempRange(parse_context, 3);
> - sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> + const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
> + sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
> P4_DYNAMIC);
> sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
> const char *error_msg =
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop
2020-02-29 15:32 ` Vladislav Shpilevoy
@ 2020-03-03 10:13 ` Roman Khabibov
0 siblings, 0 replies; 11+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:13 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review.
> On Feb 29, 2020, at 18:32, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch!
>
>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>> index d9bf8de91..76ad79350 100644
>>>> --- a/src/box/sql/build.c
>>>> +++ b/src/box/sql/build.c
>>>> @@ -2052,35 +2052,78 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
>>>> is_deferred;
>>>> }
>>>>
>>>> +/**
>>>> + * Emit code to drop the entry from _index or _ck_contstraint or
>>>> + * _fk_constraint space corresponding with the constraint type.
>>>> + */
>>>> void
>>>> -sql_drop_foreign_key(struct Parse *parse_context)
>>>> +sql_drop_constraint(struct Parse *parse_context)
>>>> {
>>>> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
>>>> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
>>>> + struct drop_entity_def *drop_def =
>>>> + &parse_context->drop_constraint_def.base;
>>>> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
>>>> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
>>>> const char *table_name = drop_def->base.entity_name->a[0].zName;
>>>> assert(table_name != NULL);
>>>> - struct space *child = space_by_name(table_name);
>>>> - if (child == NULL) {
>>>> + struct space *space = space_by_name(table_name);
>>>> + if (space == NULL) {
>>>> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
>>>> parse_context->is_aborted = true;
>>>> return;
>>>> }
>>>> - char *constraint_name =
>>>> - sql_name_from_token(parse_context->db, &drop_def->name);
>>>> - if (constraint_name == NULL) {
>>>> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
>>>> + if (name == NULL) {
>>>> + parse_context->is_aborted = true;
>>>> + return;
>>>> + }
>>>> + struct constraint_id *id = space_find_constraint_id(space, name);
>>>> + if (id == NULL) {
>>>
>>> 2. We perhaps need to do that at runtime - search in fk, ck, and index
>>> spaces, or somehow in this hash table. Because otherwise we rely on
>>> having struct space object, which in theory won't always be the case.
>>> which in theory won't always be the case.
>> Why? Can you, please, explain?
>
> Remote client nodes do not have any schema objects. If a request
> requires struct space *, it can't be compiled on the client. And
> this is where we are going to.
>
>>> However, not in scope of this patch maybe. Because anyway we have
>>> struct space used in lots of other places. And I don't know a general
>>> solution how to get rid of all of them. Yet.
>> Do you mean to avoid struct space usage within build.c?
>
> Yes, I want us to avoid any schema objects usage where possible.
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 00877b7d8..17fd07f78 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1519,7 +1517,8 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
>
> constraint_name can become 'const' now.
Done.
>> struct Vdbe *vdbe = sqlGetVdbe(parse_context);
>> assert(vdbe != NULL);
>> int key_reg = sqlGetTempRange(parse_context, 3);
>> - sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
>> + const char *name_copy = sqlDbStrDup(parse_context->db, constraint_name);
>> + sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, name_copy,
>> P4_DYNAMIC);
>> sqlVdbeAddOp2(vdbe, OP_Integer, child_def->id, key_reg + 1);
>> const char *error_msg =
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-03-05 6:32 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 10:12 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
2020-03-03 12:47 ` Roman Khabibov
2020-03-03 10:12 ` [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop Roman Khabibov
2020-03-03 22:13 ` [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Vladislav Shpilevoy
2020-03-05 6:32 ` Kirill Yukhin
2020-03-04 21:09 ` Nikita Pettik
-- strict thread matches above, loose matches on Subject: below --
2020-01-09 10:15 [Tarantool-patches] [PATCH] sql: support constraint drop Roman Khabibov
2020-02-20 23:09 ` Vladislav Shpilevoy
2020-02-29 12:47 ` [Tarantool-patches] [PATCH v2 3/3] " Roman Khabibov
2020-02-29 15:32 ` Vladislav Shpilevoy
2020-03-03 10:13 ` Roman Khabibov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox