[tarantool-patches] Re: [PATCH v4 2/4] sql: skip autoinc IDs generated inside SQL trigger

["n.pettik" <korablev@tarantool.org>]

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