[tarantool-patches] [PATCH v2 0/3] sql: do not show IDs generated by trigger

[tarantool-patches] [PATCH v2 0/3] sql: do not show IDs generated by trigger

[imeevma]

Currently, if an INSERT was executed by a trigger during the
execution of a statement, if there were any generated identifiers,
they can be displayed as a result of the statement. This is
incorrect, since we are not able to divide the IDs obtained into
those that belong to the table mentioned in the statement and
those that do not belong to this table.

https://github.com/tarantool/tarantool/issues/4188
https://github.com/tarantool/tarantool/tree/imeevma/gh-4188-remove-additonal-generated-ids

Changes in v2:
1) Patch was divided into 3 new patches.

Mergen Imeev (3):
  sql: remove unnecessary AUTOINCREMENT ID generation
  sql: do not show IDs generated by trigger
  sql: remove VDBE from TXN

 src/box/errcode.h                       |  1 +
 src/box/sequence.c                      | 22 +++++++++----
 src/box/sequence.h                      | 13 ++++++--
 src/box/sql/insert.c                    | 15 ++++++---
 src/box/sql/parse.y                     |  4 +--
 src/box/sql/sqlInt.h                    |  2 +-
 src/box/sql/trigger.c                   | 12 +++----
 src/box/sql/vdbe.c                      | 25 ++++++++-------
 src/box/sql/vdbe.h                      |  3 +-
 src/box/sql/vdbeInt.h                   |  2 +-
 src/box/sql/vdbeaux.c                   | 10 +++---
 src/box/txn.h                           | 17 ----------
 test/box/misc.result                    |  1 +
 test/sql/gh-2981-check-autoinc.result   | 32 ++++++++++++++++++
 test/sql/gh-2981-check-autoinc.test.lua | 11 ++++++-
 test/sql/iproto.result                  |  2 --
 test/sql/triggers.result                | 57 +++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua              | 14 ++++++++
 18 files changed, 177 insertions(+), 66 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v2 1/3] sql: remove unnecessary AUTOINCREMENT ID generation

[imeevma]

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. This was necessary
for the cases described in issue #2981. Here it is:
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);

In this case, the CHECK did not work properly. This is not
necessary now, because CHECK was moved to BOX due to issue #3691.
This patch removes the mentioned special way.

Part of #4188
---
 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);
+					sqlVdbeAddOp2(v, OP_Null, 0, iRegStore);
 					continue;
 				}
 				struct Expr *dflt = NULL;
diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result
index f03858a..8583115 100644
--- a/test/sql/gh-2981-check-autoinc.result
+++ b/test/sql/gh-2981-check-autoinc.result
@@ -23,6 +23,14 @@ box.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER,
 ---
 - row_count: 1
 ...
+box.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));");
+---
+- row_count: 1
+...
+box.execute("CREATE TABLE t5 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));");
+---
+- row_count: 1
+...
 box.execute("insert into t1 values (18, null);")
 ---
 - row_count: 1
@@ -55,6 +63,22 @@ box.execute("insert into t3(s2) values (null)")
 ---
 - error: 'Check constraint failed ''CK_CONSTRAINT_1_T3'': s1 < 10'
 ...
+box.execute("insert into t4 values (9, null)")
+---
+- row_count: 1
+...
+box.execute("insert into t4 values (null, null)")
+---
+- error: 'Check constraint failed ''CK_CONSTRAINT_1_T4'': s1 < 10'
+...
+box.execute("INSERT INTO t5 VALUES (18, NULL);")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO t5 SELECT NULL, NULL FROM t5;")
+---
+- error: 'Check constraint failed ''CK_CONSTRAINT_1_T5'': s1 <> 19'
+...
 box.execute("DROP TABLE t1")
 ---
 - row_count: 1
@@ -67,3 +91,11 @@ box.execute("DROP TABLE t3")
 ---
 - row_count: 1
 ...
+box.execute("DROP TABLE t4")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE t5")
+---
+- row_count: 1
+...
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));");
 
 box.execute("insert into t1 values (18, null);")
 box.execute("insert into t1(s2) values (null);")
@@ -19,7 +21,14 @@ box.execute("insert into t2(s2) values (null);")
 box.execute("insert into t3 values (9, null)")
 box.execute("insert into t3(s2) values (null)")
 
+box.execute("insert into t4 values (9, null)")
+box.execute("insert into t4 values (null, null)")
+
+box.execute("INSERT INTO t5 VALUES (18, NULL);")
+box.execute("INSERT INTO t5 SELECT NULL, NULL FROM t5;")
+
 box.execute("DROP TABLE t1")
 box.execute("DROP TABLE t2")
 box.execute("DROP TABLE t3")
-
+box.execute("DROP TABLE t4")
+box.execute("DROP TABLE t5")
-- 
2.7.4

[tarantool-patches] [PATCH v2 2/3] sql: do not show IDs generated by trigger

[imeevma]

Hi! Thank you for review. My answers and new patch below.

On 7/3/19 3:25 AM, n.pettik wrote:
>
>
>> On 25 Jun 2019, at 17:09, Imeev Mergen <imeevma@tarantool.org> wrote:
>> Hi! Thank you for review! My answers below.
>> On 6/7/19 3:38 AM, n.pettik wrote:
>>>> On 29 May 2019, at 20:09, imeevma@tarantool.org wrote:
>>>>
>>>> Currently, if INSERT is executed by a trigger, the new generated
>>>> identifiers will be stored in the VDBE. This is wrong, and this
>>>> patch fixes it.
>>> Why is it so?
>> Currently, we are not able to divide the received IDS into those
>> that belong to the mentioned table and those that do not belong.
>
> Ok, now it is clear. Add explanation to the commit message.
>
Added.

>>>> Only identifiers generated during INSERT run by
>>>> the user will be saved.
>>> Firstly, I truly do not understand why this behaviour
>>> is considered to be buggy. I don’t see discussion or
>>> bug description.
>> Now this discussion can be viewed on the dev mailing list.
>>> Secondly, I do not understand what happens in this patch.
>>> Could you please explain these changes and argue why
>>> they are needed?
>> Changes in this patch:
>> 1) At first I removed changes made in patch #2981 since it isn't
>> necessary now. Due to this, all the generation of new identifiers
>> is now performed in BOX.
>> 2) I removed VDBE from TXN. So now sequence_next() does not save
>> the generated IDs in VDBE.
>> 3) I changed the way OP_NextAutoincValue works. Now it is executed
>> when there is an insert in the field with AUTOINCREMENT. If NULL
>> was inserted it receives the last generated ID using the new
>> function sequence_get_value(), and stores it in VDBE. Otherwise it
>> is a no-op.
>
> Please, split this patch into patches according to your points,
> and re-send new patch-set.
>
Done.

>> 4) I added an extra argument to the sqlInsert() function, which
>> makes it clear that the insertion was done in a trigger.
>>
>> My point about these changes:
>> 1 - I think it should be done anyway.
>> 2 - It doesn't look like VDBE should be stored in TXN.
>
> Please, support such statements with arguments.
>
1 - it worked only for cases 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);

But it doesn't worked for these cases:
CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));
INSERT INTO t1 VALUES (18, NULL);
INSERT INTO t1 VALUES (NULL, NULL);

CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));
INSERT INTO t1 VALUES (18, NULL);
INSERT INTO t1 SELECT NULL, NULL FROM t1;

In addition, after the CHECK has been moved to the BOX, it works
correctly for all cases. So now it is not necessary.

However, I can actually leave it at that. Then I need to create a
new opcode if I want to solve #4188 using my solution.

2 - sorry i can't say exactly why. But Gosha told me about it when
I asked him verbally, and Kostja O. and Gosha mentioned it in the
server chat. I think this can be found in messages for April or
March.


New patch:

From d275de15fb14cf8ca9c8361602ed62d6d68e8aaf Mon Sep 17 00:00:00 2001
Date: Tue, 28 May 2019 17:41:15 +0300
Subject: [PATCH] sql: do not show IDs generated by trigger

Currently, if an INSERT was executed by a trigger during the
execution of a statement, if there were any generated identifiers,
they can be displayed as a result of the statement. This is
incorrect, since we are not able to divide the IDs obtained into
those that belong to the table mentioned in the statement and
those that do not belong to this table.

For example:
box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
box.execute('INSERT INTO t2 VALUES (100);')
box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')

Result should be:
---
- autoincrement_ids:
  - 1
  - 2
  - 3
  row_count: 3
...

Closes #4188

diff --git a/src/box/errcode.h b/src/box/errcode.h
index be8dab2..6d22b09 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -250,6 +250,7 @@ struct errcode_record {
 	/*195 */_(ER_CREATE_CK_CONSTRAINT,	"Failed to create check constraint '%s': %s") \
 	/*196 */_(ER_CK_CONSTRAINT_FAILED,	"Check constraint failed '%s': %s") \
 	/*197 */_(ER_SQL_COLUMN_COUNT,		"Unequal number of entries in row expression: left side has %u, but right side - %u") \
+	/*198 */_(ER_SEQUENCE_NO_ELEMENTS,	"Sequence '%s' has no elements") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sequence.c b/src/box/sequence.c
index c9828c0..ee8a8d1 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -195,9 +195,6 @@ sequence_next(struct sequence *seq, int64_t *result)
 					  new_data) == light_sequence_end)
 			return -1;
 		*result = def->start;
-		if (txn_vdbe() != NULL &&
-		    vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
-			return -1;
 		return 0;
 	}
 	old_data = light_sequence_get(&sequence_data_index, pos);
@@ -232,9 +229,6 @@ done:
 				   new_data, &old_data) == light_sequence_end)
 		unreachable();
 	*result = value;
-	if (txn_vdbe() != NULL &&
-	    vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
-		return -1;
 	return 0;
 overflow:
 	if (!def->cycle) {
@@ -347,3 +341,19 @@ sequence_data_iterator_create(void)
 	light_sequence_iterator_freeze(&sequence_data_index, &iter->iter);
 	return &iter->base;
 }
+
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value)
+{
+	uint32_t key = seq->def->id;
+	uint32_t hash = sequence_hash(key);
+	uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
+	if (pos == light_sequence_end) {
+		diag_set(ClientError, ER_SEQUENCE_NO_ELEMENTS, seq->def->name);
+		return -1;
+	}
+	struct sequence_data data = light_sequence_get(&sequence_data_index,
+						       pos);
+	*out_value = data.value;
+	return 0;
+}
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 4419427..2acb6da 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -140,9 +140,6 @@ sequence_next(struct sequence *seq, int64_t *result);
 int
 access_check_sequence(struct sequence *seq);
 
-int
-vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
-
 /**
  * Create an iterator over sequence data.
  *
@@ -153,6 +150,16 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
 struct snapshot_iterator *
 sequence_data_iterator_create(void);
 
+/**
+ * Get last element of given sequence.
+ *
+ * @param seq sequence to get value from.
+ * @param[out] out_value last element of sequence.
+ * @retval 0 on success, -1 on error.
+ */
+int
+sequence_get_value(struct sequence *seq, int64_t *out_value);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d2b4e17..c331cc5 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -233,7 +233,8 @@ sqlInsert(Parse * pParse,	/* Parser context */
 	      SrcList * pTabList,	/* Name of table into which we are inserting */
 	      Select * pSelect,	/* A SELECT statement to use as the data source */
 	      IdList * pColumn,	/* Column names corresponding to IDLIST. */
-	      enum on_conflict_action on_error)
+	      enum on_conflict_action on_error,
+	      bool is_triggered)
 {
 	sql *db;		/* The main database structure */
 	char *zTab;		/* Name of the table into which we are inserting */
@@ -738,6 +739,13 @@ sqlInsert(Parse * pParse,	/* Parser context */
 		vdbe_emit_insertion_completion(v, space, regIns + 1,
 					       space->def->field_count,
 					       on_error);
+		/*
+		 * Save the newly autogenerated value to VDBE.
+		 */
+		if (!is_triggered && autoinc_fieldno < UINT32_MAX) {
+			sqlVdbeAddOp2(v, OP_NextAutoincValue, space->def->id,
+				      regData + autoinc_fieldno);
+		}
 	}
 
 	/* Update the count of rows that are inserted
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 010feff..50ddd4b 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -826,7 +826,7 @@ cmd ::= with(W) insert_cmd(R) INTO fullname(X) idlist_opt(F) select(S). {
   sqlSubProgramsRemaining = SQL_MAX_COMPILING_TRIGGERS;
   /* Instruct SQL to initate Tarantool's transaction.  */
   pParse->initiateTTrans = true;
-  sqlInsert(pParse, X, S, F, R);
+  sqlInsert(pParse, X, S, F, R, false);
 }
 cmd ::= with(W) insert_cmd(R) INTO fullname(X) idlist_opt(F) DEFAULT VALUES.
 {
@@ -834,7 +834,7 @@ cmd ::= with(W) insert_cmd(R) INTO fullname(X) idlist_opt(F) DEFAULT VALUES.
   sqlSubProgramsRemaining = SQL_MAX_COMPILING_TRIGGERS;
   /* Instruct SQL to initate Tarantool's transaction.  */
   pParse->initiateTTrans = true;
-  sqlInsert(pParse, X, 0, F, R);
+  sqlInsert(pParse, X, 0, F, R, false);
 }
 
 %type insert_cmd {int}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 73dc6e4..5dcfd34 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3003,7 +3003,7 @@ sql_store_select(struct Parse *parse_context, struct Select *select);
 void
 sql_drop_table(struct Parse *);
 void sqlInsert(Parse *, SrcList *, Select *, IdList *,
-	       enum on_conflict_action);
+	       enum on_conflict_action, bool);
 void *sqlArrayAllocate(sql *, void *, int, int *, int *);
 
 /**
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index d746ef8..32a7052 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -631,14 +631,10 @@ codeTriggerProgram(Parse * pParse,	/* The parser context */
 				break;
 			}
 		case TK_INSERT:{
-				sqlInsert(pParse,
-					      targetSrcList(pParse, pStep),
-					      sqlSelectDup(db,
-							       pStep->pSelect,
-							       0),
-					      sqlIdListDup(db,
-							       pStep->pIdList),
-					      pParse->eOrconf);
+				sqlInsert(pParse, targetSrcList(pParse, pStep),
+					  sqlSelectDup(db, pStep->pSelect, 0),
+					  sqlIdListDup(db, pStep->pIdList),
+					  pParse->eOrconf, true);
 				break;
 			}
 		case TK_DELETE:{
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..5ecfcf4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe)
 	return &vdbe->autoinc_id_list;
 }
 
-int
+static int
 vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 {
 	assert(vdbe != NULL);
@@ -1150,29 +1150,30 @@ case OP_String: {          /* out2 */
 }
 
 /* Opcode: NextAutoincValue P1 P2 * * *
- * Synopsis: r[P2] = next value from space sequence, which pageno is r[P1]
  *
- * Get next value from space sequence, which pageno is written into register
- * P1, write this value into register P2. If space doesn't exists (invalid
- * space_id or something else), raise an error. If space with
- * specified space_id doesn't have attached sequence, also raise an error.
+ * If the value in the register P2 is NULL, get the last value
+ * from the sequence that belongs to the space with id P1, and
+ * save this value in the VDBE. If the value in register in not
+ * NULL than this opcode is a no-op.
  */
 case OP_NextAutoincValue: {
 	assert(pOp->p1 > 0);
 	assert(pOp->p2 > 0);
 
+	pIn2 = &p->aMem[pOp->p2];
+	if ((pIn2->flags & MEM_Null) == 0)
+		break;
+
 	struct space *space = space_by_id(pOp->p1);
 	if (space == NULL)
 		goto abort_due_to_error;
 
 	int64_t value;
 	struct sequence *sequence = space->sequence;
-	if (sequence == NULL || sequence_next(sequence, &value) != 0)
+	if (sequence == NULL || sequence_get_value(sequence, &value) != 0)
 		goto abort_due_to_error;
 
-	pOut = out2Prerelease(p, pOp);
-	pOut->flags = MEM_Int;
-	pOut->u.i = value;
+	vdbe_add_new_autoinc_id(p, value);
 
 	break;
 }
diff --git a/test/box/misc.result b/test/box/misc.result
index dab8549..0f8bcb1 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -525,6 +525,7 @@ t;
   195: box.error.CREATE_CK_CONSTRAINT
   196: box.error.CK_CONSTRAINT_FAILED
   197: box.error.SQL_COLUMN_COUNT
+  198: box.error.SEQUENCE_NO_ELEMENTS
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 9639ba7..3580d6b 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -552,8 +552,6 @@ cn:execute('insert into test values (null, 1)')
 ---
 - autoincrement_ids:
   - 127
-  - 1
-  - 2
   row_count: 1
 ...
 box.execute('create table test3 (id int primary key autoincrement)')
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index 307b390..cc52727 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -551,3 +551,60 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: Additional generated identifiers when INSERT is
+-- executed by triggers.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t2;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index a056e79..3d3b393 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -191,3 +191,17 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1;
 box.session.su('admin')
 box.schema.user.drop('tester')
 box.execute("DROP TABLE t1;")
+
+--
+-- gh-4188: Additional generated identifiers when INSERT is
+-- executed by triggers.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
+box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
+box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t2 VALUES (100);')
+box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('DROP TABLE t1;')
+box.execute('DROP TABLE t2;')

[tarantool-patches] [PATCH v2 3/3] sql: remove VDBE from TXN

[imeevma]

VDBE was added to TXN because the generated identifiers were added
to VDBE in the sequence_next() function. Since they are now stored
in the VDBE itself, it is not necessary to have it in TXN.

Follow-up #4188
---
 src/box/sql/vdbe.c    |  4 ++--
 src/box/sql/vdbe.h    |  3 +--
 src/box/sql/vdbeInt.h |  2 +-
 src/box/sql/vdbeaux.c | 10 ++++------
 src/box/txn.h         | 17 -----------------
 5 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5ecfcf4..3534f48 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2927,7 +2927,7 @@ case OP_CheckViewReferences: {
  * Otherwise, raise an error with appropriate error message.
  */
 case OP_TransactionBegin: {
-	if (sql_txn_begin(p) != 0)
+	if (sql_txn_begin() != 0)
 		goto abort_due_to_error;
 	p->auto_commit = false	;
 	break;
@@ -2983,7 +2983,7 @@ case OP_TransactionRollback: {
  */
 case OP_TTransaction: {
 	if (!box_txn()) {
-		if (sql_txn_begin(p) != 0)
+		if (sql_txn_begin() != 0)
 			goto abort_due_to_error;
 	} else {
 		p->anonymous_savepoint = sql_savepoint(p, NULL);
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 4f94643..fde898d 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -175,11 +175,10 @@ Vdbe *sqlVdbeCreate(Parse *);
  * Allocate and initialize SQL-specific struct which completes
  * original Tarantool's txn struct using region allocator.
  *
- * @param v Vdbe with which associate this transaction.
  * @retval NULL on OOM, new psql_txn struct on success.
  **/
 struct sql_txn *
-sql_alloc_txn(struct Vdbe *v);
+sql_alloc_txn();
 
 /**
  * Prepare given VDBE to execution: initialize structs connected
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 6bfeecc..30cac07 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -440,7 +440,7 @@ u32 sqlVdbeSerialGet(const unsigned char *, u32, Mem *);
 int sqlVdbeExec(Vdbe *);
 int sqlVdbeList(Vdbe *);
 int
-sql_txn_begin(Vdbe *p);
+sql_txn_begin();
 Savepoint *
 sql_savepoint(Vdbe *p,
 	      const char *zName);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index baeeb46..0912eaf 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -77,7 +77,7 @@ sqlVdbeCreate(Parse * pParse)
 }
 
 struct sql_txn *
-sql_alloc_txn(struct Vdbe *v)
+sql_alloc_txn()
 {
 	struct sql_txn *txn = region_alloc_object(&fiber()->gc,
 						  struct sql_txn);
@@ -86,7 +86,6 @@ sql_alloc_txn(struct Vdbe *v)
 			 "struct sql_txn");
 		return NULL;
 	}
-	txn->vdbe = v;
 	txn->pSavepoint = NULL;
 	txn->fk_deferred_count = 0;
 	return txn;
@@ -106,11 +105,10 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
 		 * check FK violations, at least now.
 		 */
 		if (txn->psql_txn == NULL) {
-			txn->psql_txn = sql_alloc_txn(vdbe);
+			txn->psql_txn = sql_alloc_txn();
 			if (txn->psql_txn == NULL)
 				return -1;
 		}
-		txn->psql_txn->vdbe = vdbe;
 	}
 	return 0;
 }
@@ -1997,7 +1995,7 @@ sqlVdbeCheckFk(Vdbe * p, int deferred)
 }
 
 int
-sql_txn_begin(Vdbe *p)
+sql_txn_begin()
 {
 	if (in_txn()) {
 		diag_set(ClientError, ER_ACTIVE_TRANSACTION);
@@ -2006,7 +2004,7 @@ sql_txn_begin(Vdbe *p)
 	struct txn *ptxn = txn_begin(false);
 	if (ptxn == NULL)
 		return -1;
-	ptxn->psql_txn = sql_alloc_txn(p);
+	ptxn->psql_txn = sql_alloc_txn();
 	if (ptxn->psql_txn == NULL) {
 		box_txn_rollback();
 		return -1;
diff --git a/src/box/txn.h b/src/box/txn.h
index a19becc..e51d5a2 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -118,8 +118,6 @@ struct sql_txn {
 	 * VDBE to the next in the same transaction.
 	 */
 	uint32_t fk_deferred_count;
-	/** Current VDBE. */
-	struct Vdbe *vdbe;
 };
 
 /**
@@ -397,21 +395,6 @@ void
 txn_on_stop(struct trigger *trigger, void *event);
 
 /**
- * Return VDBE that is being currently executed.
- *
- * @retval VDBE that is being currently executed.
- * @retval NULL Either txn or ptxn_sql or vdbe is NULL;
- */
-static inline struct Vdbe *
-txn_vdbe()
-{
-	struct txn *txn = in_txn();
-	if (txn == NULL || txn->psql_txn == NULL)
-		return NULL;
-	return txn->psql_txn->vdbe;
-}
-
-/**
  * FFI bindings: do not throw exceptions, do not accept extra
  * arguments
  */
-- 
2.7.4

[tarantool-patches] Re: [PATCH v2 1/3] sql: remove unnecessary AUTOINCREMENT ID generation

[n.pettik]

> On 4 Jul 2019, at 14:42, imeevma@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.

> 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?

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

> box.execute("insert into t1 values (18, null);")
> box.execute("insert into t1(s2) values (null);")
> @@ -19,7 +21,14 @@ box.execute("insert into t2(s2) values (null);")
> box.execute("insert into t3 values (9, null)")
> box.execute("insert into t3(s2) values (null)")
> 
> +box.execute("insert into t4 values (9, null)")
> +box.execute("insert into t4 values (null, null)")
> +
> +box.execute("INSERT INTO t5 VALUES (18, NULL);")
> +box.execute("INSERT INTO t5 SELECT NULL, NULL FROM t5;")
> +

[tarantool-patches] Re: [PATCH v2 2/3] sql: do not show IDs generated by trigger

[n.pettik]

>>> 4) I added an extra argument to the sqlInsert() function, which
>>> makes it clear that the insertion was done in a trigger.
>>> 
>>> My point about these changes:
>>> 1 - I think it should be done anyway.
>>> 2 - It doesn't look like VDBE should be stored in TXN.
>> 
>> Please, support such statements with arguments.
>> 
> 1 - it worked only for cases 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);
> 
> But it doesn't worked for these cases:
> CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));
> INSERT INTO t1 VALUES (18, NULL);
> INSERT INTO t1 VALUES (NULL, NULL);
> 
> CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));
> INSERT INTO t1 VALUES (18, NULL);
> INSERT INTO t1 SELECT NULL, NULL FROM t1;
> 
> In addition, after the CHECK has been moved to the BOX, it works
> correctly for all cases. So now it is not necessary.
> 
> However, I can actually leave it at that. Then I need to create a
> new opcode if I want to solve #4188 using my solution.
> 
> 2 - sorry i can't say exactly why. But Gosha told me about it when
> I asked him verbally, and Kostja O. and Gosha mentioned it in the
> server chat. I think this can be found in messages for April or
> March.

Please investigate subj and add motivation to the commit message.

> New patch:
> 
> From d275de15fb14cf8ca9c8361602ed62d6d68e8aaf Mon Sep 17 00:00:00 2001
> Date: Tue, 28 May 2019 17:41:15 +0300
> Subject: [PATCH] sql: do not show IDs generated by trigger
> 
> Currently, if an INSERT was executed by a trigger during the
> execution of a statement, if there were any generated identifiers,
> they can be displayed as a result of the statement. This is
> incorrect, since we are not able to divide the IDs obtained into
> those that belong to the table mentioned in the statement and
> those that do not belong to this table.

So, how did you fix it?

I’ve noticed following behaviour:

tarantool> create table t1(id int primary key autoincrement, a int check (a > 0))
---
- row_count: 1
...

tarantool> insert or ignore into t1 values(null, -1)
---
- autoincrement_ids:
  - 1
  row_count: 1
…

tarantool> select * from t1
---
- metadata:
  - name: ID
    type: integer
  - name: A
    type: integer
  rows: []
…


As you can see, insertion failed, but due to the “ignore” clause,
execution wasn’t stopped and OP_NextAutoincValue was processed.

Please, consider this case in your patch-set.

> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index be8dab2..6d22b09 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -250,6 +250,7 @@ struct errcode_record {
> 	/*195 */_(ER_CREATE_CK_CONSTRAINT,	"Failed to create check constraint '%s': %s") \
> 	/*196 */_(ER_CK_CONSTRAINT_FAILED,	"Check constraint failed '%s': %s") \
> 	/*197 */_(ER_SQL_COLUMN_COUNT,		"Unequal number of entries in row expression: left side has %u, but right side - %u") \
> +	/*198 */_(ER_SEQUENCE_NO_ELEMENTS,	"Sequence '%s' has no elements") \

Could you please explain what does it mean?
I see that this error is not tested at all.
 
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index d2b4e17..c331cc5 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -233,7 +233,8 @@ sqlInsert(Parse * pParse,	/* Parser context */
> 	      SrcList * pTabList,	/* Name of table into which we are inserting */
> 	      Select * pSelect,	/* A SELECT statement to use as the data source */
> 	      IdList * pColumn,	/* Column names corresponding to IDLIST. */
> -	      enum on_conflict_action on_error)
> +	      enum on_conflict_action on_error,
> +	      bool is_triggered)
> {
> 	sql *db;		/* The main database structure */
> 	char *zTab;		/* Name of the table into which we are inserting */
> @@ -738,6 +739,13 @@ sqlInsert(Parse * pParse,	/* Parser context */
> 		vdbe_emit_insertion_completion(v, space, regIns + 1,
> 					       space->def->field_count,
> 					       on_error);
> +		/*
> +		 * Save the newly autogenerated value to VDBE.
> +		 */

Extended comment:

@@ -740,9 +739,14 @@ sqlInsert(Parse * pParse,  /* Parser context */
                                               space->def->field_count,
                                               on_error);
                /*
-                * Save the newly autogenerated value to VDBE.
+                * Save the newly generated autoincrement value to
+                * VDBE context. We don't need to do so for triggers
+                * in order to avoid mess of incremented ids. After
+                * VDBE execution is finished, list of autoincrement
+                * values is returned as a result of query execution.
                 */

> +		if (!is_triggered && autoinc_fieldno < UINT32_MAX) {

It is likely that you don’t need this flag: you can check triggered_space
field in parsing context:

@@ -742,7 +741,8 @@ sqlInsert(Parse * pParse,   /* Parser context */
                /*
                 * Save the newly autogenerated value to VDBE.
                 */
-               if (!is_triggered && autoinc_fieldno < UINT32_MAX) {
+               if (pParse->triggered_space == NULL &&
+                   autoinc_fieldno < UINT32_MAX) {
                        sqlVdbeAddOp2(v, OP_NextAutoincValue, space->def->id,
                                      regData + autoinc_fieldno);
                }

> +			sqlVdbeAddOp2(v, OP_NextAutoincValue, space->def->id,
> +				      regData + autoinc_fieldno);
> +		}
> 	}
> 
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 73dc6e4..5dcfd34 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3003,7 +3003,7 @@ sql_store_select(struct Parse *parse_context, struct Select *select);
> void
> sql_drop_table(struct Parse *);
> void sqlInsert(Parse *, SrcList *, Select *, IdList *,
> -	       enum on_conflict_action);
> +	       enum on_conflict_action, bool);
> void *sqlArrayAllocate(sql *, void *, int, int *, int *);
> 
> /**
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c8887f9..5ecfcf4 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -559,7 +559,7 @@ vdbe_autoinc_id_list(struct Vdbe *vdbe)
> 	return &vdbe->autoinc_id_list;
> }
> 
> -int
> +static int
> vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
> {
> 	assert(vdbe != NULL);
> @@ -1150,29 +1150,30 @@ case OP_String: {          /* out2 */
> }
> 
> /* Opcode: NextAutoincValue P1 P2 * * *
> - * Synopsis: r[P2] = next value from space sequence, which pageno is r[P1]
>  *
> - * Get next value from space sequence, which pageno is written into register
> - * P1, write this value into register P2. If space doesn't exists (invalid
> - * space_id or something else), raise an error. If space with
> - * specified space_id doesn't have attached sequence, also raise an error.
> + * If the value in the register P2 is NULL, get the last value
> + * from the sequence that belongs to the space with id P1, and
> + * save this value in the VDBE. If the value in register in not
> + * NULL than this opcode is a no-op.
>  */
> case OP_NextAutoincValue: {

Let’s give this opcode more suitable name: SaveAutoincValue.

Also, I’ve extended comment:

@@ -1152,11 +1152,12 @@ case OP_String: {          /* out2 */
 /* Opcode: NextAutoincValue P1 P2 * * *
  *
  * If the value in the register P2 is NULL, get the last value
- * from the sequence that belongs to the space with id P1, and
- * save this value in the VDBE. If the value in register in not
- * NULL than this opcode is a no-op.
+ * from the sequence that attached to the space with id P1, and
+ * save this value to the VDBE. At the end of execution, mentioned
+ * list is returned as a result of query execution. If the value in
+ * register in not NULL than this opcode is a no-op.
  */

> 	assert(pOp->p1 > 0);
> 	assert(pOp->p2 > 0);
> 
> +	pIn2 = &p->aMem[pOp->p2];
> +	if ((pIn2->flags & MEM_Null) == 0)
> +		break;
> +
> 	struct space *space = space_by_id(pOp->p1);
> 	if (space == NULL)
> 		goto abort_due_to_error;
> 
> 	int64_t value;
> 	struct sequence *sequence = space->sequence;
> -	if (sequence == NULL || sequence_next(sequence, &value) != 0)
> +	if (sequence == NULL || sequence_get_value(sequence, &value) != 0)

Is it possible that we added this opcode but sequence disappeared?

> 		goto abort_due_to_error;
> 
> -	pOut = out2Prerelease(p, pOp);
> -	pOut->flags = MEM_Int;
> -	pOut->u.i = value;
> +	vdbe_add_new_autoinc_id(p, value);
> 
> 	break;
> }
> ---
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index 9639ba7..3580d6b 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -552,8 +552,6 @@ cn:execute('insert into test values (null, 1)')
> ---
> - autoincrement_ids:
>   - 127
> -  - 1
> -  - 2
>   row_count: 1
> ...
> box.execute('create table test3 (id int primary key autoincrement)')
> 
> diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
> index a056e79..3d3b393 100644
> --- a/test/sql/triggers.test.lua
> +++ b/test/sql/triggers.test.lua
> @@ -191,3 +191,17 @@ box.execute([[CREATE TRIGGER r1 AFTER INSERT ON t1 FOR EACH ROW BEGIN SELECT 1;
> box.session.su('admin')
> box.schema.user.drop('tester')
> box.execute("DROP TABLE t1;")
> +
> +--
> +-- gh-4188: Additional generated identifiers when INSERT is
> +-- executed by triggers.
> +--
> +box.execute('CREATE TABLE t1 (i INT PRIMARY KEY AUTOINCREMENT);')
> +box.execute('CREATE TABLE t2 (i INT PRIMARY KEY AUTOINCREMENT);')
> +box.execute('CREATE TRIGGER r AFTER INSERT ON t1 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END’)

Please, add example with chained triggers.