[tarantool-patches] Re: [PATCH v2 2/7] sql: rework sql_src_list_enlarge to set diag

[Vladislav Shpilevoy]

In the commit header you said:

"sql: rework sql_src_list_enlarge to set diag"

But before your patch symbol 'sql_src_list_enlarge' does not
exist. Please, fix that, and in other commits, who change
function names, too.

On 27/02/2019 14:13, Kirill Shcherbatov wrote:
> Refactored sql_src_list_enlarge routine to use diag_set in case

You've refactored sqlSrcListEnlarge, not sql_src_list_enlarge.

> of memory allocation error. This will ensure that the
> sqlSrcListAppend function throws an error using diag in
> subsequent patches.
> 
> Needed for #3931

See 4 comments below.

> ---
>   src/box/sql/build.c  | 109 ++++++++++++++++---------------------------
>   src/box/sql/select.c |  28 ++++++-----
>   src/box/sql/sqlInt.h |  27 ++++++++++-
>   3 files changed, 83 insertions(+), 81 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 3a65eb5a1..5337df450 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2597,76 +2597,45 @@ sqlIdListIndex(IdList * pList, const char *zName)
> +struct SrcList *
> +sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots,
> +		     int start_idx)
> +{
> +	assert(start_idx >= 0);
> +	assert(new_slots >= 1);
> +	assert(src_list != NULL);
> +	assert(start_idx <= src_list->nSrc);
> +
> +	/* Allocate additional space if needed. */
> +	if (src_list->nSrc + new_slots > (int)src_list->nAlloc) {
> +		int to_alloc = src_list->nSrc * 2 + new_slots;
> +		int size = sizeof(*src_list) +
> +			   (to_alloc - 1) * sizeof(src_list->a[0]);
> +		src_list = sqlDbRealloc(db, src_list, size);
> +		if (src_list == NULL) {
> +			diag_set(OutOfMemory, size, "sqlDbRealloc", "src_list");
> +			return NULL;
>   		}
> -		pSrc = pNew;
> -		nGot =
> -		    (sqlDbMallocSize(db, pNew) -
> -		     sizeof(*pSrc)) / sizeof(pSrc->a[0]) + 1;
> -		pSrc->nAlloc = nGot;
> +		src_list->nAlloc =
> +			(sqlDbMallocSize(db, src_list) -
> +			sizeof(*src_list)) / sizeof(src_list->a[0]) + 1;;

1. Double semicolon.

2. Not sure if it is worth using sqlDbMallocSize() here. Looks like
double reserve. Firstly, we already increased size twice + new_slots
(look at to_alloc). Secondly, we hope, that sqlDbRealloc will probably
realloc more than necessary, but as I see, sqlDbRealloc will reserve
at most +15 bytes. Please, just use src_list->nAlloc = to_alloc.

>   	}
>   
> -	/* Move existing slots that come after the newly inserted slots
> -	 * out of the way
> +	/*
> +	 * Move existing slots that come after the newly inserted
> +	 * slots out of the way.
>   	 */
> -	for (i = pSrc->nSrc - 1; i >= iStart; i--) {
> -		pSrc->a[i + nExtra] = pSrc->a[i];
> -	}
> -	pSrc->nSrc += nExtra;
> +	for (int i = src_list->nSrc - 1; i >= start_idx; i--)
> +		src_list->a[i + new_slots] = src_list->a[i];

3. It is just memmove. Please use memmove() for that.

> +	src_list->nSrc += new_slots;
>   
> -	/* Zero the newly allocated slots */
> -	memset(&pSrc->a[iStart], 0, sizeof(pSrc->a[0]) * nExtra);
> -	for (i = iStart; i < iStart + nExtra; i++) {
> -		pSrc->a[i].iCursor = -1;
> -	}
> +	/* Zero the newly allocated slots. */
> +	memset(&src_list->a[start_idx], 0, sizeof(src_list->a[0]) * new_slots);
> +	for (int i = start_idx; i < start_idx + new_slots; i++)
> +		src_list->a[i].iCursor = -1;
>   
> -	/* Return a pointer to the enlarged SrcList */
> -	return pSrc;
> +	/* Return a pointer to the enlarged SrcList. */
> +	return src_list;
>   }
>   
>   struct SrcList *
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index ce29dc0b1..ea6476931 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3426,7 +3426,32 @@ void sqlInsert(Parse *, SrcList *, Select *, IdList *,
>   void *sqlArrayAllocate(sql *, void *, int, int *, int *);
>   IdList *sqlIdListAppend(sql *, IdList *, Token *);
>   int sqlIdListIndex(IdList *, const char *);
> -SrcList *sqlSrcListEnlarge(sql *, SrcList *, int, int);
> +
> +/**
> + * Expand the space allocated for the given SrcList object by
> + * creating new_slots new slots beginning at start_idx.
> + * The start_idx is zero based. New slots are zeroed.
> + *
> + * For example, suppose a SrcList initially contains two entries:
> + * A,B.
> + * To append 3 new entries onto the end, do this:
> + *    sql_src_list_enlarge(db, pSrclist, 3, 2);
> + *
> + * After the call above it would contain:  A, B, nil, nil, nil.
> + * If the iStart argument had been 1 instead of 2, then the result

4.1. You have no iStart.

> + * would have been:  A, nil, nil, nil, B.  To prepend the new slots,
> + * the iStart value would be 0.  The result then would

4.2. The same.

> + * be: nil, nil, nil, A, B.
> + * @param db The database connection.
> + * @param src_list The SrcList to be enlarged.
> + * @param new_slots Number of new slots to add to src_list->a[].
> + * @param start_idx Index in pSrc->a[] of first new slot.

4.3. pSrc does not exist.

> + * @retval not NULL SrcList pointer on success.
> + * @retval NULL otherwise.

4.4. Please, start sentences with capital letters.

> + */
> +struct SrcList *
> +sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots,
> +		     int start_idx);
>   
>   /**
>    * Allocate a new empty SrcList object.
> -- 
> 2.20.1
> 
>