[tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the patch! I have pushed my review fixes
as a separate commit for this patch and for
"sql: refactor trigger API functions".

On 27/06/2018 21:04, Kirill Shcherbatov wrote:
>> Typo: ‘contains’.
>> It is better to say: ‘we can avoid storing AST pointers in HASH’.
> Ok, tnx:
>      sql: remove global sql_trigger hash
>      
>      As new _trigger format contains space_id, we can avoid
>      storing AST pointers in HASH. Requested AST could be found
>      by name in appropriate space.
>      
>      Part of #3273.
> 
>> Personally I would squash this commit with ’sql: move Triggers to server’,
>> but it is up to you.
> I'd like to keep it as is.
> 
>> You can fit index_id and name_src in one line.
> -                                 int index_id,
> -                                 const char *name_src,
> +                                 int index_id,const char *name_src,
> 
>> And add arg names to prototype.
> -void vdbe_code_drop_trigger(Parse *, SrcList *, int);
> +void
> +vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err);
> 
>> I don’t see such flag.
> - * present is raise_if_not_exists flag is set) in specified space.
> + * present - configure with cond_opcodeq) in specified space.
>> Lets drop ‘_ptr’ suffix.
>> Lets call it with ’sql_’ prefix - this function is invoked from parser
>> (see for instance sql_drop_table(), sql_drop_index() etc).
> -vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)
> +sql_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)
> 
> -vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name)
> +vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name)
> 
>> Redundant diff.
> dropped.
>