[tarantool-patches] [PATCH] sql: make DROP TABLE delete entry from _sequence_data

[tarantool-patches] [PATCH] sql: make DROP TABLE delete entry from _sequence_data

[Nikita Pettik]

Before this patch, _sequence_data system space wasn't taken into
consideration when space was dropped. However, entries in this space
may appear after recovery. For example:

CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT);
INSERT INTO t VALUES (NULL);
box.snapshot()
os.exit()
...
box.cfg{}
DROP TABLE t;

Last DROP statement didn't generate VDBE code to delete entry from
_sequence_data, but it had to do so. This patch simply modifies code
generation to remove entry from _sequence_data space before space is
dropped.

Closes #3712
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-3712-sql-sequence-recovery
Issue: https://github.com/tarantool/tarantool/issues/3712

 src/box/sql/build.c          | 19 +++++++++++++------
 test/sql/drop-table.result   | 17 +++++++++++++++++
 test/sql/drop-table.test.lua |  9 +++++++++
 3 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a806fb4b3..2704781bd 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1767,9 +1767,10 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 		trigger = trigger->next;
 	}
 	/*
-	 * Remove any entries of the _sequence and _space_sequence
-	 * space associated with the table being dropped. This is
-	 * done before the table is dropped from internal schema.
+	 * Remove any entries from the _sequence_data, _sequence
+	 * and _space_sequence spaces associated with the table
+	 * being dropped. This is done before the table is dropped
+	 * from internal schema.
 	 */
 	int idx_rec_reg = ++parse_context->nMem;
 	int space_id_reg = ++parse_context->nMem;
@@ -1777,6 +1778,15 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlite3VdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
+		/* Delete entry from _sequence_data. */
+		int sequence_id_reg = ++parse_context->nMem;
+		sqlite3VdbeAddOp2(v, OP_Integer, space->sequence->def->id,
+				  sequence_id_reg);
+		sqlite3VdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				  idx_rec_reg);
+		sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
+				  idx_rec_reg);
+		VdbeComment((v, "Delete entry from _sequence_data"));
 		/* Delete entry from _space_sequence. */
 		sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1,
 				  idx_rec_reg);
@@ -1784,9 +1794,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 				  idx_rec_reg);
 		VdbeComment((v, "Delete entry from _space_sequence"));
 		/* Delete entry by id from _sequence. */
-		int sequence_id_reg = ++parse_context->nMem;
-		sqlite3VdbeAddOp2(v, OP_Integer, space->sequence->def->id,
-				  sequence_id_reg);
 		sqlite3VdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
 				  idx_rec_reg);
 		sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f249668..bf02e3def 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -29,6 +29,23 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
 ---
 - error: 'no such table: ZZZOOBAR'
 ...
+-- gh-3712: if space features sequence, data from _sequence_data
+-- must be deleted before space is dropped.
+--
+box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
+---
+...
+box.sql.execute("INSERT INTO t1 VALUES (NULL);")
+---
+...
+box.snapshot()
+---
+- ok
+...
+test_run:cmd('restart server default')
+box.sql.execute("DROP TABLE t1;")
+---
+...
 -- Cleanup
 -- DROP TABLE should do the job
 -- Debug
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074df..2c0e3dc81 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -20,6 +20,15 @@ box.sql.execute("DROP TABLE zzzoobar")
 -- Table does not exist anymore. Should error here.
 box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
 
+-- gh-3712: if space features sequence, data from _sequence_data
+-- must be deleted before space is dropped.
+--
+box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
+box.sql.execute("INSERT INTO t1 VALUES (NULL);")
+box.snapshot()
+test_run:cmd('restart server default')
+box.sql.execute("DROP TABLE t1;")
+
 -- Cleanup
 -- DROP TABLE should do the job
 
-- 
2.15.1

[tarantool-patches] Re: [PATCH] sql: make DROP TABLE delete entry from _sequence_data

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
> index 08f249668..bf02e3def 100644
> --- a/test/sql/drop-table.result
> +++ b/test/sql/drop-table.result
> @@ -29,6 +29,23 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
>   ---
>   - error: 'no such table: ZZZOOBAR'
>   ...
> +-- gh-3712: if space features sequence, data from _sequence_data
> +-- must be deleted before space is dropped.
> +--
> +box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
> +---
> +...
> +box.sql.execute("INSERT INTO t1 VALUES (NULL);")
> +---
> +...
> +box.snapshot()
> +---
> +- ok
> +...
> +test_run:cmd('restart server default')

Unfortunately, we have no luxury of time to restart a
server on such a simple test. Lets just check that
_sequence_data space is empty.

> +box.sql.execute("DROP TABLE t1;")
> +---
> +...
>   -- Cleanup
>   -- DROP TABLE should do the job
>   -- Debug

[tarantool-patches] Re: [PATCH] sql: make DROP TABLE delete entry from _sequence_data

[n.pettik]

>> diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
>> index 08f249668..bf02e3def 100644
>> --- a/test/sql/drop-table.result
>> +++ b/test/sql/drop-table.result
>> @@ -29,6 +29,23 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
>>  ---
>>  - error: 'no such table: ZZZOOBAR'
>>  ...
>> +-- gh-3712: if space features sequence, data from _sequence_data
>> +-- must be deleted before space is dropped.
>> +--
>> +box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
>> +---
>> +...
>> +box.sql.execute("INSERT INTO t1 VALUES (NULL);")
>> +---
>> +...
>> +box.snapshot()
>> +---
>> +- ok
>> +...
>> +test_run:cmd('restart server default')
> 
> Unfortunately, we have no luxury of time to restart a
> server on such a simple test. Lets just check that
> _sequence_data space is empty.

But restart is a part of this test. Without recovery entry doesn’t appear
in _sequence_data. It is easy to check this out (on 2.0):

tarantool> box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
---
...

tarantool> box.sql.execute("INSERT INTO t1 VALUES (NULL)")
---
...

tarantool> box.snapshot()
2018-10-15 22:03:09.045 [29254] snapshot/101/main I> saving snapshot `./00000000000000000006.snap.inprogress'
2018-10-15 22:03:09.045 [29254] snapshot/101/main I> done
---
- ok
...

tarantool> box.space._sequence_data:select()
---
- []
…

Then lets restart server and see at that space again:

tarantool> os.exit()
n:tarantool n.pettik$ ./src/tarantool 
Tarantool 2.0.5-75-gdd8e14ffb
type 'help' for interactive help
tarantool> box.cfg{}
2018-10-15 22:03:58.932 [29259] main/101/interactive C> Tarantool 2.0.5-75-gdd8e14ffb
2018-10-15 22:03:58.932 [29259] main/101/interactive C> log level 5
2018-10-15 22:03:58.933 [29259] main/101/interactive I> mapping 268435456 bytes for memtx tuple arena...
2018-10-15 22:03:58.933 [29259] main/101/interactive I> mapping 134217728 bytes for vinyl tuple arena...
2018-10-15 22:03:58.943 [29259] main/101/interactive I> recovery start
2018-10-15 22:03:58.943 [29259] main/101/interactive I> recovering from `./00000000000000000006.snap'
2018-10-15 22:03:59.009 [29259] main/101/interactive I> recover from `./00000000000000000006.xlog'
2018-10-15 22:03:59.009 [29259] main/101/interactive I> done `./00000000000000000006.xlog'
2018-10-15 22:03:59.011 [29259] main/101/interactive I> ready to accept requests
2018-10-15 22:03:59.011 [29259] main/107/checkpoint_daemon I> started
2018-10-15 22:03:59.012 [29259] main/107/checkpoint_daemon I> scheduled the next snapshot at Tue Oct 16 00:02:23 2018
---
...

tarantool> box.space._sequence_data:select()
---
- - [1, 1]
...

So, to make this test be fair - restart is required.

[tarantool-patches] Re: [PATCH] sql: make DROP TABLE delete entry from _sequence_data

[Vladislav Shpilevoy]

Ok, understood. LGTM then.

On 15/10/2018 22:04, n.pettik wrote:
> 
>>> diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
>>> index 08f249668..bf02e3def 100644
>>> --- a/test/sql/drop-table.result
>>> +++ b/test/sql/drop-table.result
>>> @@ -29,6 +29,23 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
>>>   ---
>>>   - error: 'no such table: ZZZOOBAR'
>>>   ...
>>> +-- gh-3712: if space features sequence, data from _sequence_data
>>> +-- must be deleted before space is dropped.
>>> +--
>>> +box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
>>> +---
>>> +...
>>> +box.sql.execute("INSERT INTO t1 VALUES (NULL);")
>>> +---
>>> +...
>>> +box.snapshot()
>>> +---
>>> +- ok
>>> +...
>>> +test_run:cmd('restart server default')
>>
>> Unfortunately, we have no luxury of time to restart a
>> server on such a simple test. Lets just check that
>> _sequence_data space is empty.
> 
> But restart is a part of this test. Without recovery entry doesn’t appear
> in _sequence_data. It is easy to check this out (on 2.0):
> 
> tarantool> box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
> ---
> ...
> 
> tarantool> box.sql.execute("INSERT INTO t1 VALUES (NULL)")
> ---
> ...
> 
> tarantool> box.snapshot()
> 2018-10-15 22:03:09.045 [29254] snapshot/101/main I> saving snapshot `./00000000000000000006.snap.inprogress'
> 2018-10-15 22:03:09.045 [29254] snapshot/101/main I> done
> ---
> - ok
> ...
> 
> tarantool> box.space._sequence_data:select()
> ---
> - []
> …
> 
> Then lets restart server and see at that space again:
> 
> tarantool> os.exit()
> n:tarantool n.pettik$ ./src/tarantool
> Tarantool 2.0.5-75-gdd8e14ffb
> type 'help' for interactive help
> tarantool> box.cfg{}
> 2018-10-15 22:03:58.932 [29259] main/101/interactive C> Tarantool 2.0.5-75-gdd8e14ffb
> 2018-10-15 22:03:58.932 [29259] main/101/interactive C> log level 5
> 2018-10-15 22:03:58.933 [29259] main/101/interactive I> mapping 268435456 bytes for memtx tuple arena...
> 2018-10-15 22:03:58.933 [29259] main/101/interactive I> mapping 134217728 bytes for vinyl tuple arena...
> 2018-10-15 22:03:58.943 [29259] main/101/interactive I> recovery start
> 2018-10-15 22:03:58.943 [29259] main/101/interactive I> recovering from `./00000000000000000006.snap'
> 2018-10-15 22:03:59.009 [29259] main/101/interactive I> recover from `./00000000000000000006.xlog'
> 2018-10-15 22:03:59.009 [29259] main/101/interactive I> done `./00000000000000000006.xlog'
> 2018-10-15 22:03:59.011 [29259] main/101/interactive I> ready to accept requests
> 2018-10-15 22:03:59.011 [29259] main/107/checkpoint_daemon I> started
> 2018-10-15 22:03:59.012 [29259] main/107/checkpoint_daemon I> scheduled the next snapshot at Tue Oct 16 00:02:23 2018
> ---
> ...
> 
> tarantool> box.space._sequence_data:select()
> ---
> - - [1, 1]
> ...
> 
> So, to make this test be fair - restart is required.
> 

[tarantool-patches] Re: [PATCH] sql: make DROP TABLE delete entry from _sequence_data

[Kirill Yukhin]

Hello,
On 15 Oct 21:54, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
> > index 08f249668..bf02e3def 100644
> > --- a/test/sql/drop-table.result
> > +++ b/test/sql/drop-table.result
> > @@ -29,6 +29,23 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
> >   ---
> >   - error: 'no such table: ZZZOOBAR'
> >   ...
> > +-- gh-3712: if space features sequence, data from _sequence_data
> > +-- must be deleted before space is dropped.
> > +--
> > +box.sql.execute("CREATE TABLE t1 (id INT PRIMARY KEY AUTOINCREMENT);")
> > +---
> > +...
> > +box.sql.execute("INSERT INTO t1 VALUES (NULL);")
> > +---
> > +...
> > +box.snapshot()
> > +---
> > +- ok
> > +...
> > +test_run:cmd('restart server default')
> 
> Unfortunately, we have no luxury of time to restart a
> server on such a simple test. Lets just check that
> _sequence_data space is empty.
> 
> > +box.sql.execute("DROP TABLE t1;")
> > +---
> > +...
> >   -- Cleanup
> >   -- DROP TABLE should do the job
> >   -- Debug
> 

I've missed start of the thread.
Anyway, I've pushed the patch into 2.1 branch.

--
Regards, Kirill Yukhin