[Tarantool-patches] [PATCH 0/2] Support <ALTER TABLE DROP CONSTRAINT>

[Tarantool-patches] [PATCH 0/2] Support <ALTER TABLE DROP CONSTRAINT>

[Roman Khabibov]

The first patch is the little refactoring to reduce code.
The second one is about the sql operation itself.

Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4120-drop-constraint
Issue: https://github.com/tarantool/tarantool/issues/4120

Roman Khabibov (2):
  sql: refactor index drop code
  sql: support constraint drop

 src/box/constraint_id.h      |   1 +
 src/box/errcode.h            |   2 +-
 src/box/sql/alter.c          |   2 +-
 src/box/sql/build.c          | 138 ++++++++++++++++++++---------------
 src/box/sql/parse.y          |   4 +-
 src/box/sql/parse_def.h      |  15 ++--
 src/box/sql/sqlInt.h         |   6 +-
 test/sql-tap/alter2.test.lua |   2 +-
 test/sql/checks.result       |   2 +-
 test/sql/constraint.result   |  67 +++++++++++++++++
 test/sql/constraint.test.lua |  24 ++++++
 11 files changed, 187 insertions(+), 76 deletions(-)

-- 
2.21.0 (Apple Git-122)

[Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Roman Khabibov]

Wrap index drop opcode emitting into single function.

Part of #4120
---
 src/box/sql/build.c | 52 ++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bc50ecbfa..58a6a8a6b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
 	sql_table_delete_from(parse, src_list, where);
 }
 
+/**
+ * Generate VDBE program to remove entry with @a index_id and @a
+ * space_id from _index space.
+ *
+ * Note: index_id_reg have to be allocated immediately after
+ * space_id_reg.
+ */
+static void
+vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
+		     uint32_t index_id_reg, uint32_t space_id_reg)
+{
+	struct Vdbe *v = sqlGetVdbe(parse_context);
+	assert(v != NULL);
+	int record_reg = ++parse_context->nMem;
+	sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
+	sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
+	sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+	VdbeComment((v, "Remove index iid =  %u", index_id));
+}
+
 /**
  * Generate VDBE program to remove entry from _fk_constraint space.
  *
@@ -1680,23 +1700,14 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 			 * secondary exist.
 			 */
 			for (uint32_t i = 1; i < index_count; ++i) {
-				sqlVdbeAddOp2(v, OP_Integer,
-						  space->index[i]->def->iid,
-						  index_id_reg);
-				sqlVdbeAddOp3(v, OP_MakeRecord,
-						  space_id_reg, 2, idx_rec_reg);
-				sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID,
-						  idx_rec_reg);
-				VdbeComment((v,
-					     "Remove secondary index iid = %u",
-					     space->index[i]->def->iid));
+				vdbe_emit_index_drop(parse_context,
+						     space->index[i]->def->iid,
+						     index_id_reg,
+						     space_id_reg);
 			}
 		}
-		sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg);
-		sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2,
-				  idx_rec_reg);
-		sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg);
-		VdbeComment((v, "Remove primary index"));
+		vdbe_emit_index_drop(parse_context, 0, index_id_reg,
+				     space_id_reg);
 	}
 	/* Delete records about the space from the _truncate. */
 	sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg);
@@ -2704,18 +2715,11 @@ sql_drop_index(struct Parse *parse_context)
 		goto exit_drop_index;
 	}
 
-	/*
-	 * Generate code to remove entry from _index space
-	 * But firstly, delete statistics since schema
-	 * changes after DDL.
-	 */
-	int record_reg = ++parse_context->nMem;
 	int space_id_reg = ++parse_context->nMem;
 	int index_id_reg = ++parse_context->nMem;
 	sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
-	sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
-	sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
-	sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
+	vdbe_emit_index_drop(parse_context, index_id, index_id_reg,
+			     space_id_reg);
 	sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
  exit_drop_index:
 	sqlSrcListDelete(db, table_list);
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Nikita Pettik]

On 24 Jan 17:21, Roman Khabibov wrote:
> Wrap index drop opcode emitting into single function.
> 
> Part of #4120
> ---
>  src/box/sql/build.c | 52 ++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bc50ecbfa..58a6a8a6b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>  	sql_table_delete_from(parse, src_list, where);
>  }
>  
> +/**
> + * Generate VDBE program to remove entry with @a index_id and @a
> + * space_id from _index space.

Is @a common shortcat for @param? If so, please share a link to docs,
since I've managed to find only this:
'''
\a <word>
Displays the argument in italics. Use this command to emphasize words.
Use this command to refer to member arguments in the running text.
'''

> + * Note: index_id_reg have to be allocated immediately after
> + * space_id_reg.
> + */

Nit: have -> has.

Please, add an assertion verifing that.

> +static void
> +vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
> +		     uint32_t index_id_reg, uint32_t space_id_reg)

I'd also consider moving storing of index_id to register out of
this function. Then you'll need only space_id_reg to process
deletion.

> +{
> +	struct Vdbe *v = sqlGetVdbe(parse_context);
> +	assert(v != NULL);
> +	int record_reg = ++parse_context->nMem;
> +	sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
> +	sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
> +	sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
> +	VdbeComment((v, "Remove index iid =  %u", index_id));
> +}
> +
>  /**
>   * Generate VDBE program to remove entry from _fk_constraint space.
>   *

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Vladislav Shpilevoy]

>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index bc50ecbfa..58a6a8a6b 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>>  	sql_table_delete_from(parse, src_list, where);
>>  }
>>  
>> +/**
>> + * Generate VDBE program to remove entry with @a index_id and @a
>> + * space_id from _index space.
> 
> Is @a common shortcat for @param? If so, please share a link to docs,
> since I've managed to find only this:
> '''
> \a <word>
> Displays the argument in italics. Use this command to emphasize words.
> Use this command to refer to member arguments in the running text.
> '''

We use @param when describe parameters. In @brief sections we use @a
intentionally. Probably because

1) @param is used to attach a description to a paramter, and for
nothing else: http://www.doxygen.nl/manual/commands.html#cmdparam

2) @a is just shorter, and we can use parameter name multiple times
in @brief, so it is important.

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Nikita Pettik]

On 29 Jan 22:56, Vladislav Shpilevoy wrote:
> >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> >> index bc50ecbfa..58a6a8a6b 100644
> >> --- a/src/box/sql/build.c
> >> +++ b/src/box/sql/build.c
> >> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> >>  	sql_table_delete_from(parse, src_list, where);
> >>  }
> >>  
> >> +/**
> >> + * Generate VDBE program to remove entry with @a index_id and @a
> >> + * space_id from _index space.
> > 
> > Is @a common shortcat for @param? If so, please share a link to docs,
> > since I've managed to find only this:
> > '''
> > \a <word>
> > Displays the argument in italics. Use this command to emphasize words.
> > Use this command to refer to member arguments in the running text.
> > '''
> 
> We use @param when describe parameters. In @brief sections we use @a
> intentionally. Probably because
> 
> 1) @param is used to attach a description to a paramter, and for
> nothing else: http://www.doxygen.nl/manual/commands.html#cmdparam
> 
> 2) @a is just shorter, and we can use parameter name multiple times
> in @brief, so it is important.

Is this undocumented doxygen feature?:) I failed to find out any description
of '@a' command except for the one I posted above. Or this is purely Tarantool's
codestyle convention?	

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Vladislav Shpilevoy]

On 29/01/2020 23:32, Nikita Pettik wrote:
> On 29 Jan 22:56, Vladislav Shpilevoy wrote:
>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>> index bc50ecbfa..58a6a8a6b 100644
>>>> --- a/src/box/sql/build.c
>>>> +++ b/src/box/sql/build.c
>>>> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>>>>  	sql_table_delete_from(parse, src_list, where);
>>>>  }
>>>>  
>>>> +/**
>>>> + * Generate VDBE program to remove entry with @a index_id and @a
>>>> + * space_id from _index space.
>>>
>>> Is @a common shortcat for @param? If so, please share a link to docs,
>>> since I've managed to find only this:
>>> '''
>>> \a <word>
>>> Displays the argument in italics. Use this command to emphasize words.
>>> Use this command to refer to member arguments in the running text.
>>> '''
>>
>> We use @param when describe parameters. In @brief sections we use @a
>> intentionally. Probably because
>>
>> 1) @param is used to attach a description to a paramter, and for
>> nothing else: http://www.doxygen.nl/manual/commands.html#cmdparam
>>
>> 2) @a is just shorter, and we can use parameter name multiple times
>> in @brief, so it is important.
> 
> Is this undocumented doxygen feature?:) I failed to find out any description
> of '@a' command except for the one I posted above. Or this is purely Tarantool's
> codestyle convention?	

Why undocumented? It is documented, and you've found a correct description.
@a is just an italic font for a next word. It is not for parameter names
only, and is not related to @param. We use it so as parameter names in
function descriptions would be highlighted somehow. Don't know why exactly
@a was chosen instead of, for example, @b (bold) or @c (typewriter font).

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Nikita Pettik]

On 30 Jan 00:45, Vladislav Shpilevoy wrote:
> 
> 
> On 29/01/2020 23:32, Nikita Pettik wrote:
> > On 29 Jan 22:56, Vladislav Shpilevoy wrote:
> >>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> >>>> index bc50ecbfa..58a6a8a6b 100644
> >>>> --- a/src/box/sql/build.c
> >>>> +++ b/src/box/sql/build.c
> >>>> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> >>>>  	sql_table_delete_from(parse, src_list, where);
> >>>>  }
> >>>>  
> >>>> +/**
> >>>> + * Generate VDBE program to remove entry with @a index_id and @a
> >>>> + * space_id from _index space.
> >>>
> >>> Is @a common shortcat for @param? If so, please share a link to docs,
> >>> since I've managed to find only this:
> >>> '''
> >>> \a <word>
> >>> Displays the argument in italics. Use this command to emphasize words.
> >>> Use this command to refer to member arguments in the running text.
> >>> '''
> >>
> >> We use @param when describe parameters. In @brief sections we use @a
> >> intentionally. Probably because
> >>
> >> 1) @param is used to attach a description to a paramter, and for
> >> nothing else: http://www.doxygen.nl/manual/commands.html#cmdparam
> >>
> >> 2) @a is just shorter, and we can use parameter name multiple times
> >> in @brief, so it is important.
> > 
> > Is this undocumented doxygen feature?:) I failed to find out any description
> > of '@a' command except for the one I posted above. Or this is purely Tarantool's
> > codestyle convention?	
> 
> Why undocumented? It is documented, and you've found a correct description.
> @a is just an italic font for a next word. It is not for parameter names
> only, and is not related to @param. We use it so as parameter names in
> function descriptions would be highlighted somehow. Don't know why exactly
> @a was chosen instead of, for example, @b (bold) or @c (typewriter font).

At least our docs say nothing about this convention:

https://www.tarantool.io/en/doc/2.2/dev_guide/c_style_guide/#commenting-style

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Vladislav Shpilevoy]

On 30/01/2020 12:12, Nikita Pettik wrote:
> On 30 Jan 00:45, Vladislav Shpilevoy wrote:
>>
>>
>> On 29/01/2020 23:32, Nikita Pettik wrote:
>>> On 29 Jan 22:56, Vladislav Shpilevoy wrote:
>>>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>>>> index bc50ecbfa..58a6a8a6b 100644
>>>>>> --- a/src/box/sql/build.c
>>>>>> +++ b/src/box/sql/build.c
>>>>>> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>>>>>>  	sql_table_delete_from(parse, src_list, where);
>>>>>>  }
>>>>>>  
>>>>>> +/**
>>>>>> + * Generate VDBE program to remove entry with @a index_id and @a
>>>>>> + * space_id from _index space.
>>>>>
>>>>> Is @a common shortcat for @param? If so, please share a link to docs,
>>>>> since I've managed to find only this:
>>>>> '''
>>>>> \a <word>
>>>>> Displays the argument in italics. Use this command to emphasize words.
>>>>> Use this command to refer to member arguments in the running text.
>>>>> '''
>>>>
>>>> We use @param when describe parameters. In @brief sections we use @a
>>>> intentionally. Probably because
>>>>
>>>> 1) @param is used to attach a description to a paramter, and for
>>>> nothing else: http://www.doxygen.nl/manual/commands.html#cmdparam
>>>>
>>>> 2) @a is just shorter, and we can use parameter name multiple times
>>>> in @brief, so it is important.
>>>
>>> Is this undocumented doxygen feature?:) I failed to find out any description
>>> of '@a' command except for the one I posted above. Or this is purely Tarantool's
>>> codestyle convention?	
>>
>> Why undocumented? It is documented, and you've found a correct description.
>> @a is just an italic font for a next word. It is not for parameter names
>> only, and is not related to @param. We use it so as parameter names in
>> function descriptions would be highlighted somehow. Don't know why exactly
>> @a was chosen instead of, for example, @b (bold) or @c (typewriter font).
> 
> At least our docs say nothing about this convention:
> 
> https://www.tarantool.io/en/doc/2.2/dev_guide/c_style_guide/#commenting-style

Our docs don't say many things, but it does not mean they don't
exist. We use @a throughout all our code base. If you want, you
can file a doc ticket on that.

Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code

[Roman Khabibov]

> On Jan 28, 2020, at 20:44, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On 24 Jan 17:21, Roman Khabibov wrote:
>> Wrap index drop opcode emitting into single function.
>> 
>> Part of #4120
>> ---
>> src/box/sql/build.c | 52 ++++++++++++++++++++++++---------------------
>> 1 file changed, 28 insertions(+), 24 deletions(-)
>> 
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index bc50ecbfa..58a6a8a6b 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1469,6 +1469,26 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>> 	sql_table_delete_from(parse, src_list, where);
>> }
>> 
>> +/**
>> + * Generate VDBE program to remove entry with @a index_id and @a
>> + * space_id from _index space.
> 
> Is @a common shortcat for @param? If so, please share a link to docs,
> since I've managed to find only this:
> '''
> \a <word>
> Displays the argument in italics. Use this command to emphasize words.
> Use this command to refer to member arguments in the running text.
> '''
> 
>> + * Note: index_id_reg have to be allocated immediately after
>> + * space_id_reg.
>> + */
> 
> Nit: have -> has.
> 
> Please, add an assertion verifing that.
> 
>> +static void
>> +vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id,
>> +		     uint32_t index_id_reg, uint32_t space_id_reg)
> 
> I'd also consider moving storing of index_id to register out of
> this function. Then you'll need only space_id_reg to process
> deletion.
I decided to drop this patch, because the function will be too small
and it will not reduce the code. So, IMO, it is pointless then.

>> +{
>> +	struct Vdbe *v = sqlGetVdbe(parse_context);
>> +	assert(v != NULL);
>> +	int record_reg = ++parse_context->nMem;
>> +	sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg);
>> +	sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg);
>> +	sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg);
>> +	VdbeComment((v, "Remove index iid =  %u", index_id));
>> +}
>> +
>> /**
>>  * Generate VDBE program to remove entry from _fk_constraint space.
>>  *