* [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua
@ 2018-04-03 15:34 Vladislav Shpilevoy
2018-04-03 15:34 ` [tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] Vladislav Shpilevoy
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-03 15:34 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Vladislav Shpilevoy
Branch: http://github.com/tarantool/tarantool/tree/gh-2618-preliminary-patches
Issue: https://github.com/tarantool/tarantool/issues/2618
The patchset consists of patches preliminary to SQL IProto
"last updated tuple" introduction. It is possible, that its
implementation plan will be changed several times, but this
patchset looks useful even with no IProto "last updated tuple".
1) The patch does SQL INSERT/UPDATE refactoring by removal of 2
unused VDBE parameters, and by restyling of final insertion
opcodes generator: sqlite3CompleteInsertion.
2) The patch does refactoring of Lua SQL executor, that is
extremely overengineered now.
Vladislav Shpilevoy (4):
sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
sql: remove unused error codes, use enum instead of defines
sql: simplify lua SQL executor
sql: remove useless branching in insertOrReplace
src/box/lua/sql.c | 239 +++++++-----------------------------------------
src/box/sql.c | 18 ++--
src/box/sql/analyze.c | 8 +-
src/box/sql/build.c | 4 -
src/box/sql/delete.c | 2 +-
src/box/sql/expr.c | 5 +-
src/box/sql/insert.c | 80 ++++------------
src/box/sql/main.c | 32 -------
src/box/sql/os_unix.c | 9 +-
src/box/sql/select.c | 29 +++---
src/box/sql/sqliteInt.h | 102 +++++++++++++--------
src/box/sql/trigger.c | 3 +-
src/box/sql/update.c | 6 +-
src/box/sql/vdbe.c | 10 +-
src/box/sql/vdbeInt.h | 5 +-
src/box/sql/vdbeapi.c | 9 --
src/box/sql/vdbeaux.c | 1 -
src/box/sql/wherecode.c | 5 +-
18 files changed, 152 insertions(+), 415 deletions(-)
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
2018-04-03 15:34 [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua Vladislav Shpilevoy
@ 2018-04-03 15:34 ` Vladislav Shpilevoy
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
2018-04-03 15:34 ` [tarantool-patches] [PATCH 2/4] sql: remove unused error codes, use enum instead of defines Vladislav Shpilevoy
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-03 15:34 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Vladislav Shpilevoy
And rework sqlite3CompleteInsertion. Now it
* accepts actually unused pTab;
* array of records to insert in different indexes, but inserts
only one;
* has sqlite code style.
Code style fix is obvious. PTab is used only to check, that it is
not a view and to check, that a first index is primary:
1) caller functions already guarantee the pTab is not a view;
2) regardless of a first index nature: primary or not - it is not
used here. It is useless check. With the same success we can
check this in each function, that uses struct Table.
Array of records to insert has no sense, since insertion is being
done in a primary index only. It is enough to pass a register of
a primary index.
---
src/box/sql/analyze.c | 8 ++---
src/box/sql/delete.c | 2 +-
src/box/sql/expr.c | 5 ++--
src/box/sql/insert.c | 80 ++++++++++++-------------------------------------
src/box/sql/select.c | 29 +++++++-----------
src/box/sql/sqliteInt.h | 16 +++++++++-
src/box/sql/trigger.c | 3 +-
src/box/sql/update.c | 6 ++--
src/box/sql/vdbe.c | 9 ++----
src/box/sql/wherecode.c | 5 ++--
10 files changed, 59 insertions(+), 104 deletions(-)
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index fb46641a2..c5dcedd3e 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1085,11 +1085,9 @@ analyzeOneTable(Parse * pParse, /* Parser context */
regCol + i);
}
sqlite3VdbeAddOp3(v, OP_MakeRecord, regCol, nColTest,
- regSample);
- sqlite3VdbeAddOp3(v, OP_MakeRecord, regTabname, 6,
- regTemp);
- sqlite3VdbeAddOp2(v, OP_IdxReplace, iStatCur + 1,
- regTemp);
+ regSample);
+ sqlite3VdbeAddOp3(v, OP_MakeRecord, regTabname, 6, regTemp);
+ sqlite3VdbeAddOp2(v, OP_IdxReplace, iStatCur + 1, regTemp);
sqlite3VdbeAddOp2(v, OP_Goto, 1, addrNext); /* P1==1 for end-of-loop */
sqlite3VdbeJumpHere(v, addrIsNull);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 7d1d71577..ab8dd0bc3 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -476,7 +476,7 @@ sqlite3DeleteFrom(Parse * pParse, /* The parser context */
sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, iKey, zAff, nPk);
/* Set flag to save memory allocating one by malloc. */
sqlite3VdbeChangeP5(v, 1);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iEphCur, iKey, iPk, nPk);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iEphCur, iKey);
}
/* If this DELETE cannot use the ONEPASS strategy, this is the
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index f4e3cf735..78ebc91a5 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2911,9 +2911,8 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */
1, r2, &affinity, 1);
sqlite3ExprCacheAffinityChange(pParse,
r3, 1);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert,
- pExpr->iTable, r2,
- r3, 1);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert,
+ pExpr->iTable, r2);
}
sqlite3ReleaseTempReg(pParse, r1);
sqlite3ReleaseTempReg(pParse, r2);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 4a57b23f5..c7fe92cd6 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -744,7 +744,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
* with the first column.
*/
for (i = 0; i < pTab->nCol; i++) {
- int iRegStore = regTupleid + 1 + i;
+ int iRegStore = regData + i;
if (pColumn == 0) {
j = i;
} else {
@@ -846,7 +846,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */
and do the insertion.
*/
int isReplace; /* Set to true if constraints may cause a replace */
- int bUseSeek; /* True to use OPFLAG_SEEKRESULT */
sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
iIdxCur, regIns, 0,
ipkColumn >= 0, onError,
@@ -862,12 +861,13 @@ sqlite3Insert(Parse * pParse, /* Parser context */
* VdbeCursor.seekResult variable, disabling the OPFLAG_USESEEKRESULT
* functionality.
*/
- bUseSeek = isReplace == 0 || (pTrigger == 0 &&
- ((user_session->sql_flags &
- SQLITE_ForeignKeys) == 0 ||
- sqlite3FkReferences(pTab) == 0));
- sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
- bUseSeek, onError);
+ bool no_foreign_keys =
+ sqlite3FkReferences(pTab) == NULL ||
+ (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
+ bool use_seek = isReplace == 0 ||
+ (pTrigger == NULL && no_foreign_keys);
+ vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], use_seek,
+ onError);
}
/* Update the count of rows that are inserted
@@ -1490,69 +1490,27 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace));
}
-/*
- * This routine generates code to finish the INSERT or UPDATE operation
- * that was started by a prior call to sqlite3GenerateConstraintChecks.
- * A consecutive range of registers starting at regNewData contains the
- * tupleid and the content to be inserted.
- *
- * The arguments to this routine should be the same as corresponding
- * arguments to sqlite3GenerateConstraintChecks.
- */
void
-sqlite3CompleteInsertion(Parse * pParse, /* The parser context */
- Table * pTab, /* the table into which we are inserting */
- int iIdxCur, /* Primary index cursor */
- int *aRegIdx, /* Register used by each index. 0 for unused indices */
- int useSeekResult, /* True to set the USESEEKRESULT flag on OP_[Idx]Insert */
- u8 onError)
+vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
+ bool use_seek_result, u8 on_error)
{
- Vdbe *v; /* Prepared statements under construction */
- Index *pIdx; /* An index being inserted or updated */
- u16 pik_flags; /* flag values passed to the btree insert */
- int opcode;
-
- v = sqlite3GetVdbe(pParse);
- assert(v != 0);
- /* This table is not a VIEW */
- assert(!space_is_view(pTab));
- /*
- * The for loop which purpose in sqlite was to insert new
- * values to all indexes is replaced to inserting new
- * values only to pk in tarantool.
- */
- pIdx = pTab->pIndex;
- /* Each table have pk on top of the indexes list */
- assert(IsPrimaryKeyIndex(pIdx));
- /* Partial indexes should be implemented somewhere in tarantool
- * codebase to check it during inserting values to the pk #2626
- *
- */
- /*if( pIdx->pPartIdxWhere ){
- * sqlite3VdbeAddOp2(v, OP_IsNull, aRegIdx[i], sqlite3VdbeCurrentAddr(v)+2);
- * VdbeCoverage(v);
- *}
- */
- pik_flags = OPFLAG_NCHANGE;
- if (useSeekResult) {
+ assert(v != NULL);
+ u16 pik_flags = OPFLAG_NCHANGE;
+ if (use_seek_result)
pik_flags |= OPFLAG_USESEEKRESULT;
- }
- assert(pParse->nested == 0);
- if (onError == ON_CONFLICT_ACTION_REPLACE) {
+ int opcode;
+ if (on_error == ON_CONFLICT_ACTION_REPLACE)
opcode = OP_IdxReplace;
- } else {
+ else
opcode = OP_IdxInsert;
- }
- if (onError == ON_CONFLICT_ACTION_IGNORE) {
+ if (on_error == ON_CONFLICT_ACTION_IGNORE)
pik_flags |= OPFLAG_OE_IGNORE;
- } else if (onError == ON_CONFLICT_ACTION_FAIL) {
+ else if (on_error == ON_CONFLICT_ACTION_FAIL)
pik_flags |= OPFLAG_OE_FAIL;
- }
- sqlite3VdbeAddOp4Int(v, opcode, iIdxCur, aRegIdx[0],
- aRegIdx[0] + 1, index_column_count(pIdx));
+ sqlite3VdbeAddOp2(v, opcode, cursor_id, tuple_id);
sqlite3VdbeChangeP5(v, pik_flags);
}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index c7f87f340..95bd7d656 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -667,13 +667,11 @@ pushOntoSorter(Parse * pParse, /* Parser context */
sqlite3ExprCodeMove(pParse, regBase, regPrevKey, pSort->nOBSat);
sqlite3VdbeJumpHere(v, addrJmp);
}
- if (pSort->sortFlags & SORTFLAG_UseSorter) {
+ if (pSort->sortFlags & SORTFLAG_UseSorter)
op = OP_SorterInsert;
- } else {
+ else
op = OP_IdxInsert;
- }
- sqlite3VdbeAddOp4Int(v, op, pSort->iECursor, regRecord,
- regBase + nOBSat, nBase - nOBSat);
+ sqlite3VdbeAddOp2(v, op, pSort->iECursor, regRecord);
if (iLimit) {
int addr;
int r1 = 0;
@@ -752,7 +750,7 @@ codeDistinct(Parse * pParse, /* Parsing and code generating context */
sqlite3VdbeAddOp4Int(v, OP_Found, iTab, addrRepeat, iMem, N);
VdbeCoverage(v);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iMem, N, r1);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iTab, r1, iMem, N);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iTab, r1);
sqlite3ReleaseTempReg(pParse, r1);
}
@@ -967,8 +965,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
r1 = sqlite3GetTempReg(pParse);
sqlite3VdbeAddOp3(v, OP_MakeRecord, regResult,
nResultCol, r1);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1,
- regResult, nResultCol);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
sqlite3ReleaseTempReg(pParse, r1);
break;
}
@@ -1011,8 +1008,8 @@ selectInnerLoop(Parse * pParse, /* The parser context */
sqlite3VdbeAddOp4Int(v, OP_Found, iParm + 1,
addr, r1, 0);
VdbeCoverage(v);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm + 1,
- r1, regResult, nResultCol);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1,
+ r1);
assert(pSort == 0);
}
#endif
@@ -1068,8 +1065,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
sqlite3ExprCacheAffinityChange(pParse,
regResult,
nResultCol);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1,
- regResult, nResultCol);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
sqlite3ReleaseTempReg(pParse, r1);
}
break;
@@ -1165,8 +1161,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
}
sqlite3VdbeAddOp2(v, OP_Sequence, iParm, r2 + nKey);
sqlite3VdbeAddOp3(v, OP_MakeRecord, r2, nKey + 2, r1);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1, r2,
- nKey + 2);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
if (addrTest)
sqlite3VdbeJumpHere(v, addrTest);
sqlite3ReleaseTempReg(pParse, r1);
@@ -1518,8 +1513,7 @@ generateSortTail(Parse * pParse, /* Parsing context */
sqlite3VdbeAddOp4(v, OP_MakeRecord, regRow, nColumn,
regTupleid, pDest->zAffSdst, nColumn);
sqlite3ExprCacheAffinityChange(pParse, regRow, nColumn);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, regTupleid,
- regRow, nColumn);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, regTupleid);
break;
}
case SRT_Mem:{
@@ -2982,8 +2976,7 @@ generateOutputSubroutine(Parse * pParse, /* Parsing context */
pIn->nSdst);
sqlite3ExprCacheAffinityChange(pParse, pIn->iSdst,
pIn->nSdst);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, pDest->iSDParm,
- r1, pIn->iSdst, pIn->nSdst);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, pDest->iSDParm, r1);
sqlite3ReleaseTempReg(pParse, r1);
break;
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f43711a7f..e786cf842 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3715,7 +3715,21 @@ int sqlite3GenerateIndexKey(Parse *, Index *, int, int, int *, Index *, int);
void sqlite3ResolvePartIdxLabel(Parse *, int);
void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
int, u8, u8, int, int *, int *);
-void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8);
+/**
+ * This routine generates code to finish the INSERT or UPDATE
+ * operation that was started by a prior call to
+ * sqlite3GenerateConstraintChecks.
+ * @param v Virtual database engine.
+ * @param cursor_id Primary index cursor.
+ * @param tuple_id Register with data to insert.
+ * @param use_seek_result True to set the USESEEKRESULT flag on
+ * OP_[Idx]Insert.
+ * @param on_error Error action on failed insert/replace.
+ */
+void
+vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
+ bool use_seek_result, u8 on_error);
+
int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
int *, u8, u8);
void sqlite3BeginWriteOperation(Parse *, int);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 859b421db..eaa08eb4c 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -281,8 +281,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 1,
MSGPACK_SUBTYPE, zOpts, P4_DYNAMIC);
sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 2, iRecord);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, iRecord,
- iFirstCol, 7);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iCursor, iRecord);
/* Do not account nested operations: the count of such
* operations depends on Tarantool data dictionary internals,
* such as data layout in system spaces.
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 7727ae5d5..859189a7e 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -377,8 +377,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
sqlite3IndexAffinityStr(pParse->db, pPk);
sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey,
zAff, nPk);
- sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iEph, regKey, iPk,
- nPk);
+ sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey);
/* Set flag to save memory allocating one by malloc. */
sqlite3VdbeChangeP5(v, 1);
}
@@ -616,7 +615,8 @@ sqlite3Update(Parse * pParse, /* The parser context */
}
/* Insert the new index entries and the new record. */
- sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0, onError);
+ vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], false,
+ onError);
/* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
* handle rows (possibly in other tables) that refer via a foreign key
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 329d46886..bf8a27709 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4318,18 +4318,13 @@ case OP_Next: /* jump */
goto check_for_interrupt;
}
-/* Opcode: IdxInsert P1 P2 P3 P4 P5
+/* Opcode: IdxInsert P1 P2 * * P5
* Synopsis: key=r[P2]
*
* Register P2 holds an SQL index key made using the
* MakeRecord instructions. This opcode writes that key
* into the index P1. Data for the entry is nil.
*
- * If P4 is not zero, then it is the number of values in the unpacked
- * key of reg(P2). In that case, P3 is the index of the first register
- * for the unpacked key. The availability of the unpacked key can sometimes
- * be an optimization.
- *
* If P5 has the OPFLAG_APPEND bit set, that is a hint to the b-tree layer
* that this insert is likely to be an append.
*
@@ -4345,7 +4340,7 @@ case OP_Next: /* jump */
*
* This instruction only works for indices.
*/
-/* Opcode: IdxReplace P1 P2 P3 P4 P5
+/* Opcode: IdxReplace P1 P2 * * P5
* Synopsis: key=r[P2]
*
* This opcode works exactly as IdxInsert does, but in Tarantool
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 8b06baeff..a3c51eef4 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1763,10 +1763,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
sqlite3VdbeAddOp3
(v, OP_MakeRecord,
r, nPk, regPk);
- sqlite3VdbeAddOp4Int
+ sqlite3VdbeAddOp2
(v, OP_IdxInsert,
- regRowset, regPk,
- r, nPk);
+ regRowset, regPk);
if (iSet)
sqlite3VdbeChangeP5
(v,
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH 2/4] sql: remove unused error codes, use enum instead of defines
2018-04-03 15:34 [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua Vladislav Shpilevoy
2018-04-03 15:34 ` [tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] Vladislav Shpilevoy
@ 2018-04-03 15:34 ` Vladislav Shpilevoy
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
2018-04-03 15:34 ` [tarantool-patches] [PATCH 3/4] sql: simplify lua SQL executor Vladislav Shpilevoy
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-03 15:34 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Vladislav Shpilevoy
---
src/box/sql/build.c | 4 ---
src/box/sql/main.c | 32 ------------------
src/box/sql/os_unix.c | 9 +-----
src/box/sql/sqliteInt.h | 86 ++++++++++++++++++++++++++++---------------------
src/box/sql/vdbe.c | 1 -
src/box/sql/vdbeInt.h | 5 +--
src/box/sql/vdbeapi.c | 9 ------
src/box/sql/vdbeaux.c | 1 -
8 files changed, 52 insertions(+), 95 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5e3ed0f39..4510df206 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -105,10 +105,6 @@ sqlite3FinishCoding(Parse * pParse)
if (db->init.busy == 0)
sqlite3VdbeChangeP5(v, 1);
-
- VdbeComment((v, "usesStmtJournal=%d",
- pParse->mayAbort
- && pParse->isMultiWrite));
}
/* Code constant expressions that where factored out of inner loops */
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index b77348c21..8e898178a 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -800,9 +800,6 @@ sqlite3ErrName(int rc)
case SQLITE_ERROR:
zName = "SQLITE_ERROR";
break;
- case SQLITE_INTERNAL:
- zName = "SQLITE_INTERNAL";
- break;
case SQLITE_PERM:
zName = "SQLITE_PERM";
break;
@@ -821,9 +818,6 @@ sqlite3ErrName(int rc)
case SQLITE_NOMEM:
zName = "SQLITE_NOMEM";
break;
- case SQLITE_READONLY:
- zName = "SQLITE_READONLY";
- break;
case SQLITE_INTERRUPT:
zName = "SQLITE_INTERRUPT";
break;
@@ -917,12 +911,6 @@ sqlite3ErrName(int rc)
case SQLITE_CANTOPEN:
zName = "SQLITE_CANTOPEN";
break;
- case SQLITE_PROTOCOL:
- zName = "SQLITE_PROTOCOL";
- break;
- case SQLITE_EMPTY:
- zName = "SQLITE_EMPTY";
- break;
case SQLITE_SCHEMA:
zName = "SQLITE_SCHEMA";
break;
@@ -959,27 +947,15 @@ sqlite3ErrName(int rc)
case SQLITE_MISUSE:
zName = "SQLITE_MISUSE";
break;
- case SQLITE_NOLFS:
- zName = "SQLITE_NOLFS";
- break;
- case SQLITE_FORMAT:
- zName = "SQLITE_FORMAT";
- break;
case SQLITE_RANGE:
zName = "SQLITE_RANGE";
break;
- case SQLITE_NOTADB:
- zName = "SQLITE_NOTADB";
- break;
case SQL_TARANTOOL_ERROR:
zName = "SQLITE_TARANTOOL_ERROR";
break;
case SQLITE_ROW:
zName = "SQLITE_ROW";
break;
- case SQLITE_NOTICE:
- zName = "SQLITE_NOTICE";
- break;
case SQLITE_WARNING:
zName = "SQLITE_WARNING";
break;
@@ -1008,32 +984,24 @@ sqlite3ErrStr(int rc)
static const char *const aMsg[] = {
/* SQLITE_OK */ "not an error",
/* SQLITE_ERROR */ "SQL logic error or missing database",
- /* SQLITE_INTERNAL */ 0,
/* SQLITE_PERM */ "access permission denied",
/* SQLITE_ABORT */ "callback requested query abort",
/* SQLITE_BUSY */ "database is locked",
/* SQLITE_LOCKED */ "database table is locked",
/* SQLITE_NOMEM */ "out of memory",
- /* SQLITE_READONLY */ "attempt to write a readonly database",
/* SQLITE_INTERRUPT */ "interrupted",
/* SQLITE_IOERR */ "disk I/O error",
/* SQLITE_CORRUPT */ "database disk image is malformed",
/* SQLITE_NOTFOUND */ "unknown operation",
/* SQLITE_FULL */ "database or disk is full",
/* SQLITE_CANTOPEN */ "unable to open database file",
- /* SQLITE_PROTOCOL */ "locking protocol",
- /* SQLITE_EMPTY */ "table contains no data",
/* SQLITE_SCHEMA */ "database schema has changed",
/* SQLITE_TOOBIG */ "string or blob too big",
/* SQLITE_CONSTRAINT */ "constraint failed",
/* SQLITE_MISMATCH */ "datatype mismatch",
/* SQLITE_MISUSE */
"library routine called out of sequence",
- /* SQLITE_NOLFS */ "large file support is disabled",
- /* SQLITE_FORMAT */ "auxiliary database format error",
/* SQLITE_RANGE */ "bind or column index out of range",
- /* SQLITE_NOTADB */
- "file is encrypted or is not a database",
/* SQL_TARANTOOL_ITERATOR_FAIL */ "Tarantool's iterator failed",
/* SQL_TARANTOOL_INSERT_FAIL */ "Tarantool's insert failed",
/* SQL_TARANTOOL_DELETE_FAIL */ "Tarantool's delete failed",
diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
index 8dce6f0c2..9cb8a54d0 100644
--- a/src/box/sql/os_unix.c
+++ b/src/box/sql/os_unix.c
@@ -1011,10 +1011,6 @@ findInodeInfo(unixFile * pFile, /* Unix file with file desc used in the key */
rc = osFstat(fd, &statbuf);
if (rc != 0) {
storeLastErrno(pFile, errno);
-#if defined(EOVERFLOW) && defined(SQLITE_DISABLE_LFS)
- if (pFile->lastErrno == EOVERFLOW)
- return SQLITE_NOLFS;
-#endif
return SQLITE_IOERR;
}
#ifdef __APPLE__
@@ -6011,10 +6007,7 @@ proxyFileControl(sqlite3_file * id, int op, void *pArg)
int isProxyStyle = (pFile->pMethod == &proxyIoMethods);
if (pArg == NULL || (const char *)pArg == 0) {
if (isProxyStyle) {
- /* turn off proxy locking - not supported. */
- rc = SQLITE_ERROR
- /*SQLITE_PROTOCOL? SQLITE_MISUSE? */
- ;
+ rc = SQLITE_ERROR;
} else {
/* turn off proxy locking - already off - NOOP */
rc = SQLITE_OK;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index e786cf842..f78a93a71 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -405,42 +405,56 @@ struct sqlite3_vfs {
#define SQLITE_LIMIT_TRIGGER_DEPTH 9
#define SQLITE_LIMIT_WORKER_THREADS 10
-#define SQLITE_OK 0 /* Successful result */
-/* beginning-of-error-codes */
-#define SQLITE_ERROR 1 /* SQL error or missing database */
-#define SQLITE_INTERNAL 2 /* Internal logic error in SQLite */
-#define SQLITE_PERM 3 /* Access permission denied */
-#define SQLITE_ABORT 4 /* Callback routine requested an abort */
-#define SQLITE_BUSY 5 /* The database file is locked */
-#define SQLITE_LOCKED 6 /* A table in the database is locked */
-#define SQLITE_NOMEM 7 /* A malloc() failed */
-#define SQLITE_READONLY 8 /* Attempt to write a readonly database */
-#define SQLITE_INTERRUPT 9 /* Operation terminated by sqlite3_interrupt() */
-#define SQLITE_IOERR 10 /* Some kind of disk I/O error occurred */
-#define SQLITE_CORRUPT 11 /* The database disk image is malformed */
-#define SQLITE_NOTFOUND 12 /* Unknown opcode in sqlite3_file_control() */
-#define SQLITE_FULL 13 /* Insertion failed because database is full */
-#define SQLITE_CANTOPEN 14 /* Unable to open the database file */
-#define SQLITE_PROTOCOL 15 /* Database lock protocol error */
-#define SQLITE_EMPTY 16 /* Database is empty */
-#define SQLITE_SCHEMA 17 /* The database schema changed */
-#define SQLITE_TOOBIG 18 /* String or BLOB exceeds size limit */
-#define SQLITE_CONSTRAINT 19 /* Abort due to constraint violation */
-#define SQLITE_MISMATCH 20 /* Data type mismatch */
-#define SQLITE_MISUSE 21 /* Library used incorrectly */
-#define SQLITE_NOLFS 22 /* Uses OS features not supported on host */
-#define SQLITE_FORMAT 23 /* Auxiliary database format error */
-#define SQLITE_RANGE 24 /* 2nd parameter to sqlite3_bind out of range */
-#define SQLITE_NOTADB 25 /* File opened that is not a database file */
-#define SQL_TARANTOOL_ITERATOR_FAIL 26
-#define SQL_TARANTOOL_INSERT_FAIL 27
-#define SQL_TARANTOOL_DELETE_FAIL 28
-#define SQL_TARANTOOL_ERROR 29
-#define SQLITE_NOTICE 31 /* Notifications from sqlite3_log() */
-#define SQLITE_WARNING 32 /* Warnings from sqlite3_log() */
-#define SQLITE_ROW 100 /* sqlite3_step() has another row ready */
-#define SQLITE_DONE 101 /* sqlite3_step() has finished executing */
-/* end-of-error-codes */
+enum sql_ret_code {
+ /** Result of a routine is ok. */
+ SQLITE_OK = 0,
+ /** Common error code. */
+ SQLITE_ERROR,
+ /** Access permission denied. */
+ SQLITE_PERM,
+ /** Callback routine requested an abort. */
+ SQLITE_ABORT,
+ /** The database file is locked. */
+ SQLITE_BUSY,
+ /** A table in the database is locked. */
+ SQLITE_LOCKED,
+ /** A malloc() failed. */
+ SQLITE_NOMEM,
+ /** Operation terminated by sqlite3_interrupt(). */
+ SQLITE_INTERRUPT,
+ /** Some kind of disk I/O error occurred. */
+ SQLITE_IOERR,
+ /** The database disk image is malformed. */
+ SQLITE_CORRUPT,
+ /** Unknown opcode in sqlite3_file_control(). */
+ SQLITE_NOTFOUND,
+ /** Insertion failed because database is full. */
+ SQLITE_FULL,
+ /** Unable to open the database file. */
+ SQLITE_CANTOPEN,
+ /** The database schema changed. */
+ SQLITE_SCHEMA,
+ /** String or BLOB exceeds size limit. */
+ SQLITE_TOOBIG,
+ /** Abort due to constraint violation. */
+ SQLITE_CONSTRAINT,
+ /** Data type mismatch. */
+ SQLITE_MISMATCH,
+ /** Library used incorrectly. */
+ SQLITE_MISUSE,
+ /** 2nd parameter to sqlite3_bind out of range. */
+ SQLITE_RANGE,
+ SQL_TARANTOOL_ITERATOR_FAIL,
+ SQL_TARANTOOL_INSERT_FAIL,
+ SQL_TARANTOOL_DELETE_FAIL,
+ SQL_TARANTOOL_ERROR,
+ /** Warnings from sqlite3_log(). */
+ SQLITE_WARNING,
+ /** sqlite3_step() has another row ready. */
+ SQLITE_ROW,
+ /** sqlite3_step() has finished executing. */
+ SQLITE_DONE,
+};
void *
sqlite3_malloc(int);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bf8a27709..1685c27ce 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1419,7 +1419,6 @@ case OP_ResultRow: {
*/
if (SQLITE_OK!=(rc = sqlite3VdbeCheckFk(p, 0))) {
assert(user_session->sql_flags&SQLITE_CountRows);
- assert(p->usesStmtJournal);
goto abort_due_to_error;
}
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index fcb45c8a8..f8f4e566b 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -418,9 +418,6 @@ struct Vdbe {
i64 startTime; /* Time when query started - used for profiling */
#endif
int nOp; /* Number of instructions in the program */
-#ifdef SQLITE_DEBUG
- int rcApp; /* errcode set by sqlite3_result_error_code() */
-#endif
u16 nResColumn; /* Number of columns in one row of the result set */
u8 errorAction; /* Recovery action to do in case of an error */
bft expired:1; /* True if the VM needs to be recompiled */
@@ -428,7 +425,6 @@ struct Vdbe {
bft explain:2; /* True if EXPLAIN present on SQL command */
bft changeCntOn:1; /* True to update the change-counter */
bft runOnlyOnce:1; /* Automatically expire on reset */
- bft usesStmtJournal:1; /* True if uses a statement journal */
bft isPrepareV2:1; /* True if prepared with prepare_v2() */
u32 aCounter[5]; /* Counters used by sqlite3_stmt_status() */
char *zSql; /* Text of the SQL statement that generated this */
@@ -446,6 +442,7 @@ struct Vdbe {
int nScan; /* Entries in aScan[] */
ScanStatus *aScan; /* Scan definitions for sqlite3_stmt_scanstatus() */
#endif
+
};
/*
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index dff1e1697..4c6e90c64 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -469,10 +469,6 @@ sqlite3_result_error_code(sqlite3_context * pCtx, int errCode)
{
pCtx->isError = errCode;
pCtx->fErrorOrAux = 1;
-#ifdef SQLITE_DEBUG
- if (pCtx->pVdbe)
- pCtx->pVdbe->rcApp = errCode;
-#endif
if (pCtx->pOut->flags & MEM_Null) {
sqlite3VdbeMemSetStr(pCtx->pOut, sqlite3ErrStr(errCode), -1, 1,
SQLITE_STATIC);
@@ -580,9 +576,6 @@ sqlite3Step(Vdbe * p)
db->nVdbeActive++;
p->pc = 0;
}
-#ifdef SQLITE_DEBUG
- p->rcApp = SQLITE_OK;
-#endif
#ifndef SQLITE_OMIT_EXPLAIN
if (p->explain) {
rc = sqlite3VdbeList(p);
@@ -614,8 +607,6 @@ sqlite3Step(Vdbe * p)
*/
assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR
|| (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE);
- assert((p->rc != SQLITE_ROW && p->rc != SQLITE_DONE)
- || p->rc == p->rcApp);
if (p->isPrepareV2 && rc != SQLITE_ROW && rc != SQLITE_DONE) {
/* If this statement was prepared using sqlite3_prepare_v2(), and an
* error has occurred, then return the error code in p->rc to the
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e4335524a..bb121a318 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2148,7 +2148,6 @@ sqlite3VdbeMakeReady(Vdbe * p, /* The VDBE */
assert(EIGHT_BYTE_ALIGNMENT(&x.pSpace[x.nFree]));
resolveP2Values(p, &nArg);
- p->usesStmtJournal = (u8) (pParse->isMultiWrite && pParse->mayAbort);
if (pParse->explain && nMem < 10) {
nMem = 10;
}
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH 3/4] sql: simplify lua SQL executor
2018-04-03 15:34 [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua Vladislav Shpilevoy
2018-04-03 15:34 ` [tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] Vladislav Shpilevoy
2018-04-03 15:34 ` [tarantool-patches] [PATCH 2/4] sql: remove unused error codes, use enum instead of defines Vladislav Shpilevoy
@ 2018-04-03 15:34 ` Vladislav Shpilevoy
2018-04-03 23:02 ` [tarantool-patches] " n.pettik
2018-04-03 15:34 ` [tarantool-patches] [PATCH 4/4] sql: remove useless branching in insertOrReplace Vladislav Shpilevoy
2018-04-05 11:43 ` [tarantool-patches] Re: [PATCH 0/4] sql: refactor insertion and lua Kirill Yukhin
4 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-03 15:34 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Vladislav Shpilevoy
1) Code is completely unreadable and complicated by strange
optimizations;
2) It still supposes, that there can be multiple statements in a
single string;
3) It pushes first letter of affinity together with column names
- why? It is not used anywhere.
Lets remove prepared statements list, affinity first letters
pushing.
---
src/box/lua/sql.c | 239 ++++++++----------------------------------------------
1 file changed, 35 insertions(+), 204 deletions(-)
diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
index 12b81383d..e693aeaad 100644
--- a/src/box/lua/sql.c
+++ b/src/box/lua/sql.c
@@ -6,132 +6,12 @@
#include "lua/utils.h"
#include "info.h"
-struct prep_stmt
-{
- sqlite3_stmt *stmt;
-};
-
-struct prep_stmt_list
-{
- uint8_t *mem_end; /* denotes actual size of sql_ctx struct */
- uint32_t pool_size; /* mem at the end used for aux allocations;
- pool grows from mem_end
- towards stmt[] array */
- uint32_t last_select_stmt_index; /* UINT32_MAX if no selects */
- uint32_t column_count; /* in last select stmt */
- uint32_t stmt_count;
- struct prep_stmt stmt[6]; /* overlayed with the mem pool;
- actual size could be larger or smaller */
- /* uint8_t mem_pool[] */
-};
-
-static inline int
-prep_stmt_list_needs_free(struct prep_stmt_list *l)
-{
- return (uint8_t *)(l + 1) != l->mem_end;
-}
-
-/* Release resources and free the list itself, unless it was preallocated
- * (i.e. l points to an automatic variable) */
-static void
-prep_stmt_list_free(struct prep_stmt_list *l)
-{
- if (l == NULL)
- return;
- for (size_t i = 0, n = l->stmt_count; i < n; i++)
- sqlite3_finalize(l->stmt[i].stmt);
- if (prep_stmt_list_needs_free(l))
- free(l);
-}
-
-static struct prep_stmt_list *
-prep_stmt_list_init(struct prep_stmt_list *prealloc)
-{
- prealloc->mem_end = (uint8_t *)(prealloc + 1);
- prealloc->pool_size = 0;
- prealloc->last_select_stmt_index = UINT32_MAX;
- prealloc->column_count = 0;
- prealloc->stmt_count = 0;
- return prealloc;
-}
-
-/* Allocate mem from the prep_stmt_list pool.
- * If not enough space is available, reallocates the list.
- * If reallocation is needed but l was preallocated, old mem is left
- * intact and a new memory chunk is allocated. */
-static void *
-prep_stmt_list_palloc(struct prep_stmt_list **pl,
- size_t size, size_t alignment)
-{
- assert((alignment & (alignment - 1)) == 0); /* 2 ^ k */
- assert(alignment <= __alignof__((*pl)->stmt[0]));
-
- struct prep_stmt_list *l = *pl;
- uint32_t pool_size = l->pool_size;
- uint32_t pool_size_max = (uint32_t)(
- l->mem_end - (uint8_t *)(l->stmt + l->stmt_count)
- );
-
- assert(UINT32_MAX - pool_size >= size);
- pool_size += size;
-
- assert(UINT32_MAX - pool_size >= alignment - 1);
- pool_size += alignment - 1;
- pool_size &= ~(alignment - 1);
-
- if (pool_size > pool_size_max) {
- size_t prev_size = l->mem_end - (uint8_t *)l;
- size_t size = prev_size;
- while (size < prev_size + (pool_size - pool_size_max)) {
- assert(SIZE_MAX - size >= size);
- size += size;
- }
- if (prep_stmt_list_needs_free(l)) {
- l = realloc(l, size);
- if (l == NULL)
- return NULL;
- } else {
- l = malloc(size);
- if (l == NULL)
- return NULL;
- memcpy(l, *pl, prev_size);
- }
- l->mem_end = (uint8_t *)l + size;
- /* move the pool data */
- memmove((uint8_t *)l + prev_size - l->pool_size,
- l->mem_end - l->pool_size,
- l->pool_size);
- *pl = l;
- }
-
- l->pool_size = pool_size;
- return l->mem_end - pool_size;
-}
-
-/* push new stmt; reallocate memory if needed
- * returns a pointer to the new stmt or NULL if out of memory.
- * If reallocation is needed but l was preallocated, old mem is left
- * intact and a new memory chunk is allocated. */
-static struct prep_stmt *
-prep_stmt_list_push(struct prep_stmt_list **pl)
-{
- struct prep_stmt_list *l;
- /* make sure we don't collide with the pool */
- if (prep_stmt_list_palloc(pl, sizeof(l->stmt[0]), 1
- ) == NULL)
- return NULL;
- l = *pl;
- l->pool_size -= sizeof(l->stmt[0]);
- return l->stmt + (l->stmt_count++);
-}
-
static void
-lua_push_column_names(struct lua_State *L, struct prep_stmt_list *l)
+lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
{
- sqlite3_stmt *stmt = l->stmt[l->last_select_stmt_index].stmt;
- int n = l->column_count;
- lua_createtable(L, n, 0);
- for (int i = 0; i < n; i++) {
+ int column_count = sqlite3_column_count(stmt);
+ lua_createtable(L, column_count, 0);
+ for (int i = 0; i < column_count; i++) {
const char *name = sqlite3_column_name(stmt, i);
lua_pushstring(L, name == NULL ? "" : name);
lua_rawseti(L, -2, i+1);
@@ -139,11 +19,9 @@ lua_push_column_names(struct lua_State *L, struct prep_stmt_list *l)
}
static void
-lua_push_row(struct lua_State *L, struct prep_stmt_list *l)
+lua_push_row(struct lua_State *L, struct sqlite3_stmt *stmt)
{
- sqlite3_stmt *stmt = l->stmt[l->last_select_stmt_index].stmt;
- int column_count = l->column_count;
- char *typestr = (void *)(l->mem_end - column_count);
+ int column_count = sqlite3_column_count(stmt);
lua_createtable(L, column_count, 0);
lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_array_metatable_ref);
@@ -153,124 +31,77 @@ lua_push_row(struct lua_State *L, struct prep_stmt_list *l)
int type = sqlite3_column_type(stmt, i);
switch (type) {
case SQLITE_INTEGER:
- typestr[i] = 'i';
- luaL_pushint64(L, sqlite3_column_int64(stmt, i));
+ luaL_pushint64(L, sqlite3_column_int64(stmt, i));
break;
case SQLITE_FLOAT:
- typestr[i] = 'f';
lua_pushnumber(L, sqlite3_column_double(stmt, i));
break;
case SQLITE_TEXT: {
const void *text = sqlite3_column_text(stmt, i);
- typestr[i] = 's';
lua_pushlstring(L, text,
sqlite3_column_bytes(stmt, i));
break;
}
case SQLITE_BLOB: {
const void *blob = sqlite3_column_blob(stmt, i);
- typestr[i] = 'b';
lua_pushlstring(L, blob,
sqlite3_column_bytes(stmt, i));
break;
}
case SQLITE_NULL:
- typestr[i] = '-';
lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_nil_ref);
break;
default:
- typestr[i] = '?';
assert(0);
}
lua_rawseti(L, -2, i+1);
}
-
- lua_pushlstring(L, typestr, column_count);
- lua_rawseti(L, -2, 0);
}
static int
lua_sql_execute(struct lua_State *L)
{
- int rc;
sqlite3 *db = sql_get();
- struct prep_stmt_list *l = NULL, stock_l;
- size_t length;
- const char *sql, *sql_end;
-
if (db == NULL)
return luaL_error(L, "not ready");
- sql = lua_tolstring(L, 1, &length);
+ size_t length;
+ const char *sql = lua_tolstring(L, 1, &length);
if (sql == NULL)
return luaL_error(L, "usage: box.sql.execute(sqlstring)");
- assert(length <= INT_MAX);
- sql_end = sql + length;
+ struct sqlite3_stmt *stmt;
+ if (sqlite3_prepare_v2(db, sql, length, &stmt, &sql) != SQLITE_OK)
+ goto sqlerror;
+ assert(stmt != NULL);
- l = prep_stmt_list_init(&stock_l);
- do {
-
- struct prep_stmt *ps = prep_stmt_list_push(&l);
- if (ps == NULL)
- goto outofmem;
- rc = sqlite3_prepare_v2(db, sql, (int)(sql_end - sql),
- &ps->stmt, &sql);
- if (rc != SQLITE_OK)
- goto sqlerror;
-
- if (ps->stmt == NULL) {
- /* only whitespace */
- assert(sql == sql_end);
- l->stmt_count --;
- break;
- }
-
- int column_count = sqlite3_column_count(ps->stmt);
- if (column_count == 0) {
- while ((rc = sqlite3_step(ps->stmt)) == SQLITE_ROW) { ; }
- } else {
- char *typestr;
- l->column_count = column_count;
- l->last_select_stmt_index = l->stmt_count - 1;
-
- assert(l->pool_size == 0);
- /* This might possibly call realloc() and ruin *ps. */
- typestr = prep_stmt_list_palloc(&l, column_count, 1);
- if (typestr == NULL)
- goto outofmem;
- /* Refill *ps. */
- ps = l->stmt + l->stmt_count - 1;
-
- lua_settop(L, 1); /* discard any results */
-
- /* create result table */
- lua_createtable(L, 7, 0);
- lua_pushvalue(L, lua_upvalueindex(1));
- lua_setmetatable(L, -2);
- lua_push_column_names(L, l);
- lua_rawseti(L, -2, 0);
-
- int row_count = 0;
- while ((rc = sqlite3_step(ps->stmt)) == SQLITE_ROW) {
- lua_push_row(L, l);
- row_count++;
- lua_rawseti(L, -2, row_count);
- }
- l->pool_size = 0;
+ int rc;
+ int retval_count;
+ if (sqlite3_column_count(stmt) == 0) {
+ while ((rc = sqlite3_step(stmt)) == SQLITE_ROW);
+ retval_count = 0;
+ } else {
+ lua_newtable(L);
+ lua_pushvalue(L, lua_upvalueindex(1));
+ lua_setmetatable(L, -2);
+ lua_push_column_names(L, stmt);
+ lua_rawseti(L, -2, 0);
+
+ int row_count = 0;
+ while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
+ lua_push_row(L, stmt);
+ lua_rawseti(L, -2, ++row_count);
}
+ retval_count = 1;
+ }
if (rc != SQLITE_OK && rc != SQLITE_DONE)
- goto sqlerror;
- } while (sql != sql_end);
- prep_stmt_list_free(l);
- return lua_gettop(L) - 1;
+ goto sqlerror;
+ sqlite3_finalize(stmt);
+ return retval_count;
sqlerror:
lua_pushstring(L, sqlite3_errmsg(db));
- prep_stmt_list_free(l);
+ sqlite3_finalize(stmt);
return lua_error(L);
-outofmem:
- prep_stmt_list_free(l);
- return luaL_error(L, "out of memory");
}
static int
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] [PATCH 4/4] sql: remove useless branching in insertOrReplace
2018-04-03 15:34 [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua Vladislav Shpilevoy
` (2 preceding siblings ...)
2018-04-03 15:34 ` [tarantool-patches] [PATCH 3/4] sql: simplify lua SQL executor Vladislav Shpilevoy
@ 2018-04-03 15:34 ` Vladislav Shpilevoy
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
2018-04-05 11:43 ` [tarantool-patches] Re: [PATCH 0/4] sql: refactor insertion and lua Kirill Yukhin
4 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-03 15:34 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Vladislav Shpilevoy
Type of an operation for struct request can be passed directly
with no "proxying" by special codes.
---
src/box/sql.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/box/sql.c b/src/box/sql.c
index c2577abef..a6713f1f0 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -474,35 +474,29 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
return SQLITE_OK;
}
-static int insertOrReplace(BtCursor *pCur, int operationType)
+static inline int
+insertOrReplace(BtCursor *pCur, enum iproto_type type)
{
assert(pCur->curFlags & BTCF_TaCursor);
- assert(operationType == TARANTOOL_INDEX_INSERT ||
- operationType == TARANTOOL_INDEX_REPLACE);
-
struct request request;
memset(&request, 0, sizeof(request));
request.tuple = pCur->key;
request.tuple_end = pCur->key + pCur->nKey;
request.space_id = pCur->space->def->id;
+ request.type = type;
mp_tuple_assert(request.tuple, request.tuple_end);
- if (operationType == TARANTOOL_INDEX_INSERT) {
- request.type = IPROTO_INSERT;
- } else {
- request.type = IPROTO_REPLACE;
- }
int rc = box_process_rw(&request, pCur->space, NULL);
- return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;;
+ return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;
}
int tarantoolSqlite3Insert(BtCursor *pCur)
{
- return insertOrReplace(pCur, TARANTOOL_INDEX_INSERT);
+ return insertOrReplace(pCur, IPROTO_INSERT);
}
int tarantoolSqlite3Replace(BtCursor *pCur)
{
- return insertOrReplace(pCur, TARANTOOL_INDEX_REPLACE);
+ return insertOrReplace(pCur, IPROTO_REPLACE);
}
/*
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: remove unused error codes, use enum instead of defines
2018-04-03 15:34 ` [tarantool-patches] [PATCH 2/4] sql: remove unused error codes, use enum instead of defines Vladislav Shpilevoy
@ 2018-04-03 23:01 ` n.pettik
2018-04-04 10:26 ` Vladislav Shpilevoy
0 siblings, 1 reply; 15+ messages in thread
From: n.pettik @ 2018-04-03 23:01 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Your subject doesn’t fit into 50 lines and consists from two parts.
Maybe better to move it to commit message?
2 minor comments:
> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> ---
> src/box/sql/build.c | 4 ---
> src/box/sql/main.c | 32 ------------------
> src/box/sql/os_unix.c | 9 +-----
> src/box/sql/sqliteInt.h | 86 ++++++++++++++++++++++++++++---------------------
> src/box/sql/vdbe.c | 1 -
> src/box/sql/vdbeInt.h | 5 +--
> src/box/sql/vdbeapi.c | 9 ------
> src/box/sql/vdbeaux.c | 1 -
> 8 files changed, 52 insertions(+), 95 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 5e3ed0f39..4510df206 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -105,10 +105,6 @@ sqlite3FinishCoding(Parse * pParse)
>
> if (db->init.busy == 0)
> sqlite3VdbeChangeP5(v, 1);
> -
> - VdbeComment((v, "usesStmtJournal=%d",
> - pParse->mayAbort
> - && pParse->isMultiWrite));
> }
>
> /* Code constant expressions that where factored out of inner loops */
> diff --git a/src/box/sql/main.c b/src/box/sql/main.c
> index b77348c21..8e898178a 100644
> --- a/src/box/sql/main.c
> +++ b/src/box/sql/main.c
> @@ -800,9 +800,6 @@ sqlite3ErrName(int rc)
> case SQLITE_ERROR:
> zName = "SQLITE_ERROR";
> break;
> - case SQLITE_INTERNAL:
> - zName = "SQLITE_INTERNAL";
> - break;
> case SQLITE_PERM:
> zName = "SQLITE_PERM";
> break;
> @@ -821,9 +818,6 @@ sqlite3ErrName(int rc)
> case SQLITE_NOMEM:
> zName = "SQLITE_NOMEM";
> break;
> - case SQLITE_READONLY:
> - zName = "SQLITE_READONLY";
> - break;
> case SQLITE_INTERRUPT:
> zName = "SQLITE_INTERRUPT";
> break;
> @@ -917,12 +911,6 @@ sqlite3ErrName(int rc)
> case SQLITE_CANTOPEN:
> zName = "SQLITE_CANTOPEN";
> break;
> - case SQLITE_PROTOCOL:
> - zName = "SQLITE_PROTOCOL";
> - break;
> - case SQLITE_EMPTY:
> - zName = "SQLITE_EMPTY";
> - break;
> case SQLITE_SCHEMA:
> zName = "SQLITE_SCHEMA";
> break;
> @@ -959,27 +947,15 @@ sqlite3ErrName(int rc)
> case SQLITE_MISUSE:
> zName = "SQLITE_MISUSE";
> break;
> - case SQLITE_NOLFS:
> - zName = "SQLITE_NOLFS";
> - break;
> - case SQLITE_FORMAT:
> - zName = "SQLITE_FORMAT";
> - break;
> case SQLITE_RANGE:
> zName = "SQLITE_RANGE";
> break;
> - case SQLITE_NOTADB:
> - zName = "SQLITE_NOTADB";
> - break;
> case SQL_TARANTOOL_ERROR:
> zName = "SQLITE_TARANTOOL_ERROR";
> break;
> case SQLITE_ROW:
> zName = "SQLITE_ROW";
> break;
> - case SQLITE_NOTICE:
> - zName = "SQLITE_NOTICE";
> - break;
> case SQLITE_WARNING:
> zName = "SQLITE_WARNING";
> break;
> @@ -1008,32 +984,24 @@ sqlite3ErrStr(int rc)
> static const char *const aMsg[] = {
> /* SQLITE_OK */ "not an error",
> /* SQLITE_ERROR */ "SQL logic error or missing database",
> - /* SQLITE_INTERNAL */ 0,
> /* SQLITE_PERM */ "access permission denied",
> /* SQLITE_ABORT */ "callback requested query abort",
> /* SQLITE_BUSY */ "database is locked",
> /* SQLITE_LOCKED */ "database table is locked",
> /* SQLITE_NOMEM */ "out of memory",
> - /* SQLITE_READONLY */ "attempt to write a readonly database",
> /* SQLITE_INTERRUPT */ "interrupted",
> /* SQLITE_IOERR */ "disk I/O error",
> /* SQLITE_CORRUPT */ "database disk image is malformed",
> /* SQLITE_NOTFOUND */ "unknown operation",
> /* SQLITE_FULL */ "database or disk is full",
> /* SQLITE_CANTOPEN */ "unable to open database file",
> - /* SQLITE_PROTOCOL */ "locking protocol",
> - /* SQLITE_EMPTY */ "table contains no data",
> /* SQLITE_SCHEMA */ "database schema has changed",
> /* SQLITE_TOOBIG */ "string or blob too big",
> /* SQLITE_CONSTRAINT */ "constraint failed",
> /* SQLITE_MISMATCH */ "datatype mismatch",
> /* SQLITE_MISUSE */
> "library routine called out of sequence",
> - /* SQLITE_NOLFS */ "large file support is disabled",
> - /* SQLITE_FORMAT */ "auxiliary database format error",
> /* SQLITE_RANGE */ "bind or column index out of range",
> - /* SQLITE_NOTADB */
> - "file is encrypted or is not a database",
> /* SQL_TARANTOOL_ITERATOR_FAIL */ "Tarantool's iterator failed",
> /* SQL_TARANTOOL_INSERT_FAIL */ "Tarantool's insert failed",
> /* SQL_TARANTOOL_DELETE_FAIL */ "Tarantool's delete failed",
> diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
> index 8dce6f0c2..9cb8a54d0 100644
> --- a/src/box/sql/os_unix.c
> +++ b/src/box/sql/os_unix.c
> @@ -1011,10 +1011,6 @@ findInodeInfo(unixFile * pFile, /* Unix file with file desc used in the key */
> rc = osFstat(fd, &statbuf);
> if (rc != 0) {
> storeLastErrno(pFile, errno);
> -#if defined(EOVERFLOW) && defined(SQLITE_DISABLE_LFS)
> - if (pFile->lastErrno == EOVERFLOW)
> - return SQLITE_NOLFS;
> -#endif
> return SQLITE_IOERR;
> }
> #ifdef __APPLE__
> @@ -6011,10 +6007,7 @@ proxyFileControl(sqlite3_file * id, int op, void *pArg)
> int isProxyStyle = (pFile->pMethod == &proxyIoMethods);
> if (pArg == NULL || (const char *)pArg == 0) {
> if (isProxyStyle) {
> - /* turn off proxy locking - not supported. */
> - rc = SQLITE_ERROR
> - /*SQLITE_PROTOCOL? SQLITE_MISUSE? */
> - ;
> + rc = SQLITE_ERROR;
> } else {
> /* turn off proxy locking - already off - NOOP */
> rc = SQLITE_OK;
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index e786cf842..f78a93a71 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -405,42 +405,56 @@ struct sqlite3_vfs {
> #define SQLITE_LIMIT_TRIGGER_DEPTH 9
> #define SQLITE_LIMIT_WORKER_THREADS 10
>
> -#define SQLITE_OK 0 /* Successful result */
> -/* beginning-of-error-codes */
> -#define SQLITE_ERROR 1 /* SQL error or missing database */
> -#define SQLITE_INTERNAL 2 /* Internal logic error in SQLite */
> -#define SQLITE_PERM 3 /* Access permission denied */
> -#define SQLITE_ABORT 4 /* Callback routine requested an abort */
> -#define SQLITE_BUSY 5 /* The database file is locked */
> -#define SQLITE_LOCKED 6 /* A table in the database is locked */
> -#define SQLITE_NOMEM 7 /* A malloc() failed */
> -#define SQLITE_READONLY 8 /* Attempt to write a readonly database */
> -#define SQLITE_INTERRUPT 9 /* Operation terminated by sqlite3_interrupt() */
> -#define SQLITE_IOERR 10 /* Some kind of disk I/O error occurred */
> -#define SQLITE_CORRUPT 11 /* The database disk image is malformed */
> -#define SQLITE_NOTFOUND 12 /* Unknown opcode in sqlite3_file_control() */
> -#define SQLITE_FULL 13 /* Insertion failed because database is full */
> -#define SQLITE_CANTOPEN 14 /* Unable to open the database file */
> -#define SQLITE_PROTOCOL 15 /* Database lock protocol error */
> -#define SQLITE_EMPTY 16 /* Database is empty */
> -#define SQLITE_SCHEMA 17 /* The database schema changed */
> -#define SQLITE_TOOBIG 18 /* String or BLOB exceeds size limit */
> -#define SQLITE_CONSTRAINT 19 /* Abort due to constraint violation */
> -#define SQLITE_MISMATCH 20 /* Data type mismatch */
> -#define SQLITE_MISUSE 21 /* Library used incorrectly */
> -#define SQLITE_NOLFS 22 /* Uses OS features not supported on host */
> -#define SQLITE_FORMAT 23 /* Auxiliary database format error */
> -#define SQLITE_RANGE 24 /* 2nd parameter to sqlite3_bind out of range */
> -#define SQLITE_NOTADB 25 /* File opened that is not a database file */
> -#define SQL_TARANTOOL_ITERATOR_FAIL 26
> -#define SQL_TARANTOOL_INSERT_FAIL 27
> -#define SQL_TARANTOOL_DELETE_FAIL 28
> -#define SQL_TARANTOOL_ERROR 29
> -#define SQLITE_NOTICE 31 /* Notifications from sqlite3_log() */
> -#define SQLITE_WARNING 32 /* Warnings from sqlite3_log() */
> -#define SQLITE_ROW 100 /* sqlite3_step() has another row ready */
> -#define SQLITE_DONE 101 /* sqlite3_step() has finished executing */
> -/* end-of-error-codes */
> +enum sql_ret_code {
> + /** Result of a routine is ok. */
> + SQLITE_OK = 0,
> + /** Common error code. */
> + SQLITE_ERROR,
> + /** Access permission denied. */
> + SQLITE_PERM,
> + /** Callback routine requested an abort. */
> + SQLITE_ABORT,
> + /** The database file is locked. */
> + SQLITE_BUSY,
> + /** A table in the database is locked. */
> + SQLITE_LOCKED,
> + /** A malloc() failed. */
> + SQLITE_NOMEM,
> + /** Operation terminated by sqlite3_interrupt(). */
> + SQLITE_INTERRUPT,
> + /** Some kind of disk I/O error occurred. */
> + SQLITE_IOERR,
> + /** The database disk image is malformed. */
> + SQLITE_CORRUPT,
> + /** Unknown opcode in sqlite3_file_control(). */
> + SQLITE_NOTFOUND,
> + /** Insertion failed because database is full. */
> + SQLITE_FULL,
> + /** Unable to open the database file. */
> + SQLITE_CANTOPEN,
> + /** The database schema changed. */
> + SQLITE_SCHEMA,
> + /** String or BLOB exceeds size limit. */
> + SQLITE_TOOBIG,
> + /** Abort due to constraint violation. */
> + SQLITE_CONSTRAINT,
> + /** Data type mismatch. */
> + SQLITE_MISMATCH,
> + /** Library used incorrectly. */
> + SQLITE_MISUSE,
> + /** 2nd parameter to sqlite3_bind out of range. */
> + SQLITE_RANGE,
> + SQL_TARANTOOL_ITERATOR_FAIL,
> + SQL_TARANTOOL_INSERT_FAIL,
> + SQL_TARANTOOL_DELETE_FAIL,
> + SQL_TARANTOOL_ERROR,
> + /** Warnings from sqlite3_log(). */
> + SQLITE_WARNING,
> + /** sqlite3_step() has another row ready. */
> + SQLITE_ROW,
> + /** sqlite3_step() has finished executing. */
> + SQLITE_DONE,
> +};
I do like move to enum, but what about removing this obsolete ’SQLITE’ prefix?
Let it be just ’SQL_’. Moreover, some error seems to be SQLite specific.
Great time to remove them, isn’t it?
If you don’t want or don’t have time for this boring routine, you can create issue
with ‘good first issue’ label.
>
> void *
> sqlite3_malloc(int);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index bf8a27709..1685c27ce 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1419,7 +1419,6 @@ case OP_ResultRow: {
> */
> if (SQLITE_OK!=(rc = sqlite3VdbeCheckFk(p, 0))) {
> assert(user_session->sql_flags&SQLITE_CountRows);
> - assert(p->usesStmtJournal);
> goto abort_due_to_error;
> }
>
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index fcb45c8a8..f8f4e566b 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -418,9 +418,6 @@ struct Vdbe {
> i64 startTime; /* Time when query started - used for profiling */
> #endif
> int nOp; /* Number of instructions in the program */
> -#ifdef SQLITE_DEBUG
> - int rcApp; /* errcode set by sqlite3_result_error_code() */
> -#endif
> u16 nResColumn; /* Number of columns in one row of the result set */
> u8 errorAction; /* Recovery action to do in case of an error */
> bft expired:1; /* True if the VM needs to be recompiled */
> @@ -428,7 +425,6 @@ struct Vdbe {
> bft explain:2; /* True if EXPLAIN present on SQL command */
> bft changeCntOn:1; /* True to update the change-counter */
> bft runOnlyOnce:1; /* Automatically expire on reset */
> - bft usesStmtJournal:1; /* True if uses a statement journal */
> bft isPrepareV2:1; /* True if prepared with prepare_v2() */
> u32 aCounter[5]; /* Counters used by sqlite3_stmt_status() */
> char *zSql; /* Text of the SQL statement that generated this */
> @@ -446,6 +442,7 @@ struct Vdbe {
> int nScan; /* Entries in aScan[] */
> ScanStatus *aScan; /* Scan definitions for sqlite3_stmt_scanstatus() */
> #endif
> +
Nitpicking: extra diff.
> };
>
> /*
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index dff1e1697..4c6e90c64 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -469,10 +469,6 @@ sqlite3_result_error_code(sqlite3_context * pCtx, int errCode)
> {
> pCtx->isError = errCode;
> pCtx->fErrorOrAux = 1;
> -#ifdef SQLITE_DEBUG
> - if (pCtx->pVdbe)
> - pCtx->pVdbe->rcApp = errCode;
> -#endif
> if (pCtx->pOut->flags & MEM_Null) {
> sqlite3VdbeMemSetStr(pCtx->pOut, sqlite3ErrStr(errCode), -1, 1,
> SQLITE_STATIC);
> @@ -580,9 +576,6 @@ sqlite3Step(Vdbe * p)
> db->nVdbeActive++;
> p->pc = 0;
> }
> -#ifdef SQLITE_DEBUG
> - p->rcApp = SQLITE_OK;
> -#endif
> #ifndef SQLITE_OMIT_EXPLAIN
> if (p->explain) {
> rc = sqlite3VdbeList(p);
> @@ -614,8 +607,6 @@ sqlite3Step(Vdbe * p)
> */
> assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR
> || (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE);
> - assert((p->rc != SQLITE_ROW && p->rc != SQLITE_DONE)
> - || p->rc == p->rcApp);
> if (p->isPrepareV2 && rc != SQLITE_ROW && rc != SQLITE_DONE) {
> /* If this statement was prepared using sqlite3_prepare_v2(), and an
> * error has occurred, then return the error code in p->rc to the
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index e4335524a..bb121a318 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -2148,7 +2148,6 @@ sqlite3VdbeMakeReady(Vdbe * p, /* The VDBE */
> assert(EIGHT_BYTE_ALIGNMENT(&x.pSpace[x.nFree]));
>
> resolveP2Values(p, &nArg);
> - p->usesStmtJournal = (u8) (pParse->isMultiWrite && pParse->mayAbort);
> if (pParse->explain && nMem < 10) {
> nMem = 10;
> }
> --
> 2.14.3 (Apple Git-98)
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
2018-04-03 15:34 ` [tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] Vladislav Shpilevoy
@ 2018-04-03 23:01 ` n.pettik
2018-04-04 10:26 ` Vladislav Shpilevoy
0 siblings, 1 reply; 15+ messages in thread
From: n.pettik @ 2018-04-03 23:01 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Overall, I do like provided refactoring:
somewhere things become much easier.
>sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
Not opcodes themselves, but opcode operands.
> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> And rework sqlite3CompleteInsertion. Now it
Nitpicking: not *now*, but it was before patch.
It sounds very confusing.
> * accepts actually unused pTab;
> * array of records to insert in different indexes, but inserts
> only one;
> * has sqlite code style.
>
> Code style fix is obvious. PTab is used only to check, that it is
> not a view and to check, that a first index is primary:
> 1) caller functions already guarantee the pTab is not a view;
> 2) regardless of a first index nature: primary or not - it is not
> used here. It is useless check. With the same success we can
> check this in each function, that uses struct Table.
>
> Array of records to insert has no sense, since insertion is being
Nitpicking: *makes no sense*.
> done in a primary index only. It is enough to pass a register of
> a primary index.
Nitpicking: not 'a register of a primary index’,
but number of cursor opened on this space/index.
> ---
> src/box/sql/analyze.c | 8 ++---
> src/box/sql/delete.c | 2 +-
> src/box/sql/expr.c | 5 ++--
> src/box/sql/insert.c | 80 ++++++++++++-------------------------------------
> src/box/sql/select.c | 29 +++++++-----------
> src/box/sql/sqliteInt.h | 16 +++++++++-
> src/box/sql/trigger.c | 3 +-
> src/box/sql/update.c | 6 ++--
> src/box/sql/vdbe.c | 9 ++----
> src/box/sql/wherecode.c | 5 ++--
> 10 files changed, 59 insertions(+), 104 deletions(-)
>
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index fb46641a2..c5dcedd3e 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1085,11 +1085,9 @@ analyzeOneTable(Parse * pParse, /* Parser context */
> regCol + i);
> }
> sqlite3VdbeAddOp3(v, OP_MakeRecord, regCol, nColTest,
> - regSample);
> - sqlite3VdbeAddOp3(v, OP_MakeRecord, regTabname, 6,
> - regTemp);
> - sqlite3VdbeAddOp2(v, OP_IdxReplace, iStatCur + 1,
> - regTemp);
> + regSample);
> + sqlite3VdbeAddOp3(v, OP_MakeRecord, regTabname, 6, regTemp);
> + sqlite3VdbeAddOp2(v, OP_IdxReplace, iStatCur + 1, regTemp);
Nitpicking: *I doubt that this diff is related to patch,
but codestyle fixes are always appreciated.*
> sqlite3VdbeAddOp2(v, OP_Goto, 1, addrNext); /* P1==1 for end-of-loop */
> sqlite3VdbeJumpHere(v, addrIsNull);
>
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 7d1d71577..ab8dd0bc3 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -476,7 +476,7 @@ sqlite3DeleteFrom(Parse * pParse, /* The parser context */
> sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, iKey, zAff, nPk);
> /* Set flag to save memory allocating one by malloc. */
> sqlite3VdbeChangeP5(v, 1);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iEphCur, iKey, iPk, nPk);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iEphCur, iKey);
> }
>
> /* If this DELETE cannot use the ONEPASS strategy, this is the
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index f4e3cf735..78ebc91a5 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2911,9 +2911,8 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */
> 1, r2, &affinity, 1);
> sqlite3ExprCacheAffinityChange(pParse,
> r3, 1);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert,
> - pExpr->iTable, r2,
> - r3, 1);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert,
> + pExpr->iTable, r2);
> }
> sqlite3ReleaseTempReg(pParse, r1);
> sqlite3ReleaseTempReg(pParse, r2);
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 4a57b23f5..c7fe92cd6 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -744,7 +744,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> * with the first column.
> */
> for (i = 0; i < pTab->nCol; i++) {
> - int iRegStore = regTupleid + 1 + i;
> + int iRegStore = regData + i;
> if (pColumn == 0) {
> j = i;
> } else {
> @@ -846,7 +846,6 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> and do the insertion.
> */
> int isReplace; /* Set to true if constraints may cause a replace */
> - int bUseSeek; /* True to use OPFLAG_SEEKRESULT */
> sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
> iIdxCur, regIns, 0,
> ipkColumn >= 0, onError,
> @@ -862,12 +861,13 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> * VdbeCursor.seekResult variable, disabling the OPFLAG_USESEEKRESULT
> * functionality.
> */
> - bUseSeek = isReplace == 0 || (pTrigger == 0 &&
> - ((user_session->sql_flags &
> - SQLITE_ForeignKeys) == 0 ||
> - sqlite3FkReferences(pTab) == 0));
> - sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
> - bUseSeek, onError);
> + bool no_foreign_keys =
> + sqlite3FkReferences(pTab) == NULL ||
> + (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
AFAIK SQLITE_ForeignKeys flag is always set to be true
(since we always use FK). Thus, you can remove this condition check.
> + bool use_seek = isReplace == 0 ||
> + (pTrigger == NULL && no_foreign_keys);
> + vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], use_seek,
> + onError);
> }
>
> /* Update the count of rows that are inserted
> @@ -1490,69 +1490,27 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace));
> }
>
> -/*
> - * This routine generates code to finish the INSERT or UPDATE operation
> - * that was started by a prior call to sqlite3GenerateConstraintChecks.
> - * A consecutive range of registers starting at regNewData contains the
> - * tupleid and the content to be inserted.
> - *
> - * The arguments to this routine should be the same as corresponding
> - * arguments to sqlite3GenerateConstraintChecks.
> - */
> void
> -sqlite3CompleteInsertion(Parse * pParse, /* The parser context */
> - Table * pTab, /* the table into which we are inserting */
> - int iIdxCur, /* Primary index cursor */
> - int *aRegIdx, /* Register used by each index. 0 for unused indices */
> - int useSeekResult, /* True to set the USESEEKRESULT flag on OP_[Idx]Insert */
> - u8 onError)
> +vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
> + bool use_seek_result, u8 on_error)
> {
> - Vdbe *v; /* Prepared statements under construction */
> - Index *pIdx; /* An index being inserted or updated */
> - u16 pik_flags; /* flag values passed to the btree insert */
> - int opcode;
> -
> - v = sqlite3GetVdbe(pParse);
> - assert(v != 0);
> - /* This table is not a VIEW */
> - assert(!space_is_view(pTab));
> - /*
> - * The for loop which purpose in sqlite was to insert new
> - * values to all indexes is replaced to inserting new
> - * values only to pk in tarantool.
> - */
> - pIdx = pTab->pIndex;
> - /* Each table have pk on top of the indexes list */
> - assert(IsPrimaryKeyIndex(pIdx));
> - /* Partial indexes should be implemented somewhere in tarantool
> - * codebase to check it during inserting values to the pk #2626
> - *
> - */
> - /*if( pIdx->pPartIdxWhere ){
> - * sqlite3VdbeAddOp2(v, OP_IsNull, aRegIdx[i], sqlite3VdbeCurrentAddr(v)+2);
> - * VdbeCoverage(v);
> - *}
> - */
> - pik_flags = OPFLAG_NCHANGE;
> - if (useSeekResult) {
> + assert(v != NULL);
> + u16 pik_flags = OPFLAG_NCHANGE;
> + if (use_seek_result)
> pik_flags |= OPFLAG_USESEEKRESULT;
> - }
> - assert(pParse->nested == 0);
>
> - if (onError == ON_CONFLICT_ACTION_REPLACE) {
> + int opcode;
> + if (on_error == ON_CONFLICT_ACTION_REPLACE)
> opcode = OP_IdxReplace;
> - } else {
> + else
> opcode = OP_IdxInsert;
> - }
>
> - if (onError == ON_CONFLICT_ACTION_IGNORE) {
> + if (on_error == ON_CONFLICT_ACTION_IGNORE)
> pik_flags |= OPFLAG_OE_IGNORE;
> - } else if (onError == ON_CONFLICT_ACTION_FAIL) {
> + else if (on_error == ON_CONFLICT_ACTION_FAIL)
> pik_flags |= OPFLAG_OE_FAIL;
> - }
>
> - sqlite3VdbeAddOp4Int(v, opcode, iIdxCur, aRegIdx[0],
> - aRegIdx[0] + 1, index_column_count(pIdx));
> + sqlite3VdbeAddOp2(v, opcode, cursor_id, tuple_id);
> sqlite3VdbeChangeP5(v, pik_flags);
> }
>
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index c7f87f340..95bd7d656 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -667,13 +667,11 @@ pushOntoSorter(Parse * pParse, /* Parser context */
> sqlite3ExprCodeMove(pParse, regBase, regPrevKey, pSort->nOBSat);
> sqlite3VdbeJumpHere(v, addrJmp);
> }
> - if (pSort->sortFlags & SORTFLAG_UseSorter) {
> + if (pSort->sortFlags & SORTFLAG_UseSorter)
> op = OP_SorterInsert;
> - } else {
> + else
> op = OP_IdxInsert;
> - }
> - sqlite3VdbeAddOp4Int(v, op, pSort->iECursor, regRecord,
> - regBase + nOBSat, nBase - nOBSat);
> + sqlite3VdbeAddOp2(v, op, pSort->iECursor, regRecord);
> if (iLimit) {
> int addr;
> int r1 = 0;
> @@ -752,7 +750,7 @@ codeDistinct(Parse * pParse, /* Parsing and code generating context */
> sqlite3VdbeAddOp4Int(v, OP_Found, iTab, addrRepeat, iMem, N);
> VdbeCoverage(v);
> sqlite3VdbeAddOp3(v, OP_MakeRecord, iMem, N, r1);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iTab, r1, iMem, N);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iTab, r1);
> sqlite3ReleaseTempReg(pParse, r1);
> }
>
> @@ -967,8 +965,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
> r1 = sqlite3GetTempReg(pParse);
> sqlite3VdbeAddOp3(v, OP_MakeRecord, regResult,
> nResultCol, r1);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1,
> - regResult, nResultCol);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
> sqlite3ReleaseTempReg(pParse, r1);
> break;
> }
> @@ -1011,8 +1008,8 @@ selectInnerLoop(Parse * pParse, /* The parser context */
> sqlite3VdbeAddOp4Int(v, OP_Found, iParm + 1,
> addr, r1, 0);
> VdbeCoverage(v);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm + 1,
> - r1, regResult, nResultCol);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1,
> + r1);
> assert(pSort == 0);
> }
> #endif
> @@ -1068,8 +1065,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
> sqlite3ExprCacheAffinityChange(pParse,
> regResult,
> nResultCol);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1,
> - regResult, nResultCol);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
> sqlite3ReleaseTempReg(pParse, r1);
> }
> break;
> @@ -1165,8 +1161,7 @@ selectInnerLoop(Parse * pParse, /* The parser context */
> }
> sqlite3VdbeAddOp2(v, OP_Sequence, iParm, r2 + nKey);
> sqlite3VdbeAddOp3(v, OP_MakeRecord, r2, nKey + 2, r1);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1, r2,
> - nKey + 2);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
> if (addrTest)
> sqlite3VdbeJumpHere(v, addrTest);
> sqlite3ReleaseTempReg(pParse, r1);
> @@ -1518,8 +1513,7 @@ generateSortTail(Parse * pParse, /* Parsing context */
> sqlite3VdbeAddOp4(v, OP_MakeRecord, regRow, nColumn,
> regTupleid, pDest->zAffSdst, nColumn);
> sqlite3ExprCacheAffinityChange(pParse, regRow, nColumn);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, regTupleid,
> - regRow, nColumn);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, regTupleid);
> break;
> }
> case SRT_Mem:{
> @@ -2982,8 +2976,7 @@ generateOutputSubroutine(Parse * pParse, /* Parsing context */
> pIn->nSdst);
> sqlite3ExprCacheAffinityChange(pParse, pIn->iSdst,
> pIn->nSdst);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, pDest->iSDParm,
> - r1, pIn->iSdst, pIn->nSdst);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, pDest->iSDParm, r1);
> sqlite3ReleaseTempReg(pParse, r1);
> break;
> }
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index f43711a7f..e786cf842 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3715,7 +3715,21 @@ int sqlite3GenerateIndexKey(Parse *, Index *, int, int, int *, Index *, int);
> void sqlite3ResolvePartIdxLabel(Parse *, int);
> void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
> int, u8, u8, int, int *, int *);
> -void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8);
> +/**
> + * This routine generates code to finish the INSERT or UPDATE
> + * operation that was started by a prior call to
> + * sqlite3GenerateConstraintChecks.
> + * @param v Virtual database engine.
> + * @param cursor_id Primary index cursor.
> + * @param tuple_id Register with data to insert.
> + * @param use_seek_result True to set the USESEEKRESULT flag on
> + * OP_[Idx]Insert.
> + * @param on_error Error action on failed insert/replace.
> + */
> +void
> +vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
> + bool use_seek_result, u8 on_error);
> +
> int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
> int *, u8, u8);
> void sqlite3BeginWriteOperation(Parse *, int);
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 859b421db..eaa08eb4c 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -281,8 +281,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */
> sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 1,
> MSGPACK_SUBTYPE, zOpts, P4_DYNAMIC);
> sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 2, iRecord);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, iRecord,
> - iFirstCol, 7);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iCursor, iRecord);
> /* Do not account nested operations: the count of such
> * operations depends on Tarantool data dictionary internals,
> * such as data layout in system spaces.
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 7727ae5d5..859189a7e 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -377,8 +377,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
> sqlite3IndexAffinityStr(pParse->db, pPk);
> sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey,
> zAff, nPk);
> - sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iEph, regKey, iPk,
> - nPk);
> + sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey);
> /* Set flag to save memory allocating one by malloc. */
> sqlite3VdbeChangeP5(v, 1);
> }
> @@ -616,7 +615,8 @@ sqlite3Update(Parse * pParse, /* The parser context */
> }
>
> /* Insert the new index entries and the new record. */
> - sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0, onError);
> + vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], false,
> + onError);
>
> /* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
> * handle rows (possibly in other tables) that refer via a foreign key
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 329d46886..bf8a27709 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4318,18 +4318,13 @@ case OP_Next: /* jump */
> goto check_for_interrupt;
> }
>
> -/* Opcode: IdxInsert P1 P2 P3 P4 P5
> +/* Opcode: IdxInsert P1 P2 * * P5
> * Synopsis: key=r[P2]
> *
> * Register P2 holds an SQL index key made using the
> * MakeRecord instructions. This opcode writes that key
> * into the index P1. Data for the entry is nil.
What does mean ‘'Data for the entry is nil.” ?
I know that it could be boring and it is not included to diff,
but what about updating whole commentto this opcode?
> *
> - * If P4 is not zero, then it is the number of values in the unpacked
> - * key of reg(P2). In that case, P3 is the index of the first register
> - * for the unpacked key. The availability of the unpacked key can sometimes
> - * be an optimization.
> - *
> * If P5 has the OPFLAG_APPEND bit set, that is a hint to the b-tree layer
> * that this insert is likely to be an append.
The same here: I guess this comment is obsolete.
> *
> @@ -4345,7 +4340,7 @@ case OP_Next: /* jump */
> *
> * This instruction only works for indices.
> */
> -/* Opcode: IdxReplace P1 P2 P3 P4 P5
> +/* Opcode: IdxReplace P1 P2 * * P5
> * Synopsis: key=r[P2]
> *
> * This opcode works exactly as IdxInsert does, but in Tarantool
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 8b06baeff..a3c51eef4 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1763,10 +1763,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
> sqlite3VdbeAddOp3
> (v, OP_MakeRecord,
> r, nPk, regPk);
> - sqlite3VdbeAddOp4Int
> + sqlite3VdbeAddOp2
> (v, OP_IdxInsert,
> - regRowset, regPk,
> - r, nPk);
> + regRowset, regPk);
> if (iSet)
> sqlite3VdbeChangeP5
> (v,
> --
> 2.14.3 (Apple Git-98)
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 4/4] sql: remove useless branching in insertOrReplace
2018-04-03 15:34 ` [tarantool-patches] [PATCH 4/4] sql: remove useless branching in insertOrReplace Vladislav Shpilevoy
@ 2018-04-03 23:01 ` n.pettik
0 siblings, 0 replies; 15+ messages in thread
From: n.pettik @ 2018-04-03 23:01 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Ack.
> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Type of an operation for struct request can be passed directly
> with no "proxying" by special codes.
> ---
> src/box/sql.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/src/box/sql.c b/src/box/sql.c
> index c2577abef..a6713f1f0 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -474,35 +474,29 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
> return SQLITE_OK;
> }
>
> -static int insertOrReplace(BtCursor *pCur, int operationType)
> +static inline int
> +insertOrReplace(BtCursor *pCur, enum iproto_type type)
> {
> assert(pCur->curFlags & BTCF_TaCursor);
> - assert(operationType == TARANTOOL_INDEX_INSERT ||
> - operationType == TARANTOOL_INDEX_REPLACE);
> -
> struct request request;
> memset(&request, 0, sizeof(request));
> request.tuple = pCur->key;
> request.tuple_end = pCur->key + pCur->nKey;
> request.space_id = pCur->space->def->id;
> + request.type = type;
> mp_tuple_assert(request.tuple, request.tuple_end);
> - if (operationType == TARANTOOL_INDEX_INSERT) {
> - request.type = IPROTO_INSERT;
> - } else {
> - request.type = IPROTO_REPLACE;
> - }
> int rc = box_process_rw(&request, pCur->space, NULL);
> - return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;;
> + return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;
> }
>
> int tarantoolSqlite3Insert(BtCursor *pCur)
> {
> - return insertOrReplace(pCur, TARANTOOL_INDEX_INSERT);
> + return insertOrReplace(pCur, IPROTO_INSERT);
> }
>
> int tarantoolSqlite3Replace(BtCursor *pCur)
> {
> - return insertOrReplace(pCur, TARANTOOL_INDEX_REPLACE);
> + return insertOrReplace(pCur, IPROTO_REPLACE);
> }
>
> /*
> --
> 2.14.3 (Apple Git-98)
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 3/4] sql: simplify lua SQL executor
2018-04-03 15:34 ` [tarantool-patches] [PATCH 3/4] sql: simplify lua SQL executor Vladislav Shpilevoy
@ 2018-04-03 23:02 ` n.pettik
0 siblings, 0 replies; 15+ messages in thread
From: n.pettik @ 2018-04-03 23:02 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Ack.
> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> 1) Code is completely unreadable and complicated by strange
> optimizations;
> 2) It still supposes, that there can be multiple statements in a
> single string;
> 3) It pushes first letter of affinity together with column names
> - why? It is not used anywhere.
>
> Lets remove prepared statements list, affinity first letters
> pushing.
> ---
> src/box/lua/sql.c | 239 ++++++++----------------------------------------------
> 1 file changed, 35 insertions(+), 204 deletions(-)
>
> diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c
> index 12b81383d..e693aeaad 100644
> --- a/src/box/lua/sql.c
> +++ b/src/box/lua/sql.c
> @@ -6,132 +6,12 @@
> #include "lua/utils.h"
> #include "info.h"
>
> -struct prep_stmt
> -{
> - sqlite3_stmt *stmt;
> -};
> -
> -struct prep_stmt_list
> -{
> - uint8_t *mem_end; /* denotes actual size of sql_ctx struct */
> - uint32_t pool_size; /* mem at the end used for aux allocations;
> - pool grows from mem_end
> - towards stmt[] array */
> - uint32_t last_select_stmt_index; /* UINT32_MAX if no selects */
> - uint32_t column_count; /* in last select stmt */
> - uint32_t stmt_count;
> - struct prep_stmt stmt[6]; /* overlayed with the mem pool;
> - actual size could be larger or smaller */
> - /* uint8_t mem_pool[] */
> -};
> -
> -static inline int
> -prep_stmt_list_needs_free(struct prep_stmt_list *l)
> -{
> - return (uint8_t *)(l + 1) != l->mem_end;
> -}
> -
> -/* Release resources and free the list itself, unless it was preallocated
> - * (i.e. l points to an automatic variable) */
> -static void
> -prep_stmt_list_free(struct prep_stmt_list *l)
> -{
> - if (l == NULL)
> - return;
> - for (size_t i = 0, n = l->stmt_count; i < n; i++)
> - sqlite3_finalize(l->stmt[i].stmt);
> - if (prep_stmt_list_needs_free(l))
> - free(l);
> -}
> -
> -static struct prep_stmt_list *
> -prep_stmt_list_init(struct prep_stmt_list *prealloc)
> -{
> - prealloc->mem_end = (uint8_t *)(prealloc + 1);
> - prealloc->pool_size = 0;
> - prealloc->last_select_stmt_index = UINT32_MAX;
> - prealloc->column_count = 0;
> - prealloc->stmt_count = 0;
> - return prealloc;
> -}
> -
> -/* Allocate mem from the prep_stmt_list pool.
> - * If not enough space is available, reallocates the list.
> - * If reallocation is needed but l was preallocated, old mem is left
> - * intact and a new memory chunk is allocated. */
> -static void *
> -prep_stmt_list_palloc(struct prep_stmt_list **pl,
> - size_t size, size_t alignment)
> -{
> - assert((alignment & (alignment - 1)) == 0); /* 2 ^ k */
> - assert(alignment <= __alignof__((*pl)->stmt[0]));
> -
> - struct prep_stmt_list *l = *pl;
> - uint32_t pool_size = l->pool_size;
> - uint32_t pool_size_max = (uint32_t)(
> - l->mem_end - (uint8_t *)(l->stmt + l->stmt_count)
> - );
> -
> - assert(UINT32_MAX - pool_size >= size);
> - pool_size += size;
> -
> - assert(UINT32_MAX - pool_size >= alignment - 1);
> - pool_size += alignment - 1;
> - pool_size &= ~(alignment - 1);
> -
> - if (pool_size > pool_size_max) {
> - size_t prev_size = l->mem_end - (uint8_t *)l;
> - size_t size = prev_size;
> - while (size < prev_size + (pool_size - pool_size_max)) {
> - assert(SIZE_MAX - size >= size);
> - size += size;
> - }
> - if (prep_stmt_list_needs_free(l)) {
> - l = realloc(l, size);
> - if (l == NULL)
> - return NULL;
> - } else {
> - l = malloc(size);
> - if (l == NULL)
> - return NULL;
> - memcpy(l, *pl, prev_size);
> - }
> - l->mem_end = (uint8_t *)l + size;
> - /* move the pool data */
> - memmove((uint8_t *)l + prev_size - l->pool_size,
> - l->mem_end - l->pool_size,
> - l->pool_size);
> - *pl = l;
> - }
> -
> - l->pool_size = pool_size;
> - return l->mem_end - pool_size;
> -}
> -
> -/* push new stmt; reallocate memory if needed
> - * returns a pointer to the new stmt or NULL if out of memory.
> - * If reallocation is needed but l was preallocated, old mem is left
> - * intact and a new memory chunk is allocated. */
> -static struct prep_stmt *
> -prep_stmt_list_push(struct prep_stmt_list **pl)
> -{
> - struct prep_stmt_list *l;
> - /* make sure we don't collide with the pool */
> - if (prep_stmt_list_palloc(pl, sizeof(l->stmt[0]), 1
> - ) == NULL)
> - return NULL;
> - l = *pl;
> - l->pool_size -= sizeof(l->stmt[0]);
> - return l->stmt + (l->stmt_count++);
> -}
> -
> static void
> -lua_push_column_names(struct lua_State *L, struct prep_stmt_list *l)
> +lua_push_column_names(struct lua_State *L, struct sqlite3_stmt *stmt)
> {
> - sqlite3_stmt *stmt = l->stmt[l->last_select_stmt_index].stmt;
> - int n = l->column_count;
> - lua_createtable(L, n, 0);
> - for (int i = 0; i < n; i++) {
> + int column_count = sqlite3_column_count(stmt);
> + lua_createtable(L, column_count, 0);
> + for (int i = 0; i < column_count; i++) {
> const char *name = sqlite3_column_name(stmt, i);
> lua_pushstring(L, name == NULL ? "" : name);
> lua_rawseti(L, -2, i+1);
> @@ -139,11 +19,9 @@ lua_push_column_names(struct lua_State *L, struct prep_stmt_list *l)
> }
>
> static void
> -lua_push_row(struct lua_State *L, struct prep_stmt_list *l)
> +lua_push_row(struct lua_State *L, struct sqlite3_stmt *stmt)
> {
> - sqlite3_stmt *stmt = l->stmt[l->last_select_stmt_index].stmt;
> - int column_count = l->column_count;
> - char *typestr = (void *)(l->mem_end - column_count);
> + int column_count = sqlite3_column_count(stmt);
>
> lua_createtable(L, column_count, 0);
> lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_array_metatable_ref);
> @@ -153,124 +31,77 @@ lua_push_row(struct lua_State *L, struct prep_stmt_list *l)
> int type = sqlite3_column_type(stmt, i);
> switch (type) {
> case SQLITE_INTEGER:
> - typestr[i] = 'i';
> - luaL_pushint64(L, sqlite3_column_int64(stmt, i));
> + luaL_pushint64(L, sqlite3_column_int64(stmt, i));
> break;
> case SQLITE_FLOAT:
> - typestr[i] = 'f';
> lua_pushnumber(L, sqlite3_column_double(stmt, i));
> break;
> case SQLITE_TEXT: {
> const void *text = sqlite3_column_text(stmt, i);
> - typestr[i] = 's';
> lua_pushlstring(L, text,
> sqlite3_column_bytes(stmt, i));
> break;
> }
> case SQLITE_BLOB: {
> const void *blob = sqlite3_column_blob(stmt, i);
> - typestr[i] = 'b';
> lua_pushlstring(L, blob,
> sqlite3_column_bytes(stmt, i));
> break;
> }
> case SQLITE_NULL:
> - typestr[i] = '-';
> lua_rawgeti(L, LUA_REGISTRYINDEX, luaL_nil_ref);
> break;
> default:
> - typestr[i] = '?';
> assert(0);
> }
> lua_rawseti(L, -2, i+1);
> }
> -
> - lua_pushlstring(L, typestr, column_count);
> - lua_rawseti(L, -2, 0);
> }
>
> static int
> lua_sql_execute(struct lua_State *L)
> {
> - int rc;
> sqlite3 *db = sql_get();
> - struct prep_stmt_list *l = NULL, stock_l;
> - size_t length;
> - const char *sql, *sql_end;
> -
> if (db == NULL)
> return luaL_error(L, "not ready");
>
> - sql = lua_tolstring(L, 1, &length);
> + size_t length;
> + const char *sql = lua_tolstring(L, 1, &length);
> if (sql == NULL)
> return luaL_error(L, "usage: box.sql.execute(sqlstring)");
>
> - assert(length <= INT_MAX);
> - sql_end = sql + length;
> + struct sqlite3_stmt *stmt;
> + if (sqlite3_prepare_v2(db, sql, length, &stmt, &sql) != SQLITE_OK)
> + goto sqlerror;
> + assert(stmt != NULL);
>
> - l = prep_stmt_list_init(&stock_l);
> - do {
> -
> - struct prep_stmt *ps = prep_stmt_list_push(&l);
> - if (ps == NULL)
> - goto outofmem;
> - rc = sqlite3_prepare_v2(db, sql, (int)(sql_end - sql),
> - &ps->stmt, &sql);
> - if (rc != SQLITE_OK)
> - goto sqlerror;
> -
> - if (ps->stmt == NULL) {
> - /* only whitespace */
> - assert(sql == sql_end);
> - l->stmt_count --;
> - break;
> - }
> -
> - int column_count = sqlite3_column_count(ps->stmt);
> - if (column_count == 0) {
> - while ((rc = sqlite3_step(ps->stmt)) == SQLITE_ROW) { ; }
> - } else {
> - char *typestr;
> - l->column_count = column_count;
> - l->last_select_stmt_index = l->stmt_count - 1;
> -
> - assert(l->pool_size == 0);
> - /* This might possibly call realloc() and ruin *ps. */
> - typestr = prep_stmt_list_palloc(&l, column_count, 1);
> - if (typestr == NULL)
> - goto outofmem;
> - /* Refill *ps. */
> - ps = l->stmt + l->stmt_count - 1;
> -
> - lua_settop(L, 1); /* discard any results */
> -
> - /* create result table */
> - lua_createtable(L, 7, 0);
> - lua_pushvalue(L, lua_upvalueindex(1));
> - lua_setmetatable(L, -2);
> - lua_push_column_names(L, l);
> - lua_rawseti(L, -2, 0);
> -
> - int row_count = 0;
> - while ((rc = sqlite3_step(ps->stmt)) == SQLITE_ROW) {
> - lua_push_row(L, l);
> - row_count++;
> - lua_rawseti(L, -2, row_count);
> - }
> - l->pool_size = 0;
> + int rc;
> + int retval_count;
> + if (sqlite3_column_count(stmt) == 0) {
> + while ((rc = sqlite3_step(stmt)) == SQLITE_ROW);
> + retval_count = 0;
> + } else {
> + lua_newtable(L);
> + lua_pushvalue(L, lua_upvalueindex(1));
> + lua_setmetatable(L, -2);
> + lua_push_column_names(L, stmt);
> + lua_rawseti(L, -2, 0);
> +
> + int row_count = 0;
> + while ((rc = sqlite3_step(stmt)) == SQLITE_ROW) {
> + lua_push_row(L, stmt);
> + lua_rawseti(L, -2, ++row_count);
> }
> + retval_count = 1;
> + }
> if (rc != SQLITE_OK && rc != SQLITE_DONE)
> - goto sqlerror;
> - } while (sql != sql_end);
> - prep_stmt_list_free(l);
> - return lua_gettop(L) - 1;
> + goto sqlerror;
> + sqlite3_finalize(stmt);
> + return retval_count;
> sqlerror:
> lua_pushstring(L, sqlite3_errmsg(db));
> - prep_stmt_list_free(l);
> + sqlite3_finalize(stmt);
> return lua_error(L);
> -outofmem:
> - prep_stmt_list_free(l);
> - return luaL_error(L, "out of memory");
> }
>
> static int
> --
> 2.14.3 (Apple Git-98)
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
@ 2018-04-04 10:26 ` Vladislav Shpilevoy
2018-04-04 11:51 ` n.pettik
0 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-04 10:26 UTC (permalink / raw)
To: tarantool-patches, n.pettik
Hello. See the diff at the end of the letter, and my responses to some of
your comments.
>> done in a primary index only. It is enough to pass a register of
>> a primary index.
> Nitpicking: not 'a register of a primary index’,
> but number of cursor opened on this space/index.
Here you are wrong - it is a register, where tuple data is stored to
insert. Cursor id
argument is not changed. I fixed it in a new commit.
>
>> + bool no_foreign_keys =
>> + sqlite3FkReferences(pTab) == NULL ||
>> + (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
> AFAIK SQLITE_ForeignKeys flag is always set to be true
> (since we always use FK). Thus, you can remove this condition check.
Yes, Foreign Keys source code now is always built, because we have
removed OMIT_FOREIGN_KEYS,
but SQLITE_ForeignKeys is not a #define - it is a flag, that can be set
by a user on runtime in its
session, and allows to skip foreign keys checking. The removal of this
flags must be discussed,
because I think it can be useful.
In any case, talking about the patch, I found a way to remove this code
completely. See diff below.
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index c5dcedd3e..4eae896ab 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1050,7 +1050,6 @@ analyzeOneTable(Parse * pParse, /* Parser context */
sqlite3VdbeAddOp4(v, OP_MakeRecord, regTabname, 3, regTemp,
"BBB", 0);
sqlite3VdbeAddOp2(v, OP_IdxInsert, iStatCur, regTemp);
- sqlite3VdbeChangeP5(v, OPFLAG_APPEND);
/* Add the entries to the stat4 table. */
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5e3ed0f39..5c3cac881 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2659,7 +2659,6 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
sqlite3VdbeAddOp3(v, OP_SorterData, iSorter, regRecord, iIdx);
sqlite3VdbeAddOp3(v, OP_Last, iIdx, 0, -1);
sqlite3VdbeAddOp2(v, OP_IdxInsert, iIdx, regRecord);
- sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
sqlite3ReleaseTempReg(pParse, regRecord);
sqlite3VdbeAddOp2(v, OP_SorterNext, iSorter, addr2);
VdbeCoverage(v);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c7fe92cd6..c76910e5b 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -851,23 +851,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
ipkColumn >= 0, onError,
endOfLoop, &isReplace, 0);
sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
-
- /* Set the OPFLAG_USESEEKRESULT flag if either (a) there are no REPLACE
- * constraints or (b) there are no triggers and this table is not a
- * parent table in a foreign key constraint. It is safe to set the
- * flag in the second case as if any REPLACE constraint is hit, an
- * OP_Delete or OP_IdxDelete instruction will be executed on each
- * cursor that is disturbed. And these instructions both clear the
- * VdbeCursor.seekResult variable, disabling the OPFLAG_USESEEKRESULT
- * functionality.
- */
- bool no_foreign_keys =
- sqlite3FkReferences(pTab) == NULL ||
- (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
- bool use_seek = isReplace == 0 ||
- (pTrigger == NULL && no_foreign_keys);
- vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], use_seek,
- onError);
+ vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
}
/* Update the count of rows that are inserted
@@ -1491,20 +1475,16 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
}
void
-vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
- bool use_seek_result, u8 on_error)
+vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
{
assert(v != NULL);
- u16 pik_flags = OPFLAG_NCHANGE;
- if (use_seek_result)
- pik_flags |= OPFLAG_USESEEKRESULT;
-
int opcode;
if (on_error == ON_CONFLICT_ACTION_REPLACE)
opcode = OP_IdxReplace;
else
opcode = OP_IdxInsert;
+ u16 pik_flags = OPFLAG_NCHANGE;
if (on_error == ON_CONFLICT_ACTION_IGNORE)
pik_flags |= OPFLAG_OE_IGNORE;
else if (on_error == ON_CONFLICT_ACTION_FAIL)
@@ -1910,7 +1890,6 @@ xferOptimization(Parse * pParse, /* Parser context */
}
for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
- u8 idxInsFlags = 0;
for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
pSrcIdx = pSrcIdx->pNext) {
if (xferCompatibleIndex(pDestIdx, pSrcIdx))
@@ -1927,11 +1906,9 @@ xferOptimization(Parse * pParse, /* Parser context */
addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
VdbeCoverage(v);
sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) {
- idxInsFlags |= OPFLAG_NCHANGE;
- }
sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- sqlite3VdbeChangeP5(v, idxInsFlags | OPFLAG_APPEND);
+ if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
+ sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1);
VdbeCoverage(v);
sqlite3VdbeJumpHere(v, addr1);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 95bd7d656..9e64b434e 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1151,7 +1151,6 @@ selectInnerLoop(Parse * pParse, /* The parser context */
if (eDest == SRT_DistQueue) {
sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1,
r3);
- sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
}
for (i = 0; i < nKey; i++) {
sqlite3VdbeAddOp2(v, OP_SCopy,
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index e786cf842..7c0a214b5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3039,8 +3039,6 @@ struct Parse {
/* Also used in P2 (not P5) of OP_Delete */
#define OPFLAG_EPHEM 0x01 /* OP_Column: Ephemeral output is ok */
#define OPFLAG_ISUPDATE 0x04 /* This OP_Insert is an sql UPDATE */
-#define OPFLAG_APPEND 0x08 /* This is likely to be an append */
-#define OPFLAG_USESEEKRESULT 0x10 /* Try to avoid a seek in BtreeInsert() */
#define OPFLAG_OE_IGNORE 0x200 /* OP_IdxInsert: Ignore flag */
#define OPFLAG_OE_FAIL 0x400 /* OP_IdxInsert: Fail flag */
#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
@@ -3722,13 +3720,11 @@ void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
* @param v Virtual database engine.
* @param cursor_id Primary index cursor.
* @param tuple_id Register with data to insert.
- * @param use_seek_result True to set the USESEEKRESULT flag on
- * OP_[Idx]Insert.
* @param on_error Error action on failed insert/replace.
*/
void
-vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
- bool use_seek_result, u8 on_error);
+vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
+ u8 on_error);
int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
int *, u8, u8);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 859189a7e..98e696ddf 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -615,8 +615,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
}
/* Insert the new index entries and the new record. */
- vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], false,
- onError);
+ vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
/* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
* handle rows (possibly in other tables) that refer via a foreign key
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bf8a27709..5a56727da 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4321,24 +4321,11 @@ case OP_Next: /* jump */
/* Opcode: IdxInsert P1 P2 * * P5
* Synopsis: key=r[P2]
*
- * Register P2 holds an SQL index key made using the
- * MakeRecord instructions. This opcode writes that key
- * into the index P1. Data for the entry is nil.
- *
- * If P5 has the OPFLAG_APPEND bit set, that is a hint to the b-tree layer
- * that this insert is likely to be an append.
- *
- * If P5 has the OPFLAG_NCHANGE bit set, then the change counter is
- * incremented by this instruction. If the OPFLAG_NCHANGE bit is clear,
- * then the change counter is unchanged.
- *
- * If the OPFLAG_USESEEKRESULT flag of P5 is set, the implementation might
- * run faster by avoiding an unnecessary seek on cursor P1. However,
- * the OPFLAG_USESEEKRESULT flag must only be set if there have been no prior
- * seeks on the cursor or if the most recent seek used a key equivalent
- * to P2.
- *
- * This instruction only works for indices.
+ * @param P1 Index of a space cursor.
+ * @param P2 Index of a register with MessagePack data to insert.
+ * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
+ * accounts the change in a case of successful insertion in
+ * nChange counter.
*/
/* Opcode: IdxReplace P1 P2 * * P5
* Synopsis: key=r[P2]
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 79de5d69a..c51bd7455 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -844,7 +844,6 @@ constructAutomaticIndex(Parse * pParse, /* The parsing context */
sqlite3GenerateIndexKey(pParse, pIdx, pLevel->iTabCur, regRecord, 0,
0, 0);
sqlite3VdbeAddOp2(v, OP_IdxInsert, pLevel->iIdxCur, regRecord);
- sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
if (pPartial)
sqlite3VdbeResolveLabel(v, iContinue);
if (pTabItem->fg.viaCoroutine) {
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index a3c51eef4..aaffa7c1c 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1766,10 +1766,6 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
sqlite3VdbeAddOp2
(v, OP_IdxInsert,
regRowset, regPk);
- if (iSet)
- sqlite3VdbeChangeP5
- (v,
- OPFLAG_USESEEKRESULT);
}
/* Release the array of temp registers */
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: remove unused error codes, use enum instead of defines
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
@ 2018-04-04 10:26 ` Vladislav Shpilevoy
2018-04-04 11:30 ` n.pettik
0 siblings, 1 reply; 15+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-04 10:26 UTC (permalink / raw)
To: tarantool-patches, n.pettik
> I do like move to enum, but what about removing this obsolete ’SQLITE’ prefix?
> Let it be just ’SQL_’. Moreover, some error seems to be SQLite specific.
> Great time to remove them, isn’t it?
> If you don’t want or don’t have time for this boring routine, you can create issue
> with ‘good first issue’ label.
I am afraid that a diff will be huge, if a rename SQLITE_OK, for
example. Lets do it later, when more
source code and error codes will be removed.
https://github.com/tarantool/tarantool/issues/3315
Please, review the huge diff below very carefully. I could make a
mistake here.
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index f8f4e566b..6ea8c91d7 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -442,7 +442,6 @@ struct Vdbe {
int nScan; /* Entries in aScan[] */
ScanStatus *aScan; /* Scan definitions for
sqlite3_stmt_scanstatus() */
#endif
-
};
/*
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 2/4] sql: remove unused error codes, use enum instead of defines
2018-04-04 10:26 ` Vladislav Shpilevoy
@ 2018-04-04 11:30 ` n.pettik
0 siblings, 0 replies; 15+ messages in thread
From: n.pettik @ 2018-04-04 11:30 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
Ack.
> On 4 Apr 2018, at 13:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
>> I do like move to enum, but what about removing this obsolete ’SQLITE’ prefix?
>> Let it be just ’SQL_’. Moreover, some error seems to be SQLite specific.
>> Great time to remove them, isn’t it?
>> If you don’t want or don’t have time for this boring routine, you can create issue
>> with ‘good first issue’ label.
> I am afraid that a diff will be huge, if a rename SQLITE_OK, for example. Lets do it later, when more
> source code and error codes will be removed.
> https://github.com/tarantool/tarantool/issues/3315
>
> Please, review the huge diff below very carefully. I could make a mistake here.
>
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index f8f4e566b..6ea8c91d7 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -442,7 +442,6 @@ struct Vdbe {
> int nScan; /* Entries in aScan[] */
> ScanStatus *aScan; /* Scan definitions for sqlite3_stmt_scanstatus() */
> #endif
> -
> };
>
> /*
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
2018-04-04 10:26 ` Vladislav Shpilevoy
@ 2018-04-04 11:51 ` n.pettik
2018-04-04 14:48 ` n.pettik
0 siblings, 1 reply; 15+ messages in thread
From: n.pettik @ 2018-04-04 11:51 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
You may not pay attention to ’nitpickings’,
but I should make visibility of reviewing this patch.
> On 4 Apr 2018, at 13:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Hello. See the diff at the end of the letter, and my responses to some of
> your comments.
>>> done in a primary index only. It is enough to pass a register of
>>> a primary index.
>> Nitpicking: not 'a register of a primary index’,
>> but number of cursor opened on this space/index.
> Here you are wrong - it is a register, where tuple data is stored to insert. Cursor id
> argument is not changed. I fixed it in a new commit.
Ok, I have misunderstood you.
Now it is clear that you mentioned another operand.
>>
>>> + bool no_foreign_keys =
>>> + sqlite3FkReferences(pTab) == NULL ||
>>> + (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
>> AFAIK SQLITE_ForeignKeys flag is always set to be true
>> (since we always use FK). Thus, you can remove this condition check.
> Yes, Foreign Keys source code now is always built, because we have removed OMIT_FOREIGN_KEYS,
> but SQLITE_ForeignKeys is not a #define - it is a flag, that can be set by a user on runtime in its
> session, and allows to skip foreign keys checking. The removal of this flags must be discussed,
> because I think it can be useful.
>
> In any case, talking about the patch, I found a way to remove this code completely. See diff below.
Ok, looks pretty good.
>
>
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index c5dcedd3e..4eae896ab 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1050,7 +1050,6 @@ analyzeOneTable(Parse * pParse, /* Parser context */
> sqlite3VdbeAddOp4(v, OP_MakeRecord, regTabname, 3, regTemp,
> "BBB", 0);
> sqlite3VdbeAddOp2(v, OP_IdxInsert, iStatCur, regTemp);
> - sqlite3VdbeChangeP5(v, OPFLAG_APPEND);
> /* Add the entries to the stat4 table. */
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 5e3ed0f39..5c3cac881 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2659,7 +2659,6 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
> sqlite3VdbeAddOp3(v, OP_SorterData, iSorter, regRecord, iIdx);
> sqlite3VdbeAddOp3(v, OP_Last, iIdx, 0, -1);
> sqlite3VdbeAddOp2(v, OP_IdxInsert, iIdx, regRecord);
> - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
> sqlite3ReleaseTempReg(pParse, regRecord);
> sqlite3VdbeAddOp2(v, OP_SorterNext, iSorter, addr2);
> VdbeCoverage(v);
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index c7fe92cd6..c76910e5b 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -851,23 +851,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> ipkColumn >= 0, onError,
> endOfLoop, &isReplace, 0);
> sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
> -
> - /* Set the OPFLAG_USESEEKRESULT flag if either (a) there are no REPLACE
> - * constraints or (b) there are no triggers and this table is not a
> - * parent table in a foreign key constraint. It is safe to set the
> - * flag in the second case as if any REPLACE constraint is hit, an
> - * OP_Delete or OP_IdxDelete instruction will be executed on each
> - * cursor that is disturbed. And these instructions both clear the
> - * VdbeCursor.seekResult variable, disabling the OPFLAG_USESEEKRESULT
> - * functionality.
> - */
> - bool no_foreign_keys =
> - sqlite3FkReferences(pTab) == NULL ||
> - (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
> - bool use_seek = isReplace == 0 ||
> - (pTrigger == NULL && no_foreign_keys);
> - vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], use_seek,
> - onError);
> + vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
> }
> /* Update the count of rows that are inserted
> @@ -1491,20 +1475,16 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> }
> void
> -vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
> - bool use_seek_result, u8 on_error)
> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
> {
> assert(v != NULL);
> - u16 pik_flags = OPFLAG_NCHANGE;
> - if (use_seek_result)
> - pik_flags |= OPFLAG_USESEEKRESULT;
> -
> int opcode;
> if (on_error == ON_CONFLICT_ACTION_REPLACE)
> opcode = OP_IdxReplace;
> else
> opcode = OP_IdxInsert;
> + u16 pik_flags = OPFLAG_NCHANGE;
> if (on_error == ON_CONFLICT_ACTION_IGNORE)
> pik_flags |= OPFLAG_OE_IGNORE;
> else if (on_error == ON_CONFLICT_ACTION_FAIL)
> @@ -1910,7 +1890,6 @@ xferOptimization(Parse * pParse, /* Parser context */
> }
> for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
> - u8 idxInsFlags = 0;
> for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
> pSrcIdx = pSrcIdx->pNext) {
> if (xferCompatibleIndex(pDestIdx, pSrcIdx))
> @@ -1927,11 +1906,9 @@ xferOptimization(Parse * pParse, /* Parser context */
> addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
> VdbeCoverage(v);
> sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
> - if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) {
> - idxInsFlags |= OPFLAG_NCHANGE;
> - }
> sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
> - sqlite3VdbeChangeP5(v, idxInsFlags | OPFLAG_APPEND);
> + if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
Nitpicking: you can use IsPimaryKeyIndex() macros for this purpose.
> + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1);
> VdbeCoverage(v);
> sqlite3VdbeJumpHere(v, addr1);
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 95bd7d656..9e64b434e 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1151,7 +1151,6 @@ selectInnerLoop(Parse * pParse, /* The parser context */
> if (eDest == SRT_DistQueue) {
> sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1,
> r3);
> - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
> }
> for (i = 0; i < nKey; i++) {
> sqlite3VdbeAddOp2(v, OP_SCopy,
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index e786cf842..7c0a214b5 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3039,8 +3039,6 @@ struct Parse {
> /* Also used in P2 (not P5) of OP_Delete */
> #define OPFLAG_EPHEM 0x01 /* OP_Column: Ephemeral output is ok */
> #define OPFLAG_ISUPDATE 0x04 /* This OP_Insert is an sql UPDATE */
> -#define OPFLAG_APPEND 0x08 /* This is likely to be an append */
> -#define OPFLAG_USESEEKRESULT 0x10 /* Try to avoid a seek in BtreeInsert() */
> #define OPFLAG_OE_IGNORE 0x200 /* OP_IdxInsert: Ignore flag */
> #define OPFLAG_OE_FAIL 0x400 /* OP_IdxInsert: Fail flag */
> #ifdef SQLITE_ENABLE_PREUPDATE_HOOK
> @@ -3722,13 +3720,11 @@ void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
> * @param v Virtual database engine.
> * @param cursor_id Primary index cursor.
> * @param tuple_id Register with data to insert.
> - * @param use_seek_result True to set the USESEEKRESULT flag on
> - * OP_[Idx]Insert.
> * @param on_error Error action on failed insert/replace.
> */
> void
> -vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
> - bool use_seek_result, u8 on_error);
> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
> + u8 on_error);
> int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
> int *, u8, u8);
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 859189a7e..98e696ddf 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -615,8 +615,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
> }
> /* Insert the new index entries and the new record. */
> - vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], false,
> - onError);
> + vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
> /* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
> * handle rows (possibly in other tables) that refer via a foreign key
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index bf8a27709..5a56727da 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4321,24 +4321,11 @@ case OP_Next: /* jump */
> /* Opcode: IdxInsert P1 P2 * * P5
> * Synopsis: key=r[P2]
> *
> - * Register P2 holds an SQL index key made using the
> - * MakeRecord instructions. This opcode writes that key
> - * into the index P1. Data for the entry is nil.
> - *
> - * If P5 has the OPFLAG_APPEND bit set, that is a hint to the b-tree layer
> - * that this insert is likely to be an append.
> - *
> - * If P5 has the OPFLAG_NCHANGE bit set, then the change counter is
> - * incremented by this instruction. If the OPFLAG_NCHANGE bit is clear,
> - * then the change counter is unchanged.
> - *
> - * If the OPFLAG_USESEEKRESULT flag of P5 is set, the implementation might
> - * run faster by avoiding an unnecessary seek on cursor P1. However,
> - * the OPFLAG_USESEEKRESULT flag must only be set if there have been no prior
> - * seeks on the cursor or if the most recent seek used a key equivalent
> - * to P2.
> - *
> - * This instruction only works for indices.
> + * @param P1 Index of a space cursor.
> + * @param P2 Index of a register with MessagePack data to insert.
> + * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
> + * accounts the change in a case of successful insertion in
> + * nChange counter.
> */
> /* Opcode: IdxReplace P1 P2 * * P5
> * Synopsis: key=r[P2]
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 79de5d69a..c51bd7455 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -844,7 +844,6 @@ constructAutomaticIndex(Parse * pParse, /* The parsing context */
> sqlite3GenerateIndexKey(pParse, pIdx, pLevel->iTabCur, regRecord, 0,
> 0, 0);
> sqlite3VdbeAddOp2(v, OP_IdxInsert, pLevel->iIdxCur, regRecord);
> - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
> if (pPartial)
> sqlite3VdbeResolveLabel(v, iContinue);
> if (pTabItem->fg.viaCoroutine) {
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index a3c51eef4..aaffa7c1c 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1766,10 +1766,6 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
> sqlite3VdbeAddOp2
> (v, OP_IdxInsert,
> regRowset, regPk);
> - if (iSet)
> - sqlite3VdbeChangeP5
> - (v,
> - OPFLAG_USESEEKRESULT);
> }
> /* Release the array of temp registers */
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
2018-04-04 11:51 ` n.pettik
@ 2018-04-04 14:48 ` n.pettik
0 siblings, 0 replies; 15+ messages in thread
From: n.pettik @ 2018-04-04 14:48 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
[-- Attachment #1: Type: text/plain, Size: 11296 bytes --]
Now the whole patch-set looks OK to me.
> On 4 Apr 2018, at 14:51, n.pettik <korablev@tarantool.org> wrote:
>
> You may not pay attention to ’nitpickings’,
> but I should make visibility of reviewing this patch.
>
>> On 4 Apr 2018, at 13:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote:
>>
>> Hello. See the diff at the end of the letter, and my responses to some of
>> your comments.
>>>> done in a primary index only. It is enough to pass a register of
>>>> a primary index.
>>> Nitpicking: not 'a register of a primary index’,
>>> but number of cursor opened on this space/index.
>> Here you are wrong - it is a register, where tuple data is stored to insert. Cursor id
>> argument is not changed. I fixed it in a new commit.
>
> Ok, I have misunderstood you.
> Now it is clear that you mentioned another operand.
>
>>>
>>>> + bool no_foreign_keys =
>>>> + sqlite3FkReferences(pTab) == NULL ||
>>>> + (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
>>> AFAIK SQLITE_ForeignKeys flag is always set to be true
>>> (since we always use FK). Thus, you can remove this condition check.
>> Yes, Foreign Keys source code now is always built, because we have removed OMIT_FOREIGN_KEYS,
>> but SQLITE_ForeignKeys is not a #define - it is a flag, that can be set by a user on runtime in its
>> session, and allows to skip foreign keys checking. The removal of this flags must be discussed,
>> because I think it can be useful.
>>
>> In any case, talking about the patch, I found a way to remove this code completely. See diff below.
>
> Ok, looks pretty good.
>
>>
>>
>> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
>> index c5dcedd3e..4eae896ab 100644
>> --- a/src/box/sql/analyze.c
>> +++ b/src/box/sql/analyze.c
>> @@ -1050,7 +1050,6 @@ analyzeOneTable(Parse * pParse, /* Parser context */
>> sqlite3VdbeAddOp4(v, OP_MakeRecord, regTabname, 3, regTemp,
>> "BBB", 0);
>> sqlite3VdbeAddOp2(v, OP_IdxInsert, iStatCur, regTemp);
>> - sqlite3VdbeChangeP5(v, OPFLAG_APPEND);
>> /* Add the entries to the stat4 table. */
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 5e3ed0f39..5c3cac881 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2659,7 +2659,6 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
>> sqlite3VdbeAddOp3(v, OP_SorterData, iSorter, regRecord, iIdx);
>> sqlite3VdbeAddOp3(v, OP_Last, iIdx, 0, -1);
>> sqlite3VdbeAddOp2(v, OP_IdxInsert, iIdx, regRecord);
>> - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
>> sqlite3ReleaseTempReg(pParse, regRecord);
>> sqlite3VdbeAddOp2(v, OP_SorterNext, iSorter, addr2);
>> VdbeCoverage(v);
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index c7fe92cd6..c76910e5b 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -851,23 +851,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
>> ipkColumn >= 0, onError,
>> endOfLoop, &isReplace, 0);
>> sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
>> -
>> - /* Set the OPFLAG_USESEEKRESULT flag if either (a) there are no REPLACE
>> - * constraints or (b) there are no triggers and this table is not a
>> - * parent table in a foreign key constraint. It is safe to set the
>> - * flag in the second case as if any REPLACE constraint is hit, an
>> - * OP_Delete or OP_IdxDelete instruction will be executed on each
>> - * cursor that is disturbed. And these instructions both clear the
>> - * VdbeCursor.seekResult variable, disabling the OPFLAG_USESEEKRESULT
>> - * functionality.
>> - */
>> - bool no_foreign_keys =
>> - sqlite3FkReferences(pTab) == NULL ||
>> - (user_session->sql_flags & SQLITE_ForeignKeys) == 0;
>> - bool use_seek = isReplace == 0 ||
>> - (pTrigger == NULL && no_foreign_keys);
>> - vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], use_seek,
>> - onError);
>> + vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
>> }
>> /* Update the count of rows that are inserted
>> @@ -1491,20 +1475,16 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
>> }
>> void
>> -vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
>> - bool use_seek_result, u8 on_error)
>> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, u8 on_error)
>> {
>> assert(v != NULL);
>> - u16 pik_flags = OPFLAG_NCHANGE;
>> - if (use_seek_result)
>> - pik_flags |= OPFLAG_USESEEKRESULT;
>> -
>> int opcode;
>> if (on_error == ON_CONFLICT_ACTION_REPLACE)
>> opcode = OP_IdxReplace;
>> else
>> opcode = OP_IdxInsert;
>> + u16 pik_flags = OPFLAG_NCHANGE;
>> if (on_error == ON_CONFLICT_ACTION_IGNORE)
>> pik_flags |= OPFLAG_OE_IGNORE;
>> else if (on_error == ON_CONFLICT_ACTION_FAIL)
>> @@ -1910,7 +1890,6 @@ xferOptimization(Parse * pParse, /* Parser context */
>> }
>> for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
>> - u8 idxInsFlags = 0;
>> for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
>> pSrcIdx = pSrcIdx->pNext) {
>> if (xferCompatibleIndex(pDestIdx, pSrcIdx))
>> @@ -1927,11 +1906,9 @@ xferOptimization(Parse * pParse, /* Parser context */
>> addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
>> VdbeCoverage(v);
>> sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
>> - if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) {
>> - idxInsFlags |= OPFLAG_NCHANGE;
>> - }
>> sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
>> - sqlite3VdbeChangeP5(v, idxInsFlags | OPFLAG_APPEND);
>> + if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
>
> Nitpicking: you can use IsPimaryKeyIndex() macros for this purpose.
>
>> + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
>> sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1);
>> VdbeCoverage(v);
>> sqlite3VdbeJumpHere(v, addr1);
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index 95bd7d656..9e64b434e 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -1151,7 +1151,6 @@ selectInnerLoop(Parse * pParse, /* The parser context */
>> if (eDest == SRT_DistQueue) {
>> sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1,
>> r3);
>> - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
>> }
>> for (i = 0; i < nKey; i++) {
>> sqlite3VdbeAddOp2(v, OP_SCopy,
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index e786cf842..7c0a214b5 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -3039,8 +3039,6 @@ struct Parse {
>> /* Also used in P2 (not P5) of OP_Delete */
>> #define OPFLAG_EPHEM 0x01 /* OP_Column: Ephemeral output is ok */
>> #define OPFLAG_ISUPDATE 0x04 /* This OP_Insert is an sql UPDATE */
>> -#define OPFLAG_APPEND 0x08 /* This is likely to be an append */
>> -#define OPFLAG_USESEEKRESULT 0x10 /* Try to avoid a seek in BtreeInsert() */
>> #define OPFLAG_OE_IGNORE 0x200 /* OP_IdxInsert: Ignore flag */
>> #define OPFLAG_OE_FAIL 0x400 /* OP_IdxInsert: Fail flag */
>> #ifdef SQLITE_ENABLE_PREUPDATE_HOOK
>> @@ -3722,13 +3720,11 @@ void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
>> * @param v Virtual database engine.
>> * @param cursor_id Primary index cursor.
>> * @param tuple_id Register with data to insert.
>> - * @param use_seek_result True to set the USESEEKRESULT flag on
>> - * OP_[Idx]Insert.
>> * @param on_error Error action on failed insert/replace.
>> */
>> void
>> -vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
>> - bool use_seek_result, u8 on_error);
>> +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id,
>> + u8 on_error);
>> int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
>> int *, u8, u8);
>> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
>> index 859189a7e..98e696ddf 100644
>> --- a/src/box/sql/update.c
>> +++ b/src/box/sql/update.c
>> @@ -615,8 +615,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
>> }
>> /* Insert the new index entries and the new record. */
>> - vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], false,
>> - onError);
>> + vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], onError);
>> /* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
>> * handle rows (possibly in other tables) that refer via a foreign key
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index bf8a27709..5a56727da 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -4321,24 +4321,11 @@ case OP_Next: /* jump */
>> /* Opcode: IdxInsert P1 P2 * * P5
>> * Synopsis: key=r[P2]
>> *
>> - * Register P2 holds an SQL index key made using the
>> - * MakeRecord instructions. This opcode writes that key
>> - * into the index P1. Data for the entry is nil.
>> - *
>> - * If P5 has the OPFLAG_APPEND bit set, that is a hint to the b-tree layer
>> - * that this insert is likely to be an append.
>> - *
>> - * If P5 has the OPFLAG_NCHANGE bit set, then the change counter is
>> - * incremented by this instruction. If the OPFLAG_NCHANGE bit is clear,
>> - * then the change counter is unchanged.
>> - *
>> - * If the OPFLAG_USESEEKRESULT flag of P5 is set, the implementation might
>> - * run faster by avoiding an unnecessary seek on cursor P1. However,
>> - * the OPFLAG_USESEEKRESULT flag must only be set if there have been no prior
>> - * seeks on the cursor or if the most recent seek used a key equivalent
>> - * to P2.
>> - *
>> - * This instruction only works for indices.
>> + * @param P1 Index of a space cursor.
>> + * @param P2 Index of a register with MessagePack data to insert.
>> + * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE
>> + * accounts the change in a case of successful insertion in
>> + * nChange counter.
>> */
>> /* Opcode: IdxReplace P1 P2 * * P5
>> * Synopsis: key=r[P2]
>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index 79de5d69a..c51bd7455 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -844,7 +844,6 @@ constructAutomaticIndex(Parse * pParse, /* The parsing context */
>> sqlite3GenerateIndexKey(pParse, pIdx, pLevel->iTabCur, regRecord, 0,
>> 0, 0);
>> sqlite3VdbeAddOp2(v, OP_IdxInsert, pLevel->iIdxCur, regRecord);
>> - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT);
>> if (pPartial)
>> sqlite3VdbeResolveLabel(v, iContinue);
>> if (pTabItem->fg.viaCoroutine) {
>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index a3c51eef4..aaffa7c1c 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -1766,10 +1766,6 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
>> sqlite3VdbeAddOp2
>> (v, OP_IdxInsert,
>> regRowset, regPk);
>> - if (iSet)
>> - sqlite3VdbeChangeP5
>> - (v,
>> - OPFLAG_USESEEKRESULT);
>> }
>> /* Release the array of temp registers */
[-- Attachment #2: Type: text/html, Size: 41149 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tarantool-patches] Re: [PATCH 0/4] sql: refactor insertion and lua
2018-04-03 15:34 [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua Vladislav Shpilevoy
` (3 preceding siblings ...)
2018-04-03 15:34 ` [tarantool-patches] [PATCH 4/4] sql: remove useless branching in insertOrReplace Vladislav Shpilevoy
@ 2018-04-05 11:43 ` Kirill Yukhin
4 siblings, 0 replies; 15+ messages in thread
From: Kirill Yukhin @ 2018-04-05 11:43 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, Vladislav Shpilevoy
Hello Vlad,
On 03 апр 18:34, Vladislav Shpilevoy wrote:
> Branch: http://github.com/tarantool/tarantool/tree/gh-2618-preliminary-patches
> Issue: https://github.com/tarantool/tarantool/issues/2618
>
> The patchset consists of patches preliminary to SQL IProto
> "last updated tuple" introduction. It is possible, that its
> implementation plan will be changed several times, but this
> patchset looks useful even with no IProto "last updated tuple".
>
> 1) The patch does SQL INSERT/UPDATE refactoring by removal of 2
> unused VDBE parameters, and by restyling of final insertion
> opcodes generator: sqlite3CompleteInsertion.
>
> 2) The patch does refactoring of Lua SQL executor, that is
> extremely overengineered now.
>
> Vladislav Shpilevoy (4):
> sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
> sql: remove unused error codes, use enum instead of defines
> sql: simplify lua SQL executor
> sql: remove useless branching in insertOrReplace
The patchset LGTM. I've comitted it to 2.0
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-04-05 11:43 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-03 15:34 [tarantool-patches] [PATCH 0/4] sql: refactor insertion and lua Vladislav Shpilevoy
2018-04-03 15:34 ` [tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] Vladislav Shpilevoy
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
2018-04-04 10:26 ` Vladislav Shpilevoy
2018-04-04 11:51 ` n.pettik
2018-04-04 14:48 ` n.pettik
2018-04-03 15:34 ` [tarantool-patches] [PATCH 2/4] sql: remove unused error codes, use enum instead of defines Vladislav Shpilevoy
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
2018-04-04 10:26 ` Vladislav Shpilevoy
2018-04-04 11:30 ` n.pettik
2018-04-03 15:34 ` [tarantool-patches] [PATCH 3/4] sql: simplify lua SQL executor Vladislav Shpilevoy
2018-04-03 23:02 ` [tarantool-patches] " n.pettik
2018-04-03 15:34 ` [tarantool-patches] [PATCH 4/4] sql: remove useless branching in insertOrReplace Vladislav Shpilevoy
2018-04-03 23:01 ` [tarantool-patches] " n.pettik
2018-04-05 11:43 ` [tarantool-patches] Re: [PATCH 0/4] sql: refactor insertion and lua Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox