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

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 11718 bytes --]


> On 3 Apr 2018, at 21:27, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> 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.

Fixed:
-       assert(v != 0);
+       assert(v != 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.

Fixed:
-        * Do not account triggers deletion - they will be accounted
-        * in DELETE from _space below.
+        * Do not account triggers deletion - they will be
+        * accounted in DELETE from _space below.

>> @@ -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.

IDK why I didn’t do it in this way (probably due to my ‘paranoia’).
Reworked:

-                       uint32_t *iids =
-                               (uint32_t *) region_alloc(&fiber()->gc,
-                                                         sizeof(uint32_t) *
-                                                         (index_count - 1));
-                       /* Save index ids to be deleted except for PK. */
                        for (uint32_t i = 1; i < index_count; ++i) {
-                               iids[i - 1] = space->index[i]->def->iid;
-                       }
-                       for (uint32_t i = 0; i < index_count - 1; ++i) {
-                               sqlite3VdbeAddOp2(v, OP_Integer, iids[i],
+                               sqlite3VdbeAddOp2(v, OP_Integer,
+                                                 space->index[i]->def->iid,
                                                  space_id_reg + 1);
                                sqlite3VdbeAddOp3(v, OP_MakeRecord,
                                                  space_id_reg, 2, idx_rec_reg);
@@ -2233,7 +2226,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
                                                  idx_rec_reg);
                                VdbeComment((v,
                                             "Remove secondary index iid = %u",
-                                            iids[i]));
+                                            space->index[i]->def->iid));

>>  		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.

Removed obsolete comment:

-       /*
-        * Remove the table entry from SQLite's internal schema
-        * and modify the schema cookie.
-        */
+       /* Remove the table entry from SQLite's internal schema. */

>> +	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.

This this not != check, but vice versa == 0 check.
! In this case would better fit, wouldn’t it?
(Since any number except for 0 would be converted to false)

>>  		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.

Fixed whole comment (to fit into 66 chars):

-        * Generate code to remove the table from Tarantool and internal SQL
-        * tables. Basically, it consists from 3 stages:
-        * 1. Delete statistics from _stat1 and _stat4 tables (if any exist)
-        * 2. In case of presence of FK constraints, i.e. current table is child
-        *    or parent, then start new transaction and erase from table
-        *    all data row by row. On each deletion check whether any FK
-        *    violations have occurred. If ones take place, then rollback
-        *    transaction and halt VDBE. Otherwise, commit transaction.
-        * 3. Drop table by truncating (if step 2 was skipped), removing
-        *    indexes from _index table and eventually tuple with corresponding
-        *    space_id from _space.
+        * Generate code to remove the table from Tarantool
+        * and internal SQL tables. Basically, it consists
+        * from 3 stages:
+        * 1. Delete statistics from _stat1 and _stat4 tables.
+        * 2. In case of presence of FK constraints, i.e. current
+        *    table is child or parent, then start new transaction
+        *    and erase from table all data row by row. On each
+        *    deletion check whether any FK violations have
+        *    occurred. If ones take place, then rollback
+        *    transaction and halt VDBE.
+        * 3. Drop table by truncating (if step 2 was skipped),
+        *    removing indexes from _index space and eventually
+        *    tuple with corresponding space_id from _space.
         */



[-- Attachment #2: Type: text/html, Size: 39685 bytes --]