[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