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

[tarantool-patches] [PATCH v1 0/2] 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.

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

Kirill Shcherbatov (2):
  sql: restrict nullable action definitions
  sql: fixed possible leak in sqlite3EndTable

 src/box/alter.cc              |  1 +
 src/box/field_def.c           |  1 +
 src/box/field_def.h           |  3 ++-
 src/box/sql/build.c           | 53 +++++++++++++++++++++++++++++++++++++------
 src/box/sql/parse.y           |  2 +-
 test/sql/on-conflict.result   | 13 +++++++++++
 test/sql/on-conflict.test.lua |  6 +++++
 7 files changed, 70 insertions(+), 9 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v1 1/2] 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/alter.cc              |  1 +
 src/box/field_def.c           |  1 +
 src/box/field_def.h           |  3 ++-
 src/box/sql/build.c           | 46 +++++++++++++++++++++++++++++++++++++++----
 src/box/sql/parse.y           |  2 +-
 test/sql/on-conflict.result   | 13 ++++++++++++
 test/sql/on-conflict.test.lua |  6 ++++++
 7 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 7b6bd1a..3791ac7 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -390,6 +390,7 @@ field_def_decode(struct field_def *field, const char **data,
 				     "nullable action",
 				     fieldno + TUPLE_INDEX_BASE));
 	}
+	assert(field->nullable_action != ON_CONFLICT_ACTION_UNDEFINED);
 	if (!((field->is_nullable && field->nullable_action ==
 	       ON_CONFLICT_ACTION_NONE)
 	      || (!field->is_nullable
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 8dbead6..70a9e46 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -46,6 +46,7 @@ const char *field_type_strs[] = {
 };
 
 const char *on_conflict_action_strs[] = {
+	/* [ON_CONFLICT_ACTION_UNDEFINED]= */ "undefined",
 	/* [ON_CONFLICT_ACTION_NONE]     = */ "none",
 	/* [ON_CONFLICT_ACTION_ROLLBACK] = */ "rollback",
 	/* [ON_CONFLICT_ACTION_ABORT]    = */ "abort",
diff --git a/src/box/field_def.h b/src/box/field_def.h
index 05f80d4..c35e0b8 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -60,7 +60,8 @@ enum field_type {
 };
 
 enum on_conflict_action {
-	ON_CONFLICT_ACTION_NONE = 0,
+	ON_CONFLICT_ACTION_UNDEFINED = 0,
+	ON_CONFLICT_ACTION_NONE,
 	ON_CONFLICT_ACTION_ROLLBACK,
 	ON_CONFLICT_ACTION_ABORT,
 	ON_CONFLICT_ACTION_FAIL,
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 53c20a6..946b10c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -653,7 +653,7 @@ 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;
+	column_def->nullable_action = ON_CONFLICT_ACTION_UNDEFINED;
 	column_def->is_nullable = true;
 
 	if (pType->n == 0) {
@@ -701,9 +701,22 @@ sqlite3AddNotNull(Parse * pParse, int onError)
 	p = pParse->pNewTable;
 	if (p == 0 || 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_UNDEFINED) {
+		/* 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);
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
+		return;
+	}
+	field->nullable_action = (u8)onError;
+	field->is_nullable = action_is_nullable(onError);
 }
 
 /*
@@ -1728,6 +1741,31 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		p->def->id = SQLITE_PAGENO_TO_SPACEID(p->tnum);
 	}
 
+	/* 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_UNDEFINED) {
+			p->def->fields[i].nullable_action =
+				ON_CONFLICT_ACTION_NONE;
+			p->def->fields[i].is_nullable = true;
+		} else if (p->iPKey == (int)i &&
+			    p->def->fields[i].nullable_action ==
+			    ON_CONFLICT_ACTION_NONE) {
+			/*
+			 * PRIMARY KEY can not be defined with
+			 * ON_CONFLICT_ACTION_NONE.
+			 */
+			const char *err_str =
+				tt_sprintf("Cannot define PRIMARY KEY "
+					   "constraint on nullable column in "
+					   "table '%s'", p->def->name);
+			diag_set(ClientError, ER_SQL, err_str);
+			pParse->rc = SQL_TARANTOOL_ERROR;
+			pParse->nErr++;
+			return;
+		}
+	}
+
 	if (!p->def->opts.is_view) {
 		if ((p->tabFlags & TF_HasPrimaryKey) == 0) {
 			sqlite3ErrorMsg(pParse,
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ac935fd..6463019 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -276,7 +276,7 @@ ccons ::= DEFAULT id(X).              {
 // In addition to the type name, we also care about the primary key and
 // UNIQUE constraints.
 //
-ccons ::= NULL onconf.
+ccons ::= NULL onconf.           {sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);}
 ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
 ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
                                  {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}
diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
index c0d0de0..bdc2c76 100644
--- a/test/sql/on-conflict.result
+++ b/test/sql/on-conflict.result
@@ -99,3 +99,16 @@ 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 in table
+    ''TE17'''
+...
diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua
index b6d92f7..94eb84f 100644
--- a/test/sql/on-conflict.test.lua
+++ b/test/sql/on-conflict.test.lua
@@ -38,3 +38,9 @@ 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);')
-- 
2.7.4

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

[Vladislav Shpilevoy]

Hello. See 5 comments below.

1. Assertion.

tarantool> format = {}
---
...

tarantool> format[1] = {'field1', 'unsigned', nullable_action = "undefined"}
---
...

tarantool> s = box.schema.create_space('test', {format = format})
Assertion failed: (field->nullable_action != ON_CONFLICT_ACTION_UNDEFINED), function field_def_decode, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/alter.cc, line 393.
Process 13519 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
     frame #0: 0x00007fff51b71b6e libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff51b71b6e <+10>: jae    0x7fff51b71b78            ; <+20>
     0x7fff51b71b70 <+12>: movq   %rax, %rdi
     0x7fff51b71b73 <+15>: jmp    0x7fff51b68b00            ; cerror_nocancel
     0x7fff51b71b78 <+20>: retq
Target 0: (tarantool) stopped.

On 12/07/2018 19:34, 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.
> 
> Closes #3473.
> ---
>   src/box/alter.cc              |  1 +
>   src/box/field_def.c           |  1 +
>   src/box/field_def.h           |  3 ++-
>   src/box/sql/build.c           | 46 +++++++++++++++++++++++++++++++++++++++----
>   src/box/sql/parse.y           |  2 +-
>   test/sql/on-conflict.result   | 13 ++++++++++++
>   test/sql/on-conflict.test.lua |  6 ++++++
>   7 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/field_def.h b/src/box/field_def.h
> index 05f80d4..c35e0b8 100644
> --- a/src/box/field_def.h
> +++ b/src/box/field_def.h
> @@ -60,7 +60,8 @@ enum field_type {
>   };
>   
>   enum on_conflict_action {
> -	ON_CONFLICT_ACTION_NONE = 0,
> +	ON_CONFLICT_ACTION_UNDEFINED = 0,

2. Please, remove this new action, because
* it is not really action, but just a flag;
* it can be replaced with on_conflict_action_MAX or
   ON_CONFLICT_ACTION_DEFAULT.

> +	ON_CONFLICT_ACTION_NONE,
>   	ON_CONFLICT_ACTION_ROLLBACK,
>   	ON_CONFLICT_ACTION_ABORT,
>   	ON_CONFLICT_ACTION_FAIL,
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 53c20a6..946b10c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -701,9 +701,22 @@ sqlite3AddNotNull(Parse * pParse, int onError)
>   	p = pParse->pNewTable;
>   	if (p == 0 || 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_UNDEFINED) {
> +		/* 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);
> +		pParse->rc = SQL_TARANTOOL_ERROR;
> +		pParse->nErr++;
> +		return;
> +	}
> +	field->nullable_action = (u8)onError;

3. Why do you cast to u8 though nullable_action is enum?

> +	field->is_nullable = action_is_nullable(onError);
>   }
>   
>   /*
> @@ -1728,6 +1741,31 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>   		p->def->id = SQLITE_PAGENO_TO_SPACEID(p->tnum);
>   	}
>   
> +	/* Set default on_nullable action if required. */

4. This still works:

     box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))")

And 'NULL' here is ignored somehow. It is incorrect and should be caught
by core, in alter.cc, because it is not a grammar issue.

> +	for (uint32_t i = 0; i < p->def->field_count; i++) {
> +		if (p->def->fields[i].nullable_action ==
> +		    ON_CONFLICT_ACTION_UNDEFINED) {
> +			p->def->fields[i].nullable_action =
> +				ON_CONFLICT_ACTION_NONE;
> +			p->def->fields[i].is_nullable = true;
> +		} else if (p->iPKey == (int)i &&
> +			    p->def->fields[i].nullable_action ==
> +			    ON_CONFLICT_ACTION_NONE) {
> +			/*
> +			 * PRIMARY KEY can not be defined with
> +			 * ON_CONFLICT_ACTION_NONE.
> +			 */
> +			const char *err_str =
> +				tt_sprintf("Cannot define PRIMARY KEY "
> +					   "constraint on nullable column in "
> +					   "table '%s'", p->def->name);
> +			diag_set(ClientError, ER_SQL, err_str);
> +			pParse->rc = SQL_TARANTOOL_ERROR;
> +			pParse->nErr++;
> +			return;
> +		}
> +	}
> +
>   	if (!p->def->opts.is_view) {
>   		if ((p->tabFlags & TF_HasPrimaryKey) == 0) {
>   			sqlite3ErrorMsg(pParse,
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index ac935fd..6463019 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -276,7 +276,7 @@ ccons ::= DEFAULT id(X).              {
>   // In addition to the type name, we also care about the primary key and
>   // UNIQUE constraints.
>   //
> -ccons ::= NULL onconf.
> +ccons ::= NULL onconf.           {sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);}

5. I do this:

     box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE)")

But ON CONFLICT makes no sense for nullable columns.

>   ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
>   ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
>                                    {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}

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

[Kirill Shcherbatov]

Hi! Thank you for review.

On 13.07.2018 13:26, Vladislav Shpilevoy wrote:
> Hello. See 5 comments below.
> 
> 1. Assertion.
non actual already.

> 2. Please, remove this new action, because
> * it is not really action, but just a flag;
> * it can be replaced with on_conflict_action_MAX or
>    ON_CONFLICT_ACTION_DEFAULT.
Implemented with on_conflict_action_MAX 
> 3. Why do you cast to u8 though nullable_action is enum?
field->nullable_action = onError;

> 4. This still works:
> 5. I do this:
> 
>      box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE)")
> 
> But ON CONFLICT makes no sense for nullable columns.
Included in tests.

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


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           | 83 ++++++++++++++++++++++++++++++++++++++-----
 src/box/sql/parse.y           |  2 +-
 test/sql/on-conflict.result   | 21 +++++++++++
 test/sql/on-conflict.test.lua |  8 +++++
 4 files changed, 105 insertions(+), 9 deletions(-)

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
@@ -653,7 +653,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) {
@@ -701,9 +706,22 @@ sqlite3AddNotNull(Parse * pParse, int onError)
 	p = pParse->pNewTable;
 	if (p == 0 || 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);
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
+		return;
+	}
+	field->nullable_action = onError;
+	field->is_nullable = action_is_nullable(onError);
 }
 
 /*
@@ -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)
+				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);
+				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 &&
+			   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;
+		}
+	}
+
 	if (check_on_conflict_replace_entries(p)) {
 		sqlite3ErrorMsg(pParse,
 				"only PRIMARY KEY constraint can "
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ac935fd..4894b8a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -276,7 +276,7 @@ ccons ::= DEFAULT id(X).              {
 // In addition to the type name, we also care about the primary key and
 // UNIQUE constraints.
 //
-ccons ::= NULL onconf.
+ccons ::= NULL.                  {sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);}
 ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
 ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
                                  {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}
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
+...
+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'
+...
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/2] sql: restrict nullable action definitions

[Vladislav Shpilevoy]

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

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

[Kirill Shcherbatov]

> 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.
Thank you for review, I've fixed all problems on branch and squashed.

> 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.
> 2. Please, use ER_NULLABLE_PRIMARY.
> 3. This is checked already, so the rest of hunk below can be removed.
Fixed with review fixes.

> 4. This test does not work, it fails on syntax, but must fail on action.
Ok, I've hacked to make it work this way.


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


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           | 86 +++++++++++++++++++++++++++++++++----------
 src/box/sql/parse.y           |  7 +++-
 test/sql/on-conflict.result   | 21 +++++++++++
 test/sql/on-conflict.test.lua |  8 ++++
 4 files changed, 101 insertions(+), 21 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 8fba373..925193e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -653,7 +653,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) {
@@ -701,9 +706,22 @@ sqlite3AddNotNull(Parse * pParse, int onError)
 	p = pParse->pNewTable;
 	if (p == 0 || 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);
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
+		return;
+	}
+	field->nullable_action = onError;
+	field->is_nullable = action_is_nullable(onError);
 }
 
 /*
@@ -1283,6 +1301,24 @@ hasColumn(const i16 * aiCol, int nCol, int x)
 	return 0;
 }
 
+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
@@ -1301,18 +1337,8 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 	Index *pPk;
 	int i, j;
 	sqlite3 *db = pParse->db;
-
-	/* Mark every PRIMARY KEY column as NOT NULL (except for imposter tables)
-	 */
-	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;
-			}
-		}
-	}
+	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.
@@ -1320,7 +1346,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));
@@ -1343,11 +1372,17 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 		 * "PRIMARY KEY(a,b,a,b,c,b,c,d)" into just "PRIMARY KEY(a,b,c,d)".  Later
 		 * code assumes the PRIMARY KEY contains no repeated columns.
 		 */
-		for (i = j = 1; i < pPk->nColumn; i++) {
-			if (hasColumn(pPk->aiColumn, j, pPk->aiColumn[i])) {
+		u16 pk_columns = pPk->nColumn;
+		for (i = j = 0; i < pk_columns; i++) {
+			uint32_t part_no = pPk->aiColumn[i];
+			if (hasColumn(pPk->aiColumn, j, part_no)) {
 				pPk->nColumn--;
 			} else {
-				pPk->aiColumn[j++] = pPk->aiColumn[i];
+				pPk->aiColumn[j++] = part_no;
+				if (field_def_create_for_pk(pParse,
+							    &fields[part_no],
+							    space_name) != 0)
+					return;
 			}
 		}
 		pPk->nColumn = j;
@@ -1736,6 +1771,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 ac935fd..e2bdc4a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -276,7 +276,12 @@ ccons ::= DEFAULT id(X).              {
 // In addition to the type name, we also care about the primary key and
 // UNIQUE constraints.
 //
-ccons ::= NULL onconf.
+ccons ::= NULL onconf(R).        {
+    sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
+    /* Trigger nullability mismatch error if required. */
+    if (R != ON_CONFLICT_ACTION_DEFAULT)
+        sqlite3AddNotNull(pParse, R);
+}
 ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
 ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
                                  {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}
diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
index c0d0de0..2970cb8 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/2] sql: restrict nullable action definitions

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See 3 comments below.

On 16/07/2018 12:43, Kirill Shcherbatov wrote:
>> 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.
> Thank you for review, I've fixed all problems on branch and squashed.
> 
>> 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.
>> 2. Please, use ER_NULLABLE_PRIMARY.
>> 3. This is checked already, so the rest of hunk below can be removed.
> Fixed with review fixes.

1. No, is_primkey still is not removed. I see it in struct Column.

> 
> 
> ============================================
> 
> 
> 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           | 86 +++++++++++++++++++++++++++++++++----------
>   src/box/sql/parse.y           |  7 +++-
>   test/sql/on-conflict.result   | 21 +++++++++++
>   test/sql/on-conflict.test.lua |  8 ++++
>   4 files changed, 101 insertions(+), 21 deletions(-)
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index ac935fd..e2bdc4a 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -276,7 +276,12 @@ ccons ::= DEFAULT id(X).              {
>   // In addition to the type name, we also care about the primary key and
>   // UNIQUE constraints.
>   //
> -ccons ::= NULL onconf.
> +ccons ::= NULL onconf(R).        {
> +    sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
> +    /* Trigger nullability mismatch error if required. */
> +    if (R != ON_CONFLICT_ACTION_DEFAULT)

2. Why do you need this check?

3. Why sqlite3AddNotNull is called when NULL is allowed?
Please, rename the function or split it or something.

> +        sqlite3AddNotNull(pParse, R);
> +}
>   ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
>   ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
>                                    {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}

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

[Kirill Shcherbatov]

> 1. No, is_primkey still is not removed. I see it in struct Column.
I've removed is_primkey and struct Column at all in separate commit that would be sent next this letter.

>> +ccons ::= NULL onconf(R).        {
>> +    sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
>> +    /* Trigger nullability mismatch error if required. */
>> +    if (R != ON_CONFLICT_ACTION_DEFAULT)
> 
> 2. Why do you need this check?
>>> 4. This test does not work, it fails on syntax, but must fail on action.
>>Ok, I've hacked to make it work this way.
sqlite3AddNotNull would rise and error on rewriting on_conflict;
ON_CONFLICT_ACTION_DEFAULT is returned when no ON CONFLICT action is specified.

> 3. Why sqlite3AddNotNull is called when NULL is allowed?
> Please, rename the function or split it or something.
Renamed to sql_column_nullable_action_add as separate commit.

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

[Kirill Shcherbatov]

---
 src/box/sql/build.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 946b10c..200f7e8 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1762,7 +1762,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 			diag_set(ClientError, ER_SQL, err_str);
 			pParse->rc = SQL_TARANTOOL_ERROR;
 			pParse->nErr++;
-			return;
+			goto cleanup;
 		}
 	}
 
@@ -1771,7 +1771,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 			sqlite3ErrorMsg(pParse,
 					"PRIMARY KEY missing on table %s",
 					p->def->name);
-			return;
+			goto cleanup;
 		} else {
 			convertToWithoutRowidTable(pParse, p);
 		}
@@ -1782,7 +1782,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) {
@@ -1795,7 +1795,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);
 	}
 
@@ -1923,6 +1923,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/2] sql: fixed possible leak in sqlite3EndTable

[Vladislav Shpilevoy]

Thanks for the patch! See 1 comment below.

On 12/07/2018 19:34, Kirill Shcherbatov wrote:
> ---
>   src/box/sql/build.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 946b10c..200f7e8 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1795,7 +1795,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);
>   	}

I see two returns below this line. Why have not you replaced
them with goto cleanup?

>   
> @@ -1923,6 +1923,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;
>   }
> 

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

[Kirill Shcherbatov]

> Thanks for the patch! See 1 comment below.

> I see two returns below this line. Why have not you replaced
> them with goto cleanup?
It is not important after space_def_rebuild. But I re factored code with goto cleanup 
to be consistent.

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

[Vladislav Shpilevoy]

Thanks for the patchset! See 1 comments below.

On 12/07/2018 19:34, Kirill Shcherbatov wrote:
> This patch dissallows define multiple "NULL", "NOT NULL"

Typo.

> options per column and fixes silent implicit behavior
> for invalid "NULL PRIMARY KEY" construction.
> 
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3473-on-conflict-defaults-fixes
> Issue: https://github.com/tarantool/tarantool/issues/3473
> 
> Kirill Shcherbatov (2):
>    sql: restrict nullable action definitions
>    sql: fixed possible leak in sqlite3EndTable
> 
>   src/box/alter.cc              |  1 +
>   src/box/field_def.c           |  1 +
>   src/box/field_def.h           |  3 ++-
>   src/box/sql/build.c           | 53 +++++++++++++++++++++++++++++++++++++------
>   src/box/sql/parse.y           |  2 +-
>   test/sql/on-conflict.result   | 13 +++++++++++
>   test/sql/on-conflict.test.lua |  6 +++++
>   7 files changed, 70 insertions(+), 9 deletions(-)
> 

[tarantool-patches] [PATCH v1 2/4] sql: refactor sqlite3AddNotNull function

[Kirill Shcherbatov]

---
 src/box/sql/build.c     | 22 ++++++++--------------
 src/box/sql/parse.y     |  6 +++---
 src/box/sql/sqliteInt.h | 17 ++++++++++++++++-
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 925193e..f53a37a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -693,18 +693,12 @@ 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;
 	struct field_def *field = &p->def->fields[p->def->field_count - 1];
 	if (field->nullable_action != on_conflict_action_MAX) {
@@ -716,12 +710,12 @@ sqlite3AddNotNull(Parse * pParse, int onError)
 				   on_conflict_action_strs[field->
 							   nullable_action]);
 		diag_set(ClientError, ER_SQL, err_msg);
-		pParse->rc = SQL_TARANTOOL_ERROR;
-		pParse->nErr++;
+		parser->rc = SQL_TARANTOOL_ERROR;
+		parser->nErr++;
 		return;
 	}
-	field->nullable_action = onError;
-	field->is_nullable = action_is_nullable(onError);
+	field->nullable_action = nullable_action;
+	field->is_nullable = action_is_nullable(nullable_action);
 }
 
 /*
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index e2bdc4a..fe77ae0 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -277,12 +277,12 @@ ccons ::= DEFAULT id(X).              {
 // UNIQUE constraints.
 //
 ccons ::= NULL onconf(R).        {
-    sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
+    sql_column_nullable_action_add(pParse, ON_CONFLICT_ACTION_NONE);
     /* Trigger nullability mismatch error if required. */
     if (R != ON_CONFLICT_ACTION_DEFAULT)
-        sqlite3AddNotNull(pParse, R);
+        sql_column_nullable_action_add(pParse, R);
 }
-ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(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 18bf949..c89d256 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3515,7 +3515,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);
 
 /**
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 2/4] sql: refactor sqlite3AddNotNull function

[Vladislav Shpilevoy]

I do not thing the commit is worth itself. Please, merge it into
the previous one. Anyway, most of this patch just changes the
latter.

On 16/07/2018 15:28, Kirill Shcherbatov wrote:
> ---
>   src/box/sql/build.c     | 22 ++++++++--------------
>   src/box/sql/parse.y     |  6 +++---
>   src/box/sql/sqliteInt.h | 17 ++++++++++++++++-
>   3 files changed, 27 insertions(+), 18 deletions(-)
> 

[tarantool-patches] [PATCH v1 4/4] 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/field_def.c     |  1 +
 src/box/field_def.h     |  2 ++
 src/box/sql/alter.c     | 16 +++-------
 src/box/sql/build.c     | 79 +++++++++++++++++--------------------------------
 src/box/sql/resolve.c   | 11 +++----
 src/box/sql/select.c    | 43 ++++++++++-----------------
 src/box/sql/sqliteInt.h | 23 +++++++-------
 7 files changed, 65 insertions(+), 110 deletions(-)

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..4f50988 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,9 @@ 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);
+	if (index_has_column(pk->aiColumn, pk->nColumn,
+			     pNew->def->field_count - 1)) {
 		sqlite3ErrorMsg(pParse, "Cannot add a PRIMARY KEY column");
 		return;
 	}
@@ -299,19 +299,11 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
 	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 = NULL;
 		pNew->def->fields[i].coll_id = COLL_NONE;
 	}
 	pNew->pSchema = db->pSchema;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ec0bc4b..a6f3559 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -114,6 +114,16 @@ sql_finish_coding(struct Parse *parse_context)
 	}
 }
 
+bool
+index_has_column(const i16 *parts, int parts_count, int column_idx)
+{
+	while (parts_count-- > 0) {
+		if (column_idx == *(parts++))
+			return 1;
+	}
+	return 0;
+}
+
 /**
  * This is a function which should be called during execution
  * of sqlite3EndTable. It ensures that only PRIMARY KEY
@@ -131,10 +141,10 @@ check_on_conflict_replace_entries(struct Table *table)
 	for (int i = 0; i < (int)table->def->field_count; i++) {
 		enum on_conflict_action on_error =
 			table->def->fields[i].nullable_action;
+		struct Index *pk = sqlite3PrimaryKeyIndex(table);
 		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
-		    table->aCol[i].is_primkey == false) {
+		    !index_has_column(pk->aiColumn, pk->nColumn, i))
 			return true;
-		}
 	}
 	/* Check all UNIQUE constraints. */
 	for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
@@ -337,7 +347,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. */
@@ -601,7 +610,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;
@@ -639,17 +647,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;
@@ -887,7 +884,6 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
     )
 {
 	Table *pTab = pParse->pNewTable;
-	Column *pCol = 0;
 	int iCol = -1, i;
 	int nTerm;
 	if (pTab == 0)
@@ -901,8 +897,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;
@@ -915,21 +909,19 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 				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;
+			for (uint32_t collumn_id = 0;
+			     collumn_id < pTab->def->field_count; collumn_id++) {
+				if (strcmp(zCName,
+					   pTab->def->fields[collumn_id].name)
+				    == 0) {
+					iCol = collumn_id;
 					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;
@@ -998,8 +990,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) {
 		Index *pIdx;
 		/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
 		 * then an index may have been created on this column before the
@@ -1220,14 +1212,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 = "";
@@ -1245,10 +1234,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",
@@ -1284,17 +1273,6 @@ createTableStmt(sqlite3 * db, Table * p)
 	return zStmt;
 }
 
-/* Return true if value x is found any of the first nCol entries of aiCol[]
- */
-static int
-hasColumn(const i16 * aiCol, int nCol, int x)
-{
-	while (nCol-- > 0)
-		if (x == *(aiCol++))
-			return 1;
-	return 0;
-}
-
 static int
 field_def_create_for_pk(struct Parse *parser, struct field_def *field,
 			const char *space_name)
@@ -1369,7 +1347,7 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 		u16 pk_columns = pPk->nColumn;
 		for (i = j = 0; i < pk_columns; i++) {
 			uint32_t part_no = pPk->aiColumn[i];
-			if (hasColumn(pPk->aiColumn, j, part_no)) {
+			if (index_has_column(pPk->aiColumn, j, part_no)) {
 				pPk->nColumn--;
 			} else {
 				pPk->aiColumn[j++] = part_no;
@@ -1961,12 +1939,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 ceb7e34..745ae2f 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 c89d256..a67627f 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 */
@@ -4418,6 +4407,18 @@ void sqlite3FileSuffix3(const char *, char *);
 #endif
 u8 sqlite3GetBoolean(const char *z, u8);
 
+/**
+ * Test if @column_idx is a part of first @parts_count of index.
+ *
+ * @param parts pointer to array with columns representing index.
+ * @param parts_count count of index parts.
+ * @column_idx index of column to test.
+ * @retval true if index contains column_idx.
+ * @retval false else.
+ */
+bool
+index_has_column(const i16 *parts, int parts_count, int column_idx);
+
 const void *sqlite3ValueText(sqlite3_value *);
 int sqlite3ValueBytes(sqlite3_value *);
 void sqlite3ValueSetStr(sqlite3_value *, int, const void *,
-- 
2.7.4

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

[Vladislav Shpilevoy]

Thanks for the patch! See 6 comments below.

On 16/07/2018 15:28, Kirill Shcherbatov wrote:
> 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/field_def.c     |  1 +
>   src/box/field_def.h     |  2 ++
>   src/box/sql/alter.c     | 16 +++-------
>   src/box/sql/build.c     | 79 +++++++++++++++++--------------------------------
>   src/box/sql/resolve.c   | 11 +++----
>   src/box/sql/select.c    | 43 ++++++++++-----------------
>   src/box/sql/sqliteInt.h | 23 +++++++-------
>   7 files changed, 65 insertions(+), 110 deletions(-)
> 
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index fe54e55..4f50988 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -299,19 +299,11 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
>   	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);

1. And why do you need nAlloc now? Please, do not hurry. It is not ok that
you overlook such conspicuous things.

>   	/* 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); */

2. What does this comment mean?

3. What about other fields? Please, use = field_def_default.

> -		pCol->coll = NULL;
> +		pNew->def->fields[i].coll = NULL;
>   		pNew->def->fields[i].coll_id = COLL_NONE;
>   	}
>   	pNew->pSchema = db->pSchema;
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ec0bc4b..a6f3559 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c> @@ -131,10 +141,10 @@ check_on_conflict_replace_entries(struct Table *table)
>   	for (int i = 0; i < (int)table->def->field_count; i++) {
>   		enum on_conflict_action on_error =
>   			table->def->fields[i].nullable_action;
> +		struct Index *pk = sqlite3PrimaryKeyIndex(table);
>   		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
> -		    table->aCol[i].is_primkey == false) {
> +		    !index_has_column(pk->aiColumn, pk->nColumn, i))

4. No. As I said you on the first review, you should not use here
scans or is_primkey. This check can be spread over other column check
functions, involved in the first commit with no additional scans.

>   			return true;
> -		}
>   	}
>   	/* Check all UNIQUE constraints. */
>   	for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) {
> @@ -915,21 +909,19 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
>   				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;
> +			for (uint32_t collumn_id = 0;
> +			     collumn_id < pTab->def->field_count; collumn_id++) {
> +				if (strcmp(zCName,
> +					   pTab->def->fields[collumn_id].name)
> +				    == 0) {
> +					iCol = collumn_id;
>   					break;

5. Please, make the code more readable. You should avoid such long names
as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields
in a separate variable etc.

>   				}
>   			}
>   		}
>   	}
> -	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;
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index ceb7e34..745ae2f 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;

6. Why do you need to enclose != NULL into the brackets?

>   	/*
>   	 * This should be a table without resolved columns.
>   	 * sqlite3ViewGetColumnNames could use it to resolve

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

[Kirill Shcherbatov]

Thank you for review.

> 1. And why do you need nAlloc now? Please, do not hurry. It is not ok that
> you overlook such conspicuous things.
-       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);
Dropped.

> 2. What does this comment mean?
> 3. What about other fields? Please, use = field_def_default.
-       for (i = 0; i < pNew->def->field_count; i++) {
-               /* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */
-               pNew->def->fields[i].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;


> 4. No. As I said you on the first review, you should not use here
> scans or is_primkey. This check can be spread over other column check
> functions, involved in the first commit with no additional scans.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a6f3559..3449d3c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -126,38 +126,48 @@ index_has_column(const i16 *parts, int parts_count, int column_idx)
 
 /**
  * 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;
-		struct Index *pk = sqlite3PrimaryKeyIndex(table);
-		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
+static int
+check_on_conflict_replace_entries(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 &&
 		    !index_has_column(pk->aiColumn, pk->nColumn, i))
-			return true;
+			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;
 }
 
 /*
@@ -1748,22 +1758,8 @@ 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 (check_on_conflict_replace_entries(pParse, p))
 		goto cleanup;
-	}
 
 	if (db->init.busy) {
 		/*
@@ -1906,6 +1902,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 cleanup:
 	sql_expr_list_delete(db, p->def->opts.checks);
 	p->def->opts.checks = NULL;
+	return;
 }

> 5. Please, make the code more readable. You should avoid such long names
> as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields
> in a separate variable etc.
@@ -905,16 +915,15 @@ 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 (uint32_t collumn_id = 0;
-                            collumn_id < pTab->def->field_count; collumn_id++) {
-                               if (strcmp(zCName,
-                                          pTab->def->fields[collumn_id].name)
-                                   == 0) {
-                                       iCol = collumn_id;
+                       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;
                                }
                        }


> 6. Why do you need to enclose != NULL into the brackets?
-               (expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0;
+               expr_list != NULL ? (uint32_t)expr_list->nExpr : 0;

Hm, It's is a little more convenient to read for me. I'll try to be a little more pedantic.

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

[Vladislav Shpilevoy]

Hello. Thanks for the patch! But it again is raw. You skipped
field_def.coll initialization for non-sql code, alter.cc etc.

On 16/07/2018 19:33, Kirill Shcherbatov wrote:
> Thank you for review.
> 
>> 1. And why do you need nAlloc now? Please, do not hurry. It is not ok that
>> you overlook such conspicuous things.
> -       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);
> Dropped.
> 
>> 2. What does this comment mean?
>> 3. What about other fields? Please, use = field_def_default.
> -       for (i = 0; i < pNew->def->field_count; i++) {
> -               /* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */
> -               pNew->def->fields[i].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;
> 
> 
>> 4. No. As I said you on the first review, you should not use here
>> scans or is_primkey. This check can be spread over other column check
>> functions, involved in the first commit with no additional scans.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a6f3559..3449d3c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -126,38 +126,48 @@ index_has_column(const i16 *parts, int parts_count, int column_idx)
>   
>   /**
>    * 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;
> -		struct Index *pk = sqlite3PrimaryKeyIndex(table);
> -		if (on_error == ON_CONFLICT_ACTION_REPLACE &&
> +static int
> +check_on_conflict_replace_entries(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 &&
>   		    !index_has_column(pk->aiColumn, pk->nColumn, i))
> -			return true;
> +			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;
>   }
>   
>   /*
> @@ -1748,22 +1758,8 @@ 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 (check_on_conflict_replace_entries(pParse, p))
>   		goto cleanup;
> -	}
>   
>   	if (db->init.busy) {
>   		/*
> @@ -1906,6 +1902,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
>   cleanup:
>   	sql_expr_list_delete(db, p->def->opts.checks);
>   	p->def->opts.checks = NULL;
> +	return;
>   }
> 
>> 5. Please, make the code more readable. You should avoid such long names
>> as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields
>> in a separate variable etc.
> @@ -905,16 +915,15 @@ 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 (uint32_t collumn_id = 0;
> -                            collumn_id < pTab->def->field_count; collumn_id++) {
> -                               if (strcmp(zCName,
> -                                          pTab->def->fields[collumn_id].name)
> -                                   == 0) {
> -                                       iCol = collumn_id;
> +                       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;
>                                  }
>                          }
> 
> 
>> 6. Why do you need to enclose != NULL into the brackets?
> -               (expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0;
> +               expr_list != NULL ? (uint32_t)expr_list->nExpr : 0;
> 
> Hm, It's is a little more convenient to read for me. I'll try to be a little more pedantic.
> 

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

[Kirill Shcherbatov]

Hi. I've rebased my this branch on actual 2.0 with Ivan's Index refactoring.

On 17.07.2018 12:32, Vladislav Shpilevoy wrote:
> Hello. Thanks for the patch! But it again is raw. You skipped
> field_def.coll initialization for non-sql code, alter.cc etc.

It was a single place in alter.cc in field_def_decode, if I have correctly understood you:
struct coll_id *collation = coll_by_id(field->coll_id);
	if (collation != NULL)
		field->coll = collation->coll;

(I've grepped where coll_id is used)

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

[Vladislav Shpilevoy]

Thanks for the patch! See 2 comments below.

> commit 5d1010dfcd7d0666774afaaf5934de018f99e181
> Author: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date:   Mon Jul 16 14:21:54 2018 +0300
> 
>     sql: get rid of Column structure
>     
>     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.
> 
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index fe54e5531..c2a211c95 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,8 @@ 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);
> +	if (index_has_column(pk, pNew->def->field_count - 1)) {

1. Please, rebase on the latest 2.0 and use pk->def + key_def_find.

>  		sqlite3ErrorMsg(pParse, "Cannot add a PRIMARY KEY column");
>  		return;
>  	}
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ee97ef9ee..ce4012081 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -114,40 +114,70 @@ sql_finish_coding(struct Parse *parse_context)
>  	}
>  }
>  
> +/**
> + * Test if @column_idx is a part of @index.
> + *
> + * @param index to lookup.
> + * @column_idx index of column to test.
> + * @retval true if index contains column_idx.
> + * @retval false else.
> + */
> +bool
> +index_has_column(struct Index *index, uint32_t  column_idx)
> +{
> +	uint32_t parts_count = index->def->key_def->part_count;
> +	struct key_part *parts = index->def->key_def->parts;
> +	for (uint32_t i = 0; i < parts_count; i++) {
> +		if (column_idx == parts[i].fieldno)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  /**
>   * 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 &&
> +		    !index_has_column(pk, i))

2. As I said verbally, you should not add this redundant scan of primary
index columns. You already have the primary index scan in
convertToWithoutRowidTable, that is called few lines above.

When you merge this scan into convertToWithoutRowidTable, you can
inline the rest of the function into EndTable and remove it together
with index_has_column.

> +			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;
>  }
>  

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

[Kirill Shcherbatov]

> 1. Please, rebase on the latest 2.0 and use pk->def + key_def_find.
-       if (index_has_column(pk, pNew->def->field_count - 1)) {
+       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) {

> 2. As I said verbally, you should not add this redundant scan of primary
> index columns. You already have the primary index scan in
> convertToWithoutRowidTable, that is called few lines above.
> 
> When you merge this scan into convertToWithoutRowidTable, you can
> inline the rest of the function into EndTable and remove it together
> with index_has_column.
I can't understand your suggestion. 
convertToWithoutRowidTable iterates by index field;
but this check is a about field with ON CONFLICT REPLACE could be only a part
of index. We iterate over all fields.

I've get rid off index_has_column. 

                if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
-                   !index_has_column(pk, i))
+                   (pk == NULL || key_def_find(pk->def->key_def, i) == NULL))


-bool
-index_has_column(struct Index *index, uint32_t  column_idx)
etc.

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

[Vladislav Shpilevoy]

LGTM. Nikita, please, review.

On 18/07/2018 10:25, Kirill Shcherbatov wrote:
>> 1. Please, rebase on the latest 2.0 and use pk->def + key_def_find.
> -       if (index_has_column(pk, pNew->def->field_count - 1)) {
> +       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) {
> 
>> 2. As I said verbally, you should not add this redundant scan of primary
>> index columns. You already have the primary index scan in
>> convertToWithoutRowidTable, that is called few lines above.
>>
>> When you merge this scan into convertToWithoutRowidTable, you can
>> inline the rest of the function into EndTable and remove it together
>> with index_has_column.
> I can't understand your suggestion.
> convertToWithoutRowidTable iterates by index field;
> but this check is a about field with ON CONFLICT REPLACE could be only a part
> of index. We iterate over all fields.
> 
> I've get rid off index_has_column.
> 
>                  if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE &&
> -                   !index_has_column(pk, i))
> +                   (pk == NULL || key_def_find(pk->def->key_def, i) == NULL))
> 
> 
> -bool
> -index_has_column(struct Index *index, uint32_t  column_idx)
> etc.
> 

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

[n.pettik]

> LGTM. Nikita, please, review.

Kirill, could you please resend patch after Vlad’s review fixes? Thx.