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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> You've refactored sqlSrcListEnlarge, not sql_src_list_enlarge.
I've reword all commit messages including this one.

> 1. Double semicolon.

Fixed

> 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.

- nGot =
- (sqlDbMallocSize(db, pNew) -
- sizeof(*pSrc)) / sizeof(pSrc->a[0]) + 1;
- pSrc->nAlloc = nGot;
+ src_list->nAlloc = to_alloc;

> 3. It is just memmove. Please use memmove() for that.
- for (i = pSrc->nSrc - 1; i >= iStart; i--) {
- pSrc->a[i + nExtra] = pSrc->a[i];
- }
- pSrc->nSrc += nExtra;
+ memmove(&src_list->a[start_idx + new_slots], &src_list->a[start_idx],
+ (src_list->nSrc - start_idx) * sizeof(src_list->a[0]));
+ src_list->nSrc += new_slots;

> 4.1. You have no iStart.
> 4.2. The same.
> 4.3. pSrc does not exist.
> 4.4. Please, start sentences with capital letters.
Hope I've fixed them all.
=====================================================

Refactored sqlSrcListEnlarge routine as sql_src_list_enlarge and
reworked 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.

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 usage in sqlSrcListAppend
remained to be non-Tarantool (for now). It means, that if
sql_src_list_enlarge fails in sqlSrcListAppend the diag will
never be thrown.

Part of #3931
---
 src/box/sql/build.c  | 107 +++++++++++++++----------------------------
 src/box/sql/select.c |  18 +++++---
 src/box/sql/sqlInt.h |  28 ++++++++++-
 3 files changed, 77 insertions(+), 76 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 760054552..f53b2561c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2597,76 +2597,43 @@ sqlIdListIndex(IdList * pList, const char *zName)
 	return -1;
 }
 
-/*
- * Expand the space allocazted for the given SrcList object by
- * creating nExtra new slots beginning at iStart.  iStart 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:
- *
- *    sqlSrcListEnlarge(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
- * would have been:  A, nil, nil, nil, B.  To prepend the new slots,
- * the iStart value would be 0.  The result then would
- * be: nil, nil, nil, A, B.
- *
- * If a memory allocation fails the SrcList is unchanged.  The
- * db->mallocFailed flag will be set to true.
- */
-SrcList *
-sqlSrcListEnlarge(sql * db,	/* Database connection to notify of OOM errors */
-		      SrcList * pSrc,	/* The SrcList to be enlarged */
-		      int nExtra,	/* Number of new slots to add to pSrc->a[] */
-		      int iStart	/* Index in pSrc->a[] of first new slot */
-    )
-{
-	int i;
-
-	/* Sanity checking on calling parameters */
-	assert(iStart >= 0);
-	assert(nExtra >= 1);
-	assert(pSrc != 0);
-	assert(iStart <= pSrc->nSrc);
-
-	/* Allocate additional space if needed */
-	if ((u32) pSrc->nSrc + nExtra > pSrc->nAlloc) {
-		SrcList *pNew;
-		int nAlloc = pSrc->nSrc * 2 + nExtra;
-		int nGot;
-		pNew = sqlDbRealloc(db, pSrc,
-					sizeof(*pSrc) + (nAlloc -
-							 1) *
-					sizeof(pSrc->a[0]));
-		if (pNew == 0) {
-			assert(db->mallocFailed);
-			return pSrc;
+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 = 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;
+	memmove(&src_list->a[start_idx + new_slots], &src_list->a[start_idx],
+		(src_list->nSrc - start_idx) * sizeof(src_list->a[0]));
+	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 *
@@ -2732,11 +2699,13 @@ sqlSrcListAppend(sql * db,	/* Connection to notify of malloc failures */
 		if (pList == 0)
 			return 0;
 	} else {
-		pList = sqlSrcListEnlarge(db, pList, 1, pList->nSrc);
-	}
-	if (db->mallocFailed) {
-		sqlSrcListDelete(db, pList);
-		return 0;
+		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);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 16d535076..840bca556 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -279,14 +279,18 @@ src_list_append_unique(struct sql *db, struct SrcList *list,
 		if (name != NULL && strcmp(new_name, name) == 0)
 			return list;
 	}
-	list = sqlSrcListEnlarge(db, list, 1, list->nSrc);
-	if (db->mallocFailed) {
+	struct SrcList *new_list =
+		sql_src_list_enlarge(db, list, 1, list->nSrc);
+	if (new_list == NULL) {
 		sqlSrcListDelete(db, list);
 		return NULL;
 	}
+	list = new_list;
 	struct SrcList_item *pItem = &list->a[list->nSrc - 1];
 	pItem->zName = sqlDbStrNDup(db, new_name, strlen(new_name));
 	if (pItem->zName == NULL) {
+		diag_set(OutOfMemory, strlen(new_name), "sqlDbStrNDup",
+			 "pItem->zName");
 		sqlSrcListDelete(db, list);
 		return NULL;
 	}
@@ -4135,12 +4139,14 @@ flattenSubquery(Parse * pParse,		/* Parsing context */
 		 * for the two elements in the FROM clause of the subquery.
 		 */
 		if (nSubSrc > 1) {
-			pParent->pSrc = pSrc =
-			    sqlSrcListEnlarge(db, pSrc, nSubSrc - 1,
-						  iFrom + 1);
-			if (db->mallocFailed) {
+			struct SrcList *new_list =
+				sql_src_list_enlarge(db, pSrc, nSubSrc - 1,
+						     iFrom + 1);
+			if (new_list == NULL) {
+				sql_parser_error(pParse);
 				break;
 			}
+			pParent->pSrc = pSrc = new_list;
 		}
 
 		/* Transfer the FROM clause terms from the subquery into the
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4a4a748ba..0895f7de1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3390,7 +3390,33 @@ 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, src_list, 3, 2);
+ *
+ * After the call above it would contain:  A, B, nil, nil, nil.
+ * If the start_idx argument had been 1 instead of 2, then the
+ * result would have been:  A, nil, nil, nil, B.  To prepend the
+ * new slots, the start_idx value would be 0. The result then
+ * would 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 src_list->a[] of first new slot.
+ * @retval Not NULL SrcList pointer on success.
+ * @retval NULL Otherwise. The diag message is set.
+ */
+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.21.0