[Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Chris Sosnin]

Dropping table with sql removes everything
associated with it but grants, which is
inconsistent. Generating code for it fixes this bug.

Closes #4546
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
issue: https://github.com/tarantool/tarantool/issues/4546

 src/box/sql/build.c                       | 43 +++++++++++++++++++++++
 src/box/user.cc                           |  7 ++++
 src/box/user.h                            |  4 +++
 test/sql/gh-4546-sql-drop-grants.result   | 34 ++++++++++++++++++
 test/sql/gh-4546-sql-drop-grants.test.lua | 15 ++++++++
 5 files changed, 103 insertions(+)
 create mode 100644 test/sql/gh-4546-sql-drop-grants.result
 create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..d700ec5e2 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -56,6 +56,7 @@
 #include "box/schema.h"
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
+#include "box/user.h"

 void
 sql_finish_coding(struct Parse *parse_context)
@@ -1523,6 +1524,43 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
 	sqlReleaseTempRange(parser, key_reg, 3);
 }

+/**
+ * Generate VDBE program to remove table grants.
+ *
+ * @param parser Parsing context.
+ * @param space Space whose grants will be deleted.
+ */
+static void
+vdbe_emit_drop_grants(struct Parse *parser, struct space *space)
+{
+	struct Vdbe *v = sqlGetVdbe(parser);
+	if (v == NULL)
+		return;
+	/*
+	 * Get uid of users through space->access
+	 * and generate code to delete corresponding
+	 * entries from _priv
+	 */
+	int key_reg = sqlGetTempRange(parser, 4);
+	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
+		if (space->access[token].granted) {
+			struct user *user = user_find_by_token(token);
+			sqlVdbeAddOp2(v, OP_Integer, user->def->uid,
+				 	  key_reg);
+			sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
+				 	  "space", P4_STATIC);
+			sqlVdbeAddOp2(v, OP_Integer, space->def->id,
+					  key_reg + 2);
+			sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3,
+				 	  key_reg + 3);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID,
+					  key_reg + 3);
+		}
+	}
+	VdbeComment((v, "Remove table grants"));
+	sqlReleaseTempRange(parser, key_reg, 3);
+}
+
 /**
  * Generate code to drop a table.
  * This routine includes dropping triggers, sequences,
@@ -1538,6 +1576,11 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 {
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
+	/*
+	 * Remove all grants associated with
+	 * with the table being dropped.
+	 */
+	vdbe_emit_drop_grants(parse_context, space);
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
diff --git a/src/box/user.cc b/src/box/user.cc
index a012cb196..fe0555886 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -517,6 +517,13 @@ user_find(uint32_t uid)
 	return user;
 }

+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token)
+{
+    return &users[auth_token];
+}
+
 /** Find user by name. */
 struct user *
 user_find_by_name(const char *name, uint32_t len)
diff --git a/src/box/user.h b/src/box/user.h
index 545401d5c..ccfc59346 100644
--- a/src/box/user.h
+++ b/src/box/user.h
@@ -112,6 +112,10 @@ user_find_by_name(const char *name, uint32_t len);
 struct user *
 user_find(uint32_t uid);

+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token);
+
 /** Create a cache of user's privileges in @a cr. */
 void
 credentials_create(struct credentials *cr, struct user *user);
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
new file mode 100644
index 000000000..915a265bd
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -0,0 +1,34 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+- row_count: 0
+...
+-- If we drop the table with sql, all associated
+-- grants must be deleted so we don't recieve an error
+box.cfg{}
+---
+...
+box.schema.user.create('test_user1')
+---
+...
+box.schema.user.create('test_user2')
+---
+...
+test_space = box.schema.create_space('T')
+---
+...
+box.schema.user.grant('test_user1', 'read', 'space', 'T')
+---
+...
+box.schema.user.grant('test_user2', 'write', 'space', 'T')
+---
+...
+box.execute([[DROP TABLE T;]])
+---
+- row_count: 1
+...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
new file mode 100644
index 000000000..6aeca4ce0
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -0,0 +1,15 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+
+-- If we drop the table with sql, all associated
+-- grants must be deleted so we don't recieve an error
+
+box.cfg{}
+
+box.schema.user.create('test_user1')
+box.schema.user.create('test_user2')
+test_space = box.schema.create_space('T')
+box.schema.user.grant('test_user1', 'read', 'space', 'T')
+box.schema.user.grant('test_user2', 'write', 'space', 'T')
+box.execute([[DROP TABLE T;]])
--
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Chris Sosnin]

There was a typo, I fixed it:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
+	sqlReleaseTempRange(parser, key_reg, 4);

--
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

I realized, that grants problem also affects
sequence. Take a look at

    if sequence_tuple ~= nil and sequence_tuple.is_generated == true then
        -- Delete automatically generated sequence.
        box.schema.sequence.drop(sequence_tuple.sequence_id)
    end

In schema.lua in box.schema.space.drop.

Sequences also can have grants. And sequence.drop removes them.
I propose you to drop sequence grants in a second commit. The
problematic place is

    if (space->sequence != NULL) {

in sql_code_drop_table(). It should drop sequence grants too.

See 5 comments below.

On 06/12/2019 13:53, Chris Sosnin wrote:
> Dropping table with sql removes everything
> associated with it but grants, which is
> inconsistent. Generating code for it fixes this bug.
> 
> Closes #4546
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
> issue: https://github.com/tarantool/tarantool/issues/4546
> 
>  src/box/sql/build.c                       | 43 +++++++++++++++++++++++
>  src/box/user.cc                           |  7 ++++
>  src/box/user.h                            |  4 +++
>  test/sql/gh-4546-sql-drop-grants.result   | 34 ++++++++++++++++++
>  test/sql/gh-4546-sql-drop-grants.test.lua | 15 ++++++++
>  5 files changed, 103 insertions(+)
>  create mode 100644 test/sql/gh-4546-sql-drop-grants.result
>  create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce63..d700ec5e2 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1523,6 +1524,43 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>  	sqlReleaseTempRange(parser, key_reg, 3);
>  }
> 
> +/**
> + * Generate VDBE program to remove table grants.
> + *
> + * @param parser Parsing context.
> + * @param space Space whose grants will be deleted.
> + */
> +static void
> +vdbe_emit_drop_grants(struct Parse *parser, struct space *space)

1. Seems like the space is not changed here, so can be made
'const struct space *'.

> +{
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	if (v == NULL)
> +		return;

2. Lets assert that v is never NULL. By the time this
function is called, Vdbe is always created.

> +	/*
> +	 * Get uid of users through space->access
> +	 * and generate code to delete corresponding
> +	 * entries from _priv
> +	 */
> +	int key_reg = sqlGetTempRange(parser, 4);
> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
> +		if (space->access[token].granted) {
> +			struct user *user = user_find_by_token(token);
> +			sqlVdbeAddOp2(v, OP_Integer, user->def->uid,
> +				 	  key_reg);
> +			sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> +				 	  "space", P4_STATIC);
> +			sqlVdbeAddOp2(v, OP_Integer, space->def->id,
> +					  key_reg + 2);
> +			sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3,
> +				 	  key_reg + 3);
> +			sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID,
> +					  key_reg + 3);

3. The indentation is broken, some calls fit one line,
and there are dangling whitespaces right below 'Add' on
each line.

Consider this refactoring:

=========================================================================
@@ -1543,19 +1543,15 @@ vdbe_emit_drop_grants(struct Parse *parser, struct space *space)
 	 */
 	int key_reg = sqlGetTempRange(parser, 4);
 	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
-		if (space->access[token].granted) {
-			struct user *user = user_find_by_token(token);
-			sqlVdbeAddOp2(v, OP_Integer, user->def->uid,
-				 	  key_reg);
-			sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
-				 	  "space", P4_STATIC);
-			sqlVdbeAddOp2(v, OP_Integer, space->def->id,
-					  key_reg + 2);
-			sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3,
-				 	  key_reg + 3);
-			sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID,
-					  key_reg + 3);
-		}
+		if (! space->access[token].granted)
+			continue;
+		struct user *user = user_find_by_token(token);
+		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
+		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0, "space",
+			      P4_STATIC);
+		sqlVdbeAddOp2(v, OP_Integer, space->def->id, key_reg + 2);
+		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
+		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
 	}
 	VdbeComment((v, "Remove table grants"));
 	sqlReleaseTempRange(parser, key_reg, 4);
=========================================================================

> diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
> new file mode 100644
> index 000000000..915a265bd
> --- /dev/null
> +++ b/test/sql/gh-4546-sql-drop-grants.result
> @@ -0,0 +1,34 @@
> +test_run = require('test_run').new()
> +---
> +...
> +engine = test_run:get_cfg('engine')
> +---
> +...
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +---
> +- row_count: 0
> +...
> +-- If we drop the table with sql, all associated
> +-- grants must be deleted so we don't recieve an error
> +box.cfg{}

4. Box.cfg{} is already called by that time, so you
don't need to call it again. All test folders have a
file 'suite.ini'. Here you can find a name of the
script with is executed before executing tests in that
folder. For example, sql/suite.ini says:

    script = app.lua

It means, that app.lua is executed before all the other
tests in the folder. And it does box.cfg.

> +---
> +...
> +box.schema.user.create('test_user1')
> +---
> +...
> +box.schema.user.create('test_user2')
> +---
> +...
> +test_space = box.schema.create_space('T')

5. Lets add a minimal format. Just one field type/name.
Because generally SQL does not work with spaces not
having a format.

[Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Chris Sosnin]

Thank you for the review!
See fixes for your comments below:

> Sequences also can have grants. And sequence.drop removes them.
> I propose you to drop sequence grants in a second commit.

For this purpose I rewrote the function so it can be used not only for space
objects but for sequences too. Diff:

+/**
+ * Generate VDBE program to revoke all
+ * privileges associated with the given object.
+ *
+ * @param parser Parsing context.
+ * @param object_type Object type
+ * @param object_id Object id
+ * @param access Access array associated with an object
+ */
+static void
+vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
+			uint32_t object_id, struct access access[])
+{
+	struct Vdbe *v = sqlGetVdbe(parser);
+	assert(v != NULL);
+	/*
+	 * Get uid of users through access array
+	 * and generate code to delete corresponding
+	 * entries from _priv
+	 */
+	int key_reg = sqlGetTempRange(parser, 4);
+	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
+		if (!access[token].granted)
+			continue;
+		const struct user *user = user_find_by_token(token);
+		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
+		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
+			      object_type, P4_STATIC);
+		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
+		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
+		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
+	}
+	VdbeComment((v, "Remove %s grants", object_type));
+	sqlReleaseTempRange(parser, key_reg, 4);
+}

And it is used like this:
+	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
+				space->access);

> 1. Seems like the space is not changed here, so can be made
> 'const struct space *'.

Not needed anymore.

> 2. Lets assert that v is never NULL. By the time this
> function is called, Vdbe is always created.

Done:
+	struct Vdbe *v = sqlGetVdbe(parser);
+	assert(v != NULL);

> 3. The indentation is broken, some calls fit one line,
> and there are dangling whitespaces right below 'Add' on
> each line.

Done, you can see it in the first diff above.

> 4. Box.cfg{} is already called by that time, so you
> don't need to call it again.

Done.

> 5. Lets add a minimal format. Just one field type/name.
> Because generally SQL does not work with spaces not
> having a format.

Done. Also added sequence grants in tests:
+test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+seq = box.schema.sequence.create('S')
+ind = test_space:create_index('I', {sequence = 'S'})
+box.schema.user.grant('test_user1', 'write', 'sequence', 'S')

New version of the patch below:

Dropping table with sql removes everything
associated with it but grants, which is
inconsistent. Generating code for it fixes this bug.

Closes #4546
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
issue: https://github.com/tarantool/tarantool/issues/4546

 src/box/sql/build.c                       | 47 +++++++++++++++++++++++
 src/box/user.cc                           |  7 ++++
 src/box/user.h                            |  4 ++
 test/sql/gh-4546-sql-drop-grants.result   | 40 +++++++++++++++++++
 test/sql/gh-4546-sql-drop-grants.test.lua | 16 ++++++++
 5 files changed, 114 insertions(+)
 create mode 100644 test/sql/gh-4546-sql-drop-grants.result
 create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..af937de3b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -56,6 +56,7 @@
 #include "box/schema.h"
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
+#include "box/user.h"

 void
 sql_finish_coding(struct Parse *parse_context)
@@ -1523,6 +1524,42 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
 	sqlReleaseTempRange(parser, key_reg, 3);
 }

+/**
+ * Generate VDBE program to revoke all
+ * privileges associated with the given object.
+ *
+ * @param parser Parsing context.
+ * @param object_type Object type
+ * @param object_id Object id
+ * @param access Access array associated with an object
+ */
+static void
+vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
+			uint32_t object_id, struct access access[])
+{
+	struct Vdbe *v = sqlGetVdbe(parser);
+	assert(v != NULL);
+	/*
+	 * Get uid of users through access array
+	 * and generate code to delete corresponding
+	 * entries from _priv
+	 */
+	int key_reg = sqlGetTempRange(parser, 4);
+	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
+		if (!access[token].granted)
+			continue;
+		const struct user *user = user_find_by_token(token);
+		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
+		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
+			      object_type, P4_STATIC);
+		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
+		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
+		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
+	}
+	VdbeComment((v, "Remove %s grants", object_type));
+	sqlReleaseTempRange(parser, key_reg, 4);
+}
+
 /**
  * Generate code to drop a table.
  * This routine includes dropping triggers, sequences,
@@ -1538,6 +1575,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 {
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
+	/*
+	 * Remove all grants associated with
+	 * with the table being dropped.
+	 */
+	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
+				space->access);
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
@@ -1565,6 +1608,10 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
+		/* Delete entries from _priv */
+		vdbe_emit_revoke_object(parse_context, "sequence",
+					space->sequence->def->id,
+					space->sequence->access);
 		/* Delete entry from _sequence_data. */
 		int sequence_id_reg = ++parse_context->nMem;
 		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
diff --git a/src/box/user.cc b/src/box/user.cc
index a012cb196..fe0555886 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -517,6 +517,13 @@ user_find(uint32_t uid)
 	return user;
 }

+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token)
+{
+    return &users[auth_token];
+}
+
 /** Find user by name. */
 struct user *
 user_find_by_name(const char *name, uint32_t len)
diff --git a/src/box/user.h b/src/box/user.h
index 545401d5c..ccfc59346 100644
--- a/src/box/user.h
+++ b/src/box/user.h
@@ -112,6 +112,10 @@ user_find_by_name(const char *name, uint32_t len);
 struct user *
 user_find(uint32_t uid);

+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token);
+
 /** Create a cache of user's privileges in @a cr. */
 void
 credentials_create(struct credentials *cr, struct user *user);
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
new file mode 100644
index 000000000..ca95ed0f9
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -0,0 +1,40 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+- row_count: 0
+...
+-- If we drop the table with sql, all associated
+-- grants must be deleted so we don't recieve an error
+box.schema.user.create('test_user1')
+---
+...
+box.schema.user.create('test_user2')
+---
+...
+test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+---
+...
+seq = box.schema.sequence.create('S')
+---
+...
+ind = test_space:create_index('I', {sequence = 'S'})
+---
+...
+box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
+---
+...
+box.schema.user.grant('test_user1', 'read', 'space', 'T')
+---
+...
+box.schema.user.grant('test_user2', 'write', 'space', 'T')
+---
+...
+box.execute([[DROP TABLE T;]])
+---
+- row_count: 1
+...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
new file mode 100644
index 000000000..5ac4b5701
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -0,0 +1,16 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+
+-- If we drop the table with sql, all associated
+-- grants must be deleted so we don't recieve an error
+
+box.schema.user.create('test_user1')
+box.schema.user.create('test_user2')
+test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+seq = box.schema.sequence.create('S')
+ind = test_space:create_index('I', {sequence = 'S'})
+box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
+box.schema.user.grant('test_user1', 'read', 'space', 'T')
+box.schema.user.grant('test_user2', 'write', 'space', 'T')
+box.execute([[DROP TABLE T;]])
--
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

See N comments below, 2 my commits in the end, and on
top of your branch.

> Dropping table with sql removes everything
> associated with it but grants, which is
> inconsistent. Generating code for it fixes this bug.
> 
> Closes #4546
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
> issue: https://github.com/tarantool/tarantool/issues/4546
> 
>  src/box/sql/build.c                       | 47 +++++++++++++++++++++++
>  src/box/user.cc                           |  7 ++++
>  src/box/user.h                            |  4 ++
>  test/sql/gh-4546-sql-drop-grants.result   | 40 +++++++++++++++++++
>  test/sql/gh-4546-sql-drop-grants.test.lua | 16 ++++++++
>  5 files changed, 114 insertions(+)
>  create mode 100644 test/sql/gh-4546-sql-drop-grants.result
>  create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce63..af937de3b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1523,6 +1524,42 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
>  	sqlReleaseTempRange(parser, key_reg, 3);
>  }
> 
> +/**
> + * Generate VDBE program to revoke all
> + * privileges associated with the given object.
> + *
> + * @param parser Parsing context.
> + * @param object_type Object type
> + * @param object_id Object id
> + * @param access Access array associated with an object
> + */
> +static void
> +vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
> +			uint32_t object_id, struct access access[])
> +{
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	assert(v != NULL);
> +	/*
> +	 * Get uid of users through access array
> +	 * and generate code to delete corresponding
> +	 * entries from _priv
> +	 */
> +	int key_reg = sqlGetTempRange(parser, 4);

1. I run some tests and appeared, that VdbeComment
below asserts, when the object didn't have grants.

I've fixed it in my commit.

> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
> +		if (!access[token].granted)
> +			continue;
> +		const struct user *user = user_find_by_token(token);
> +		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
> +		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> +			      object_type, P4_STATIC);
> +		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
> +		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
> +		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
> +	}
> +	VdbeComment((v, "Remove %s grants", object_type));
> +	sqlReleaseTempRange(parser, key_reg, 4);
> +}
> +
>  /**
>   * Generate code to drop a table.
>   * This routine includes dropping triggers, sequences,
> @@ -1565,6 +1608,10 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>  	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>  	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
>  	if (space->sequence != NULL) {
> +		/* Delete entries from _priv */
> +		vdbe_emit_revoke_object(parse_context, "sequence",
> +					space->sequence->def->id,
> +					space->sequence->access);

2. I took a look at this code again, and realized, that
sequence problem is more severe. This code deletes a
sequence even if it does not belong to the space, i.e.
was not generated automatically.

I've fixed that in a separate commit.

>  		/* Delete entry from _sequence_data. */
>  		int sequence_id_reg = ++parse_context->nMem;
>  		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
> diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
> new file mode 100644
> index 000000000..5ac4b5701
> --- /dev/null
> +++ b/test/sql/gh-4546-sql-drop-grants.test.lua
> @@ -0,0 +1,16 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')

3. Engine is unused. To actually use it you need to
give it to create_space() arguments. I've fixed it in
my commit.

> +box.execute('pragma sql_default_engine=\''..engine..'\'')

4. Since this test does not create spaces via SQL, this
setting becomes not needed. I've drop it.

> +
> +-- If we drop the table with sql, all associated
> +-- grants must be deleted so we don't recieve an error
> +
> +box.schema.user.create('test_user1')
> +box.schema.user.create('test_user2')
> +test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
> +seq = box.schema.sequence.create('S')
> +ind = test_space:create_index('I', {sequence = 'S'})
> +box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
> +box.schema.user.grant('test_user1', 'read', 'space', 'T')
> +box.schema.user.grant('test_user2', 'write', 'space', 'T')
> +box.execute([[DROP TABLE T;]])

5. Test users are not dropped in the end of the test. I've
fixed it in my commit.

> --
> 2.21.0 (Apple Git-122.2)
> 

See my 2 commits below. First is review fixes for your commit.
Please, review it and squash if you agree with the fixes.

Second commit is independent one. Please, review it, but do
not squash. Instead, change author of the commit to yourself.
If you have comments on the second commit - bring them up and
lets discuss.

If you agree with both commits, and you've done squash and the
author change, then send for a second review to Nikita.

First commit:

===================================================================

commit 717ad6bbc382a798ef94d6a233e3b6464adc066b
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Dec 17 23:13:40 2019 +0100

    Review fixes

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index af937de3b..f1433645a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1529,13 +1529,13 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
  * privileges associated with the given object.
  *
  * @param parser Parsing context.
- * @param object_type Object type
- * @param object_id Object id
- * @param access Access array associated with an object
+ * @param object_type Object type.
+ * @param object_id Object id.
+ * @param access Access array associated with an object.
  */
 static void
 vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
-			uint32_t object_id, struct access access[])
+			uint32_t object_id, struct access *access)
 {
 	struct Vdbe *v = sqlGetVdbe(parser);
 	assert(v != NULL);
@@ -1545,9 +1545,11 @@ vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
 	 * entries from _priv
 	 */
 	int key_reg = sqlGetTempRange(parser, 4);
+	bool had_grants = false;
 	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
 		if (!access[token].granted)
 			continue;
+		had_grants = true;
 		const struct user *user = user_find_by_token(token);
 		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
 		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
@@ -1556,7 +1558,8 @@ vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
 		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
 		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
 	}
-	VdbeComment((v, "Remove %s grants", object_type));
+	if (had_grants)
+		VdbeComment((v, "Remove %s grants", object_type));
 	sqlReleaseTempRange(parser, key_reg, 4);
 }
 
@@ -1608,10 +1611,6 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
-		/* Delete entries from _priv */
-		vdbe_emit_revoke_object(parse_context, "sequence",
-					space->sequence->def->id,
-					space->sequence->access);
 		/* Delete entry from _sequence_data. */
 		int sequence_id_reg = ++parse_context->nMem;
 		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
index ca95ed0f9..04d9ef172 100644
--- a/test/sql/gh-4546-sql-drop-grants.result
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -4,19 +4,20 @@ test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
 ---
 ...
-box.execute('pragma sql_default_engine=\''..engine..'\'')
----
-- row_count: 0
-...
--- If we drop the table with sql, all associated
--- grants must be deleted so we don't recieve an error
+--
+-- gh-4546: DROP TABLE must delete all privileges given on that
+-- table to any user.
+--
 box.schema.user.create('test_user1')
 ---
 ...
 box.schema.user.create('test_user2')
 ---
 ...
-test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+test_space = box.schema.create_space('T', {		\
+	engine = engine,				\
+	format = {{'i', 'integer'}}			\
+})
 ---
 ...
 seq = box.schema.sequence.create('S')
@@ -25,9 +26,6 @@ seq = box.schema.sequence.create('S')
 ind = test_space:create_index('I', {sequence = 'S'})
 ---
 ...
-box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
----
-...
 box.schema.user.grant('test_user1', 'read', 'space', 'T')
 ---
 ...
@@ -38,3 +36,13 @@ box.execute([[DROP TABLE T;]])
 ---
 - row_count: 1
 ...
+box.schema.user.drop('test_user1')
+---
+...
+box.schema.user.drop('test_user2')
+---
+...
+box.space._sequence:select{}
+---
+- []
+...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
index 5ac4b5701..966733718 100644
--- a/test/sql/gh-4546-sql-drop-grants.test.lua
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -1,16 +1,22 @@
 test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
-box.execute('pragma sql_default_engine=\''..engine..'\'')
 
--- If we drop the table with sql, all associated
--- grants must be deleted so we don't recieve an error
+--
+-- gh-4546: DROP TABLE must delete all privileges given on that
+-- table to any user.
+--
 
 box.schema.user.create('test_user1')
 box.schema.user.create('test_user2')
-test_space = box.schema.create_space('T', {format = {{'i', 'integer'}}})
+test_space = box.schema.create_space('T', {		\
+	engine = engine,				\
+	format = {{'i', 'integer'}}			\
+})
 seq = box.schema.sequence.create('S')
 ind = test_space:create_index('I', {sequence = 'S'})
-box.schema.user.grant('test_user1', 'write', 'sequence', 'S')
 box.schema.user.grant('test_user1', 'read', 'space', 'T')
 box.schema.user.grant('test_user2', 'write', 'space', 'T')
 box.execute([[DROP TABLE T;]])
+box.schema.user.drop('test_user1')
+box.schema.user.drop('test_user2')
+box.space._sequence:select{}

===================================================================

Second commit:

===================================================================

commit 6550d9034dc5cce16f824379b28ce5abd138c6aa
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Dec 17 23:59:00 2019 +0100

    sql: drop only generated sequence in DROP TABLE
    
    It is possible to create a sequence manually, and give it to a
    newly created index as a source of unique identifiers. Such
    sequences are not owned by a space, and therefore shouldn't be
    deleted when the space is dropped. They are not dropped when
    space:drop() in Lua is called, but were dropped in SQL
    'DROP TABLE' before this patch. Now Lua and SQL are consistent in
    that case.
    
    Follow up #4546

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f1433645a..93aae0894 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1611,26 +1611,33 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
-		/* Delete entry from _sequence_data. */
-		int sequence_id_reg = ++parse_context->nMem;
-		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
-				  sequence_id_reg);
-		sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
-				  idx_rec_reg);
-		VdbeComment((v, "Delete entry from _sequence_data"));
 		/* Delete entry from _space_sequence. */
 		sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1,
 				  idx_rec_reg);
 		sqlVdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID,
 				  idx_rec_reg);
 		VdbeComment((v, "Delete entry from _space_sequence"));
-		/* Delete entry by id from _sequence. */
-		sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
-		VdbeComment((v, "Delete entry from _sequence"));
+		if (space->sequence->is_generated) {
+			/* Delete entry from _sequence_data. */
+			int sequence_id_reg = ++parse_context->nMem;
+			sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
+				      sequence_id_reg);
+			sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				      idx_rec_reg);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
+				      idx_rec_reg);
+			VdbeComment((v, "Delete entry from _sequence_data"));
+			/* Delete entries from _priv */
+		        vdbe_emit_revoke_object(parse_context, "sequence",
+						space->sequence->def->id,
+						space->sequence->access);
+			/* Delete entry by id from _sequence. */
+			sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				      idx_rec_reg);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID,
+				      idx_rec_reg);
+			VdbeComment((v, "Delete entry from _sequence"));
+		}
 	}
 	/* Delete all child FK constraints. */
 	struct fk_constraint *child_fk;
diff --git a/test/sql/autoincrement.result b/test/sql/autoincrement.result
new file mode 100644
index 000000000..ca3d6f3cf
--- /dev/null
+++ b/test/sql/autoincrement.result
@@ -0,0 +1,80 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+
+--
+-- Not automatic sequence should not be removed by DROP TABLE.
+--
+box.schema.user.create('user1')
+ | ---
+ | ...
+test_space = box.schema.create_space('T', {         \
+    engine = engine,                                \
+    format = {{'i', 'integer'}}                     \
+})
+ | ---
+ | ...
+seq = box.schema.sequence.create('S')
+ | ---
+ | ...
+ind = test_space:create_index('I', {sequence = 'S'})
+ | ---
+ | ...
+box.schema.user.grant('user1', 'write', 'sequence', 'S')
+ | ---
+ | ...
+box.execute('DROP TABLE t');
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 1 and seqs[1].name == seq.name or seqs
+ | ---
+ | - true
+ | ...
+seq:drop()
+ | ---
+ | ...
+
+--
+-- Automatic sequence should be removed at DROP TABLE, together
+-- with all the grants.
+--
+box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 1 or seqs
+ | ---
+ | - true
+ | ...
+seq = seqs[1].name
+ | ---
+ | ...
+box.schema.user.grant('user1', 'write', 'sequence', seq)
+ | ---
+ | ...
+box.execute('DROP TABLE t')
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 0 or seqs
+ | ---
+ | - true
+ | ...
+
+box.schema.user.drop('user1')
+ | ---
+ | ...
diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua
new file mode 100644
index 000000000..63a902aea
--- /dev/null
+++ b/test/sql/autoincrement.test.lua
@@ -0,0 +1,33 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+
+--
+-- Not automatic sequence should not be removed by DROP TABLE.
+--
+box.schema.user.create('user1')
+test_space = box.schema.create_space('T', {         \
+    engine = engine,                                \
+    format = {{'i', 'integer'}}                     \
+})
+seq = box.schema.sequence.create('S')
+ind = test_space:create_index('I', {sequence = 'S'})
+box.schema.user.grant('user1', 'write', 'sequence', 'S')
+box.execute('DROP TABLE t');
+seqs = box.space._sequence:select{}
+#seqs == 1 and seqs[1].name == seq.name or seqs
+seq:drop()
+
+--
+-- Automatic sequence should be removed at DROP TABLE, together
+-- with all the grants.
+--
+box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
+seqs = box.space._sequence:select{}
+#seqs == 1 or seqs
+seq = seqs[1].name
+box.schema.user.grant('user1', 'write', 'sequence', seq)
+box.execute('DROP TABLE t')
+seqs = box.space._sequence:select{}
+#seqs == 0 or seqs
+
+box.schema.user.drop('user1')
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
index 04d9ef172..3ba26e380 100644
--- a/test/sql/gh-4546-sql-drop-grants.result
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -42,7 +42,10 @@ box.schema.user.drop('test_user1')
 box.schema.user.drop('test_user2')
 ---
 ...
-box.space._sequence:select{}
+seqs = box.space._sequence:select{}
 ---
-- []
+...
+#seqs == 1 and seqs[1].name == seq.name or seqs
+---
+- true
 ...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
index 966733718..fc2c27e08 100644
--- a/test/sql/gh-4546-sql-drop-grants.test.lua
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -19,4 +19,5 @@ box.schema.user.grant('test_user2', 'write', 'space', 'T')
 box.execute([[DROP TABLE T;]])
 box.schema.user.drop('test_user1')
 box.schema.user.drop('test_user2')
-box.space._sequence:select{}
+seqs = box.space._sequence:select{}
+#seqs == 1 and seqs[1].name == seq.name or seqs

Re: [Tarantool-patches] [PATCH] sql: remove grants associated with the table

[Vladislav Shpilevoy]

On 18/12/2019 00:13, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See N comments below, 2 my commits in the end, and on
> top of your branch.

Sorry, N = 5. Five comments.

[Tarantool-patches] [PATCH 0/2] sql: revoke table privileges on drop

[Chris Sosnin]

Hi! Thank you for the review and for your fixes!

> +box.schema.user.drop('test_user1')
> +---
> +...
> +box.schema.user.drop('test_user2')
> +---
> +...
> +box.space._sequence:select{}
> +---
> +- []
> +...

I moved sequence tests to the second commit, firstly, because here you
don't do seq:drop() (I believe because after my initial patch sequences
are getting deleted anyways, but after yours seq remains in _sequence
space) and, secondly, because this way we can better isolate these two patches
from each other.

I agree with the rest, especially with your catch about sequences.
Thus, I'm sending this patchset to Nikita.

branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
issue: https://github.com/tarantool/tarantool/issues/4546

Chris Sosnin (2):
  sql: remove grants associated with the table
  sql: drop only generated sequence in DROP TABLE

 src/box/sql/build.c                       | 81 +++++++++++++++++++----
 src/box/user.cc                           |  7 ++
 src/box/user.h                            |  4 ++
 test/sql/autoincrement.result             | 80 ++++++++++++++++++++++
 test/sql/autoincrement.test.lua           | 33 +++++++++
 test/sql/gh-4546-sql-drop-grants.result   | 54 +++++++++++++++
 test/sql/gh-4546-sql-drop-grants.test.lua | 24 +++++++
 7 files changed, 269 insertions(+), 14 deletions(-)
 create mode 100644 test/sql/autoincrement.result
 create mode 100644 test/sql/autoincrement.test.lua
 create mode 100644 test/sql/gh-4546-sql-drop-grants.result
 create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua

-- 
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table

[Chris Sosnin]

Dropping table with sql removes everything
associated with it but grants, which is
inconsistent. Generating code for it fixes this bug.

Closes #4546
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
issue: https://github.com/tarantool/tarantool/issues/4546

 src/box/sql/build.c                       | 46 +++++++++++++++++++++++
 src/box/user.cc                           |  7 ++++
 src/box/user.h                            |  4 ++
 test/sql/gh-4546-sql-drop-grants.result   | 38 +++++++++++++++++++
 test/sql/gh-4546-sql-drop-grants.test.lua | 19 ++++++++++
 5 files changed, 114 insertions(+)
 create mode 100644 test/sql/gh-4546-sql-drop-grants.result
 create mode 100644 test/sql/gh-4546-sql-drop-grants.test.lua

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..f1433645a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -56,6 +56,7 @@
 #include "box/schema.h"
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
+#include "box/user.h"
 
 void
 sql_finish_coding(struct Parse *parse_context)
@@ -1523,6 +1524,45 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
 	sqlReleaseTempRange(parser, key_reg, 3);
 }
 
+/**
+ * Generate VDBE program to revoke all
+ * privileges associated with the given object.
+ *
+ * @param parser Parsing context.
+ * @param object_type Object type.
+ * @param object_id Object id.
+ * @param access Access array associated with an object.
+ */
+static void
+vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
+			uint32_t object_id, struct access *access)
+{
+	struct Vdbe *v = sqlGetVdbe(parser);
+	assert(v != NULL);
+	/*
+	 * Get uid of users through access array
+	 * and generate code to delete corresponding
+	 * entries from _priv
+	 */
+	int key_reg = sqlGetTempRange(parser, 4);
+	bool had_grants = false;
+	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
+		if (!access[token].granted)
+			continue;
+		had_grants = true;
+		const struct user *user = user_find_by_token(token);
+		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
+		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
+			      object_type, P4_STATIC);
+		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
+		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
+		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
+	}
+	if (had_grants)
+		VdbeComment((v, "Remove %s grants", object_type));
+	sqlReleaseTempRange(parser, key_reg, 4);
+}
+
 /**
  * Generate code to drop a table.
  * This routine includes dropping triggers, sequences,
@@ -1538,6 +1578,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 {
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
+	/*
+	 * Remove all grants associated with
+	 * with the table being dropped.
+	 */
+	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
+				space->access);
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
diff --git a/src/box/user.cc b/src/box/user.cc
index a012cb196..fe0555886 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -517,6 +517,13 @@ user_find(uint32_t uid)
 	return user;
 }
 
+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token)
+{
+    return &users[auth_token];
+}
+
 /** Find user by name. */
 struct user *
 user_find_by_name(const char *name, uint32_t len)
diff --git a/src/box/user.h b/src/box/user.h
index 545401d5c..ccfc59346 100644
--- a/src/box/user.h
+++ b/src/box/user.h
@@ -112,6 +112,10 @@ user_find_by_name(const char *name, uint32_t len);
 struct user *
 user_find(uint32_t uid);
 
+/* Find a user by authentication token. */
+struct user *
+user_find_by_token(uint8_t auth_token);
+
 /** Create a cache of user's privileges in @a cr. */
 void
 credentials_create(struct credentials *cr, struct user *user);
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
new file mode 100644
index 000000000..1079d2ab4
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -0,0 +1,38 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+--
+-- gh-4546: DROP TABLE must delete all privileges given on that
+-- table to any user.
+--
+box.schema.user.create('test_user1')
+---
+...
+box.schema.user.create('test_user2')
+---
+...
+test_space = box.schema.create_space('T', {		\
+	engine = engine,				\
+	format = {{'i', 'integer'}}			\
+})
+---
+...
+box.schema.user.grant('test_user1', 'read', 'space', 'T')
+---
+...
+box.schema.user.grant('test_user2', 'write', 'space', 'T')
+---
+...
+box.execute([[DROP TABLE T;]])
+---
+- row_count: 1
+...
+box.schema.user.drop('test_user1')
+---
+...
+box.schema.user.drop('test_user2')
+---
+...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
new file mode 100644
index 000000000..09cfeda19
--- /dev/null
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -0,0 +1,19 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+
+--
+-- gh-4546: DROP TABLE must delete all privileges given on that
+-- table to any user.
+--
+
+box.schema.user.create('test_user1')
+box.schema.user.create('test_user2')
+test_space = box.schema.create_space('T', {		\
+	engine = engine,				\
+	format = {{'i', 'integer'}}			\
+})
+box.schema.user.grant('test_user1', 'read', 'space', 'T')
+box.schema.user.grant('test_user2', 'write', 'space', 'T')
+box.execute([[DROP TABLE T;]])
+box.schema.user.drop('test_user1')
+box.schema.user.drop('test_user2')
-- 
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE

[Chris Sosnin]

It is possible to create a sequence manually, and give it to a
newly created index as a source of unique identifiers. Such
sequences are not owned by a space, and therefore shouldn't be
deleted when the space is dropped. They are not dropped when
space:drop() in Lua is called, but were dropped in SQL
'DROP TABLE' before this patch. Now Lua and SQL are consistent in
that case.

Follow up #4546
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
issue: https://github.com/tarantool/tarantool/issues/4546

 src/box/sql/build.c                       | 35 ++++++----
 test/sql/autoincrement.result             | 80 +++++++++++++++++++++++
 test/sql/autoincrement.test.lua           | 33 ++++++++++
 test/sql/gh-4546-sql-drop-grants.result   | 16 +++++
 test/sql/gh-4546-sql-drop-grants.test.lua |  5 ++
 5 files changed, 155 insertions(+), 14 deletions(-)
 create mode 100644 test/sql/autoincrement.result
 create mode 100644 test/sql/autoincrement.test.lua

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f1433645a..93aae0894 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1611,26 +1611,33 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
 	sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg);
 	if (space->sequence != NULL) {
-		/* Delete entry from _sequence_data. */
-		int sequence_id_reg = ++parse_context->nMem;
-		sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
-				  sequence_id_reg);
-		sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
-				  idx_rec_reg);
-		VdbeComment((v, "Delete entry from _sequence_data"));
 		/* Delete entry from _space_sequence. */
 		sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1,
 				  idx_rec_reg);
 		sqlVdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID,
 				  idx_rec_reg);
 		VdbeComment((v, "Delete entry from _space_sequence"));
-		/* Delete entry by id from _sequence. */
-		sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
-		VdbeComment((v, "Delete entry from _sequence"));
+		if (space->sequence->is_generated) {
+			/* Delete entry from _sequence_data. */
+			int sequence_id_reg = ++parse_context->nMem;
+			sqlVdbeAddOp2(v, OP_Integer, space->sequence->def->id,
+				      sequence_id_reg);
+			sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				      idx_rec_reg);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_DATA_ID,
+				      idx_rec_reg);
+			VdbeComment((v, "Delete entry from _sequence_data"));
+			/* Delete entries from _priv */
+		        vdbe_emit_revoke_object(parse_context, "sequence",
+						space->sequence->def->id,
+						space->sequence->access);
+			/* Delete entry by id from _sequence. */
+			sqlVdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1,
+				      idx_rec_reg);
+			sqlVdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID,
+				      idx_rec_reg);
+			VdbeComment((v, "Delete entry from _sequence"));
+		}
 	}
 	/* Delete all child FK constraints. */
 	struct fk_constraint *child_fk;
diff --git a/test/sql/autoincrement.result b/test/sql/autoincrement.result
new file mode 100644
index 000000000..ca3d6f3cf
--- /dev/null
+++ b/test/sql/autoincrement.result
@@ -0,0 +1,80 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+
+--
+-- Not automatic sequence should not be removed by DROP TABLE.
+--
+box.schema.user.create('user1')
+ | ---
+ | ...
+test_space = box.schema.create_space('T', {         \
+    engine = engine,                                \
+    format = {{'i', 'integer'}}                     \
+})
+ | ---
+ | ...
+seq = box.schema.sequence.create('S')
+ | ---
+ | ...
+ind = test_space:create_index('I', {sequence = 'S'})
+ | ---
+ | ...
+box.schema.user.grant('user1', 'write', 'sequence', 'S')
+ | ---
+ | ...
+box.execute('DROP TABLE t');
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 1 and seqs[1].name == seq.name or seqs
+ | ---
+ | - true
+ | ...
+seq:drop()
+ | ---
+ | ...
+
+--
+-- Automatic sequence should be removed at DROP TABLE, together
+-- with all the grants.
+--
+box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 1 or seqs
+ | ---
+ | - true
+ | ...
+seq = seqs[1].name
+ | ---
+ | ...
+box.schema.user.grant('user1', 'write', 'sequence', seq)
+ | ---
+ | ...
+box.execute('DROP TABLE t')
+ | ---
+ | - row_count: 1
+ | ...
+seqs = box.space._sequence:select{}
+ | ---
+ | ...
+#seqs == 0 or seqs
+ | ---
+ | - true
+ | ...
+
+box.schema.user.drop('user1')
+ | ---
+ | ...
diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua
new file mode 100644
index 000000000..63a902aea
--- /dev/null
+++ b/test/sql/autoincrement.test.lua
@@ -0,0 +1,33 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+
+--
+-- Not automatic sequence should not be removed by DROP TABLE.
+--
+box.schema.user.create('user1')
+test_space = box.schema.create_space('T', {         \
+    engine = engine,                                \
+    format = {{'i', 'integer'}}                     \
+})
+seq = box.schema.sequence.create('S')
+ind = test_space:create_index('I', {sequence = 'S'})
+box.schema.user.grant('user1', 'write', 'sequence', 'S')
+box.execute('DROP TABLE t');
+seqs = box.space._sequence:select{}
+#seqs == 1 and seqs[1].name == seq.name or seqs
+seq:drop()
+
+--
+-- Automatic sequence should be removed at DROP TABLE, together
+-- with all the grants.
+--
+box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
+seqs = box.space._sequence:select{}
+#seqs == 1 or seqs
+seq = seqs[1].name
+box.schema.user.grant('user1', 'write', 'sequence', seq)
+box.execute('DROP TABLE t')
+seqs = box.space._sequence:select{}
+#seqs == 0 or seqs
+
+box.schema.user.drop('user1')
diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
index 1079d2ab4..9376c367d 100644
--- a/test/sql/gh-4546-sql-drop-grants.result
+++ b/test/sql/gh-4546-sql-drop-grants.result
@@ -20,6 +20,12 @@ test_space = box.schema.create_space('T', {		\
 })
 ---
 ...
+seq = box.schema.sequence.create('S')
+---
+...
+ind = test_space:create_index('I', {sequence = 'S'})
+---
+...
 box.schema.user.grant('test_user1', 'read', 'space', 'T')
 ---
 ...
@@ -36,3 +42,13 @@ box.schema.user.drop('test_user1')
 box.schema.user.drop('test_user2')
 ---
 ...
+seqs = box.space._sequence:select{}
+---
+...
+#seqs == 1 and seqs[1].name == seq.name or seqs
+---
+- true
+...
+seq:drop()
+---
+...
diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
index 09cfeda19..ddb8cb0bb 100644
--- a/test/sql/gh-4546-sql-drop-grants.test.lua
+++ b/test/sql/gh-4546-sql-drop-grants.test.lua
@@ -12,8 +12,13 @@ test_space = box.schema.create_space('T', {		\
 	engine = engine,				\
 	format = {{'i', 'integer'}}			\
 })
+seq = box.schema.sequence.create('S')
+ind = test_space:create_index('I', {sequence = 'S'})
 box.schema.user.grant('test_user1', 'read', 'space', 'T')
 box.schema.user.grant('test_user2', 'write', 'space', 'T')
 box.execute([[DROP TABLE T;]])
 box.schema.user.drop('test_user1')
 box.schema.user.drop('test_user2')
+seqs = box.space._sequence:select{}
+#seqs == 1 and seqs[1].name == seq.name or seqs
+seq:drop()
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE

[Nikita Pettik]

On 18 Dec 14:00, Chris Sosnin wrote:
>  create mode 100644 test/sql/autoincrement.result
>  create mode 100644 test/sql/autoincrement.test.lua
> 
> diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua

Patch itself is LGTM (as obvious). But I am not a big fan of placing each
test in separate file (at least when there's no specific issue for that).
Moreover, in commit message it is said that this patch is follow-up to
#4546, so could you move this test to gh-4546-sql-drop-grants.test.lua ?

> new file mode 100644
> index 000000000..63a902aea
> --- /dev/null
> +++ b/test/sql/autoincrement.test.lua
> @@ -0,0 +1,33 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +
> +--
> +-- Not automatic sequence should not be removed by DROP TABLE.
> +--
> +box.schema.user.create('user1')
> +test_space = box.schema.create_space('T', {         \
> +    engine = engine,                                \
> +    format = {{'i', 'integer'}}                     \
> +})
> +seq = box.schema.sequence.create('S')
> +ind = test_space:create_index('I', {sequence = 'S'})
> +box.schema.user.grant('user1', 'write', 'sequence', 'S')
> +box.execute('DROP TABLE t');
> +seqs = box.space._sequence:select{}
> +#seqs == 1 and seqs[1].name == seq.name or seqs
> +seq:drop()
> +
> +--
> +-- Automatic sequence should be removed at DROP TABLE, together
> +-- with all the grants.
> +--
> +box.execute('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)')
> +seqs = box.space._sequence:select{}
> +#seqs == 1 or seqs
> +seq = seqs[1].name
> +box.schema.user.grant('user1', 'write', 'sequence', seq)
> +box.execute('DROP TABLE t')
> +seqs = box.space._sequence:select{}
> +#seqs == 0 or seqs
> +
> +box.schema.user.drop('user1')
> diff --git a/test/sql/gh-4546-sql-drop-grants.result b/test/sql/gh-4546-sql-drop-grants.result
> index 1079d2ab4..9376c367d 100644
> --- a/test/sql/gh-4546-sql-drop-grants.result
> +++ b/test/sql/gh-4546-sql-drop-grants.result
> @@ -20,6 +20,12 @@ test_space = box.schema.create_space('T', {		\
>  })
>  ---
>  ...
> +seq = box.schema.sequence.create('S')
> +---
> +...
> +ind = test_space:create_index('I', {sequence = 'S'})
> +---
> +...
>  box.schema.user.grant('test_user1', 'read', 'space', 'T')
>  ---
>  ...
> @@ -36,3 +42,13 @@ box.schema.user.drop('test_user1')
>  box.schema.user.drop('test_user2')
>  ---
>  ...
> +seqs = box.space._sequence:select{}
> +---
> +...
> +#seqs == 1 and seqs[1].name == seq.name or seqs
> +---
> +- true
> +...
> +seq:drop()
> +---
> +...
> diff --git a/test/sql/gh-4546-sql-drop-grants.test.lua b/test/sql/gh-4546-sql-drop-grants.test.lua
> index 09cfeda19..ddb8cb0bb 100644
> --- a/test/sql/gh-4546-sql-drop-grants.test.lua
> +++ b/test/sql/gh-4546-sql-drop-grants.test.lua
> @@ -12,8 +12,13 @@ test_space = box.schema.create_space('T', {		\
>  	engine = engine,				\
>  	format = {{'i', 'integer'}}			\
>  })
> +seq = box.schema.sequence.create('S')
> +ind = test_space:create_index('I', {sequence = 'S'})
>  box.schema.user.grant('test_user1', 'read', 'space', 'T')
>  box.schema.user.grant('test_user2', 'write', 'space', 'T')
>  box.execute([[DROP TABLE T;]])
>  box.schema.user.drop('test_user1')
>  box.schema.user.drop('test_user2')
> +seqs = box.space._sequence:select{}
> +#seqs == 1 and seqs[1].name == seq.name or seqs
> +seq:drop()
> -- 
> 2.21.0 (Apple Git-122.2)
> 

Re: [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table

[Nikita Pettik]

On 18 Dec 14:00, Chris Sosnin wrote:
> Dropping table with sql removes everything
> associated with it but grants, which is
> inconsistent. Generating code for it fixes this bug.

Feel free to use up to 77 chars in commit message. You can enable
auto-formatting in vim:

au FileType gitcommit setlocal tw=72

Or you can manually highlight commit message and format with :gq
 
> Closes #4546
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4546-sql-drop-grants
> issue: https://github.com/tarantool/tarantool/issues/4546

You don't have to put these links in each patch, they are required
only in cover letter.
 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 51cd7ce63..f1433645a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> +static void
> +vdbe_emit_revoke_object(struct Parse *parser, const char *object_type,
> +			uint32_t object_id, struct access *access)
> +{
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	assert(v != NULL);
> +	/*
> +	 * Get uid of users through access array
> +	 * and generate code to delete corresponding
> +	 * entries from _priv
> +	 */
> +	int key_reg = sqlGetTempRange(parser, 4);
> +	bool had_grants = false;

As a rule we use present time: has_grants.

> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
> +		if (!access[token].granted)
> +			continue;
> +		had_grants = true;

Personally I wouldn't bother with separate variable solely to
display comment. Let's keep it tho.

> +		const struct user *user = user_find_by_token(token);
> +		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
> +		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> +			      object_type, P4_STATIC);
> +		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
> +		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
> +		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
> +	}
> +	if (had_grants)
> +		VdbeComment((v, "Remove %s grants", object_type));
> +	sqlReleaseTempRange(parser, key_reg, 4);
> +}
> +
>  /**
>   * Generate code to drop a table.
>   * This routine includes dropping triggers, sequences,
> @@ -1538,6 +1578,12 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
>  {
>  	struct Vdbe *v = sqlGetVdbe(parse_context);
>  	assert(v != NULL);
> +	/*
> +	 * Remove all grants associated with
> +	 * with the table being dropped.

Nit: double with.

The rest is OK. LGTM and pushed to master.

> +	 */
> +	vdbe_emit_revoke_object(parse_context, "space", space->def->id,
> +				space->access);
>  	/*

Re: [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE

[Vladislav Shpilevoy]

Thanks for the review!

On 24/12/2019 02:23, Nikita Pettik wrote:
> On 18 Dec 14:00, Chris Sosnin wrote:
>>  create mode 100644 test/sql/autoincrement.result
>>  create mode 100644 test/sql/autoincrement.test.lua
>>
>> diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua
> 
> Patch itself is LGTM (as obvious). But I am not a big fan of placing each
> test in separate file (at least when there's no specific issue for that).

As you know, me neither. I added this new file to put here all
autoincrement stuff now and in future. We have sql-tap/autoinc,
but sql-tap tests are really hard to write and fix.

> Moreover, in commit message it is said that this patch is follow-up to
> #4546, so could you move this test to gh-4546-sql-drop-grants.test.lua ?

4546 was about table grants. The test in autoincrement file about a
different bug - drop of sequence. It is kinda related to 4546 in that
sequence grants also should be dropped, if their sequence is deleted.

Re: [Tarantool-patches] [PATCH 1/2] sql: remove grants associated with the table

[Vladislav Shpilevoy]

>> +	for (uint8_t token = 0; token < BOX_USER_MAX; ++token) {
>> +		if (!access[token].granted)
>> +			continue;
>> +		had_grants = true;
> 
> Personally I wouldn't bother with separate variable solely to
> display comment. Let's keep it tho.
> 

We wouldn't either, but appears that VdbeComment asserts when there
is no operation before it. So we must ensure, that the comment is
added only in case at least one opcode is generated.

>> +		const struct user *user = user_find_by_token(token);
>> +		sqlVdbeAddOp2(v, OP_Integer, user->def->uid, key_reg);
>> +		sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
>> +			      object_type, P4_STATIC);
>> +		sqlVdbeAddOp2(v, OP_Integer, object_id, key_reg + 2);
>> +		sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 3, key_reg + 3);
>> +		sqlVdbeAddOp2(v, OP_SDelete, BOX_PRIV_ID, key_reg + 3);
>> +	}
>> +	if (had_grants)
>> +		VdbeComment((v, "Remove %s grants", object_type));
>> +	sqlReleaseTempRange(parser, key_reg, 4);
>> +}
>> +

Re: [Tarantool-patches] [PATCH 2/2] sql: drop only generated sequence in DROP TABLE

[Nikita Pettik]

On 24 Dec 17:26, Vladislav Shpilevoy wrote:
> Thanks for the review!
> 
> On 24/12/2019 02:23, Nikita Pettik wrote:
> > On 18 Dec 14:00, Chris Sosnin wrote:
> >>  create mode 100644 test/sql/autoincrement.result
> >>  create mode 100644 test/sql/autoincrement.test.lua
> >>
> >> diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua
> > 
> > Patch itself is LGTM (as obvious). But I am not a big fan of placing each
> > test in separate file (at least when there's no specific issue for that).
> 
> As you know, me neither. I added this new file to put here all
> autoincrement stuff now and in future. We have sql-tap/autoinc,
> but sql-tap tests are really hard to write and fix.

It's hard in comparison with sql/ suite. But in general, sql tap tests
are quite simple and easy to write (IMHO).
 
> > Moreover, in commit message it is said that this patch is follow-up to
> > #4546, so could you move this test to gh-4546-sql-drop-grants.test.lua ?
> 
> 4546 was about table grants. The test in autoincrement file about a
> different bug - drop of sequence. It is kinda related to 4546 in that
> sequence grants also should be dropped, if their sequence is deleted.

Ok, then I will simply remove 'follow-up' remark and push it to master.