[tarantool-patches] [PATCH v1 1/1] sql: do not show additional generated ids
imeevma at tarantool.org
imeevma at tarantool.org
Wed May 29 20:09:15 MSK 2019
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. Only identifiers generated during INSERT run by
the user will be saved.
Closes #4188
---
https://github.com/tarantool/tarantool/issues/4188
https://github.com/tarantool/tarantool/tree/imeevma/gh-4188-remove-additonal-generated-ids
src/box/errcode.h | 1 +
src/box/sequence.c | 22 +++++++++++----
src/box/sequence.h | 13 +++++++--
src/box/sql/insert.c | 16 +++++++----
src/box/sql/parse.y | 4 +--
src/box/sql/sqlInt.h | 2 +-
src/box/sql/trigger.c | 12 +++-----
src/box/sql/vdbe.c | 35 ++++++++++-------------
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 | 26 +++++++++++++++++
test/sql/gh-2981-check-autoinc.test.lua | 8 ++++++
test/sql/iproto.result | 2 --
test/sql/triggers.result | 49 +++++++++++++++++++++++++++++++++
test/sql/triggers.test.lua | 12 ++++++++
18 files changed, 161 insertions(+), 74 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index e2ec240..858256b 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -249,6 +249,7 @@ struct errcode_record {
/*194 */_(ER_MULTIKEY_INDEX_MISMATCH, "Field %s is used as multikey in one index and as single key in another") \
/*195 */_(ER_CREATE_CK_CONSTRAINT, "Failed to create check constraint '%s': %s") \
/*196 */_(ER_CK_CONSTRAINT_FAILED, "Check constraint failed '%s': %s") \
+ /*197 */_(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 a390ed5..b00f586 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -247,7 +247,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 */
@@ -644,10 +645,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;
@@ -746,7 +744,6 @@ sqlInsert(Parse * pParse, /* Parser context */
iRegStore);
}
}
-
/*
* Generate code to check constraints and process
* final insertion.
@@ -757,6 +754,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 e93dfe7..a708b16 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -839,7 +839,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.
{
@@ -847,7 +847,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 d602116..ef6067f 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3446,7 +3446,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 bf5c2e0..63069a9 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 33c5458..3877a97 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -574,7 +574,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);
@@ -1268,35 +1268,30 @@ case OP_String: { /* out2 */
break;
}
-/* Opcode: NextAutoincValue P1 P2 * * *
- * Synopsis: r[P2] = next value from space sequence, which pageno is r[P1]
+/* Opcode: NextAutoincValue P1 * * * *
+ * Synopsis: save last value from sequence of space with id 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.
+ * Get last value from space sequence, which space id is written
+ * into register P1, save this value into VDBE.
*/
case OP_NextAutoincValue: {
assert(pOp->p1 > 0);
assert(pOp->p2 > 0);
- struct space *space = space_by_id(pOp->p1);
- if (space == NULL) {
- rc = SQL_TARANTOOL_ERROR;
- goto abort_due_to_error;
- }
+ pIn2 = &p->aMem[pOp->p2];
+ if ((pIn2->flags & MEM_Null) == 0)
+ break;
+ struct space *space = space_by_id(pOp->p1);
+ assert(space != NULL);
+ assert(space->sequence != NULL);
int64_t value;
- struct sequence *sequence = space->sequence;
- if (sequence == NULL || sequence_next(sequence, &value) != 0) {
+ if (sequence_get_value(space->sequence, &value) != 0) {
rc = SQL_TARANTOOL_ERROR;
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;
}
@@ -3112,7 +3107,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) {
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
@@ -3175,7 +3170,7 @@ case OP_TransactionRollback: {
*/
case OP_TTransaction: {
if (!box_txn()) {
- if (sql_txn_begin(p) != 0) {
+ if (sql_txn_begin() != 0) {
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 09ea893..3c1a469 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -179,11 +179,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 cb51446..154ecd0 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -481,7 +481,7 @@ void sqlVdbeDeleteAuxData(sql *, AuxData **, int, int);
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 25d4cd7..788c541 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;
}
@@ -2264,7 +2262,7 @@ sqlVdbeCheckFk(Vdbe * p, int deferred)
}
int
-sql_txn_begin(Vdbe *p)
+sql_txn_begin()
{
if (in_txn()) {
diag_set(ClientError, ER_ACTIVE_TRANSACTION);
@@ -2273,7 +2271,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 f4d8618..8187062 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;
};
/**
@@ -379,21 +377,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
*/
diff --git a/test/box/misc.result b/test/box/misc.result
index 33e41b5..1dbc651 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -525,6 +525,7 @@ t;
194: box.error.MULTIKEY_INDEX_MISMATCH
195: box.error.CREATE_CK_CONSTRAINT
196: box.error.CK_CONSTRAINT_FAILED
+ 197: box.error.SEQUENCE_NO_ELEMENTS
...
test_run:cmd("setopt delimiter ''");
---
diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result
index e577898..6162d91 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 <> 19));");
+---
+- 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
@@ -59,6 +67,24 @@ box.execute("insert into t3(s2) values (null)")
- error: 'Failed to execute SQL statement: Check constraint failed ''CK_CONSTRAINT_1_T3'':
s1 < 10'
...
+box.execute("INSERT INTO t4 VALUES (18, NULL);")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO t4 VALUES (NULL, NULL);")
+---
+- error: 'Failed to execute SQL statement: Check constraint failed ''CK_CONSTRAINT_1_T4'':
+ s1 <> 19'
+...
+box.execute("INSERT INTO t5 VALUES (18, NULL);")
+---
+- row_count: 1
+...
+box.execute("INSERT INTO t5 SELECT NULL, NULL FROM t5;")
+---
+- error: 'Failed to execute SQL statement: Check constraint failed ''CK_CONSTRAINT_1_T5'':
+ s1 <> 19'
+...
box.execute("DROP TABLE t1")
---
- 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..3e38d53 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 <> 19));");
+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,6 +21,12 @@ 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 (18, 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")
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index e734872..8f1b8c0 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -547,8 +547,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 af9085d..25100a9 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -548,3 +548,52 @@ 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]
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index a056e79..2088717 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -191,3 +191,15 @@ 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;')
--
2.7.4
More information about the Tarantool-patches
mailing list