[patches] [PATCH V2 8/9] sql: remove sqlite authorization

[n.pettik]

>Remove P4_Table type from 4th VDBE argument, because there are no tables
>anymore

Put dot at the end of sentences, please.

>Remove code under ifndef OMIT_DEPRECATED

The same.

>@@ -382,12 +380,9 @@ sqlite3UnlinkAndDeleteIndex(sqlite3 * db, Index * pIndex)
>void
>sqlite3ResetOneSchema(sqlite3 * db)
>{
>-	Db *pDb;
>-
>	/* Case 1:  Reset the single schema of the database  */
>-	pDb = &db->mdb;
>-	assert(pDb->pSchema != 0);
>-	sqlite3SchemaClear(pDb->pSchema);
>+	assert(db->pSchema != 0);
>+	sqlite3SchemaClear(db->pSchema);
>}

Since now, this function does nothing, it is just wrapper around sqlite3SchemaClear().
I think, it should be removed.

>@@ -4153,14 +4143,12 @@ reindexTable(Parse * pParse, Table * pTab, char const *zColl)
>+	assert(db->pSchema);

In Tarantool we prefer explicit != NULL.

>@@ -163,10 +163,9 @@ extern int
>sqlite3InitDatabase(sqlite3 * db)
>{
>-	assert(db->mdb.pSchema);
>+	assert(db->pSchema);

The same.

>@@ -387,12 +387,14 @@ sqlite3FindFunction(sqlite3 * db,	/* An open database */
> * The Schema.cache_size variable is not cleared.
> */
>void
>-sqlite3SchemaClear(void *p)
>+sqlite3SchemaClear(sqlite3 * db)
>
>+	assert(db->pSchema);

The same. Fix all occurences (I found several more in patch set).

>+++ b/src/box/sql/main.c
>@@ -2373,7 +2373,7 @@ openDatabase(const char *zFilename,	/* Database filename UTF-8 encoded */
>		goto opendb_out;
>	}
>
>-	db->pSchema = sqlite3SchemaCreate(db);
>+	db->pSchema = 0;

Better use NULL instead of 0 for pointers.

>+++ b/src/box/sql/sqliteInt.h
>@@ -892,39 +892,12 @@ typedef int VList;
 >* An instance of the following structure stores a database schema.
 >*/
>struct Schema {
>-	int schema_cookie;	/* Database schema version number for this file */
>+	int schema_cookie;      /* Database schema version number for this file */

Redundant diff.

>@@ -488,7 +488,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
> *
> * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
> */
>-int tarantoolSqlite3EphemeralInsert(BtCursor *pCur, const CursorPayload *pX)
>+int tarantoolSqlite3EphemeralInsert(BtCursor *pCur, const BtCursor *pX)
>
>@@ -4457,7 +4457,7 @@ case OP_SorterInsert:       /* in2 */
>case OP_IdxReplace:
>case OP_IdxInsert: {        /* in2 */
>	VdbeCursor *pC;
>-	CursorPayload x;
>+	BtCursor x;

You have misunderstood me. You don't need to pass key and its length
in separate BtCursor structure, one will be enough. But you should be
careful with resource owning: vdbe will attempt at deallocating them
twice (once when freeing BtCursor and than when feeing opcode operands).
Thus, after call you should nullify them (in BtCursor).

>+++ b/src/box/sql/update.c
>@@ -220,9 +220,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>
>	/* Resolve the column names in all the expressions of the
>	 * of the UPDATE statement.  Also find the column index
>-	 * for each column to be updated in the pChanges array.  For each
>-	 * column to be updated, make sure we have authorization to change
>-	 * that column.
>+	 * for each column to be updated in the pChanges array.
>	 */

I don't think that this fix belongs to patch on removing OP_Transaction.

>+++ b/src/box/sql/vdbe.c
>@@ -3016,69 +3016,31 @@ 
>+/* Opcode: TTransaction P1 P2 * * *
>+ *
>+ * Start Tarantool's transaction if P1 is not zero. If P2 is
>+ * non-zero, then write-transaction is started (from VDBE
>+ * point of view), if P2 is zero, then a read-transaction
>+ * is started (temporarily leave it as it is, VDBE
>+ * flags like readOnly and others will be removed later).

But you don't need (and actually you don't use) P2 arg.
Fix comment.

Moreover, you even don't need P1 arg: the only place where you
are using it:

>+++ b/src/box/sql/build.c
>@@ -99,12 +99,10 @@ sqlite3FinishCoding(Parse * pParse)
>			Schema *pSchema;
>			if (DbMaskTest(pParse->cookieMask, 0) != 0) {
>				pSchema = db->pSchema;
>-				sqlite3VdbeAddOp2(v, OP_Transaction,	/* Opcode */
>-						  0,	/* P1 */
>-						  DbMaskTest(pParse->writeMask, 0)	/* P2 */
>-				    );
>-				if (pParse->initiateTTrans)
>-					sqlite3VdbeAddOp0(v, OP_TTransaction);
>+
>+				sqlite3VdbeAddOp2(v, OP_TTransaction,
>+						  pParse->initiateTTrans,
>+						  DbMaskTest(pParse->writeMask, 0));

I don't understand why you delegate this check from compilation time to runtime.
And don't set second arg there, you don't use it anyway.