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

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

[Kirill Shcherbatov]

This patch dissallows define multiple "NULL", "NOT NULL"
options per column and fixes silent implicit behavior
for invalid "NULL PRIMARY KEY" construction.
Then, we remove useless SQL Column structure.

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3473-on-conflict-defaults-fixes
Issue: https://github.com/tarantool/tarantool/issues/3473

Kirill Shcherbatov (3):
  sql: restrict nullable action definitions
  sql: fixed possible leak in sqlite3EndTable
  sql: get rid of Column structure

 src/box/alter.cc                |   3 +
 src/box/field_def.c             |   1 +
 src/box/field_def.h             |   2 +
 src/box/sql/alter.c             |  27 ++---
 src/box/sql/build.c             | 221 +++++++++++++++++++++-------------------
 src/box/sql/parse.y             |   9 +-
 src/box/sql/resolve.c           |  11 +-
 src/box/sql/select.c            |  43 +++-----
 src/box/sql/sqliteInt.h         |  28 ++---
 test/sql-tap/conflict3.test.lua |  10 +-
 test/sql/on-conflict.result     |  21 ++++
 test/sql/on-conflict.test.lua   |   8 ++
 12 files changed, 207 insertions(+), 177 deletions(-)

-- 
2.7.4

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

[Kirill Shcherbatov]

This patch dissallows define multiple "NULL", "NOT NULL"
options per column and fixes silent implicit behavior
for invalid "NULL PRIMARY KEY" construction.

Closes #3473.
---
 src/box/sql/build.c           | 98 +++++++++++++++++++++++++++++++------------
 src/box/sql/parse.y           |  9 +++-
 src/box/sql/sqliteInt.h       | 17 +++++++-
 test/sql/on-conflict.result   | 21 ++++++++++
 test/sql/on-conflict.test.lua |  8 ++++
 5 files changed, 123 insertions(+), 30 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a64d723..a023b1e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -656,7 +656,12 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
 	struct field_def *column_def = &p->def->fields[p->def->field_count];
 	memcpy(column_def, &field_def_default, sizeof(field_def_default));
 	column_def->name = z;
-	column_def->nullable_action = ON_CONFLICT_ACTION_NONE;
+	/*
+	 * Marker on_conflict_action_MAX is used to detect
+	 * attempts to define NULL multiple time or to detect
+	 * invalid primary key definition.
+	 */
+	column_def->nullable_action = on_conflict_action_MAX;
 	column_def->is_nullable = true;
 
 	if (pType->n == 0) {
@@ -691,22 +696,29 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
 	pParse->constraintName.n = 0;
 }
 
-/*
- * This routine is called by the parser while in the middle of
- * parsing a CREATE TABLE statement.  A "NOT NULL" constraint has
- * been seen on a column.  This routine sets the notNull flag on
- * the column currently under construction.
- */
 void
-sqlite3AddNotNull(Parse * pParse, int onError)
+sql_column_nullable_action_add(struct Parse *parser,
+			       enum on_conflict_action 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) {
+		/* 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);
 }
 
 /*
@@ -1215,6 +1227,24 @@ createTableStmt(sqlite3 * db, Table * p)
 	return zStmt;
 }
 
+static int
+field_def_create_for_pk(struct Parse *parser, struct field_def *field,
+			const char *space_name)
+{
+	if (field->nullable_action != ON_CONFLICT_ACTION_ABORT &&
+	    field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
+	    field->nullable_action != on_conflict_action_MAX) {
+		diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name);
+		parser->rc = SQL_TARANTOOL_ERROR;
+		parser->nErr++;
+		return -1;
+	} else if (field->nullable_action == on_conflict_action_MAX) {
+		field->nullable_action = ON_CONFLICT_ACTION_ABORT;
+		field->is_nullable = false;
+	}
+	return 0;
+}
+
 /*
  * This routine runs at the end of parsing a CREATE TABLE statement.
  * The job of this routine is to convert both
@@ -1232,18 +1262,8 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 {
 	Index *pPk;
 	sqlite3 *db = pParse->db;
-
-	/* Mark every PRIMARY KEY column as NOT NULL (except for imposter tables)
-	 */
-	if (!db->init.imposterTable) {
-		for (uint32_t i = 0; i < 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;
-			}
-		}
-	}
+	struct field_def *fields = pTab->def->fields;
+	const char *space_name = pTab->def->name;
 
 	/* Locate the PRIMARY KEY index.  Or, if this table was originally
 	 * an INTEGER PRIMARY KEY table, create a new PRIMARY KEY index.
@@ -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];
+		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));
@@ -1268,6 +1291,16 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 		pTab->iPKey = -1;
 	} else {
 		pPk = sqlite3PrimaryKeyIndex(pTab);
+		assert(pPk != NULL);
+		struct key_def *key_def = pPk->def->key_def;
+		assert(key_def != NULL);
+		for (uint32_t i = 0; i < key_def->part_count; i++) {
+			struct field_def *field =
+				&fields[key_def->parts[i].fieldno];
+			if (field_def_create_for_pk(pParse, field,
+						    space_name) != 0)
+				return;
+		}
 	}
 	assert(pPk != 0);
 }
@@ -1654,6 +1687,17 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 			return;
 		} else {
 			convertToWithoutRowidTable(pParse, p);
+			if (pParse->nErr > 0)
+				return;
+		}
+	}
+
+	/* Set default on_nullable action if required. */
+	struct field_def *field = p->def->fields;
+	for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) {
+		if (field->nullable_action == on_conflict_action_MAX) {
+			field->nullable_action = ON_CONFLICT_ACTION_NONE;
+			field->is_nullable = true;
 		}
 	}
 
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index e8a36e4..45321a7 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -276,8 +276,13 @@ ccons ::= DEFAULT id(X).              {
 // In addition to the type name, we also care about the primary key and
 // UNIQUE constraints.
 //
-ccons ::= NULL onconf.
-ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
+ccons ::= NULL onconf(R).        {
+    sql_column_nullable_action_add(pParse, ON_CONFLICT_ACTION_NONE);
+    /* Trigger nullability mismatch error if required. */
+    if (R != ON_CONFLICT_ACTION_DEFAULT)
+        sql_column_nullable_action_add(pParse, R);
+}
+ccons ::= NOT NULL onconf(R).    {sql_column_nullable_action_add(pParse, R);}
 ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
                                  {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}
 ccons ::= UNIQUE onconf(R).      {sql_create_index(pParse,0,0,0,R,0,0,
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 54661cb..9f6e97e 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3506,7 +3506,22 @@ Table *sqlite3ResultSetOfSelect(Parse *, Select *);
 Index *sqlite3PrimaryKeyIndex(Table *);
 void sqlite3StartTable(Parse *, Token *, int);
 void sqlite3AddColumn(Parse *, Token *, Token *);
-void sqlite3AddNotNull(Parse *, int);
+
+/**
+ * This routine is called by the parser while in the middle of
+ * parsing a CREATE TABLE statement.  A "NOT NULL" constraint has
+ * been seen on a column.  This routine sets the is_nullable flag
+ * on the column currently under construction.
+ * If nullable_action has been already set, this function raises
+ * an error.
+ *
+ * @param parser SQL Parser object.
+ * @param nullable_action on_conflict_action value.
+ */
+void
+sql_column_nullable_action_add(struct Parse *parser,
+			       enum on_conflict_action nullable_action);
+
 void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order);
 
 /**
diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
index 4080648..a15acca 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: Primary index of the space 'TE17' can not contain nullable parts
+...
+box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);")
+---
+- error: 'SQL error: NULL declaration for column ''B'' of table ''TEST'' has been
+    already set to ''none'''
+...
+box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))")
+---
+- error: Primary index of the space 'TEST' can not contain nullable parts
+...
diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua
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.
+--
+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))")
-- 
2.7.4

[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))")
> -- 

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

[Kirill Shcherbatov]

> This name sounds really weird.. Why not sql_column_add_nullable_action ?
If you wish.
-sql_column_nullable_action_add(struct Parse *parser,
+sql_column_add_nullable_action(struct Parse *parser,

> 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.
This is the central idea of the patch.  on_conflict_action_MAX is a marker that this field wasn't
initialized yet manually. This allows to detect second attempt to specify NULL/NOT NULL etc.
There is a comment about this concept in sqlite3AddColumn where on_conflict_action_MAX is set.
The default behavior is  ON_CONFLICT_ACTION_NONE and we have to distinguish non-initialized
columns and initialized with ON_CONFLICT_ACTION_DEFAULT.

> Could we avoid using iPKey in this function? We are going to remove it soon.
I don't increase a complexity here and believe that is patch is not about to suggest a way to exclude iPkey.
No idea.

-		sqlite3TokenInit(&ipkToken, pTab->def->fields[pTab->iPKey].name);
+		struct field_def *field = &fields[pTab->iPKey];


> Lets write ‘can’t be declared with NULL’. Otherwise, it seems to be confusing.
--- gh-3473: Primary key can be declared with NULL.
+-- gh-3473: Primary key can't be declared with NULL.

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

[n.pettik]

>> 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.
> This is the central idea of the patch.  on_conflict_action_MAX is a marker that this field wasn't
> initialized yet manually. This allows to detect second attempt to specify NULL/NOT NULL etc.
> There is a comment about this concept in sqlite3AddColumn where on_conflict_action_MAX is set.
> The default behavior is  ON_CONFLICT_ACTION_NONE and we have to distinguish non-initialized
> columns and initialized with ON_CONFLICT_ACTION_DEFAULT.

Well, it seems to be total mess. DEFAULT (for nullable on conflict action)
is always converted into ABORT. If you move this conversation right after
parser’s pass, you can get rid off using additional enum value:

create table t1(a NULL PRIMARY KEY NOT NULL , b);

a == DEFAULT —> NONE —> ABORT
b == DEFAULT —> NONE

>> Could we avoid using iPKey in this function? We are going to remove it soon.
> I don't increase a complexity here and believe that is patch is not about to suggest a way to exclude iPkey.
> No idea.

Nevermind, Kirill already reworked this function in order to avoid using iPKey.

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

[Kirill Shcherbatov]

>>> 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.
>> This is the central idea of the patch.  on_conflict_action_MAX is a marker that this field wasn't
>> initialized yet manually. This allows to detect second attempt to specify NULL/NOT NULL etc.
>> There is a comment about this concept in sqlite3AddColumn where on_conflict_action_MAX is set.
>> The default behavior is  ON_CONFLICT_ACTION_NONE and we have to distinguish non-initialized
>> columns and initialized with ON_CONFLICT_ACTION_DEFAULT.
> 
> Well, it seems to be total mess. DEFAULT (for nullable on conflict action)
> is always converted into ABORT. If you move this conversation right after
> parser’s pass, you can get rid off using additional enum value:
> 
> create table t1(a NULL PRIMARY KEY NOT NULL , b);
> 
> a == DEFAULT —> NONE —> ABORT
> b == DEFAULT —> NONE
Please, read my previous message again. 

I need to distinguish no-initialized and initialized columns.
+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'''

I need a marker. I've tried to use ON_CONFLICT_ACTION_DEFAULT in this role, but it looks not
workable until Index->onError present: onconf parse.y rule should be universal.

Let's discuss this verbally if you need.

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

[Kirill Shcherbatov]

Hi again!
I've rebased my patch on Kirill's Y. branch kyukhin/gh-3482-remove-ipkey;
I've also have succeed in using  ON_CONFLICT_ACTION_DEFAULT instead of on_conflict_action_MAX.
This changes represented as "[draft] don't use on_conflict_default_MAX" commit on my branch.
I've have had to introduce small rule index_onconf that looks very similar with onconf. If I not mistaken,
you are going to get rid of Index on_conflict action; and this rudiment crap would disappear. 

On 20.07.2018 10:29, Kirill Shcherbatov wrote:
>>>> 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.
>>> This is the central idea of the patch.  on_conflict_action_MAX is a marker that this field wasn't
>>> initialized yet manually. This allows to detect second attempt to specify NULL/NOT NULL etc.
>>> There is a comment about this concept in sqlite3AddColumn where on_conflict_action_MAX is set.
>>> The default behavior is  ON_CONFLICT_ACTION_NONE and we have to distinguish non-initialized
>>> columns and initialized with ON_CONFLICT_ACTION_DEFAULT.
>>
>> Well, it seems to be total mess. DEFAULT (for nullable on conflict action)
>> is always converted into ABORT. If you move this conversation right after
>> parser’s pass, you can get rid off using additional enum value:
>>
>> create table t1(a NULL PRIMARY KEY NOT NULL , b);
>>
>> a == DEFAULT —> NONE —> ABORT
>> b == DEFAULT —> NONE
> Please, read my previous message again. 
> 
> I need to distinguish no-initialized and initialized columns.
> +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'''
> 
> I need a marker. I've tried to use ON_CONFLICT_ACTION_DEFAULT in this role, but it looks not
> workable until Index->onError present: onconf parse.y rule should be universal.
> 
> Let's discuss this verbally if you need.
> 
> 

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

[Kirill Shcherbatov]

After verbal discussion, I've made small fix and merged "[draft] don't use on_conflict_default_MAX".
Don't forget that this patch is now on Kirill's iPkey patch and should be merged to upstream later.

On 23.07.2018 11:31, Kirill Shcherbatov wrote:
> Hi again!
> I've rebased my patch on Kirill's Y. branch kyukhin/gh-3482-remove-ipkey;
> I've also have succeed in using  ON_CONFLICT_ACTION_DEFAULT instead of on_conflict_action_MAX.
> This changes represented as "[draft] don't use on_conflict_default_MAX" commit on my branch.
> I've have had to introduce small rule index_onconf that looks very similar with onconf. If I not mistaken,
> you are going to get rid of Index on_conflict action; and this rudiment crap would disappear. 

=============================================

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ba80d1d..ec0c06b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -950,7 +950,7 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 					    sqlite3ExprAlloc(db, TK_ID,
 							     &token, 0));
 		if (list == NULL)
-			return;
+			goto primary_key_exit;
 		sql_create_index(pParse, 0, 0, list, onError, 0, 0,
 				 SORT_ORDER_ASC, false,
 				 SQL_INDEX_TYPE_CONSTRAINT_PK);
@@ -959,21 +959,23 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 	} else if (autoInc) {
 		sqlite3ErrorMsg(pParse, "AUTOINCREMENT is only allowed on an "
 				"INTEGER PRIMARY KEY or INT PRIMARY KEY");
+		goto primary_key_exit;
 	} else {
 		sql_create_index(pParse, 0, 0, pList, onError, 0,
 				 0, sortOrder, false,
 				 SQL_INDEX_TYPE_CONSTRAINT_PK);
 		pList = 0;
+		if (pParse->nErr > 0)
+			goto primary_key_exit;
 	}
 
 	struct Index *pk = sqlite3PrimaryKeyIndex(pTab);
-	if (pk != NULL) {
-		struct key_def *pk_key_def = pk->def->key_def;
-		for (uint32_t i = 0; i < pk_key_def->part_count; i++) {
-			uint32_t idx = pk_key_def->parts[i].fieldno;
-			field_def_create_for_pk(pParse, &pTab->def->fields[idx],
-						pTab->def->name);
-		}
+	assert(pk != NULL);
+	struct key_def *pk_key_def = pk->def->key_def;
+	for (uint32_t i = 0; i < pk_key_def->part_count; i++) {
+		uint32_t idx = pk_key_def->parts[i].fieldno;
+		field_def_create_for_pk(pParse, &pTab->def->fields[idx],
+					pTab->def->name);
 	}
 primary_key_exit:
 	sql_expr_list_delete(pParse->db, pList);

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

[n.pettik]

Now LGTM.

[tarantool-patches] [PATCH v1 2/3] sql: fixed possible leak in sqlite3EndTable

[Kirill Shcherbatov]

---
 src/box/sql/build.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a023b1e..ee97ef9 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1684,11 +1684,11 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 			sqlite3ErrorMsg(pParse,
 					"PRIMARY KEY missing on table %s",
 					p->def->name);
-			return;
+			goto cleanup;
 		} else {
 			convertToWithoutRowidTable(pParse, p);
 			if (pParse->nErr > 0)
-				return;
+				goto cleanup;
 		}
 	}
 
@@ -1706,7 +1706,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 				"only PRIMARY KEY constraint can "
 				"have ON CONFLICT REPLACE clause "
 				"- %s", p->def->name);
-		return;
+		goto cleanup;
 	}
 	if (db->init.busy) {
 		/*
@@ -1718,7 +1718,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		 */
 		struct ExprList *old_checks = p->def->opts.checks;
 		if (sql_table_def_rebuild(db, p) != 0)
-			return;
+			goto cleanup;
 		sql_expr_list_delete(db, old_checks);
 	}
 
@@ -1736,7 +1736,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 
 		v = sqlite3GetVdbe(pParse);
 		if (NEVER(v == 0))
-			return;
+			goto cleanup;
 
 		/*
 		 * Initialize zType for the new view or table.
@@ -1821,7 +1821,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		if (pOld) {
 			assert(p == pOld);	/* Malloc must have failed inside HashInsert() */
 			sqlite3OomFault(db);
-			return;
+			goto cleanup;
 		}
 		pParse->pNewTable = 0;
 		current_session()->sql_flags |= SQLITE_InternChanges;
@@ -1846,6 +1846,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 	 * don't require make a copy on space_def_dup and to improve
 	 * debuggability.
 	 */
+cleanup:
 	sql_expr_list_delete(db, p->def->opts.checks);
 	p->def->opts.checks = NULL;
 }
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 2/3] sql: fixed possible leak in sqlite3EndTable

[n.pettik]

LGTM, but why does this patch belong to current patch-set?

[tarantool-patches] [PATCH v1 3/3] sql: get rid of Column structure

[Kirill Shcherbatov]

Get rid of is_primkey in Column structure as it become
redundant. Moved the last member coll with collation pointer
to field_def structure. Finally, dropped Column.
---
 src/box/alter.cc                |   3 +
 src/box/field_def.c             |   1 +
 src/box/field_def.h             |   2 +
 src/box/sql/alter.c             |  27 ++-------
 src/box/sql/build.c             | 130 ++++++++++++++++------------------------
 src/box/sql/resolve.c           |  11 ++--
 src/box/sql/select.c            |  43 +++++--------
 src/box/sql/sqliteInt.h         |  11 ----
 test/sql-tap/conflict3.test.lua |  10 ++--
 9 files changed, 87 insertions(+), 151 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 7b6bd1a..444bc48 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -407,6 +407,9 @@ field_def_decode(struct field_def *field, const char **data,
 			  tt_sprintf("collation is reasonable only for "
 				     "string, scalar and any fields"));
 	}
+	struct coll_id *collation = coll_by_id(field->coll_id);
+	if (collation != NULL)
+		field->coll = collation->coll;
 
 	const char *dv = field->default_value;
 	if (dv != NULL) {
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 8dbead6..12cc3b2 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -106,6 +106,7 @@ const struct field_def field_def_default = {
 	.is_nullable = false,
 	.nullable_action = ON_CONFLICT_ACTION_DEFAULT,
 	.coll_id = COLL_NONE,
+	.coll = NULL,
 	.default_value = NULL,
 	.default_value_expr = NULL
 };
diff --git a/src/box/field_def.h b/src/box/field_def.h
index 05f80d4..c8f158c 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -123,6 +123,8 @@ struct field_def {
 	enum on_conflict_action nullable_action;
 	/** Collation ID for string comparison. */
 	uint32_t coll_id;
+	/** Collating sequence for string comparison. */
+	struct coll *coll;
 	/** 0-terminated SQL expression for DEFAULT value. */
 	char *default_value;
 	/** AST for parsed default value. */
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index fe54e55..38c36e8 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -146,7 +146,6 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
 	Table *pNew;		/* Copy of pParse->pNewTable */
 	Table *pTab;		/* Table being altered */
 	const char *zTab;	/* Table name */
-	Column *pCol;		/* The new column */
 	Expr *pDflt;		/* Default value for the new column */
 	sqlite3 *db;		/* The database connection; */
 	Vdbe *v = pParse->pVdbe;	/* The prepared statement under construction */
@@ -161,7 +160,6 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
 
 	/* Skip the "sqlite_altertab_" prefix on the name. */
 	zTab = &pNew->def->name[16];
-	pCol = &pNew->aCol[pNew->def->field_count - 1];
 	assert(pNew->def != NULL);
 	pDflt = space_column_default_expr(SQLITE_PAGENO_TO_SPACEID(pNew->tnum),
 					  pNew->def->field_count - 1);
@@ -181,7 +179,10 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
 	 * If there is a NOT NULL constraint, then the default value for the
 	 * column must not be NULL.
 	 */
-	if (pCol->is_primkey) {
+	struct Index *pk = sqlite3PrimaryKeyIndex(pTab);
+	assert(pk != NULL);
+	struct key_def *pk_key_def = pk->def->key_def;
+	if (key_def_find(pk_key_def, pNew->def->field_count - 1) != NULL) {
 		sqlite3ErrorMsg(pParse, "Cannot add a PRIMARY KEY column");
 		return;
 	}
@@ -258,8 +259,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
 	Table *pNew;
 	Table *pTab;
 	Vdbe *v;
-	uint32_t i;
-	int nAlloc;
 	sqlite3 *db = pParse->db;
 
 	/* Look up the table being altered. */
@@ -296,24 +295,10 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
 	}
 	pParse->pNewTable = pNew;
 	assert(pNew->def->field_count > 0);
-	nAlloc = (((pNew->def->field_count - 1) / 8) * 8) + 8;
-	assert((uint32_t)nAlloc >= pNew->def->field_count && nAlloc % 8 == 0 &&
-	       nAlloc - pNew->def->field_count < 8);
-	pNew->aCol =
-	    (Column *) sqlite3DbMallocZero(db, sizeof(Column) * nAlloc);
 	/* FIXME: pNew->zName = sqlite3MPrintf(db, "sqlite_altertab_%s", pTab->zName); */
 	/* FIXME: if (!pNew->aCol || !pNew->zName) { */
-	if (!pNew->aCol) {
-		assert(db->mallocFailed);
-		goto exit_begin_add_column;
-	}
-	memcpy(pNew->aCol, pTab->aCol, sizeof(Column) * pNew->def->field_count);
-	for (i = 0; i < pNew->def->field_count; i++) {
-		Column *pCol = &pNew->aCol[i];
-		/* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */
-		pCol->coll = NULL;
-		pNew->def->fields[i].coll_id = COLL_NONE;
-	}
+	for (uint32_t i = 0; i < pNew->def->field_count; i++)
+		pNew->def->fields[i] = field_def_default;
 	pNew->pSchema = db->pSchema;
 	pNew->addColOffset = pTab->addColOffset;
 	pNew->nTabRef = 1;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ee97ef9..ed104e4 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -116,38 +116,48 @@ sql_finish_coding(struct Parse *parse_context)
 
 /**
  * This is a function which should be called during execution
- * of sqlite3EndTable. It ensures that only PRIMARY KEY
- * constraint may have ON CONFLICT REPLACE clause.
+ * of sqlite3EndTable. It set defaults for columns having no
+ * separate NULL/NOT NULL specifiers and ensures that only
+ * PRIMARY KEY constraint may have ON CONFLICT REPLACE clause.
  *
+ * @param parser SQL Parser object.
  * @param table Space which should be checked.
- * @retval False, if only primary key constraint has
- *         ON CONFLICT REPLACE clause or if there are no indexes
- *         with REPLACE as error action. True otherwise.
+ * @retval -1 on error. Parser SQL_TARANTOOL_ERROR is set.
+ * @retval 0 on success.
  */
-static bool
-check_on_conflict_replace_entries(struct Table *table)
-{
-	/* Check all NOT NULL constraints. */
-	for (int i = 0; i < (int)table->def->field_count; i++) {
-		enum on_conflict_action on_error =
-			table->def->fields[i].nullable_action;
-		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
-		    table->aCol[i].is_primkey == false) {
-			return true;
+static int
+actualize_on_conflict_actions(struct Parse *parser, struct Table *table)
+{
+	const char *err_msg = NULL;
+	struct field_def *field = table->def->fields;
+	struct Index *pk = sqlite3PrimaryKeyIndex(table);
+	for (uint32_t i = 0; i < table->def->field_count; ++i, ++field) {
+		if (field->nullable_action == on_conflict_action_MAX) {
+			/* Set default. */
+			field->nullable_action = ON_CONFLICT_ACTION_NONE;
+			field->is_nullable = true;
 		}
+		if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
+		    (pk == NULL || key_def_find(pk->def->key_def, i) == NULL))
+			goto non_pk_on_conflict_error;
 	}
-	/* Check all UNIQUE constraints. */
+
 	for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
 		if (idx->onError == ON_CONFLICT_ACTION_REPLACE &&
-		    !IsPrimaryKeyIndex(idx)) {
-			return true;
-		}
+		    !IsPrimaryKeyIndex(idx))
+			goto non_pk_on_conflict_error;
 	}
-	/*
-	 * CHECK constraints are not allowed to have REPLACE as
-	 * error action and therefore can be skipped.
-	 */
-	return false;
+
+	return 0;
+
+non_pk_on_conflict_error:
+	err_msg = tt_sprintf("only PRIMARY KEY constraint can have "
+			     "ON CONFLICT REPLACE clause - %s",
+			     table->def->name);
+	diag_set(ClientError, ER_SQL, err_msg);
+	parser->rc = SQL_TARANTOOL_ERROR;
+	parser->nErr++;
+	return -1;
 }
 
 /*
@@ -340,7 +350,6 @@ deleteTable(sqlite3 * db, Table * pTable)
 	/* Delete the Table structure itself.
 	 */
 	sqlite3HashClear(&pTable->idxHash);
-	sqlite3DbFree(db, pTable->aCol);
 	sqlite3DbFree(db, pTable->zColAff);
 	assert(pTable->def != NULL);
 	/* Do not delete pTable->def allocated on region. */
@@ -604,7 +613,6 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
 	int i;
 	char *z;
 	char *zType;
-	Column *pCol;
 	sqlite3 *db = pParse->db;
 	if ((p = pParse->pNewTable) == 0)
 		return;
@@ -642,17 +650,6 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
 			return;
 		}
 	}
-	if ((p->def->field_count & 0x7) == 0) {
-		Column *aNew =
-			sqlite3DbRealloc(db, p->aCol,
-					 (p->def->field_count + 8) *
-					 sizeof(p->aCol[0]));
-		if (aNew == NULL)
-			return;
-		p->aCol = aNew;
-	}
-	pCol = &p->aCol[p->def->field_count];
-	memset(pCol, 0, sizeof(p->aCol[0]));
 	struct field_def *column_def = &p->def->fields[p->def->field_count];
 	memcpy(column_def, &field_def_default, sizeof(field_def_default));
 	column_def->name = z;
@@ -890,7 +887,6 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
     )
 {
 	Table *pTab = pParse->pNewTable;
-	Column *pCol = 0;
 	int iCol = -1, i;
 	int nTerm;
 	if (pTab == 0)
@@ -904,8 +900,6 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 	pTab->tabFlags |= TF_HasPrimaryKey;
 	if (pList == 0) {
 		iCol = pTab->def->field_count - 1;
-		pCol = &pTab->aCol[iCol];
-		pCol->is_primkey = 1;
 		nTerm = 1;
 	} else {
 		nTerm = pList->nExpr;
@@ -914,25 +908,22 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 			    sqlite3ExprSkipCollate(pList->a[i].pExpr);
 			assert(pCExpr != 0);
 			if (pCExpr->op != TK_ID) {
-				sqlite3ErrorMsg(pParse, "expressions prohibited in PRIMARY KEY");
+				sqlite3ErrorMsg(pParse, "expressions prohibited"
+							" in PRIMARY KEY");
 				goto primary_key_exit;
 			}
-			const char *zCName = pCExpr->u.zToken;
-			for (iCol = 0;
-				iCol < (int)pTab->def->field_count; iCol++) {
-				if (strcmp
-				    (zCName,
-				     pTab->def->fields[iCol].name) == 0) {
-					pCol = &pTab->aCol[iCol];
-					pCol->is_primkey = 1;
+			const char *name = pCExpr->u.zToken;
+			struct space_def *def = pTab->def;
+			for (uint32_t idx = 0; idx < def->field_count; idx++) {
+				if (strcmp(name, def->fields[idx].name) == 0) {
+					iCol = idx;
 					break;
 				}
 			}
 		}
 	}
-	assert(pCol == NULL || pCol == &pTab->aCol[iCol]);
-	if (nTerm == 1 && pCol != NULL &&
-	    (pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER) &&
+	if (nTerm == 1 && iCol != -1 &&
+	    pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER &&
 	    sortOrder != SORT_ORDER_DESC) {
 		assert(autoInc == 0 || autoInc == 1);
 		pTab->iPKey = iCol;
@@ -1002,8 +993,8 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 	if (!zColl)
 		return;
 	uint32_t *id = &p->def->fields[i].coll_id;
-	p->aCol[i].coll = sql_get_coll_seq(pParse, zColl, id);
-	if (p->aCol[i].coll != NULL) {
+	p->def->fields[i].coll = sql_get_coll_seq(pParse, zColl, id);
+	if (p->def->fields[i].coll != NULL) {
 		/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
 		 * then an index may have been created on this column before the
 		 * collation type was added. Correct this if it is the case.
@@ -1163,14 +1154,11 @@ identPut(char *z, int *pIdx, char *zSignedIdent)
 static char *
 createTableStmt(sqlite3 * db, Table * p)
 {
-	int i, k, n;
 	char *zStmt;
 	char *zSep, *zSep2, *zEnd;
-	Column *pCol;
-	n = 0;
-	for (pCol = p->aCol, i = 0; i < (int)p->def->field_count; i++, pCol++) {
+	int n = 0;
+	for (uint32_t i = 0; i < p->def->field_count; i++)
 		n += identLength(p->def->fields[i].name) + 5;
-	}
 	n += identLength(p->def->name);
 	if (n < 50) {
 		zSep = "";
@@ -1188,10 +1176,10 @@ createTableStmt(sqlite3 * db, Table * p)
 		return 0;
 	}
 	sqlite3_snprintf(n, zStmt, "CREATE TABLE ");
-	k = sqlite3Strlen30(zStmt);
+	int k = sqlite3Strlen30(zStmt);
 	identPut(zStmt, &k, p->def->name);
 	zStmt[k++] = '(';
-	for (pCol = p->aCol, i = 0; i < (int)p->def->field_count; i++, pCol++) {
+	for (uint32_t i = 0; i < p->def->field_count; i++) {
 		static const char *const azType[] = {
 			/* AFFINITY_BLOB    */ "",
 			/* AFFINITY_TEXT    */ " TEXT",
@@ -1692,22 +1680,9 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		}
 	}
 
-	/* Set default on_nullable action if required. */
-	struct field_def *field = p->def->fields;
-	for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) {
-		if (field->nullable_action == on_conflict_action_MAX) {
-			field->nullable_action = ON_CONFLICT_ACTION_NONE;
-			field->is_nullable = true;
-		}
-	}
-
-	if (check_on_conflict_replace_entries(p)) {
-		sqlite3ErrorMsg(pParse,
-				"only PRIMARY KEY constraint can "
-				"have ON CONFLICT REPLACE clause "
-				"- %s", p->def->name);
+	if (actualize_on_conflict_actions(pParse, p))
 		goto cleanup;
-	}
+
 	if (db->init.busy) {
 		/*
 		 * As rebuild creates a new ExpList tree and
@@ -1882,12 +1857,9 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
 		sqlite3SelectAddColumnTypeAndCollation(parse_context, p,
 						       select);
 	} else {
-		assert(p->aCol == NULL);
 		assert(sel_tab->def->opts.is_temporary);
 		p->def->fields = sel_tab->def->fields;
 		p->def->field_count = sel_tab->def->field_count;
-		p->aCol = sel_tab->aCol;
-		sel_tab->aCol = NULL;
 		sel_tab->def->fields = NULL;
 		sel_tab->def->field_count = 0;
 	}
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index a185473..2c49e2c 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -219,7 +219,6 @@ lookupName(Parse * pParse,	/* The parsing context */
 	NameContext *pTopNC = pNC;	/* First namecontext in the list */
 	int isTrigger = 0;	/* True if resolved to a trigger column */
 	Table *pTab = 0;	/* Table hold the row */
-	Column *pCol;		/* A column of pTab */
 
 	assert(pNC);		/* the name context cannot be NULL. */
 	assert(zCol);		/* The Z in X.Y.Z cannot be NULL */
@@ -272,9 +271,8 @@ lookupName(Parse * pParse,	/* The parsing context */
 				if (0 == (cntTab++)) {
 					pMatch = pItem;
 				}
-				for (j = 0, pCol = pTab->aCol;
-					j < (int)pTab->def->field_count;
-					j++, pCol++) {
+				for (j = 0; j < (int)pTab->def->field_count;
+				     j++) {
 					if (strcmp(pTab->def->fields[j].name,
 						   zCol) == 0) {
 						/* If there has been exactly one prior match and this match
@@ -331,9 +329,8 @@ lookupName(Parse * pParse,	/* The parsing context */
 			if (pTab) {
 				int iCol;
 				cntTab++;
-				for (iCol = 0, pCol = pTab->aCol;
-				     iCol < (int)pTab->def->field_count;
-				     iCol++, pCol++) {
+				for (iCol = 0; iCol <
+				     (int)pTab->def->field_count; iCol++) {
 					if (strcmp(pTab->def->fields[iCol].name,
 						   zCol) == 0) {
 						if (iCol == pTab->iPKey) {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 34d5329..d577648 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1811,25 +1811,15 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
 {
 	/* Database connection */
 	sqlite3 *db = parse->db;
-	int i, j;		/* Loop counters */
 	u32 cnt;		/* Index added to make the name unique */
-	Column *aCol, *pCol;	/* For looping over result columns */
-	int nCol;		/* Number of columns in the result set */
 	Expr *p;		/* Expression for a single result column */
 	char *zName;		/* Column name */
 	int nName;		/* Size of name in zName[] */
 	Hash ht;		/* Hash table of column names */
 
 	sqlite3HashInit(&ht);
-	if (expr_list) {
-		nCol = expr_list->nExpr;
-		aCol = sqlite3DbMallocZero(db, sizeof(aCol[0]) * nCol);
-		testcase(aCol == 0);
-	} else {
-		nCol = 0;
-		aCol = NULL;
-	}
-	assert(nCol == (i16) nCol);
+	uint32_t column_count =
+		expr_list != NULL ? (uint32_t)expr_list->nExpr : 0;
 	/*
 	 * This should be a table without resolved columns.
 	 * sqlite3ViewGetColumnNames could use it to resolve
@@ -1838,21 +1828,21 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
 	assert(table->def->fields == NULL);
 	struct region *region = &parse->region;
 	table->def->fields =
-		region_alloc(region, nCol * sizeof(table->def->fields[0]));
+		region_alloc(region,
+			     column_count * sizeof(table->def->fields[0]));
 	if (table->def->fields == NULL) {
 		sqlite3OomFault(db);
 		goto cleanup;
 	}
-	for (int i = 0; i < nCol; i++) {
+	for (uint32_t i = 0; i < column_count; i++) {
 		memcpy(&table->def->fields[i], &field_def_default,
 		       sizeof(field_def_default));
 		table->def->fields[i].nullable_action = ON_CONFLICT_ACTION_NONE;
 		table->def->fields[i].is_nullable = true;
 	}
-	table->def->field_count = (uint32_t)nCol;
-	table->aCol = aCol;
+	table->def->field_count = column_count;
 
-	for (i = 0, pCol = aCol; i < nCol; i++, pCol++) {
+	for (uint32_t i = 0; i < column_count; i++) {
 		/* Get an appropriate name for the column
 		 */
 		p = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
@@ -1889,9 +1879,9 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
 		while (zName && sqlite3HashFind(&ht, zName) != 0) {
 			nName = sqlite3Strlen30(zName);
 			if (nName > 0) {
+				int j;
 				for (j = nName - 1;
-				     j > 0 && sqlite3Isdigit(zName[j]); j--) {
-				}
+				     j > 0 && sqlite3Isdigit(zName[j]); j--);
 				if (zName[j] == ':')
 					nName = j;
 			}
@@ -1901,7 +1891,9 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table)
 				sqlite3_randomness(sizeof(cnt), &cnt);
 		}
 		size_t name_len = strlen(zName);
-		if (zName != NULL && sqlite3HashInsert(&ht, zName, pCol) == pCol)
+		void *field = &table->def->fields[i];
+		if (zName != NULL &&
+		    sqlite3HashInsert(&ht, zName, field) == field)
 			sqlite3OomFault(db);
 		table->def->fields[i].name =
 			region_alloc(region, name_len + 1);
@@ -1921,10 +1913,8 @@ cleanup:
 		 * pTable->def could be not temporal in
 		 * sqlite3ViewGetColumnNames so we need clean-up.
 		 */
-		sqlite3DbFree(db, aCol);
 		table->def->fields = NULL;
 		table->def->field_count = 0;
-		table->aCol = NULL;
 		rc = SQLITE_NOMEM_BKPT;
 	}
 	return rc;
@@ -1949,8 +1939,6 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
 {
 	sqlite3 *db = pParse->db;
 	NameContext sNC;
-	Column *pCol;
-	int i;
 	Expr *p;
 	struct ExprList_item *a;
 
@@ -1963,8 +1951,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
 	memset(&sNC, 0, sizeof(sNC));
 	sNC.pSrcList = pSelect->pSrc;
 	a = pSelect->pEList->a;
-	for (i = 0, pCol = pTab->aCol;
-		i < (int)pTab->def->field_count; i++, pCol++) {
+	for (uint32_t i = 0; i < pTab->def->field_count; i++) {
 		enum field_type type;
 		p = a[i].pExpr;
 		type = columnType(&sNC, p, 0, 0);
@@ -1977,8 +1964,8 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
 		bool unused;
 		uint32_t id;
 		struct coll *coll = sql_expr_coll(pParse, p, &unused, &id);
-		if (coll != NULL && pCol->coll == NULL) {
-			pCol->coll = coll;
+		if (coll != NULL && pTab->def->fields[i].coll == NULL) {
+			pTab->def->fields[i].coll = coll;
 			pTab->def->fields[i].coll_id = id;
 		}
 	}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 9f6e97e..984bec9 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1872,16 +1872,6 @@ struct Savepoint {
 #define SAVEPOINT_RELEASE    1
 #define SAVEPOINT_ROLLBACK   2
 
-/*
- * information about each column of an SQL table is held in an instance
- * of this structure.
- */
-struct Column {
-	/** Collating sequence. */
-	struct coll *coll;
-	u8 is_primkey;		/* Boolean propertie for being PK */
-};
-
 #define sqlite3IsNumericAffinity(X)  ((X)>=AFFINITY_NUMERIC)
 
 /*
@@ -1910,7 +1900,6 @@ struct Column {
  * by an instance of the following structure.
  */
 struct Table {
-	Column *aCol;		/* Information about each column */
 	Index *pIndex;		/* List of SQL indexes on this table. */
 	FKey *pFKey;		/* Linked list of all foreign keys in this table */
 	char *zColAff;		/* String defining the affinity of each column */
diff --git a/test/sql-tap/conflict3.test.lua b/test/sql-tap/conflict3.test.lua
index 345537e..9b55550 100755
--- a/test/sql-tap/conflict3.test.lua
+++ b/test/sql-tap/conflict3.test.lua
@@ -359,7 +359,7 @@ test:do_catchsql_test(
         CREATE TABLE t3(a PRIMARY KEY ON CONFLICT REPLACE,
                         b UNIQUE ON CONFLICT REPLACE);
     ]], {
-        1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
+        1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
     })
 
 test:do_catchsql_test(
@@ -368,7 +368,7 @@ test:do_catchsql_test(
         CREATE TABLE t3(a PRIMARY KEY,
                         b UNIQUE ON CONFLICT REPLACE);
     ]], {
-        1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
+        1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
     })
 
 test:do_catchsql_test(
@@ -378,7 +378,7 @@ test:do_catchsql_test(
                         b UNIQUE ON CONFLICT REPLACE,
                         c UNIQUE ON CONFLICT REPLACE);
     ]], {
-        1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
+        1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
     })
 
 test:do_catchsql_test(
@@ -387,7 +387,7 @@ test:do_catchsql_test(
         CREATE TABLE t3(a PRIMARY KEY,
                         b NOT NULL ON CONFLICT REPLACE DEFAULT 1488);
     ]], {
-        1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
+        1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
     })
 
 test:do_catchsql_test(
@@ -396,7 +396,7 @@ test:do_catchsql_test(
         CREATE TABLE t3(a PRIMARY KEY ON CONFLICT REPLACE,
                         b NOT NULL ON CONFLICT REPLACE DEFAULT 1488);
     ]], {
-        1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
+        1, "SQL error: only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3"
     })
 
 test:finish_test()
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 3/3] sql: get rid of Column structure

[n.pettik]

Again: why did this patch trapped in patch-set? All three patches seem to be independent.

> On 18 Jul 2018, at 19:52, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> Get rid of is_primkey in Column structure as it become

Typo: ‘becomes’.

> redundant. Moved the last member coll with collation pointer
> to field_def structure.

Why do you need to move collation ptr to field_def? It already features collation id,
so you can always get pointer to it by simple lookup. It would make sense if it was
utilised everywhere. But I see assignment only in sqlite3SelectAddColumnTypeAndCollation()
and no real usages..Lets remove it at all.

> @@ -1692,22 +1680,9 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
> 		}
> 	}
> 
> -	/* Set default on_nullable action if required. */
> -	struct field_def *field = p->def->fields;
> -	for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) {
> -		if (field->nullable_action == on_conflict_action_MAX) {
> -			field->nullable_action = ON_CONFLICT_ACTION_NONE;
> -			field->is_nullable = true;
> -		}
> -	}
> -
> -	if (check_on_conflict_replace_entries(p)) {
> -		sqlite3ErrorMsg(pParse,
> -				"only PRIMARY KEY constraint can "
> -				"have ON CONFLICT REPLACE clause "
> -				"- %s", p->def->name);

Here you are simply fixing changes made in first patch, so mb it is better to
move them to first patch?

[tarantool-patches] Re: [PATCH v1 3/3] sql: get rid of Column structure

[Kirill Shcherbatov]

> Again: why did this patch trapped in patch-set? All three patches seem to be independent.
You and Vlad sometimes ask me to do some unrelated to patch work; This is the same situation.
And it is also convenient to ask Kirill to merge a big patch set than to noise a lot for each small fix.

> Typo: ‘becomes’.
Yep, fixed.

> Why do you need to move collation ptr to field_def? It already features collation id,
> so you can always get pointer to it by simple lookup. It would make sense if it was
> utilised everywhere. But I see assignment only in sqlite3SelectAddColumnTypeAndCollation()
> and no real usages..Lets remove it at all.
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 444bc48..7b6bd1a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -407,9 +407,6 @@ field_def_decode(struct field_def *field, const char **data,
 			  tt_sprintf("collation is reasonable only for "
 				     "string, scalar and any fields"));
 	}
-	struct coll_id *collation = coll_by_id(field->coll_id);
-	if (collation != NULL)
-		field->coll = collation->coll;
 
 	const char *dv = field->default_value;
 	if (dv != NULL) {
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 12cc3b2..8dbead6 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -106,7 +106,6 @@ const struct field_def field_def_default = {
 	.is_nullable = false,
 	.nullable_action = ON_CONFLICT_ACTION_DEFAULT,
 	.coll_id = COLL_NONE,
-	.coll = NULL,
 	.default_value = NULL,
 	.default_value_expr = NULL
 };
diff --git a/src/box/field_def.h b/src/box/field_def.h
index c8f158c..05f80d4 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -123,8 +123,6 @@ struct field_def {
 	enum on_conflict_action nullable_action;
 	/** Collation ID for string comparison. */
 	uint32_t coll_id;
-	/** Collating sequence for string comparison. */
-	struct coll *coll;
 	/** 0-terminated SQL expression for DEFAULT value. */
 	char *default_value;
 	/** AST for parsed default value. */
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 6f803cf..2615624 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -992,9 +992,8 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 	char *zColl = sqlite3NameFromToken(db, pToken);
 	if (!zColl)
 		return;
-	uint32_t *id = &p->def->fields[i].coll_id;
-	p->def->fields[i].coll = sql_get_coll_seq(pParse, zColl, id);
-	if (p->def->fields[i].coll != NULL) {
+	uint32_t *coll_id = &p->def->fields[i].coll_id;
+	if (sql_get_coll_seq(pParse, zColl, coll_id) != NULL) {
 		/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
 		 * then an index may have been created on this column before the
 		 * collation type was added. Correct this if it is the case.
@@ -1003,9 +1002,8 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 		     pIdx = pIdx->pNext) {
 			assert(pIdx->def->key_def->part_count == 1);
 			if (pIdx->def->key_def->parts[0].fieldno == i) {
-				id = &pIdx->def->key_def->parts[0].coll_id;
-				pIdx->def->key_def->parts[0].coll =
-					sql_column_collation(p->def, i, id);
+				coll_id = &pIdx->def->key_def->parts[0].coll_id;
+				(void)sql_column_collation(p->def, i, coll_id);
 			}
 		}
 	}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d577648..ab91bd0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1962,12 +1962,8 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
 			affinity = AFFINITY_BLOB;
 		pTab->def->fields[i].affinity = affinity;
 		bool unused;
-		uint32_t id;
-		struct coll *coll = sql_expr_coll(pParse, p, &unused, &id);
-		if (coll != NULL && pTab->def->fields[i].coll == NULL) {
-			pTab->def->fields[i].coll = coll;
-			pTab->def->fields[i].coll_id = id;
-		}
+		(void)sql_expr_coll(pParse, p, &unused,
+				    &pTab->def->fields[i].coll_id);
 	}
 }
 
> Here you are simply fixing changes made in first patch, so mb it is better to
> move them to first patch?
No, I rethink thous changes taking into account new situation. It is better to keep it as is, except you think
to merge this patch with first one. But it seems to be a bad idea.

[tarantool-patches] Re: [PATCH v1 3/3] sql: get rid of Column structure

[Vladislav Shpilevoy]

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index d577648..ab91bd0 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1962,12 +1962,8 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
>   			affinity = AFFINITY_BLOB;
>   		pTab->def->fields[i].affinity = affinity;
>   		bool unused;
> -		uint32_t id;
> -		struct coll *coll = sql_expr_coll(pParse, p, &unused, &id);
> -		if (coll != NULL && pTab->def->fields[i].coll == NULL) {
> -			pTab->def->fields[i].coll = coll;
> -			pTab->def->fields[i].coll_id = id;
> -		}
> +		(void)sql_expr_coll(pParse, p, &unused,
> +				    &pTab->def->fields[i].coll_id);

Now you have changed the behavior. Before this fix the collation was
assigned once. Now it is assigned every time.

>   	}
>   }
>   

[tarantool-patches] Re: [PATCH v1 3/3] sql: get rid of Column structure

[Kirill Shcherbatov]

> Now you have changed the behavior. Before this fix the collation was
> assigned once. Now it is assigned every time.
hi! Thank you for lookup. You right: columns could have default collation that could be 
overwritten with specified one. 
I've returned old semantics.

CREATE TABLE t3(x TEXT PRIMARY KEY COLLATE "unicode_ci");
SELECT b FROM t4 UNION SELECT b FROM v4 ORDER BY 1 COLLATE text;

-               bool unused;
-               uint32_t id;
-               struct coll *coll = sql_expr_coll(pParse, p, &unused, &id);
-               if (coll != NULL && pCol->coll == NULL) {
-                       pCol->coll = coll;
-                       pTab->def->fields[i].coll_id = id;
-               }
+               bool is_found;
+               uint32_t coll_id;
+               if (pTab->def->fields[i].coll_id == COLL_NONE &&
+                   sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found)
+                       pTab->def->fields[i].coll_id = coll_id;

[tarantool-patches] Re: [PATCH v1 3/3] sql: get rid of Column structure

[n.pettik]

>> Again: why did this patch trapped in patch-set? All three patches seem to be independent.
> You and Vlad sometimes ask me to do some unrelated to patch work;

So you can send such patches as independent. It is not required to include
them in the same patch-set. Or, if it can be related and you include it,
add Follow-up: #xxxx marker.

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

[Kirill Yukhin]

Hello,
On 18 июл 19:52, Kirill Shcherbatov wrote:
> This patch dissallows define multiple "NULL", "NOT NULL"
> options per column and fixes silent implicit behavior
> for invalid "NULL PRIMARY KEY" construction.
> Then, we remove useless SQL Column structure.
> 
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3473-on-conflict-defaults-fixes
> Issue: https://github.com/tarantool/tarantool/issues/3473
I've checked the patch set into 2.0 branch.

--
Regards, Kirill Yukhin