[tarantool-patches] Re: [PATCH 1/2] sql: remove obsolete SQLite routine

[Vladislav Shpilevoy]

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.
>   
>   	/* 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.
> @@ -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) ?
> @@ -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. Same about checking 
sqlite3HashFind results. And if it is
possible with not huge diff, can you please rename sqlite3HashFind to 
sql_hash_find ?

5. sqlite3MultiWrite in commit message is listed among deleted 
functions, but its declaration still exists.

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.

7. How about remove DbMaskTest, DbMaskZero and other dbmask shit?