[tarantool-patches] Re: [PATCH v3 2/4] sql: an ability to disable CK constraints
Nikita Pettik
korablev at tarantool.org
Wed Sep 25 16:48:02 MSK 2019
On 16 Sep 15:47, Kirill Shcherbatov wrote:
> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 2a59d6a61..3c3de4cca 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -89,6 +89,7 @@ static Keyword aKeywordTable[] = {
> { "DEFERRED", "TK_DEFERRED", false },
> { "DEFERRABLE", "TK_DEFERRABLE", false },
> { "DELETE", "TK_DELETE", true },
> + { "DISABLE", "TK_DISABLE", false },
> { "DESC", "TK_DESC", true },
> { "DISTINCT", "TK_DISTINCT", true },
> { "DROP", "TK_DROP", true },
> @@ -203,6 +204,7 @@ static Keyword aKeywordTable[] = {
> { "DETERMINISTIC", "TK_STANDARD", true },
> { "DOUBLE", "TK_STANDARD", true },
> { "ELSEIF", "TK_STANDARD", true },
> + { "ENABLE", "TK_ENABLE", false },
> { "FETCH", "TK_STANDARD", true },
> { "FLOAT", "TK_STANDARD", true },
> { "FUNCTION", "TK_STANDARD", true },
That's not enough to make keywords be non-reserved. Now I can't
use disable/enable identifiers:
tarantool create table disable(id int primary key)
---
- null
- Syntax error near 'disable'
...
Grep 'fallback ID' in parse.y and add tests verifying that such
keywords can be used as valid indentifiers.
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 765600186..963d77a0a 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -36,6 +36,7 @@
> #include "sqlInt.h"
> #include "box/box.h"
> #include "box/schema.h"
> +#include "box/ck_constraint.h"
>
> void
> sql_alter_table_rename(struct Parse *parse)
> @@ -79,6 +80,43 @@ tnt_error:
> goto exit_rename_table;
> }
>
> +void
> +sql_alter_ck_constraint_enable(struct Parse *parse)
> +{
> + struct enable_entity_def *enable_def = &parse->enable_entity_def;
> + struct SrcList *src_tab = enable_def->base.entity_name;
> + assert(enable_def->base.entity_type == ENTITY_TYPE_CK);
> + assert(enable_def->base.alter_action == ALTER_ACTION_ENABLE);
> + assert(src_tab->nSrc == 1);
> + struct sql *db = parse->db;
> +
> + char *constraint_name = NULL;
> + const char *tbl_name = src_tab->a[0].zName;
> + struct space *space = space_by_name(tbl_name);
> + if (space == NULL) {
> + diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
> + goto tnt_error;
> + }
> +
> + constraint_name = sql_name_from_token(db, &enable_def->name);
> + if (constraint_name == NULL)
> + goto tnt_error;
> +
> + if (space_ck_constraint_enable(space, constraint_name,
> + enable_def->new_status) != 0) {
> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, constraint_name);
> + goto tnt_error;
> + }
> +
> +exit_alter_ck_constraint:
> + sqlDbFree(db, constraint_name);
> + sqlSrcListDelete(db, src_tab);
> + return;
> +tnt_error:
Looks like crutch..Please, apply next refactoring:
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index e63fbdf2f..06bbdf163 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -95,26 +95,27 @@ sql_alter_ck_constraint_enable(struct Parse *parse)
struct space *space = space_by_name(tbl_name);
if (space == NULL) {
diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
- goto tnt_error;
+ parse->is_aborted = true;
+ goto exit_alter_ck_constraint;
}
constraint_name = sql_name_from_token(db, &enable_def->name);
- if (constraint_name == NULL)
- goto tnt_error;
+ if (constraint_name == NULL) {
+ parse->is_aborted = true;
+ goto exit_alter_ck_constraint;
+ }
if (space_ck_constraint_set_state(space, constraint_name,
enable_def->new_status) != 0) {
diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, constraint_name);
- goto tnt_error;
+ parse->is_aborted = true;
}
exit_alter_ck_constraint:
sqlDbFree(db, constraint_name);
sqlSrcListDelete(db, src_tab);
return;
-tnt_error:
- parse->is_aborted = true;
- goto exit_alter_ck_constraint;
+
}
> + parse->is_aborted = true;
> + goto exit_alter_ck_constraint;
> +}
> +
> /* This function is used to implement the ALTER TABLE command.
> * The table name in the CREATE TRIGGER statement is replaced with the third
> * argument and the result returned. This is analagous to rename_table()
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 643e025bd..9d134cc12 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -760,6 +760,10 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
> sqlExprListSetSortOrder(A,Z);
> }
>
> +%type enable {bool}
> +enable(A) ::= ENABLE. {A = true;}
> +enable(A) ::= DISABLE. {A = false;}
> +
> %type sortorder {int}
>
> sortorder(A) ::= ASC. {A = SORT_ORDER_ASC;}
> @@ -1747,6 +1751,12 @@ cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
> sql_drop_foreign_key(pParse);
> }
>
> +cmd ::= alter_table_start(A) enable(E) CONSTRAINT nm(Z). {
Using this syntax doesn't allow us to tell one type of constraints from
others. For instance, now it constraint disabling is allowed only for
CHECK constraints. I suggest adding to this clause constraint_type
option. For instance:
ALTER TABLE t ENABLE CHECK CONSTRAINT name;
Or even:
ALTER TABLE t ALTER CHECK CONSTRAINT name ENABLE;
> + enable_entity_def_init(&pParse->enable_entity_def, ENTITY_TYPE_CK, A,
> + &Z, E);
> + sql_alter_ck_constraint_enable(pParse);
> +}
> +
> //////////////////////// COMMON TABLE EXPRESSIONS ////////////////////////////
> %type with {With*}
> %type wqlist {With*}
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> index 557e41529..0f07a1dc5 100644
> --- a/src/box/sql/parse_def.h
> +++ b/src/box/sql/parse_def.h
> @@ -169,7 +169,8 @@ enum entity_type {
> enum alter_action {
> ALTER_ACTION_CREATE = 0,
> ALTER_ACTION_DROP,
> - ALTER_ACTION_RENAME
> + ALTER_ACTION_RENAME,
> + ALTER_ACTION_ENABLE,
> };
>
> struct alter_entity_def {
> @@ -181,6 +182,17 @@ struct alter_entity_def {
> struct SrcList *entity_name;
> };
>
> +struct enable_entity_def {
> + struct alter_entity_def base;
> + /**
> + * Name of index/trigger/constraint to be
> + * enabled/disabled.
Now it only conserns constraints.
> + */
> + struct Token name;
> + /** A new state to be set for entity found. */
> + bool new_status;
> +};
> +
> struct rename_entity_def {
> struct alter_entity_def base;
> struct Token new_name;
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index e617edd79..24687e08f 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2192,6 +2192,7 @@ struct Parse {
> struct drop_table_def drop_table_def;
> struct drop_trigger_def drop_trigger_def;
> struct drop_view_def drop_view_def;
> + struct enable_entity_def enable_entity_def;
> };
> /**
> * Table def is not part of union since information
> @@ -3961,6 +3962,15 @@ extern int sqlPendingByte;
> void
> sql_alter_table_rename(struct Parse *parse);
>
> +/**
> + * Generate code to implement the "ALTER TABLE xxx RENAME TO yyy"
> + * command.
Mmm?
> + *
> + * @param parse Current parsing context.
> + */
> +void
> +sql_alter_ck_constraint_enable(struct Parse *parse);
> +
> /**
> * Return the length (in bytes) of the token that begins at z[0].
> * Store the token type in *type before returning.
> diff --git a/test/sql/checks.re b/test/sql/checks.re
> new file mode 100644
> index 000000000..d701bc11c
> --- /dev/null
> +++ b/test/sql/checks.re
> @@ -0,0 +1,817 @@
???
> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index db5d0b05f..442c50eaa 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -247,5 +247,20 @@ box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
> box.space.TEST:insert({11})
> box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(true)
> box.space.TEST:insert({12})
> +box.execute("DROP TABLE test;")
> +
> +--
> +-- Test ENABLE/DISABLE CK constraints from SQL works.
> +--
> +box.execute("ALTER TABLE falsch DISABLE CONSTRAINT \"ck_unnamed_TEST_1\"")
I wouldn't rely on default constraint names: if we suddenly decide to change
them, we will have to fix a lot of tests.
> +box.execute("CREATE TABLE test(a INT CHECK (a < 10) primary key);");
> +box.execute("ALTER TABLE test DISABLE CONSTRAINT \"falsch\"")
> +box.execute("ALTER TABLE test DISABLE CONSTRAINT \"ck_unnamed_TEST_1\"")
> +box.space.TEST.ck_constraint.ck_unnamed_TEST_1.is_enabled == false
You simply wrap it in assertion.
> +box.space.TEST:insert({11})
> +box.execute("ALTER TABLE test ENABLE CONSTRAINT \"ck_unnamed_TEST_1\"")
> +box.space.TEST.ck_constraint.ck_unnamed_TEST_1.is_enabled == true
> +box.space.TEST:insert({12})
> +box.execute("DROP TABLE test;")
>
> test_run:cmd("clear filter")
> --
> 2.23.0
>
>
More information about the Tarantool-patches
mailing list