> On 3 Apr 2018, at 20:54, Vladislav Shpilevoy wrote: > > Hello. See 7 comments below. > > 03.04.2018 19:14, Nikita Pettik пишет: >> Some of legacy functions seem to be useless, since they serve as >> wrappers around others; the rest rely on capabilities which are no >> longer relevant. This patch provides slight refactoring of such >> functions. >> >> Removed entities: >> - sqlite3LocateTableItem() - replaced with sqlite3LocateTable(); >> - sqlite3FindTable() - replaced with sqlite3HashFind(); >> - sqlite3ColumnOfIndex() - in Tarantool order of columns always the same; >> - sqlite3FindIndex() - replaced with sqlite3LocateIndex(); >> - sqlite3CodeVerifySchema(); >> - sqlite3SchemaToIndex(); >> - sqlite3MultiWrite(); >> - Parse->cookieMast; >> - Vdbe->usesStmtJournal; >> --- >> src/box/sql/alter.c | 13 ++- >> src/box/sql/analyze.c | 25 +++--- >> src/box/sql/build.c | 213 ++++++++++-------------------------------------- >> src/box/sql/delete.c | 5 +- >> src/box/sql/expr.c | 9 +- >> src/box/sql/fkey.c | 3 +- >> src/box/sql/insert.c | 10 +-- >> src/box/sql/pragma.c | 18 ++-- >> src/box/sql/prepare.c | 29 ------- >> src/box/sql/select.c | 4 +- >> src/box/sql/sqliteInt.h | 11 +-- >> src/box/sql/trigger.c | 7 +- >> src/box/sql/update.c | 2 +- >> src/box/sql/vdbe.c | 1 - >> src/box/sql/vdbeInt.h | 1 - >> src/box/sql/vdbeaux.c | 1 - >> src/box/sql/where.c | 2 - >> src/box/sql/wherecode.c | 7 +- >> 18 files changed, 83 insertions(+), 278 deletions(-) >> >> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c >> index 054c0856c..a19324ed2 100644 >> --- a/src/box/sql/alter.c >> +++ b/src/box/sql/alter.c >> @@ -122,7 +121,7 @@ sqlite3AlterRenameTable(Parse * pParse, /* Parser context. */ >> if (v == 0) { >> goto exit_rename_table; >> } >> - sqlite3BeginWriteOperation(pParse, false); >> + sql_set_multi_write(pParse, false); > 1. Can you please alongside with this patch make sql_set_multi_write take boolean as the last > argument? Now it sometimes gets true/false, sometimes 1/0. It is strange. Done. >> /* Drop and reload the internal table schema. */ >> reloadTableSchema(pParse, pTab, zName); >> >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 5e3ed0f39..61194e06b 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -93,23 +93,16 @@ sqlite3FinishCoding(Parse * pParse) >> * transaction on each used database and to verify the schema cookie >> * on each used database. >> */ >> - if (db->mallocFailed == 0 >> - && (DbMaskNonZero(pParse->cookieMask) || pParse->pConstExpr) >> - ) { >> + if (db->mallocFailed == 0 || pParse->pConstExpr) { > 2. Please, together with cookie mask checking remove or refactor a comment above. Fixed. >> @@ -2240,7 +2164,6 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) >> v = sqlite3GetVdbe(pParse); >> assert(v != 0); >> - sqlite3BeginWriteOperation(pParse, 1); > 3. Why did you delete it with no replacement by sql_set_multi_write(false) ? Because in this particular case is is completely useless: table dropping is implemented as hardcoded opcodes sequences. AFAIK this mask is used to provide kind of optimisation/checks during query compilation. Also, removed: @@ -2414,8 +2326,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr) * space_id from _space. */ - sqlite3BeginWriteOperation(pParse, 1); sql_clear_stat_spaces(pParse, "tbl", pTab->zName); sqlite3FkDropTable(pParse, pName, pTab); sqlite3CodeDropTable(pParse, pTab, isView); >> @@ -2376,15 +2299,12 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr) >> if (noErr) >> db->suppressErr++; >> assert(isView == 0 || isView == LOCATE_VIEW); >> - pTab = sqlite3LocateTableItem(pParse, isView, &pName->a[0]); >> + pTab = sqlite3LocateTable(pParse, isView, pName->a[0].zName); >> if (noErr) >> db->suppressErr--; >> - if (pTab == 0) { >> - if (noErr) >> - sqlite3CodeVerifySchema(pParse); >> + if (pTab == 0) > 4. Lets use ==/!= NULL in all new code. Done. > Same about checking sqlite3HashFind results. And if it is > possible with not huge diff, can you please rename sqlite3HashFind to sql_hash_find ? Is it worth doing? It is going to disappear soon. > > 5. sqlite3MultiWrite in commit message is listed among deleted functions, but its declaration still exists. Fixed: @@ -3718,8 +3689,8 @@ void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int, void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8); int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *, int *, u8, u8); -void sqlite3BeginWriteOperation(Parse *, int); -void sqlite3MultiWrite(Parse *); +void +sql_set_multi_write(Parse *, bool); > > 6. cookieMast - typo in commit message. And how about do not list deleted functions in a commit body? > I can not imagine, that somebody except me will search for any of these functions. And the list is deprecated - > for example, sqlite3BeginWriteOperation is deleted too, but does not presence in the list. Ok, fixed commit message. Left only description. > > 7. How about remove DbMaskTest, DbMaskZero and other dbmask shit? As you wish. Done. Diff is below: diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index a19324ed2..129ef823c 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -85,7 +85,7 @@ sqlite3AlterRenameTable(Parse * pParse, /* Parser context. */ assert(pSrc->nSrc == 1); pTab = sqlite3LocateTable(pParse, 0, pSrc->a[0].zName); - if (!pTab) + if (pTab == NULL) goto exit_rename_table; user_session->sql_flags |= SQLITE_PreferBuiltin; @@ -257,7 +257,7 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc) if (db->mallocFailed) goto exit_begin_add_column; pTab = sqlite3LocateTable(pParse, 0, pSrc->a[0].zName); - if (!pTab) + if (pTab == NULL) goto exit_begin_add_column; /* Make sure this is not an attempt to ALTER a view. */ @@ -304,7 +304,7 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc) pNew->nTabRef = 1; /* Begin a transaction and increment the schema cookie. */ - sql_set_multi_write(pParse, 0); + sql_set_multi_write(pParse, false); v = sqlite3GetVdbe(pParse); if (!v) goto exit_begin_add_column; diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 25e93aa15..60f4eaac4 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -157,6 +157,7 @@ openStatTable(Parse * pParse, /* Parsing context */ Table *pStat; /* The table already exists, because it is a system space */ pStat = sqlite3HashFind(&db->pSchema->tblHash, zTab); + assert(pStat != NULL); aRoot[i] = pStat->tnum; aCreateTbl[i] = 0; if (zWhere) { @@ -1122,7 +1123,7 @@ analyzeDatabase(Parse * pParse) int iMem; int iTab; - sql_set_multi_write(pParse, 0); + sql_set_multi_write(pParse, false); iStatCur = pParse->nTab; pParse->nTab += 3; openStatTable(pParse, iStatCur, 0, 0); @@ -1146,7 +1147,7 @@ analyzeTable(Parse * pParse, Table * pTab, Index * pOnlyIdx) int iStatCur; assert(pTab != 0); - sql_set_multi_write(pParse, 0); + sql_set_multi_write(pParse, true); iStatCur = pParse->nTab; pParse->nTab += 3; if (pOnlyIdx) { @@ -1294,9 +1295,8 @@ analysisLoader(void *pData, int argc, char **argv, char **NotUsed) return 0; } pTable = sqlite3HashFind(&pInfo->db->pSchema->tblHash, argv[0]); - if (pTable == 0) { + if (pTable == NULL) return 0; - } if (argv[1] == 0) { pIndex = 0; } else if (sqlite3_stricmp(argv[0], argv[1]) == 0) { @@ -1631,19 +1631,17 @@ loadStatTbl(sqlite3 * db, /* Database handle */ static int loadStat4(sqlite3 * db) { - int rc = SQLITE_OK; /* Result codes from subroutines */ Table *pTab = 0; /* Pointer to stat table */ assert(db->lookaside.bDisable); pTab = sqlite3HashFind(&db->pSchema->tblHash, "_sql_stat4"); - if (pTab) { - rc = loadStatTbl(db, - pTab, - "SELECT \"tbl\",\"idx\",count(*) FROM \"_sql_stat4\" GROUP BY \"tbl\",\"idx\"", - "SELECT \"tbl\",\"idx\",\"neq\",\"nlt\",\"ndlt\",\"sample\" FROM \"_sql_stat4\""); - } - - return rc; + /* _slq_stat4 is a system space, so it always exists. */ + assert(pTab != NULL); + return loadStatTbl(db, pTab, + "SELECT \"tbl\",\"idx\",count(*) FROM \"_sql_stat4\"" + " GROUP BY \"tbl\",\"idx\"", + "SELECT \"tbl\",\"idx\",\"neq\",\"nlt\",\"ndlt\"," + "\"sample\" FROM \"_sql_stat4\""); } /* diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 61194e06b..c50847a02 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -86,13 +86,6 @@ sqlite3FinishCoding(Parse * pParse) || sqlite3VdbeAssertMayAbort(v, pParse->mayAbort)); if (v) { sqlite3VdbeAddOp0(v, OP_Halt); - - /* The cookie mask contains one bit for each database file open. - * (Bit 0 is for main, bit 1 is for temp, and so forth.) Bits are - * set for each database that is used. Generate code to start a - * transaction on each used database and to verify the schema cookie - * on each used database. - */ if (db->mallocFailed == 0 || pParse->pConstExpr) { int i; assert(sqlite3VdbeGetOp(v, 0)->opcode == OP_Init); @@ -192,7 +185,7 @@ sqlite3LocateTable(Parse * pParse, /* context in which to report errors */ assert(pParse->db->pSchema != NULL); p = sqlite3HashFind(&pParse->db->pSchema->tblHash, zName); - if (p == 0) { + if (p == NULL) { const char *zMsg = flags & LOCATE_VIEW ? "no such view" : "no such table"; if ((flags & LOCATE_NOERR) == 0) { @@ -212,9 +205,8 @@ sqlite3LocateIndex(sqlite3 * db, const char *zName, const char *zTable) Table *pTab = sqlite3HashFind(&db->pSchema->tblHash, zTable); - if (pTab == 0) { - return 0; - } + if (pTab == NULL) + return NULL; return sqlite3HashFind(&pTab->idxHash, zName); } @@ -545,7 +537,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr) assert(db->pSchema != NULL); pTable = sqlite3HashFind(&db->pSchema->tblHash, zName); - if (pTable) { + if (pTable != NULL) { if (!noErr) { sqlite3ErrorMsg(pParse, "table %s already exists", @@ -583,7 +575,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr) * now. */ if (!db->init.busy && (v = sqlite3GetVdbe(pParse)) != 0) - sql_set_multi_write(pParse, 1); + sql_set_multi_write(pParse, true); /* Normal (non-error) return. */ return; @@ -2303,7 +2295,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr) if (noErr) db->suppressErr--; - if (pTab == 0) + if (pTab == NULL) goto exit_drop_table; #ifndef SQLITE_OMIT_VIEW /* Ensure DROP TABLE is not used on a view, and DROP VIEW is not used @@ -2334,7 +2326,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr) * space_id from _space. */ - sql_set_multi_write(pParse, 1); + sql_set_multi_write(pParse, true); sql_clear_stat_spaces(pParse, "tbl", pTab->zName); sqlite3FkDropTable(pParse, pName, pTab); sqlite3CodeDropTable(pParse, pTab, isView); @@ -2841,14 +2833,15 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ goto exit_create_index; assert(pName->z != 0); if (!db->init.busy) { - if (sqlite3HashFind(&db->pSchema->tblHash, zName) != 0) { + if (sqlite3HashFind(&db->pSchema->tblHash, zName) != + NULL) { sqlite3ErrorMsg(pParse, "there is already a table named %s", zName); goto exit_create_index; } } - if (sqlite3HashFind(&pTab->idxHash, zName) != 0) { + if (sqlite3HashFind(&pTab->idxHash, zName) != NULL) { if (!ifNotExist) { sqlite3ErrorMsg(pParse, "index %s.%s already exists", @@ -3107,7 +3100,7 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ if (v == 0) goto exit_create_index; - sql_set_multi_write(pParse, 1); + sql_set_multi_write(pParse, true); sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_INDEX_ID, @@ -3853,11 +3846,10 @@ sqlite3Savepoint(Parse * pParse, int op, Token * pName) * execution multiple insertion/updates may occur. */ void -sql_set_multi_write(Parse *pParse, int setStatement) +sql_set_multi_write(struct Parse *parse_context, bool is_set) { - Parse *pToplevel = sqlite3ParseToplevel(pParse); - DbMaskSet(pToplevel->writeMask, 0); - pToplevel->isMultiWrite |= setStatement; + Parse *pToplevel = sqlite3ParseToplevel(parse_context); + pToplevel->isMultiWrite |= is_set; } /* @@ -3974,7 +3966,7 @@ reindexTable(Parse * pParse, Table * pTab, char const *zColl) for (pIndex = pTab->pIndex; pIndex; pIndex = pIndex->pNext) { if (zColl == 0 || collationMatch(zColl, pIndex)) { - sql_set_multi_write(pParse, 0); + sql_set_multi_write(pParse, false); sqlite3RefillIndex(pParse, pIndex, -1); } } @@ -4050,7 +4042,7 @@ sqlite3Reindex(Parse * pParse, Token * pName1, Token * pName2) if (z == 0) return; pTab = sqlite3HashFind(&db->pSchema->tblHash, z); - if (pTab) { + if (pTab != NULL) { reindexTable(pParse, pTab, 0); sqlite3DbFree(db, z); return; @@ -4066,8 +4058,8 @@ sqlite3Reindex(Parse * pParse, Token * pName1, Token * pName2) } pIndex = sqlite3HashFind(&pTab->idxHash, z); - if (pIndex) { - sql_set_multi_write(pParse, 0); + if (pIndex != NULL) { + sql_set_multi_write(pParse, false); sqlite3RefillIndex(pParse, pIndex, -1); return; } diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index a07ef1980..9f4ce1026 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -59,12 +59,10 @@ sqlite3SrcListLookup(Parse * pParse, SrcList * pSrc) pTab = sqlite3LocateTable(pParse, 0, pItem->zName); sqlite3DeleteTable(pParse->db, pItem->pTab); pItem->pTab = pTab; - if (pTab) { + if (pTab != NULL) pTab->nTabRef++; - } - if (sqlite3IndexedByLookup(pParse, pItem)) { - pTab = 0; - } + if (sqlite3IndexedByLookup(pParse, pItem)) + pTab = NULL; return pTab; } @@ -324,7 +322,7 @@ sqlite3DeleteFrom(Parse * pParse, /* The parser context */ } if (pParse->nested == 0) sqlite3VdbeCountChanges(v); - sql_set_multi_write(pParse, 1); + sql_set_multi_write(pParse, true); /* If we are trying to delete from a view, realize that view into * an ephemeral table. diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 2719bbaf4..704e48d9c 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -1461,7 +1461,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ default: { Trigger *pTrigger = 0; assert(onError == ON_CONFLICT_ACTION_REPLACE); - sql_set_multi_write(pParse, 1); + sql_set_multi_write(pParse, true); if (user_session-> sql_flags & SQLITE_RecTriggers) { pTrigger = @@ -1769,9 +1769,8 @@ xferOptimization(Parse * pParse, /* Parser context */ int regData, regTupleid; /* Registers holding data and tupleid */ struct session *user_session = current_session(); - if (pSelect == 0) { + if (pSelect == 0) return 0; /* Must be of the form INSERT INTO ... SELECT ... */ - } if (pParse->pWith || pSelect->pWith) { /* Do not attempt to process this query if there are an WITH clauses * attached to it. Proceeding may generate a false "no such table: xxx" @@ -1833,7 +1832,7 @@ xferOptimization(Parse * pParse, /* Parser context */ */ pItem = pSelect->pSrc->a; pSrc = sqlite3LocateTable(pParse, 0, pItem->zName); - if (pSrc == 0) { + if (pSrc == NULL) { return 0; /* FROM clause does not contain a real table */ } if (pSrc == pDest) { diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 04b4020dd..c19a811ff 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -480,7 +480,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ int i; pTab = sqlite3HashFind(&db->pSchema->tblHash, zRight); - if (pTab) { + if (pTab != NULL) { pParse->nMem = 5; for (pIdx = pTab->pIndex, i = 0; pIdx; pIdx = pIdx->pNext, i++) { @@ -540,7 +540,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ Table *pTab; pTab = sqlite3HashFind(&db->pSchema->tblHash, zRight); - if (pTab) { + if (pTab != NULL) { pFK = pTab->pFKey; if (pFK) { int i = 0; @@ -617,7 +617,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ pParent = sqlite3HashFind(&db->pSchema->tblHash, pFK->zTo); - if (pParent == 0) + if (pParent == NULL) continue; pIdx = 0; x = sqlite3FkLocateIndex(pParse, diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 6ff8dc25e..679aa2246 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -4746,7 +4746,7 @@ selectExpander(Walker * pWalker, Select * p) assert(pFrom->pTab == 0); pFrom->pTab = pTab = sqlite3LocateTable(pParse, 0, pFrom->zName); - if (pTab == 0) + if (pTab == NULL) return WRC_Abort; if (pTab->nTabRef >= 0xffff) { sqlite3ErrorMsg(pParse, diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 3234f992b..6d78f791d 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -2887,25 +2887,6 @@ struct TriggerPrg { u32 aColmask[2]; /* Masks of old.*, new.* columns accessed */ }; -/* - * The yDbMask datatype for the bitmask of all attached databases. - */ -#if SQLITE_MAX_ATTACHED>30 -typedef unsigned char yDbMask[(SQLITE_MAX_ATTACHED + 9) / 8]; -#define DbMaskTest(M,I) (((M)[(I)/8]&(1<<((I)&7)))!=0) -#define DbMaskZero(M) memset((M),0,sizeof(M)) -#define DbMaskSet(M,I) (M)[(I)/8]|=(1<<((I)&7)) -#define DbMaskAllZero(M) sqlite3DbMaskAllZero(M) -#define DbMaskNonZero(M) (sqlite3DbMaskAllZero(M)==0) -#else -typedef unsigned int yDbMask; -#define DbMaskTest(M,I) (((M)&(((yDbMask)1)<<(I)))!=0) -#define DbMaskZero(M) (M)=0 -#define DbMaskSet(M,I) (M)|=(((yDbMask)1)<<(I)) -#define DbMaskAllZero(M) (M)==0 -#define DbMaskNonZero(M) (M)!=0 -#endif - /* * An SQL parser context. A copy of this structure is passed through * the parser and down into all the parser action routine in order to @@ -2946,7 +2927,6 @@ struct Parse { int *aLabel; /* Space to hold the labels */ ExprList *pConstExpr; /* Constant expressions */ Token constraintName; /* Name of the constraint currently being parsed */ - yDbMask writeMask; /* Start a write transaction on these databases */ int regRoot; /* Register holding root page number for new objects */ int nMaxArg; /* Max args passed to user function by sub-program */ #ifdef SELECTTRACE_ENABLED @@ -3585,9 +3565,6 @@ int sqlite3ViewGetColumnNames(Parse *, Table *); #define sqlite3ViewGetColumnNames(A,B) 0 #endif -#if SQLITE_MAX_ATTACHED>30 -int sqlite3DbMaskAllZero(yDbMask); -#endif void sqlite3DropTable(Parse *, SrcList *, int, int); void sqlite3DeleteTable(sqlite3 *, Table *); void sqlite3Insert(Parse *, SrcList *, Select *, IdList *, int); diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index decbc8c21..a102c32b6 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -293,7 +293,7 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */ iFirstCol = pParse->nMem + 1; pParse->nMem += 2; - sql_set_multi_write(pParse, 0); + sql_set_multi_write(pParse, false); sqlite3VdbeAddOp4(v, OP_String8, 0, iFirstCol, 0, zName, P4_STATIC); diff --git a/src/box/sql/update.c b/src/box/sql/update.c index ad6aad5e6..778519bd6 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -288,7 +288,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ goto update_cleanup; if (pParse->nested == 0) sqlite3VdbeCountChanges(v); - sql_set_multi_write(pParse, 1); + sql_set_multi_write(pParse, true); /* Allocate required registers. */ regOldPk = regNewPk = ++pParse->nMem;