[tarantool-patches] Re: [PATCH v1 04/28] sql: remove sqlError() and remove sqlErrorWithMsg()

[Mergen Imeev <imeevma@tarantool.org>]

On Fri, Jun 14, 2019 at 12:25:06AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Consider my review fixes below and on the branch
> in a separate commit.
> 

Thank you! New patch:

From 0199a43b36b51c43333e47fa240328072741214c Mon Sep 17 00:00:00 2001
Date: Sat, 27 Apr 2019 15:18:15 +0300
Subject: [PATCH] sql: remove sqlError() and remove sqlErrorWithMsg()

These functions are part of the SQL error system, which is
replaced by the Tarantool error system. They are not needed now,
so they will be deleted.

diff --git a/src/box/lua/lua_sql.c b/src/box/lua/lua_sql.c
index 529ee59..4d90a53 100644
--- a/src/box/lua/lua_sql.c
+++ b/src/box/lua/lua_sql.c
@@ -199,6 +199,6 @@ lbox_sql_create_function(struct lua_State *L)
 					   func_info, lua_sql_call, NULL, NULL,
 					   lua_sql_destroy);
 	if (rc != 0)
-		return luaL_error(L, sqlErrStr(rc));
+		return luaT_error(L);
 	return 0;
 }
diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
index a57677e..d519991 100644
--- a/src/box/sql/legacy.c
+++ b/src/box/sql/legacy.c
@@ -69,7 +69,6 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
 	if (zSql == 0)
 		zSql = "";
 
-	sqlError(db, SQL_OK);
 	while (rc == SQL_OK && zSql[0]) {
 		int nCol;
 		char **azVals = 0;
@@ -141,7 +140,6 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
 					rc = SQL_ABORT;
 					sqlVdbeFinalize((Vdbe *) pStmt);
 					pStmt = 0;
-					sqlError(db, SQL_ABORT);
 					goto exec_out;
 				}
 			}
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 17ddf5a..0811b2e 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -272,61 +272,6 @@ functionDestroy(sql * db, FuncDef * p)
 }
 
 /*
- * Return TRUE if database connection db has unfinalized prepared
- * statement.
- */
-static int
-connectionIsBusy(sql * db)
-{
-	if (db->pVdbe)
-		return 1;
-	return 0;
-}
-
-/*
- * Close an existing sql database
- */
-static int
-sqlClose(sql * db, int forceZombie)
-{
-	assert(db);
-	if (!sqlSafetyCheckSickOrOk(db)) {
-		return SQL_MISUSE;
-	}
-	if (db->mTrace & SQL_TRACE_CLOSE) {
-		db->xTrace(SQL_TRACE_CLOSE, db->pTraceArg, db, 0);
-	}
-
-	/* Legacy behavior (sql_close() behavior) is to return
-	 * SQL_BUSY if the connection can not be closed immediately.
-	 */
-	if (!forceZombie && connectionIsBusy(db)) {
-		sqlErrorWithMsg(db, SQL_BUSY,
-				    "unable to close due to unfinalized "
-				    "statements");
-		return SQL_BUSY;
-	}
-
-	/* Convert the connection into a zombie and then close it.
-	 */
-	db->magic = SQL_MAGIC_ZOMBIE;
-
-	return SQL_OK;
-}
-
-/*
- * Two variations on the public interface for closing a database
- * connection. The sql_close() version returns SQL_BUSY and
- * leaves the connection option if there are unfinalized prepared
- * statements.
- */
-int
-sql_close(sql * db)
-{
-	return sqlClose(db, 0);
-}
-
-/*
  * Rollback all database files.  If tripCode is not SQL_OK, then
  * any write cursors are invalidated ("tripped" - as in "tripping a circuit
  * breaker") and made to return tripCode if there are any further
@@ -405,7 +350,9 @@ sqlCreateFunc(sql * db,
 	    (!xSFunc && (!xFinal && xStep)) ||
 	    (nArg < -1 || nArg > SQL_MAX_FUNCTION_ARG) ||
 	    (255 < (sqlStrlen30(zFunctionName)))) {
-		return SQL_MISUSE;
+		diag_set(ClientError, ER_CREATE_FUNCTION, zFunctionName,
+			 "wrong function definition");
+		return SQL_TARANTOOL_ERROR;
 	}
 
 	assert(SQL_FUNC_CONSTANT == SQL_DETERMINISTIC);
@@ -420,10 +367,10 @@ sqlCreateFunc(sql * db,
 	p = sqlFindFunction(db, zFunctionName, nArg, 0);
 	if (p && p->nArg == nArg) {
 		if (db->nVdbeActive) {
-			sqlErrorWithMsg(db, SQL_BUSY,
-					    "unable to delete/modify user-function due to active statements");
-			assert(!db->mallocFailed);
-			return SQL_BUSY;
+			diag_set(ClientError, ER_CREATE_FUNCTION, zFunctionName,
+				 "unable to create function due to active "\
+				 "statements");
+			return SQL_TARANTOOL_ERROR;
 		} else {
 			sqlExpirePreparedStatements(db);
 		}
@@ -431,9 +378,8 @@ sqlCreateFunc(sql * db,
 
 	p = sqlFindFunction(db, zFunctionName, nArg, 1);
 	assert(p || db->mallocFailed);
-	if (!p) {
-		return SQL_NOMEM;
-	}
+	if (p == NULL)
+		return SQL_TARANTOOL_ERROR;
 
 	/* If an older version of the function with a configured destructor is
 	 * being replaced invoke the destructor function here.
@@ -637,22 +583,17 @@ sql_init_db(sql **out_db)
 	 * database schema yet. This is delayed until the first time the database
 	 * is accessed.
 	 */
-	sqlError(db, SQL_OK);
 	sqlRegisterPerConnectionBuiltinFunctions(db);
 
-	if (rc)
-		sqlError(db, rc);
-
 	/* Enable the lookaside-malloc subsystem */
 	setupLookaside(db, 0, sqlGlobalConfig.szLookaside,
 		       sqlGlobalConfig.nLookaside);
 
 opendb_out:
 	assert(db != 0 || rc == SQL_NOMEM);
-	if (rc == SQL_NOMEM) {
-		sql_close(db);
-		db = 0;
-	} else if (rc != SQL_OK)
+	if (rc == SQL_NOMEM)
+		db = NULL;
+	else if (rc != SQL_OK)
 		db->magic = SQL_MAGIC_SICK;
 
 	*out_db = db;
diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c
index 45667fe..5df4606 100644
--- a/src/box/sql/malloc.c
+++ b/src/box/sql/malloc.c
@@ -756,7 +756,6 @@ static SQL_NOINLINE int
 apiOomError(sql * db)
 {
 	sqlOomClear(db);
-	sqlError(db, SQL_NOMEM);
 	return SQL_NOMEM;
 }
 
diff --git a/src/box/sql/os.c b/src/box/sql/os.c
index eb7450b..a060675 100644
--- a/src/box/sql/os.c
+++ b/src/box/sql/os.c
@@ -129,12 +129,6 @@ sqlOsRandomness(sql_vfs * pVfs, int nByte, char *zBufOut)
 }
 
 int
-sqlOsGetLastError(sql_vfs * pVfs)
-{
-	return pVfs->xGetLastError ? pVfs->xGetLastError(pVfs, 0, 0) : 0;
-}
-
-int
 sqlOsCurrentTimeInt64(sql_vfs * pVfs, sql_int64 * pTimeOut)
 {
 	int rc;
diff --git a/src/box/sql/os.h b/src/box/sql/os.h
index 1514f0d..9122e9c 100644
--- a/src/box/sql/os.h
+++ b/src/box/sql/os.h
@@ -146,7 +146,6 @@ int sqlOsUnfetch(sql_file *, i64, void *);
  */
 int sqlOsOpen(sql_vfs *, const char *, sql_file *, int, int *);
 int sqlOsRandomness(sql_vfs *, int, char *);
-int sqlOsGetLastError(sql_vfs *);
 int sqlOsCurrentTimeInt64(sql_vfs *, sql_int64 *);
 
 /*
diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
index f9969b4..e0a2805 100644
--- a/src/box/sql/os_unix.c
+++ b/src/box/sql/os_unix.c
@@ -2017,21 +2017,6 @@ unixCurrentTimeInt64(sql_vfs * NotUsed, sql_int64 * piNow)
 }
 
 /*
- * The xGetLastError() method is designed to return a better
- * low-level error message when operating-system problems come up
- * during sql operation.  Only the integer return code is currently
- * used.
- */
-static int
-unixGetLastError(sql_vfs * NotUsed, int NotUsed2, char *NotUsed3)
-{
-	UNUSED_PARAMETER(NotUsed);
-	UNUSED_PARAMETER(NotUsed2);
-	UNUSED_PARAMETER(NotUsed3);
-	return errno;
-}
-
-/*
  *********************** End of sql_vfs methods ***************************
  *****************************************************************************/
 
@@ -2055,7 +2040,6 @@ unixGetLastError(sql_vfs * NotUsed, int NotUsed2, char *NotUsed3)
     unixDelete,           /* xDelete */                     \
     unixRandomness,       /* xRandomness */                 \
     NULL,                 /* xCurrentTime */                \
-    unixGetLastError,     /* xGetLastError */               \
     unixCurrentTimeInt64, /* xCurrentTimeInt64 */           \
   }
 
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e2f8c0b..e692be0 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -81,9 +81,9 @@ sqlPrepare(sql * db,	/* Database handle. */
 		testcase(nBytes == mxLen);
 		testcase(nBytes == mxLen + 1);
 		if (nBytes > mxLen) {
-			sqlErrorWithMsg(db, SQL_TOOBIG,
-					    "statement too long");
-			rc = sqlApiExit(db, SQL_TOOBIG);
+			diag_set(ClientError, ER_SQL_PARSER_LIMIT,
+				 "SQL command length", nBytes, mxLen);
+			rc = SQL_TARANTOOL_ERROR;
 			goto end_prepare;
 		}
 		zSqlCopy = sqlDbStrNDup(db, zSql, nBytes);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4862794..7051aa9 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -304,7 +304,6 @@ struct sql_vfs {
 	int (*xDelete) (sql_vfs *, const char *zName, int syncDir);
 	int (*xRandomness) (sql_vfs *, int nByte, char *zOut);
 	int (*xCurrentTime) (sql_vfs *, double *);
-	int (*xGetLastError) (sql_vfs *, int, char *);
 	/*
 	** The methods above are in version 1 of the sql_vfs object
 	** definition.  Those that follow are added in version 2 or later
@@ -816,8 +815,6 @@ sql_stmt_busy(sql_stmt *);
 int
 sql_init_db(sql **db);
 
-int
-sql_close(sql *);
 
 /**
  * Get number of the named parameter in the prepared sql
@@ -1258,7 +1255,6 @@ struct sql {
 	struct coll *pDfltColl;	/* The default collating sequence (BINARY) */
 	i64 szMmap;		/* Default mmap_size setting */
 	int errMask;		/* & result codes with this before returning */
-	int iSysErrno;		/* Errno value from last system error */
 	u16 dbOptFlags;		/* Flags to enable/disable optimizations */
 	u8 enc;			/* Text encoding */
 	u8 mallocFailed;	/* True if we have seen a malloc failure */
@@ -4247,9 +4243,6 @@ sql_atoi64(const char *z, int64_t *val, int length);
 int
 sql_dec_or_hex_to_i64(const char *z, int64_t *val);
 
-void sqlErrorWithMsg(sql *, int, const char *, ...);
-void sqlError(sql *, int);
-void sqlSystemError(sql *, int);
 void *sqlHexToBlob(sql *, const char *z, int n);
 u8 sqlHexToInt(int h);
 
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 7596c6c..3de157e 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -92,75 +92,6 @@ sqlStrlen30(const char *z)
 }
 
 /*
- * Helper function for sqlError() - called rarely.  Broken out into
- * a separate routine to avoid unnecessary register saves on entry to
- * sqlError().
- */
-static SQL_NOINLINE void
-sqlErrorFinish(sql * db, int err_code)
-{
-	sqlSystemError(db, err_code);
-}
-
-/*
- * Set the current error code to err_code and clear any prior error message.
- * Also set iSysErrno (by calling sqlSystem) if the err_code indicates
- * that would be appropriate.
- */
-void
-sqlError(sql * db, int err_code)
-{
-	assert(db != 0);
-	if (err_code)
-		sqlErrorFinish(db, err_code);
-}
-
-/*
- * Load the sql.iSysErrno field if that is an appropriate thing
- * to do based on the sql error code in rc.
- */
-void
-sqlSystemError(sql * db, int rc)
-{
-	if (rc == SQL_IOERR_NOMEM)
-		return;
-	rc &= 0xff;
-	if (rc == SQL_CANTOPEN || rc == SQL_IOERR) {
-		db->iSysErrno = sqlOsGetLastError(db->pVfs);
-	}
-}
-
-/*
- * Set the most recent error code and error string for the sql
- * handle "db". The error code is set to "err_code".
- *
- * If it is not NULL, string zFormat specifies the format of the
- * error string in the style of the printf functions: The following
- * format characters are allowed:
- *
- *      %s      Insert a string
- *      %z      A string that should be freed after use
- *      %d      Insert an integer
- *      %T      Insert a token
- *      %S      Insert the first element of a SrcList
- *
- * zFormat and any string tokens that follow it are assumed to be
- * encoded in UTF-8.
- *
- * To clear the most recent error for sql handle "db", sqlError
- * should be called with err_code set to SQL_OK and zFormat set
- * to NULL.
- */
-void
-sqlErrorWithMsg(sql * db, int err_code, const char *zFormat, ...)
-{
-	assert(db != 0);
-	sqlSystemError(db, err_code);
-	if (zFormat == 0)
-		sqlError(db, err_code);
-}
-
-/*
  * Convert an SQL-style quoted string into a normal string by removing
  * the quote characters.  The conversion is done in-place.  If the
  * input does not begin with a quote character, then this routine
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 09a1b13..064eb46 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -497,7 +497,6 @@ int sqlVdbeMemClearAndResize(Mem * pMem, int n);
 int sqlVdbeCloseStatement(Vdbe *, int);
 void sqlVdbeFrameDelete(VdbeFrame *);
 int sqlVdbeFrameRestore(VdbeFrame *);
-int sqlVdbeTransferError(Vdbe * p);
 
 int sqlVdbeSorterInit(struct sql *db, struct VdbeCursor *cursor);
 void sqlVdbeSorterReset(sql *, VdbeSorter *);
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 98d708a..7d519ce 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -504,7 +504,7 @@ sqlStep(Vdbe * p)
 		 * error has occurred, then return the error code in p->rc to the
 		 * caller. Set the error code in the database handle to the same value.
 		 */
-		rc = sqlVdbeTransferError(p);
+		rc = p->rc;
 	}
 	return (rc & db->errMask);
 }
@@ -736,7 +736,6 @@ columnMem(sql_stmt * pStmt, int i)
 	if (pVm->pResultSet != 0 && i < pVm->nResColumn && i >= 0) {
 		pOut = &pVm->pResultSet[i];
 	} else {
-		sqlError(pVm->db, SQL_RANGE);
 		pOut = (Mem *) columnNullValue();
 	}
 	return pOut;
@@ -958,20 +957,17 @@ vdbeUnbind(Vdbe * p, int i)
 		return SQL_MISUSE;
 	}
 	if (p->magic != VDBE_MAGIC_RUN || p->pc >= 0) {
-		sqlError(p->db, SQL_MISUSE);
 		sql_log(SQL_MISUSE,
 			    "bind on a busy prepared statement: [%s]", p->zSql);
 		return SQL_MISUSE;
 	}
 	if (i < 1 || i > p->nVar) {
-		sqlError(p->db, SQL_RANGE);
 		return SQL_RANGE;
 	}
 	i--;
 	pVar = &p->aVar[i];
 	sqlVdbeMemRelease(pVar);
 	pVar->flags = MEM_Null;
-	sqlError(p->db, SQL_OK);
 
 	/* If the bit corresponding to this variable in Vdbe.expmask is set, then
 	 * binding a new value to this variable invalidates the current query plan.
@@ -1057,7 +1053,6 @@ bindText(sql_stmt * pStmt,	/* The statement to bind against */
 			rc = sqlVdbeMemSetStr(pVar, zData, nData, 1, xDel);
 			if (rc == SQL_OK)
 				rc = sql_bind_type(p, i, "TEXT");
-			sqlError(p->db, rc);
 			rc = sqlApiExit(p->db, rc);
 		}
 	} else if (xDel != SQL_STATIC && xDel != SQL_TRANSIENT) {
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e3f9289..042e8eb 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2290,23 +2290,6 @@ sqlVdbeResetStepResult(Vdbe * p)
 }
 
 /*
- * Copy the error code and error message belonging to the VDBE passed
- * as the first argument to its database handle (so that they will be
- * returned by calls to sql_errcode() and sql_errmsg()).
- *
- * This function does not clear the VDBE error code or message, just
- * copies them to the database handle.
- */
-int
-sqlVdbeTransferError(Vdbe * p)
-{
-	sql *db = p->db;
-	int rc = p->rc;
-	sqlError(db, rc);
-	return rc;
-}
-
-/*
  * Clean up a VDBE after execution but do not delete the VDBE just yet.
  * Return the result code.
  *
@@ -2335,15 +2318,17 @@ sqlVdbeReset(Vdbe * p)
 	 * instructions yet, leave the main database error information unchanged.
 	 */
 	if (p->pc >= 0) {
-		sqlVdbeTransferError(p);
 		if (p->runOnlyOnce)
 			p->expired = 1;
-	} else if (p->rc && p->expired) {
-		/* The expired flag was set on the VDBE before the first call
-		 * to sql_step(). For consistency (since sql_step() was
-		 * called), set the database error in this case as well.
+	} else {
+		/*
+		 * An error should be thrown here if the expired
+		 * flag is set on the VDBE flag with the first
+		 * call to sql_step(). However, the expired flag
+		 * is currently disabled, so this error has been
+		 * replaced with assert.
 		 */
-		sqlErrorWithMsg(db, p->rc, NULL);
+		assert(p->rc == 0 || p->expired == 0);
 	}
 
 	/* Reclaim all memory used by the VDBE
diff --git a/test/sql/func-recreate.result b/test/sql/func-recreate.result
index d713292..73fb03c 100644
--- a/test/sql/func-recreate.result
+++ b/test/sql/func-recreate.result
@@ -26,7 +26,8 @@ fiber.sleep(0.1)
 ...
 box.internal.sql_create_function('WAITFOR', 'INT', function (n) require('fiber').sleep(n) return n end)
 ---
-- error: database is locked
+- error: 'Failed to create function ''WAITFOR'': unable to create function due to
+    active statements'
 ...
 ch:get()
 ---