Re[2]: [patches] [PATCH V3 00/10] sql: SQLite backend removal

[Bulat Niatshin]

Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-3119-backend-removal  

///////////////////////////////////////////////////////////////////////

PATCH V3 04/10] sql: remove struct Db

>diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
>@ -182,7 +181,6 @@ sqlite3InitDatabase(sqlite3 * db)
>
> /* Create a cursor to hold the database open
> */
>- pDb = &db->mdb;

>Also, remove comment: it seems to belong to code you deleted.

-----------------------------------------------------------
- /* Create a cursor to hold the database open
- */
- pDb = &db->mdb;
-

///////////////////////////////////////////////////////////////////////

[PATCH V3 08/10] sql: delete CursorPayload structure

>@@ -519,36 +519,36 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
> return SQLITE_OK;
>}
>
>-static int insertOrReplace(BtCursor *pCur, const CursorPayload *pX,
>- int operationType)
>+static int insert_or_replace(BtCursor *pCur, int operation_type)
>{
>+ assert(pCur != NULL);
> assert(pCur->curFlags & BTCF_TaCursor);
>- assert(operationType == TARANTOOL_INDEX_INSERT ||
>- operationType == TARANTOOL_INDEX_REPLACE);
>+ assert(operation_type == TARANTOOL_INDEX_INSERT ||
>+ operation_type == TARANTOOL_INDEX_REPLACE);


Also travis build seems ok - https://travis-ci.org/tarantool/tarantool/builds/349785542

> If you are fixing codestyle (which is not encoureged until
> it is the main goal of patch OR it is already trapped in your patch),
> then fix name of pCur: in Tarantool we don't use hungarian notation.
>
> Also, not sure if assert which you added is suitable:
> insert and replace funcs are called only from one place.
> Moreover, there dereference happens firsly, thus such assertion
> will never be fired. Don't use asserts just for make visibility of safety.
---------------------------------------------------------------
-static int insertOrReplace(BtCursor *pCur, const CursorPayload *pX,
- int operationType)
+static int insertOrReplace(BtCursor *pCur, int operationType)
{
+ assert(pCur != NULL);
assert(pCur->curFlags & BTCF_TaCursor);
- assert(operationType == TARANTOOL_INDEX_INSERT ||
- operationType == TARANTOOL_INDEX_REPLACE);
int space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
int rc;
if (operationType == TARANTOOL_INDEX_INSERT) {
- rc = box_insert(space_id, pX->pKey,
- (const char *)pX->pKey + pX->nKey,
+ rc = box_insert(space_id, pCur->pKey,
+ (const char *)pCur->pKey + pCur->nKey,
NULL /* result */);
} else {
- rc = box_replace(space_id, pX->pKey,
- (const char *)pX->pKey + pX->nKey,
+ rc = box_replace(space_id, pCur->pKey,
+ (const char *)pCur->pKey + pCur->nKey,
NULL /* result */);
}
return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;;
}
-int tarantoolSqlite3Insert(BtCursor *pCur, const CursorPayload *pX)
+int tarantoolSqlite3Insert(BtCursor *pCur)
{
- return insertOrReplace(pCur, pX, TARANTOOL_INDEX_INSERT);
+ return insertOrReplace(pCur, TARANTOOL_INDEX_INSERT);
}
-int tarantoolSqlite3Replace(BtCursor *pCur, const CursorPayload *pX)
+int tarantoolSqlite3Replace(BtCursor *pCur)
{
- return insertOrReplace(pCur, pX, TARANTOOL_INDEX_REPLACE);
+ return insertOrReplace(pCur, TARANTOOL_INDEX_REPLACE);
}
///////////////////////////////////////////////////////////////////

>- assert((x.pKey == 0) == (pBtCur->pKeyInfo == 0));

>Why have you deleted this assert? You are able to rewrite it using
>BtCur->pKey.
--------------------------------------------------------

+ assert((pIn2->z == 0) == (pBtCur->pKeyInfo == 0));

////////////////////////////////////////////////////////////////////

>+ * release it, so memory
>+ * should be explicitly nulllified.

> I guess, not memory but pointers should be nullified.

----------------------------
+ * release it, so pointers should be explicitly
+ * nullified.

//////////////////////////////////////////////////////////////////////

PATCH V3 09/10] sql: remove unnecessary VDBE flags

>This flags are unnecessary for us, so in this patch
>they were removed (and all related to them).

>I guess, phrase "all related to them" isn't correct.
>I would better say sort of "everything related to them".

that opcode VDBE flags like 'readOnly', 'bIsReader'
'''
would be assigned incorrectly. Also unnecessary OP_Destroy
opcode was removed.
This flags are unnecessary for us, so in this patch
they were removed (and everything related to them).
''''

//////////////////////////////////////////////////////////////////////////

>You can completely remove this opcode: AFAIK it is not used anymore.
>But it is up to you.

--------
-/* Opcode: Destroy P1 P2 P3 * *
- *
- * Delete an entire database table or index whose root page in the database
- * file is given by P1.
- *
- * The table being destroyed is in the main database file if P3==0. If
- * P3==1 then the table to be clear is in the auxiliary database file
- * that is used to store tables create using CREATE TEMPORARY TABLE.
- *
- * Zero is stored in register P2.
- *
- * See also: Clear
- */
-case OP_Destroy: { /* out2 */
- int iMoved;
-
- assert(p->readOnly==0);
- assert(pOp->p1>1);
- pOut = out2Prerelease(p, pOp);
- pOut->flags = MEM_Null;
- if (db->nVdbeRead > 1) {
- rc = SQLITE_LOCKED;
- p->errorAction = ON_CONFLICT_ACTION_ABORT;
- goto abort_due_to_error;
- } else {
- iMoved = 0; /* Not needed. Only to silence a warning. */
- pOut->flags = MEM_Int;
- pOut->u.i = iMoved;
- if (rc) goto abort_due_to_error;
- }
- break;
-}
-
@@ -581,7 +581,6 @@ opIterNext(VdbeOpIter * p)
*
* * OP_Halt with P1=SQLITE_CONSTRAINT and P2=ON_CONFLICT_ACTION_ABORT.
* * OP_HaltIfNull with P1=SQLITE_CONSTRAINT and P2=ON_CONFLICT_ACTION_ABORT.
- * * OP_Destroy
* * OP_FkCounter with P2==0 (immediate foreign key constraint)
*
* Then check that the value of Parse.mayAbort is true if an
@@ -603,11 +602,9 @@ sqlite3VdbeAssertMayAbort(Vdbe * v, int mayAbort)
while ((pOp = opIterNext(&sIter)) != 0) {
int opcode = pOp->opcode;
- if (opcode == OP_Destroy ||
- ((opcode == OP_Halt || opcode == OP_HaltIfNull)
- && ((pOp->p1 & 0xff) == SQLITE_CONSTRAINT
- && pOp->p2 == ON_CONFLICT_ACTION_ABORT))
- ) {
+ if ((opcode == OP_Halt || opcode == OP_HaltIfNull) &&
+ (pOp->p1 & 0xff) == SQLITE_CONSTRAINT &&
+ pOp->p2 == ON_CONFLICT_ACTION_ABORT){
hasAbort = 1;
break;
}

--- a/src/box/sql/opcodes.h
+++ b/src/box/sql/opcodes.h
@@ -119,26 +119,25 @@
#define OP_IdxReplace 116 /* synopsis: key=r[P2] */
#define OP_IdxInsert 117 /* synopsis: key=r[P2] */
#define OP_IdxDelete 118 /* synopsis: key=r[P2 at P3] */
-#define OP_Destroy 119




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180306/5f7c55ce/attachment.html>