[tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger
n.pettik
korablev at tarantool.org
Wed Jul 17 19:58:43 MSK 2019
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index d2b4e17..6a2a742 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -728,6 +728,11 @@ sqlInsert(Parse * pParse, /* Parser context */
> }
> }
>
> + assert(regData > 0);
Pretty much useless assertion here.
> + int autoinc_reg = 0;
> + if (autoinc_fieldno < UINT32_MAX &&
> + pParse->triggered_space == NULL)
> + autoinc_reg = regData + autoinc_fieldno;
> /*
> * Generate code to check constraints and process
> * final insertion.
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 3b8c82d..654eebd 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3506,7 +3506,8 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
> 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,
> + int autoinc_reg);
You forgot to update comment to this function.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 6bb7bc3..951303c 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);
> @@ -4161,12 +4161,17 @@ case OP_SorterInsert: { /* in2 */
> break;
> }
>
> -/* Opcode: IdxInsert P1 P2 * P4 P5
> +/* Opcode: IdxInsert P1 P2 P3 P4 P5
> * Synopsis: key=r[P1]
> *
> * @param P1 Index of a register with MessagePack data to insert.
> * @param P2 If P4 is not set, then P2 is register containing pointer
> * to space to insert into.
> + * @param P3 If not 0, than it is an index of a register that
> + * contains value that will be inserted into field with
> + * AUTOINCREMENT. If the value is NULL, than the newly
> + * generated autoincrement value will be saved to VDBE
> + * context.
> * @param P4 Pointer to the struct space to insert to.
> * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
> * accounts the change in a case of successful insertion in
> @@ -4208,6 +4213,13 @@ case OP_IdxInsert: {
> rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
> pIn2->z + pIn2->n);
> }
> + if (rc == 0 && pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
> + assert(space->sequence != NULL);
> + int64_t value;
> + if (sequence_get_value(space->sequence, &value) != 0)
> + goto abort_due_to_error;
> + vdbe_add_new_autoinc_id(p, value);
> + }
I’d refactor this way (placed it at the top of last commit):
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 02f35c406..f3d70a982 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4171,7 +4171,8 @@ case OP_SorterInsert: { /* in2 */
* contains value that will be inserted into field with
* AUTOINCREMENT. If the value is NULL, than the newly
* generated autoincrement value will be saved to VDBE
- * context.
+ * context. At the end of execution, mentioned list of
+ * ids is returned as a result of query execution.
* @param P4 Pointer to the struct space to insert to.
* @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
* accounts the change in a case of successful insertion in
@@ -4211,31 +4212,36 @@ case OP_IdxInsert: {
rc = tarantoolsqlEphemeralInsert(space, pIn2->z,
pIn2->z + pIn2->n);
}
- if (rc == 0) {
- if (pOp->p5 & OPFLAG_NCHANGE)
- p->nChange++;
- if (pOp->p3 > 0 && ((aMem[pOp->p3].flags) & MEM_Null) != 0) {
- assert(space->sequence != NULL);
- int64_t value;
- if (sequence_get_value(space->sequence, &value) != 0)
- goto abort_due_to_error;
- vdbe_add_new_autoinc_id(p, value);
+ if (rc != 0) {
+ if ((pOp->p5 & OPFLAG_OE_IGNORE) != 0) {
+ /*
+ * Ignore any kind of fails and do not
+ * raise error message.
+ */
+ rc = 0;
+ /*
+ * If we are in trigger, increment ignore
+ * raised counter.
+ */
+ if (p->pFrame)
+ p->ignoreRaised++;
+ break;
}
+ if (pOp->p5 & OPFLAG_OE_FAIL)
+ p->errorAction = ON_CONFLICT_ACTION_FAIL;
+ else if (pOp->p5 & OPFLAG_OE_ROLLBACK)
+ p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+ goto abort_due_to_error;
}
-
- if (pOp->p5 & OPFLAG_OE_IGNORE) {
- /* Ignore any kind of failes and do not raise error message */
- rc = 0;
- /* If we are in trigger, increment ignore raised counter */
- if (p->pFrame)
- p->ignoreRaised++;
- } else if (pOp->p5 & OPFLAG_OE_FAIL) {
- p->errorAction = ON_CONFLICT_ACTION_FAIL;
- } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
- p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
+ if (pOp->p5 & OPFLAG_NCHANGE)
+ p->nChange++;
+ if (pOp->p3 > 0 && (aMem[pOp->p3].flags & MEM_Null) != 0) {
+ assert(space->sequence != NULL);
+ int64_t value;
+ if (sequence_get_value(space->sequence, &value) != 0)
+ goto abort_due_to_error;
+ vdbe_add_new_autoinc_id(p, value);
}
- if (rc != 0)
- goto abort_due_to_error;
break;
}
>
> if (pOp->p5 & OPFLAG_OE_IGNORE) {
> /* Ignore any kind of failes and do not raise error message */
>
> 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.
Please, leave comments in form “Make sure that …
doesn’t appear / is fixed” or sort of. It allows to avoid
confusion while reading such comments.
More information about the Tarantool-patches
mailing list