[Tarantool-patches] [PATCH] sql: support constraint drop
Nikita Pettik
korablev at tarantool.org
Mon Jan 13 20:00:45 MSK 2020
On 09 Jan 13:15, Roman Khabibov wrote:
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 3065b1948..9ba42dfb4 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -221,7 +221,7 @@ struct errcode_record {
> /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not exist") \
> /*167 */_(ER_CREATE_FK_CONSTRAINT, "Failed to create foreign key constraint '%s': %s") \
> /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \
> - /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \
> + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does not exist in space '%s'") \
Why did you change error (in scope of this patch)?
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bc50ecbfa..7c4b5760e 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> sql_table_delete_from(parse, src_list, where);
> }
>
> +/**
> + * Generate VDBE program to remove entry with @a index_id and @a
> + * space_id from _index space.
> + */
> +static void
> +vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
> + uint32_t space_id)
> +{
Instead of passing values of space/index ids, I'd rather pass registers
holding these values. In most cases it allows to avoid duplicating
the same intstruction (which stores id in the given register).
> + struct Vdbe *v = sqlGetVdbe(parse_context);
> + assert(v != NULL);
> + int record_reg = ++parse_context->nMem;
> + int space_id_reg = ++parse_context->nMem;
> + int index_id_reg = ++parse_context->nMem;
> + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + if (index_id == 0)
> + VdbeComment((v, "Remove primary key", index_id));
> + else
> + VdbeComment((v, "Remove secondary index iid = %u", index_id));
Simply without branching: "Remove index with iid = %u"
> +}
> +
> /**
> * Generate VDBE program to remove entry from _fk_constraint space.
> *
> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
> P4_DYNAMIC);
> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1);
> - const char *error_msg =
> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> - constraint_name);
> - if (vdbe_emit_halt_with_presence_test(parse_context,
> - BOX_FK_CONSTRAINT_ID, 0,
> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> - error_msg, false,
> - OP_Found) != 0) {
> - sqlDbFree(parse_context->db, constraint_name);
> - return;
> - }
Why did you drop this check?
> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg);
> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> sqlDbStrDup(db, ck_name), P4_DYNAMIC);
> - const char *error_msg =
> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), ck_name);
> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
> - key_reg, 2, ER_NO_SUCH_CONSTRAINT,
> - error_msg, false, OP_Found) != 0)
> - return;
Same question.
> sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
> sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
> VdbeComment((v, "Delete CK constraint %s", ck_name));
> @@ -1617,7 +1623,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
> */
> int idx_rec_reg = ++parse_context->nMem;
> int space_id_reg = ++parse_context->nMem;
> - int index_id_reg = ++parse_context->nMem;
> int space_id = space->def->id;
> sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
> sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
> @@ -1680,23 +1685,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
> * secondary exist.
> */
> for (uint32_t i = 1; i < index_count; ++i) {
> - sqlVdbeAddOp2(v, OP_Integer,
> - space->index[i]->def->iid,
> - index_id_reg);
> - sqlVdbeAddOp3(v, OP_MakeRecord,
> - space_id_reg, 2, idx_rec_reg);
> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID,
> - idx_rec_reg);
> - VdbeComment((v,
> - "Remove secondary index iid = %u",
> - space->index[i]->def->iid));
> + vdbe_emit_index_drop(parse_context,
> + space->index[i]->def->iid,
> + space_id);
I'd better move this refactoring to a separate patch.
> }
> }
> - sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg);
> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2,
> - idx_rec_reg);
> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg);
> - VdbeComment((v, "Remove primary index"));
> + vdbe_emit_index_drop(parse_context, 0, space_id);
> }
> /* Delete records about the space from the _truncate. */
> sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg);
> @@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse *parse_context, bool is_deferred)
> is_deferred;
> }
>
> +/**
> + * Emit code to drop the entry from _index or _ck_contstraint or
> + * _fk_constraint space corresponding with the constraint type.
> + */
> void
> -sql_drop_foreign_key(struct Parse *parse_context)
> +sql_drop_constraint(struct Parse *parse_context)
> {
> - struct drop_entity_def *drop_def = &parse_context->drop_fk_def.base;
> - assert(drop_def->base.entity_type == ENTITY_TYPE_FK);
> + struct drop_entity_def *drop_def =
> + &parse_context->drop_constraint_def.base;
> + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT);
> assert(drop_def->base.alter_action == ALTER_ACTION_DROP);
> const char *table_name = drop_def->base.entity_name->a[0].zName;
> assert(table_name != NULL);
> - struct space *child = space_by_name(table_name);
> - if (child == NULL) {
> + struct space *space = space_by_name(table_name);
> + if (space == NULL) {
> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - char *constraint_name =
> - sql_name_from_token(parse_context->db, &drop_def->name);
> - if (constraint_name == NULL) {
> + char *name = sql_name_from_token(parse_context->db, &drop_def->name);
> + if (name == NULL) {
> + parse_context->is_aborted = true;
> + return;
> + }
> + struct constraint_id *id = space_find_constraint_id(space, name);
> + if (id == NULL) {
> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name);
> parse_context->is_aborted = true;
> return;
> }
> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
> - child->def->id);
> + uint32_t space_id = space->def->id;
> + assert(id->type == CONSTRAINT_TYPE_PK || id->type ==
> + CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK ||
> + id->type == CONSTRAINT_TYPE_CK);
Instead of such huge checks iterating over all possible variants,
enum can be extended with _MAX last element (see enum index_type for
example) and check becomes quite simple: assert(id->type < contraint_type_MAX);
> + switch (id->type) {
> + case CONSTRAINT_TYPE_PK:
> + case CONSTRAINT_TYPE_UNIQUE: {
> + uint32_t index_id = box_index_id_by_name(space_id, name,
> + strlen(name));
> + /*
> + * We have already sure, that this index exists,
> + * so we don't check index_id for BOX_ID_NIL.
> + */
Then add assert(index_id != BOX_ID_NIL);
> + vdbe_emit_index_drop(parse_context, index_id, space_id);
> + break;
> + }
> + case CONSTRAINT_TYPE_FK:
> + vdbe_emit_fk_constraint_drop(parse_context, name, space_id);
> + break;
> + case CONSTRAINT_TYPE_CK:
> + vdbe_emit_ck_constraint_drop(parse_context, name, space_id);
> + break;
> + default:
> + unreachable();
> + }
> /*
> * We account changes to row count only if drop of
> * foreign keys take place in a separate
> @@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context)
> goto exit_drop_index;
> }
>
> - /*
> - * Generate code to remove entry from _index space
> - * But firstly, delete statistics since schema
> - * changes after DDL.
> - */
> - int record_reg = ++parse_context->nMem;
> - int space_id_reg = ++parse_context->nMem;
> - int index_id_reg = ++parse_context->nMem;
> - sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
> - sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> + vdbe_emit_index_drop(parse_context, index_id, space->def->id);
> + /* Delete statistics since schema changes after DDL. */
Looks like obsolete comment.
> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
> index 163f4309c..0cada5f09 100755
> --- a/test/sql/constraint.test.lua
> +++ b/test/sql/constraint.test.lua
> @@ -131,6 +131,43 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);')
> -- uniqueness, index names should be unique in a space.
> box.execute('CREATE UNIQUE INDEX d ON t2(i);')
>
> +--
> +-- gh-4120: Ensure that <ALTER TABLE DROP CONSTRAINT> works
> +-- correctly for each type of constraint.
> +--
> +-- Drop PRIMARY KEY constraint.
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;')
> +box.space.T2.index.C ~= nil
But PK is not dropped. Please, add tests where PK will be successfully
dropped.
> +-- Drop UNIQUE constraint.
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.index.E == nil
> +-- Drop CHECK constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
> +box.space.T2.ck_constraint.E ~= nil
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +box.space.T2.ck_constraint.E == nil
> +-- Drop FOREIGN KEY constraint.
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);')
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +-- Check the error message.
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;')
> +
> +--
> +-- Ensure that constraint name dropped from/added into the
> +-- internal space hash table during a transaction.
> +--
This patch is about extending SQL syntax, don't get how this check
is related to subj..
> +box.begin() \
> +box.execute('CREATE UNIQUE INDEX e ON t2(i);') \
> +-- Drop UNIQUE constraint. \
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
> +-- Drop CHECK constraint. \
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
> +-- Drop FOREIGN KEY constraint. \
> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES t1(i);') \
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \
> +box.commit()
> +
> --
> -- Cleanup.
> --
> --
> 2.21.0 (Apple Git-122)
>
More information about the Tarantool-patches
mailing list