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 6493B2D1B8 for ; Thu, 22 Mar 2018 07:42:41 -0400 (EDT) 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 vgniftqAgJAM for ; Thu, 22 Mar 2018 07:42:41 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 B3F862D1A7 for ; Thu, 22 Mar 2018 07:42:40 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces From: "v.shpilevoy@tarantool.org" In-Reply-To: Date: Thu, 22 Mar 2018 14:42:37 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <6AAB3A70-E619-4232-AFC9-5EAB27D03961@tarantool.org> References: 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: Nikita Pettik See below 6 comments. > 21 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 2:48, Nikita = Pettik =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB= (=D0=B0): >=20 > Since it is impossible to use space pointers during execution of DDL > (after any DDL schema may change and pointers fetched at compile time > can become expired), special opcodes to operate on system spaces have > been introduced: OP_SInsert and OP_SDelete. They take space id as > an argument and make space lookup each time before insertion or = deletion. > However, sometimes it is still required to iterate through space (e.g. > to satisfy WHERE clause) during DDL. As far as now cursors rely on > pointers to space, it is required to convert space id to space pointer > during VDBE execution. Hence, another opcode is added: OP_SIDtoPtr. > Finally, existing opcodes, which are invoked only during DDL, have = been > also refactored. >=20 > Part of #3252 > --- > src/box/sql.c | 51 ++++++++++++------- > src/box/sql/opcodes.c | 45 +++++++++-------- > src/box/sql/opcodes.h | 53 ++++++++++---------- > src/box/sql/tarantoolInt.h | 11 ++--- > src/box/sql/vdbe.c | 121 = ++++++++++++++++++++++++++++++++++----------- > 5 files changed, 182 insertions(+), 99 deletions(-) >=20 > diff --git a/src/box/sql.c b/src/box/sql.c > index e943131ae..db4c7e7ea 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > +int > +sql_delete_by_key(struct space *space, char *key, uint32_t key_size) > +{ > struct request request; > memset(&request, 0, sizeof(request)); > request.type =3D IPROTO_DELETE; > request.key =3D key; > request.key_end =3D key + key_size; > - request.space_id =3D pCur->space->def->id; > - rc =3D sql_execute_dml(&request, pCur->space); > + request.space_id =3D space->def->id; > + int rc =3D sql_execute_dml(&request, space); 1. Why do you need sql_execute_dml? It does exactly the same as = process_rw, but process_rw can also return a tuple. And this process_rw = feature is needed for the patch on last inserted tuple returning. >=20 > @@ -1088,20 +1107,16 @@ int tarantoolSqlite3IncrementMaxid(BtCursor = *pCur) > request.ops =3D ops; > request.ops_end =3D ops + sizeof(ops); > request.type =3D IPROTO_UPSERT; > - request.space_id =3D pCur->space->def->id; > - rc =3D sql_execute_dml(&request, pCur->space); > + request.space_id =3D BOX_SCHEMA_ID; > + rc =3D sql_execute_dml(&request, space_schema); 2. If you use process_rw, then you do not need iterator to get the = previous max_id and increment it - you just do process_rw, get result = and do tuple_field_u64(result, 1, space_max_id); > if (rc !=3D 0) { > iterator_delete(it); > return SQL_TARANTOOL_ERROR; > } > - if (pCur->last_tuple !=3D NULL) { > - box_tuple_unref(pCur->last_tuple); > - } > - box_tuple_ref(res); > - pCur->last_tuple =3D res; > - pCur->eState =3D CURSOR_VALID; > + rc =3D tuple_field_u64(res, 1, space_max_id); > + (*space_max_id)++; > iterator_delete(it); > - return SQLITE_OK; > + return rc =3D=3D 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; > } >=20 > /* > @@ -1687,14 +1702,12 @@ int tarantoolSqlite3EphemeralGetMaxId(BtCursor = *pCur, uint32_t fieldno, > * If index is empty - return 0 in max_id and success status > */ > int > -tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno, > - uint64_t *max_id) > +tarantoolSqlGetMaxId(uint32_t space_id, uint64_t *max_id) 3. After changing a function signature, please, fix a comments too. And = why this function is sure, that the max id stored in the first column? = And why it can not be used for secondary indexes? > { > char key[16]; > struct tuple *tuple; > char *key_end =3D mp_encode_array(key, 0); > - if (box_index_max(cur->space->def->id, cur->index->def->iid, > - key, key_end, &tuple) !=3D 0) > + if (box_index_max(space_id, 0 /* PK */, key, key_end, &tuple) !=3D= 0) > return -1; >=20 > /* Index is empty */ > @@ -1703,5 +1716,5 @@ tarantoolSqlGetMaxId(BtCursor *cur, uint32_t = fieldno, > return 0; > } >=20 > - return tuple_field_u64(tuple, fieldno, max_id); > + return tuple_field_u64(tuple, 0, max_id); > } > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index 6f1ba3784..0b1e22ca2 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -77,6 +77,7 @@ int tarantoolSqlite3Count(BtCursor * pCur, i64 * = pnEntry); > int tarantoolSqlite3Insert(BtCursor * pCur); > int tarantoolSqlite3Replace(BtCursor * pCur); > int tarantoolSqlite3Delete(BtCursor * pCur, u8 flags); > +int sql_delete_by_key(struct space *space, char *key, uint32_t = key_size); > int tarantoolSqlite3ClearTable(struct space *space); >=20 > /* Rename table pTab with zNewName by inserting new tuple to _space. > @@ -112,11 +113,10 @@ int tarantoolSqlite3IdxKeyCompare(BtCursor * = pCur, UnpackedRecord * pUnpacked, > int *res); >=20 > /* > - * The function assumes the cursor is open on _schema. > - * Increment max_id and store updated tuple in the cursor > - * object. > + * The function assumes to be applied on _schema space. > + * Increment max_id and store updated id in given argument. > */ > -int tarantoolSqlite3IncrementMaxid(BtCursor * pCur); > +int tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id); >=20 > /* > * Render "format" array for _space entry. > @@ -150,5 +150,4 @@ int tarantoolSqlite3MakeIdxOpts(Index * index, = const char *zSql, void *buf); > * Fetch maximum value from ineger column number `fieldno` of = space_id/index_id > * Return 0 on success, -1 otherwise > */ > -int tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno, > - uint64_t * max_id); > +int tarantoolSqlGetMaxId(uint32_t space_id, uint64_t *max_id); > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 5d1227afa..d333d4177 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3791,28 +3791,18 @@ case OP_Sequence: { /* out2 */ > break; > } >=20 > -/* Opcode: NextId P1 P2 P3 * * > - * Synopsis: r[P3]=3Dget_max(space_index[P1]{Column[P2]}) > +/* Opcode: NextId P1 P2 * * * > + * Synopsis: r[P2]=3Dget_max(space_id[P1]) > * > - * Get next Id of the table. P1 is a table cursor, P2 is column > - * number. Return in P3 maximum id found in provided column, > + * Get next Id of the table. P1 is a space id. > + * Return in P2 maximum id found in provided column, > * incremented by one. > - * > - * This opcode is Tarantool specific and will segfault in case > - * of SQLite cursor. > */ > -case OP_NextId: { /* out3 */ > - VdbeCursor *pC; /* The VDBE cursor */ > - int p2; /* Column number, which stores the id */ > - pC =3D p->apCsr[pOp->p1]; > - p2 =3D pOp->p2; > - pOut =3D &aMem[pOp->p3]; > - > - /* This opcode is Tarantool specific. */ > - assert(pC->uc.pCursor->curFlags & BTCF_TaCursor); > +case OP_NextId: { > + assert(pOp->p1 > 0); > + pOut =3D &aMem[pOp->p2]; >=20 > - tarantoolSqlGetMaxId(pC->uc.pCursor, p2, > - (uint64_t *) &pOut->u.i); > + tarantoolSqlGetMaxId(pOp->p1, (uint64_t *) &pOut->u.i); >=20 > pOut->u.i +=3D 1;=20 > pOut->flags =3D MEM_Int; > @@ -4456,6 +4446,84 @@ case OP_IdxInsert: { /* in2 */ > break; > } >=20 > +/* Opcode: SInsert P1 P2 * * P5 > + * Synopsis: space id =3D P1, key =3D r[P2] > + * > + * This opcode is used only during DML routine. 4. Do you mean DDL instead of DML? > + * In contrast to ordinary insertion, insertion to system spaces > + * such as _space or _index will lead to schema changes. > + * Thus, usage of space pointers is going to be impossible, > + * as far as pointers can be expired since compilation time. > + * > + * If P5 is set to OPFLAG_NCHANGE, account overall changes > + * made to database. > + */ > +case OP_SInsert: { > + assert(pOp->p1 > 0); > + assert(pOp->p2 >=3D 0); > + > + pIn2 =3D &aMem[pOp->p2]; > + struct space *space =3D space_by_id(pOp->p1); > + assert(space !=3D NULL); > + assert(space_is_system(space)); > + /* Create surrogate cursor to pass to SQL bindings. */ > + BtCursor surrogate_cur; > + surrogate_cur.space =3D space; > + surrogate_cur.key =3D pIn2->z; > + surrogate_cur.nKey =3D pIn2->n; > + surrogate_cur.curFlags =3D BTCF_TaCursor; > + rc =3D tarantoolSqlite3Insert(&surrogate_cur); > + if (rc) > + goto abort_due_to_error; > + if (pOp->p5 & OPFLAG_NCHANGE) > + p->nChange++; > + break; > +} > + > +/* Opcode: SDelete P1 P2 * * P5 > + * Synopsis: space id =3D P1, key =3D r[P2] > + * > + * This opcode is used only during DML routine. 5. Same as 4 - do you mean DDL? > + * Delete entry with given key from system space. > + * > + * If P5 is set to OPFLAG_NCHANGE, account overall changes > + * made to database. > + */ > +case OP_SDelete: { > + assert(pOp->p1 > 0); > + assert(pOp->p2 >=3D 0); > + > + pIn2 =3D &aMem[pOp->p2]; > + struct space *space =3D space_by_id(pOp->p1); > + assert(space !=3D NULL); > + assert(space_is_system(space)); > + rc =3D sql_delete_by_key(space, pIn2->z, pIn2->n); > + if (rc) > + goto abort_due_to_error; > + if (pOp->p5 & OPFLAG_NCHANGE) > + p->nChange++; > + break; > +} > + > +/* Opcode: SIDtoPtr P1 P2 * * * > + * Synopsis: space id =3D P1, space[out] =3D r[P2] > + * > + * This opcode makes look up by space id and save found space > + * into register, specified by the content of register P2. > + * Such trick is needed during DML routine, since schema may > + * change and pointers become expired. > + */ > +case OP_SIDtoPtr: { 6. It seems to be unused in the patch. Can you use it for all these = things, introduced by the previous patch? struct space *space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); assert(space !=3D NULL); sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void *) = space); sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, pIdx->tnum, space_ptr_reg); Using OP_SIDtoPtr you already in this patch can defer space_by_id = lookup. > + assert(pOp->p1 > 0); > + assert(pOp->p2 > 0); > + > + pIn2 =3D &aMem[pOp->p2]; > + struct space *space =3D space_by_id(pOp->p1); > + assert(space !=3D NULL); > + pIn2->u.i =3D (int64_t) space; > + break; > +} > + >=20 >=20