[Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints

[Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints

[Roman Khabibov]

@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)

[Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message

[Roman Khabibov]

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)

[Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing

[Roman Khabibov]

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)

[Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop

[Roman Khabibov]

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)

Re: [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing

[Roman Khabibov]

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)

Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints

[Vladislav Shpilevoy]

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(-)
> 

Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints

[Nikita Pettik]

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)
> 

Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints

[Kirill Yukhin]

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

Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop

[Roman Khabibov]

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 =

Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop

[Vladislav Shpilevoy]

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 =

Re: [Tarantool-patches] [PATCH v2 3/3] sql: support constraint drop

[Roman Khabibov]

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)