From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4710F28FA9 for ; Mon, 11 Mar 2019 11:04:48 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id YwDTkVb7KpyF for ; Mon, 11 Mar 2019 11:04:48 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CA36728F8C for ; Mon, 11 Mar 2019 11:04:47 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 2/7] sql: rework sql_src_list_enlarge to set diag References: <73774bba8d68b70c0c6d5529ad30df4875370e87.1551265819.git.kshcherbatov@tarantool.org> <9b814291-6165-b96f-8843-a0d67522ca40@tarantool.org> From: Kirill Shcherbatov Message-ID: <48631d89-858c-5567-63b3-541a044218de@tarantool.org> Date: Mon, 11 Mar 2019 18:04:45 +0300 MIME-Version: 1.0 In-Reply-To: <9b814291-6165-b96f-8843-a0d67522ca40@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Vladislav Shpilevoy > 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