From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A31F323F00 for ; Fri, 28 Dec 2018 09:17:34 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Sa94GZT36ua3 for ; Fri, 28 Dec 2018 09:17:34 -0500 (EST) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B964A23EFB for ; Fri, 28 Dec 2018 09:17:33 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH v1 3/3] sql: do not use OP_Delete+OP_Insert for UPDATES From: "n.pettik" In-Reply-To: Date: Fri, 28 Dec 2018 16:17:28 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: <1E61D342-073F-454C-91D1-49C94DED93AF@tarantool.org> References: <7d8b0941a364a42f801f93e1526c1a6b244092e5.1545233551.git.kshcherbatov@tarantool.org> <487E532B-E70C-4707-890A-3D68ADAE0929@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov >> Please, don=E2=80=99t 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=E2=80=99d 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.=20 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 !=3D NULL); u16 pik_flags =3D OPFLAG_NCHANGE; - if (on_conflict =3D=3D ON_CONFLICT_ACTION_IGNORE) - pik_flags |=3D OPFLAG_OE_IGNORE; - else if (on_conflict =3D=3D ON_CONFLICT_ACTION_FAIL) - pik_flags |=3D OPFLAG_OE_FAIL; - else if (on_conflict =3D=3D ON_CONFLICT_ACTION_ROLLBACK) - pik_flags |=3D 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 =3D=3D ON_CONFLICT_ACTION_IGNORE) \ + opflag |=3D OPFLAG_OE_IGNORE; \ + else if (on_conflict =3D=3D ON_CONFLICT_ACTION_FAIL) \ + opflag |=3D OPFLAG_OE_FAIL; \ + else if (on_conflict =3D=3D ON_CONFLICT_ACTION_ROLLBACK) \ + opflag |=3D 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) } } =20 -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 !=3D NULL); - u16 pik_flags =3D OPFLAG_NCHANGE; - assert(on_conflict !=3D ON_CONFLICT_ACTION_REPLACE); - if (on_conflict =3D=3D ON_CONFLICT_ACTION_IGNORE) - pik_flags |=3D OPFLAG_OE_IGNORE; - else if (on_conflict =3D=3D ON_CONFLICT_ACTION_FAIL) - pik_flags |=3D OPFLAG_OE_FAIL; - else if (on_conflict =3D=3D ON_CONFLICT_ACTION_ROLLBACK) - pik_flags |=3D 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 =3D 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 =3D 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=E2=80=99t. 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=E2=80=99t 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. > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D >=20 > 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: >=20 > 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 =3D s2 + 1; > SELECT * FROM tj; > [1, 4], [3, 6] >=20 > 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. >=20 > 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. >=20 > Closes #3850 >=20