[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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'
> +...