[tarantool-patches] [PATCH v1 1/1] sql: fix assertion on invalid PK column name

[tarantool-patches] [PATCH v1 1/1] sql: fix assertion on invalid PK column name

[Kirill Shcherbatov]

Close #3522.
---
https://github.com/tarantool/tarantool/compare/kshch/gh-3522-invalid-primary-key-column-name
https://github.com/tarantool/tarantool/issues/3522

 src/box/sql/build.c                   | 1 -
 test/sql/gh-2929-primary-key.result   | 7 +++++++
 test/sql/gh-2929-primary-key.test.lua | 5 +++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e1d896e..5584d83 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -964,7 +964,6 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 			}
 		}
 	}
-	assert(pCol == &pTab->aCol[iCol]);
 	if (nTerm == 1 && pCol != NULL &&
 	    (pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER) &&
 	    sortOrder != SORT_ORDER_DESC) {
diff --git a/test/sql/gh-2929-primary-key.result b/test/sql/gh-2929-primary-key.result
index c238ed7..66a9b96 100644
--- a/test/sql/gh-2929-primary-key.result
+++ b/test/sql/gh-2929-primary-key.result
@@ -35,3 +35,10 @@ box.sql.execute("CREATE TABLE t5(a, b UNIQUE)")
 box.sql.execute("DROP TABLE t1")
 ---
 ...
+--
+-- gh-3522: invalid primary key name
+--
+box.sql.execute("CREATE TABLE tx (a INT, PRIMARY KEY (b));")
+---
+- error: 'no such column: B'
+...
diff --git a/test/sql/gh-2929-primary-key.test.lua b/test/sql/gh-2929-primary-key.test.lua
index 0a9395c..0e05354 100644
--- a/test/sql/gh-2929-primary-key.test.lua
+++ b/test/sql/gh-2929-primary-key.test.lua
@@ -16,3 +16,8 @@ box.sql.execute("CREATE TABLE t4(a, b)")
 box.sql.execute("CREATE TABLE t5(a, b UNIQUE)")
 
 box.sql.execute("DROP TABLE t1")
+
+--
+-- gh-3522: invalid primary key name
+--
+box.sql.execute("CREATE TABLE tx (a INT, PRIMARY KEY (b));")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix assertion on invalid PK column name

[Vladislav Shpilevoy]

Hello. Thanks for the patch!

I know, that removal of assertion always 'fixes' it, but please,
either fix it without removal (and for me it is not obvious why it
should be removed), or explain, why it is invalid.

On 12/07/2018 09:30, Kirill Shcherbatov wrote:
> Close #3522.
> ---
> https://github.com/tarantool/tarantool/compare/kshch/gh-3522-invalid-primary-key-column-name
> https://github.com/tarantool/tarantool/issues/3522
> 
>   src/box/sql/build.c                   | 1 -
>   test/sql/gh-2929-primary-key.result   | 7 +++++++
>   test/sql/gh-2929-primary-key.test.lua | 5 +++++
>   3 files changed, 12 insertions(+), 1 deletion(-)
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix assertion on invalid PK column name

[Kirill Shcherbatov]

> I know, that removal of assertion always 'fixes' it, but please,
> either fix it without removal (and for me it is not obvious why it
> should be removed), or explain, why it is invalid.
I've included this assertion during refactoring and there is no root causes to
keep it, but as we verbally discuss, let's make it working correctly appending
additional pCol == NULL OR condition that code below correctly deal with.

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

If specified PK column modifier refers nonexistent column,
lookup by name keeps NULL column pointer unchanged that should
be accounted in assert.

Close #3522.
---
https://github.com/tarantool/tarantool/compare/kshch/gh-3522-invalid-primary-key-column-name
https://github.com/tarantool/tarantool/issues/3522

 src/box/sql/build.c                   | 2 +-
 test/sql/gh-2929-primary-key.result   | 7 +++++++
 test/sql/gh-2929-primary-key.test.lua | 5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e1d896e..5d04e3e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -964,7 +964,7 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 			}
 		}
 	}
-	assert(pCol == &pTab->aCol[iCol]);
+	assert(pCol == NULL || pCol == &pTab->aCol[iCol]);
 	if (nTerm == 1 && pCol != NULL &&
 	    (pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER) &&
 	    sortOrder != SORT_ORDER_DESC) {
diff --git a/test/sql/gh-2929-primary-key.result b/test/sql/gh-2929-primary-key.result
index c238ed7..66a9b96 100644
--- a/test/sql/gh-2929-primary-key.result
+++ b/test/sql/gh-2929-primary-key.result
@@ -35,3 +35,10 @@ box.sql.execute("CREATE TABLE t5(a, b UNIQUE)")
 box.sql.execute("DROP TABLE t1")
 ---
 ...
+--
+-- gh-3522: invalid primary key name
+--
+box.sql.execute("CREATE TABLE tx (a INT, PRIMARY KEY (b));")
+---
+- error: 'no such column: B'
+...
diff --git a/test/sql/gh-2929-primary-key.test.lua b/test/sql/gh-2929-primary-key.test.lua
index 0a9395c..0e05354 100644
--- a/test/sql/gh-2929-primary-key.test.lua
+++ b/test/sql/gh-2929-primary-key.test.lua
@@ -16,3 +16,8 @@ box.sql.execute("CREATE TABLE t4(a, b)")
 box.sql.execute("CREATE TABLE t5(a, b UNIQUE)")
 
 box.sql.execute("DROP TABLE t1")
+
+--
+-- gh-3522: invalid primary key name
+--
+box.sql.execute("CREATE TABLE tx (a INT, PRIMARY KEY (b));")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix assertion on invalid PK column name

[Vladislav Shpilevoy]

LGTM.

On 12/07/2018 13:03, Kirill Shcherbatov wrote:
>> I know, that removal of assertion always 'fixes' it, but please,
>> either fix it without removal (and for me it is not obvious why it
>> should be removed), or explain, why it is invalid.
> I've included this assertion during refactoring and there is no root causes to
> keep it, but as we verbally discuss, let's make it working correctly appending
> additional pCol == NULL OR condition that code below correctly deal with.
> 
> ========================================
> 
> If specified PK column modifier refers nonexistent column,
> lookup by name keeps NULL column pointer unchanged that should
> be accounted in assert.
> 
> Close #3522.
> ---
> https://github.com/tarantool/tarantool/compare/kshch/gh-3522-invalid-primary-key-column-name
> https://github.com/tarantool/tarantool/issues/3522
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix assertion on invalid PK column name

[Kirill Yukhin]

Hello,
On 12 июл 09:30, Kirill Shcherbatov wrote:
> Close #3522.
> ---
> https://github.com/tarantool/tarantool/compare/kshch/gh-3522-invalid-primary-key-column-name
> https://github.com/tarantool/tarantool/issues/3522
I've checked the patch into 2.0 branch.

--
Regards, Kirill Yukhin