[tarantool-patches] Re: [PATCH v1 26/28] sql: cleanup of legacy memory management system

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the squash. I've found one another part of
SQL memory subsystem that can be removed. Please, consider
my review fixes here and on the branch in a separate commit.

======================================================

diff --git a/src/box/sql/CMakeLists.txt b/src/box/sql/CMakeLists.txt
index f0b2e78de..1f2a6640f 100644
--- a/src/box/sql/CMakeLists.txt
+++ b/src/box/sql/CMakeLists.txt
@@ -31,7 +31,6 @@ add_library(sql STATIC
     date.c
     delete.c
     expr.c
-    fault.c
     fk_constraint.c
     func.c
     global.c
diff --git a/src/box/sql/fault.c b/src/box/sql/fault.c
deleted file mode 100644
index ee3a4ea6d..000000000
--- a/src/box/sql/fault.c
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * Copyright 2010-2017, Tarantool AUTHORS, please see AUTHORS file.
- *
- * Redistribution and use in source and binary forms, with or
- * without modification, are permitted provided that the following
- * conditions are met:
- *
- * 1. Redistributions of source code must retain the above
- *    copyright notice, this list of conditions and the
- *    following disclaimer.
- *
- * 2. Redistributions in binary form must reproduce the above
- *    copyright notice, this list of conditions and the following
- *    disclaimer in the documentation and/or other materials
- *    provided with the distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
- * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
- * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
- * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
- * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-
-/*
- *
- * This file contains code to support the concept of "benign"
- * malloc failures (when the xMalloc() or xRealloc() method of the
- * sql_mem_methods structure fails to allocate a block of memory
- * and returns 0).
- *
- * Most malloc failures are non-benign. After they occur, sql
- * abandons the current operation and returns an error
- * to the user. However, sometimes a fault is not necessarily
- * fatal. For example, if a malloc fails while resizing a hash table, this
- * is completely recoverable simply by not carrying out the resize. The
- * hash table will continue to function normally.  So a malloc failure
- * during a hash table resize is a benign fault.
- */
-
-#include "sqlInt.h"
-
-/*
- * Global variables.
- */
-typedef struct BenignMallocHooks BenignMallocHooks;
-static SQL_WSD struct BenignMallocHooks {
-	void (*xBenignBegin) (void);
-	void (*xBenignEnd) (void);
-} sqlHooks = {
-0, 0};
-
-/* The "wsdHooks" macro will resolve to the appropriate BenignMallocHooks
- * structure.  If writable static data is unsupported on the target,
- * we have to locate the state vector at run-time.  In the more common
- * case where writable static data is supported, wsdHooks can refer directly
- * to the "sqlHooks" state vector declared above.
- */
-#define wsdHooksInit
-#define wsdHooks sqlHooks
-
-/*
- * This (sqlEndBenignMalloc()) is called by sql code to indicate that
- * subsequent malloc failures are benign. A call to sqlEndBenignMalloc()
- * indicates that subsequent malloc failures are non-benign.
- */
-void
-sqlBeginBenignMalloc(void)
-{
-	wsdHooksInit;
-	if (wsdHooks.xBenignBegin) {
-		wsdHooks.xBenignBegin();
-	}
-}
-
-void
-sqlEndBenignMalloc(void)
-{
-	wsdHooksInit;
-	if (wsdHooks.xBenignEnd) {
-		wsdHooks.xBenignEnd();
-	}
-}
-
diff --git a/src/box/sql/hash.c b/src/box/sql/hash.c
index 5bc8109dc..36f965b81 100644
--- a/src/box/sql/hash.c
+++ b/src/box/sql/hash.c
@@ -149,18 +149,7 @@ rehash(Hash * pH, unsigned int new_size)
 	if (new_size == pH->htsize)
 		return 0;
 #endif
-
-	/* The inability to allocates space for a larger hash table is
-	 * a performance hit but it is not a fatal error.  So mark the
-	 * allocation as a benign. Use sqlMalloc()/memset(0) instead of
-	 * sqlMallocZero() to make the allocation, as sqlMallocZero()
-	 * only zeroes the requested number of bytes whereas this module will
-	 * use the actual amount of space allocated for the hash table (which
-	 * may be larger than the requested amount).
-	 */
-	sqlBeginBenignMalloc();
 	new_ht = (struct _ht *)sqlMalloc(new_size * sizeof(struct _ht));
-	sqlEndBenignMalloc();
 
 	if (new_ht == 0)
 		return 0;
diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c
index f19b7d027..d92709b8e 100644
--- a/src/box/sql/malloc.c
+++ b/src/box/sql/malloc.c
@@ -368,17 +368,6 @@ sqlDbStrNDup(sql * db, const char *z, u64 n)
 	return zNew;
 }
 
-/*
- * Call this routine to record the fact that an OOM (out-of-memory) error
- * has happened.  This routine will set db->mallocFailed.
- */
-void
-sqlOomFault(sql * db)
-{
-	if (db->mallocFailed == 0 && db->bBenignMalloc == 0)
-		db->mallocFailed = 1;
-}
-
 /*
  * This routine reactivates the memory allocator and clears the
  * db->mallocFailed flag as necessary.
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2e7bf3363..e4c584333 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1107,7 +1107,6 @@ struct sql {
 	u16 dbOptFlags;		/* Flags to enable/disable optimizations */
 	u8 enc;			/* Text encoding */
 	u8 mallocFailed;	/* True if we have seen a malloc failure */
-	u8 bBenignMalloc;	/* Do not require OOMs if true */
 	u8 dfltLockMode;	/* Default locking-mode for attached dbs */
 	u8 mTrace;		/* zero or more sql_TRACE flags */
 	u32 magic;		/* Magic number for detect library misuse */
@@ -4290,7 +4289,14 @@ int sqlCreateFunc(sql *, const char *, enum field_type,
 		      void (*)(sql_context *, int, sql_value **),
 		      void (*)(sql_context *),
 		      FuncDestructor * pDestructor);
-void sqlOomFault(sql *);
+
+/** Set OOM error flag. */
+static inline void
+sqlOomFault(struct sql *db)
+{
+	db->mallocFailed = 1;
+}
+
 void sqlOomClear(sql *);
 int sqlApiExit(sql * db, int);
 
@@ -4472,13 +4478,6 @@ fk_constraint_emit_actions(struct Parse *parser, struct space *space, int reg_ol
 bool
 fk_constraint_is_required(struct space *space, const int *changes);
 
-/*
- * The interface to the code in fault.c used for identifying "benign"
- * malloc failures.
- */
-void sqlBeginBenignMalloc(void);
-void sqlEndBenignMalloc(void);
-
 /*
  * Allowed return values from sqlFindInIndex()
  */
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 579b2006c..c8887f9b7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -747,7 +747,6 @@ int sqlVdbeExec(Vdbe *p)
 	assert(p->explain==0);
 	p->pResultSet = 0;
 #ifdef SQL_DEBUG
-	sqlBeginBenignMalloc();
 	if (p->pc == 0 &&
 	    (p->sql_flags & (SQL_VdbeListing|SQL_VdbeEQP|SQL_VdbeTrace)) != 0) {
 		int i;
@@ -771,7 +770,6 @@ int sqlVdbeExec(Vdbe *p)
 		if ((p->sql_flags & SQL_VdbeTrace) != 0)
 			printf("VDBE Trace:\n");
 	}
-	sqlEndBenignMalloc();
 #endif
 	for(pOp=&aOp[p->pc]; 1; pOp++) {
 		/* Errors are detected by individual opcodes, with an immediate