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

[n.pettik]

> void
> -sqlite3AddNotNull(Parse * pParse, int onError)
> +sql_column_nullable_action_add(struct Parse *parser,
> +			       enum on_conflict_action nullable_action)

This name sounds really weird.. Why not sql_column_add_nullable_action ?

> {
> -	Table *p;
> -	p = pParse->pNewTable;
> -	if (p == 0 || NEVER(p->def->field_count < 1))
> +	struct Table *p = parser->pNewTable;
> +	if (p == NULL || NEVER(p->def->field_count < 1))
> 		return;
> -	p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError;
> -	p->def->fields[p->def->field_count - 1].is_nullable =
> -		action_is_nullable(onError);
> +	struct field_def *field = &p->def->fields[p->def->field_count - 1];
> +	if (field->nullable_action != on_conflict_action_MAX) {

Why do you need on_conflict_action_MAX when you already have ON_CONFLICT_ACTION_DEFAULT?
Anyway, there is no action DEFAULT, it is sooner or later converted to ABORT.

> +		/* Prevent defining nullable_action many times. */
> +		const char *err_msg =
> +			tt_sprintf("NULL declaration for column '%s' of table "
> +				   "'%s' has been already set to '%s'",
> +				   field->name, p->def->name,
> +				   on_conflict_action_strs[field->
> +							   nullable_action]);
> +		diag_set(ClientError, ER_SQL, err_msg);
> +		parser->rc = SQL_TARANTOOL_ERROR;
> +		parser->nErr++;
> +		return;
> +	}
> +	field->nullable_action = nullable_action;
> +	field->is_nullable = action_is_nullable(nullable_action);
> }
> 
> /*
> @@ -1251,7 +1271,10 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
> 	if (pTab->iPKey >= 0) {
> 		ExprList *pList;
> 		Token ipkToken;
> -		sqlite3TokenInit(&ipkToken, pTab->def->fields[pTab->iPKey].name);
> +		struct field_def *field = &fields[pTab->iPKey];

Could we avoid using iPKey in this function? We are going to remove it soon.

> +		if (field_def_create_for_pk(pParse, field, space_name) != 0)
> +			return;
> +		sqlite3TokenInit(&ipkToken, field->name);
> 		pList = sql_expr_list_append(pParse->db, NULL,
> 					     sqlite3ExprAlloc(db, TK_ID,
> 							      &ipkToken, 0));
> index b6d92f7..5ae6b0c 100644
> --- a/test/sql/on-conflict.test.lua
> +++ b/test/sql/on-conflict.test.lua
> @@ -38,3 +38,11 @@ box.sql.execute('DROP TABLE p')
> box.sql.execute('DROP TABLE e')
> box.sql.execute('DROP TABLE t1')
> box.sql.execute('DROP TABLE t2')
> +
> +--
> +-- gh-3473: Primary key can be declared with NULL.

Lets write ‘can’t be declared with NULL’. Otherwise, it seems to be confusing.

> +--
> +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);')
> +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);')
> +box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);")
> +box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))")
> --