* [Tarantool-patches] [PATCH 0/2] Support <ALTER TABLE DROP CONSTRAINT>
@ 2020-01-24 14:21 Roman Khabibov
2020-01-24 14:21 ` [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code Roman Khabibov
0 siblings, 1 reply; 9+ messages in thread
From: Roman Khabibov @ 2020-01-24 14:21 UTC (permalink / raw)
To: tarantool-patches
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-24 14:21 [Tarantool-patches] [PATCH 0/2] Support <ALTER TABLE DROP CONSTRAINT> Roman Khabibov
@ 2020-01-24 14:21 ` Roman Khabibov
2020-01-28 17:44 ` Nikita Pettik
0 siblings, 1 reply; 9+ messages in thread
From: Roman Khabibov @ 2020-01-24 14:21 UTC (permalink / raw)
To: tarantool-patches
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-24 14:21 ` [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code Roman Khabibov
@ 2020-01-28 17:44 ` Nikita Pettik
2020-01-29 21:56 ` Vladislav Shpilevoy
2020-02-01 17:40 ` Roman Khabibov
0 siblings, 2 replies; 9+ messages in thread
From: Nikita Pettik @ 2020-01-28 17:44 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
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.
> *
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-28 17:44 ` Nikita Pettik
@ 2020-01-29 21:56 ` Vladislav Shpilevoy
2020-01-29 22:32 ` Nikita Pettik
2020-02-01 17:40 ` Roman Khabibov
1 sibling, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-01-29 21:56 UTC (permalink / raw)
To: Nikita Pettik, Roman Khabibov; +Cc: tarantool-patches
>> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-29 21:56 ` Vladislav Shpilevoy
@ 2020-01-29 22:32 ` Nikita Pettik
2020-01-29 23:45 ` Vladislav Shpilevoy
0 siblings, 1 reply; 9+ messages in thread
From: Nikita Pettik @ 2020-01-29 22:32 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-29 22:32 ` Nikita Pettik
@ 2020-01-29 23:45 ` Vladislav Shpilevoy
2020-01-30 11:12 ` Nikita Pettik
0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-01-29 23:45 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
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).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-29 23:45 ` Vladislav Shpilevoy
@ 2020-01-30 11:12 ` Nikita Pettik
2020-01-30 21:05 ` Vladislav Shpilevoy
0 siblings, 1 reply; 9+ messages in thread
From: Nikita Pettik @ 2020-01-30 11:12 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-30 11:12 ` Nikita Pettik
@ 2020-01-30 21:05 ` Vladislav Shpilevoy
0 siblings, 0 replies; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-01-30 21:05 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code
2020-01-28 17:44 ` Nikita Pettik
2020-01-29 21:56 ` Vladislav Shpilevoy
@ 2020-02-01 17:40 ` Roman Khabibov
1 sibling, 0 replies; 9+ messages in thread
From: Roman Khabibov @ 2020-02-01 17:40 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
> 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.
>> *
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-01 17:40 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 14:21 [Tarantool-patches] [PATCH 0/2] Support <ALTER TABLE DROP CONSTRAINT> Roman Khabibov
2020-01-24 14:21 ` [Tarantool-patches] [PATCH 1/2] sql: refactor index drop code Roman Khabibov
2020-01-28 17:44 ` Nikita Pettik
2020-01-29 21:56 ` Vladislav Shpilevoy
2020-01-29 22:32 ` Nikita Pettik
2020-01-29 23:45 ` Vladislav Shpilevoy
2020-01-30 11:12 ` Nikita Pettik
2020-01-30 21:05 ` Vladislav Shpilevoy
2020-02-01 17:40 ` Roman Khabibov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox