[tarantool-patches] Re: [PATCH 2/2] sql: rework 'DROP INDEX' and 'DROP TABLE' handling

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

See 6 comments below.

03.04.2018 19:14, Nikita Pettik пишет:
> As a part of SQL data dictionary integration, 'DROP INDEX' and
> 'DROP TABLE' statements proccessing has been refactored in order
> to avoid using SQL specific internal structures.
> However, triggers still aren't transfered to server, so to drop them
> it is required to lookup SQL table and emit apporpriate opcodes.
> Also, added comments and fixed codestyle for functions processing
> 'DROP' routine.
>
> Part of #3222.
> ---
>   src/box/sql/build.c     | 241 ++++++++++++++++++++++++++----------------------
>   src/box/sql/parse.c     |   6 +-
>   src/box/sql/parse.y     |   6 +-
>   src/box/sql/sqliteInt.h |   6 +-
>   4 files changed, 140 insertions(+), 119 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 61194e06b..219cc974b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -46,6 +46,7 @@
>   #include "sqliteInt.h"
>   #include "vdbeInt.h"
>   #include "tarantoolInt.h"
> +#include "box/box.h"
>   #include "box/sequence.h"
>   #include "box/session.h"
>   #include "box/identifier.h"
> @@ -2152,48 +2153,50 @@ sql_clear_stat_spaces(Parse * pParse, const char *zType, const char *zName)
>   			   zType, zName);
>   }
>   
> -/*
> +/**
>    * Generate code to drop a table.
> + * This routine includes dropping triggers, sequences,
> + * all indexes and entry from _space space.
> + *
> + * @param parse_context Current parsing context.
> + * @param space Space to be dropped.
> + * @param is_view True, if space is
>    */
>   static void
> -sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView)
> +sql_code_drop_table(struct Parse *parse_context, struct space *space,
> +		    bool is_view)
>   {
> -	Vdbe *v;
> -	sqlite3 *db = pParse->db;
> -	Trigger *pTrigger;
> -
> -	v = sqlite3GetVdbe(pParse);
>
> +	struct sqlite3 *db = parse_context->db;
> +	struct Vdbe *v = sqlite3GetVdbe(parse_context);
>   	assert(v != 0);
1. Lets in all new code use explicit ==/!= NULL.
>   	/*
>   	 * Drop all triggers associated with the table being
>   	 * dropped. Code is generated to remove entries from
>   	 * _trigger. OP_DropTrigger will remove it from internal
>   	 * SQL structures.
> -	 */
> -	pTrigger = pTab->pTrigger;
> -	/* Do not account triggers deletion - they will be accounted
> +	 *
> +	 * Do not account triggers deletion - they will be accounted
2. Out of 66.
> @@ -2214,7 +2216,7 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView)
>   	 * Drop all _space and _index entries that refer to the
>   	 * table.
>   	 */
> -	if (!isView) {
> +	if (!is_view) {
3. I looked at the indexes deletion more carefully and noticed, that you 
can avoid any allocations
here. Why can not you just walk through space->index array and generate 
OP_SDelete opcodes for
each index, starting from 1 (to skip primary)? AFAIK the entire index 
array is not changed until you
reach VdbeExec, so you can do not save index identifiers on region to 
generate opcodes.
>   		uint32_t index_count = space->index_count;
>   		if (index_count > 1) {
>   			/*
> @@ -2257,71 +2259,78 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView)
>   	sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_ID, idx_rec_reg);
>   	sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
>   	VdbeComment((v, "Delete entry from _space"));
> -	/* Remove the table entry from SQLite's internal schema and modify
> -	 * the schema cookie.
> +	/*
> +	 * Remove the table entry from SQLite's internal schema
> +	 * and modify the schema cookie.
>   	 */
4. But you have deleted cookie updates in the previous commit. Source 
code of OP_DropTable and all
called there functions does not contain cookie changes.
> +	sqlite3VdbeAddOp4(v, OP_DropTable, 0, 0, 0, space->def->name, 0);
>   
> -	sqlite3VdbeAddOp4(v, OP_DropTable, 0, 0, 0, pTab->zName, 0);
> -
> -	/* FIXME: DDL is impossible inside a transaction so far.
> +	/*
>   	 * Replace to _space/_index will fail if active
>   	 * transaction. So, don't pretend, that we are able to
>   	 * anything back. To be fixed when transactions
>   	 * DDL are enabled.
>   	 */
> -	/* sqlite3ChangeCookie(pParse); */
>   	sqliteViewResetAll(db);
>   }
>   
> -/*
> +/**
>    * This routine is called to do the work of a DROP TABLE statement.
> - * pName is the name of the table to be dropped.
> + *
> + * @param parse_context Current parsing context.
> + * @param table_name_list List containing table name.
> + * @param is_view True, if statement is really 'DROP VIEW'.
> + * @param if_exists True, if statement contains 'IF EXISTS' clause.
>    */
>   void
> -sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr)
> +sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
> +	       bool is_view, bool if_exists)
>   {
> -	Table *pTab;
> -	Vdbe *v = sqlite3GetVdbe(pParse);
> -	sqlite3 *db = pParse->db;
> -
> +	struct Vdbe *v = sqlite3GetVdbe(parse_context);
> +	struct sqlite3 *db = parse_context->db;
>   	if (v == NULL || db->mallocFailed) {
>   		goto exit_drop_table;
>   	}
> -	/* Activate changes counting here to account
> +	/*
> +	 * Activate changes counting here to account
>   	 * DROP TABLE IF NOT EXISTS, if the table really does not
>   	 * exist.
>   	 */
> -	if (!pParse->nested)
> +	if (!parse_context->nested)
5. Please, use explicit != 0. Why you use !int, the variable seems to be 
boolean. It slightly confuses.
>   		sqlite3VdbeCountChanges(v);
> -	assert(pParse->nErr == 0);
> -	assert(pName->nSrc == 1);
> -	assert(db->pSchema != NULL);
> -	if (noErr)
> -		db->suppressErr++;
> -	assert(isView == 0 || isView == LOCATE_VIEW);
> -	pTab = sqlite3LocateTable(pParse, isView, pName->a[0].zName);
> -	if (noErr)
> -		db->suppressErr--;
> -
> -	if (pTab == 0)
> +	assert(parse_context->nErr == 0);
> +	assert(table_name_list->nSrc == 1);
> +	assert(is_view == 0 || is_view == LOCATE_VIEW);
> +	const char *space_name = table_name_list->a[0].zName;
> +	uint32_t space_id = box_space_id_by_name(space_name,
> +						 strlen(space_name));
> +	if (space_id == BOX_ID_NIL) {
> +		if (!is_view && !if_exists)
> +			sqlite3ErrorMsg(parse_context, "no such table: %s",
> +					space_name);
> +		if (is_view && !if_exists)
> +			sqlite3ErrorMsg(parse_context, "no such view: %s",
> +					space_name);
>   		goto exit_drop_table;
> -#ifndef SQLITE_OMIT_VIEW
> -	/* Ensure DROP TABLE is not used on a view, and DROP VIEW is not used
> -	 * on a table.
> +	}
> +	struct space *space = space_by_id(space_id);
> +	assert(space != NULL);
> +	/*
> +	 * Ensure DROP TABLE is not used on a view,
> +	 * and DROP VIEW is not used on a table.
>   	 */
> -	if (isView && !space_is_view(pTab)) {
> -		sqlite3ErrorMsg(pParse, "use DROP TABLE to delete table %s",
> -				pTab->zName);
> +	if (is_view && !space->def->opts.is_view) {
> +		sqlite3ErrorMsg(parse_context, "use DROP TABLE to delete table %s",
> +				space_name);
>   		goto exit_drop_table;
>   	}
> -	if (!isView && space_is_view(pTab)) {
> -		sqlite3ErrorMsg(pParse, "use DROP VIEW to delete view %s",
> -				pTab->zName);
> +	if (!is_view && space->def->opts.is_view) {
> +		sqlite3ErrorMsg(parse_context, "use DROP VIEW to delete view %s",
> +				space_name);
>   		goto exit_drop_table;
>   	}
> -#endif
> -
> -	/* Generate code to remove the table from Tarantool and internal SQL
> +	/*
> +	 * Generate code to remove the table from Tarantool and internal SQL
6. Out of 66.