> On 22 Mar 2018, at 16:09, v.shpilevoy@tarantool.org wrote: > > > >> 22 марта 2018 г., в 15:23, n.pettik > написал(а): >> >> >>> On 22 Mar 2018, at 14:42, v.shpilevoy@tarantool.org wrote: >>> >>> See below 6 comments. >>> >>>> 21 марта 2018 г., в 2:48, Nikita Pettik > написал(а): >>>> >>>> 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. >>>> >>>> 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(-) >>>> >>>> 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 = IPROTO_DELETE; >>>> request.key = key; >>>> request.key_end = key + key_size; >>>> - request.space_id = pCur->space->def->id; >>>> - rc = sql_execute_dml(&request, pCur->space); >>>> + request.space_id = space->def->id; >>>> + int rc = 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. >> >> I need it as an analogue of process_rw(), since process_rw() is unavailable >> via public BOX interface (declared as static). If you approve to make it public, >> where should I place it and should I rename it? >> Or, for instance, make sql_execute_dml be complete copy of process_rw()? >> It is unlikely to be useful somewhere except for SQL (and box internals surely). >> Initial indent was to reduce overhead while calling box functions from SQL bindings (sql.c): >> ordinary call stack looks like: box_insert() -> box_process1() -> space lookup + process_rw(). >> In such scheme we also process redundant space lookup since we already have struct space * in cursor. >> (Patch where I introduced this function isn’t in trunk, so I can easily fix it). > > Yes, I very like the removal of box_process1. I just do not like code duplicate. But also I can not see a place for declaration of process_rw in any header. So lets make it extern "C" non-static in box.cc , and declare it as extern in sql.c. Ok, I’m attaching patch to email (since it belongs to another patch-set: np/gh-3122-remove-pgnoRoot) > >> >>>> >>>> @@ -1088,20 +1107,16 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur) >>>> request.ops = ops; >>>> request.ops_end = ops + sizeof(ops); >>>> request.type = IPROTO_UPSERT; >>>> - request.space_id = pCur->space->def->id; >>>> - rc = sql_execute_dml(&request, pCur->space); >>>> + request.space_id = BOX_SCHEMA_ID; >>>> + rc = 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); > > Suppose, this will be fixed, if you will use process_rw, as described above. Fixed (I am attaching changes at the end of mail). > >>> >>>> if (rc != 0) { >>>> iterator_delete(it); >>>> return SQL_TARANTOOL_ERROR; >>>> } >>>> - if (pCur->last_tuple != NULL) { >>>> - box_tuple_unref(pCur->last_tuple); >>>> - } >>>> - box_tuple_ref(res); >>>> - pCur->last_tuple = res; >>>> - pCur->eState = CURSOR_VALID; >>>> + rc = tuple_field_u64(res, 1, space_max_id); >>>> + (*space_max_id)++; >>>> iterator_delete(it); >>>> - return SQLITE_OK; >>>> + return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; >>>> } >>>> >>>> /* >>>> @@ -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? >> >> It might, but it is only applied for _sequence space during DDL routine >> (and this is also answer for the first question: format of system space doesn’t change frequently), >> so I decided to remove cursor/index overhead (as a part of overall DDL refactoring). >> Probably, I should also rename this opcode to make it more clear. > > Yes, lets rename it, and remove space_id argument, if it is always used for _sequence space. Done (I am attaching changes at the end of mail). >>>> >>>> +/* Opcode: SInsert P1 P2 * * P5 >>>> + * Synopsis: space id = P1, key = r[P2] >>>> + * >>>> + * This opcode is used only during DML routine. >>> >>> 4. Do you mean DDL instead of DML? > > Hm? Fixed (at both places). > >>> >>>> + * 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); >>>> + >>>> +/* Opcode: SIDtoPtr P1 P2 * * * >>>> + * Synopsis: space id = P1, space[out] = 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 = space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); >>> assert(space != 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. >> >> It is used in the next patch. I don’t want to make a lot of diffs, since introducing of any opcodes >> leads to auto-generating of opcodes.c/h and as a result to vast amount of redundant diff. > > Ok, then lets leave it in this patch. > >> As for your example, during SQL DDL we need to execute queries such as: >> 'DELETE FROM BOX_SEQUENCE WHERE name = zName;’ >> And ‘name’ is not a key. Hence, we have to open cursor and iterate through the space. >> But if we used pointers to spaces which were fetched at compile state, >> they would become expired at VDBE runtime: >> after first stage of DDL (e.g. deletion from _trigger), schema may change. >> Moreover, we are going to support syntax like: ‘CREATE TABLE AS SELECT …’, >> where DDL and DML are mixing. Thus we need facility to make space lookup at VDBE runtime. > > I agree, that we must have this opcode, but why it can not be used for DML too? In my example you at compile time get space *, store it by OP_LoadPtr, and then use to open iterator (but I can not see where - I commented this in a previous patch review), but for DML you can replace compile lookups on runtime lookups using OP_SIDtoPtr. And it also solves the problem, that a prepared VDBE statement (which in a future we will store for a long time) must store space * in opcodes - we can use OP_SIDtoPtr and do not store it until execution. Sorry, can not explain more clear. We can also discuss it face-to-face. Now I get what you mean. Yep, currently space is slightly used during compile time, so its lookup can be always deferred until runtime. But in the nearest future, all properties will be fetched directly from space (at compile time), so there will be no need to make additional space lookup at runtime, if we load it into VDBE during compile time. Patches on process_rw() and updates from this one are below. ======================================================================= Originally, process_rw() function implements internal logic (such as txn or metrics routine) and calls DML executor. However, it isn't available for other modules (i.e. declared as static). On the other hand, it seems to be very useful for SQL, since it allows to reduce call stack and get rid of redundant space lookup. Thus, lets rename it to 'box_process_rw()' and declare in box.h --- src/box/box.cc | 11 ++++++----- src/box/box.h | 17 ++++++++++++++++- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/box/box.cc b/src/box/box.cc index a3bbdfce8..12c662dd8 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -160,8 +160,9 @@ box_check_memtx_min_tuple_size(ssize_t memtx_min_tuple_size) "specified value is out of bounds"); } -static int -process_rw(struct request *request, struct space *space, struct tuple **result) +int +box_process_rw(struct request *request, struct space *space, + struct tuple **result) { assert(iproto_type_is_dml(request->type)); rmean_collect(rmean_box, request->type, 1); @@ -279,7 +280,7 @@ apply_row(struct xstream *stream, struct xrow_header *row) struct request request; xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type)); struct space *space = space_cache_find_xc(request.space_id); - if (process_rw(&request, space, NULL) != 0) { + if (box_process_rw(&request, space, NULL) != 0) { say_error("error applying row: %s", request_str(&request)); diag_raise(); } @@ -821,7 +822,7 @@ boxk(int type, uint32_t space_id, const char *format, ...) struct space *space = space_cache_find(space_id); if (space == NULL) return -1; - return process_rw(&request, space, NULL); + return box_process_rw(&request, space, NULL); } int @@ -893,7 +894,7 @@ box_process1(struct request *request, box_tuple_t **result) return -1; if (!space->def->opts.temporary && box_check_writable() != 0) return -1; - return process_rw(request, space, result); + return box_process_rw(request, space, result); } int diff --git a/src/box/box.h b/src/box/box.h index c9b5aad01..e337cb0ee 100644 --- a/src/box/box.h +++ b/src/box/box.h @@ -50,6 +50,7 @@ struct xrow_header; struct obuf; struct ev_io; struct auth_request; +struct space; /* * Initialize box library @@ -379,15 +380,29 @@ box_sequence_reset(uint32_t seq_id); /** \endcond public */ /** - * The main entry point to the + * Used to be entry point to the * Box: callbacks into the request processor. * These are function pointers since they can * change when entering/leaving read-only mode * (master->slave propagation). + * However, it makes space lookup. If space is already obtained, + * one can simply use internal box_process_rw(). */ int box_process1(struct request *request, box_tuple_t **result); +/** + * Execute request on given space. + * + * \param request Request to be executed + * \param space Space to be updated + * \param result Result of executed request + * \retval 0 in success, -1 otherwise + */ +int +box_process_rw(struct request *request, struct space *space, + struct tuple **result); + int boxk(int type, uint32_t space_id, const char *format, ...); -- 2.15.1 ======================================================================= /* * The function assumes the cursor is open on _schema. - * Increment max_id and store updated tuple in the cursor - * object. + * Increment max_id and store updated value it output parameter. */ -int tarantoolSqlite3IncrementMaxid(BtCursor *pCur) +int tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id) { - assert(pCur->curFlags & BTCF_TaCursor); /* ["max_id"] */ static const char key[] = { (char)0x91, /* MsgPack array(1) */ @@ -1035,6 +1050,8 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur) struct tuple *res = NULL; int rc; + struct space *space_schema = space_by_id(BOX_SCHEMA_ID); + assert(space_schema != NULL); struct request request; memset(&request, 0, sizeof(request)); request.tuple = ops; @@ -1042,18 +1059,14 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur) request.key = key; request.key_end = key + sizeof(key); request.type = IPROTO_UPDATE; - request.space_id = pCur->space->def->id; - rc = box_process_rw(&request, pCur->space, &res); + request.space_id = space_schema->def->id; + rc = box_process_rw(&request, space_schema, &res); if (rc != 0 || res == NULL) { return SQL_TARANTOOL_ERROR; } - if (pCur->last_tuple != NULL) { - box_tuple_unref(pCur->last_tuple); - } - box_tuple_ref(res); - pCur->last_tuple = res; - pCur->eState = CURSOR_VALID; - return SQLITE_OK; + rc = tuple_field_u64(res, 1, space_max_id); + (*space_max_id)++; + return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; } /* @@ -1633,25 +1646,20 @@ int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno, } /** - * Extract maximum integer value from: - * @param index space_id - * @param index_id - * @param field number fieldno - * @param[out] fetched value in max_id + * Extract next id from _sequence space. + * If index is empty - return 0 in max_id and success status * + * @param[out] max_id Fetched value. * @retval 0 on success, -1 otherwise. - * - * If index is empty - return 0 in max_id and success status */ int -tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno, - uint64_t *max_id) +tarantoolSqlNextSeqId(uint64_t *max_id) { char key[16]; struct tuple *tuple; char *key_end = mp_encode_array(key, 0); - if (box_index_max(cur->space->def->id, cur->index->def->iid, - key, key_end, &tuple) != 0) + if (box_index_max(BOX_SEQUENCE_ID, 0 /* PK */, key, + key_end, &tuple) != 0) return -1; /* Index is empty */ @@ -1660,5 +1668,5 @@ tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno, return 0; } - return tuple_field_u64(tuple, fieldno, max_id); + return tuple_field_u64(tuple, 0, max_id); } diff --git a/src/box/sql/build.c b/src/box/sql/build.c index d5b0f6001..db7ffe24e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2018,7 +2018,8 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ sqlite3OpenTable(pParse, iCursor, sys_sequence, OP_OpenWrite); reg_seq_id = ++pParse->nMem; - sqlite3VdbeAddOp3(v, OP_NextId, iCursor, 0, reg_seq_id); + sqlite3VdbeAddOp3(v, OP_NextSequenceId, iCursor, 0, + reg_seq_id); reg_seq_record = emitNewSysSequenceRecord(pParse, reg_seq_id, --- 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); /* Rename table pTab with zNewName by inserting new tuple to _space. @@ -112,11 +113,10 @@ int tarantoolSqlite3IdxKeyCompare(BtCursor * pCur, UnpackedRecord * pUnpacked, int *res); /* - * 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); /* * Render "format" array for _space entry. @@ -147,8 +147,6 @@ int tarantoolSqlite3MakeIdxParts(Index * index, void *buf); 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 + * Fetch next id from _sequence space. */ -int tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno, - uint64_t * max_id); +int tarantoolSqlNextSeqId(uint64_t *max_id); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 0ae682f9e..3d9ac8e26 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3830,28 +3830,16 @@ case OP_Sequence: { /* out2 */ break; } -/* Opcode: NextId P1 P2 P3 * * - * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) +/* Opcode: NextSequenceId * P2 * * * + * Synopsis: r[P2]=get_max(_sequence) * - * 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 _sequence space. + * Return in P2 maximum id found in _sequence, * 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 = p->apCsr[pOp->p1]; - p2 = pOp->p2; - pOut = &aMem[pOp->p3]; - - /* This opcode is Tarantool specific. */ - assert(pC->uc.pCursor->curFlags & BTCF_TaCursor); - - tarantoolSqlGetMaxId(pC->uc.pCursor, p2, - (uint64_t *) &pOut->u.i); +case OP_NextSequenceId: { + pOut = &aMem[pOp->p2]; + tarantoolSqlNextSeqId((uint64_t *) &pOut->u.i); pOut->u.i += 1; pOut->flags = MEM_Int; @@ -4495,6 +4483,85 @@ case OP_IdxInsert: { /* in2 */ break; } +/* Opcode: SInsert P1 P2 * * P5 + * Synopsis: space id = P1, key = r[P2] + * + * This opcode is used only during DDL routine. + * 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 >= 0); + + pIn2 = &aMem[pOp->p2]; + struct space *space = space_by_id(pOp->p1); + assert(space != NULL); + assert(space_is_system(space)); + /* Create surrogate cursor to pass to SQL bindings. */ + BtCursor surrogate_cur; + surrogate_cur.space = space; + surrogate_cur.key = pIn2->z; + surrogate_cur.nKey = pIn2->n; + surrogate_cur.curFlags = BTCF_TaCursor; + rc = 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 = P1, key = r[P2] + * + * This opcode is used only during DDL routine. + * 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 >= 0); + + pIn2 = &aMem[pOp->p2]; + struct space *space = space_by_id(pOp->p1); + assert(space != NULL); + assert(space_is_system(space)); + rc = 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 = P1, space[out] = 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 DLL routine, since schema may + * change and pointers become expired. + */ +case OP_SIDtoPtr: { + assert(pOp->p1 > 0); + assert(pOp->p2 >= 0); + + pIn2 = out2Prerelease(p, pOp); + struct space *space = space_by_id(pOp->p1); + assert(space != NULL); + pIn2->u.p = (void *) space; + pIn2->flags = MEM_Ptr; + break; +} + /* Opcode: IdxDelete P1 P2 P3 * * * Synopsis: key=r[P2@P3] * @@ -5424,22 +5491,19 @@ case OP_Init: { /* jump */ /* Opcode: IncMaxid P1 * * * * * - * The cursor (P1) should be open on _schema. - * Increment the max_id (max space id) and store updated tuple in the - * cursor. + * Increment the max_id from _schema (max space id) + * and store updated id in register specified by first operand. + * It is system opcode and must be used only during DDL routine. */ case OP_IncMaxid: { - VdbeCursor *pC; - - assert(pOp->p1>=0 && pOp->p1nCursor); - pC = p->apCsr[pOp->p1]; - assert(pC != 0); + assert(pOp->p1 > 0); + pOut = &aMem[pOp->p1]; - rc = tarantoolSqlite3IncrementMaxid(pC->uc.pCursor); + rc = tarantoolSqlite3IncrementMaxid((uint64_t*) &pOut->u.i); if (rc!=SQLITE_OK) { goto abort_due_to_error; } - pC->nullRow = 0; + pOut->flags = MEM_Int; break; } -- 2.15.1