[tarantool-patches] Re: [PATCH v1 3/3] sql: do not use OP_Delete+OP_Insert for UPDATES

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

>> Please, don’t change code without any explanations.
>> You should state it within commit message or in source code.
>       /**
> 	 * Make a lookup in a parent table with OP_Found.
> 	 * We mustn't make it for a self-referenced table since
> 	 * it's tuple will be modified by the update operation.
> 	 * And since the foreign key has already detected a
> 	 * conflict, fk counter must be increased.
> 	 */
> 	if (!(fkey_is_self_referenced(fk_def) && is_update)) {

So, AFAIU this part can be moved into separate commit
which fixes fk+update behaviour. I mean, would it work
with old update which consists of insert+delete?

>> I’d better introduce separate function (or simply inline) for emitting
>> update operation. Lets keep vdbe_emit_insertion_completion as is.
> I've reworked this code as separete vdbe_emit_update_completion. I didn't it before because of
> same prolog an epilog in thouse routines. 

Ok, then you can do it like this:

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 1b02ea907..f147f6a50 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1059,12 +1059,7 @@ vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 {
        assert(v != NULL);
        u16 pik_flags = OPFLAG_NCHANGE;
-       if (on_conflict == ON_CONFLICT_ACTION_IGNORE)
-               pik_flags |= OPFLAG_OE_IGNORE;
-       else if (on_conflict == ON_CONFLICT_ACTION_FAIL)
-               pik_flags |= OPFLAG_OE_FAIL;
-       else if (on_conflict == ON_CONFLICT_ACTION_ROLLBACK)
-               pik_flags |= OPFLAG_OE_ROLLBACK;
+       SET_CONFLICT_FLAG(pik_flags, on_conflict);
        sqlite3VdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len,
                          raw_data_reg + tuple_len);
        sqlite3VdbeAddOp1(v, OP_IdxInsert, raw_data_reg + tuple_len);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 4110a5991..281f9da32 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2847,6 +2847,16 @@ struct Parse {
 #define OPFLAG_SYSTEMSP      0x20      /* OP_Open**: set if space pointer
                                         * points to system space.
                                         */
+
+#define SET_CONFLICT_FLAG(opflag, on_conflict) do { \
+       if (on_conflict == ON_CONFLICT_ACTION_IGNORE) \
+               opflag |= OPFLAG_OE_IGNORE; \
+       else if (on_conflict == ON_CONFLICT_ACTION_FAIL) \
+               opflag |= OPFLAG_OE_FAIL; \
+       else if (on_conflict == ON_CONFLICT_ACTION_ROLLBACK) \
+               opflag |= OPFLAG_OE_ROLLBACK; \
+} while (0)
+
 /* OP_RowData: xferOptimization started processing */
 #ifdef SQLITE_TEST
 #define OPFLAG_XFER_OPT      0x01
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index b45bbf5d3..dbef22fd8 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -66,25 +66,6 @@ sqlite3ColumnDefault(Vdbe *v, struct space_def *def, int i, int ireg)
        }
 }
 
-void
-vdbe_emit_update_completion(struct Vdbe *v, struct space *space,
-                           int raw_data_reg, int raw_key_reg, int upd_cols_reg,
-                           enum on_conflict_action on_conflict)
-{
-       assert(v != NULL);
-       u16 pik_flags = OPFLAG_NCHANGE;
-       assert(on_conflict != ON_CONFLICT_ACTION_REPLACE);
-       if (on_conflict == ON_CONFLICT_ACTION_IGNORE)
-               pik_flags |= OPFLAG_OE_IGNORE;
-       else if (on_conflict == ON_CONFLICT_ACTION_FAIL)
-               pik_flags |= OPFLAG_OE_FAIL;
-       else if (on_conflict == ON_CONFLICT_ACTION_ROLLBACK)
-               pik_flags |= OPFLAG_OE_ROLLBACK;
-       sqlite3VdbeAddOp4(v, OP_Update, raw_data_reg, raw_key_reg,
-                         upd_cols_reg, (char *)space, P4_SPACEPTR);
-       sqlite3VdbeChangeP5(v, pik_flags);
-}
-
 /*
  * Process an UPDATE statement.
  *
@@ -507,8 +488,12 @@ sqlite3Update(Parse * pParse,              /* The parser context */
                        int upd_cols_reg = sqlite3GetTempReg(pParse);
                        sqlite3VdbeAddOp4(v, OP_Blob, upd_cols_sz, upd_cols_reg,
                                        0, (const char *)upd_cols, P4_DYNAMIC);
-                       vdbe_emit_update_completion(v, space, regNew, key_reg,
-                                                   upd_cols_reg, on_error);
+                       u16 pik_flags = OPFLAG_NCHANGE;
+                       SET_CONFLICT_FLAG(pik_flags, on_error);
+                       sqlite3VdbeAddOp4(v, OP_Update, regNew, key_reg,
+                                         upd_cols_reg, (char *)space,
+                                         P4_SPACEPTR);
+                       sqlite3VdbeChangeP5(v, pik_flags);

So I would even prefer simple inlining to static function.

>> Can we use this routine to encode MsgPack during execution of OP_MakeRecord?
>> Now it uses sqlite3VdbeMsgpackRecordPut() which is almost the same.
> Yep, but it requires previous sqlite3VdbeMsgpackRecordLen() but region allocation
> doesn’t.

So? In some cases it is enough to allocate memory on region (see bIsEphemeral
flag in OP_MakeRecord). In the rest situations you can copy it to malloc after all.
You may argue that in this case we will gain 2x memory consumption. However,
sqlite3VdbeMsgpackRecordLen() uses very rough heuristic to estimate size for
msgpack and in some cases it may result in 5-6 times larger memory chunks:
https://github.com/tarantool/tarantool/issues/3035

All points considered, I suggest to replace all calls of sqlite3VdbeMsgpackRecordPut()
with mpstream as a preparation patch (and close #3035). I guess, it shouldn’t
be too complicated to implement.

> For example, as a part of #3545 task we reworked same code with region.
> I believe that this is much better and moreover
> sqlite3VdbeMsgpackRecordLen+sqlite3VdbeMsgpackRecordPut calls should be
> reworked with mpstream_encode_vdbe_mem where possible (maybe it worth to
> create such task).

There are only two places where these functions are called:
OP_MakeRecord and encoding msgpack key to pass it to
the index iterator.


> ======================================================
> 
> Introduced a new OP_Update opcode making Tarantool native Update
> operation.

Nit: not making but processing/handling/executing etc.

> In case of UPDATE or REPLACE we can't use new OP_Update as it
> has a complex SQL-specific semantics:
> 
> CREATE TABLE tj (s1 INT PRIMARY KEY, s2 INT);
> INSERT INTO tj VALUES (1, 3),(2, 4),(3,5);
> CREATE UNIQUE INDEX i ON tj (s2);
> SELECT * FROM tj;
> [1, 3], [2, 4], [3, 5]
> UPDATE OR REPLACE tj SET s2 = s2 + 1;
> SELECT * FROM tj;
> [1, 4], [3, 6]
> 
> I.e. [1, 3] tuple is updated as [1, 4] and have replaced tuple
> [2, 4]. This logic is implemented as preventive tuples deletion
> by all corresponding indexes in SQL.
> 
> The other significant change is forbidden primary key update.
> It was possible to deal with it the same way like with or
> REPLACE specifier but we need an atomic UPDATE step for #3691
> ticket to support "or IGNORE/or ABORT/or FAIL" specifiers.
> Reworked tests to make testing avoiding primary key UPDATE where
> possible.
> 
> Closes #3850
>