* [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message
2020-02-29 12:46 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
@ 2020-02-29 12:46 ` Roman Khabibov
2020-02-29 15:31 ` Vladislav Shpilevoy
2020-02-29 12:46 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Roman Khabibov @ 2020-02-29 12:46 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 didn'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 6f6e28c6c..b7787d378 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] 10+ messages in thread* [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing
2020-02-29 12:46 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
2020-02-29 12:46 ` [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message Roman Khabibov
@ 2020-02-29 12:46 ` Roman Khabibov
2020-02-29 15:31 ` Vladislav Shpilevoy
2020-02-29 15:31 ` [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Vladislav Shpilevoy
2020-03-02 12:11 ` Nikita Pettik
3 siblings, 1 reply; 10+ messages in thread
From: Roman Khabibov @ 2020-02-29 12:46 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 | 60 +++++++++++++++++++++++++++-----------------
src/box/sql/pragma.c | 8 ++----
src/box/sql/vdbe.c | 10 +++++---
4 files changed, 60 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..00877b7d8 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,41 @@ 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.
+ * It will be freed in VDBE.
+ * @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 +2729,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] 10+ messages in thread* Re: [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing
2020-02-29 12:46 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
@ 2020-02-29 15:31 ` Vladislav Shpilevoy
2020-03-03 10:15 ` Roman Khabibov
0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-29 15:31 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
Thanks for the patch!
See 2 comments below.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d9bf8de91..00877b7d8 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,41 @@ 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.
> + * It will be freed in VDBE.
1. This easily fits into one line. Also I don't understand what does
it mean 'freed in VDBE'. From what I see, this function has nothing
to do with freeing 'name' argument, as well as VDBE. It is freed
by the caller in the end of sql_drop_index().
> + * @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);
> +}
> 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
> @@ -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);
2. Yeah, ok, good enough. The final goal should be to stop using
even struct space pointer here. Sometime later.
> parse->nMem = 6;
> struct Vdbe *v = sqlGetVdbe(parse);
> assert(v != NULL);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing
2020-02-29 15:31 ` Vladislav Shpilevoy
@ 2020-03-03 10:15 ` Roman Khabibov
0 siblings, 0 replies; 10+ messages in thread
From: Roman Khabibov @ 2020-03-03 10:15 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
> On Feb 29, 2020, at 18:31, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the patch!
>
> See 2 comments below.
>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index d9bf8de91..00877b7d8 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,41 @@ 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.
>> + * It will be freed in VDBE.
>
> 1. This easily fits into one line. Also I don't understand what does
> it mean 'freed in VDBE'. From what I see, this function has nothing
> to do with freeing 'name' argument, as well as VDBE. It is freed
> by the caller in the end of sql_drop_index().
Oops. This line turned out to be a case here.
>> + * @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);
>> +}
>> 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
>> @@ -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);
>
> 2. Yeah, ok, good enough. The final goal should be to stop using
> even struct space pointer here. Sometime later.
>
>> parse->nMem = 6;
>> struct Vdbe *v = sqlGetVdbe(parse);
>> assert(v != NULL);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints
2020-02-29 12:46 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
2020-02-29 12:46 ` [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message Roman Khabibov
2020-02-29 12:46 ` [Tarantool-patches] [PATCH v2 2/3] sql: don't select from _index during parsing Roman Khabibov
@ 2020-02-29 15:31 ` Vladislav Shpilevoy
2020-03-02 12:11 ` Nikita Pettik
3 siblings, 0 replies; 10+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-29 15:31 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
Hi! Thanks for the patchset!
Almost ready to push, good patch. I have very
few nits.
On 29/02/2020 13:46, Roman Khabibov wrote:
> @ChangeLog
> - Add function space_index_by_name().
> - Add ability to spacify searching index in OP_SDelete.
> - Add function vdbe_emit_index_drop() to drop index by name.
> - Remove box_index_by_name() from build.c and pragma.c.
> - Rewrite function sql_drop_foreign_key() to sql_drop_constraint().
> - Add ability to drop all the types of constraints.
> - Modify "no such constrait" error message.
Sorry, this is not a change log which is needed for the
release notes. See examples in
https://github.com/tarantool/tarantool/releases.
Users don't care about internal functions being moved/deleted/
renamed/patched. Also I don't see a word about DROP CONSTRAINT,
which was this ticket about in the first place.
Also I see that you somewhy split the thread in two parts, so
I received only 0/3, 1/3 and 2/3 letters in this thread, while
3/3 was sent in response to the old thread - please, don't do
that. It complicates review process. In an old thread you should
send only answers on comments. All new patches should go in a new
thread, if you decided to start it.
Last thing - the branch is very outdated. Please, rebase it on
the latest master.
> Hello, everybody!
> As Nikita requested, I performed the patch to remove
> box_index_by_name() from build.c and pragma.c, but what about
> analyze.c? Still had to add space_index_by_name() to use in
> pragma.c, beacuse struct index is required.
We can't do anything about analyze, because it is disabled.
> 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 | 143 ++++++++++++++++++++++-------------
> 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, 240 insertions(+), 76 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints
2020-02-29 12:46 [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Roman Khabibov
` (2 preceding siblings ...)
2020-02-29 15:31 ` [Tarantool-patches] [PATCH v2 0/3] Add ability to drop constraints Vladislav Shpilevoy
@ 2020-03-02 12:11 ` Nikita Pettik
3 siblings, 0 replies; 10+ messages in thread
From: Nikita Pettik @ 2020-03-02 12:11 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches, v.shpilevoy
On 29 Feb 15:46, Roman Khabibov wrote:
> @ChangeLog
> - Add function space_index_by_name().
> - Add ability to spacify searching index in OP_SDelete.
> - Add function vdbe_emit_index_drop() to drop index by name.
> - Remove box_index_by_name() from build.c and pragma.c.
> - Rewrite function sql_drop_foreign_key() to sql_drop_constraint().
> - Add ability to drop all the types of constraints.
> - Modify "no such constrait" error message.
>
> Hello, everybody!
> As Nikita requested, I performed the patch to remove
> box_index_by_name() from build.c and pragma.c, but what about
> analyze.c? Still had to add space_index_by_name() to use in
> pragma.c, beacuse struct index is required.
analyze.c now is unreachable. Even if you fix those usages, you won't
be able to test them.
> 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 | 143 ++++++++++++++++++++++-------------
> 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, 240 insertions(+), 76 deletions(-)
>
> --
> 2.21.0 (Apple Git-122)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH v2 1/3] sql: improve "no such constraint" error message
2020-03-03 10:12 Roman Khabibov
@ 2020-03-03 10:12 ` Roman Khabibov
0 siblings, 0 replies; 10+ 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] 10+ messages in thread