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

n.pettik korablev at tarantool.org
Mon Mar 5 17:23:18 MSK 2018


Common comments:
1) In commit message we are able to use 72 chars.
So, don't try to put it in less width. Moreover, if you use vim, you
can adjust autoformating: http://www.methods.co.nz/asciidoc/chunked/ch36.html

2) I have noticed that code on your branch doesn't match with one you
have sent. Please, firstly push your patch and THEN send the patch for review.
Without actual version of code it is impossible to check tests, for instance.

3)
>Branch:
>https://github.com/tarantool/tarantool/tree/bn/gh-3119-final

Please, don't change branch of your patches when pushing next versions
(At lest until it is vital for you).

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

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.

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

[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);

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.

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

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

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

I guess, not memory but pointers should be 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".

>@@ -4668,22 +4641,12 @@ case OP_IdxGE:  {       /* jump */
> * 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;
>-	}
>+	pOut->flags = MEM_Int;
>+	pOut->u.i = 0;
>+	if (rc) goto abort_due_to_error;
>	break;
>}

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

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

The rest seems to be OK. You are able not to send next version, but
send only diff you have made since current version.

Also, Travis build has failed. Please, check Travis BEFORE you send patches.
(I hope it is connected with outdated version of patch on your branch)

/Users/travis/build/tarantool/tarantool/src/box/sql/alter.c:153:8: warning: unused variable 'v' [-Wunused-variable]
        Vdbe *v = pParse->pVdbe;        /* The prepared statement under construction */





More information about the Tarantool-patches mailing list