[patches] [PATCH 8/8] sql: remove backend authorization, OP_Transaction

[n.pettik]

Please, add link to the issue to cover letter. Moreover, it would be great if
you provided short summury of your patch in cover letter (but still this is
optional).

>Remove all code under OMIT_AUTOINCREMENT define and refactor the rest

I suppose, the problem in ticket was incorrectly declared initially:
you don't need to delete ALL code inside this define, but delete this define
itself (because we ALWAYS use autoincrement). However, some parts turn out
to be really unused, such as OP_MemMax and should be removed. Thus, paraphrase
commit message.

Also, put dot at the end of sentence. It is applied not only for this commit
message, but for other too. We don't put dots only in commit header.

>-730,16 +730,6 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>-	/* If this is the magic _sequence table used by autoincrement,
>-	 * then record a pointer to this table in the main database structure
>-	 * so that INSERT can find the table easily.
>-	 */
>-#ifndef SQLITE_OMIT_AUTOINCREMENT
>-	if (!pParse->nested && strcmp(zName, "_SEQUENCE") == 0) {
>-		pTable->pSchema->pSeqTab = pTable;
>-	}
>-#endif
>-

I wouldn't remove this code: it seems to be functional. But delete defines.

You also forget to remove one more appearence in sqlite3EndTable: just grep it.

>-813,7 +813,7 @@ sqlite3GenerateRowDelete(Parse * pParse,	/* Parsing context */
>		/* sqlite3GenerateRowIndexDelete(pParse, pTab, iDataCur, iIdxCur,0,iIdxNoSeek);  */
>		sqlite3VdbeAddOp2(v, OP_Delete, iDataCur,
>				  (count ? OPFLAG_NCHANGE : 0));
>-		sqlite3VdbeAppendP4(v, (char *)pTab, P4_TABLE);
>+		sqlite3VdbeAppendP4(v, (char *)pTab, P4_STATIC);

OP_Delete doesn't use P4 register at all: there is no sence to set type of P4.

>@@ -629,7 +629,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>				    || pPk != 0) ? 0 : OPFLAG_ISNOOP),
>				  regNewRowid);
>		if (!pParse->nested) {
>-			sqlite3VdbeAppendP4(v, pTab, P4_TABLE);
>+			sqlite3VdbeAppendP4(v, pTab, P4_STATIC);

The same is here. (In point of fact, this is dead code, but anyway).

>@@ -92,10 +92,6 @@ int sqlite3CursorMovetoUnpacked(BtCursor *, UnpackedRecord * pUnKey, int *pRes);
>struct CursorPayload {
>	const void *pKey;	/* Key content for indexes.  NULL for tables */
>	sqlite3_int64 nKey;	/* Size of pKey for indexes.  PRIMARY KEY for tabs */
>-	const void *pData;	/* Data for tables.  NULL for indexes */
>-	struct Mem *aMem;	/* First of nMem value in the unpacked pKey */
>-	u16 nMem;		/* Number of aMem[] value.  Might be zero */
>-	int nData;		/* Size of pData.  0 if none. */
>};

Look: BtCursor the same as ta_cursor have nPkey and char*/void* fields
to hold information about key size and payload itself. So, suggestion is to
remove CursorPayload structure completely and delegate resposobility of
holding payload to BtCursor or ta_cursor.

>-182,15 +181,6 @@ sqlite3InitDatabase(sqlite3 * db)
>	/* Create a cursor to hold the database open
>	 */
>
>-	/* Tarantool: schema_cookie is not used so far, but
>-	 * might be used in future. Set it to dummy value.
>-	 */
>-	pDb->pSchema->schema_cookie = 0;
>-
>-	if (pDb->pSchema->cache_size == 0) {
>-		pDb->pSchema->cache_size = SQLITE_DEFAULT_CACHE_SIZE;
>-	}

I suppose, this comment was left for a resason. schema_cookie should be
an alias for schema_version from Tarantool. For instance, when schema is
changed, all prepared statements should be invalidated.

>Remove every code used for sqlite backend authorization, because it is
>completely unnecessary now. Also remove OP_Transaction, modify
>OP_TTransaction, regenerate opcodes.

Why have you put them into one patch? I don't think that OP_Transaction
intersects heavily with authorization.

@@ -111,12 +99,9 @@ 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 ? 1 : 0,
+						  DbMaskTest(pParse->writeMask, 0));

Why did you rewrite 'if' branch with ternary operator? It makes no sense,
but adds extra diff.

@@ -797,7 +797,9 @@ sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab)

		pParse->disableTriggers = 1;
		/* Staring new transaction before DELETE FROM <tbl> */
-		sqlite3VdbeAddOp0(v, OP_TTransaction);
+		sqlite3VdbeAddOp2(v, OP_TTransaction,
+				  1,
+				  DbMaskTest(pParse->writeMask, 0));

Why have you put "1" at separate line? Looks awful.

>+/* Opcode: TTransaction P1 P2 * * *

It should be: "Opcode: OP_TTransaction ...". If you add new arguments to
this opcode, provide comment for them.

>+	assert(p->readOnly==0 || pOp->p2==0);

Why have you added this assert? I think we are willing to remove readOnly,
isReader and other fields, which are not applied to Tarantool.

>+		if (box_txn()
>+				&& p->autoCommit == 0){

Wrong alignment: fit condition into one line.

>+			rc = box_txn_begin() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR;

Make it fit into 80 chars.

@@ -3209,7 +3164,6 @@ case OP_ReopenIdx: {
	 */
case OP_OpenRead:
case OP_OpenWrite:
-

Extra diff.

Also, remove ALL other code, which is connected with authorization.
For instance, it includes SQL_DENY/SQL_AUTH define, cmake file etc.
Fix commets which mentions authorization.

>@@ -4384,10 +4379,10 @@ getFileMode(const char *zFile,	/* File name */
>- * But if the file being opened is a WAL or regular journal file, then
>+ * But if the file being opened is a regular journal file, then

>-	if (flags & (SQLITE_OPEN_WAL | SQLITE_OPEN_MAIN_JOURNAL)) {
>+	if (flags & (SQLITE_OPEN_MAIN_JOURNAL)) {

You say that you have removed journal mode and memjournal, but you
still use main journal. Thus, whether update commit message and explain
why you have left some usages of journal, OR remove journal remains.

>For #3119

/* Very specific remark. */
I stick to the point that "for" is used when you have implemeted separate
feature, which doesn't belong to your initial intent. When you are implementing
ticket which consists from several points (such as yours), better use "Part
of" instead. But it is up to you.