Re: [Tarantool-patches] [PATCH v3 02/20] sql: refactor sql_prepare() and sqlPrepare()

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch, LGTM.

Sergos

On 20 Dec 15:47, Nikita Pettik wrote:
> - Removed saveSqlFlag as argument from sqlPrepare(). It was used to
>   indicate that its caller is sql_prepare_v2() not sql_prepare().
>   Since in previous commit we've left only one version of this function
>   let's remove this flag at all.
> 
> - Removed struct db from list of sql_prepare() arguments. There's one
>   global database handler and it can be obtained by sql_get() call.
>   Hence, it makes no sense to pass around this argument.
> 
> Needed for #3292
> ---
>  src/box/execute.c     |  3 +--
>  src/box/sql/analyze.c | 16 +++++++---------
>  src/box/sql/legacy.c  |  2 +-
>  src/box/sql/prepare.c | 10 ++++------
>  src/box/sql/sqlInt.h  |  3 +--
>  src/box/sql/vdbe.h    |  2 +-
>  src/box/sql/vdbeInt.h |  1 -
>  src/box/sql/vdbeapi.c |  2 +-
>  src/box/sql/vdbeaux.c |  5 +----
>  9 files changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 130a3f675..0b21386b5 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -442,8 +442,7 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind,
>  			struct region *region)
>  {
>  	struct sql_stmt *stmt;
> -	struct sql *db = sql_get();
> -	if (sql_prepare(db, sql, len, &stmt, NULL) != 0)
> +	if (sql_prepare(sql, len, &stmt, NULL) != 0)
>  		return -1;
>  	assert(stmt != NULL);
>  	port_sql_create(port, stmt);
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index b9858c8d6..e43011dd0 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1336,14 +1336,13 @@ sample_compare(const void *a, const void *b, void *arg)
>   * statistics (i.e. arrays lt, dt, dlt and avg_eq). 'load' query
>   * is needed for
>   *
> - * @param db Database handler.
>   * @param sql_select_prepare SELECT statement, see above.
>   * @param sql_select_load SELECT statement, see above.
>   * @param[out] stats Statistics is saved here.
>   * @retval 0 on success, -1 otherwise.
>   */
>  static int
> -load_stat_from_space(struct sql *db, const char *sql_select_prepare,
> +load_stat_from_space(const char *sql_select_prepare,
>  		     const char *sql_select_load, struct index_stat *stats)
>  {
>  	struct index **indexes = NULL;
> @@ -1359,7 +1358,7 @@ load_stat_from_space(struct sql *db, const char *sql_select_prepare,
>  		}
>  	}
>  	sql_stmt *stmt = NULL;
> -	int rc = sql_prepare(db, sql_select_prepare, -1, &stmt, 0);
> +	int rc = sql_prepare(sql_select_prepare, -1, &stmt, 0);
>  	if (rc)
>  		goto finalize;
>  	uint32_t current_idx_count = 0;
> @@ -1427,7 +1426,7 @@ load_stat_from_space(struct sql *db, const char *sql_select_prepare,
>  	rc = sql_finalize(stmt);
>  	if (rc)
>  		goto finalize;
> -	rc = sql_prepare(db, sql_select_load, -1, &stmt, 0);
> +	rc = sql_prepare(sql_select_load, -1, &stmt, 0);
>  	if (rc)
>  		goto finalize;
>  	struct index *prev_index = NULL;
> @@ -1505,12 +1504,11 @@ load_stat_from_space(struct sql *db, const char *sql_select_prepare,
>  }
>  
>  static int
> -load_stat_to_index(struct sql *db, const char *sql_select_load,
> -		   struct index_stat **stats)
> +load_stat_to_index(const char *sql_select_load, struct index_stat **stats)
>  {
>  	assert(stats != NULL && *stats != NULL);
>  	struct sql_stmt *stmt = NULL;
> -	if (sql_prepare(db, sql_select_load, -1, &stmt, 0) != 0)
> +	if (sql_prepare(sql_select_load, -1, &stmt, 0) != 0)
>  		return -1;
>  	uint32_t current_idx_count = 0;
>  	while (sql_step(stmt) == SQL_ROW) {
> @@ -1696,7 +1694,7 @@ sql_analysis_load(struct sql *db)
>  	const char *load_query = "SELECT \"tbl\",\"idx\",\"neq\",\"nlt\","
>  				 "\"ndlt\",\"sample\" FROM \"_sql_stat4\"";
>  	/* Load the statistics from the _sql_stat4 table. */
> -	if (load_stat_from_space(db, init_query, load_query, stats) != 0)
> +	if (load_stat_from_space(init_query, load_query, stats) != 0)
>  		goto fail;
>  	/*
>  	 * Now we have complete statistics for each index
> @@ -1739,7 +1737,7 @@ sql_analysis_load(struct sql *db)
>  	 */
>  	const char *order_query = "SELECT \"tbl\",\"idx\" FROM "
>  				  "\"_sql_stat4\" GROUP BY \"tbl\",\"idx\"";
> -	if (load_stat_to_index(db, order_query, heap_stats) == 0)
> +	if (load_stat_to_index(order_query, heap_stats) == 0)
>  		return box_txn_commit();
>  fail:
>  	box_txn_rollback();
> diff --git a/src/box/sql/legacy.c b/src/box/sql/legacy.c
> index bfd1e32b9..16507b334 100644
> --- a/src/box/sql/legacy.c
> +++ b/src/box/sql/legacy.c
> @@ -70,7 +70,7 @@ sql_exec(sql * db,	/* The database on which the SQL executes */
>  		char **azVals = 0;
>  
>  		pStmt = 0;
> -		rc = sql_prepare(db, zSql, -1, &pStmt, &zLeftover);
> +		rc = sql_prepare(zSql, -1, &pStmt, &zLeftover);
>  		assert(rc == 0 || pStmt == NULL);
>  		if (rc != 0)
>  			continue;
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 35e81212d..520b52d64 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -46,7 +46,6 @@ static int
>  sqlPrepare(sql * db,	/* Database handle. */
>  	       const char *zSql,	/* UTF-8 encoded SQL statement. */
>  	       int nBytes,	/* Length of zSql in bytes. */
> -	       int saveSqlFlag,	/* True to copy SQL text into the sql_stmt */
>  	       Vdbe * pReprepare,	/* VM being reprepared */
>  	       sql_stmt ** ppStmt,	/* OUT: A pointer to the prepared statement */
>  	       const char **pzTail	/* OUT: End of parsed string */
> @@ -156,8 +155,7 @@ sqlPrepare(sql * db,	/* Database handle. */
>  
>  	if (db->init.busy == 0) {
>  		Vdbe *pVdbe = sParse.pVdbe;
> -		sqlVdbeSetSql(pVdbe, zSql, (int)(sParse.zTail - zSql),
> -				  saveSqlFlag);
> +		sqlVdbeSetSql(pVdbe, zSql, (int)(sParse.zTail - zSql));
>  	}
>  	if (sParse.pVdbe != NULL && (rc != 0 || db->mallocFailed)) {
>  		sqlVdbeFinalize(sParse.pVdbe);
> @@ -192,7 +190,7 @@ sqlReprepare(Vdbe * p)
>  	zSql = sql_sql((sql_stmt *) p);
>  	assert(zSql != 0);	/* Reprepare only called for prepare_v2() statements */
>  	db = sqlVdbeDb(p);
> -	if (sqlPrepare(db, zSql, -1, 0, p, &pNew, 0) != 0) {
> +	if (sqlPrepare(db, zSql, -1, p, &pNew, 0) != 0) {
>  		assert(pNew == 0);
>  		return -1;
>  	}
> @@ -205,10 +203,10 @@ sqlReprepare(Vdbe * p)
>  }
>  
>  int
> -sql_prepare(struct sql *db, const char *sql, int length, struct sql_stmt **stmt,
> +sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
>  	    const char **sql_tail)
>  {
> -	int rc = sqlPrepare(db, sql, length, 1, 0, stmt, sql_tail);
> +	int rc = sqlPrepare(sql_get(), sql, length, 0, stmt, sql_tail);
>  	assert(rc == 0 || stmt == NULL || *stmt == NULL);
>  	return rc;
>  }
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 7bd952a17..ac1d8ce42 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -471,14 +471,13 @@ typedef void (*sql_destructor_type) (void *);
>  /**
>   * Prepare (compile into VDBE byte-code) statement.
>   *
> - * @param db Database handle.
>   * @param sql UTF-8 encoded SQL statement.
>   * @param length Length of @param sql in bytes.
>   * @param[out] stmt A pointer to the prepared statement.
>   * @param[out] sql_tail End of parsed string.
>   */
>  int
> -sql_prepare(struct sql *db, const char *sql, int length, struct sql_stmt **stmt,
> +sql_prepare(const char *sql, int length, struct sql_stmt **stmt,
>  	    const char **sql_tail);
>  
>  int
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index 582d48a1f..573577355 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -251,7 +251,7 @@ void sqlVdbeSetNumCols(Vdbe *, int);
>  int sqlVdbeSetColName(Vdbe *, int, int, const char *, void (*)(void *));
>  void sqlVdbeCountChanges(Vdbe *);
>  sql *sqlVdbeDb(Vdbe *);
> -void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
> +void sqlVdbeSetSql(Vdbe *, const char *z, int n);
>  void sqlVdbeSwap(Vdbe *, Vdbe *);
>  VdbeOp *sqlVdbeTakeOpArray(Vdbe *, int *, int *);
>  sql_value *sqlVdbeGetBoundValue(Vdbe *, int, u8);
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index 0f32b4cd6..078ebc34e 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -421,7 +421,6 @@ struct Vdbe {
>  	bft explain:2;		/* True if EXPLAIN present on SQL command */
>  	bft changeCntOn:1;	/* True to update the change-counter */
>  	bft runOnlyOnce:1;	/* Automatically expire on reset */
> -	bft isPrepareV2:1;	/* True if prepared with prepare_v2() */
>  	u32 aCounter[5];	/* Counters used by sql_stmt_status() */
>  	char *zSql;		/* Text of the SQL statement that generated this */
>  	void *pFree;		/* Free this when deleting the vdbe */
> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index 12449d3bc..db7936e78 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -451,7 +451,7 @@ sqlStep(Vdbe * p)
>  	if (rc != SQL_ROW)
>  		checkProfileCallback(db, p);
>  
> -	if (p->isPrepareV2 && rc != SQL_ROW && rc != SQL_DONE) {
> +	if (rc != SQL_ROW && rc != SQL_DONE) {
>  		/* If this statement was prepared using sql_prepare(), and an
>  		 * error has occurred, then return an error.
>  		 */
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index a1d658648..619105820 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -89,14 +89,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
>   * Remember the SQL string for a prepared statement.
>   */
>  void
> -sqlVdbeSetSql(Vdbe * p, const char *z, int n, int isPrepareV2)
> +sqlVdbeSetSql(Vdbe * p, const char *z, int n)
>  {
> -	assert(isPrepareV2 == 1 || isPrepareV2 == 0);
>  	if (p == 0)
>  		return;
>  	assert(p->zSql == 0);
>  	p->zSql = sqlDbStrNDup(p->db, z, n);
> -	p->isPrepareV2 = (u8) isPrepareV2;
>  }
>  
>  /*
> @@ -120,7 +118,6 @@ sqlVdbeSwap(Vdbe * pA, Vdbe * pB)
>  	zTmp = pA->zSql;
>  	pA->zSql = pB->zSql;
>  	pB->zSql = zTmp;
> -	pB->isPrepareV2 = pA->isPrepareV2;
>  }
>  
>  /*
> -- 
> 2.15.1
>