[tarantool-patches] Re: [PATCH v2 1/7] sql: refactor sql_alloc_src_list to set diag

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 27/02/2019 14:13, Kirill Shcherbatov wrote:
> Refactored sql_alloc_src_list routine to use diag_set in case of
> memory allocation error. This will ensure that the
> sqlSrcListAppend function throws an error using diag in
> subsequent patches.

The issue was about normalization error, but here I do not
see a word about it. Only an obfuscated necessity to refactor
sqlSrcListAppend. Why do you needed it? How it makes you closer
to the name normalization? I spent significant time to recover
the whole tree of functions, using name normalization, in order
to learn what sqlSrcListAppend has to do with it.

Please, write more accurate and elaborated comments to each
commit in the patchset.

Also, as I see, you decided to go from the bottom and firstly
refactor some helper functions, after which you fixed the
functions who use normalization.

But the thing is that this commit and some of the others
do not work because you set diag, but diag error never
reaches a user.

1) In this commit you refactored sql_alloc_str_list, its
    usage in sqlSrcListAppend remained to be non-tarantool.

    It means, that if sql_alloc_str_list fails in sqlSrcListAppend
    and sets a diag, it is never thrown to a user.

2) About commit 'sql: rework sql_src_list_enlarge to set diag':
    if sql_src_list_enlarge fails in sqlSrcListAppend or in
    src_list_append_unique, the diag will never be thrown.

3) About commit 'sql: refactor sql_name_from_token to set diag':
    if sql_name_from_token fails in sql_id_list_append or
    sqlSrcListIndexedBy - the diag error is lost.

I do not see a way how to gracefully split the patch into
multiple preparation patches, and keep diag working, so I ask
you to just write a comment in the commit messages, listed
above, that they are not self-sufficient.

> 
> Needed for #3931
> ---
>   src/box/sql/build.c  | 30 +++++++++++++++++-------------
>   src/box/sql/sqlInt.h | 12 ++++++++++--
>   2 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index dbfdbc6f5..ce29dc0b1 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3427,8 +3427,16 @@ void *sqlArrayAllocate(sql *, void *, int, int *, int *);
>   IdList *sqlIdListAppend(sql *, IdList *, Token *);
>   int sqlIdListIndex(IdList *, const char *);
>   SrcList *sqlSrcListEnlarge(sql *, SrcList *, int, int);
> -SrcList *
> -sql_alloc_src_list(sql *db);
> +
> +/**
> + * Allocate a new empty SrcList object.
> + * @param db The database connection.
> + * @retval not NULL list pointer on success.
> + * @retval NULL otherwise.

As I've already said - please, use capital letters at
the beginning of a sentence. Also, it is worth mentioning
that it sets diag, and db->mallocFailed means nothing. The
same about all other refactored functions in scope of this
patchset.

> + */
> +struct SrcList *
> +sql_alloc_src_list(struct sql *db);
> +
>   SrcList *sqlSrcListAppend(sql *, SrcList *, Token *);
>   SrcList *sqlSrcListAppendFromTerm(Parse *, SrcList *, Token *,
>   				      Token *, Select *, Expr *, IdList *);
> -- 
> 2.20.1
> 
>