* [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