[tarantool-patches] Re: [PATCH] sql: disallow id duplicate in insert & update

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

LGTM.

On 09/04/2018 07:29, AKhatskevich wrote:
> Before this patch one was able to exec query like this:
>    `insert into t(col1, col1) values(1, 1)`
>    `update t set col1 = 1, col1 = 2;`
> which is nonsense.
> 
> New policy is to throw an error in case of duplicates in columns
> description.
> 
> Closes #2358
> ---
>   src/box/sql/insert.c          | 15 +++++++++++++++
>   src/box/sql/sqliteLimit.h     |  9 +++++++++
>   src/box/sql/update.c          |  7 +++++++
>   test/sql-tap/insert1.test.lua | 30 +++++++++++++++++++++++++++++-
>   test/sql-tap/update.test.lua  | 23 ++++++++++++++++++++++-
>   5 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 42254ddf4..33175e54c 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -35,6 +35,7 @@
>    */
>   #include "sqliteInt.h"
>   #include "box/session.h"
> +#include "bit/bit.h"
>   
>   /*
>    * Generate code that will open pTab as cursor iCur.
> @@ -450,6 +451,12 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>   	 * is appears in the original table.  (The index of the INTEGER
>   	 * PRIMARY KEY in the original table is pTab->iPKey.)
>   	 */
> +	/* Create bitmask to mark used columns of the table. */
> +	void *used_columns = tt_static_buf();
> +	/* The size of used_columns buffer is checked during compilation time
> +	 * using SQLITE_MAX_COLUMN constant.
> +	 */
> +	memset(used_columns, 0, (pTab->nCol + 7) / 8);
>   	bIdListInOrder = 1;
>   	if (pColumn) {
>   		for (i = 0; i < pColumn->nId; i++) {
> @@ -477,6 +484,14 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>   				pParse->checkSchema = 1;
>   				goto insert_cleanup;
>   			}
> +			if (bit_test(used_columns, j)) {
> +				const char *err;
> +				err = "table id list: duplicate column name %s";
> +				sqlite3ErrorMsg(pParse,
> +						err, pColumn->a[i].zName);
> +				goto insert_cleanup;
> +			}
> +			bit_set(used_columns, j);
>   		}
>   	}
>   
> diff --git a/src/box/sql/sqliteLimit.h b/src/box/sql/sqliteLimit.h
> index 138268259..f6b580665 100644
> --- a/src/box/sql/sqliteLimit.h
> +++ b/src/box/sql/sqliteLimit.h
> @@ -35,6 +35,7 @@
>    *
>    * This file defines various limits of what SQLite can process.
>    */
> +#include "trivia/util.h"
>   
>   enum {
>   	/*
> @@ -74,6 +75,14 @@ enum {
>   #ifndef SQLITE_MAX_COLUMN
>   #define SQLITE_MAX_COLUMN 2000
>   #endif
> +/*
> + * tt_static_buf() is used to store bitmask for used columns in a table during
> + * SQL parsing stage. The following statement checks if static buffer is big
> + * enough to store the bitmask.
> + */
> +#if SQLITE_MAX_COLUMN > TT_STATIC_BUF_LEN * 8
> +#error "Bitmask for used table columns cannot fit into static buffer"
> +#endif
>   
>   /*
>    * The maximum length of a single SQL statement in bytes.
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index bf413252d..91717d7bd 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -233,6 +233,13 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>   				if (pPk && table_column_is_in_pk(pTab, j)) {
>   					chngPk = 1;
>   				}
> +				if (aXRef[j] != -1) {
> +					sqlite3ErrorMsg(pParse,
> +							"set id list: duplicate"
> +							" column name %s",
> +							pChanges->a[i].zName);
> +					goto update_cleanup;
> +				}
>   				aXRef[j] = i;
>   				break;
>   			}
> diff --git a/test/sql-tap/insert1.test.lua b/test/sql-tap/insert1.test.lua
> index 847b25ad2..750732d37 100755
> --- a/test/sql-tap/insert1.test.lua
> +++ b/test/sql-tap/insert1.test.lua
> @@ -1,7 +1,7 @@
>   #!/usr/bin/env tarantool
>   test = require("sqltester")
>   yaml = require('yaml')
> -test:plan(24)
> +test:plan(28)
>   
>   --!./tcltestrunner.lua
>   -- 2001 September 15
> @@ -516,5 +516,33 @@ test:do_execsql_test("insert-4.7", [[
>   --   SELECT * FROM t12c;
>   -- } {one xyzzy two}
>   -- integrity_check insert-99.0
> +
> +
> +test:do_execsql_test(
> +    "insert-13.0",
> +    [[
> +        create table test(a primary key, b)
> +    ]])
> +
> +test:do_catchsql_test(
> +    "insert-13.1",
> +    [[
> +        insert into test(a, a, b) values(1, 1, 1)
> +    ]],
> +    {1, "table id list: duplicate column name A"})
> +
> +test:do_catchsql_test(
> +    "insert-13.2",
> +    [[
> +        insert into test(a, b, b) values(1, 1, 1)
> +    ]],
> +    {1, "table id list: duplicate column name B"})
> +
> +test:do_execsql_test(
> +    "insert-13.3",
> +    [[
> +        drop table test;
> +    ]])
> +
>   test:finish_test()
>   
> diff --git a/test/sql-tap/update.test.lua b/test/sql-tap/update.test.lua
> index f6e756731..a4efe4015 100755
> --- a/test/sql-tap/update.test.lua
> +++ b/test/sql-tap/update.test.lua
> @@ -1,6 +1,6 @@
>   #!/usr/bin/env tarantool
>   test = require("sqltester")
> -test:plan(108)
> +test:plan(111)
>   
>   --!./tcltestrunner.lua
>   -- 2001 September 15
> @@ -1109,5 +1109,26 @@ test:do_catchsql_test("update-10.10", [[
>   --   UPDATE t15 SET c=printf("y%d",a) WHERE c IS NULL;
>   --   SELECT a,b,c,'|' FROM t15 ORDER BY a;
>   -- } {5 zyx y5 | 10 abc y10 | 15 wvu y15 | 20 def y20 | 25 tsr y25 | 30 ghi y30 | 35 qpo y35 |}
> +
> +test:do_execsql_test(
> +    "insert-15.0",
> +    [[
> +        create table test(a primary key);
> +        insert into test(a) values(1);
> +    ]])
> +
> +test:do_catchsql_test(
> +    "insert-15.1",
> +    [[
> +        update test set a = 2, a = 3;
> +    ]],
> +    {1, "set id list: duplicate column name A"})
> +
> +test:do_execsql_test(
> +  "insert-15.2",
> +  [[
> +      drop table test;
> +  ]])
> +
>   test:finish_test()
>   
>