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 3185A21A1D for ; Tue, 25 Dec 2018 12:26: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 NZltV-YB9AzN for ; Tue, 25 Dec 2018 12:26:34 -0500 (EST) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 C373121A04 for ; Tue, 25 Dec 2018 12:26: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: <7d8b0941a364a42f801f93e1526c1a6b244092e5.1545233551.git.kshcherbatov@tarantool.org> Date: Tue, 25 Dec 2018 19:26:31 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: <487E532B-E70C-4707-890A-3D68ADAE0929@tarantool.org> References: <7d8b0941a364a42f801f93e1526c1a6b244092e5.1545233551.git.kshcherbatov@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 > @@ -253,15 +246,29 @@ fkey_lookup_parent(struct Parse *parse_context, = struct space *parent, > } > sqlite3VdbeGoto(v, ok_label); > } > - struct index *idx =3D space_index(parent, referenced_idx); > - assert(idx !=3D NULL); > - sqlite3VdbeAddOp4(v, OP_MakeRecord, temp_regs, field_count, = rec_reg, > - = sql_space_index_affinity_str(parse_context->db, > - parent->def, = idx->def), > - P4_DYNAMIC); > - sqlite3VdbeAddOp4Int(v, OP_Found, cursor, ok_label, rec_reg, 0); > - sqlite3ReleaseTempReg(parse_context, rec_reg); > - sqlite3ReleaseTempRange(parse_context, temp_regs, field_count); Please, don=E2=80=99t change code without any explanations. You should state it within commit message or in source code. > + if (!(fkey_is_self_referenced(fk_def) && op =3D=3D TK_UPDATE)) { > + int temp_regs =3D sqlite3GetTempRange(parse_context, = field_count); > + int rec_reg =3D sqlite3GetTempReg(parse_context); > + vdbe_emit_open_cursor(parse_context, cursor, = referenced_idx, > + parent); > + link =3D fk_def->links; > + for (uint32_t i =3D 0; i < field_count; ++i, ++link) { > + sqlite3VdbeAddOp2(v, OP_Copy, > + link->child_field + 1 + = reg_data, > + temp_regs + i); > + } > + struct index *idx =3D space_index(parent, = referenced_idx); > + assert(idx !=3D NULL); > + sqlite3VdbeAddOp4(v, OP_MakeRecord, temp_regs, = field_count, > + rec_reg, > + = sql_space_index_affinity_str(parse_context->db, > + = parent->def, > + = idx->def), > + P4_DYNAMIC); > + sqlite3VdbeAddOp4Int(v, OP_Found, cursor, ok_label, = rec_reg, 0); > + sqlite3ReleaseTempReg(parse_context, rec_reg); > + sqlite3ReleaseTempRange(parse_context, temp_regs, = field_count); > + } > struct session *session =3D current_session(); > if (!fk_def->is_deferred && > (session->sql_flags & SQLITE_DeferFKs) =3D=3D 0 && > @@ -515,7 +522,7 @@ fkey_action_is_set_null(struct Parse = *parse_context, const struct fkey *fkey) >=20 > void > fkey_emit_check(struct Parse *parser, struct Table *tab, int reg_old, > - int reg_new, const int *changed_cols) > + int reg_new, const int *changed_cols, int op) Why do you need additional argument? reg_new !=3D 0 for insert reg_old !=3D 0 for delete Changed_cols !=3D 0 for update (According to comment to this function) What is more, according to code you need only case when update is processed. > { > struct sqlite3 *db =3D parser->db; > struct session *user_session =3D current_session(); > @@ -549,7 +556,7 @@ fkey_emit_check(struct Parse *parser, struct Table = *tab, int reg_old, > * foreign key constraint violation. > */ > fkey_lookup_parent(parser, parent, fk_def, = fk->index_id, > - reg_old, -1); > + reg_old, -1, op); > } > if (reg_new !=3D 0 && !fkey_action_is_set_null(parser, = fk)) { > /* > @@ -568,7 +575,7 @@ fkey_emit_check(struct Parse *parser, struct Table = *tab, int reg_old, > * cause an FK violation. > */ > fkey_lookup_parent(parser, parent, fk_def, = fk->index_id, > - reg_new, +1); > + reg_new, +1, op); > } > } > /* >=20 > void > -vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space, > - int raw_data_reg, uint32_t tuple_len, > +vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space, = int op, > + int raw_data_reg, int raw_key_reg, > + int upd_cols_reg, > enum on_conflict_action on_conflict) > { > assert(v !=3D NULL); > @@ -1065,10 +1065,18 @@ vdbe_emit_insertion_completion(struct Vdbe *v, = struct space *space, > pik_flags |=3D OPFLAG_OE_FAIL; > else if (on_conflict =3D=3D ON_CONFLICT_ACTION_ROLLBACK) > pik_flags |=3D OPFLAG_OE_ROLLBACK; > - sqlite3VdbeAddOp3(v, OP_MakeRecord, raw_data_reg, tuple_len, > - raw_data_reg + tuple_len); > - sqlite3VdbeAddOp1(v, OP_IdxInsert, raw_data_reg + tuple_len); > - sqlite3VdbeChangeP4(v, -1, (char *)space, P4_SPACEPTR); > + if (op =3D=3D OP_IdxInsert) { > + uint32_t tuple_raw_reg =3D raw_data_reg + = space->def->field_count; > + sqlite3VdbeAddOp3(v, OP_MakeRecord, raw_data_reg, > + space->def->field_count, = tuple_raw_reg); > + sqlite3VdbeAddOp1(v, op, tuple_raw_reg); > + sqlite3VdbeChangeP4(v, -1, (char *)space, P4_SPACEPTR); > + } else if (op =3D=3D OP_IdxUpdate) { > + sqlite3VdbeAddOp4(v, op, raw_data_reg, raw_key_reg, > + upd_cols_reg, (char *)space, = P4_SPACEPTR); > + } else { > + unreachable(); > + } I=E2=80=99d better introduce separate function (or simply inline) for = emitting update operation. Lets keep vdbe_emit_insertion_completion as is. > sqlite3VdbeChangeP5(v, pik_flags); > } >=20 >=20 > void > @@ -4730,10 +4738,11 @@ void sqlite3WithPush(Parse *, With *, u8); > * @param reg_old Register with deleted row. > * @param reg_new Register with inserted row. > * @param changed_cols Array of updated columns. Can be NULL. > + * @param op operation, one of TK_UPDATE, TK_INSERT, TK_DELETE. > */ > void > fkey_emit_check(struct Parse *parser, struct Table *tab, int reg_old, > - int reg_new, const int *changed_cols); > + int reg_new, const int *changed_cols, int op); >=20 > /** > * Emit VDBE code to do CASCADE, SET NULL or SET DEFAULT actions > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 0e2d0fde8..aca8e4adc 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -36,6 +36,7 @@ > #include "sqliteInt.h" > #include "box/session.h" > #include "tarantoolInt.h" > +#include "box/tuple_format.h" > #include "box/schema.h" >=20 > void > @@ -112,6 +113,8 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > int regNew =3D 0; /* Content of the NEW.* table in = triggers */ > int regOld =3D 0; /* Content of OLD.* table in = triggers */ > int regKey =3D 0; /* composite PRIMARY KEY value = */ > + /* Count of changed rows. Match aXRef items !=3D -1. */ > + int upd_cols_cnt =3D 0; >=20 > db =3D pParse->db; > if (pParse->nErr || db->mallocFailed) { > @@ -183,6 +186,7 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > goto update_cleanup; > } > aXRef[j] =3D i; > + upd_cols_cnt++; > break; > } > } > @@ -216,7 +220,7 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > regNewPk =3D ++pParse->nMem; > } > regNew =3D pParse->nMem + 1; > - pParse->nMem +=3D def->field_count; > + pParse->nMem +=3D def->field_count + 1; >=20 > /* If we are trying to update a view, realize that view into > * an ephemeral table. > @@ -430,23 +434,64 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > vdbe_emit_constraint_checks(pParse, pTab, regNewPk + 1, > on_error, labelContinue, = aXRef); > /* Do FK constraint checks. */ > - if (hasFK) > - fkey_emit_check(pParse, pTab, regOldPk, 0, = aXRef); > - /* > - * Delete the index entries associated with the > - * current record. It can be already removed by > - * trigger or REPLACE conflict action. > - */ > - int addr1 =3D sqlite3VdbeAddOp4Int(v, OP_NotFound, = pk_cursor, 0, > - regKey, nKey); > - assert(regNew =3D=3D regNewPk + 1); > - sqlite3VdbeAddOp2(v, OP_Delete, pk_cursor, 0); > - sqlite3VdbeJumpHere(v, addr1); > - if (hasFK) > - fkey_emit_check(pParse, pTab, 0, regNewPk, = aXRef); > - vdbe_emit_insertion_completion(v, space, regNew, > - pTab->def->field_count, > - on_error); > + if (hasFK) { > + fkey_emit_check(pParse, pTab, regOldPk, 0, = aXRef, > + TK_UPDATE); > + } > + if (on_error =3D=3D ON_CONFLICT_ACTION_REPLACE) { > + /* > + * Delete the index entries associated with the > + * current record. It can be already removed by > + * trigger or REPLACE conflict action. > + */ > + int not_found_lbl =3D > + sqlite3VdbeAddOp4Int(v, OP_NotFound, = pk_cursor, > + 0, regKey, nKey); > + assert(regNew =3D=3D regNewPk + 1); > + sqlite3VdbeAddOp2(v, OP_Delete, pk_cursor, 0); > + sqlite3VdbeJumpHere(v, not_found_lbl); > + } > + if (hasFK) { > + fkey_emit_check(pParse, pTab, 0, regNewPk, = aXRef, > + TK_UPDATE); > + } > + if (on_error =3D=3D ON_CONFLICT_ACTION_REPLACE) { > + vdbe_emit_insertion_completion(v, space, = OP_IdxInsert, > + regNew, 0, 0, = on_error); > + } else { > + int key_reg; > + if (okOnePass) { > + key_reg =3D sqlite3GetTempReg(pParse); > + const char *zAff =3D Nit: zAff -> aff_str. > + = sql_space_index_affinity_str(pParse->db, > + = def, > + = pPk->def); > + sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, > + pk_part_count, = key_reg, zAff, > + pk_part_count); > + } else { > + assert(nKey =3D=3D 0); > + key_reg =3D regKey; > + } > + > + /* Prapare array of changed fields. */ Nit: prepare. > + uint32_t upd_cols_sz =3D upd_cols_cnt * = sizeof(uint32_t); > + uint32_t *upd_cols =3D sqlite3DbMallocRaw(db, = upd_cols_sz); > + if (upd_cols =3D=3D NULL) > + goto update_cleanup; > + upd_cols_cnt =3D 0; > + for (uint32_t i =3D 0; i < def->field_count; = i++) { > + if (aXRef[i] =3D=3D -1) > + continue; > + upd_cols[upd_cols_cnt++] =3D i; > + } > + int upd_cols_reg =3D sqlite3GetTempReg(pParse); I remember that there was issue connected with different register allocation policy (during analyze registers were allocated with sqlite3GetTempReg and simple nMem++). Wouldn=E2=80=99t we face the same problem here? > + sqlite3VdbeAddOp4(v, OP_Blob, upd_cols_sz, = upd_cols_reg, > + 0, (const char *)upd_cols, = P4_DYNAMIC); > + vdbe_emit_insertion_completion(v, space, = OP_IdxUpdate, > + regNew, key_reg, > + upd_cols_reg, = on_error); > + } > /* > * Do any ON CASCADE, SET NULL or SET DEFAULT > * operations required to handle rows that refer > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index bf6f4cdd1..534d3e885 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c >=20 > +/* Opcode: OP_IdxUpdate P1 P2 P3 P4 P5 > + * Synopsis: key=3Dr[P1] Hmm, during vibe execution I see no =E2=80=99synopsis=E2=80=99. What is more, I look at opcodes.h and: /* 114 */ "IdxUpdate" OpHelp("=E2=80=9D), So, our script didn=E2=80=99t generate auxiliary information for opcode. Open an issue or investigate this case pls. If you fix it, extend synopsis with other args. > + * > + * Make space Update operation. Primary key fields could not be > + * modified - use OP_Replace opcode instead. > + * This opcode extends IdxInsert/IdxReplace interface to > + * transform to OP_Replace in particular case of sql operator > + * "UPDATE or REPLACE". > + * > + * @param P1 The first field of updated tuple. > + * @param P2 Index of a register with index key MakeRecord. > + * @param P3 Index of a register with upd_fields blob. > + * This blob has size sizeof(uint32_t)*upd_fields_cnt. > + * It's items are numbers of fields to be replaced with > + * new values from P1 blob. They must be sorted in > + * ascending order. I would slightly change comment: - * Make space Update operation. Primary key fields could not be - * modified - use OP_Replace opcode instead. - * This opcode extends IdxInsert/IdxReplace interface to - * transform to OP_Replace in particular case of sql operator - * "UPDATE or REPLACE". - * - * @param P1 The first field of updated tuple. - * @param P2 Index of a register with index key MakeRecord. + * Process UPDATE operation. Primary key fields can not be + * modified - in this case OP_IdxReplace is used instead. + * Under the hood it performs box_update() call. + * For the performance sake, it takes whole affected row (P1) + * and encodes into msgpack only fields to be updated (P3). + * + * @param P1 The first field to be updated. Fields are located + * in the range of [P1...] in decoded state. + * Encoded only fields which numbers are presented + * in @P3 array. + * @param P2 Encoded key to be passed to box_update(). * @param P3 Index of a register with upd_fields blob. - * This blob has size sizeof(uint32_t)*upd_fields_cnt. * It's items are numbers of fields to be replaced with - * new values from P1 blob. They must be sorted in - * ascending order. + * new values from P1. + * They must be sorted in ascending order. > + * @param P4 Pointer to the struct space to be updated. > + * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE > + * accounts the change in a case of successful > + * insertion in nChange counter. If P5 contains > + * OPFLAG_OE_IGNORE, then we are processing INSERT OR > + * INGORE statement. Thus, in case of conflict we don't > + * raise an error. > + */ > +case OP_IdxUpdate: { Don=E2=80=99t call it IdxUpdate - it has nothing in common with index. OP_IdxInsert is called this way due to historical reasons. Lets simply name it OP_Update. > + struct Mem *new_tuple =3D &aMem[pOp->p1]; > + if (pOp->p5 & OPFLAG_NCHANGE) > + p->nChange++; > + > + struct space *space =3D pOp->p4.space; > + assert(pOp->p4type =3D=3D P4_SPACEPTR); > + > + struct Mem *key_mem =3D &aMem[pOp->p2]; > + assert((key_mem->flags & MEM_Blob) !=3D 0); > + > + struct Mem *upd_fields_mem =3D &aMem[pOp->p3]; > + assert((upd_fields_mem->flags & MEM_Blob) !=3D 0); > + uint32_t *upd_fields =3D (uint32_t *)upd_fields_mem->z; > + uint32_t upd_fields_cnt =3D upd_fields_mem->n / = sizeof(uint32_t); > + > + /* Prepare Tarantool update ops msgpack. */ > + struct region *region =3D &fiber()->gc; > + size_t used =3D region_used(region); > + bool is_error =3D false; > + struct mpstream stream; > + mpstream_init(&stream, region, region_reserve_cb, = region_alloc_cb, > + mpstream_encode_error, &is_error); > + mpstream_encode_array(&stream, upd_fields_cnt); > + for (uint32_t i =3D 0; i < upd_fields_cnt; i++) { > + uint32_t field_idx =3D upd_fields[i]; > + assert(field_idx < space->def->field_count); > + mpstream_encode_array(&stream, 3); > + mpstream_encode_strn(&stream, "=3D", 1); > + mpstream_encode_uint(&stream, field_idx + 1); > + mpstream_encode_vdbe_mem(&stream, new_tuple + = field_idx); > + } > + mpstream_flush(&stream); > + if (is_error) { > + diag_set(OutOfMemory, stream.pos - stream.buf, > + "mpstream_flush", "stream"); > + rc =3D SQL_TARANTOOL_ERROR; > + goto abort_due_to_error; > + } > + uint32_t ops_size =3D region_used(region) - used; > + const char *ops =3D region_join(region, ops_size); > + if (ops =3D=3D NULL) { > + diag_set(OutOfMemory, ops_size, "region_join", "raw"); > + rc =3D SQL_TARANTOOL_ERROR; > + goto abort_due_to_error; > + } > + > + rc =3D SQLITE_OK; assert(rc =3D=3D SQLITE_OK); If no error takes place, rc must be 0. Otherwise, error must be handled before. > + if (box_update(space->def->id, 0, key_mem->z, key_mem->z + = key_mem->n, > + ops, ops + ops_size, TUPLE_INDEX_BASE, NULL) !=3D = 0) I see comment to struct request: /** Base field offset for UPDATE/UPSERT, e.g. 0 for C and 1 for Lua. */ int index_base; So why do you pass 1 instead of 0? > + rc =3D SQL_TARANTOOL_ERROR; > + > + if (pOp->p5 & OPFLAG_OE_IGNORE) { > + /* > + * Ignore any kind of failes and do not raise Nit: fails. > + * error message > + */ > + rc =3D SQLITE_OK; > + /* > + * If we are in trigger, increment ignore raised > + * counter. > + */ > + if (p->pFrame) > + p->ignoreRaised++; > + } else if (pOp->p5 & OPFLAG_OE_FAIL) { > + p->errorAction =3D ON_CONFLICT_ACTION_FAIL; > + } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) { > + p->errorAction =3D ON_CONFLICT_ACTION_ROLLBACK; > + } > + if (rc !=3D 0) > + goto abort_due_to_error; > + break; > +} >=20 > /* Opcode: SInsert P1 P2 P3 * P5 > * Synopsis: space id =3D P1, key =3D r[P3], on error goto P2 > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index 50bc35b2b..69898bbcf 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -552,4 +552,10 @@ int sqlite3VdbeRecordCompareMsgpack(const void = *key1, > struct UnpackedRecord *key2); > u32 sqlite3VdbeMsgpackGet(const unsigned char *buf, Mem * pMem); >=20 > +struct mpstream; > + > +/** Perform encoding memory variable to stream. */ > +void > +mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var); > + > #endif /* !defined(SQLITE_VDBEINT_H) */ > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c > index 22beba8be..1fee2673d 100644 > --- a/src/box/sql/vdbemem.c > +++ b/src/box/sql/vdbemem.c > @@ -40,6 +40,7 @@ > #include "vdbeInt.h" > #include "tarantoolInt.h" > #include "box/schema.h" > +#include "mpstream.h" >=20 > #ifdef SQLITE_DEBUG > /* > @@ -1714,3 +1715,38 @@ sqlite3ValueBytes(sqlite3_value * pVal) > return 0; > return valueBytes(pVal); > } > + > +void > +mpstream_encode_vdbe_mem(struct mpstream *stream, struct Mem *var) Can we use this routine to encode MsgPack during execution of = OP_MakeRecord? Now it uses sqlite3VdbeMsgpackRecordPut() which is almost the same. > diff --git a/src/mpstream.c b/src/mpstream.c > index e4f7950ba..843fc959e 100644 > --- a/src/mpstream.c > +++ b/src/mpstream.c > @@ -175,3 +175,23 @@ mpstream_encode_bool(struct mpstream *stream, = bool val) > char *pos =3D mp_encode_bool(data, val); > mpstream_advance(stream, pos - data); > } > + > +void > +mpstream_encode_raw(struct mpstream *stream, const char *raw, = uint32_t len) Hm, it doesn=E2=80=99t encode in fact. Mb better call _copy_raw? > +{ > + char *data =3D mpstream_reserve(stream, len); > + if (data =3D=3D NULL) > + return; > + memcpy(data, raw, len); > + mpstream_advance(stream, len); > +} >=20 > #if defined(__cplusplus) > } /* extern "C" */ > #endif /* defined(__cplusplus) */ At this point I didn=E2=80=99t review test changes.