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

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

Changes in v3:
1) Removed tests in refactoring patch "sql: remove unnecessary
AUTOINCREMENT ID generation". They shows nothing.
2) Changed the way to check that INSERT executed by trigger.
3) Fixed a bug with the display of generated AUTOINCREMENT
identifiers, even if INSERT failed in the case of INSERT OR
IGNORE.

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

 src/box/sequence.c          |  19 +++++---
 src/box/sequence.h          |  13 +++--
 src/box/sql/insert.c        |  27 ++++++++---
 src/box/sql/sqlInt.h        |  10 +++-
 src/box/sql/update.c        |   2 +-
 src/box/sql/vdbe.c          |  46 +++++++++++-------
 src/box/sql/vdbe.h          |   3 +-
 src/box/sql/vdbeInt.h       |   2 +-
 src/box/sql/vdbeaux.c       |  10 ++--
 src/box/txn.h               |  17 -------
 test/sql/iproto.result      |   2 -
 test/sql/row-count.result   |  26 ++++++++++
 test/sql/row-count.test.lua |   7 +++
 test/sql/triggers.result    | 114 ++++++++++++++++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua  |  24 ++++++++++
 15 files changed, 259 insertions(+), 63 deletions(-)

-- 
2.7.4

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

[imeevma]

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@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@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;

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

[imeevma]

Thank you for review. My answers, new patch and diff below.

On 7/8/19 9:54 PM, n.pettik wrote:
>
>>>> 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.
>
Is it necessary? It is not used now, and I deleted it. I described
this in a commit-message. Does it really need any other reason?

>> 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?
>
Changed commit-message.

> 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.
>
Fixed in a new patch.

>> 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:
>
Thanks, fixed.

> @@ -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);
>                 }
>
Thanks, fixed.

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

> 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.
>   */
>
Thanks, fixed.

>> 	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?
>
No, I think. Changed to assert.

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


Diff between the patches are slightly different from the real
changes. The difference is in the changes to the comment to the
OP_SaveAutoincValue.

Diff:

diff --git a/src/box/errcode.h b/src/box/errcode.h
index 6d22b09..be8dab2 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -250,7 +250,6 @@ 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 c666972..5996519 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -369,10 +369,7 @@ 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;
-	}
+	assert(pos != light_sequence_end);
 	struct sequence_data data = light_sequence_get(&sequence_data_index,
 						       pos);
 	*out_value = data.value;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c331cc5..db95a02 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -233,8 +233,7 @@ 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,
-	      bool is_triggered)
+	      enum on_conflict_action on_error)
 {
 	sql *db;		/* The main database structure */
 	char *zTab;		/* Name of the table into which we are inserting */
@@ -740,10 +739,16 @@ 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) {
-			sqlVdbeAddOp2(v, OP_NextAutoincValue, space->def->id,
+		if (autoinc_fieldno < UINT32_MAX &&
+		    pParse->triggered_space == NULL) {
+			sqlVdbeAddOp2(v, OP_SaveAutoincValue, space->def->id,
 				      regData + autoinc_fieldno);
 		}
 	}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 50ddd4b..010feff 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, false);
+  sqlInsert(pParse, X, S, F, R);
 }
 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, false);
+  sqlInsert(pParse, X, 0, F, R);
 }
 
 %type insert_cmd {int}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 5dcfd34..73dc6e4 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, bool);
+	       enum on_conflict_action);
 void *sqlArrayAllocate(sql *, void *, int, int *, int *);
 
 /**
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 32a7052..d746ef8 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -631,10 +631,14 @@ 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, true);
+				sqlInsert(pParse,
+					      targetSrcList(pParse, pStep),
+					      sqlSelectDup(db,
+							       pStep->pSelect,
+							       0),
+					      sqlIdListDup(db,
+							       pStep->pIdList),
+					      pParse->eOrconf);
 				break;
 			}
 		case TK_DELETE:{
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5ecfcf4..55a8a08 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1156,7 +1156,7 @@ case OP_String: {          /* out2 */
  * save this value in the VDBE. If the value in register in not
  * NULL than this opcode is a no-op.
  */
-case OP_NextAutoincValue: {
+case OP_SaveAutoincValue: {
 	assert(pOp->p1 > 0);
 	assert(pOp->p2 > 0);
 
@@ -1169,8 +1169,8 @@ case OP_NextAutoincValue: {
 		goto abort_due_to_error;
 
 	int64_t value;
-	struct sequence *sequence = space->sequence;
-	if (sequence == NULL || sequence_get_value(sequence, &value) != 0)
+	assert(space->sequence != NULL);
+	if (sequence_get_value(space->sequence, &value) != 0)
 		goto abort_due_to_error;
 
 	vdbe_add_new_autoinc_id(p, value);
diff --git a/test/box/misc.result b/test/box/misc.result
index 0f8bcb1..dab8549 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -525,7 +525,6 @@ 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/triggers.result b/test/sql/triggers.result
index cc52727..8ef9648 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -563,15 +563,19 @@ 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')
+box.execute('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
 ---
 - row_count: 1
 ...
-box.execute('INSERT INTO t2 VALUES (100);')
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
 ---
 - row_count: 1
 ...
-box.execute('INSERT INTO t1 VALUES (NULL), (NULL), (NULL);')
+box.execute('INSERT INTO t1 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
 ---
 - autoincrement_ids:
   - 1
@@ -585,11 +589,34 @@ box.execute('SELECT * FROM t1;')
   - name: I
     type: integer
   rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
   - [1]
   - [2]
   - [3]
 ...
-box.execute('SELECT * FROM t2;')
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
 ---
 - metadata:
   - name: I
@@ -599,6 +626,32 @@ box.execute('SELECT * FROM t2;')
   - [101]
   - [102]
   - [103]
+  - [104]
+  - [105]
+  - [106]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+  - [4]
+  - [5]
+  - [6]
+...
+box.execute('SELECT * FROM t3;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
 ...
 box.execute('DROP TABLE t1;')
 ---
@@ -608,3 +661,7 @@ box.execute('DROP TABLE t2;')
 ---
 - row_count: 1
 ...
+box.execute('DROP TABLE t3;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index 3d3b393..ee77192 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -198,10 +198,20 @@ box.execute("DROP TABLE t1;")
 --
 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('CREATE TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+box.execute('INSERT INTO t1 VALUES (100);')
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
 box.execute('SELECT * FROM t1;')
 box.execute('SELECT * FROM t2;')
+
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('SELECT * FROM t3;')
+
 box.execute('DROP TABLE t1;')
 box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t3;')


New patch:

From 4c36c8fa80439c4900c9916dd89bac2bd8cf77f8 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. This has now been fixed
by adding a new opcode and using it to retrieve and save the newly
generated identifiers.

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/sequence.c b/src/box/sequence.c
index 2bf38f9..5996519 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -216,9 +216,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);
@@ -253,9 +250,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) {
@@ -368,3 +362,16 @@ 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);
+	assert(pos != light_sequence_end);
+	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 9a745ad..b56236e 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -157,9 +157,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.
  *
@@ -170,6 +167,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" */
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d2b4e17..db95a02 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -738,6 +738,19 @@ sqlInsert(Parse * pParse,	/* Parser context */
 		vdbe_emit_insertion_completion(v, space, regIns + 1,
 					       space->def->field_count,
 					       on_error);
+		/*
+		 * 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 (autoinc_fieldno < UINT32_MAX &&
+		    pParse->triggered_space == NULL) {
+			sqlVdbeAddOp2(v, OP_SaveAutoincValue, space->def->id,
+				      regData + autoinc_fieldno);
+		}
 	}
 
 	/* Update the count of rows that are inserted
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..d6b32f5 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);
@@ -1149,30 +1149,32 @@ case OP_String: {          /* out2 */
 	break;
 }
 
-/* Opcode: NextAutoincValue P1 P2 * * *
- * Synopsis: r[P2] = next value from space sequence, which pageno is r[P1]
+/* Opcode: SaveAutoincValue P1 P2 * * *
  *
- * 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 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 is not NULL than this opcode is a no-op.
  */
-case OP_NextAutoincValue: {
+case OP_SaveAutoincValue: {
 	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)
+	assert(space->sequence != NULL);
+	if (sequence_get_value(space->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/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..8ef9648 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -551,3 +551,117 @@ 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 TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+---
+- row_count: 1
+...
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t1 VALUES (100);')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+---
+- autoincrement_ids:
+  - 1
+  - 2
+  - 3
+  row_count: 3
+...
+box.execute('SELECT * FROM t1;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [100]
+  - [101]
+  - [102]
+  - [103]
+  - [104]
+  - [105]
+  - [106]
+...
+box.execute('SELECT * FROM t2;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+  - [4]
+  - [5]
+  - [6]
+...
+box.execute('SELECT * FROM t3;')
+---
+- metadata:
+  - name: I
+    type: integer
+  rows:
+  - [1]
+  - [2]
+  - [3]
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t2;')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t3;')
+---
+- row_count: 1
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index a056e79..ee77192 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -191,3 +191,27 @@ 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 TABLE t3 (i INT PRIMARY KEY AUTOINCREMENT);')
+
+box.execute('CREATE TRIGGER r1 AFTER INSERT ON t2 FOR EACH ROW BEGIN INSERT INTO t1 VALUES (null); END')
+box.execute('INSERT INTO t1 VALUES (100);')
+box.execute('INSERT INTO t2 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+
+box.execute('CREATE TRIGGER r2 AFTER INSERT ON t3 FOR EACH ROW BEGIN INSERT INTO t2 VALUES (null); END')
+box.execute('INSERT INTO t3 VALUES (NULL), (NULL), (NULL);')
+box.execute('SELECT * FROM t1;')
+box.execute('SELECT * FROM t2;')
+box.execute('SELECT * FROM t3;')
+
+box.execute('DROP TABLE t1;')
+box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t3;')

[tarantool-patches] [PATCH v3 3/4] 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 d6b32f5..47ef0ab 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2928,7 +2928,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;
@@ -2984,7 +2984,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 d1ef220..0aa32b6 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;
 };
 
 /**
@@ -406,21 +404,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] [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed

[imeevma]

When the INSERT was executed as an INSERT OR IGNORE, it was
possible to get the generated autoincrement identifiers, even if
the insert failed.
For example:
CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT,
                a INT check (a > 0));
INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);

should return only two identifiers.

After this patch, only in case of successful insertion, the
generated identifiers will be returned. Also, row-count shows now
number of successfull insertions in case of INSERT OR IGNORE.

Follow-up #4188
---
 src/box/sql/insert.c        |  9 +++++++--
 src/box/sql/sqlInt.h        | 10 +++++++++-
 src/box/sql/update.c        |  2 +-
 src/box/sql/vdbe.c          | 14 +++++++++++---
 test/sql/row-count.result   | 26 ++++++++++++++++++++++++++
 test/sql/row-count.test.lua |  7 +++++++
 6 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index db95a02..d162242 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -737,7 +737,9 @@ sqlInsert(Parse * pParse,	/* Parser context */
 		fk_constraint_emit_check(pParse, space, 0, regIns, 0);
 		vdbe_emit_insertion_completion(v, space, regIns + 1,
 					       space->def->field_count,
-					       on_error);
+					       on_error,
+					       autoinc_fieldno < UINT32_MAX &&
+					       pParse->triggered_space == NULL);
 		/*
 		 * Save the newly generated autoincrement value to
 		 * VDBE context. We don't need to do so for
@@ -984,10 +986,13 @@ process_index:  ;
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict)
+			       enum on_conflict_action on_conflict,
+			       bool is_autoinc_present)
 {
 	assert(v != NULL);
 	u16 pik_flags = OPFLAG_NCHANGE;
+	if (is_autoinc_present)
+		pik_flags |= OPFLAG_AUTOINC;
 	SET_CONFLICT_FLAG(pik_flags, on_conflict);
 	sqlVdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len,
 			  raw_data_reg + tuple_len);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 73dc6e4..f668236 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2365,6 +2365,11 @@ struct Parse {
 #define OPFLAG_NCHANGE       0x01	/* OP_Insert: Set to update db->nChange */
 				     /* Also used in P2 (not P5) of OP_Delete */
 #define OPFLAG_EPHEM         0x01	/* OP_Column: Ephemeral output is ok */
+/*
+ * OP_IdxInsert, OP_IdxReplace: Inserted tuple can contain NULL in
+ * PK field, that will be replaced by integer using sequence.
+ */
+#define OPFLAG_AUTOINC      0x100
 #define OPFLAG_OE_IGNORE    0x200	/* OP_IdxInsert: Ignore flag */
 #define OPFLAG_OE_FAIL      0x400	/* OP_IdxInsert: Fail flag */
 #define OPFLAG_OE_ROLLBACK  0x800	/* OP_IdxInsert: Rollback flag. */
@@ -3502,11 +3507,14 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
  * @param raw_data_reg Register with raw data to insert.
  * @param tuple_len Number of registers to hold the tuple.
  * @param on_conflict On conflict action.
+ * @param is_autoinc_present True if space has a field with
+ *        autoincrement.
  */
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict);
+			       enum on_conflict_action on_conflict,
+			       bool is_autoinc_present);
 
 void
 sql_set_multi_write(Parse *, bool);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d77bee0..d249a55 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -450,7 +450,7 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 		if (on_error == ON_CONFLICT_ACTION_REPLACE) {
 			 vdbe_emit_insertion_completion(v, space, regNew,
 							space->def->field_count,
-							on_error);
+							on_error, false);
 
 		} else {
 			int key_reg;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 47ef0ab..884c660 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4204,8 +4204,6 @@ case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
 	assert((pIn2->flags & MEM_Blob) != 0);
-	if (pOp->p5 & OPFLAG_NCHANGE)
-		p->nChange++;
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -4228,8 +4226,18 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
+	if (rc == 0 && (pOp->p5 & OPFLAG_NCHANGE) != 0)
+		p->nChange++;
 
-	if (pOp->p5 & OPFLAG_OE_IGNORE) {
+	if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) {
+		/*
+		 * If we ignore errors, we must skip the
+		 * OP_SaveAutoincValue opcode, if present. This
+		 * should be the following opcode if the space has
+		 * a field with AUTOINCREMENT.
+		 */
+		if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0)
+			++pOp;
 		/* Ignore any kind of failes and do not raise error message */
 		rc = 0;
 		/* If we are in trigger, increment ignore raised counter */
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index e7841ca..522d2f1 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -335,3 +335,29 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: check that generated IDs is not showed for failed
+-- insertions in case of INSERT OR IGNORE.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+---
+- row_count: 1
+...
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+---
+- autoincrement_ids:
+  - 1
+  - 3
+  row_count: 2
+...
+box.execute("SELECT * FROM t;")
+---
+- metadata:
+  - name: I
+    type: integer
+  - name: A
+    type: integer
+  rows:
+  - [1, 1]
+  - [3, 2]
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 9f5215c..30de8b4 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -73,3 +73,10 @@ box.execute("DROP TABLE t2;")
 box.execute("DROP TABLE t3;")
 box.execute("DROP TABLE t1;")
 
+--
+-- gh-4188: check that generated IDs is not showed for failed
+-- insertions in case of INSERT OR IGNORE.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+box.execute("SELECT * FROM t;")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed

[Mergen Imeev]

Sorry, forgot to drop table in test. Fixed here and on branch.
Diff and new patch below.

Diff:

diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index 522d2f1..69bfb78 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -361,3 +361,7 @@ box.execute("SELECT * FROM t;")
   - [1, 1]
   - [3, 2]
 ...
+box.execute("DROP TABLE t;")
+---
+- row_count: 1
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 30de8b4..965408e 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -80,3 +80,4 @@ box.execute("DROP TABLE t1;")
 box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
 box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
 box.execute("SELECT * FROM t;")
+box.execute("DROP TABLE t;")

New patch:

From 988b155477f12ed27cf20edf14a6a88d6f995a2b Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 10 Jul 2019 12:56:16 +0300
Subject: [PATCH] sql: do not show generated IDs if INSERT failed

When the INSERT was executed as an INSERT OR IGNORE, it was
possible to get the generated autoincrement identifiers, even if
the insert failed.
For example:
CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT,
                a INT check (a > 0));
INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);

should return only two identifiers.

After this patch, only in case of successful insertion, the
generated identifiers will be returned. Also, row-count shows now
number of successfull insertions in case of INSERT OR IGNORE.

Follow-up #4188

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index db95a02..d162242 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -737,7 +737,9 @@ sqlInsert(Parse * pParse,	/* Parser context */
 		fk_constraint_emit_check(pParse, space, 0, regIns, 0);
 		vdbe_emit_insertion_completion(v, space, regIns + 1,
 					       space->def->field_count,
-					       on_error);
+					       on_error,
+					       autoinc_fieldno < UINT32_MAX &&
+					       pParse->triggered_space == NULL);
 		/*
 		 * Save the newly generated autoincrement value to
 		 * VDBE context. We don't need to do so for
@@ -984,10 +986,13 @@ process_index:  ;
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict)
+			       enum on_conflict_action on_conflict,
+			       bool is_autoinc_present)
 {
 	assert(v != NULL);
 	u16 pik_flags = OPFLAG_NCHANGE;
+	if (is_autoinc_present)
+		pik_flags |= OPFLAG_AUTOINC;
 	SET_CONFLICT_FLAG(pik_flags, on_conflict);
 	sqlVdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len,
 			  raw_data_reg + tuple_len);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 73dc6e4..f668236 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2365,6 +2365,11 @@ struct Parse {
 #define OPFLAG_NCHANGE       0x01	/* OP_Insert: Set to update db->nChange */
 				     /* Also used in P2 (not P5) of OP_Delete */
 #define OPFLAG_EPHEM         0x01	/* OP_Column: Ephemeral output is ok */
+/*
+ * OP_IdxInsert, OP_IdxReplace: Inserted tuple can contain NULL in
+ * PK field, that will be replaced by integer using sequence.
+ */
+#define OPFLAG_AUTOINC      0x100
 #define OPFLAG_OE_IGNORE    0x200	/* OP_IdxInsert: Ignore flag */
 #define OPFLAG_OE_FAIL      0x400	/* OP_IdxInsert: Fail flag */
 #define OPFLAG_OE_ROLLBACK  0x800	/* OP_IdxInsert: Rollback flag. */
@@ -3502,11 +3507,14 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
  * @param raw_data_reg Register with raw data to insert.
  * @param tuple_len Number of registers to hold the tuple.
  * @param on_conflict On conflict action.
+ * @param is_autoinc_present True if space has a field with
+ *        autoincrement.
  */
 void
 vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 			       int raw_data_reg, uint32_t tuple_len,
-			       enum on_conflict_action on_conflict);
+			       enum on_conflict_action on_conflict,
+			       bool is_autoinc_present);
 
 void
 sql_set_multi_write(Parse *, bool);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d77bee0..d249a55 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -450,7 +450,7 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 		if (on_error == ON_CONFLICT_ACTION_REPLACE) {
 			 vdbe_emit_insertion_completion(v, space, regNew,
 							space->def->field_count,
-							on_error);
+							on_error, false);
 
 		} else {
 			int key_reg;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 47ef0ab..884c660 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4204,8 +4204,6 @@ case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
 	assert((pIn2->flags & MEM_Blob) != 0);
-	if (pOp->p5 & OPFLAG_NCHANGE)
-		p->nChange++;
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -4228,8 +4226,18 @@ case OP_IdxInsert: {
 		rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
 						     pIn2->z + pIn2->n);
 	}
+	if (rc == 0 && (pOp->p5 & OPFLAG_NCHANGE) != 0)
+		p->nChange++;
 
-	if (pOp->p5 & OPFLAG_OE_IGNORE) {
+	if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) {
+		/*
+		 * If we ignore errors, we must skip the
+		 * OP_SaveAutoincValue opcode, if present. This
+		 * should be the following opcode if the space has
+		 * a field with AUTOINCREMENT.
+		 */
+		if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0)
+			++pOp;
 		/* Ignore any kind of failes and do not raise error message */
 		rc = 0;
 		/* If we are in trigger, increment ignore raised counter */
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
index e7841ca..69bfb78 100644
--- a/test/sql/row-count.result
+++ b/test/sql/row-count.result
@@ -335,3 +335,33 @@ box.execute("DROP TABLE t1;")
 ---
 - row_count: 1
 ...
+--
+-- gh-4188: check that generated IDs is not showed for failed
+-- insertions in case of INSERT OR IGNORE.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+---
+- row_count: 1
+...
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+---
+- autoincrement_ids:
+  - 1
+  - 3
+  row_count: 2
+...
+box.execute("SELECT * FROM t;")
+---
+- metadata:
+  - name: I
+    type: integer
+  - name: A
+    type: integer
+  rows:
+  - [1, 1]
+  - [3, 2]
+...
+box.execute("DROP TABLE t;")
+---
+- row_count: 1
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
index 9f5215c..965408e 100644
--- a/test/sql/row-count.test.lua
+++ b/test/sql/row-count.test.lua
@@ -73,3 +73,11 @@ box.execute("DROP TABLE t2;")
 box.execute("DROP TABLE t3;")
 box.execute("DROP TABLE t1;")
 
+--
+-- gh-4188: check that generated IDs is not showed for failed
+-- insertions in case of INSERT OR IGNORE.
+--
+box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
+box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
+box.execute("SELECT * FROM t;")
+box.execute("DROP TABLE t;")

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

[n.pettik]

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

You changes affect more than 50% of opcode's content.
Please, to make patches well structured, remove opcode
in current patch and resurrect in the next one.

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

[n.pettik]

I’ve edited commit message:

    sql: skip autoinc IDs generated inside SQL trigger
    
    Currently, if an INSERT is executed inside SQL trigger and it results
    in generated autoincrement identifiers, ones will be displayed as a
    result of the statement. This is wrong, since we are not able to divide
    IDs obtained into those that belong to the table mentioned in the
    statement and those that do not belong to this table. This has been
    fixed by adding a new opcode OP_SaveAutoincValue, which follows each
    OP_IdxInsert when there's autoincrement field. On the other hand, we can
    avoid adding this opcode while producing VDBE instructions for triggers.
    OP_SaveAutoincValue retrieves and saves recently generated identifiers
    into the list, which is held in VDBE itself. Note that from now we don't
    save autoincremented value to VDBE right in sequence_next() - this
    operation is moved to OP_SaveAutoincValue. So that, VDBE can be removed
    from struct txn.
    
    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
    
    Closes #4188

[tarantool-patches] Re: [PATCH v3 4/4] sql: do not show generated IDs if INSERT failed

[n.pettik]

I’ve edited commit message:

    sql: omit auto-inc ID if INSERT OR IGNORE failed
    
    If INSERT statement is executed with IGNORE error action
    (i.e. INSERT OR IGNORE ...), it will return generated autoincrement
    identifiers. Even in case the insert has failed. For example:
    
    CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT,
                    a INT check (a > 0));
    INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);
    
    In this case only two identifiers should be returned, but before this
    patch all three are appeared. So, let's account only identifiers which
    are really inserted to the space.  Also, row-count shows now
    number of successful insertions in case of INSERT OR IGNORE.
    
    Follow-up #4188


TBO I dislike your approach for several reasons.
Firstly, it introduces another one auxiliary flag, which
enlarges branching, complicates code reading etc.
Secondly, it changes program counter in OP_Insert:
generally speaking, I’d not consider implicit incrementation
of program counter to be a good practice. You even don’t
check that the next instruction is OP_SaveAutoinc…

Could you spend some more time trying come up with
another solution?

Also, I wonder why didn’t you split OP_SaveAutoinc into
two opcode: OP_IsNull + OP_SaveAutoinc which would
unconditionally save autroincremented value to the vdbe?

Obvious diff:

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d1622428c..e9a9c3afc 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -735,11 +735,11 @@ sqlInsert(Parse * pParse, /* Parser context */
                vdbe_emit_constraint_checks(pParse, space, regIns + 1,
                                            on_error, endOfLoop, 0);
                fk_constraint_emit_check(pParse, space, 0, regIns, 0);
+               bool has_autoinc_field = autoinc_fieldno < UINT32_MAX &&
+                                        pParse->triggered_space == NULL;
                vdbe_emit_insertion_completion(v, space, regIns + 1,
                                               space->def->field_count,
-                                              on_error,
-                                              autoinc_fieldno < UINT32_MAX &&
-                                              pParse->triggered_space == NULL);
+                                              on_error, has_autoinc_field);
                /*
                 * Save the newly generated autoincrement value to
                 * VDBE context. We don't need to do so for
@@ -748,8 +748,7 @@ sqlInsert(Parse * pParse,   /* Parser context */
                 * autoincrement values is returned as a result of
                 * query execution.
                 */
-               if (autoinc_fieldno < UINT32_MAX &&
-                   pParse->triggered_space == NULL) {
+               if (has_autoinc_field) {
                        sqlVdbeAddOp2(v, OP_SaveAutoincValue, space->def->id,
                                      regData + autoinc_fieldno);
                }
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 884c6605c..366f1f989 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4236,8 +4236,10 @@ case OP_IdxInsert: {
                 * should be the following opcode if the space has
                 * a field with AUTOINCREMENT.
                 */
-               if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0)
+               if (rc != 0 && (pOp->p5 & OPFLAG_AUTOINC) != 0) {
                        ++pOp;
+                       assert(pOp->opcode == OP_SaveAutoincValue);
+               }
                /* Ignore any kind of failes and do not raise error message */
                rc = 0;