[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jul 13 23:03:43 MSK 2018
Thanks for the fixes! See 4 comments below.
I have pushed my review fixes on the branch, and they
correct some of the comments. Please, finish the patch
after my fixes including rest of the comments.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 8fba373..b00f4ff 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1306,11 +1324,31 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
> */
> if (!db->init.imposterTable) {
> for (i = 0; i < (int)pTab->def->field_count; i++) {
> - if (pTab->aCol[i].is_primkey) {
> - pTab->def->fields[i].nullable_action
> - = ON_CONFLICT_ACTION_ABORT;
> - pTab->def->fields[i].is_nullable = false;
> + if (pTab->aCol[i].is_primkey == 0)
1. Please, remove is_primkey. It is not needed here and can be easily
removed together with merging this cycle into the code below. The
only place where it is still used except here is
check_on_conflict_replace_entries, that can be spread over other
primary key checks.
> + continue;
> + enum on_conflict_action action =
> + pTab->def->fields[i].nullable_action;
> + if (action != on_conflict_action_MAX &&
> + action != ON_CONFLICT_ACTION_ABORT &&
> + action != ON_CONFLICT_ACTION_DEFAULT) {
> + const char *action_str =
> + on_conflict_action_strs[
> + pTab->def->fields[i].
> + nullable_action];
> + const char *err_str =
> + tt_sprintf("Cannot define PRIMARY KEY "
> + "constraint on column %s "
> + "with on conflict action %s",
> + pTab->def->fields[i].name,
> + action_str);
> + diag_set(ClientError, ER_SQL, err_str);
2. Please, use ER_NULLABLE_PRIMARY.
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + pParse->nErr++;
> + return;
> }
> + pTab->def->fields[i].nullable_action =
> + ON_CONFLICT_ACTION_ABORT;
> + pTab->def->fields[i].is_nullable = false;
> }
> }
>
> @@ -1739,6 +1777,35 @@ sqlite3EndTable(Parse * pParse, /* Parse context */
> }
> }
>
> + /* Set default on_nullable action if required. */
> + for (uint32_t i = 0; i < p->def->field_count; i++) {
> + if (p->def->fields[i].nullable_action ==
> + on_conflict_action_MAX) {
> + p->def->fields[i].nullable_action =
> + ON_CONFLICT_ACTION_NONE;
> + p->def->fields[i].is_nullable = true;
> + } else if (p->iPKey == (int)i &&
3. This is checked already, so the rest of hunk below can be removed.
> + p->def->fields[i].nullable_action ==
> + ON_CONFLICT_ACTION_NONE) {
> + /*
> + * PRIMARY KEY can not be defined with
> + * ON_CONFLICT_ACTION_NONE.
> + */
> + const char *action_str =
> + on_conflict_action_strs[p->def->fields[i].
> + nullable_action];
> + const char *err_str =
> + tt_sprintf("Cannot define PRIMARY KEY "
> + "constraint on nullable column %s",
> + p->def->fields[i].name,
> + action_str);
> + diag_set(ClientError, ER_SQL, err_str);
> + pParse->rc = SQL_TARANTOOL_ERROR;
> + pParse->nErr++;
> + return;
> + }
> + }
> +
> diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
> index c0d0de0..5c8131a 100644
> --- a/test/sql/on-conflict.result
> +++ b/test/sql/on-conflict.result
> @@ -99,3 +99,24 @@ box.sql.execute('DROP TABLE t1')
> box.sql.execute('DROP TABLE t2')
> ---
> ...
> +--
> +-- gh-3473: Primary key can be declared with NULL.
> +--
> +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);')
> +---
> +- error: 'SQL error: NULL declaration for column ''S1'' of table ''TE17'' has been
> + already set to ''none'''
> +...
> +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);')
> +---
> +- error: 'SQL error: Cannot define PRIMARY KEY constraint on nullable column S1'
> +...
> +box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);")
> +---
> +- error: keyword "ON" is reserved
4. This test does not work, it fails on syntax, but must fail on action.
> +...
> +box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))")
> +---
> +- error: 'SQL error: Cannot define PRIMARY KEY constraint on column B with on conflict
> + action none'
> +...
More information about the Tarantool-patches
mailing list