* [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime
@ 2018-03-20 23:48 Nikita Pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Nikita Pettik @ 2018-03-20 23:48 UTC (permalink / raw)
To: tarantool-patches; +Cc: Nikita Pettik
Branch: https://github.com/tarantool/tarantool/tree/np/gh-3252-slit-dml-and-ddl-sql
Issue: https://github.com/tarantool/tarantool/issues/3252
In order to avoid additional lookups during VDBE execution,
it was suggested to pass pointer fetched at query compilation time to
opcodes which create cursors (i.e. OP_OpenRead and OP_OpenWrite).
However, any DDL routine may invalidate pointers. Thus, to operate on
system spaces (which are used during DDL) new opcodes have been introduced:
OP_SInsert and OP_SDelete. But originally, SQL DDL under hood used nested
parsing to generate code for non-trivial deletion. For instance:
"DELETE FROM _INDEX WHERE id = space_id AND iid > 0" -- to remove all
secondary indexes. To use new operands such nested parsing has been
substituted with hardcoded VDBE programs.
Finally, fresh pointer to space can also be obtained at VDBE rutime using
new opcode OP_SIDtoPtr, which converts space id to ptr and saves it to
given register. This opcode should be used when it is impossible to avoid
mixing DDL and DML routine in the same query
(e.g. CREATE TABLE AS SELECT FROM ...).
Nikita Pettik (4):
sql: pass space pointer to OP_OpenRead/OpenWrite
sql: introduce opcodes to operate on system spaces
sql: rework code generation for DDL routine
sql: rework OP_OpenWrite/OpenRead
src/box/sql.c | 51 ++++---
src/box/sql/analyze.c | 17 ++-
src/box/sql/build.c | 321 ++++++++++++++++++++++---------------------
src/box/sql/expr.c | 14 +-
src/box/sql/fkey.c | 11 +-
src/box/sql/insert.c | 37 ++++-
src/box/sql/opcodes.c | 51 +++----
src/box/sql/opcodes.h | 59 ++++----
src/box/sql/select.c | 11 +-
src/box/sql/sqliteInt.h | 5 +
src/box/sql/tarantoolInt.h | 11 +-
src/box/sql/trigger.c | 27 ++--
src/box/sql/vdbe.c | 310 +++++++++++++++++++++--------------------
src/box/sql/vdbe.h | 1 +
src/box/sql/vdbeInt.h | 1 +
src/box/sql/vdbeaux.c | 13 ++
src/box/sql/where.c | 12 +-
test/sql/transition.result | 11 +-
test/sql/transition.test.lua | 8 +-
19 files changed, 550 insertions(+), 421 deletions(-)
--
2.15.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik @ 2018-03-20 23:48 ` Nikita Pettik 2018-03-21 13:14 ` [tarantool-patches] " Kirill Yukhin 2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Nikita Pettik @ 2018-03-20 23:48 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik Originally in SQLite, to open table (i.e. btree) it was required to pass number of page root to OP_OpenRead or OP_OpenWrite opcodes as an argument. However, now there are only Tarantool spaces and nothing prevents from operating directly on pointers to them. On the other hand, pointers are able to expire after schema changes (i.e. after DML routine). For this reason, schema version is saved to VDBE at compile time and checked each time during cursor opening. Part of #3252 --- src/box/sql/analyze.c | 17 +++++++++++++++-- src/box/sql/build.c | 7 ++++++- src/box/sql/expr.c | 14 ++++++++++++-- src/box/sql/fkey.c | 11 ++++++++++- src/box/sql/insert.c | 37 ++++++++++++++++++++++++++++++++----- src/box/sql/select.c | 11 ++++++++++- src/box/sql/vdbe.c | 12 ++++++++++-- src/box/sql/vdbe.h | 1 + src/box/sql/vdbeInt.h | 1 + src/box/sql/vdbeaux.c | 13 +++++++++++++ src/box/sql/where.c | 12 +++++++++++- 11 files changed, 121 insertions(+), 15 deletions(-) diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index db06d0182..57fc33c70 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -174,10 +174,16 @@ openStatTable(Parse * pParse, /* Parsing context */ /* Open the sql_stat[134] tables for writing. */ for (i = 0; aTable[i]; i++) { + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i])); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, + ((int64_t) space)); int addr; addr = sqlite3VdbeAddOp3(v, OP_OpenWrite, iStatCur + i, aRoot[i], - 0); + space_ptr_reg); v->aOp[addr].p4.pKeyInfo = 0; v->aOp[addr].p4type = P4_KEYINFO; sqlite3VdbeChangeP5(v, aCreateTbl[i]); @@ -814,6 +820,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */ int iTabCur; /* Table cursor */ Vdbe *v; /* The virtual machine being built up */ int i; /* Loop counter */ + int space_ptr_reg = iMem++; int regStat4 = iMem++; /* Register to hold Stat4Accum object */ int regChng = iMem++; /* Index of changed index field */ int regKey = iMem++; /* Key argument passed to stat_push() */ @@ -910,7 +917,13 @@ analyzeOneTable(Parse * pParse, /* Parser context */ /* Open a read-only cursor on the index being analyzed. */ assert(sqlite3SchemaToIndex(db, pIdx->pSchema) == 0); - sqlite3VdbeAddOp2(v, OP_OpenRead, iIdxCur, pIdx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); + assert(space != NULL); + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, + ((int64_t) space)); + sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); VdbeComment((v, "%s", pIdx->zName)); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 9ad0c0605..229c8b4d5 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2603,7 +2603,12 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); - sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, 0, + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, + ((int64_t) space)); + sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, (char *)pKey, P4_KEYINFO); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR | ((memRootPage >= 0) ? diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index b69a176cb..2a553925c 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -35,7 +35,9 @@ */ #include <box/coll.h> #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" /* Forward declarations */ static void exprCodeBetween(Parse *, Expr *, int, @@ -2586,8 +2588,16 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */ pIdx->zName), P4_DYNAMIC); #endif - sqlite3VdbeAddOp2(v, OP_OpenRead, iTab, - pIdx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, + space_ptr_reg, 0, + ((int64_t) space)); + sqlite3VdbeAddOp3(v, OP_OpenRead, iTab, + pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); VdbeComment((v, "%s", pIdx->zName)); assert(IN_INDEX_INDEX_DESC == diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 439f38352..f4d73b289 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -35,7 +35,9 @@ */ #include <box/coll.h> #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" #ifndef SQLITE_OMIT_FOREIGN_KEY #ifndef SQLITE_OMIT_TRIGGER @@ -434,7 +436,14 @@ fkLookupParent(Parse * pParse, /* Parse context */ int regTemp = sqlite3GetTempRange(pParse, nCol); int regRec = sqlite3GetTempReg(pParse); - sqlite3VdbeAddOp2(v, OP_OpenRead, iCur, pIdx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, + ((int64_t) space)); + sqlite3VdbeAddOp3(v, OP_OpenRead, iCur, pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); for (i = 0; i < nCol; i++) { sqlite3VdbeAddOp2(v, OP_Copy, diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 54fcca5c9..c57466868 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -34,7 +34,9 @@ * to handle INSERT statements in SQLite. */ #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" /* * Generate code that will open pTab as cursor iCur. @@ -51,7 +53,12 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */ Index *pPk = sqlite3PrimaryKeyIndex(pTab); assert(pPk != 0); assert(pPk->tnum == pTab->tnum); - sqlite3VdbeAddOp3(v, opcode, iCur, pPk->tnum, 0); + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, + ((int64_t) space)); + sqlite3VdbeAddOp3(v, opcode, iCur, pPk->tnum, space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pPk); VdbeComment((v, "%s", pTab->zName)); } @@ -183,7 +190,7 @@ readsTable(Parse * p, Table * pTab) for (i = 1; i < iEnd; i++) { VdbeOp *pOp = sqlite3VdbeGetOp(v, i); assert(pOp != 0); - if (pOp->opcode == OP_OpenRead && pOp->p3 == 0) { + if (pOp->opcode == OP_OpenRead) { Index *pIndex; int tnum = pOp->p2; if (tnum == pTab->tnum) { @@ -1560,6 +1567,11 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ *piDataCur = iDataCur; if (piIdxCur) *piIdxCur = iBase; + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pTab->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, + ((int64_t) space)); /* One iteration of this cycle adds OpenRead/OpenWrite which * opens cursor for current index. @@ -1607,7 +1619,8 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ p5 = 0; } if (aToOpen == 0 || aToOpen[i + 1]) { - sqlite3VdbeAddOp2(v, op, iIdxCur, pIdx->tnum); + sqlite3VdbeAddOp3(v, op, iIdxCur, pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); sqlite3VdbeChangeP5(v, p5); VdbeComment((v, "%s", pIdx->zName)); @@ -1911,10 +1924,24 @@ xferOptimization(Parse * pParse, /* Parser context */ break; } assert(pSrcIdx); - sqlite3VdbeAddOp2(v, OP_OpenRead, iSrc, pSrcIdx->tnum); + struct space *space_src = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum)); + assert(space_src != NULL); + int space_src_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_src_ptr_reg, 0, + ((int64_t) space_src)); + sqlite3VdbeAddOp3(v, OP_OpenRead, iSrc, pSrcIdx->tnum, + space_src_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx); VdbeComment((v, "%s", pSrcIdx->zName)); - sqlite3VdbeAddOp2(v, OP_OpenWrite, iDest, pDestIdx->tnum); + struct space *space_dest = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum)); + assert(space_dest != NULL); + int space_dest_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_dest_ptr_reg, 0, + ((int64_t) space_dest)); + sqlite3VdbeAddOp3(v, OP_OpenWrite, iDest, pDestIdx->tnum, + space_dest_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR); VdbeComment((v, "%s", pDestIdx->zName)); diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 2a8c83d06..7da3f0e05 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -35,7 +35,9 @@ */ #include <box/coll.h> #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" /* * Trace output macros @@ -6187,8 +6189,15 @@ sqlite3Select(Parse * pParse, /* The parser context */ } /* Open a read-only cursor, execute the OP_Count, close the cursor. */ + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(iRoot)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, + space_ptr_reg, 0, + ((int64_t) space)); sqlite3VdbeAddOp4Int(v, OP_OpenRead, iCsr, - iRoot, 0, 1); + iRoot, space_ptr_reg, 1); if (pKeyInfo) { sqlite3VdbeChangeP4(v, -1, (char *)pKeyInfo, diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9929dfb96..5d1227afa 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3217,9 +3217,17 @@ case OP_OpenWrite: assert(p2 >= 1); pBtCur = pCur->uc.pCursor; - pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); + if (box_schema_version() == p->schema_ver) { + pIn3 = &aMem[pOp->p3]; + /* Make sure that 64-bit pointer can fit into int64_t. */ + assert(sizeof(pBtCur->space) >= sizeof(pIn3->u.i)); + pBtCur->space = ((struct space *) pIn3->u.i); + } else { + pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); + } + assert(pBtCur->space != NULL); pBtCur->index = space_index(pBtCur->space, SQLITE_PAGENO_TO_INDEXID(p2)); - assert(pBtCur->space != NULL && pBtCur->index != NULL); + assert(pBtCur->index != NULL); pBtCur->eState = CURSOR_INVALID; pBtCur->curFlags |= BTCF_TaCursor; pCur->pKeyInfo = pKeyInfo; diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index 7241963e4..a1ecf729d 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -200,6 +200,7 @@ int sqlite3VdbeAddOp3(Vdbe *, int, int, int, int); int sqlite3VdbeAddOp4(Vdbe *, int, int, int, int, const char *zP4, int); int sqlite3VdbeAddOp4Dup8(Vdbe *, int, int, int, int, const u8 *, int); int sqlite3VdbeAddOp4Int(Vdbe *, int, int, int, int, int); +int sqlite3VdbeAddOp4Int64(Vdbe *, int, int, int, int, int64_t); void sqlite3VdbeEndCoroutine(Vdbe *, int); #if defined(SQLITE_DEBUG) && !defined(SQLITE_TEST_REALLOC_STRESS) void sqlite3VdbeVerifyNoMallocRequired(Vdbe * p, int N); diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 8b622de5b..99262ab7b 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -378,6 +378,7 @@ struct Vdbe { i64 nFkConstraint; /* Number of imm. FK constraints this VM */ i64 nStmtDefCons; /* Number of def. constraints when stmt started */ i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ + uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ /* * In recursive triggers we can execute INSERT/UPDATE OR IGNORE diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 92bf9943b..b35d0712e 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -66,6 +66,7 @@ sqlite3VdbeCreate(Parse * pParse) p->magic = VDBE_MAGIC_INIT; p->pParse = pParse; p->autoCommit = (char)box_txn() == 0 ? 1 : 0; + p->schema_ver = box_schema_version(); if (!p->autoCommit) { p->psql_txn = in_txn()->psql_txn; p->nDeferredCons = p->psql_txn->nDeferredConsSave; @@ -413,6 +414,18 @@ sqlite3VdbeAddOp4Int(Vdbe * p, /* Add the opcode to this VM */ return addr; } +int +sqlite3VdbeAddOp4Int64(Vdbe *p, int op, int p1, int p2, int p3, int64_t p4) +{ + int addr = sqlite3VdbeAddOp3(p, op, p1, p2, p3); + VdbeOp *pOp = &p->aOp[addr]; + pOp->p4type = P4_INT64; + pOp->p4.pI64 = sqlite3DbMallocRawNN(sqlite3VdbeDb(p), sizeof(int64_t)); + if (p->db->mallocFailed == 0) + *pOp->p4.pI64 = p4; + return addr; +} + /* Insert the end of a co-routine */ void diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 2f1c627e5..9d0397eef 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -42,6 +42,8 @@ #include "vdbeInt.h" #include "whereInt.h" #include "box/session.h" +#include "box/schema.h" +#include "tarantoolInt.h" /* Forward declaration of methods */ static int whereLoopResize(sqlite3 *, WhereLoop *, int); @@ -4606,7 +4608,15 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */ assert(pIx->pSchema == pTab->pSchema); assert(iIndexCur >= 0); if (op) { - sqlite3VdbeAddOp2(v, op, iIndexCur, pIx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, + space_ptr_reg, 0, + ((int64_t) space)); + sqlite3VdbeAddOp3(v, op, iIndexCur, pIx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIx); if ((pLoop->wsFlags & WHERE_CONSTRAINT) != 0 && (pLoop-> -- 2.15.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite 2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik @ 2018-03-21 13:14 ` Kirill Yukhin 2018-03-22 10:07 ` n.pettik 0 siblings, 1 reply; 19+ messages in thread From: Kirill Yukhin @ 2018-03-21 13:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik Hello, My comments inlined. On 21 мар 02:48, Nikita Pettik wrote: > Originally in SQLite, to open table (i.e. btree) it was required to pass > number of page root to OP_OpenRead or OP_OpenWrite opcodes as an > argument. However, now there are only Tarantool spaces and nothing > prevents from operating directly on pointers to them. On the other hand, > pointers are able to expire after schema changes (i.e. after DML > routine). For this reason, schema version is saved to VDBE at compile > time and checked each time during cursor opening. > > Part of #3252 > --- > src/box/sql/analyze.c | 17 +++++++++++++++-- > src/box/sql/build.c | 7 ++++++- > src/box/sql/expr.c | 14 ++++++++++++-- > src/box/sql/fkey.c | 11 ++++++++++- > src/box/sql/insert.c | 37 ++++++++++++++++++++++++++++++++----- > src/box/sql/select.c | 11 ++++++++++- > src/box/sql/vdbe.c | 12 ++++++++++-- > src/box/sql/vdbe.h | 1 + > src/box/sql/vdbeInt.h | 1 + > src/box/sql/vdbeaux.c | 13 +++++++++++++ > src/box/sql/where.c | 12 +++++++++++- > 11 files changed, 121 insertions(+), 15 deletions(-) > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index db06d0182..57fc33c70 100644 > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 9929dfb96..5d1227afa 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3217,9 +3217,17 @@ case OP_OpenWrite: > > assert(p2 >= 1); > pBtCur = pCur->uc.pCursor; > - pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); > + if (box_schema_version() == p->schema_ver) { > + pIn3 = &aMem[pOp->p3]; > + /* Make sure that 64-bit pointer can fit into int64_t. */ > + assert(sizeof(pBtCur->space) >= sizeof(pIn3->u.i)); I don't like this. If we're going to extensively use pointers space/index then let's extend Memory struct adding dedicated types to the union. Note, that new opcode (say, LoadPtr) will be needed. > + pBtCur->space = ((struct space *) pIn3->u.i); > + } else { > + pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); > + } Don't surround single stmt withcurly braces pls. Also, if schema was changed then error should be returned (stmt expired or smth). > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h > index 7241963e4..a1ecf729d 100644 > --- a/src/box/sql/vdbe.h > +++ b/src/box/sql/vdbe.h > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index 8b622de5b..99262ab7b 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -378,6 +378,7 @@ struct Vdbe { > i64 nFkConstraint; /* Number of imm. FK constraints this VM */ > i64 nStmtDefCons; /* Number of def. constraints when stmt started */ > i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ > + uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ > > /* > * In recursive triggers we can execute INSERT/UPDATE OR IGNORE > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index 92bf9943b..b35d0712e 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -66,6 +66,7 @@ sqlite3VdbeCreate(Parse * pParse) > p->magic = VDBE_MAGIC_INIT; > p->pParse = pParse; > p->autoCommit = (char)box_txn() == 0 ? 1 : 0; > + p->schema_ver = box_schema_version(); > if (!p->autoCommit) { > p->psql_txn = in_txn()->psql_txn; > p->nDeferredCons = p->psql_txn->nDeferredConsSave; > @@ -413,6 +414,18 @@ sqlite3VdbeAddOp4Int(Vdbe * p, /* Add the opcode to this VM */ > return addr; > } > > +int > +sqlite3VdbeAddOp4Int64(Vdbe *p, int op, int p1, int p2, int p3, int64_t p4) > +{ > + int addr = sqlite3VdbeAddOp3(p, op, p1, p2, p3); > + VdbeOp *pOp = &p->aOp[addr]; > + pOp->p4type = P4_INT64; > + pOp->p4.pI64 = sqlite3DbMallocRawNN(sqlite3VdbeDb(p), sizeof(int64_t)); > + if (p->db->mallocFailed == 0) > + *pOp->p4.pI64 = p4; > + return addr; > +} This is useless if LoadPTR will be introduced. -- Thanks, Kirill ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite 2018-03-21 13:14 ` [tarantool-patches] " Kirill Yukhin @ 2018-03-22 10:07 ` n.pettik 2018-03-22 11:04 ` v.shpilevoy 0 siblings, 1 reply; 19+ messages in thread From: n.pettik @ 2018-03-22 10:07 UTC (permalink / raw) To: tarantool-patches, Кирилл Юхин [-- Attachment #1: Type: text/plain, Size: 39923 bytes --] > On 21 Mar 2018, at 16:14, Kirill Yukhin <kyukhin@tarantool.org> wrote: > > Hello, > My comments inlined. > > On 21 мар 02:48, Nikita Pettik wrote: >> Originally in SQLite, to open table (i.e. btree) it was required to pass >> number of page root to OP_OpenRead or OP_OpenWrite opcodes as an >> argument. However, now there are only Tarantool spaces and nothing >> prevents from operating directly on pointers to them. On the other hand, >> pointers are able to expire after schema changes (i.e. after DML >> routine). For this reason, schema version is saved to VDBE at compile >> time and checked each time during cursor opening. >> >> Part of #3252 >> --- >> src/box/sql/analyze.c | 17 +++++++++++++++-- >> src/box/sql/build.c | 7 ++++++- >> src/box/sql/expr.c | 14 ++++++++++++-- >> src/box/sql/fkey.c | 11 ++++++++++- >> src/box/sql/insert.c | 37 ++++++++++++++++++++++++++++++++----- >> src/box/sql/select.c | 11 ++++++++++- >> src/box/sql/vdbe.c | 12 ++++++++++-- >> src/box/sql/vdbe.h | 1 + >> src/box/sql/vdbeInt.h | 1 + >> src/box/sql/vdbeaux.c | 13 +++++++++++++ >> src/box/sql/where.c | 12 +++++++++++- >> 11 files changed, 121 insertions(+), 15 deletions(-) >> >> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c >> index db06d0182..57fc33c70 100644 >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 9929dfb96..5d1227afa 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -3217,9 +3217,17 @@ case OP_OpenWrite: >> >> assert(p2 >= 1); >> pBtCur = pCur->uc.pCursor; >> - pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); >> + if (box_schema_version() == p->schema_ver) { >> + pIn3 = &aMem[pOp->p3]; >> + /* Make sure that 64-bit pointer can fit into int64_t. */ >> + assert(sizeof(pBtCur->space) >= sizeof(pIn3->u.i)); > I don't like this. If we're going to extensively use pointers space/index then > let's extend Memory struct adding dedicated types to the union. > Note, that new opcode (say, LoadPtr) will be needed. Done. > >> + pBtCur->space = ((struct space *) pIn3->u.i); >> + } else { >> + pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); >> + } > Don't surround single stmt withcurly braces pls. This is only only advise (https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces <https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces>): "Do not unnecessarily use braces where a single statement will do." So, I guess, both variants are allowed. > Also, if schema was changed then error should be returned (stmt expired or > smth). I have dedicated separate patch (the last one) for this issue. It can’t be done right here since new DDL processing is also introduced in next patch. In order to make no confusions, I will remove this diff and place it to the last patch. >> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h >> index 7241963e4..a1ecf729d 100644 >> --- a/src/box/sql/vdbe.h >> +++ b/src/box/sql/vdbe.h >> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h >> index 8b622de5b..99262ab7b 100644 >> --- a/src/box/sql/vdbeInt.h >> +++ b/src/box/sql/vdbeInt.h >> @@ -378,6 +378,7 @@ struct Vdbe { >> i64 nFkConstraint; /* Number of imm. FK constraints this VM */ >> i64 nStmtDefCons; /* Number of def. constraints when stmt started */ >> i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ >> + uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ >> >> /* >> * In recursive triggers we can execute INSERT/UPDATE OR IGNORE >> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >> index 92bf9943b..b35d0712e 100644 >> --- a/src/box/sql/vdbeaux.c >> +++ b/src/box/sql/vdbeaux.c >> @@ -66,6 +66,7 @@ sqlite3VdbeCreate(Parse * pParse) >> p->magic = VDBE_MAGIC_INIT; >> p->pParse = pParse; >> p->autoCommit = (char)box_txn() == 0 ? 1 : 0; >> + p->schema_ver = box_schema_version(); >> if (!p->autoCommit) { >> p->psql_txn = in_txn()->psql_txn; >> p->nDeferredCons = p->psql_txn->nDeferredConsSave; >> @@ -413,6 +414,18 @@ sqlite3VdbeAddOp4Int(Vdbe * p, /* Add the opcode to this VM */ >> return addr; >> } >> >> +int >> +sqlite3VdbeAddOp4Int64(Vdbe *p, int op, int p1, int p2, int p3, int64_t p4) >> +{ >> + int addr = sqlite3VdbeAddOp3(p, op, p1, p2, p3); >> + VdbeOp *pOp = &p->aOp[addr]; >> + pOp->p4type = P4_INT64; >> + pOp->p4.pI64 = sqlite3DbMallocRawNN(sqlite3VdbeDb(p), sizeof(int64_t)); >> + if (p->db->mallocFailed == 0) >> + *pOp->p4.pI64 = p4; >> + return addr; >> +} > This is useless if LoadPTR will be introduced. Done. The whole patch is below: ======================================================================= Originally in SQLite, to open table (i.e. btree) it was required to pass number of page root to OP_OpenRead or OP_OpenWrite opcodes as an argument. However, now there are only Tarantool spaces and nothing prevents from operating directly on pointers to them. Thus, to pass pointers from compile time to runtime, opcode OP_LoadPtr has been introduced. It fetches pointer from P4 and stores to the register specified by P2. It is worth mentioning that, pointers are able to expire after schema changes (i.e. after DML routine). For this reason, schema version is saved to VDBE at compile time and checked each time during cursor opening. Part of #3252 --- src/box/sql/analyze.c | 17 +++++- src/box/sql/build.c | 7 ++- src/box/sql/expr.c | 14 ++++- src/box/sql/fkey.c | 11 +++- src/box/sql/insert.c | 36 ++++++++++-- src/box/sql/opcodes.c | 137 +++++++++++++++++++++---------------------- src/box/sql/opcodes.h | 157 +++++++++++++++++++++++++------------------------- src/box/sql/select.c | 11 +++- src/box/sql/vdbe.c | 13 +++++ src/box/sql/vdbe.h | 2 + src/box/sql/vdbeInt.h | 3 + src/box/sql/vdbeaux.c | 11 ++++ src/box/sql/where.c | 12 +++- 13 files changed, 272 insertions(+), 159 deletions(-) diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index db06d0182..d121dd2b9 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -174,10 +174,16 @@ openStatTable(Parse * pParse, /* Parsing context */ /* Open the sql_stat[134] tables for writing. */ for (i = 0; aTable[i]; i++) { + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i])); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, + (void *) space); int addr; addr = sqlite3VdbeAddOp3(v, OP_OpenWrite, iStatCur + i, aRoot[i], - 0); + space_ptr_reg); v->aOp[addr].p4.pKeyInfo = 0; v->aOp[addr].p4type = P4_KEYINFO; sqlite3VdbeChangeP5(v, aCreateTbl[i]); @@ -814,6 +820,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */ int iTabCur; /* Table cursor */ Vdbe *v; /* The virtual machine being built up */ int i; /* Loop counter */ + int space_ptr_reg = iMem++; int regStat4 = iMem++; /* Register to hold Stat4Accum object */ int regChng = iMem++; /* Index of changed index field */ int regKey = iMem++; /* Key argument passed to stat_push() */ @@ -910,7 +917,13 @@ analyzeOneTable(Parse * pParse, /* Parser context */ /* Open a read-only cursor on the index being analyzed. */ assert(sqlite3SchemaToIndex(db, pIdx->pSchema) == 0); - sqlite3VdbeAddOp2(v, OP_OpenRead, iIdxCur, pIdx->tnum); + 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); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); VdbeComment((v, "%s", pIdx->zName)); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 9ad0c0605..9cdfd0b7a 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2603,7 +2603,12 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); - sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, 0, + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, + (void *) space); + sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, (char *)pKey, P4_KEYINFO); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR | ((memRootPage >= 0) ? diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index b69a176cb..009538095 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -35,7 +35,9 @@ */ #include <box/coll.h> #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" /* Forward declarations */ static void exprCodeBetween(Parse *, Expr *, int, @@ -2586,8 +2588,16 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */ pIdx->zName), P4_DYNAMIC); #endif - sqlite3VdbeAddOp2(v, OP_OpenRead, iTab, - pIdx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, + space_ptr_reg, 0, + (void *) space); + sqlite3VdbeAddOp3(v, OP_OpenRead, iTab, + pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); VdbeComment((v, "%s", pIdx->zName)); assert(IN_INDEX_INDEX_DESC == diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 439f38352..77565cb50 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -35,7 +35,9 @@ */ #include <box/coll.h> #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" #ifndef SQLITE_OMIT_FOREIGN_KEY #ifndef SQLITE_OMIT_TRIGGER @@ -434,7 +436,14 @@ fkLookupParent(Parse * pParse, /* Parse context */ int regTemp = sqlite3GetTempRange(pParse, nCol); int regRec = sqlite3GetTempReg(pParse); - sqlite3VdbeAddOp2(v, OP_OpenRead, iCur, pIdx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, + (void *) space); + sqlite3VdbeAddOp3(v, OP_OpenRead, iCur, pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); for (i = 0; i < nCol; i++) { sqlite3VdbeAddOp2(v, OP_Copy, diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 54fcca5c9..4f3e2f316 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -34,7 +34,9 @@ * to handle INSERT statements in SQLite. */ #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" /* * Generate code that will open pTab as cursor iCur. @@ -51,7 +53,12 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */ Index *pPk = sqlite3PrimaryKeyIndex(pTab); assert(pPk != 0); assert(pPk->tnum == pTab->tnum); - sqlite3VdbeAddOp3(v, opcode, iCur, pPk->tnum, 0); + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, + (void *) space); + sqlite3VdbeAddOp3(v, opcode, iCur, pPk->tnum, space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pPk); VdbeComment((v, "%s", pTab->zName)); } @@ -183,7 +190,7 @@ readsTable(Parse * p, Table * pTab) for (i = 1; i < iEnd; i++) { VdbeOp *pOp = sqlite3VdbeGetOp(v, i); assert(pOp != 0); - if (pOp->opcode == OP_OpenRead && pOp->p3 == 0) { + if (pOp->opcode == OP_OpenRead) { Index *pIndex; int tnum = pOp->p2; if (tnum == pTab->tnum) { @@ -1560,6 +1567,10 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ *piDataCur = iDataCur; if (piIdxCur) *piIdxCur = iBase; + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pTab->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void *) space); /* One iteration of this cycle adds OpenRead/OpenWrite which * opens cursor for current index. @@ -1607,7 +1618,8 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */ p5 = 0; } if (aToOpen == 0 || aToOpen[i + 1]) { - sqlite3VdbeAddOp2(v, op, iIdxCur, pIdx->tnum); + sqlite3VdbeAddOp3(v, op, iIdxCur, pIdx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIdx); sqlite3VdbeChangeP5(v, p5); VdbeComment((v, "%s", pIdx->zName)); @@ -1911,10 +1923,24 @@ xferOptimization(Parse * pParse, /* Parser context */ break; } assert(pSrcIdx); - sqlite3VdbeAddOp2(v, OP_OpenRead, iSrc, pSrcIdx->tnum); + struct space *space_src = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum)); + assert(space_src != NULL); + int space_src_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_src_ptr_reg, 0, + (void *) space_src); + sqlite3VdbeAddOp3(v, OP_OpenRead, iSrc, pSrcIdx->tnum, + space_src_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx); VdbeComment((v, "%s", pSrcIdx->zName)); - sqlite3VdbeAddOp2(v, OP_OpenWrite, iDest, pDestIdx->tnum); + struct space *space_dest = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum)); + assert(space_dest != NULL); + int space_dest_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_dest_ptr_reg, 0, + (void *) space_dest); + sqlite3VdbeAddOp3(v, OP_OpenWrite, iDest, pDestIdx->tnum, + space_dest_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx); sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR); VdbeComment((v, "%s", pDestIdx->zName)); diff --git a/src/box/sql/opcodes.c b/src/box/sql/opcodes.c index 7a40b28a8..b108d5f9e 100644 --- a/src/box/sql/opcodes.c +++ b/src/box/sql/opcodes.c @@ -78,76 +78,77 @@ const char *sqlite3OpcodeName(int i){ /* 64 */ "Integer" OpHelp("r[P2]=P1"), /* 65 */ "Bool" OpHelp("r[P2]=P1"), /* 66 */ "Int64" OpHelp("r[P2]=P4"), - /* 67 */ "String" OpHelp("r[P2]='P4' (len=P1)"), - /* 68 */ "Null" OpHelp("r[P2..P3]=NULL"), - /* 69 */ "SoftNull" OpHelp("r[P1]=NULL"), - /* 70 */ "Blob" OpHelp("r[P2]=P4 (len=P1, subtype=P3)"), - /* 71 */ "Variable" OpHelp("r[P2]=parameter(P1,P4)"), - /* 72 */ "Move" OpHelp("r[P2@P3]=r[P1@P3]"), - /* 73 */ "Copy" OpHelp("r[P2@P3+1]=r[P1@P3+1]"), - /* 74 */ "SCopy" OpHelp("r[P2]=r[P1]"), + /* 67 */ "LoadPtr" OpHelp("r[P2] = P4"), + /* 68 */ "String" OpHelp("r[P2]='P4' (len=P1)"), + /* 69 */ "Null" OpHelp("r[P2..P3]=NULL"), + /* 70 */ "SoftNull" OpHelp("r[P1]=NULL"), + /* 71 */ "Blob" OpHelp("r[P2]=P4 (len=P1, subtype=P3)"), + /* 72 */ "Variable" OpHelp("r[P2]=parameter(P1,P4)"), + /* 73 */ "Move" OpHelp("r[P2@P3]=r[P1@P3]"), + /* 74 */ "Copy" OpHelp("r[P2@P3+1]=r[P1@P3+1]"), /* 75 */ "String8" OpHelp("r[P2]='P4'"), - /* 76 */ "IntCopy" OpHelp("r[P2]=r[P1]"), - /* 77 */ "ResultRow" OpHelp("output=r[P1@P2]"), - /* 78 */ "CollSeq" OpHelp(""), - /* 79 */ "Function0" OpHelp("r[P3]=func(r[P2@P5])"), - /* 80 */ "Function" OpHelp("r[P3]=func(r[P2@P5])"), - /* 81 */ "AddImm" OpHelp("r[P1]=r[P1]+P2"), - /* 82 */ "RealAffinity" OpHelp(""), - /* 83 */ "Cast" OpHelp("affinity(r[P1])"), - /* 84 */ "Permutation" OpHelp(""), - /* 85 */ "Compare" OpHelp("r[P1@P3] <-> r[P2@P3]"), - /* 86 */ "Column" OpHelp("r[P3]=PX"), - /* 87 */ "Affinity" OpHelp("affinity(r[P1@P2])"), - /* 88 */ "MakeRecord" OpHelp("r[P3]=mkrec(r[P1@P2])"), - /* 89 */ "Count" OpHelp("r[P2]=count()"), - /* 90 */ "FkCheckCommit" OpHelp(""), - /* 91 */ "TTransaction" OpHelp(""), - /* 92 */ "ReadCookie" OpHelp(""), - /* 93 */ "SetCookie" OpHelp(""), - /* 94 */ "ReopenIdx" OpHelp("root=P2"), - /* 95 */ "OpenRead" OpHelp("root=P2"), - /* 96 */ "OpenWrite" OpHelp("root=P2"), - /* 97 */ "OpenTEphemeral" OpHelp("nColumn = P2"), - /* 98 */ "SorterOpen" OpHelp(""), - /* 99 */ "SequenceTest" OpHelp("if (cursor[P1].ctr++) pc = P2"), - /* 100 */ "OpenPseudo" OpHelp("P3 columns in r[P2]"), - /* 101 */ "Close" OpHelp(""), - /* 102 */ "ColumnsUsed" OpHelp(""), - /* 103 */ "Sequence" OpHelp("r[P2]=cursor[P1].ctr++"), - /* 104 */ "NextId" OpHelp("r[P3]=get_max(space_index[P1]{Column[P2]})"), - /* 105 */ "NextIdEphemeral" OpHelp("r[P3]=get_max(space_index[P1]{Column[P2]})"), - /* 106 */ "FCopy" OpHelp("reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)]"), - /* 107 */ "Delete" OpHelp(""), - /* 108 */ "ResetCount" OpHelp(""), - /* 109 */ "SorterCompare" OpHelp("if key(P1)!=trim(r[P3],P4) goto P2"), - /* 110 */ "SorterData" OpHelp("r[P2]=data"), - /* 111 */ "RowData" OpHelp("r[P2]=data"), - /* 112 */ "NullRow" OpHelp(""), - /* 113 */ "SorterInsert" OpHelp("key=r[P2]"), - /* 114 */ "IdxReplace" OpHelp("key=r[P2]"), + /* 76 */ "SCopy" OpHelp("r[P2]=r[P1]"), + /* 77 */ "IntCopy" OpHelp("r[P2]=r[P1]"), + /* 78 */ "ResultRow" OpHelp("output=r[P1@P2]"), + /* 79 */ "CollSeq" OpHelp(""), + /* 80 */ "Function0" OpHelp("r[P3]=func(r[P2@P5])"), + /* 81 */ "Function" OpHelp("r[P3]=func(r[P2@P5])"), + /* 82 */ "AddImm" OpHelp("r[P1]=r[P1]+P2"), + /* 83 */ "RealAffinity" OpHelp(""), + /* 84 */ "Cast" OpHelp("affinity(r[P1])"), + /* 85 */ "Permutation" OpHelp(""), + /* 86 */ "Compare" OpHelp("r[P1@P3] <-> r[P2@P3]"), + /* 87 */ "Column" OpHelp("r[P3]=PX"), + /* 88 */ "Affinity" OpHelp("affinity(r[P1@P2])"), + /* 89 */ "MakeRecord" OpHelp("r[P3]=mkrec(r[P1@P2])"), + /* 90 */ "Count" OpHelp("r[P2]=count()"), + /* 91 */ "FkCheckCommit" OpHelp(""), + /* 92 */ "TTransaction" OpHelp(""), + /* 93 */ "ReadCookie" OpHelp(""), + /* 94 */ "SetCookie" OpHelp(""), + /* 95 */ "ReopenIdx" OpHelp("root=P2"), + /* 96 */ "OpenRead" OpHelp("root=P2"), + /* 97 */ "OpenWrite" OpHelp("root=P2"), + /* 98 */ "OpenTEphemeral" OpHelp("nColumn = P2"), + /* 99 */ "SorterOpen" OpHelp(""), + /* 100 */ "SequenceTest" OpHelp("if (cursor[P1].ctr++) pc = P2"), + /* 101 */ "OpenPseudo" OpHelp("P3 columns in r[P2]"), + /* 102 */ "Close" OpHelp(""), + /* 103 */ "ColumnsUsed" OpHelp(""), + /* 104 */ "Sequence" OpHelp("r[P2]=cursor[P1].ctr++"), + /* 105 */ "NextId" OpHelp("r[P3]=get_max(space_index[P1]{Column[P2]})"), + /* 106 */ "NextIdEphemeral" OpHelp("r[P3]=get_max(space_index[P1]{Column[P2]})"), + /* 107 */ "FCopy" OpHelp("reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)]"), + /* 108 */ "Delete" OpHelp(""), + /* 109 */ "ResetCount" OpHelp(""), + /* 110 */ "SorterCompare" OpHelp("if key(P1)!=trim(r[P3],P4) goto P2"), + /* 111 */ "SorterData" OpHelp("r[P2]=data"), + /* 112 */ "RowData" OpHelp("r[P2]=data"), + /* 113 */ "NullRow" OpHelp(""), + /* 114 */ "SorterInsert" OpHelp("key=r[P2]"), /* 115 */ "Real" OpHelp("r[P2]=P4"), - /* 116 */ "IdxInsert" OpHelp("key=r[P2]"), - /* 117 */ "IdxDelete" OpHelp("key=r[P2@P3]"), - /* 118 */ "Clear" OpHelp("space id = P1"), - /* 119 */ "ResetSorter" OpHelp(""), - /* 120 */ "ParseSchema2" OpHelp("rows=r[P1@P2]"), - /* 121 */ "ParseSchema3" OpHelp("name=r[P1] sql=r[P1+1]"), - /* 122 */ "RenameTable" OpHelp("P1 = root, P4 = name"), - /* 123 */ "LoadAnalysis" OpHelp(""), - /* 124 */ "DropTable" OpHelp(""), - /* 125 */ "DropIndex" OpHelp(""), - /* 126 */ "DropTrigger" OpHelp(""), - /* 127 */ "Param" OpHelp(""), - /* 128 */ "FkCounter" OpHelp("fkctr[P1]+=P2"), - /* 129 */ "OffsetLimit" OpHelp("if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1)"), - /* 130 */ "AggStep0" OpHelp("accum=r[P3] step(r[P2@P5])"), - /* 131 */ "AggStep" OpHelp("accum=r[P3] step(r[P2@P5])"), - /* 132 */ "AggFinal" OpHelp("accum=r[P1] N=P2"), - /* 133 */ "Expire" OpHelp(""), - /* 134 */ "IncMaxid" OpHelp(""), - /* 135 */ "Noop" OpHelp(""), - /* 136 */ "Explain" OpHelp(""), + /* 116 */ "IdxReplace" OpHelp("key=r[P2]"), + /* 117 */ "IdxInsert" OpHelp("key=r[P2]"), + /* 118 */ "IdxDelete" OpHelp("key=r[P2@P3]"), + /* 119 */ "Clear" OpHelp("space id = P1"), + /* 120 */ "ResetSorter" OpHelp(""), + /* 121 */ "ParseSchema2" OpHelp("rows=r[P1@P2]"), + /* 122 */ "ParseSchema3" OpHelp("name=r[P1] sql=r[P1+1]"), + /* 123 */ "RenameTable" OpHelp("P1 = root, P4 = name"), + /* 124 */ "LoadAnalysis" OpHelp(""), + /* 125 */ "DropTable" OpHelp(""), + /* 126 */ "DropIndex" OpHelp(""), + /* 127 */ "DropTrigger" OpHelp(""), + /* 128 */ "Param" OpHelp(""), + /* 129 */ "FkCounter" OpHelp("fkctr[P1]+=P2"), + /* 130 */ "OffsetLimit" OpHelp("if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1)"), + /* 131 */ "AggStep0" OpHelp("accum=r[P3] step(r[P2@P5])"), + /* 132 */ "AggStep" OpHelp("accum=r[P3] step(r[P2@P5])"), + /* 133 */ "AggFinal" OpHelp("accum=r[P1] N=P2"), + /* 134 */ "Expire" OpHelp(""), + /* 135 */ "IncMaxid" OpHelp(""), + /* 136 */ "Noop" OpHelp(""), + /* 137 */ "Explain" OpHelp(""), }; return azName[i]; } diff --git a/src/box/sql/opcodes.h b/src/box/sql/opcodes.h index af2ba1963..7b62f6d80 100644 --- a/src/box/sql/opcodes.h +++ b/src/box/sql/opcodes.h @@ -67,76 +67,77 @@ #define OP_Integer 64 /* synopsis: r[P2]=P1 */ #define OP_Bool 65 /* synopsis: r[P2]=P1 */ #define OP_Int64 66 /* synopsis: r[P2]=P4 */ -#define OP_String 67 /* synopsis: r[P2]='P4' (len=P1) */ -#define OP_Null 68 /* synopsis: r[P2..P3]=NULL */ -#define OP_SoftNull 69 /* synopsis: r[P1]=NULL */ -#define OP_Blob 70 /* synopsis: r[P2]=P4 (len=P1, subtype=P3) */ -#define OP_Variable 71 /* synopsis: r[P2]=parameter(P1,P4) */ -#define OP_Move 72 /* synopsis: r[P2@P3]=r[P1@P3] */ -#define OP_Copy 73 /* synopsis: r[P2@P3+1]=r[P1@P3+1] */ -#define OP_SCopy 74 /* synopsis: r[P2]=r[P1] */ +#define OP_LoadPtr 67 /* synopsis: r[P2] = P4 */ +#define OP_String 68 /* synopsis: r[P2]='P4' (len=P1) */ +#define OP_Null 69 /* synopsis: r[P2..P3]=NULL */ +#define OP_SoftNull 70 /* synopsis: r[P1]=NULL */ +#define OP_Blob 71 /* synopsis: r[P2]=P4 (len=P1, subtype=P3) */ +#define OP_Variable 72 /* synopsis: r[P2]=parameter(P1,P4) */ +#define OP_Move 73 /* synopsis: r[P2@P3]=r[P1@P3] */ +#define OP_Copy 74 /* synopsis: r[P2@P3+1]=r[P1@P3+1] */ #define OP_String8 75 /* same as TK_STRING, synopsis: r[P2]='P4' */ -#define OP_IntCopy 76 /* synopsis: r[P2]=r[P1] */ -#define OP_ResultRow 77 /* synopsis: output=r[P1@P2] */ -#define OP_CollSeq 78 -#define OP_Function0 79 /* synopsis: r[P3]=func(r[P2@P5]) */ -#define OP_Function 80 /* synopsis: r[P3]=func(r[P2@P5]) */ -#define OP_AddImm 81 /* synopsis: r[P1]=r[P1]+P2 */ -#define OP_RealAffinity 82 -#define OP_Cast 83 /* synopsis: affinity(r[P1]) */ -#define OP_Permutation 84 -#define OP_Compare 85 /* synopsis: r[P1@P3] <-> r[P2@P3] */ -#define OP_Column 86 /* synopsis: r[P3]=PX */ -#define OP_Affinity 87 /* synopsis: affinity(r[P1@P2]) */ -#define OP_MakeRecord 88 /* synopsis: r[P3]=mkrec(r[P1@P2]) */ -#define OP_Count 89 /* synopsis: r[P2]=count() */ -#define OP_FkCheckCommit 90 -#define OP_TTransaction 91 -#define OP_ReadCookie 92 -#define OP_SetCookie 93 -#define OP_ReopenIdx 94 /* synopsis: root=P2 */ -#define OP_OpenRead 95 /* synopsis: root=P2 */ -#define OP_OpenWrite 96 /* synopsis: root=P2 */ -#define OP_OpenTEphemeral 97 /* synopsis: nColumn = P2 */ -#define OP_SorterOpen 98 -#define OP_SequenceTest 99 /* synopsis: if (cursor[P1].ctr++) pc = P2 */ -#define OP_OpenPseudo 100 /* synopsis: P3 columns in r[P2] */ -#define OP_Close 101 -#define OP_ColumnsUsed 102 -#define OP_Sequence 103 /* synopsis: r[P2]=cursor[P1].ctr++ */ -#define OP_NextId 104 /* synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) */ -#define OP_NextIdEphemeral 105 /* synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) */ -#define OP_FCopy 106 /* synopsis: reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)] */ -#define OP_Delete 107 -#define OP_ResetCount 108 -#define OP_SorterCompare 109 /* synopsis: if key(P1)!=trim(r[P3],P4) goto P2 */ -#define OP_SorterData 110 /* synopsis: r[P2]=data */ -#define OP_RowData 111 /* synopsis: r[P2]=data */ -#define OP_NullRow 112 -#define OP_SorterInsert 113 /* synopsis: key=r[P2] */ -#define OP_IdxReplace 114 /* synopsis: key=r[P2] */ +#define OP_SCopy 76 /* synopsis: r[P2]=r[P1] */ +#define OP_IntCopy 77 /* synopsis: r[P2]=r[P1] */ +#define OP_ResultRow 78 /* synopsis: output=r[P1@P2] */ +#define OP_CollSeq 79 +#define OP_Function0 80 /* synopsis: r[P3]=func(r[P2@P5]) */ +#define OP_Function 81 /* synopsis: r[P3]=func(r[P2@P5]) */ +#define OP_AddImm 82 /* synopsis: r[P1]=r[P1]+P2 */ +#define OP_RealAffinity 83 +#define OP_Cast 84 /* synopsis: affinity(r[P1]) */ +#define OP_Permutation 85 +#define OP_Compare 86 /* synopsis: r[P1@P3] <-> r[P2@P3] */ +#define OP_Column 87 /* synopsis: r[P3]=PX */ +#define OP_Affinity 88 /* synopsis: affinity(r[P1@P2]) */ +#define OP_MakeRecord 89 /* synopsis: r[P3]=mkrec(r[P1@P2]) */ +#define OP_Count 90 /* synopsis: r[P2]=count() */ +#define OP_FkCheckCommit 91 +#define OP_TTransaction 92 +#define OP_ReadCookie 93 +#define OP_SetCookie 94 +#define OP_ReopenIdx 95 /* synopsis: root=P2 */ +#define OP_OpenRead 96 /* synopsis: root=P2 */ +#define OP_OpenWrite 97 /* synopsis: root=P2 */ +#define OP_OpenTEphemeral 98 /* synopsis: nColumn = P2 */ +#define OP_SorterOpen 99 +#define OP_SequenceTest 100 /* synopsis: if (cursor[P1].ctr++) pc = P2 */ +#define OP_OpenPseudo 101 /* synopsis: P3 columns in r[P2] */ +#define OP_Close 102 +#define OP_ColumnsUsed 103 +#define OP_Sequence 104 /* synopsis: r[P2]=cursor[P1].ctr++ */ +#define OP_NextId 105 /* synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) */ +#define OP_NextIdEphemeral 106 /* synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) */ +#define OP_FCopy 107 /* synopsis: reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)] */ +#define OP_Delete 108 +#define OP_ResetCount 109 +#define OP_SorterCompare 110 /* synopsis: if key(P1)!=trim(r[P3],P4) goto P2 */ +#define OP_SorterData 111 /* synopsis: r[P2]=data */ +#define OP_RowData 112 /* synopsis: r[P2]=data */ +#define OP_NullRow 113 +#define OP_SorterInsert 114 /* synopsis: key=r[P2] */ #define OP_Real 115 /* same as TK_FLOAT, synopsis: r[P2]=P4 */ -#define OP_IdxInsert 116 /* synopsis: key=r[P2] */ -#define OP_IdxDelete 117 /* synopsis: key=r[P2@P3] */ -#define OP_Clear 118 /* synopsis: space id = P1 */ -#define OP_ResetSorter 119 -#define OP_ParseSchema2 120 /* synopsis: rows=r[P1@P2] */ -#define OP_ParseSchema3 121 /* synopsis: name=r[P1] sql=r[P1+1] */ -#define OP_RenameTable 122 /* synopsis: P1 = root, P4 = name */ -#define OP_LoadAnalysis 123 -#define OP_DropTable 124 -#define OP_DropIndex 125 -#define OP_DropTrigger 126 -#define OP_Param 127 -#define OP_FkCounter 128 /* synopsis: fkctr[P1]+=P2 */ -#define OP_OffsetLimit 129 /* synopsis: if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1) */ -#define OP_AggStep0 130 /* synopsis: accum=r[P3] step(r[P2@P5]) */ -#define OP_AggStep 131 /* synopsis: accum=r[P3] step(r[P2@P5]) */ -#define OP_AggFinal 132 /* synopsis: accum=r[P1] N=P2 */ -#define OP_Expire 133 -#define OP_IncMaxid 134 -#define OP_Noop 135 -#define OP_Explain 136 +#define OP_IdxReplace 116 /* synopsis: key=r[P2] */ +#define OP_IdxInsert 117 /* synopsis: key=r[P2] */ +#define OP_IdxDelete 118 /* synopsis: key=r[P2@P3] */ +#define OP_Clear 119 /* synopsis: space id = P1 */ +#define OP_ResetSorter 120 +#define OP_ParseSchema2 121 /* synopsis: rows=r[P1@P2] */ +#define OP_ParseSchema3 122 /* synopsis: name=r[P1] sql=r[P1+1] */ +#define OP_RenameTable 123 /* synopsis: P1 = root, P4 = name */ +#define OP_LoadAnalysis 124 +#define OP_DropTable 125 +#define OP_DropIndex 126 +#define OP_DropTrigger 127 +#define OP_Param 128 +#define OP_FkCounter 129 /* synopsis: fkctr[P1]+=P2 */ +#define OP_OffsetLimit 130 /* synopsis: if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1) */ +#define OP_AggStep0 131 /* synopsis: accum=r[P3] step(r[P2@P5]) */ +#define OP_AggStep 132 /* synopsis: accum=r[P3] step(r[P2@P5]) */ +#define OP_AggFinal 133 /* synopsis: accum=r[P1] N=P2 */ +#define OP_Expire 134 +#define OP_IncMaxid 135 +#define OP_Noop 136 +#define OP_Explain 137 /* Properties such as "out2" or "jump" that are specified in ** comments following the "case" for each opcode in the vdbe.c @@ -157,16 +158,16 @@ /* 40 */ 0x09, 0x09, 0x09, 0x09, 0x09, 0x09, 0x01, 0x01,\ /* 48 */ 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,\ /* 56 */ 0x03, 0x03, 0x03, 0x01, 0x02, 0x02, 0x08, 0x00,\ -/* 64 */ 0x10, 0x10, 0x10, 0x10, 0x10, 0x00, 0x10, 0x10,\ -/* 72 */ 0x00, 0x00, 0x10, 0x10, 0x10, 0x00, 0x00, 0x00,\ -/* 80 */ 0x00, 0x02, 0x02, 0x02, 0x00, 0x00, 0x00, 0x00,\ -/* 88 */ 0x00, 0x10, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00,\ -/* 96 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,\ -/* 104 */ 0x20, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,\ -/* 112 */ 0x00, 0x04, 0x00, 0x10, 0x04, 0x00, 0x00, 0x00,\ -/* 120 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,\ -/* 128 */ 0x00, 0x1a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,\ -/* 136 */ 0x00,} +/* 64 */ 0x10, 0x10, 0x10, 0x00, 0x10, 0x10, 0x00, 0x10,\ +/* 72 */ 0x10, 0x00, 0x00, 0x10, 0x10, 0x10, 0x00, 0x00,\ +/* 80 */ 0x00, 0x00, 0x02, 0x02, 0x02, 0x00, 0x00, 0x00,\ +/* 88 */ 0x00, 0x00, 0x10, 0x00, 0x00, 0x10, 0x00, 0x00,\ +/* 96 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,\ +/* 104 */ 0x10, 0x20, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00,\ +/* 112 */ 0x00, 0x00, 0x04, 0x10, 0x00, 0x04, 0x00, 0x00,\ +/* 120 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,\ +/* 128 */ 0x10, 0x00, 0x1a, 0x00, 0x00, 0x00, 0x00, 0x00,\ +/* 136 */ 0x00, 0x00,} /* The sqlite3P2Values() routine is able to run faster if it knows ** the value of the largest JUMP opcode. The smaller the maximum diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 2a8c83d06..39c1be53d 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -35,7 +35,9 @@ */ #include <box/coll.h> #include "sqliteInt.h" +#include "tarantoolInt.h" #include "box/session.h" +#include "box/schema.h" /* * Trace output macros @@ -6187,8 +6189,15 @@ sqlite3Select(Parse * pParse, /* The parser context */ } /* Open a read-only cursor, execute the OP_Count, close the cursor. */ + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(iRoot)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, + space_ptr_reg, 0, + (void *) space); sqlite3VdbeAddOp4Int(v, OP_OpenRead, iCsr, - iRoot, 0, 1); + iRoot, space_ptr_reg, 1); if (pKeyInfo) { sqlite3VdbeChangeP4(v, -1, (char *)pKeyInfo, diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9929dfb96..a44a17062 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1072,6 +1072,19 @@ case OP_Int64: { /* out2 */ break; } +/* Opcode: LoadPtr * P2 * P4 * + * Synopsis: r[P2] = P4 + * + * P4 is a generic pointer. Copy it into register P2. + */ +case OP_LoadPtr: { + pOut = out2Prerelease(p, pOp); + assert(pOp->p4type == P4_PTR); + pOut->u.p = pOp->p4.p; + pOut->flags = MEM_Ptr; + break; +} + #ifndef SQLITE_OMIT_FLOATING_POINT /* Opcode: Real * P2 * P4 * * Synopsis: r[P2]=P4 diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index 7241963e4..340ddc766 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -144,6 +144,7 @@ typedef struct VdbeOpList VdbeOpList; #define P4_INDEX (-15) /* P4 is a pointer to a Index structure */ #define P4_FUNCCTX (-16) /* P4 is a pointer to an sqlite3_context object */ #define P4_BOOL (-17) /* P4 is a bool value */ +#define P4_PTR (-18) /* P4 is a generic pointer */ /* Error message codes for OP_Halt */ @@ -200,6 +201,7 @@ int sqlite3VdbeAddOp3(Vdbe *, int, int, int, int); int sqlite3VdbeAddOp4(Vdbe *, int, int, int, int, const char *zP4, int); int sqlite3VdbeAddOp4Dup8(Vdbe *, int, int, int, int, const u8 *, int); int sqlite3VdbeAddOp4Int(Vdbe *, int, int, int, int, int); +int sqlite3VdbeAddOp4Ptr(Vdbe *, int, int, int, int, void *); void sqlite3VdbeEndCoroutine(Vdbe *, int); #if defined(SQLITE_DEBUG) && !defined(SQLITE_TEST_REALLOC_STRESS) void sqlite3VdbeVerifyNoMallocRequired(Vdbe * p, int N); diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 8b622de5b..fcb45c8a8 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -196,6 +196,7 @@ struct Mem { i64 i; /* Integer value used when MEM_Int is set in flags */ bool b; /* Boolean value used when MEM_Bool is set in flags */ int nZero; /* Used when bit MEM_Zero is set in flags */ + void *p; /* Generic pointer */ FuncDef *pDef; /* Used only when flags==MEM_Agg */ VdbeFrame *pFrame; /* Used when flags==MEM_Frame */ } u; @@ -239,6 +240,7 @@ struct Mem { #define MEM_Real 0x0008 /* Value is a real number */ #define MEM_Blob 0x0010 /* Value is a BLOB */ #define MEM_Bool 0x0020 /* Value is a bool */ +#define MEM_Ptr 0x0040 /* Value is a generic pointer */ #define MEM_AffMask 0x003f /* Mask of affinity bits */ #define MEM_Frame 0x0080 /* Value is a VdbeFrame object */ #define MEM_Undefined 0x0100 /* Value is undefined */ @@ -378,6 +380,7 @@ struct Vdbe { i64 nFkConstraint; /* Number of imm. FK constraints this VM */ i64 nStmtDefCons; /* Number of def. constraints when stmt started */ i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ + uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ /* * In recursive triggers we can execute INSERT/UPDATE OR IGNORE diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 92bf9943b..37a3c90d2 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -66,6 +66,7 @@ sqlite3VdbeCreate(Parse * pParse) p->magic = VDBE_MAGIC_INIT; p->pParse = pParse; p->autoCommit = (char)box_txn() == 0 ? 1 : 0; + p->schema_ver = box_schema_version(); if (!p->autoCommit) { p->psql_txn = in_txn()->psql_txn; p->nDeferredCons = p->psql_txn->nDeferredConsSave; @@ -413,6 +414,16 @@ sqlite3VdbeAddOp4Int(Vdbe * p, /* Add the opcode to this VM */ return addr; } +int +sqlite3VdbeAddOp4Ptr(Vdbe *p, int op, int p1, int p2, int p3, void *ptr) +{ + int addr = sqlite3VdbeAddOp3(p, op, p1, p2, p3); + VdbeOp *pOp = &p->aOp[addr]; + pOp->p4type = P4_PTR; + pOp->p4.p = ptr; + return addr; +} + /* Insert the end of a co-routine */ void diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 2f1c627e5..47da3c84c 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -42,6 +42,8 @@ #include "vdbeInt.h" #include "whereInt.h" #include "box/session.h" +#include "box/schema.h" +#include "tarantoolInt.h" /* Forward declaration of methods */ static int whereLoopResize(sqlite3 *, WhereLoop *, int); @@ -4606,7 +4608,15 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */ assert(pIx->pSchema == pTab->pSchema); assert(iIndexCur >= 0); if (op) { - sqlite3VdbeAddOp2(v, op, iIndexCur, pIx->tnum); + struct space *space = + space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum)); + assert(space != NULL); + int space_ptr_reg = ++pParse->nMem; + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, + space_ptr_reg, 0, + (void *) space); + sqlite3VdbeAddOp3(v, op, iIndexCur, pIx->tnum, + space_ptr_reg); sqlite3VdbeSetP4KeyInfo(pParse, pIx); if ((pLoop->wsFlags & WHERE_CONSTRAINT) != 0 && (pLoop-> [-- Attachment #2: Type: text/html, Size: 86774 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite 2018-03-22 10:07 ` n.pettik @ 2018-03-22 11:04 ` v.shpilevoy 2018-03-23 16:01 ` n.pettik 0 siblings, 1 reply; 19+ messages in thread From: v.shpilevoy @ 2018-03-22 11:04 UTC (permalink / raw) To: tarantool-patches Cc: Кирилл Юхин, Nikita Pettik See below 1 comment, and 1 off topic comment. And look at travis - it fails on the branch. > > Originally in SQLite, to open table (i.e. btree) it was required to pass > number of page root to OP_OpenRead or OP_OpenWrite opcodes as an > argument. However, now there are only Tarantool spaces and nothing > prevents from operating directly on pointers to them. Thus, to pass > pointers from compile time to runtime, opcode OP_LoadPtr has been > introduced. It fetches pointer from P4 and stores to the register > specified by P2. > It is worth mentioning that, pointers are able to expire after schema > changes (i.e. after DML routine). For this reason, schema version is > saved to VDBE at compile time and checked each time during cursor > opening. > > Part of #3252 > --- > src/box/sql/analyze.c | 17 +++++- > src/box/sql/build.c | 7 ++- > src/box/sql/expr.c | 14 ++++- > src/box/sql/fkey.c | 11 +++- > src/box/sql/insert.c | 36 ++++++++++-- > src/box/sql/opcodes.c | 137 +++++++++++++++++++++---------------------- > src/box/sql/opcodes.h | 157 +++++++++++++++++++++++++------------------------- > src/box/sql/select.c | 11 +++- > src/box/sql/vdbe.c | 13 +++++ > src/box/sql/vdbe.h | 2 + > src/box/sql/vdbeInt.h | 3 + > src/box/sql/vdbeaux.c | 11 ++++ > src/box/sql/where.c | 12 +++- > 13 files changed, 272 insertions(+), 159 deletions(-) > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index db06d0182..d121dd2b9 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -174,10 +174,16 @@ openStatTable(Parse * pParse, /* Parsing context */ > > /* Open the sql_stat[134] tables for writing. */ > for (i = 0; aTable[i]; i++) { > + struct space *space = > + space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i])); > + assert(space != NULL); > + int space_ptr_reg = ++pParse->nMem; > + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, > + (void *) space); > int addr; > addr = > sqlite3VdbeAddOp3(v, OP_OpenWrite, iStatCur + i, aRoot[i], > - 0); > + space_ptr_reg); 1. Why do you pass here p3, if it is not used in OP_OpenWrite? Why can not you just use sqlite3VdbeAddOp2 here? Same about OpenRead in all the diff below. And how about to wrap this pair of calls sqlite3VdbeAddOp4Ptr(space) + sqlite3VdbeAddOp3(open_read/write) into a separate function? This code is repeated many times, as I can see. > v->aOp[addr].p4.pKeyInfo = 0; > v->aOp[addr].p4type = P4_KEYINFO; > sqlite3VdbeChangeP5(v, aCreateTbl[i]); > @@ -814,6 +820,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */ > int iTabCur; /* Table cursor */ > Vdbe *v; /* The virtual machine being built up */ > int i; /* Loop counter */ > + int space_ptr_reg = iMem++; Off topic: How about create a mempool of Mems? I see, that this object is created really often and in big amount (see src/lib/small/small/mempool.h). (It is not a part of the patch - just think about it, and possibly open a ticket, if it worth). > > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite 2018-03-22 11:04 ` v.shpilevoy @ 2018-03-23 16:01 ` n.pettik 0 siblings, 0 replies; 19+ messages in thread From: n.pettik @ 2018-03-23 16:01 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > On 22 Mar 2018, at 14:04, v.shpilevoy@tarantool.org wrote: > > See below 1 comment, and 1 off topic comment. > > And look at travis - it fails on the branch. It was tricky bug which I didn’t manage to reproduce on macOS. Travis failed on box.cfg{} when initialising SQL subsystem. (Somehow I used sqlite3_free() on memory allocated by ordinary malloc.) > >> >> Originally in SQLite, to open table (i.e. btree) it was required to pass >> number of page root to OP_OpenRead or OP_OpenWrite opcodes as an >> argument. However, now there are only Tarantool spaces and nothing >> prevents from operating directly on pointers to them. Thus, to pass >> pointers from compile time to runtime, opcode OP_LoadPtr has been >> introduced. It fetches pointer from P4 and stores to the register >> specified by P2. >> It is worth mentioning that, pointers are able to expire after schema >> changes (i.e. after DML routine). For this reason, schema version is >> saved to VDBE at compile time and checked each time during cursor >> opening. >> >> Part of #3252 >> --- >> src/box/sql/analyze.c | 17 +++++- >> src/box/sql/build.c | 7 ++- >> src/box/sql/expr.c | 14 ++++- >> src/box/sql/fkey.c | 11 +++- >> src/box/sql/insert.c | 36 ++++++++++-- >> src/box/sql/opcodes.c | 137 +++++++++++++++++++++---------------------- >> src/box/sql/opcodes.h | 157 +++++++++++++++++++++++++------------------------- >> src/box/sql/select.c | 11 +++- >> src/box/sql/vdbe.c | 13 +++++ >> src/box/sql/vdbe.h | 2 + >> src/box/sql/vdbeInt.h | 3 + >> src/box/sql/vdbeaux.c | 11 ++++ >> src/box/sql/where.c | 12 +++- >> 13 files changed, 272 insertions(+), 159 deletions(-) >> >> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c >> index db06d0182..d121dd2b9 100644 >> --- a/src/box/sql/analyze.c >> +++ b/src/box/sql/analyze.c >> @@ -174,10 +174,16 @@ openStatTable(Parse * pParse, /* Parsing context */ >> >> /* Open the sql_stat[134] tables for writing. */ >> for (i = 0; aTable[i]; i++) { >> + struct space *space = >> + space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i])); >> + assert(space != NULL); >> + int space_ptr_reg = ++pParse->nMem; >> + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, >> + (void *) space); >> int addr; >> addr = >> sqlite3VdbeAddOp3(v, OP_OpenWrite, iStatCur + i, aRoot[i], >> - 0); >> + space_ptr_reg); > > 1. Why do you pass here p3, if it is not used in OP_OpenWrite? Why can not you just use sqlite3VdbeAddOp2 here? Same about OpenRead in all the diff below. In this patch I don’t update OP_OpenWrite, since it will be still incomplete. Only after introducing new DDL it will be possible to refactor OP_OpenWrite. > And how about to wrap this pair of calls sqlite3VdbeAddOp4Ptr(space) + sqlite3VdbeAddOp3(open_read/write) into a separate function? This code is repeated many times, as I can see. Done (updated on branch): @@ -1160,6 +1160,30 @@ index_collation_name(Index *idx, uint32_t column) return index->def->key_def->parts[column].coll->name; } +/** + * Create cursor which will be positioned to the space/index. + * It makes space lookup and loads pointer to it into register, + * which is passes to OP_OpenWrite as an argument. + * + * @param parse_context Parse context. + * @param cursor Number of cursor to be created. + * @param entity_id Encoded space and index ids. + * @retval address of last opcode. + */ +int +emit_open_cursor(Parse *parse_context, int cursor, int entity_id) +{ + assert(entity_id > 0); + struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id)); + assert(space != NULL); + Vdbe *vdbe = parse_context->pVdbe; + int space_ptr_reg = ++parse_context->nMem; + sqlite3VdbeAddOp4Ptr(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, + (void *) space); + return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, entity_id, + space_ptr_reg); +} + > >> v->aOp[addr].p4.pKeyInfo = 0; >> v->aOp[addr].p4type = P4_KEYINFO; >> sqlite3VdbeChangeP5(v, aCreateTbl[i]); >> @@ -814,6 +820,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */ >> int iTabCur; /* Table cursor */ >> Vdbe *v; /* The virtual machine being built up */ >> int i; /* Loop counter */ >> + int space_ptr_reg = iMem++; > > Off topic: How about create a mempool of Mems? I see, that this object is created really often and in big amount (see src/lib/small/small/mempool.h). (It is not a part of the patch - just think about it, and possibly open a ticket, if it worth). Memory for ’struct Mem’ objects is allocated at once (at sqlite3VdbeMakeReady()), so I don’t think that there will be any benefit from mempool. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik @ 2018-03-20 23:48 ` Nikita Pettik 2018-03-22 11:42 ` [tarantool-patches] " v.shpilevoy 2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Nikita Pettik @ 2018-03-20 23:48 UTC (permalink / raw) To: tarantool-patches; +Cc: 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 @@ -578,13 +578,30 @@ int tarantoolSqlite3Delete(BtCursor *pCur, u8 flags) if (key == NULL) return SQL_TARANTOOL_DELETE_FAIL; + rc = sql_delete_by_key(pCur->space, key, key_size); + + return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_DELETE_FAIL; +} + +/** + * Delete entry from space by its key. + * + * @param space Space which contains record to be deleted. + * @param key Key of record to be deleted. + * @param key_size Size of key. + * + * @retval SQLITE_OK on success, SQL_TARANTOOL_DELETE_FAIL otherwise. + */ +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); return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_DELETE_FAIL; } @@ -1053,9 +1070,8 @@ out: * Increment max_id and store updated tuple in the cursor * object. */ -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) */ @@ -1075,7 +1091,10 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur) struct tuple *res; int rc; - struct iterator *it = index_create_iterator(pCur->index, ITER_ALL, + struct space *space_schema = space_by_id(BOX_SCHEMA_ID); + assert(space_schema != NULL); + struct index *pk = space_index(space_schema, 0); + struct iterator *it = index_create_iterator(pk, ITER_ALL, &key[1], sizeof(key) - 1); if (iterator_next(it, &res) != 0 || res == NULL) { iterator_delete(it); @@ -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); 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) { 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(space_id, 0 /* PK */, key, key_end, &tuple) != 0) return -1; /* Index is empty */ @@ -1703,5 +1716,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/opcodes.c b/src/box/sql/opcodes.c index 7a40b28a8..5d892094a 100644 --- a/src/box/sql/opcodes.c +++ b/src/box/sql/opcodes.c @@ -115,7 +115,7 @@ const char *sqlite3OpcodeName(int i){ /* 101 */ "Close" OpHelp(""), /* 102 */ "ColumnsUsed" OpHelp(""), /* 103 */ "Sequence" OpHelp("r[P2]=cursor[P1].ctr++"), - /* 104 */ "NextId" OpHelp("r[P3]=get_max(space_index[P1]{Column[P2]})"), + /* 104 */ "NextId" OpHelp("r[P2]=get_max(space_id[P1])"), /* 105 */ "NextIdEphemeral" OpHelp("r[P3]=get_max(space_index[P1]{Column[P2]})"), /* 106 */ "FCopy" OpHelp("reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)]"), /* 107 */ "Delete" OpHelp(""), @@ -128,26 +128,29 @@ const char *sqlite3OpcodeName(int i){ /* 114 */ "IdxReplace" OpHelp("key=r[P2]"), /* 115 */ "Real" OpHelp("r[P2]=P4"), /* 116 */ "IdxInsert" OpHelp("key=r[P2]"), - /* 117 */ "IdxDelete" OpHelp("key=r[P2@P3]"), - /* 118 */ "Clear" OpHelp("space id = P1"), - /* 119 */ "ResetSorter" OpHelp(""), - /* 120 */ "ParseSchema2" OpHelp("rows=r[P1@P2]"), - /* 121 */ "ParseSchema3" OpHelp("name=r[P1] sql=r[P1+1]"), - /* 122 */ "RenameTable" OpHelp("P1 = root, P4 = name"), - /* 123 */ "LoadAnalysis" OpHelp(""), - /* 124 */ "DropTable" OpHelp(""), - /* 125 */ "DropIndex" OpHelp(""), - /* 126 */ "DropTrigger" OpHelp(""), - /* 127 */ "Param" OpHelp(""), - /* 128 */ "FkCounter" OpHelp("fkctr[P1]+=P2"), - /* 129 */ "OffsetLimit" OpHelp("if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1)"), - /* 130 */ "AggStep0" OpHelp("accum=r[P3] step(r[P2@P5])"), - /* 131 */ "AggStep" OpHelp("accum=r[P3] step(r[P2@P5])"), - /* 132 */ "AggFinal" OpHelp("accum=r[P1] N=P2"), - /* 133 */ "Expire" OpHelp(""), - /* 134 */ "IncMaxid" OpHelp(""), - /* 135 */ "Noop" OpHelp(""), - /* 136 */ "Explain" OpHelp(""), + /* 117 */ "SInsert" OpHelp("space id = P1, key = r[P2]"), + /* 118 */ "SDelete" OpHelp("space id = P1, key = r[P2]"), + /* 119 */ "SIDtoPtr" OpHelp("space id = P1, space[out] = r[P2]"), + /* 120 */ "IdxDelete" OpHelp("key=r[P2@P3]"), + /* 121 */ "Clear" OpHelp("space id = P1"), + /* 122 */ "ResetSorter" OpHelp(""), + /* 123 */ "ParseSchema2" OpHelp("rows=r[P1@P2]"), + /* 124 */ "ParseSchema3" OpHelp("name=r[P1] sql=r[P1+1]"), + /* 125 */ "RenameTable" OpHelp("P1 = root, P4 = name"), + /* 126 */ "LoadAnalysis" OpHelp(""), + /* 127 */ "DropTable" OpHelp(""), + /* 128 */ "DropIndex" OpHelp(""), + /* 129 */ "DropTrigger" OpHelp(""), + /* 130 */ "Param" OpHelp(""), + /* 131 */ "FkCounter" OpHelp("fkctr[P1]+=P2"), + /* 132 */ "OffsetLimit" OpHelp("if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1)"), + /* 133 */ "AggStep0" OpHelp("accum=r[P3] step(r[P2@P5])"), + /* 134 */ "AggStep" OpHelp("accum=r[P3] step(r[P2@P5])"), + /* 135 */ "AggFinal" OpHelp("accum=r[P1] N=P2"), + /* 136 */ "Expire" OpHelp(""), + /* 137 */ "IncMaxid" OpHelp(""), + /* 138 */ "Noop" OpHelp(""), + /* 139 */ "Explain" OpHelp(""), }; return azName[i]; } diff --git a/src/box/sql/opcodes.h b/src/box/sql/opcodes.h index af2ba1963..762a23205 100644 --- a/src/box/sql/opcodes.h +++ b/src/box/sql/opcodes.h @@ -104,7 +104,7 @@ #define OP_Close 101 #define OP_ColumnsUsed 102 #define OP_Sequence 103 /* synopsis: r[P2]=cursor[P1].ctr++ */ -#define OP_NextId 104 /* synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) */ +#define OP_NextId 104 /* synopsis: r[P2]=get_max(space_id[P1]) */ #define OP_NextIdEphemeral 105 /* synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) */ #define OP_FCopy 106 /* synopsis: reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)] */ #define OP_Delete 107 @@ -117,26 +117,29 @@ #define OP_IdxReplace 114 /* synopsis: key=r[P2] */ #define OP_Real 115 /* same as TK_FLOAT, synopsis: r[P2]=P4 */ #define OP_IdxInsert 116 /* synopsis: key=r[P2] */ -#define OP_IdxDelete 117 /* synopsis: key=r[P2@P3] */ -#define OP_Clear 118 /* synopsis: space id = P1 */ -#define OP_ResetSorter 119 -#define OP_ParseSchema2 120 /* synopsis: rows=r[P1@P2] */ -#define OP_ParseSchema3 121 /* synopsis: name=r[P1] sql=r[P1+1] */ -#define OP_RenameTable 122 /* synopsis: P1 = root, P4 = name */ -#define OP_LoadAnalysis 123 -#define OP_DropTable 124 -#define OP_DropIndex 125 -#define OP_DropTrigger 126 -#define OP_Param 127 -#define OP_FkCounter 128 /* synopsis: fkctr[P1]+=P2 */ -#define OP_OffsetLimit 129 /* synopsis: if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1) */ -#define OP_AggStep0 130 /* synopsis: accum=r[P3] step(r[P2@P5]) */ -#define OP_AggStep 131 /* synopsis: accum=r[P3] step(r[P2@P5]) */ -#define OP_AggFinal 132 /* synopsis: accum=r[P1] N=P2 */ -#define OP_Expire 133 -#define OP_IncMaxid 134 -#define OP_Noop 135 -#define OP_Explain 136 +#define OP_SInsert 117 /* synopsis: space id = P1, key = r[P2] */ +#define OP_SDelete 118 /* synopsis: space id = P1, key = r[P2] */ +#define OP_SIDtoPtr 119 /* synopsis: space id = P1, space[out] = r[P2] */ +#define OP_IdxDelete 120 /* synopsis: key=r[P2@P3] */ +#define OP_Clear 121 /* synopsis: space id = P1 */ +#define OP_ResetSorter 122 +#define OP_ParseSchema2 123 /* synopsis: rows=r[P1@P2] */ +#define OP_ParseSchema3 124 /* synopsis: name=r[P1] sql=r[P1+1] */ +#define OP_RenameTable 125 /* synopsis: P1 = root, P4 = name */ +#define OP_LoadAnalysis 126 +#define OP_DropTable 127 +#define OP_DropIndex 128 +#define OP_DropTrigger 129 +#define OP_Param 130 +#define OP_FkCounter 131 /* synopsis: fkctr[P1]+=P2 */ +#define OP_OffsetLimit 132 /* synopsis: if r[P1]>0 then r[P2]=r[P1]+max(0,r[P3]) else r[P2]=(-1) */ +#define OP_AggStep0 133 /* synopsis: accum=r[P3] step(r[P2@P5]) */ +#define OP_AggStep 134 /* synopsis: accum=r[P3] step(r[P2@P5]) */ +#define OP_AggFinal 135 /* synopsis: accum=r[P1] N=P2 */ +#define OP_Expire 136 +#define OP_IncMaxid 137 +#define OP_Noop 138 +#define OP_Explain 139 /* Properties such as "out2" or "jump" that are specified in ** comments following the "case" for each opcode in the vdbe.c @@ -162,11 +165,11 @@ /* 80 */ 0x00, 0x02, 0x02, 0x02, 0x00, 0x00, 0x00, 0x00,\ /* 88 */ 0x00, 0x10, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00,\ /* 96 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,\ -/* 104 */ 0x20, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,\ +/* 104 */ 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00,\ /* 112 */ 0x00, 0x04, 0x00, 0x10, 0x04, 0x00, 0x00, 0x00,\ -/* 120 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10,\ -/* 128 */ 0x00, 0x1a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,\ -/* 136 */ 0x00,} +/* 120 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,\ +/* 128 */ 0x00, 0x00, 0x10, 0x00, 0x1a, 0x00, 0x00, 0x00,\ +/* 136 */ 0x00, 0x00, 0x00, 0x00,} /* The sqlite3P2Values() routine is able to run faster if it knows ** the value of the largest JUMP opcode. The smaller the maximum 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); /* 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. @@ -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; } -/* Opcode: NextId P1 P2 P3 * * - * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) +/* Opcode: NextId P1 P2 * * * + * Synopsis: r[P2]=get_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 = p->apCsr[pOp->p1]; - p2 = pOp->p2; - pOut = &aMem[pOp->p3]; - - /* This opcode is Tarantool specific. */ - assert(pC->uc.pCursor->curFlags & BTCF_TaCursor); +case OP_NextId: { + assert(pOp->p1 > 0); + pOut = &aMem[pOp->p2]; - tarantoolSqlGetMaxId(pC->uc.pCursor, p2, - (uint64_t *) &pOut->u.i); + tarantoolSqlGetMaxId(pOp->p1, (uint64_t *) &pOut->u.i); pOut->u.i += 1; pOut->flags = MEM_Int; @@ -4456,6 +4446,84 @@ case OP_IdxInsert: { /* in2 */ break; } +/* Opcode: SInsert P1 P2 * * P5 + * Synopsis: space id = P1, key = r[P2] + * + * This opcode is used only during DML 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 DML 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 DML routine, since schema may + * change and pointers become expired. + */ +case OP_SIDtoPtr: { + assert(pOp->p1 > 0); + assert(pOp->p2 > 0); + + pIn2 = &aMem[pOp->p2]; + struct space *space = space_by_id(pOp->p1); + assert(space != NULL); + pIn2->u.i = (int64_t) space; + break; +} + /* Opcode: IdxDelete P1 P2 P3 * * * Synopsis: key=r[P2@P3] * @@ -5385,22 +5453,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->p1<p->nCursor); - 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces 2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik @ 2018-03-22 11:42 ` v.shpilevoy 2018-03-22 12:23 ` n.pettik 0 siblings, 1 reply; 19+ messages in thread From: v.shpilevoy @ 2018-03-22 11:42 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik See below 6 comments. > 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а): > > 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. > > @@ -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); > 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? > { > 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(space_id, 0 /* PK */, key, key_end, &tuple) != 0) > return -1; > > /* Index is empty */ > @@ -1703,5 +1716,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/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); > > /* 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. > @@ -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; > } > > -/* Opcode: NextId P1 P2 P3 * * > - * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) > +/* Opcode: NextId P1 P2 * * * > + * Synopsis: r[P2]=get_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 = p->apCsr[pOp->p1]; > - p2 = pOp->p2; > - pOut = &aMem[pOp->p3]; > - > - /* This opcode is Tarantool specific. */ > - assert(pC->uc.pCursor->curFlags & BTCF_TaCursor); > +case OP_NextId: { > + assert(pOp->p1 > 0); > + pOut = &aMem[pOp->p2]; > > - tarantoolSqlGetMaxId(pC->uc.pCursor, p2, > - (uint64_t *) &pOut->u.i); > + tarantoolSqlGetMaxId(pOp->p1, (uint64_t *) &pOut->u.i); > > pOut->u.i += 1; > pOut->flags = MEM_Int; > @@ -4456,6 +4446,84 @@ case OP_IdxInsert: { /* in2 */ > break; > } > > +/* 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? > + * 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 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 >= 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 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. > + assert(pOp->p1 > 0); > + assert(pOp->p2 > 0); > + > + pIn2 = &aMem[pOp->p2]; > + struct space *space = space_by_id(pOp->p1); > + assert(space != NULL); > + pIn2->u.i = (int64_t) space; > + break; > +} > + > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces 2018-03-22 11:42 ` [tarantool-patches] " v.shpilevoy @ 2018-03-22 12:23 ` n.pettik 2018-03-22 13:09 ` v.shpilevoy 0 siblings, 1 reply; 19+ messages in thread From: n.pettik @ 2018-03-22 12:23 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 12081 bytes --] > On 22 Mar 2018, at 14:42, v.shpilevoy@tarantool.org wrote: > > See below 6 comments. > >> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> написал(а): >> >> 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). >> >> @@ -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); > >> 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. > >> { >> 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(space_id, 0 /* PK */, key, key_end, &tuple) != 0) >> return -1; >> >> /* Index is empty */ >> @@ -1703,5 +1716,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/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); >> >> /* 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. >> @@ -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; >> } >> >> -/* Opcode: NextId P1 P2 P3 * * >> - * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]}) >> +/* Opcode: NextId P1 P2 * * * >> + * Synopsis: r[P2]=get_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 = p->apCsr[pOp->p1]; >> - p2 = pOp->p2; >> - pOut = &aMem[pOp->p3]; >> - >> - /* This opcode is Tarantool specific. */ >> - assert(pC->uc.pCursor->curFlags & BTCF_TaCursor); >> +case OP_NextId: { >> + assert(pOp->p1 > 0); >> + pOut = &aMem[pOp->p2]; >> >> - tarantoolSqlGetMaxId(pC->uc.pCursor, p2, >> - (uint64_t *) &pOut->u.i); >> + tarantoolSqlGetMaxId(pOp->p1, (uint64_t *) &pOut->u.i); >> >> pOut->u.i += 1; >> pOut->flags = MEM_Int; >> @@ -4456,6 +4446,84 @@ case OP_IdxInsert: { /* in2 */ >> break; >> } >> >> +/* 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? > >> + * 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 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 >= 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 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. 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. [-- Attachment #2: Type: text/html, Size: 35500 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces 2018-03-22 12:23 ` n.pettik @ 2018-03-22 13:09 ` v.shpilevoy 2018-03-23 16:20 ` n.pettik 0 siblings, 1 reply; 19+ messages in thread From: v.shpilevoy @ 2018-03-22 13:09 UTC (permalink / raw) To: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 8443 bytes --] > 22 марта 2018 г., в 15:23, n.pettik <korablev@tarantool.org> написал(а): > > >> On 22 Mar 2018, at 14:42, v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org> wrote: >> >> See below 6 comments. >> >>> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> написал(а): >>> >>> 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. > >>> >>> @@ -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. >> >>> 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. >>> >>> +/* 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? >> >>> + * 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. > [-- Attachment #2: Type: text/html, Size: 26092 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces 2018-03-22 13:09 ` v.shpilevoy @ 2018-03-23 16:20 ` n.pettik 0 siblings, 0 replies; 19+ messages in thread From: n.pettik @ 2018-03-23 16:20 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 22233 bytes --] > On 22 Mar 2018, at 16:09, v.shpilevoy@tarantool.org wrote: > > > >> 22 марта 2018 г., в 15:23, n.pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> написал(а): >> >> >>> On 22 Mar 2018, at 14:42, v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org> wrote: >>> >>> See below 6 comments. >>> >>>> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> написал(а): >>>> >>>> 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 <http://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->p1<p->nCursor); - 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 [-- Attachment #2: Type: text/html, Size: 63396 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik @ 2018-03-20 23:48 ` Nikita Pettik 2018-03-22 13:57 ` [tarantool-patches] " v.shpilevoy 2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik 2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy 4 siblings, 1 reply; 19+ messages in thread From: Nikita Pettik @ 2018-03-20 23:48 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik As far as new opcodes to operate on system spaces have been introduced, there is no more need to open cursors on such tables, when it comes to insert/delete operations. In addition, it allows to get rid of system spaces lookups from SQL data dictionary. Moreover, previously DDL relied on nested parse to make deletions from system space. But during nested parse it is impossible to convert space id to space pointer (i.e. emit OP_SIDtoPtr opcode) without any overhead or "hacks". So, nested parse has been replaced with hardcoded sequences of opcodes, implementing the same logic. Closes #3252 --- src/box/sql/build.c | 322 ++++++++++++++++++++++--------------------- src/box/sql/sqliteInt.h | 5 + src/box/sql/trigger.c | 27 ++-- test/sql/transition.result | 11 +- test/sql/transition.test.lua | 8 +- 5 files changed, 190 insertions(+), 183 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 229c8b4d5..02d27dec6 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1425,13 +1425,12 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab) * Return register storing the result. */ static int -getNewSpaceId(Parse * pParse, int iCursor) +getNewSpaceId(Parse * pParse) { Vdbe *v = sqlite3GetVdbe(pParse); int iRes = ++pParse->nMem; - sqlite3VdbeAddOp1(v, OP_IncMaxid, iCursor); - sqlite3VdbeAddOp3(v, OP_Column, iCursor, 1, iRes); + sqlite3VdbeAddOp1(v, OP_IncMaxid, iRes); return iRes; } @@ -1441,10 +1440,8 @@ getNewSpaceId(Parse * pParse, int iCursor) * space id or designates a register storing the id. */ static void -createIndex(Parse * pParse, - Index * pIndex, - int iSpaceId, - int iIndexId, const char *zSql, Table * pSysIndex, int iCursor) +createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId, + const char *zSql) { Vdbe *v = sqlite3GetVdbe(pParse); int iFirstCol = ++pParse->nMem; @@ -1509,7 +1506,7 @@ createIndex(Parse * pParse, sqlite3VdbeAddOp4(v, OP_Blob, zPartsSz, iFirstCol + 5, MSGPACK_SUBTYPE, zParts, P4_STATIC); sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 6, iRecord); - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, iRecord, iFirstCol, 6); + sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, iRecord); /* Do not account nested operations: the count of such * operations depends on Tarantool data dictionary internals, * such as data layout in system spaces. Also do not account @@ -1518,7 +1515,6 @@ createIndex(Parse * pParse, */ if (!pParse->nested && pIndex->idxType == SQLITE_IDXTYPE_APPDEF) sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); - sqlite3TableAffinity(v, pSysIndex, 0); } /* @@ -1571,8 +1567,7 @@ makeIndexSchemaRecord(Parse * pParse, * iCursor is a cursor to access _space. */ static void -createSpace(Parse * pParse, - int iSpaceId, char *zStmt, int iCursor, Table * pSysSpace) +createSpace(Parse * pParse, int iSpaceId, char *zStmt) { Table *p = pParse->pNewTable; Vdbe *v = sqlite3GetVdbe(pParse); @@ -1615,14 +1610,13 @@ createSpace(Parse * pParse, sqlite3VdbeAddOp4(v, OP_Blob, zFormatSz, iFirstCol + 6, MSGPACK_SUBTYPE, zFormat, P4_STATIC); sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord); - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, iRecord, iFirstCol, 7); + sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord); /* Do not account nested operations: the count of such * operations depends on Tarantool data dictionary internals, * such as data layout in system spaces. */ if (!pParse->nested) sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); - sqlite3TableAffinity(v, pSysSpace, 0); } /* @@ -1631,8 +1625,7 @@ createSpace(Parse * pParse, * iCursor is a cursor to access _index. */ static void -createImplicitIndices(Parse * pParse, - int iSpaceId, int iCursor, Table * pSysIndex) +createImplicitIndices(Parse * pParse, int iSpaceId) { Table *p = pParse->pNewTable; Index *pIdx, *pPrimaryIdx = sqlite3PrimaryKeyIndex(p); @@ -1640,22 +1633,21 @@ createImplicitIndices(Parse * pParse, if (pPrimaryIdx) { /* Tarantool quirk: primary index is created first */ - createIndex(pParse, pPrimaryIdx, iSpaceId, 0, NULL, pSysIndex, - iCursor); + createIndex(pParse, pPrimaryIdx, iSpaceId, 0, NULL); } else { /* * This branch should not be taken. * If it is, then the current CREATE TABLE statement fails to * specify the PRIMARY KEY. The error is reported elsewhere. */ + unreachable(); } /* (pIdx->i) mapping must be consistent with parseTableSchemaRecord */ for (pIdx = p->pIndex, i = 0; pIdx; pIdx = pIdx->pNext) { if (pIdx == pPrimaryIdx) continue; - createIndex(pParse, pIdx, iSpaceId, ++i, NULL, pSysIndex, - iCursor); + createIndex(pParse, pIdx, iSpaceId, ++i, NULL); } } @@ -1849,29 +1841,12 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ Vdbe *v; char *zType; /* "VIEW" or "TABLE" */ char *zStmt; /* Text of the CREATE TABLE or CREATE VIEW statement */ - Table *pSysSchema, *pSysSpace, *pSysIndex; - int iCursor = pParse->nTab++; int iSpaceId; v = sqlite3GetVdbe(pParse); if (NEVER(v == 0)) return; - pSysSchema = sqlite3HashFind(&pParse->db->pSchema->tblHash, - TARANTOOL_SYS_SCHEMA_NAME); - if (NEVER(!pSysSchema)) - return; - - pSysSpace = sqlite3HashFind(&pParse->db->pSchema->tblHash, - TARANTOOL_SYS_SPACE_NAME); - if (NEVER(!pSysSpace)) - return; - - pSysIndex = sqlite3HashFind(&pParse->db->pSchema->tblHash, - TARANTOOL_SYS_INDEX_NAME); - if (NEVER(!pSysIndex)) - return; - /* * Initialize zType for the new view or table. */ @@ -1913,66 +1888,35 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ pParse->sNameToken.z); } - sqlite3OpenTable(pParse, iCursor, pSysSchema, OP_OpenRead); - sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ); - iSpaceId = getNewSpaceId(pParse, iCursor); - sqlite3OpenTable(pParse, iCursor, pSysSpace, OP_OpenWrite); - createSpace(pParse, iSpaceId, zStmt, iCursor, pSysSpace); - sqlite3OpenTable(pParse, iCursor, pSysIndex, OP_OpenWrite); - createImplicitIndices(pParse, iSpaceId, iCursor, pSysIndex); - sqlite3VdbeAddOp1(v, OP_Close, iCursor); + iSpaceId = getNewSpaceId(pParse); + createSpace(pParse, iSpaceId, zStmt); + /* Indexes aren't required for VIEW's. */ + if (p->pSelect == NULL) { + createImplicitIndices(pParse, iSpaceId); + } /* Check to see if we need to create an _sequence table for * keeping track of autoincrement keys. */ if ((p->tabFlags & TF_Autoincrement) != 0) { - Table *sys_sequence, *sys_space_sequence; int reg_seq_id; int reg_seq_record, reg_space_seq_record; assert(iSpaceId); - - /* Do an insertion into _sequence */ - sys_sequence = sqlite3HashFind( - &pParse->db->pSchema->tblHash, - TARANTOOL_SYS_SEQUENCE_NAME); - if (NEVER(!sys_sequence)) - return; - - sqlite3OpenTable(pParse, iCursor, sys_sequence, - OP_OpenWrite); + /* Do an insertion into _sequence. */ reg_seq_id = ++pParse->nMem; - sqlite3VdbeAddOp3(v, OP_NextId, iCursor, 0, reg_seq_id); - + sqlite3VdbeAddOp2(v, OP_NextId, BOX_SEQUENCE_ID, + reg_seq_id); reg_seq_record = emitNewSysSequenceRecord(pParse, reg_seq_id, p->zName); - if (reg_seq_record < 0) - return; - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, - reg_seq_record, - reg_seq_record + 1, 9); - sqlite3VdbeAddOp1(v, OP_Close, iCursor); - - /* Do an insertion into _space_sequence */ - sys_space_sequence = sqlite3HashFind( - &pParse->db->pSchema->tblHash, - TARANTOOL_SYS_SPACE_SEQUENCE_NAME); - if (NEVER(!sys_space_sequence)) - return; - - sqlite3OpenTable(pParse, iCursor, sys_space_sequence, - OP_OpenWrite); - - reg_space_seq_record = emitNewSysSpaceSequenceRecord( - pParse, - iSpaceId, - reg_seq_id); - - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, - reg_space_seq_record, - reg_space_seq_record + 1, 3); - - sqlite3VdbeAddOp1(v, OP_Close, iCursor); + sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, + reg_seq_record); + /* Do an insertion into _space_sequence. */ + reg_space_seq_record = + emitNewSysSpaceSequenceRecord(pParse, iSpaceId, + reg_seq_id); + sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, + reg_space_seq_record); } /* Reparse everything to update our internal data structures */ @@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) v = sqlite3GetVdbe(pParse); assert(v != 0); sqlite3BeginWriteOperation(pParse, 1); - - /* Drop all triggers associated with the table being dropped. Code - * is generated to remove entries from _trigger space. + /* + * Drop all triggers associated with the table being + * dropped. Code is generated to remove entries from + * _trigger. OP_DropTrigger will remove it from internal + * SQL structures. */ pTrigger = pTab->pTrigger; /* Do not account triggers deletion - they will be accounted @@ -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) pTrigger = pTrigger->pNext; } pParse->nested--; - - /* Remove any entries of the _sequence table associated with - * the table being dropped. This is done before the table is dropped - * at the btree level, in case the _sequence table needs to - * move as a result of the drop. + /* + * Remove any entries of the _sequence and _space_sequence + * space associated with the table being dropped. This is + * done before the table is dropped from internal schema. */ + int idx_rec_reg = ++pParse->nMem; + int space_id_reg = ++pParse->nMem; + int space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); + sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg); if (pTab->tabFlags & TF_Autoincrement) { - sqlite3NestedParse(pParse, - "DELETE FROM \"%s\" WHERE \"space_id\"=%d", - TARANTOOL_SYS_SPACE_SEQUENCE_NAME, - SQLITE_PAGENO_TO_SPACEID(pTab->tnum)); - sqlite3NestedParse(pParse, - "DELETE FROM \"%s\" WHERE \"name\"=%Q", - TARANTOOL_SYS_SEQUENCE_NAME, - pTab->zName); - } - - /* Drop all _space and _index entries that refer to the - * table. The program loops through the _index & _space tables and deletes - * every row that refers to a table. - * Triggers are handled separately because a trigger can be - * created in the temp database that refers to a table in another - * database. + /* Delete entry by from _space_sequence. */ + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, + idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID, + idx_rec_reg); + VdbeComment((v, "Delete entry from _space_sequence")); + /* + * Program below implement analogue of SQL statement: + * "DELETE FROM BOX_SEQUENCE WHERE name = zName;" + * However, we can't use nested parse for DDL, + * since pointers to spaces will expire. + */ + /* Name to be found in _sequence. */ + int space_ref_name = ++pParse->nMem; + /* Pointer to _sequence to be passe to OpenWrite. */ + int space_ptr = ++pParse->nMem; + /* Cursor to _sequence which will generate OpenWrite. */ + int cursor_to_seq = pParse->nTab++; + /* Name of space fetched from _sequence. */ + int space_name = ++pParse->nMem; + /* Space id fetched from _sequence. */ + int space_id_seq = ++pParse->nMem; + int record_to_delete = ++pParse->nMem; + /* Save name to be found in register. */ + sqlite3VdbeAddOp4(v, OP_String8, 0, space_ref_name, 0, + pTab->zName, P4_STATIC); + /* Convert space id to space pointer and open cursor. */ + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_SEQUENCE_ID, space_ptr); + sqlite3VdbeAddOp4Int(v, OP_OpenWrite, cursor_to_seq, 0, + space_ptr, + 9 /* Number of fields in _sequence. */); + int loop_exit = sqlite3VdbeAddOp1(v, OP_Rewind, cursor_to_seq); + /* Fetch space name from tuple and save to space_name reg. */ + int loop_start = sqlite3VdbeAddOp3(v, OP_Column, cursor_to_seq, + 2, space_name); + /* Compare it with the name of space we want to delete. */ + int loop_end = sqlite3VdbeAddOp3(v, OP_Ne, space_ref_name, 0, + space_name); + /* Store _sequence.id to space_id_seq register. */ + sqlite3VdbeAddOp3(v, OP_Column, cursor_to_seq, 0, space_id_seq); + /* Prepare record to be deleted. Key is in space_id_seq. */ + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_seq, 1, + record_to_delete); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, + record_to_delete); + VdbeComment((v, "Delete entry from _sequence")); + sqlite3VdbeJumpHere(v, loop_end); + sqlite3VdbeAddOp2(v, OP_Next, cursor_to_seq, loop_start); + sqlite3VdbeJumpHere(v, loop_exit); + } + /* + * Drop all _space and _index entries that refer to the + * table. */ - int space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); if (!isView) { - if (pTab->pIndex && pTab->pIndex->pNext) { - /* Remove all indexes, except for primary. - Tarantool won't allow remove primary when secondary exist. */ - sqlite3NestedParse(pParse, - "DELETE FROM \"%s\" WHERE \"id\"=%d AND \"iid\">0", - TARANTOOL_SYS_INDEX_NAME, space_id); + struct space *space = space_by_id(space_id); + assert(space != NULL); + uint32_t index_count = space->index_count; + if (index_count > 1) { + /* + * Remove all indexes, except for primary. + * Tarantool won't allow remove primary when + * secondary exist. + */ + uint32_t *iids = + (uint32_t *) sqlite3Malloc(sizeof(uint32_t) * + (index_count - 1)); + /* Save index ids to be deleted except for PK. */ + for (uint32_t i = 1; i < index_count; ++i) { + iids[i - 1] = space->index[i]->def->iid; + } + for (uint32_t i = 0; i < index_count - 1; ++i) { + sqlite3VdbeAddOp2(v, OP_Integer, iids[i], + space_id_reg + 1); + sqlite3VdbeAddOp3(v, OP_MakeRecord, + space_id_reg, 2, idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, + idx_rec_reg); + VdbeComment((v, + "Remove secondary index iid = %u", + iids[i])); + } + sqlite3_free(iids); } - - /* Remove primary index. */ - sqlite3NestedParse(pParse, - "DELETE FROM \"%s\" WHERE \"id\"=%d AND \"iid\"=0", - TARANTOOL_SYS_INDEX_NAME, space_id); + sqlite3VdbeAddOp2(v, OP_Integer, 0, space_id_reg + 1); + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, + idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg); + VdbeComment((v, "Remove primary index")); } /* Delete records about the space from the _truncate. */ - sqlite3NestedParse(pParse, "DELETE FROM \"" - TARANTOOL_SYS_TRUNCATE_NAME "\" WHERE \"id\" = %d", - space_id); - - Expr *id_value = sqlite3ExprInteger(db, space_id); - const char *column = "id"; - /* Execute not nested DELETE of a space to account DROP TABLE - * changes. - */ - sqlite3DeleteByKey(pParse, TARANTOOL_SYS_SPACE_NAME, - &column, &id_value, 1); - + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_TRUNCATE_ID, idx_rec_reg); + VdbeComment((v, "Delete entry from _truncate")); + /* Eventually delete entry from _space. */ + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_ID, idx_rec_reg); + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); + VdbeComment((v, "Delete entry from _space")); /* Remove the table entry from SQLite's internal schema and modify * the schema cookie. */ @@ -2603,16 +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) sqlite3VdbeJumpHere(v, addr1); if (memRootPage < 0) sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); - assert(space != NULL); int space_ptr_reg = ++pParse->nMem; - sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, - ((int64_t) space)); + /* + * OP_Clear can use truncate optimization + * (i.e. insert record into _truncate) and schema + * may change. Thus, use dynamic conversion from + * space id to ptr. + */ + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, SQLITE_PAGENO_TO_SPACEID(tnum), + space_ptr_reg); sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, (char *)pKey, P4_KEYINFO); - sqlite3VdbeChangeP5(v, - OPFLAG_BULKCSR | ((memRootPage >= 0) ? - OPFLAG_P2ISREG : 0)); + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR); addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0); VdbeCoverage(v); @@ -3138,8 +3143,8 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ else if (pTblName) { Vdbe *v; char *zStmt; - Table *pSysIndex; int iCursor = pParse->nTab++; + int index_space_ptr_reg = pParse->nTab++; int iSpaceId, iIndexId, iFirstSchemaCol; v = sqlite3GetVdbe(pParse); @@ -3148,13 +3153,11 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ sqlite3BeginWriteOperation(pParse, 1); - pSysIndex = - sqlite3HashFind(&pParse->db->pSchema->tblHash, - TARANTOOL_SYS_INDEX_NAME); - if (NEVER(!pSysIndex)) - return; - sqlite3OpenTable(pParse, iCursor, pSysIndex, OP_OpenWrite); + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_INDEX_ID, + index_space_ptr_reg); + sqlite3VdbeAddOp4Int(v, OP_OpenWrite, iCursor, 0, + index_space_ptr_reg, 6); sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ); /* Gather the complete text of the CREATE INDEX statement into @@ -3176,9 +3179,8 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ iSpaceId = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); iIndexId = getNewIid(pParse, iSpaceId, iCursor); - createIndex(pParse, pIndex, iSpaceId, iIndexId, zStmt, - pSysIndex, iCursor); sqlite3VdbeAddOp1(v, OP_Close, iCursor); + createIndex(pParse, pIndex, iSpaceId, iIndexId, zStmt); /* consumes zStmt */ iFirstSchemaCol = @@ -3322,17 +3324,21 @@ sqlite3DropIndex(Parse * pParse, SrcList * pName, Token * pName2, int ifExists) goto exit_drop_index; } - /* Generate code to remove the index and from the master table */ - sqlite3BeginWriteOperation(pParse, 1); - const char *columns[2] = { "id", "iid" }; - Expr *values[2]; - values[0] = - sqlite3ExprInteger(db, SQLITE_PAGENO_TO_SPACEID(pIndex->tnum)); - values[1] = - sqlite3ExprInteger(db, SQLITE_PAGENO_TO_INDEXID(pIndex->tnum)); - sqlite3DeleteByKey(pParse, TARANTOOL_SYS_INDEX_NAME, - columns, values, 2); + /* + * Generate code to remove entry from _index space + * But firstly, delete statistics since schema + * changes after DDL. + */ sqlite3ClearStatTables(pParse, "idx", pIndex->zName); + int record_reg = ++pParse->nMem; + int space_id_reg = ++pParse->nMem; + sqlite3VdbeAddOp2(v, OP_Integer, SQLITE_PAGENO_TO_SPACEID(pIndex->tnum), + space_id_reg); + sqlite3VdbeAddOp2(v, OP_Integer, SQLITE_PAGENO_TO_INDEXID(pIndex->tnum), + space_id_reg + 1); + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg); + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); sqlite3ChangeCookie(pParse); sqlite3VdbeAddOp3(v, OP_DropIndex, 0, 0, 0); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 190a33cf2..95bad3773 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2455,6 +2455,11 @@ struct Parse { #define OPFLAG_NOOP_IF_NULL 0x02 /* OP_FCopy: if source register is NULL * then do nothing */ +#define OPFLAG_FRESH_PTR 0x20 /* OP_Open**: set if space pointer + * comes from OP_SIDtoPtr, i.e. it + * is fresh, even in case schema + * changes previously. + */ /* * Each trigger present in the database schema is stored as an instance of diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index a2827c882..eca6ce6b3 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -557,23 +557,22 @@ tableOfTrigger(Trigger * pTrigger) void sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger) { - Table *pTable MAYBE_UNUSED; Vdbe *v; - sqlite3 *db = pParse->db; - - assert(sqlite3SchemaToIndex(pParse->db, pTrigger->pSchema) == 0); - pTable = tableOfTrigger(pTrigger); - assert(pTable); - assert(pTable->pSchema == pTrigger->pSchema); - - /* Generate code to destroy the database record of the trigger. + /* Generate code to delete entry from _trigger and + * internal SQL structures. */ - assert(pTable != 0); if ((v = sqlite3GetVdbe(pParse)) != 0) { - const char *column = "name"; - Expr *value = sqlite3Expr(db, TK_STRING, pTrigger->zName); - sqlite3DeleteByKey(pParse, TARANTOOL_SYS_TRIGGER_NAME, - &column, &value, 1); + int trig_name_reg = ++pParse->nMem; + int record_to_delete = ++pParse->nMem; + sqlite3VdbeAddOp4(v, OP_String8, 0, trig_name_reg, 0, + pTrigger->zName, P4_STATIC); + sqlite3VdbeAddOp3(v, OP_MakeRecord, trig_name_reg, 1, + record_to_delete); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_TRIGGER_ID, + record_to_delete); + if (!pParse->nested) + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); + sqlite3ChangeCookie(pParse); sqlite3VdbeAddOp4(v, OP_DropTrigger, 0, 0, 0, pTrigger->zName, 0); diff --git a/test/sql/transition.result b/test/sql/transition.result index 500fe85c6..202ad2ac8 100644 --- a/test/sql/transition.result +++ b/test/sql/transition.result @@ -112,10 +112,6 @@ box.sql.execute("SELECT COUNT(*) FROM foobar WHERE bar='cacodaemon'") --- - - [0] ... --- cleanup -box.space.FOOBAR:drop() ---- -... -- multi-index -- create space box.sql.execute("CREATE TABLE barfoo (bar, foo NUM PRIMARY KEY)") @@ -191,6 +187,7 @@ box.sql.execute("CREATE TABLE implicit_indices(a PRIMARY KEY,b,c,d UNIQUE)") box.space.IMPLICIT_INDICES:drop() --- ... -box.sql.execute("DROP TABLE implicit_indices") ---- -... +-- Commented until SQL and Tarantool data-dictionaries are integrated. +-- It is incorrect, since after space dropping via Lua interface, +-- it still remain in SQL internal structures. +-- box.sql.execute("DROP TABLE implicit_indices") diff --git a/test/sql/transition.test.lua b/test/sql/transition.test.lua index 996e634ec..885993bb8 100644 --- a/test/sql/transition.test.lua +++ b/test/sql/transition.test.lua @@ -33,9 +33,6 @@ box.sql.execute("SELECT COUNT(*) FROM foobar WHERE bar='cacodaemon'") box.sql.execute("DELETE FROM foobar WHERE bar='cacodaemon'") box.sql.execute("SELECT COUNT(*) FROM foobar WHERE bar='cacodaemon'") --- cleanup -box.space.FOOBAR:drop() - -- multi-index -- create space @@ -70,4 +67,7 @@ box.sql.execute("CREATE TABLE rowid(x)") -- create a table with implicit indices (used to SEGFAULT) box.sql.execute("CREATE TABLE implicit_indices(a PRIMARY KEY,b,c,d UNIQUE)") box.space.IMPLICIT_INDICES:drop() -box.sql.execute("DROP TABLE implicit_indices") +-- Commented until SQL and Tarantool data-dictionaries are integrated. +-- It is incorrect, since after space dropping via Lua interface, +-- it still remain in SQL internal structures. +-- box.sql.execute("DROP TABLE implicit_indices") -- 2.15.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine 2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik @ 2018-03-22 13:57 ` v.shpilevoy 2018-03-23 16:33 ` n.pettik 0 siblings, 1 reply; 19+ messages in thread From: v.shpilevoy @ 2018-03-22 13:57 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik See below 4 comments. > 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а): > > As far as new opcodes to operate on system spaces have been introduced, > there is no more need to open cursors on such tables, when it comes to > insert/delete operations. In addition, it allows to get rid of system > spaces lookups from SQL data dictionary. Moreover, previously DDL > relied on nested parse to make deletions from system space. But during > nested parse it is impossible to convert space id to space pointer (i.e. > emit OP_SIDtoPtr opcode) without any overhead or "hacks". So, nested > parse has been replaced with hardcoded sequences of opcodes, > implementing the same logic. > > Closes #3252 > --- > src/box/sql/build.c | 322 ++++++++++++++++++++++--------------------- > src/box/sql/sqliteInt.h | 5 + > src/box/sql/trigger.c | 27 ++-- > test/sql/transition.result | 11 +- > test/sql/transition.test.lua | 8 +- > 5 files changed, 190 insertions(+), 183 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 229c8b4d5..02d27dec6 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) 1. sqlite3CodeDropTable now is used in build.c only, and can be declared as static (and removed from sqliteInt.h). > v = sqlite3GetVdbe(pParse); > assert(v != 0); > sqlite3BeginWriteOperation(pParse, 1); > - > - /* Drop all triggers associated with the table being dropped. Code > - * is generated to remove entries from _trigger space. > + /* > + * Drop all triggers associated with the table being > + * dropped. Code is generated to remove entries from > + * _trigger. OP_DropTrigger will remove it from internal > + * SQL structures. > */ > pTrigger = pTab->pTrigger; > /* Do not account triggers deletion - they will be accounted > @@ -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) > - > - /* Drop all _space and _index entries that refer to the > - * table. The program loops through the _index & _space tables and deletes > - * every row that refers to a table. > - * Triggers are handled separately because a trigger can be > - * created in the temp database that refers to a table in another > - * database. > + /* Delete entry by from _space_sequence. */ > + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, > + idx_rec_reg); > + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID, > + idx_rec_reg); > + VdbeComment((v, "Delete entry from _space_sequence")); > + /* > + * Program below implement analogue of SQL statement: > + * "DELETE FROM BOX_SEQUENCE WHERE name = zName;" > + * However, we can't use nested parse for DDL, > + * since pointers to spaces will expire. > + */ 2. Why do you delete by name, if you have struct space.sequence->def->id? > > + /* > + * Drop all _space and _index entries that refer to the > + * table. > */ > - int space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); > if (!isView) { > - if (pTab->pIndex && pTab->pIndex->pNext) { > - /* Remove all indexes, except for primary. > - Tarantool won't allow remove primary when secondary exist. */ > - sqlite3NestedParse(pParse, > - "DELETE FROM \"%s\" WHERE \"id\"=%d AND \"iid\">0", > - TARANTOOL_SYS_INDEX_NAME, space_id); > + struct space *space = space_by_id(space_id); > + assert(space != NULL); > + uint32_t index_count = space->index_count; > + if (index_count > 1) { > + /* > + * Remove all indexes, except for primary. > + * Tarantool won't allow remove primary when > + * secondary exist. > + */ > + uint32_t *iids = > + (uint32_t *) sqlite3Malloc(sizeof(uint32_t) * > + (index_count - 1)); 3.1. Check for OOM; 3.2. For such short lived allocations you can use region_alloc - it allows to avoid multiple free() calls on each alloc, and free all temp allocations at once after a transaction is finished. > > @@ -2603,16 +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) > sqlite3VdbeJumpHere(v, addr1); > if (memRootPage < 0) > sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); > - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); > - assert(space != NULL); > int space_ptr_reg = ++pParse->nMem; > - sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, > - ((int64_t) space)); > + /* > + * OP_Clear can use truncate optimization > + * (i.e. insert record into _truncate) and schema > + * may change. Thus, use dynamic conversion from > + * space id to ptr. > + */ > + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, SQLITE_PAGENO_TO_SPACEID(tnum), > + space_ptr_reg); > sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, > (char *)pKey, P4_KEYINFO); > - sqlite3VdbeChangeP5(v, > - OPFLAG_BULKCSR | ((memRootPage >= 0) ? > - OPFLAG_P2ISREG : 0)); > + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR); 4. OPFLAG_FRESH_PTR seems to be unused. Why do you need it? > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine 2018-03-22 13:57 ` [tarantool-patches] " v.shpilevoy @ 2018-03-23 16:33 ` n.pettik 0 siblings, 0 replies; 19+ messages in thread From: n.pettik @ 2018-03-23 16:33 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 6985 bytes --] > On 22 Mar 2018, at 16:57, v.shpilevoy@tarantool.org wrote: > > See below 4 comments. > >> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> написал(а): >> >> As far as new opcodes to operate on system spaces have been introduced, >> there is no more need to open cursors on such tables, when it comes to >> insert/delete operations. In addition, it allows to get rid of system >> spaces lookups from SQL data dictionary. Moreover, previously DDL >> relied on nested parse to make deletions from system space. But during >> nested parse it is impossible to convert space id to space pointer (i.e. >> emit OP_SIDtoPtr opcode) without any overhead or "hacks". So, nested >> parse has been replaced with hardcoded sequences of opcodes, >> implementing the same logic. >> >> Closes #3252 >> --- >> src/box/sql/build.c | 322 ++++++++++++++++++++++--------------------- >> src/box/sql/sqliteInt.h | 5 + >> src/box/sql/trigger.c | 27 ++-- >> test/sql/transition.result | 11 +- >> test/sql/transition.test.lua | 8 +- >> 5 files changed, 190 insertions(+), 183 deletions(-) >> >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 229c8b4d5..02d27dec6 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) > > 1. sqlite3CodeDropTable now is used in build.c only, and can be declared as static (and removed from sqliteInt.h). Done: --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2306,7 +2250,7 @@ sqlite3ClearStatTables(Parse * pParse, /* The parsing context */ /* * Generate code to drop a table. */ -void +static void sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3007,7 +3012,6 @@ int sqlite3ViewGetColumnNames(Parse *, Table *); int sqlite3DbMaskAllZero(yDbMask); #endif void sqlite3DropTable(Parse *, SrcList *, int, int); -void sqlite3CodeDropTable(Parse *, Table *, int); > >> v = sqlite3GetVdbe(pParse); >> assert(v != 0); >> sqlite3BeginWriteOperation(pParse, 1); >> - >> - /* Drop all triggers associated with the table being dropped. Code >> - * is generated to remove entries from _trigger space. >> + /* >> + * Drop all triggers associated with the table being >> + * dropped. Code is generated to remove entries from >> + * _trigger. OP_DropTrigger will remove it from internal >> + * SQL structures. >> */ >> pTrigger = pTab->pTrigger; >> /* Do not account triggers deletion - they will be accounted >> @@ -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) >> - >> - /* Drop all _space and _index entries that refer to the >> - * table. The program loops through the _index & _space tables and deletes >> - * every row that refers to a table. >> - * Triggers are handled separately because a trigger can be >> - * created in the temp database that refers to a table in another >> - * database. >> + /* Delete entry by from _space_sequence. */ >> + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, >> + idx_rec_reg); >> + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID, >> + idx_rec_reg); >> + VdbeComment((v, "Delete entry from _space_sequence")); >> + /* >> + * Program below implement analogue of SQL statement: >> + * "DELETE FROM BOX_SEQUENCE WHERE name = zName;" >> + * However, we can't use nested parse for DDL, >> + * since pointers to spaces will expire. >> + */ > > 2. Why do you delete by name, if you have struct space.sequence->def->id? To be honest, I forgot about such opportunity. Fetching id directly from sequence allows to significantly reduce number of opcodes and simplify query logic: + /* Delete entry by id from _sequence. */ + assert(space->sequence != NULL); + int sequence_id_reg = ++pParse->nMem; + sqlite3VdbeAddOp2(v, OP_Integer, space->sequence->def->id, + sequence_id_reg); + sqlite3VdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1, + idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg); + VdbeComment((v, "Delete entry from _sequence")); + } > >> >> + /* >> + * Drop all _space and _index entries that refer to the >> + * table. >> */ >> - int space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); >> if (!isView) { >> - if (pTab->pIndex && pTab->pIndex->pNext) { >> - /* Remove all indexes, except for primary. >> - Tarantool won't allow remove primary when secondary exist. */ >> - sqlite3NestedParse(pParse, >> - "DELETE FROM \"%s\" WHERE \"id\"=%d AND \"iid\">0", >> - TARANTOOL_SYS_INDEX_NAME, space_id); >> + struct space *space = space_by_id(space_id); >> + assert(space != NULL); >> + uint32_t index_count = space->index_count; >> + if (index_count > 1) { >> + /* >> + * Remove all indexes, except for primary. >> + * Tarantool won't allow remove primary when >> + * secondary exist. >> + */ >> + uint32_t *iids = >> + (uint32_t *) sqlite3Malloc(sizeof(uint32_t) * >> + (index_count - 1)); > > 3.1. Check for OOM; > 3.2. For such short lived allocations you can use region_alloc - it allows to avoid multiple free() calls on each alloc, and free all temp allocations at once after a transaction is finished. Ok, substituted with region_alloc: + size_t size = sizeof(uint32_t) * (index_count - 1); + uint32_t *iids = + (uint32_t *) region_alloc(&fiber()->gc, size); + if (iids == NULL) { + diag_set(OutOfMemory, size, "region_alloc", + "iids"); + return; + } >> >> @@ -2603,16 +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) >> sqlite3VdbeJumpHere(v, addr1); >> if (memRootPage < 0) >> sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); >> - struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); >> - assert(space != NULL); >> int space_ptr_reg = ++pParse->nMem; >> - sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, >> - ((int64_t) space)); >> + /* >> + * OP_Clear can use truncate optimization >> + * (i.e. insert record into _truncate) and schema >> + * may change. Thus, use dynamic conversion from >> + * space id to ptr. >> + */ >> + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, SQLITE_PAGENO_TO_SPACEID(tnum), >> + space_ptr_reg); >> sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, >> (char *)pKey, P4_KEYINFO); >> - sqlite3VdbeChangeP5(v, >> - OPFLAG_BULKCSR | ((memRootPage >= 0) ? >> - OPFLAG_P2ISREG : 0)); >> + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR); > > 4. OPFLAG_FRESH_PTR seems to be unused. Why do you need it? I need it to indicate that pointer can’t become expired. Its usage is introduced in the last patch. [-- Attachment #2: Type: text/html, Size: 28601 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik ` (2 preceding siblings ...) 2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik @ 2018-03-20 23:48 ` Nikita Pettik 2018-03-22 14:11 ` [tarantool-patches] " v.shpilevoy 2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy 4 siblings, 1 reply; 19+ messages in thread From: Nikita Pettik @ 2018-03-20 23:48 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik After new DDL SQL implementation has been introduced, OP_OpenWrite, OP_OpenRead and OP_ReopenIdx opcodes can be refactored. Firstly, if schema versions at compile time and runtime don't match, finish VDBE execution with appropriate message error. Exception is the situation when fifth pointer is set to OPFLAG_FRESH_PTR, which means that space pointer has been fetched during runtime right before that. Secondly, there is no need to fetch number of columns in index from KeyInfo: iterator yields the full tuple, so it would always be equal to the number of fields in a whole space. Finally, now we always can pass space pointer to these opcodes regardless of DML routine. In case of OP_ReopenIdx opcode space and index from given cursor is checked on equality to those given in arguments. If they match, this opcode will become no-op. --- src/box/sql/opcodes.c | 6 +- src/box/sql/opcodes.h | 6 +- src/box/sql/vdbe.c | 197 +++++++++++++++++--------------------------------- 3 files changed, 71 insertions(+), 138 deletions(-) diff --git a/src/box/sql/opcodes.c b/src/box/sql/opcodes.c index 5d892094a..b149d9a4b 100644 --- a/src/box/sql/opcodes.c +++ b/src/box/sql/opcodes.c @@ -105,9 +105,9 @@ const char *sqlite3OpcodeName(int i){ /* 91 */ "TTransaction" OpHelp(""), /* 92 */ "ReadCookie" OpHelp(""), /* 93 */ "SetCookie" OpHelp(""), - /* 94 */ "ReopenIdx" OpHelp("root=P2"), - /* 95 */ "OpenRead" OpHelp("root=P2"), - /* 96 */ "OpenWrite" OpHelp("root=P2"), + /* 94 */ "ReopenIdx" OpHelp("index id = P2, space ptr = P3"), + /* 95 */ "OpenRead" OpHelp("index id = P2, space ptr = P3"), + /* 96 */ "OpenWrite" OpHelp("index id = P2, space ptr = P3"), /* 97 */ "OpenTEphemeral" OpHelp("nColumn = P2"), /* 98 */ "SorterOpen" OpHelp(""), /* 99 */ "SequenceTest" OpHelp("if (cursor[P1].ctr++) pc = P2"), diff --git a/src/box/sql/opcodes.h b/src/box/sql/opcodes.h index 762a23205..8e4d756ca 100644 --- a/src/box/sql/opcodes.h +++ b/src/box/sql/opcodes.h @@ -94,9 +94,9 @@ #define OP_TTransaction 91 #define OP_ReadCookie 92 #define OP_SetCookie 93 -#define OP_ReopenIdx 94 /* synopsis: root=P2 */ -#define OP_OpenRead 95 /* synopsis: root=P2 */ -#define OP_OpenWrite 96 /* synopsis: root=P2 */ +#define OP_ReopenIdx 94 /* synopsis: index id = P2, space ptr = P3 */ +#define OP_OpenRead 95 /* synopsis: index id = P2, space ptr = P3 */ +#define OP_OpenWrite 96 /* synopsis: index id = P2, space ptr = P3 */ #define OP_OpenTEphemeral 97 /* synopsis: nColumn = P2 */ #define OP_SorterOpen 98 #define OP_SequenceTest 99 /* synopsis: if (cursor[P1].ctr++) pc = P2 */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index d333d4177..61d8af244 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3066,86 +3066,57 @@ case OP_SetCookie: { break; } -/* Opcode: OpenRead P1 P2 * P4 P5 - * Synopsis: root=P2 +/* Opcode: OpenRead P1 P2 PЗ P4 P5 + * Synopsis: index id = P2, space ptr = P3 * - * Open a read-only cursor for the database table whose root page is - * P2 in a database file. - * Give the new cursor an identifier of P1. The P1 - * values need not be contiguous but all P1 values should be small integers. - * It is an error for P1 to be negative. + * Open a cursor for a space specified by pointer in P3 and index + * id in P2. Give the new cursor an identifier of P1. The P1 + * values need not be contiguous but all P1 values should be + * small integers. It is an error for P1 to be negative. * - * If P5!=0 then use the content of register P2 as the root page, not - * the value of P2 itself. + * The P4 value may be a pointer to a KeyInfo structure. + * If it is a pointer to a KeyInfo structure, then said structure + * defines the content and collatining sequence of the index + * being opened. Otherwise, P4 is NULL. * - * There will be a read lock on the database whenever there is an - * open cursor. If the database was unlocked prior to this instruction - * then a read lock is acquired as part of this instruction. A read - * lock allows other processes to read the database but prohibits - * any other process from modifying the database. The read lock is - * released when all cursors are closed. If this instruction attempts - * to get a read lock but fails, the script terminates with an - * SQLITE_BUSY error code. - * - * The P4 value may be either an integer (P4_INT32) or a pointer to - * a KeyInfo structure (P4_KEYINFO). If it is a pointer to a KeyInfo - * structure, then said structure defines the content and collating - * sequence of the index being opened. Otherwise, if P4 is an integer - * value, it is set to the number of columns in the table. - * - * See also: OpenWrite, ReopenIdx + * If schema has changed since compile time, VDBE ends execution + * with appropriate error message. The only exception is + * when P5 is set to OPFLAG_FRESH_PTR, which means that + * space pointer has been fetched in runtime right before + * this opcode. */ -/* Opcode: ReopenIdx P1 P2 * P4 P5 - * Synopsis: root=P2 +/* Opcode: ReopenIdx P1 P2 P3 P4 P5 + * Synopsis: index id = P2, space ptr = P3 * - * The ReopenIdx opcode works exactly like ReadOpen except that it first - * checks to see if the cursor on P1 is already open with a root page - * number of P2 and if it is this opcode becomes a no-op. In other words, - * if the cursor is already open, do not reopen it. + * The ReopenIdx opcode works exactly like ReadOpen except that + * it first checks to see if the cursor on P1 is already open + * with the same index and if it is this opcode becomes a no-op. + * In other words, if the cursor is already open, do not reopen it. * - * The ReopenIdx opcode may only be used with P5==0 and with P4 being - * a P4_KEYINFO object. - * - * See the OpenRead opcode documentation for additional information. + * The ReopenIdx opcode may only be used with P5 == 0. */ -/* Opcode: OpenWrite P1 P2 * P4 P5 - * Synopsis: root=P2 - * - * Open a read/write cursor named P1 on the table or index whose root - * page is P2. Or if P5!=0 use the content of register P2 to find the - * root page. +/* Opcode: OpenWrite P1 P2 P3 P4 P5 + * Synopsis: index id = P2, space ptr = P3 * - * The P4 value may be either an integer (P4_INT32) or a pointer to - * a KeyInfo structure (P4_KEYINFO). If it is a pointer to a KeyInfo - * structure, then said structure defines the content and collating - * sequence of the index being opened. Otherwise, if P4 is an integer - * value, it is set to the number of columns in the table, or to the - * largest index of any column of the table that is actually used. - * - * This instruction works just like OpenRead except that it opens the cursor - * in read/write mode. For a given table, there can be one or more read-only - * cursors or a single read/write cursor but not both. - * - * See also OpenRead. + * For now, OpenWrite is an alias for OpenRead. + * It exists just due legacy reasons and should be removed: + * it isn't neccessary to open cursor to make insertion or + * deletion. */ case OP_ReopenIdx: { int nField; - KeyInfo *pKeyInfo; int p2; VdbeCursor *pCur; BtCursor *pBtCur; assert(pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ); - assert(pOp->p4type==P4_KEYINFO); - /* - * FIXME: optimization temporary disabled until the whole - * OP_Open* is reworked, i.e. pointer to space is passed - * to this opcode instead of space id. - */ - /* pCur = p->apCsr[pOp->p1]; - if (pCur && pCur->pgnoRoot==(u32)pOp->p2) { + pCur = p->apCsr[pOp->p1]; + p2 = pOp->p2; + pIn3 = &aMem[pOp->p3]; + if (pCur && pCur->uc.pCursor->space == (struct space *) pIn3->u.i && + pCur->uc.pCursor->index->def->iid == SQLITE_PAGENO_TO_INDEXID(p2)) { goto open_cursor_set_hints; - } */ + } /* If the cursor is not currently open or is open on a different * index, then fall through into OP_OpenRead to force a reopen */ @@ -3153,86 +3124,48 @@ case OP_OpenRead: case OP_OpenWrite: assert(pOp->opcode==OP_OpenWrite || pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ); - - if (p->expired) { - rc = SQLITE_ABORT_ROLLBACK; + /* + * Even if schema has changed, pointer can come from + * OP_SIDtoPtr opcode, which converts space id to pointer + * during runtime. + */ + if (box_schema_version() != p->schema_ver && + pOp->p5 != OPFLAG_FRESH_PTR) { + p->expired = 1; + rc = SQLITE_ERROR; + sqlite3VdbeError(p, "schema version has changed: " \ + "need to re-compile SQL statement."); goto abort_due_to_error; } - - nField = 0; - pKeyInfo = 0; p2 = pOp->p2; - if (pOp->p5 & OPFLAG_P2ISREG) { - assert(p2>0); - assert(p2<=(p->nMem+1 - p->nCursor)); - pIn2 = &aMem[p2]; - assert(memIsValid(pIn2)); - assert((pIn2->flags & MEM_Int)!=0); - sqlite3VdbeMemIntegerify(pIn2); - p2 = (int)pIn2->u.i; - /* The p2 value always comes from a prior OP_CreateTable opcode and - * that opcode will always set the p2 value to 2 or more or else fail. - * If there were a failure, the prepared statement would have halted - * before reaching this instruction. - */ - assert(p2>=2); - } - if (pOp->p4type==P4_KEYINFO) { - if (pOp->p4.pKeyInfo) { - pKeyInfo = pOp->p4.pKeyInfo; - } else { - unsigned spaceId; - struct space *space; - const char *zName; - Table *pTab; - Index *pIdx; - /* Try to extract KeyInfo from PK if it was not passed. */ - spaceId = SQLITE_PAGENO_TO_SPACEID(p2); - space = space_by_id(spaceId); - assert(space); - - zName = space_name(space); - assert(zName); - - pTab = sqlite3HashFind(&db->pSchema->tblHash, zName); - assert(pTab); - - pIdx = sqlite3PrimaryKeyIndex(pTab); - assert(pIdx); - - pKeyInfo = sqlite3KeyInfoOfIndex(0, db, pIdx); - assert(pKeyInfo); - } - assert(pKeyInfo->db==db); - nField = pKeyInfo->nField+pKeyInfo->nXField; - } else if (pOp->p4type==P4_INT32) { - nField = pOp->p4.i; - } + pIn3 = &aMem[pOp->p3]; + /* Make sure that pointer can fit into int64_t. */ + assert(sizeof(struct space *) >= sizeof(pIn3->u.i)); + struct space *space = ((struct space *) pIn3->u.i); + assert(space != NULL); + struct index *index = space_index(space, SQLITE_PAGENO_TO_INDEXID(p2)); + assert(index != NULL); + /* + * Since Tarantool iterator yields the full tuple, + * we need a number of fields as wide as the table itself. + * Otherwise, not enough slots for row parser cache are + * allocated in VdbeCursor object. + */ + nField = space->def->field_count; assert(pOp->p1>=0); assert(nField>=0); - testcase( nField==0); /* Table with INTEGER PRIMARY KEY and nothing else */ pCur = allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL); if (pCur==0) goto no_mem; pCur->nullRow = 1; - - assert(p2 >= 1); pBtCur = pCur->uc.pCursor; - if (box_schema_version() == p->schema_ver) { - pIn3 = &aMem[pOp->p3]; - /* Make sure that 64-bit pointer can fit into int64_t. */ - assert(sizeof(pBtCur->space) >= sizeof(pIn3->u.i)); - pBtCur->space = ((struct space *) pIn3->u.i); - } else { - pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); - } - assert(pBtCur->space != NULL); - pBtCur->index = space_index(pBtCur->space, SQLITE_PAGENO_TO_INDEXID(p2)); - assert(pBtCur->index != NULL); - pBtCur->eState = CURSOR_INVALID; pBtCur->curFlags |= BTCF_TaCursor; - pCur->pKeyInfo = pKeyInfo; + pBtCur->space = space; + pBtCur->index = index; + pBtCur->eState = CURSOR_INVALID; + /* Key info still contains sorter order and collation. */ + pCur->pKeyInfo = pOp->p4.pKeyInfo; - /* open_cursor_set_hints: */ +open_cursor_set_hints: assert(OPFLAG_BULKCSR==BTREE_BULKLOAD); assert(OPFLAG_SEEKEQ==BTREE_SEEK_EQ); testcase( pOp->p5 & OPFLAG_BULKCSR); -- 2.15.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead 2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik @ 2018-03-22 14:11 ` v.shpilevoy 2018-03-23 16:39 ` n.pettik 0 siblings, 1 reply; 19+ messages in thread From: v.shpilevoy @ 2018-03-22 14:11 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik See below 3 comments. > 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а): > > After new DDL SQL implementation has been introduced, OP_OpenWrite, > OP_OpenRead and OP_ReopenIdx opcodes can be refactored. > > Firstly, if schema versions at compile time and runtime don't match, > finish VDBE execution with appropriate message error. Exception is the > situation when fifth pointer is set to OPFLAG_FRESH_PTR, which means > that space pointer has been fetched during runtime right before that. > > Secondly, there is no need to fetch number of columns in index from > KeyInfo: iterator yields the full tuple, so it would always be equal to > the number of fields in a whole space. > > Finally, now we always can pass space pointer to these opcodes > regardless of DML routine. In case of OP_ReopenIdx opcode space and > index from given cursor is checked on equality to those given in > arguments. If they match, this opcode will become no-op. > --- > src/box/sql/opcodes.c | 6 +- > src/box/sql/opcodes.h | 6 +- > src/box/sql/vdbe.c | 197 +++++++++++++++++--------------------------------- > 3 files changed, 71 insertions(+), 138 deletions(-) > > -/* Opcode: OpenRead P1 P2 * P4 P5 > - * Synopsis: root=P2 > +/* Opcode: OpenRead P1 P2 PЗ P4 P5 1. Seems like you wrote here a cyrillic letter 'З' instead of digit three. > + * Synopsis: index id = P2, space ptr = P3 > * > - * Open a read-only cursor for the database table whose root page is > - * P2 in a database file. > - * Give the new cursor an identifier of P1. The P1 > - * values need not be contiguous but all P1 values should be small integers. > - * It is an error for P1 to be negative. > + * Open a cursor for a space specified by pointer in P3 and index > + * id in P2. Give the new cursor an identifier of P1. The P1 > + * values need not be contiguous but all P1 values should be > + * small integers. It is an error for P1 to be negative. > * > - * If P5!=0 then use the content of register P2 as the root page, not > - * the value of P2 itself. > + * The P4 value may be a pointer to a KeyInfo structure. > + * If it is a pointer to a KeyInfo structure, then said structure > + * defines the content and collatining sequence of the index > + * being opened. Otherwise, P4 is NULL. > * > - * There will be a read lock on the database whenever there is an > - * open cursor. If the database was unlocked prior to this instruction > - * then a read lock is acquired as part of this instruction. A read > - * lock allows other processes to read the database but prohibits > - * any other process from modifying the database. The read lock is > - * released when all cursors are closed. If this instruction attempts > - * to get a read lock but fails, the script terminates with an > - * SQLITE_BUSY error code. > - * > - * The P4 value may be either an integer (P4_INT32) or a pointer to > - * a KeyInfo structure (P4_KEYINFO). If it is a pointer to a KeyInfo > - * structure, then said structure defines the content and collating > - * sequence of the index being opened. Otherwise, if P4 is an integer > - * value, it is set to the number of columns in the table. > - * > - * See also: OpenWrite, ReopenIdx > + * If schema has changed since compile time, VDBE ends execution > + * with appropriate error message. The only exception is > + * when P5 is set to OPFLAG_FRESH_PTR, which means that > + * space pointer has been fetched in runtime right before > + * this opcode. > */ > -/* Opcode: ReopenIdx P1 P2 * P4 P5 > - * Synopsis: root=P2 > +/* Opcode: ReopenIdx P1 P2 P3 P4 P5 > + * Synopsis: index id = P2, space ptr = P3 > * > - * The ReopenIdx opcode works exactly like ReadOpen except that it first > - * checks to see if the cursor on P1 is already open with a root page > - * number of P2 and if it is this opcode becomes a no-op. In other words, > - * if the cursor is already open, do not reopen it. > + * The ReopenIdx opcode works exactly like ReadOpen except that 2. I can not find ReadOpen - possibly it is OP_OpenRead? > > + /* > + * Since Tarantool iterator yields the full tuple, > + * we need a number of fields as wide as the table itself. > + * Otherwise, not enough slots for row parser cache are > + * allocated in VdbeCursor object. 3. Term 'yields' slightly confused. For the first moments it seems, that an iterator yields a fiber, not fields. Can you please find another word? ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead 2018-03-22 14:11 ` [tarantool-patches] " v.shpilevoy @ 2018-03-23 16:39 ` n.pettik 0 siblings, 0 replies; 19+ messages in thread From: n.pettik @ 2018-03-23 16:39 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 5046 bytes --] > On 22 Mar 2018, at 17:11, v.shpilevoy@tarantool.org wrote: > > See below 3 comments. > >> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org <mailto:korablev@tarantool.org>> написал(а): >> >> After new DDL SQL implementation has been introduced, OP_OpenWrite, >> OP_OpenRead and OP_ReopenIdx opcodes can be refactored. >> >> Firstly, if schema versions at compile time and runtime don't match, >> finish VDBE execution with appropriate message error. Exception is the >> situation when fifth pointer is set to OPFLAG_FRESH_PTR, which means >> that space pointer has been fetched during runtime right before that. >> >> Secondly, there is no need to fetch number of columns in index from >> KeyInfo: iterator yields the full tuple, so it would always be equal to >> the number of fields in a whole space. >> >> Finally, now we always can pass space pointer to these opcodes >> regardless of DML routine. In case of OP_ReopenIdx opcode space and >> index from given cursor is checked on equality to those given in >> arguments. If they match, this opcode will become no-op. >> --- >> src/box/sql/opcodes.c | 6 +- >> src/box/sql/opcodes.h | 6 +- >> src/box/sql/vdbe.c | 197 +++++++++++++++++--------------------------------- >> 3 files changed, 71 insertions(+), 138 deletions(-) >> >> -/* Opcode: OpenRead P1 P2 * P4 P5 >> - * Synopsis: root=P2 >> +/* Opcode: OpenRead P1 P2 PЗ P4 P5 > > 1. Seems like you wrote here a cyrillic letter 'З' instead of digit three. Holy sh… How did you manage to notice that? My font displays them almost identical.. I would call it ’next-level review’. Anyway thanks, fixed. > >> + * Synopsis: index id = P2, space ptr = P3 >> * >> - * Open a read-only cursor for the database table whose root page is >> - * P2 in a database file. >> - * Give the new cursor an identifier of P1. The P1 >> - * values need not be contiguous but all P1 values should be small integers. >> - * It is an error for P1 to be negative. >> + * Open a cursor for a space specified by pointer in P3 and index >> + * id in P2. Give the new cursor an identifier of P1. The P1 >> + * values need not be contiguous but all P1 values should be >> + * small integers. It is an error for P1 to be negative. >> * >> - * If P5!=0 then use the content of register P2 as the root page, not >> - * the value of P2 itself. >> + * The P4 value may be a pointer to a KeyInfo structure. >> + * If it is a pointer to a KeyInfo structure, then said structure >> + * defines the content and collatining sequence of the index >> + * being opened. Otherwise, P4 is NULL. >> * >> - * There will be a read lock on the database whenever there is an >> - * open cursor. If the database was unlocked prior to this instruction >> - * then a read lock is acquired as part of this instruction. A read >> - * lock allows other processes to read the database but prohibits >> - * any other process from modifying the database. The read lock is >> - * released when all cursors are closed. If this instruction attempts >> - * to get a read lock but fails, the script terminates with an >> - * SQLITE_BUSY error code. >> - * >> - * The P4 value may be either an integer (P4_INT32) or a pointer to >> - * a KeyInfo structure (P4_KEYINFO). If it is a pointer to a KeyInfo >> - * structure, then said structure defines the content and collating >> - * sequence of the index being opened. Otherwise, if P4 is an integer >> - * value, it is set to the number of columns in the table. >> - * >> - * See also: OpenWrite, ReopenIdx >> + * If schema has changed since compile time, VDBE ends execution >> + * with appropriate error message. The only exception is >> + * when P5 is set to OPFLAG_FRESH_PTR, which means that >> + * space pointer has been fetched in runtime right before >> + * this opcode. >> */ >> -/* Opcode: ReopenIdx P1 P2 * P4 P5 >> - * Synopsis: root=P2 >> +/* Opcode: ReopenIdx P1 P2 P3 P4 P5 >> + * Synopsis: index id = P2, space ptr = P3 >> * >> - * The ReopenIdx opcode works exactly like ReadOpen except that it first >> - * checks to see if the cursor on P1 is already open with a root page >> - * number of P2 and if it is this opcode becomes a no-op. In other words, >> - * if the cursor is already open, do not reopen it. >> + * The ReopenIdx opcode works exactly like ReadOpen except that > > 2. I can not find ReadOpen - possibly it is OP_OpenRead? Fixed. >> >> + /* >> + * Since Tarantool iterator yields the full tuple, >> + * we need a number of fields as wide as the table itself. >> + * Otherwise, not enough slots for row parser cache are >> + * allocated in VdbeCursor object. > > 3. Term 'yields' slightly confused. For the first moments it seems, that an iterator yields a fiber, not fields. Can you please find another word? Ok, replaced with ‘provides’. BTW, I didn’t come up with this word combination myself, just found it somewhere in source code... [-- Attachment #2: Type: text/html, Size: 11750 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik ` (3 preceding siblings ...) 2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik @ 2018-03-24 12:37 ` v.shpilevoy 2018-03-27 16:28 ` n.pettik 4 siblings, 1 reply; 19+ messages in thread From: v.shpilevoy @ 2018-03-24 12:37 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik See my commits with the final review fixes on you branch (you can squash them, or not - on your choice). And please fix failing tests on commit "sql: introduce opcodes to operate on system spaces". > 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а): > > Branch: https://github.com/tarantool/tarantool/tree/np/gh-3252-slit-dml-and-ddl-sql > Issue: https://github.com/tarantool/tarantool/issues/3252 > > In order to avoid additional lookups during VDBE execution, > it was suggested to pass pointer fetched at query compilation time to > opcodes which create cursors (i.e. OP_OpenRead and OP_OpenWrite). > However, any DDL routine may invalidate pointers. Thus, to operate on > system spaces (which are used during DDL) new opcodes have been introduced: > OP_SInsert and OP_SDelete. But originally, SQL DDL under hood used nested > parsing to generate code for non-trivial deletion. For instance: > "DELETE FROM _INDEX WHERE id = space_id AND iid > 0" -- to remove all > secondary indexes. To use new operands such nested parsing has been > substituted with hardcoded VDBE programs. > Finally, fresh pointer to space can also be obtained at VDBE rutime using > new opcode OP_SIDtoPtr, which converts space id to ptr and saves it to > given register. This opcode should be used when it is impossible to avoid > mixing DDL and DML routine in the same query > (e.g. CREATE TABLE AS SELECT FROM ...). > > Nikita Pettik (4): > sql: pass space pointer to OP_OpenRead/OpenWrite > sql: introduce opcodes to operate on system spaces > sql: rework code generation for DDL routine > sql: rework OP_OpenWrite/OpenRead > > src/box/sql.c | 51 ++++--- > src/box/sql/analyze.c | 17 ++- > src/box/sql/build.c | 321 ++++++++++++++++++++++--------------------- > src/box/sql/expr.c | 14 +- > src/box/sql/fkey.c | 11 +- > src/box/sql/insert.c | 37 ++++- > src/box/sql/opcodes.c | 51 +++---- > src/box/sql/opcodes.h | 59 ++++---- > src/box/sql/select.c | 11 +- > src/box/sql/sqliteInt.h | 5 + > src/box/sql/tarantoolInt.h | 11 +- > src/box/sql/trigger.c | 27 ++-- > src/box/sql/vdbe.c | 310 +++++++++++++++++++++-------------------- > src/box/sql/vdbe.h | 1 + > src/box/sql/vdbeInt.h | 1 + > src/box/sql/vdbeaux.c | 13 ++ > src/box/sql/where.c | 12 +- > test/sql/transition.result | 11 +- > test/sql/transition.test.lua | 8 +- > 19 files changed, 550 insertions(+), 421 deletions(-) > > -- > 2.15.1 > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime 2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy @ 2018-03-27 16:28 ` n.pettik 0 siblings, 0 replies; 19+ messages in thread From: n.pettik @ 2018-03-27 16:28 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches > On 24 Mar 2018, at 15:37, v.shpilevoy@tarantool.org wrote: > > See my commits with the final review fixes on you branch (you can squash them, or not - on your choice). > > And please fix failing tests on commit "sql: introduce opcodes to operate on system spaces”. Ok, thanks. I have squashed your fixes and make that commit pass tests. > >> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а): >> >> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3252-slit-dml-and-ddl-sql >> Issue: https://github.com/tarantool/tarantool/issues/3252 >> >> In order to avoid additional lookups during VDBE execution, >> it was suggested to pass pointer fetched at query compilation time to >> opcodes which create cursors (i.e. OP_OpenRead and OP_OpenWrite). >> However, any DDL routine may invalidate pointers. Thus, to operate on >> system spaces (which are used during DDL) new opcodes have been introduced: >> OP_SInsert and OP_SDelete. But originally, SQL DDL under hood used nested >> parsing to generate code for non-trivial deletion. For instance: >> "DELETE FROM _INDEX WHERE id = space_id AND iid > 0" -- to remove all >> secondary indexes. To use new operands such nested parsing has been >> substituted with hardcoded VDBE programs. >> Finally, fresh pointer to space can also be obtained at VDBE rutime using >> new opcode OP_SIDtoPtr, which converts space id to ptr and saves it to >> given register. This opcode should be used when it is impossible to avoid >> mixing DDL and DML routine in the same query >> (e.g. CREATE TABLE AS SELECT FROM ...). >> >> Nikita Pettik (4): >> sql: pass space pointer to OP_OpenRead/OpenWrite >> sql: introduce opcodes to operate on system spaces >> sql: rework code generation for DDL routine >> sql: rework OP_OpenWrite/OpenRead >> >> src/box/sql.c | 51 ++++--- >> src/box/sql/analyze.c | 17 ++- >> src/box/sql/build.c | 321 ++++++++++++++++++++++--------------------- >> src/box/sql/expr.c | 14 +- >> src/box/sql/fkey.c | 11 +- >> src/box/sql/insert.c | 37 ++++- >> src/box/sql/opcodes.c | 51 +++---- >> src/box/sql/opcodes.h | 59 ++++---- >> src/box/sql/select.c | 11 +- >> src/box/sql/sqliteInt.h | 5 + >> src/box/sql/tarantoolInt.h | 11 +- >> src/box/sql/trigger.c | 27 ++-- >> src/box/sql/vdbe.c | 310 +++++++++++++++++++++-------------------- >> src/box/sql/vdbe.h | 1 + >> src/box/sql/vdbeInt.h | 1 + >> src/box/sql/vdbeaux.c | 13 ++ >> src/box/sql/where.c | 12 +- >> test/sql/transition.result | 11 +- >> test/sql/transition.test.lua | 8 +- >> 19 files changed, 550 insertions(+), 421 deletions(-) >> >> -- >> 2.15.1 >> >> > > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-03-27 16:28 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik 2018-03-21 13:14 ` [tarantool-patches] " Kirill Yukhin 2018-03-22 10:07 ` n.pettik 2018-03-22 11:04 ` v.shpilevoy 2018-03-23 16:01 ` n.pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik 2018-03-22 11:42 ` [tarantool-patches] " v.shpilevoy 2018-03-22 12:23 ` n.pettik 2018-03-22 13:09 ` v.shpilevoy 2018-03-23 16:20 ` n.pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik 2018-03-22 13:57 ` [tarantool-patches] " v.shpilevoy 2018-03-23 16:33 ` n.pettik 2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik 2018-03-22 14:11 ` [tarantool-patches] " v.shpilevoy 2018-03-23 16:39 ` n.pettik 2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy 2018-03-27 16:28 ` n.pettik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox