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