[tarantool-patches] [PATCH v3 1/4] sql: remove unnecessary AUTOINCREMENT ID generation

[imeevma at tarantool.org]

Hi! Thank you for review. My answers and new patch below. There
won't be diff since I just removed tests.

On 7/8/19 9:54 PM, n.pettik wrote:
>
>
>> On 4 Jul 2019, at 14:42, imeevma at tarantool.org wrote:
>>
>> Currently, if we perform something like
>> CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);
>> INSERT INTO t(a) VALUES (NULL);
>>
>> we generate a new identifier in a special way.
>
> Describe this “special” way.
>
Fixed commit-message.

>> src/box/sql/insert.c                    |  5 +----
>> test/sql/gh-2981-check-autoinc.result   | 32 ++++++++++++++++++++++++++++++++
>> test/sql/gh-2981-check-autoinc.test.lua | 11 ++++++++++-
>> 3 files changed, 43 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index b353148..d2b4e17 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -628,10 +628,7 @@ sqlInsert(Parse * pParse,	/* Parser context */
>> 			if (j < 0 || nColumn == 0
>> 			    || (pColumn && j >= pColumn->nId)) {
>> 				if (i == (int) autoinc_fieldno) {
>> -					sqlVdbeAddOp2(v,
>> -							  OP_NextAutoincValue,
>> -							  space->def->id,
>> -							  iRegStore);
>
> Why didn’t you delete OP_NextAutioncValue?
>
I changed it functionality in the next patch.

>> +					sqlVdbeAddOp2(v, OP_Null, 0, iRegStore);
>> 					continue;
>> 				}
>> 				struct Expr *dflt = NULL;
>>
>> diff --git a/test/sql/gh-2981-check-autoinc.test.lua b/test/sql/gh-2981-check-autoinc.test.lua
>> index 0eb8f73..ac5624e 100644
>> --- a/test/sql/gh-2981-check-autoinc.test.lua
>> +++ b/test/sql/gh-2981-check-autoinc.test.lua
>> @@ -7,6 +7,8 @@ box.cfg{}
>> box.execute("CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));");
>> box.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19 AND s1 <> 25));");
>> box.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));”);
>> +box.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));");
>> +box.execute("CREATE TABLE t5 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));”);
>
> Add explanation to these tests.. What do they verify?
> I build master branch and they are passed as well.
>
I deleted the tests. They don't really show anything as they
should, and this was described in the commit message. They would
be useful if you run them before #3691 was pushed.


New patch:

>From 89267d98f7f336a5c76fd72e3888b83954b32f45 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma at gmail.com>
Date: Wed, 3 Jul 2019 14:05:11 +0300
Subject: [PATCH] sql: remove unnecessary AUTOINCREMENT ID generation

Currently, if we perform something like
CREATE TABLE t1 (
        s1 INTEGER PRIMARY KEY AUTOINCREMENT,
        s2 INTEGER,
        CHECK (s1 <> 19)
);
INSERT INTO t1 VALUES (18, NULL);
INSERT INTO t1 (s2) VALUES (NULL);

we generate a new identifier in VDBE, but in any other case we
generate it in BOX. That was needed since the CHECK did not work
properly. This is not necessary now, because CHECK was moved to
BOX due to issue #3691. After this patch all new identifiers will
be generated in BOX.

Part of #4188

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b353148..d2b4e17 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -628,10 +628,7 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			if (j < 0 || nColumn == 0
 			    || (pColumn && j >= pColumn->nId)) {
 				if (i == (int) autoinc_fieldno) {
-					sqlVdbeAddOp2(v,
-							  OP_NextAutoincValue,
-							  space->def->id,
-							  iRegStore);
+					sqlVdbeAddOp2(v, OP_Null, 0, iRegStore);
 					continue;
 				}
 				struct Expr *dflt = NULL;