[tarantool-patches] Re: [PATCH v2 0/7] sql: store regular identifiers in case-normal form
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Mar 18 22:33:58 MSK 2019
I see, that you've added a new patch, but decided not to
send it. I've done that. Below you can find my 2 comments on
it.
> commit 47f6bd7a079c8a5af73abe88d9abc24e4bf95f75
> Author: Kirill Shcherbatov <kshcherbatov at tarantool.org>
> Date: Mon Mar 11 14:33:57 2019 +0300
>
> sql: rework sqlIdListAppend to set diag
>
> Refactored sqlIdListAppend routine as sql_id_list_append and
> reworked it to use diag_set in case of memory allocation error.
> This change is necessary because the sql_id_list_append body has
> a sqlNameFromToken call that will be changed in subsequent
> patches.
>
> This patch refers to a series of preparatory patches that provide
> the use of Tarantool errors in the call tree that includes
> sqlNormalizeName, since this call can later return errors.
>
> This patch is not self-sufficient, its sqlNameFromToken call
> remained to be non-Tarantool (for now). It means, that if
> sqlNameFromToken fails in sql_id_list_append there is no
> diag message created.
>
> Part of #3931
>
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 122fda2f0..8a8f1b82f 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -797,10 +797,17 @@ insert_cmd(A) ::= REPLACE. {A = ON_CONFLICT_ACTION_REPLACE;}
>
> idlist_opt(A) ::= . {A = 0;}
> idlist_opt(A) ::= LP idlist(X) RP. {A = X;}
> -idlist(A) ::= idlist(A) COMMA nm(Y).
> - {A = sqlIdListAppend(pParse->db,A,&Y);}
> -idlist(A) ::= nm(Y).
> - {A = sqlIdListAppend(pParse->db,0,&Y); /*A-overwrites-Y*/}
> +idlist(A) ::= idlist(A) COMMA nm(Y). {
> + A = sql_id_list_append(pParse->db,A,&Y);
> + if (A == NULL)
> + sql_parser_error(pParse);
> +}
> +idlist(A) ::= nm(Y). {
> + /* A-overwrites-Y. */
> + A = sql_id_list_append(pParse->db,0,&Y);
> + if (A == NULL)
> + sql_parser_error(pParse);
1. Why do not you return after an error?
> +}
>
> /////////////////////////// Expression Processing /////////////////////////////
> //
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 2df1830d6..56ae90a95 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3388,7 +3388,20 @@ sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
> void sqlInsert(Parse *, SrcList *, Select *, IdList *,
> enum on_conflict_action);
> void *sqlArrayAllocate(sql *, void *, int, int *, int *);
> -IdList *sqlIdListAppend(sql *, IdList *, Token *);
> +
> +/**
> + * Append a new element to the given IdList. Create a new IdList
> + * if need be.
> + *
> + * @param db The database connection.
> + * @param list The pointer to existent Id list if exists.
> + * @param name_token The token containing name.
> + * @retval Not NULL IdList pointer is returned on success.
> + * @retval NULL otherwise. Diag message is set.
> + */
> +struct IdList *
> +sql_id_list_append(struct sql *db, struct IdList *pList, struct Token *pToken);
2. The arg names are old, and do not match the implementation
and the comment.
> +
> int sqlIdListIndex(IdList *, const char *);
>
> /**
More information about the Tarantool-patches
mailing list