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

[Vladislav Shpilevoy]

See 4 comments below.

On 27/02/2019 14:13, Kirill Shcherbatov wrote:
> Refactored sql_src_list_append routine to use diag_set in case
> of memory allocation error.
> This change is necessary because the sql_src_list_append body has
> a sqlNameFromToken call that will be changed in subsequent
> patches.
> 
> Needed for #3931
> ---
>   src/box/sql/build.c         | 83 +++++++++++--------------------------
>   src/box/sql/delete.c        | 14 ++++---
>   src/box/sql/fk_constraint.c | 17 ++++----
>   src/box/sql/parse.y         | 24 ++++++++---
>   src/box/sql/select.c        |  8 ++--
>   src/box/sql/sqlInt.h        | 48 ++++++++++++++++++++-
>   src/box/sql/trigger.c       | 11 ++---
>   7 files changed, 119 insertions(+), 86 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 5337df450..4fe838608 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2654,64 +2654,28 @@ sql_alloc_src_list(struct sql *db)
>   	return src_list;
>   }
>   
> -/*
> - * Append a new table name to the given SrcList.  Create a new SrcList if
> - * need be.  A new entry is created in the SrcList even if pTable is NULL.
> - *
> - * A SrcList is returned, or NULL if there is an OOM error.  The returned
> - * SrcList might be the same as the SrcList that was input or it might be
> - * a new one.  If an OOM error does occurs, then the prior value of pList
> - * that is input to this routine is automatically freed.
> - *
> - * If pDatabase is not null, it means that the table has an optional
> - * database name prefix.  Like this:  "database.table".  The pDatabase
> - * points to the table name and the pTable points to the database name.
> - * The SrcList.a[].zName field is filled with the table name which might
> - * come from pTable (if pDatabase is NULL) or from pDatabase.
> - * SrcList.a[].zDatabase is filled with the database name from pTable,
> - * or with NULL if no database is specified.
> - *
> - * In other words, if call like this:
> - *
> - *         sqlSrcListAppend(D,A,B,0);
> - *
> - * Then B is a table name and the database name is unspecified.  If called
> - * like this:
> - *
> - *         sqlSrcListAppend(D,A,B,C);
> - *
> - * Then C is the table name and B is the database name.  If C is defined
> - * then so is B.  In other words, we never have a case where:
> - *
> - *         sqlSrcListAppend(D,A,0,C);
> - *
> - * Both pTable and pDatabase are assumed to be quoted.  They are dequoted
> - * before being added to the SrcList.
> - */
> -SrcList *
> -sqlSrcListAppend(sql * db,	/* Connection to notify of malloc failures */
> -		     SrcList * pList,	/* Append to this SrcList. NULL creates a new SrcList */
> -		     Token * pTable	/* Table to append */
> -    )
> +struct SrcList *
> +sql_src_list_append(struct sql *db, struct SrcList *list,
> +		    struct Token *name_token)
>   {
>   	struct SrcList_item *pItem;
> -	assert(db != 0);
> -	if (pList == 0) {
> -		pList = sql_alloc_src_list(db);
> -		if (pList == 0)
> -			return 0;
> +	if (list == NULL) {
> +		list = sql_alloc_src_list(db);
> +		if (list == NULL)
> +			return NULL;
>   	} else {
>   		struct SrcList *new_list =
> -			sql_src_list_enlarge(db, pList, 1, pList->nSrc);
> -		if (new_list == NULL) {
> -			sqlSrcListDelete(db, pList);
> -			return NULL;
> -		}
> -		pList = new_list;
> -	}
> -	pItem = &pList->a[pList->nSrc - 1];
> -	pItem->zName = sqlNameFromToken(db, pTable);
> -	return pList;
> +			sql_src_list_enlarge(db, list, 1, list->nSrc);
> +		if (new_list == NULL)
> +			goto error;

1. When you have a single goto error, usually you can
just inline it. Besides, you do not goto error above on
sql_alloc_src_list() fail. I understand why, but then it
is not a common error.

In other words, just inline error processing here.

> +		list = new_list;
> +	}
> +	pItem = &list->a[list->nSrc - 1];
> +	pItem->zName = sqlNameFromToken(db, name_token);

2. sqlNameFromToken can fail even now, before next patches,
and here it does not set diag, as I see, but the commit
message says, that sql_src_list_append is now on diag.

> +	return list;
> +error:
> +	sqlSrcListDelete(db, list);
> +	return NULL;
>   }
>   
>   /*
> @@ -2803,10 +2767,10 @@ sqlSrcListAppendFromTerm(Parse * pParse,	/* Parsing context */
>   		    );
>   		goto append_from_error;
>   	}
> -	p = sqlSrcListAppend(db, p, pTable);
> -	if (p == 0 || NEVER(p->nSrc == 0)) {
> -		goto append_from_error;
> -	}
> +	p = sql_src_list_append(db, p, pTable);
> +	if (p == NULL)
> +		goto tnt_error;

3. The same as 1.

> +	assert(p->nSrc != 0);
>   	pItem = &p->a[p->nSrc - 1];
>   	assert(pAlias != 0);
>   	if (pAlias->n) {
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index ea6476931..eae2ec8e8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3462,7 +3462,53 @@ sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots,
>   struct SrcList *
>   sql_alloc_src_list(struct sql *db);
>   
> -SrcList *sqlSrcListAppend(sql *, SrcList *, Token *);
> +/**
> + * Append a new table name to the given list.  Create a new
> + * SrcList if need be. A new entry is created in the list even
> + * if name_token is NULL.
> + *
> + * A SrcList is returned, or NULL if there is an OOM error.
> + * The returned SrcList might be the same as the SrcList that was
> + * input or it might be a new one. If an OOM error does occurs,
> + * then the prior value of list that is input to this routine is
> + * automatically freed.
> + *
> + * If pDatabase is not null, it means that the table has an
> + * optional database name prefix. Like this: "database.table".
> + * The pDatabase points to the table name and the pTable points
> + * to the database name. The SrcList.a[].zName field is filled
> + * with the table name which might come from pTable (if pDatabase
> + * is NULL) or from pDatabase.
> + * SrcList.a[].zDatabase is filled with the database name from
> + * name_token, or with NULL if no database is specified.
> + *
> + * In other words, if call like this:
> + *
> + *         sql_src_list_append(D,A,B,0);
> + *
> + * Then B is a table name and the database name is unspecified.
> + * If called like this:
> + *
> + *         sql_src_list_append(D,A,B,C);
> + *
> + * Then C is the table name and B is the database name.  If C is
> + * defined then so is B.  In other words, we never have a case
> + * where:
> + *
> + *         sql_src_list_append(D,A,0,C);
> + *
> + * Both pTable and pDatabase are assumed to be quoted. They are
> + * dequoted before being added to the SrcList.
> + * @param db The database connection.
> + * @param list Append to this SrcList. NULL creates a new SrcList.
> + * @param name_token Token representing table name.
> + * @retval not NULL SrcList pointer on success.
> + * @retval NULL otherwise.

4.1 The comment is obsolete in terms of variable names. Please,
update it. You have no parameters 'pDatabase', 'pTable'.

4.2. Sentences and capital letters.

> + */
> +struct SrcList *
> +sql_src_list_append(struct sql *db, struct SrcList *list,
> +		    struct Token *name_token);
> +
>   SrcList *sqlSrcListAppendFromTerm(Parse *, SrcList *, Token *,
>   				      Token *, Select *, Expr *, IdList *);
>   void sqlSrcListIndexedBy(Parse *, SrcList *, Token *);