[tarantool-patches] [PATCH 0/3] Merge struct sql_txn into struct txn/savepoint

[tarantool-patches] [PATCH 0/3] Merge struct sql_txn into struct txn/savepoint

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/move-sql-structs-from-txn

As a one of final steps of merging SQL and NoSQL codebases, it is
required to squash struct sql_txn and struct txn/struct txn_savepoint.
struct sql_txn was needed to operate on named savepoints. Hence, it
contains name of savepoint and a link to next savepoint. This patch-set
adds optional name of savepoint to struct txn_savepoint and orginizes
txn_savepoints into list. Head of list is held in stuct txn. Iterating
over list allows to find savepoint by its name. Finally, after this
procedure is completed, we can remove struct sql_txn and struct Savepoint.

Nikita Pettik (3):
  txn: move fk_deferred_count from psql_txn to txn
  txn: merge struct sql_txn into struct txn
  sql: use struct txn_savepoint as anonymous savepoint

 src/box/sql/sqlInt.h  | 13 --------
 src/box/sql/vdbe.c    | 58 ++++++++++------------------------
 src/box/sql/vdbe.h    |  9 ------
 src/box/sql/vdbeInt.h |  6 ++--
 src/box/sql/vdbeaux.c | 66 ++-------------------------------------
 src/box/txn.c         | 86 ++++++++++++++++++++++++++++++++++++++-------------
 src/box/txn.h         | 32 ++++++++++++-------
 7 files changed, 106 insertions(+), 164 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/3] txn: move fk_deferred_count from psql_txn to txn

[Nikita Pettik]

We are going to merge struct psql_txn with struct txn as a part of SQL
integration into NoSQL, so let's move counter of deferred foreign key
violations directly to struct txn.
---
 src/box/sql/vdbe.c    |  6 +++---
 src/box/sql/vdbeaux.c |  4 +---
 src/box/txn.c         | 16 ++++++----------
 src/box/txn.h         |  6 +-----
 4 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e096e1f65..246654cb7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2900,7 +2900,7 @@ case OP_Savepoint: {
 				assert(pSavepoint == psql_txn->pSavepoint);
 				psql_txn->pSavepoint = pSavepoint->pNext;
 			} else {
-				psql_txn->fk_deferred_count =
+				txn->fk_deferred_count =
 					pSavepoint->tnt_savepoint->fk_deferred_count;
 			}
 		}
@@ -4844,7 +4844,7 @@ case OP_FkCounter: {
 	    !p->auto_commit) {
 		struct txn *txn = in_txn();
 		assert(txn != NULL && txn->psql_txn != NULL);
-		txn->psql_txn->fk_deferred_count += pOp->p2;
+		txn->fk_deferred_count += pOp->p2;
 	} else {
 		p->nFkConstraint += pOp->p2;
 	}
@@ -4868,7 +4868,7 @@ case OP_FkIfZero: {         /* jump */
 	    !p->auto_commit) {
 		struct txn *txn = in_txn();
 		assert(txn != NULL && txn->psql_txn != NULL);
-		if (txn->psql_txn->fk_deferred_count == 0)
+		if (txn->fk_deferred_count == 0)
 			goto jump_to_p2;
 	} else {
 		if (p->nFkConstraint == 0)
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e7accc745..bbbb99c70 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -87,7 +87,6 @@ sql_alloc_txn()
 		return NULL;
 	}
 	txn->pSavepoint = NULL;
-	txn->fk_deferred_count = 0;
 	return txn;
 }
 
@@ -1988,8 +1987,7 @@ int
 sqlVdbeCheckFk(Vdbe * p, int deferred)
 {
 	struct txn *txn = in_txn();
-	if ((deferred && txn != NULL && txn->psql_txn != NULL &&
-	     txn->psql_txn->fk_deferred_count > 0) ||
+	if ((deferred && txn != NULL && txn->fk_deferred_count > 0) ||
 	    (!deferred && p->nFkConstraint > 0)) {
 		p->is_aborted = true;
 		p->errorAction = ON_CONFLICT_ACTION_ABORT;
diff --git a/src/box/txn.c b/src/box/txn.c
index 53ebfc053..b57240846 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -222,6 +222,7 @@ txn_begin()
 	txn->engine = NULL;
 	txn->engine_tx = NULL;
 	txn->psql_txn = NULL;
+	txn->fk_deferred_count = 0;
 	txn->fiber = NULL;
 	fiber_set_txn(fiber(), txn);
 	/* fiber_on_yield is initialized by engine on demand */
@@ -537,12 +538,9 @@ txn_prepare(struct txn *txn)
 	 * foreign key constraints must not be violated.
 	 * If not so, just rollback transaction.
 	 */
-	if (txn->psql_txn != NULL) {
-		struct sql_txn *sql_txn = txn->psql_txn;
-		if (sql_txn->fk_deferred_count != 0) {
-			diag_set(ClientError, ER_FOREIGN_KEY_CONSTRAINT);
-			return -1;
-		}
+	if (txn->fk_deferred_count != 0) {
+		diag_set(ClientError, ER_FOREIGN_KEY_CONSTRAINT);
+		return -1;
 	}
 	/*
 	 * Perform transaction conflict resolution. Engine == NULL when
@@ -754,8 +752,7 @@ box_txn_savepoint()
 	}
 	svp->stmt = stailq_last(&txn->stmts);
 	svp->in_sub_stmt = txn->in_sub_stmt;
-	if (txn->psql_txn != NULL)
-		svp->fk_deferred_count = txn->psql_txn->fk_deferred_count;
+	svp->fk_deferred_count = txn->fk_deferred_count;
 	return svp;
 }
 
@@ -782,8 +779,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 		return -1;
 	}
 	txn_rollback_to_svp(txn, svp->stmt);
-	if (txn->psql_txn != NULL)
-		txn->psql_txn->fk_deferred_count = svp->fk_deferred_count;
+	txn->fk_deferred_count = svp->fk_deferred_count;
 	return 0;
 }
 
diff --git a/src/box/txn.h b/src/box/txn.h
index 37d90932b..87e880a6a 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -135,11 +135,6 @@ extern double too_long_threshold;
 struct sql_txn {
 	/** List of active SQL savepoints. */
 	struct Savepoint *pSavepoint;
-	/**
-	 * This variables transfer deferred constraints from one
-	 * VDBE to the next in the same transaction.
-	 */
-	uint32_t fk_deferred_count;
 };
 
 /**
@@ -213,6 +208,7 @@ struct txn {
 	struct trigger fiber_on_stop;
 	/** Commit and rollback triggers. */
 	struct rlist on_commit, on_rollback;
+	uint32_t fk_deferred_count;
 	struct sql_txn *psql_txn;
 };
 
-- 
2.15.1

[tarantool-patches] [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Nikita Pettik]

This procedure is processed in a several steps. Firstly, we add name
to struct txn_savepoint since we should be capable of operating on named
savepoints (which in turn is SQL feature). Still, anonymous (in the sense
of name absence) savepoints are also valid. Then, we add list (as
implementation of stailq) of savepoints to struct txn: it allows us to
find savepoint by its name. Finally, we patch rollback to/release
savepoint routines: for rollback tail of the list containing savepoints
is cut (but subject of rollback routine remains in the list); for
release routine we cut tail including node being released.
---
 src/box/sql/vdbe.c    | 52 +++++++++++---------------------------
 src/box/sql/vdbe.h    |  9 -------
 src/box/sql/vdbeaux.c | 32 -----------------------
 src/box/txn.c         | 70 ++++++++++++++++++++++++++++++++++++++++++---------
 src/box/txn.h         | 26 +++++++++++++------
 5 files changed, 91 insertions(+), 98 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 246654cb7..7d05ce11c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2823,13 +2823,9 @@ case OP_Count: {         /* out2 */
 case OP_Savepoint: {
 	int p1;                         /* Value of P1 operand */
 	char *zName;                    /* Name of savepoint */
-	Savepoint *pNew;
-	Savepoint *pSavepoint;
-	Savepoint *pTmp;
 	struct txn *txn = in_txn();
-	struct sql_txn *psql_txn = txn != NULL ? txn->psql_txn : NULL;
 
-	if (psql_txn == NULL) {
+	if (txn == NULL) {
 		assert(!box_txn());
 		diag_set(ClientError, ER_NO_TRANSACTION);
 		goto abort_due_to_error;
@@ -2840,25 +2836,21 @@ case OP_Savepoint: {
 	/* Assert that the p1 parameter is valid. Also that if there is no open
 	 * transaction, then there cannot be any savepoints.
 	 */
-	assert(psql_txn->pSavepoint == NULL || box_txn());
+	assert(stailq_empty(&txn->savepoints) || box_txn());
 	assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
 
 	if (p1==SAVEPOINT_BEGIN) {
-		/* Create a new savepoint structure. */
-		pNew = sql_savepoint(p, zName);
-		/* Link the new savepoint into the database handle's list. */
-		pNew->pNext = psql_txn->pSavepoint;
-		psql_txn->pSavepoint = pNew;
+		/*
+		 * Savepoint is available by its name so we don't
+		 * care about object itself.
+		 */
+		(void) txn_savepoint_new(txn, zName);
 	} else {
 		/* Find the named savepoint. If there is no such savepoint, then an
 		 * an error is returned to the user.
 		 */
-		for(
-			pSavepoint = psql_txn->pSavepoint;
-			pSavepoint && sqlStrICmp(pSavepoint->zName, zName);
-			pSavepoint = pSavepoint->pNext
-			);
-		if (!pSavepoint) {
+		struct txn_savepoint *sv = txn_savepoint_by_name(txn, zName);
+		if (sv == NULL) {
 			diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
 			goto abort_due_to_error;
 		} else {
@@ -2867,8 +2859,7 @@ case OP_Savepoint: {
 			 * and this is a RELEASE command, then the current transaction
 			 * is committed.
 			 */
-			int isTransaction = pSavepoint->pNext == 0;
-			if (isTransaction && p1==SAVEPOINT_RELEASE) {
+			if (p1 == SAVEPOINT_RELEASE) {
 				if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
 					goto vdbe_return;
 				sqlVdbeHalt(p);
@@ -2876,19 +2867,8 @@ case OP_Savepoint: {
 					goto abort_due_to_error;
 			} else {
 				if (p1==SAVEPOINT_ROLLBACK)
-					box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint);
-			}
-
-			/* Regardless of whether this is a RELEASE or ROLLBACK, destroy all
-			 * savepoints nested inside of the savepoint being operated on.
-			 */
-			while (psql_txn->pSavepoint != pSavepoint) {
-				pTmp = psql_txn->pSavepoint;
-				psql_txn->pSavepoint = pTmp->pNext;
-				/*
-				 * Since savepoints are stored in region, we do not
-				 * have to destroy them
-				 */
+					if (box_txn_rollback_to_savepoint(sv) != 0)
+						goto abort_due_to_error;
 			}
 
 			/* If it is a RELEASE, then destroy the savepoint being operated on
@@ -2897,11 +2877,9 @@ case OP_Savepoint: {
 			 * when the savepoint was created.
 			 */
 			if (p1==SAVEPOINT_RELEASE) {
-				assert(pSavepoint == psql_txn->pSavepoint);
-				psql_txn->pSavepoint = pSavepoint->pNext;
+				txn_savepoint_release(sv);
 			} else {
-				txn->fk_deferred_count =
-					pSavepoint->tnt_savepoint->fk_deferred_count;
+				txn->fk_deferred_count = sv->fk_deferred_count;
 			}
 		}
 	}
@@ -4843,7 +4821,6 @@ case OP_FkCounter: {
 	if (((p->sql_flags & SQL_DeferFKs) != 0 || pOp->p1 != 0) &&
 	    !p->auto_commit) {
 		struct txn *txn = in_txn();
-		assert(txn != NULL && txn->psql_txn != NULL);
 		txn->fk_deferred_count += pOp->p2;
 	} else {
 		p->nFkConstraint += pOp->p2;
@@ -4867,7 +4844,6 @@ case OP_FkIfZero: {         /* jump */
 	if (((p->sql_flags & SQL_DeferFKs) != 0 || pOp->p1 != 0) &&
 	    !p->auto_commit) {
 		struct txn *txn = in_txn();
-		assert(txn != NULL && txn->psql_txn != NULL);
 		if (txn->fk_deferred_count == 0)
 			goto jump_to_p2;
 	} else {
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 8f16202ba..06f258805 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -172,15 +172,6 @@ struct SubProgram {
  */
 Vdbe *sqlVdbeCreate(Parse *);
 
-/**
- * Allocate and initialize SQL-specific struct which completes
- * original Tarantool's txn struct using region allocator.
- *
- * @retval NULL on OOM, new psql_txn struct on success.
- **/
-struct sql_txn *
-sql_alloc_txn();
-
 /**
  * Prepare given VDBE to execution: initialize structs connected
  * with transaction routine: autocommit mode, deferred foreign
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index bbbb99c70..3d256f86a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -76,39 +76,12 @@ sqlVdbeCreate(Parse * pParse)
 	return p;
 }
 
-struct sql_txn *
-sql_alloc_txn()
-{
-	struct sql_txn *txn = region_alloc_object(&fiber()->gc,
-						  struct sql_txn);
-	if (txn == NULL) {
-		diag_set(OutOfMemory, sizeof(struct sql_txn), "region",
-			 "struct sql_txn");
-		return NULL;
-	}
-	txn->pSavepoint = NULL;
-	return txn;
-}
-
 int
 sql_vdbe_prepare(struct Vdbe *vdbe)
 {
 	assert(vdbe != NULL);
 	struct txn *txn = in_txn();
 	vdbe->auto_commit = txn == NULL;
-	if (txn != NULL) {
-		/*
-		 * If transaction has been started in Lua, then
-		 * sql_txn is NULL. On the other hand, it is not
-		 * critical, since in Lua it is impossible to
-		 * check FK violations, at least now.
-		 */
-		if (txn->psql_txn == NULL) {
-			txn->psql_txn = sql_alloc_txn();
-			if (txn->psql_txn == NULL)
-				return -1;
-		}
-	}
 	return 0;
 }
 
@@ -2008,11 +1981,6 @@ sql_txn_begin()
 	struct txn *ptxn = txn_begin(false);
 	if (ptxn == NULL)
 		return -1;
-	ptxn->psql_txn = sql_alloc_txn();
-	if (ptxn->psql_txn == NULL) {
-		box_txn_rollback();
-		return -1;
-	}
 	return 0;
 }
 
diff --git a/src/box/txn.c b/src/box/txn.c
index b57240846..451cc9eb1 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -221,8 +221,8 @@ txn_begin()
 	txn->signature = -1;
 	txn->engine = NULL;
 	txn->engine_tx = NULL;
-	txn->psql_txn = NULL;
 	txn->fk_deferred_count = 0;
+	stailq_create(&txn->savepoints);
 	txn->fiber = NULL;
 	fiber_set_txn(fiber(), txn);
 	/* fiber_on_yield is initialized by engine on demand */
@@ -734,28 +734,56 @@ box_txn_alloc(size_t size)
 	                            alignof(union natural_align));
 }
 
-box_txn_savepoint_t *
-box_txn_savepoint()
+struct txn_savepoint *
+txn_savepoint_new(struct txn *txn, const char *name)
 {
-	struct txn *txn = in_txn();
-	if (txn == NULL) {
-		diag_set(ClientError, ER_NO_TRANSACTION);
-		return NULL;
-	}
+	assert(txn == in_txn());
+	size_t svp_sz = sizeof(struct txn_savepoint);
+	svp_sz += name != NULL ? strlen(name) + 1 : 0;
 	struct txn_savepoint *svp =
-		(struct txn_savepoint *) region_alloc_object(&txn->region,
-							struct txn_savepoint);
+		(struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
 	if (svp == NULL) {
-		diag_set(OutOfMemory, sizeof(*svp),
-			 "region", "struct txn_savepoint");
+		diag_set(OutOfMemory, svp_sz, "region", "svp");
 		return NULL;
 	}
 	svp->stmt = stailq_last(&txn->stmts);
 	svp->in_sub_stmt = txn->in_sub_stmt;
 	svp->fk_deferred_count = txn->fk_deferred_count;
+	if (name != NULL) {
+		svp->name = (char *) svp + sizeof(struct txn_savepoint);
+		memcpy(svp->name, name, strlen(name) + 1);
+	} else {
+		svp->name = NULL;
+	}
+	stailq_add_tail_entry(&txn->savepoints, svp, next);
 	return svp;
 }
 
+struct txn_savepoint *
+txn_savepoint_by_name(struct txn *txn, const char *name)
+{
+	assert(txn == in_txn());
+	struct txn_savepoint *sv = NULL;
+	stailq_foreach_entry(sv, &txn->savepoints, next) {
+		if (sv->name == NULL)
+			continue;
+		if (strcmp(sv->name, name) == 0)
+			return sv;
+	}
+	return NULL;
+}
+
+box_txn_savepoint_t *
+box_txn_savepoint()
+{
+	struct txn *txn = in_txn();
+	if (txn == NULL) {
+		diag_set(ClientError, ER_NO_TRANSACTION);
+		return NULL;
+	}
+	return txn_savepoint_new(txn, NULL);
+}
+
 int
 box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 {
@@ -779,10 +807,28 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 		return -1;
 	}
 	txn_rollback_to_svp(txn, svp->stmt);
+	/* Discard from list all prior savepoints. */
+	struct stailq discard;
+	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
 	txn->fk_deferred_count = svp->fk_deferred_count;
 	return 0;
 }
 
+void
+txn_savepoint_release(struct txn_savepoint *svp)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL);
+	/*
+	 * Discard current savepoint alongside with all
+	 * created after it savepoints. Note that operation
+	 * is currently available only for SQL.
+	 */
+	struct stailq discard;
+	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
+	stailq_cut_tail(&txn->savepoints, *txn->savepoints.last, &discard);
+}
+
 static void
 txn_on_stop(struct trigger *trigger, void *event)
 {
diff --git a/src/box/txn.h b/src/box/txn.h
index 87e880a6a..a14f3871a 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -43,7 +43,6 @@ extern "C" {
 
 /** box statistics */
 extern struct rmean *rmean_box;
-struct Savepoint;
 
 struct engine;
 struct space;
@@ -128,15 +127,12 @@ struct txn_savepoint {
 	 * state violating any number of deferred FK constraints.
 	 */
 	uint32_t fk_deferred_count;
+	struct stailq_entry next;
+	char *name;
 };
 
 extern double too_long_threshold;
 
-struct sql_txn {
-	/** List of active SQL savepoints. */
-	struct Savepoint *pSavepoint;
-};
-
 /**
  * An element of list of autogenerated ids, being returned as SQL
  * response metadata.
@@ -209,7 +205,7 @@ struct txn {
 	/** Commit and rollback triggers. */
 	struct rlist on_commit, on_rollback;
 	uint32_t fk_deferred_count;
-	struct sql_txn *psql_txn;
+	struct stailq savepoints;
 };
 
 static inline bool
@@ -455,6 +451,22 @@ txn_current_stmt(struct txn *txn)
 	return stailq_entry(stmt, struct txn_stmt, next);
 }
 
+/**
+ * Allocate new savepoint object using region allocator.
+ * Savepoint is allowed to be anonymous (i.e. without
+ * name).
+ */
+struct txn_savepoint *
+txn_savepoint_new(struct txn *txn, const char *name);
+
+/** Find savepoint by its name in savepoint list. */
+struct txn_savepoint *
+txn_savepoint_by_name(struct txn *txn, const char *name);
+
+/** Remove given and all prior entries from savepoint list. */
+void
+txn_savepoint_release(struct txn_savepoint *svp);
+
 /**
  * FFI bindings: do not throw exceptions, do not accept extra
  * arguments
-- 
2.15.1

[tarantool-patches] [PATCH 3/3] sql: use struct txn_savepoint as anonymous savepoint

[Nikita Pettik]

This allows us to completely remove SQL specific struct Savepoint and
use instead original struct txn_savepoint.
---
 src/box/sql/sqlInt.h  | 13 -------------
 src/box/sql/vdbe.c    |  2 +-
 src/box/sql/vdbeInt.h |  6 ++----
 src/box/sql/vdbeaux.c | 30 ++----------------------------
 4 files changed, 5 insertions(+), 46 deletions(-)

diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 87e5d22ee..c61605046 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -1050,7 +1050,6 @@ typedef struct NameContext NameContext;
 typedef struct Parse Parse;
 typedef struct PrintfArguments PrintfArguments;
 typedef struct RowSet RowSet;
-typedef struct Savepoint Savepoint;
 typedef struct Select Select;
 typedef struct sqlThread sqlThread;
 typedef struct SelectDest SelectDest;
@@ -1406,18 +1405,6 @@ enum trim_side_mask {
   {nArg, (nc*SQL_FUNC_NEEDCOLL)|extraFlags, \
    SQL_INT_TO_PTR(arg), 0, xStep,xFinal,#zName, {0}, type}
 
-/*
- * All current savepoints are stored in a linked list starting at
- * sql.pSavepoint. The first element in the list is the most recently
- * opened savepoint. Savepoints are added to the list by the vdbe
- * OP_Savepoint instruction.
- */
-struct Savepoint {
-	box_txn_savepoint_t *tnt_savepoint; /* Tarantool's savepoint struct */
-	char *zName;		/* Savepoint name (nul-terminated) */
-	Savepoint *pNext;	/* Parent savepoint (if any) */
-};
-
 /*
  * The following are used as the second parameter to sqlSavepoint(),
  * and as the P1 argument to the OP_Savepoint instruction.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7d05ce11c..9165db6c3 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2977,7 +2977,7 @@ case OP_TTransaction: {
 		if (sql_txn_begin() != 0)
 			goto abort_due_to_error;
 	} else {
-		p->anonymous_savepoint = sql_savepoint(p, NULL);
+		p->anonymous_savepoint = txn_savepoint_new(in_txn(), NULL);
 		if (p->anonymous_savepoint == NULL)
 			goto abort_due_to_error;
 	}
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 5582d9506..f3e05a180 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -425,7 +425,7 @@ struct Vdbe {
 	/** Parser flags with which this object was built. */
 	uint32_t sql_flags;
 	/* Anonymous savepoint for aborts only */
-	Savepoint *anonymous_savepoint;
+	struct txn_savepoint *anonymous_savepoint;
 };
 
 /*
@@ -455,9 +455,7 @@ int sqlVdbeExec(Vdbe *);
 int sqlVdbeList(Vdbe *);
 int
 sql_txn_begin();
-Savepoint *
-sql_savepoint(Vdbe *p,
-	      const char *zName);
+
 int sqlVdbeHalt(Vdbe *);
 int sqlVdbeMemTooBig(Mem *);
 int sqlVdbeMemCopy(Mem *, const Mem *);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3d256f86a..7e512dbea 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1937,12 +1937,12 @@ int
 sqlVdbeCloseStatement(Vdbe * p, int eOp)
 {
 	int rc = 0;
-	const Savepoint *savepoint = p->anonymous_savepoint;
+	struct txn_savepoint *savepoint = p->anonymous_savepoint;
 	/*
 	 * If we have an anonymous transaction opened -> perform eOp.
 	 */
 	if (savepoint && eOp == SAVEPOINT_ROLLBACK)
-		rc = box_txn_rollback_to_savepoint(savepoint->tnt_savepoint);
+		rc = box_txn_rollback_to_savepoint(savepoint);
 	p->anonymous_savepoint = NULL;
 	return rc;
 }
@@ -1984,32 +1984,6 @@ sql_txn_begin()
 	return 0;
 }
 
-Savepoint *
-sql_savepoint(MAYBE_UNUSED Vdbe *p, const char *zName)
-{
-	assert(p);
-	size_t nName = zName ? strlen(zName) + 1 : 0;
-	size_t savepoint_sz = sizeof(Savepoint) + nName;
-	Savepoint *pNew;
-
-	pNew = (Savepoint *)region_aligned_alloc(&fiber()->gc,
-						 savepoint_sz,
-						 alignof(Savepoint));
-	if (pNew == NULL) {
-		diag_set(OutOfMemory, savepoint_sz, "region",
-			 "savepoint");
-		return NULL;
-	}
-	pNew->tnt_savepoint = box_txn_savepoint();
-	if (!pNew->tnt_savepoint)
-		return NULL;
-	if (zName) {
-		pNew->zName = (char *)&pNew[1];
-		memcpy(pNew->zName, zName, nName);
-	};
-	return pNew;
-}
-
 /*
  * This routine is called the when a VDBE tries to halt.  If the VDBE
  * has made changes and is in autocommit mode, then commit those
-- 
2.15.1

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/07 18:19]:
> +box_txn_savepoint_t *
> +box_txn_savepoint()
> +{
> +	struct txn *txn = in_txn();
> +	if (txn == NULL) {
> +		diag_set(ClientError, ER_NO_TRANSACTION);
> +		return NULL;
> +	}
> +	return txn_savepoint_new(txn, NULL);
> +}

Please don't use box_ API in SQL. We do not use box_ API
exclusively any more, so there is no point in extra wrappers.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 0/3] Merge struct sql_txn into struct txn/savepoint

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/08/07 18:19]:
> Branch: https://github.com/tarantool/tarantool/tree/np/move-sql-structs-from-txn
> 
> As a one of final steps of merging SQL and NoSQL codebases, it is
> required to squash struct sql_txn and struct txn/struct txn_savepoint.
> struct sql_txn was needed to operate on named savepoints. Hence, it
> contains name of savepoint and a link to next savepoint. This patch-set
> adds optional name of savepoint to struct txn_savepoint and orginizes
> txn_savepoints into list. Head of list is held in stuct txn. Iterating
> over list allows to find savepoint by its name. Finally, after this
> procedure is completed, we can remove struct sql_txn and struct Savepoint.

Thanks for the patch, overall LGTM (but I did not look carefully).

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 1/3] txn: move fk_deferred_count from psql_txn to txn

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/src/box/txn.h b/src/box/txn.h
> index 37d90932b..87e880a6a 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -213,6 +208,7 @@ struct txn {
>  	struct trigger fiber_on_stop;
>  	/** Commit and rollback triggers. */
>  	struct rlist on_commit, on_rollback;
> +	uint32_t fk_deferred_count;

Please, add a comment. A detailed one. That this number is used
by SQL only, what does it mean 'deferred fk', why is it stored
in the transaction.

>  	struct sql_txn *psql_txn;
>  };
>  
> 

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Vladislav Shpilevoy]

Thanks for the patch! See 9 comments below.

On 07/08/2019 17:13, Nikita Pettik wrote:
> This procedure is processed in a several steps. Firstly, we add name

1. Typo: 'a' is not used with plural.

> to struct txn_savepoint since we should be capable of operating on named
> savepoints (which in turn is SQL feature). Still, anonymous (in the sense
> of name absence) savepoints are also valid. Then, we add list (as
> implementation of stailq) of savepoints to struct txn: it allows us to
> find savepoint by its name. Finally, we patch rollback to/release
> savepoint routines: for rollback tail of the list containing savepoints
> is cut (but subject of rollback routine remains in the list); for
> release routine we cut tail including node being released.
> ---
>  src/box/sql/vdbe.c    | 52 +++++++++++---------------------------
>  src/box/sql/vdbe.h    |  9 -------
>  src/box/sql/vdbeaux.c | 32 -----------------------
>  src/box/txn.c         | 70 ++++++++++++++++++++++++++++++++++++++++++---------
>  src/box/txn.h         | 26 +++++++++++++------
>  5 files changed, 91 insertions(+), 98 deletions(-)
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 246654cb7..7d05ce11c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2867,8 +2859,7 @@ case OP_Savepoint: {
>  			 * and this is a RELEASE command, then the current transaction
>  			 * is committed.
>  			 */
> -			int isTransaction = pSavepoint->pNext == 0;
> -			if (isTransaction && p1==SAVEPOINT_RELEASE) {
> +			if (p1 == SAVEPOINT_RELEASE) {

2. First, the comment is outdated now (perhaps).
Second, I don't understand that code. Why release
of a savepoint

1) affects fk? Because it is not rollback, it is just release. It
does not rollback anything, and only frees savepoint objects (freed,
when it was on malloc).

2) leads to halt?

>  				if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
>  					goto vdbe_return;
>  				sqlVdbeHalt(p);
> @@ -2876,19 +2867,8 @@ case OP_Savepoint: {
>  					goto abort_due_to_error;
>  			} else {
>  				if (p1==SAVEPOINT_ROLLBACK)
> -					box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint);
> -			}
> -
> -			/* Regardless of whether this is a RELEASE or ROLLBACK, destroy all
> -			 * savepoints nested inside of the savepoint being operated on.
> -			 */
> -			while (psql_txn->pSavepoint != pSavepoint) {
> -				pTmp = psql_txn->pSavepoint;
> -				psql_txn->pSavepoint = pTmp->pNext;
> -				/*
> -				 * Since savepoints are stored in region, we do not
> -				 * have to destroy them
> -				 */
> +					if (box_txn_rollback_to_savepoint(sv) != 0)
> +						goto abort_due_to_error;

3. Something is wrong with braces now. The top 'if' has two lines
in body, but has no braces. Honestly, the whole code block looks bad.
Please, consider these changes. They are below and on top of the branch
in a separate commit.

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

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9165db6c3..7b0e450c5 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2853,34 +2853,19 @@ case OP_Savepoint: {
 		if (sv == NULL) {
 			diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
 			goto abort_due_to_error;
+		}
+		if (p1 == SAVEPOINT_RELEASE) {
+			if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
+				goto vdbe_return;
+			sqlVdbeHalt(p);
+			if (p->is_aborted)
+				goto abort_due_to_error;
+			txn_savepoint_release(sv);
 		} else {
-
-			/* Determine whether or not this is a transaction savepoint. If so,
-			 * and this is a RELEASE command, then the current transaction
-			 * is committed.
-			 */
-			if (p1 == SAVEPOINT_RELEASE) {
-				if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
-					goto vdbe_return;
-				sqlVdbeHalt(p);
-				if (p->is_aborted)
-					goto abort_due_to_error;
-			} else {
-				if (p1==SAVEPOINT_ROLLBACK)
-					if (box_txn_rollback_to_savepoint(sv) != 0)
-						goto abort_due_to_error;
-			}
-
-			/* If it is a RELEASE, then destroy the savepoint being operated on
-			 * too. If it is a ROLLBACK TO, then set the number of deferred
-			 * constraint violations present in the database to the value stored
-			 * when the savepoint was created.
-			 */
-			if (p1==SAVEPOINT_RELEASE) {
-				txn_savepoint_release(sv);
-			} else {
-				txn->fk_deferred_count = sv->fk_deferred_count;
-			}
+			assert(p1 == SAVEPOINT_ROLLBACK);
+			if (box_txn_rollback_to_savepoint(sv) != 0)
+				goto abort_due_to_error;
+			txn->fk_deferred_count = sv->fk_deferred_count;
 		}
 	}
 
=================================================================================

> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index 8f16202ba..06f258805 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -172,15 +172,6 @@ struct SubProgram {
>   */
>  Vdbe *sqlVdbeCreate(Parse *);
>  
> -/**
> - * Allocate and initialize SQL-specific struct which completes
> - * original Tarantool's txn struct using region allocator.
> - *
> - * @retval NULL on OOM, new psql_txn struct on success.
> - **/
> -struct sql_txn *
> -sql_alloc_txn();

4. Please, drop sql_txn_begin too.

> diff --git a/src/box/txn.c b/src/box/txn.c
> index b57240846..451cc9eb1 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -779,10 +807,28 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
>  		return -1;
>  	}
>  	txn_rollback_to_svp(txn, svp->stmt);
> +	/* Discard from list all prior savepoints. */
> +	struct stailq discard;
> +	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
>  	txn->fk_deferred_count = svp->fk_deferred_count;
>  	return 0;
>  }
>  
> +void
> +txn_savepoint_release(struct txn_savepoint *svp)
> +{
> +	struct txn *txn = in_txn();
> +	assert(txn != NULL);

5. Please, add an assertion, that this savepoint is not released
yet. I.e. that its statement has non NULL space and row.

> +	/*
> +	 * Discard current savepoint alongside with all
> +	 * created after it savepoints. Note that operation
> +	 * is currently available only for SQL.
> +	 */
> +	struct stailq discard;
> +	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
> +	stailq_cut_tail(&txn->savepoints, *txn->savepoints.last, &discard);

6. Sorry, I failed to understand what is happening here, and how
does it work. Why do you touch internal attributte of 'struct stailq',
and why the first call is not enough?

> +}
> +
>  static void
>  txn_on_stop(struct trigger *trigger, void *event)
>  {
> diff --git a/src/box/txn.h b/src/box/txn.h
> index 87e880a6a..a14f3871a 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -128,15 +127,12 @@ struct txn_savepoint {
>  	 * state violating any number of deferred FK constraints.
>  	 */
>  	uint32_t fk_deferred_count;
> +	struct stailq_entry next;

7. Why 'next'? It is current. 'next' is a part of
struct stailq_entry. Here you basically wrote 'next.next'.
We name queue members 'in_<queue_name>'.

> +	char *name;

8. I didn't like that name is stored by pointer - it consumes more
memory, and is slower. So I changed to name[1] with variable length
tail. But is it an advisory fix, you can skip it if you think it
complicates the code too much. See below and on the branch.

A mandatory remark - please, add a comment. And note here, that the
name is optional. If you stick to my solution, then mention, that an
omitted name is "". If you keep yours, then mention that name == NULL
means omission.

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

diff --git a/src/box/txn.c b/src/box/txn.c
index 451cc9eb1..1b9713348 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -739,7 +739,8 @@ txn_savepoint_new(struct txn *txn, const char *name)
 {
 	assert(txn == in_txn());
 	size_t svp_sz = sizeof(struct txn_savepoint);
-	svp_sz += name != NULL ? strlen(name) + 1 : 0;
+	int name_len = name != NULL ? strlen(name) : 0;
+	svp_sz += name_len;
 	struct txn_savepoint *svp =
 		(struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
 	if (svp == NULL) {
@@ -749,12 +750,10 @@ txn_savepoint_new(struct txn *txn, const char *name)
 	svp->stmt = stailq_last(&txn->stmts);
 	svp->in_sub_stmt = txn->in_sub_stmt;
 	svp->fk_deferred_count = txn->fk_deferred_count;
-	if (name != NULL) {
-		svp->name = (char *) svp + sizeof(struct txn_savepoint);
-		memcpy(svp->name, name, strlen(name) + 1);
-	} else {
-		svp->name = NULL;
-	}
+	if (name != NULL)
+		memcpy(svp->name, name, name_len + 1);
+	else
+		svp->name[0] = 0;
 	stailq_add_tail_entry(&txn->savepoints, svp, next);
 	return svp;
 }
@@ -763,10 +762,8 @@ struct txn_savepoint *
 txn_savepoint_by_name(struct txn *txn, const char *name)
 {
 	assert(txn == in_txn());
-	struct txn_savepoint *sv = NULL;
+	struct txn_savepoint *sv;
 	stailq_foreach_entry(sv, &txn->savepoints, next) {
-		if (sv->name == NULL)
-			continue;
 		if (strcmp(sv->name, name) == 0)
 			return sv;
 	}
diff --git a/src/box/txn.h b/src/box/txn.h
index a14f3871a..4f931f68f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -128,7 +128,7 @@ struct txn_savepoint {
 	 */
 	uint32_t fk_deferred_count;
 	struct stailq_entry next;
-	char *name;
+	char name[1];
 };
 
 extern double too_long_threshold;

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

> @@ -455,6 +451,22 @@ txn_current_stmt(struct txn *txn)
>  	return stailq_entry(stmt, struct txn_stmt, next);
>  }
>  
> +/**
> + * Allocate new savepoint object using region allocator.
> + * Savepoint is allowed to be anonymous (i.e. without
> + * name).
> + */
> +struct txn_savepoint *
> +txn_savepoint_new(struct txn *txn, const char *name);
> +
> +/** Find savepoint by its name in savepoint list. */
> +struct txn_savepoint *
> +txn_savepoint_by_name(struct txn *txn, const char *name);
> +
> +/** Remove given and all prior entries from savepoint list. */

9. I guess, all newer entries. Not prior (= older).

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/08/10 00:01]:
> > +	char *name;
> 
> 8. I didn't like that name is stored by pointer - it consumes more
> memory, and is slower. So I changed to name[1] with variable length
> tail. But is it an advisory fix, you can skip it if you think it
> complicates the code too much. See below and on the branch.

Vlad, please, stop this optimization craziness, come on, how big
of a deal is it to allocate a name once you already parsed the
statement to create this savepoint?


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 1/3] txn: move fk_deferred_count from psql_txn to txn

[n.pettik]

> On 9 Aug 2019, at 23:59, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
>> diff --git a/src/box/txn.h b/src/box/txn.h
>> index 37d90932b..87e880a6a 100644
>> --- a/src/box/txn.h
>> +++ b/src/box/txn.h
>> @@ -213,6 +208,7 @@ struct txn {
>> 	struct trigger fiber_on_stop;
>> 	/** Commit and rollback triggers. */
>> 	struct rlist on_commit, on_rollback;
>> +	uint32_t fk_deferred_count;
> 
> Please, add a comment. A detailed one. That this number is used
> by SQL only, what does it mean 'deferred fk', why is it stored
> in the transaction.

Done:

diff --git a/src/box/txn.h b/src/box/txn.h
index 87e880a6a..f795cb76f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -208,6 +208,17 @@ struct txn {
        struct trigger fiber_on_stop;
        /** Commit and rollback triggers. */
        struct rlist on_commit, on_rollback;
+       /**
+        * This member represents counter of deferred foreign key
+        * violations within transaction. DEFERRED mode means
+        * that until transaction is committed violations are
+        * allowed to appear. However, transaction can't be
+        * committed in presence of violations, i.e. if this
+        * counter is not equal to zero. In the normal mode
+        * violations of FK are checked at the end of each
+        * statement processing. Note that at the moment it is
+        * SQL specific property.
+        */
        uint32_t fk_deferred_count;
        struct sql_txn *psql_txn;
 };

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[n.pettik]

> On 10 Aug 2019, at 00:02, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> Thanks for the patch! See 9 comments below.
> On 07/08/2019 17:13, Nikita Pettik wrote:
>> This procedure is processed in a several steps. Firstly, we add name
> 
> 1. Typo: 'a' is not used with plural.

Shame on me, fixed.

>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 246654cb7..7d05ce11c 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -2867,8 +2859,7 @@ case OP_Savepoint: {
>> 			 * and this is a RELEASE command, then the current transaction
>> 			 * is committed.
>> 			 */
>> -			int isTransaction = pSavepoint->pNext == 0;
>> -			if (isTransaction && p1==SAVEPOINT_RELEASE) {
>> +			if (p1 == SAVEPOINT_RELEASE) {
> 
> 2. First, the comment is outdated now (perhaps).
> Second, I don't understand that code. Why release
> of a savepoint
> 
> 1) affects fk? Because it is not rollback, it is just release. It
> does not rollback anything, and only frees savepoint objects (freed,
> when it was on malloc).
> 
> 2) leads to halt?

Have no idea either. I just didn’t touch that code.
Ok, since we started to refactor OP_Savepoint,
let’s remove these strange routines as well. 

>> 				if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
>> 					goto vdbe_return;
>> 				sqlVdbeHalt(p);
>> @@ -2876,19 +2867,8 @@ case OP_Savepoint: {
>> 					goto abort_due_to_error;
>> 			} else {
>> 				if (p1==SAVEPOINT_ROLLBACK)
>> -					box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint);
>> -			}
>> -
>> -			/* Regardless of whether this is a RELEASE or ROLLBACK, destroy all
>> -			 * savepoints nested inside of the savepoint being operated on.
>> -			 */
>> -			while (psql_txn->pSavepoint != pSavepoint) {
>> -				pTmp = psql_txn->pSavepoint;
>> -				psql_txn->pSavepoint = pTmp->pNext;
>> -				/*
>> -				 * Since savepoints are stored in region, we do not
>> -				 * have to destroy them
>> -				 */
>> +					if (box_txn_rollback_to_savepoint(sv) != 0)
>> +						goto abort_due_to_error;
> 
> 3. Something is wrong with braces now. The top 'if' has two lines
> in body, but has no braces. Honestly, the whole code block looks bad.
> Please, consider these changes. They are below and on top of the branch
> in a separate commit.

Seems good, applied.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 9165db6c3..7b0e450c5 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2853,34 +2853,19 @@ case OP_Savepoint: {
> 		if (sv == NULL) {
> 			diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
> 			goto abort_due_to_error;
> +		}
> +		if (p1 == SAVEPOINT_RELEASE) {
> +			if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
> +				goto vdbe_return;
> +			sqlVdbeHalt(p);
> +			if (p->is_aborted)
> +				goto abort_due_to_error;
> +			txn_savepoint_release(sv);
> 		} else {
> -
> -			/* Determine whether or not this is a transaction savepoint. If so,
> -			 * and this is a RELEASE command, then the current transaction
> -			 * is committed.
> -			 */
> -			if (p1 == SAVEPOINT_RELEASE) {
> -				if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
> -					goto vdbe_return;
> -				sqlVdbeHalt(p);
> -				if (p->is_aborted)
> -					goto abort_due_to_error;
> -			} else {
> -				if (p1==SAVEPOINT_ROLLBACK)
> -					if (box_txn_rollback_to_savepoint(sv) != 0)
> -						goto abort_due_to_error;
> -			}
> -
> -			/* If it is a RELEASE, then destroy the savepoint being operated on
> -			 * too. If it is a ROLLBACK TO, then set the number of deferred
> -			 * constraint violations present in the database to the value stored
> -			 * when the savepoint was created.
> -			 */
> -			if (p1==SAVEPOINT_RELEASE) {
> -				txn_savepoint_release(sv);
> -			} else {
> -				txn->fk_deferred_count = sv->fk_deferred_count;
> -			}
> +			assert(p1 == SAVEPOINT_ROLLBACK);
> +			if (box_txn_rollback_to_savepoint(sv) != 0)
> +				goto abort_due_to_error;
> +			txn->fk_deferred_count = sv->fk_deferred_count;
> 		}
> 	}
> 
> =================================================================================
> 
>> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
>> index 8f16202ba..06f258805 100644
>> --- a/src/box/sql/vdbe.h
>> +++ b/src/box/sql/vdbe.h
>> @@ -172,15 +172,6 @@ struct SubProgram {
>>  */
>> Vdbe *sqlVdbeCreate(Parse *);
>> 
>> -/**
>> - * Allocate and initialize SQL-specific struct which completes
>> - * original Tarantool's txn struct using region allocator.
>> - *
>> - * @retval NULL on OOM, new psql_txn struct on success.
>> - **/
>> -struct sql_txn *
>> -sql_alloc_txn();
> 
> 4. Please, drop sql_txn_begin too.

OK:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7d05ce11c..cb7b585e2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2918,7 +2918,11 @@ case OP_CheckViewReferences: {
  * Otherwise, raise an error with appropriate error message.
  */
 case OP_TransactionBegin: {
-       if (sql_txn_begin() != 0)
+       if (in_txn()) {
+               diag_set(ClientError, ER_ACTIVE_TRANSACTION);
+               goto abort_due_to_error;
+       }
+       if (txn_begin() == NULL)
                goto abort_due_to_error;
        p->auto_commit = false  ;
        break;
@@ -2974,7 +2978,7 @@ case OP_TransactionRollback: {
  */
 case OP_TTransaction: {
        if (!box_txn()) {
-               if (sql_txn_begin() != 0)
+               if (txn_begin() == NULL)
                        goto abort_due_to_error;
        } else {
                p->anonymous_savepoint = sql_savepoint(p, NULL);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 5582d9506..3050f75b7 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -453,8 +453,7 @@ u32 sqlVdbeSerialGet(const unsigned char *, u32, Mem *);
 
 int sqlVdbeExec(Vdbe *);
 int sqlVdbeList(Vdbe *);
-int
-sql_txn_begin();
+
 Savepoint *
 sql_savepoint(Vdbe *p,
              const char *zName);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3d256f86a..b77fffaec 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1971,19 +1971,6 @@ sqlVdbeCheckFk(Vdbe * p, int deferred)
        return 0;
 }
 
-int
-sql_txn_begin()
-{
-       if (in_txn()) {
-               diag_set(ClientError, ER_ACTIVE_TRANSACTION);
-               return -1;
-       }
-       struct txn *ptxn = txn_begin(false);
-       if (ptxn == NULL)
-               return -1;
-       return 0;
-}
-
 Savepoint *
 sql_savepoint(MAYBE_UNUSED Vdbe *p, const char *zName)
 {

>> diff --git a/src/box/txn.c b/src/box/txn.c
>> index b57240846..451cc9eb1 100644
>> --- a/src/box/txn.c
>> +++ b/src/box/txn.c
>> @@ -779,10 +807,28 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
>> 		return -1;
>> 	}
>> 	txn_rollback_to_svp(txn, svp->stmt);
>> +	/* Discard from list all prior savepoints. */
>> +	struct stailq discard;
>> +	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
>> 	txn->fk_deferred_count = svp->fk_deferred_count;
>> 	return 0;
>> }
>> 
>> +void
>> +txn_savepoint_release(struct txn_savepoint *svp)
>> +{
>> +	struct txn *txn = in_txn();
>> +	assert(txn != NULL);
> 
> 5. Please, add an assertion, that this savepoint is not released
> yet. I.e. that its statement has non NULL space and row.

Ok, but we should also consider savepoint containing
empty statements list (like SAVEPOINT t1; RELEASE SAVEPOINT t1;):

diff --git a/src/box/txn.c b/src/box/txn.c
index 451cc9eb1..b112d3ad9 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -819,6 +819,11 @@ txn_savepoint_release(struct txn_savepoint *svp)
 {
        struct txn *txn = in_txn();
        assert(txn != NULL);
+       /* Make sure that savepoint hasn't been released yet. */
+       assert(svp->in_sub_stmt == 0 ||
+              (stailq_entry(svp->stmt, struct txn_stmt, next)->space != NULL &&
+               stailq_entry(svp->stmt, struct txn_stmt, next)->row != NULL));

>> +	/*
>> +	 * Discard current savepoint alongside with all
>> +	 * created after it savepoints. Note that operation
>> +	 * is currently available only for SQL.
>> +	 */
>> +	struct stailq discard;
>> +	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
>> +	stailq_cut_tail(&txn->savepoints, *txn->savepoints.last, &discard);
> 
> 6. Sorry, I failed to understand what is happening here, and how
> does it work. Why do you touch internal attributte of 'struct stailq',
> and why the first call is not enough?

stailq_cut_tail() cuts tail starting from the next entry, so
it doesn’t remove savepoint itself from list. Yep, this code is
obviously wrong (heh but our test suite doesn’t include test
verifying it; surely it is my bad. But I had no doubts that such
simple case is tested) since *last == NULL, so list of savepoints
is discarded entirely. Correct way is to find predecessor traversing the list:

diff --git a/src/box/txn.c b/src/box/txn.c
index 8fc5f182e..aef655df2 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -820,15 +820,25 @@ txn_savepoint_release(struct txn_savepoint *svp)
        /*
         * Discard current savepoint alongside with all
-        * created after it savepoints. Note that operation
-        * is currently available only for SQL.
+        * created after it savepoints. To do so we should
+        * firstly find predecessor of the original node. Note
+        * that operation is currently available only for SQL.
         */
+       struct stailq_entry *prev;
+       stailq_foreach(prev, &txn->savepoints) {
+               /*
+                * If savepoint to be removed is the first in the
+                * list, prev will be equal to NULL after
+                * iterations. So _cut_tail() is going to erase
+                * the whole list.
+                */
+               if (stailq_next(prev) == &svp->link)
+                       break;
+       }
        struct stailq discard;
-       stailq_cut_tail(&txn->savepoints, &svp->link, &discard);
-       stailq_cut_tail(&txn->savepoints, *txn->savepoints.last, &discard);
+       stailq_cut_tail(&txn->savepoints, prev, &discard);
 }
 
 static void

And test case I am talking about:

diff --git a/test/sql/savepoints.test.lua b/test/sql/savepoints.test.lua
index d96b04600..a4312c114 100644
--- a/test/sql/savepoints.test.lua
+++ b/test/sql/savepoints.test.lua
@@ -31,6 +31,18 @@ end;
 release_sv();
 box.commit();
 
+release_sv_2 = function()
+    box.begin()
+    box.execute('SAVEPOINT t1;')
+    box.execute('SAVEPOINT t2;')
+    box.execute('SAVEPOINT t3;')
+    box.execute('RELEASE SAVEPOINT t2;')
+    local _, err = box.execute('ROLLBACK TO t1;')
+    assert(err == nil)
+end;
+release_sv_2();
+box.commit();
+

>> diff --git a/src/box/txn.h b/src/box/txn.h
>> index 87e880a6a..a14f3871a 100644
>> --- a/src/box/txn.h
>> +++ b/src/box/txn.h
>> @@ -128,15 +127,12 @@ struct txn_savepoint {
>> 	 * state violating any number of deferred FK constraints.
>> 	 */
>> 	uint32_t fk_deferred_count;
>> +	struct stailq_entry next;
> 
> 7. Why 'next'? It is current. 'next' is a part of
> struct stailq_entry. Here you basically wrote 'next.next'.
> We name queue members 'in_<queue_name>’.

However, ’next' prefix is used as well. Ofc I looked through
source code before choosing a name :)

struct stailq_entry next_in_log; in struct txv
struct stailq_entry next; in struct vy_squash
struct stailq_entry next; in struct txn_stmt
struct stailq_entry next; in struct applier_tx_row

Even in test/unit/stailq.c this member is called ’next’.

On second thought, I can say that ’next’ really seems to be
a bit confusing. There’s also ‘link’ name:

struct stailq_entry link; in struct memtx_gc_task
struct stailq_entry link; in truct autoinc_id_entry

IMHO ‘in_svps' sounds worse than just ‘link’. If you don't
mind I'll rename it to link.

>> +	char *name;
> 
> 8. I didn't like that name is stored by pointer - it consumes more
> memory, and is slower. So I changed to name[1] with variable length
> tail. But is it an advisory fix, you can skip it if you think it
> complicates the code too much. See below and on the branch.
> 
> A mandatory remark - please, add a comment. And note here, that the
> name is optional. If you stick to my solution, then mention, that an
> omitted name is "". If you keep yours, then mention that name == NULL
> means omission.

Ok, I don’t mind your changes: I also tried both versions
when was preparing patch, but failed to decide which is
better. I don’t think that your version would get any performance
gain but I like that we don’t need check NULL pointer while
iterating over the list and comparing names.

Comment to the “name” field:

diff --git a/src/box/txn.h b/src/box/txn.h
index 06092e3f9..3d378ce29 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -127,7 +127,16 @@ struct txn_savepoint {
         * state violating any number of deferred FK constraints.
         */
        uint32_t fk_deferred_count;
+       /** Organize savepoints into linked list. */
        struct stailq_entry link;
+       /**
+        * Optional name of savepoint. If savepoint lacks
+        * name (i.e. anonymous savepoint available only by
+        * reference to the object), name[0] == ''. Otherwise,
+        * memory for name is reserved in the same memory chunk
+        * as struct txn_savepoint itself - name is placed
+        * right after structure (see txn_savepoint_new()).
+        */
        char name[1];
 };

>> @@ -455,6 +451,22 @@ txn_current_stmt(struct txn *txn)
>> 	return stailq_entry(stmt, struct txn_stmt, next);
>> }
>> 
>> +/**
>> + * Allocate new savepoint object using region allocator.
>> + * Savepoint is allowed to be anonymous (i.e. without
>> + * name).
>> + */
>> +struct txn_savepoint *
>> +txn_savepoint_new(struct txn *txn, const char *name);
>> +
>> +/** Find savepoint by its name in savepoint list. */
>> +struct txn_savepoint *
>> +txn_savepoint_by_name(struct txn *txn, const char *name);
>> +
>> +/** Remove given and all prior entries from savepoint list. */
> 
> 9. I guess, all newer entries. Not prior (= older).

Yep, fixed:

diff --git a/src/box/txn.h b/src/box/txn.h
index 50114a4a9..32d7f83f5 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -474,7 +474,7 @@ txn_savepoint_new(struct txn *txn, const char *name);
 struct txn_savepoint *
 txn_savepoint_by_name(struct txn *txn, const char *name);
 
-/** Remove given and all prior entries from savepoint list. */
+/** Remove given and all newer entries from savepoint list. */
 void
 txn_savepoint_release(struct txn_savepoint *svp);

Whole patch:

Author: Nikita Pettik <korablev@tarantool.org>
Date:   Fri Aug 2 19:40:55 2019 +0300

    txn: merge struct sql_txn into struct txn
    
    This procedure is processed in several steps. Firstly, we add name
    to struct txn_savepoint since we should be capable of operating on named
    savepoints (which in turn is SQL feature). Still, anonymous (in the sense
    of name absence) savepoints are also valid. Then, we add list (as
    implementation of stailq) of savepoints to struct txn: it allows us to
    find savepoint by its name. Finally, we patch rollback to/release
    savepoint routines: for rollback tail of the list containing savepoints
    is cut (but subject of rollback routine remains in the list); for
    release routine we cut tail including node being released.

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 246654cb7..0f4a160a7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2823,13 +2823,9 @@ case OP_Count: {         /* out2 */
 case OP_Savepoint: {
        int p1;                         /* Value of P1 operand */
        char *zName;                    /* Name of savepoint */
-       Savepoint *pNew;
-       Savepoint *pSavepoint;
-       Savepoint *pTmp;
        struct txn *txn = in_txn();
-       struct sql_txn *psql_txn = txn != NULL ? txn->psql_txn : NULL;
 
-       if (psql_txn == NULL) {
+       if (txn == NULL) {
                assert(!box_txn());
                diag_set(ClientError, ER_NO_TRANSACTION);
                goto abort_due_to_error;
@@ -2840,69 +2836,31 @@ case OP_Savepoint: {
        /* Assert that the p1 parameter is valid. Also that if there is no open
         * transaction, then there cannot be any savepoints.
         */
-       assert(psql_txn->pSavepoint == NULL || box_txn());
+       assert(stailq_empty(&txn->savepoints) || box_txn());
        assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
 
        if (p1==SAVEPOINT_BEGIN) {
-               /* Create a new savepoint structure. */
-               pNew = sql_savepoint(p, zName);
-               /* Link the new savepoint into the database handle's list. */
-               pNew->pNext = psql_txn->pSavepoint;
-               psql_txn->pSavepoint = pNew;
+               /*
+                * Savepoint is available by its name so we don't
+                * care about object itself.
+                */
+               (void) txn_savepoint_new(txn, zName);
        } else {
                /* Find the named savepoint. If there is no such savepoint, then an
                 * an error is returned to the user.
                 */
-               for(
-                       pSavepoint = psql_txn->pSavepoint;
-                       pSavepoint && sqlStrICmp(pSavepoint->zName, zName);
-                       pSavepoint = pSavepoint->pNext
-                       );
-               if (!pSavepoint) {
+               struct txn_savepoint *sv = txn_savepoint_by_name(txn, zName);
+               if (sv == NULL) {
                        diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
                        goto abort_due_to_error;
+               }
+               if (p1 == SAVEPOINT_RELEASE) {
+                       txn_savepoint_release(sv);
                } else {
-
-                       /* Determine whether or not this is a transaction savepoint. If so,
-                        * and this is a RELEASE command, then the current transaction
-                        * is committed.
-                        */
-                       int isTransaction = pSavepoint->pNext == 0;
-                       if (isTransaction && p1==SAVEPOINT_RELEASE) {
-                               if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
-                                       goto vdbe_return;
-                               sqlVdbeHalt(p);
-                               if (p->is_aborted)
-                                       goto abort_due_to_error;
-                       } else {
-                               if (p1==SAVEPOINT_ROLLBACK)
-                                       box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint);
-                       }
-
-                       /* Regardless of whether this is a RELEASE or ROLLBACK, destroy all
-                        * savepoints nested inside of the savepoint being operated on.
-                        */
-                       while (psql_txn->pSavepoint != pSavepoint) {
-                               pTmp = psql_txn->pSavepoint;
-                               psql_txn->pSavepoint = pTmp->pNext;
-                               /*
-                                * Since savepoints are stored in region, we do not
-                                * have to destroy them
-                                */
-                       }
-
-                       /* If it is a RELEASE, then destroy the savepoint being operated on
-                        * too. If it is a ROLLBACK TO, then set the number of deferred
-                        * constraint violations present in the database to the value stored
-                        * when the savepoint was created.
-                        */
-                       if (p1==SAVEPOINT_RELEASE) {
-                               assert(pSavepoint == psql_txn->pSavepoint);
-                               psql_txn->pSavepoint = pSavepoint->pNext;
-                       } else {
-                               txn->fk_deferred_count =
-                                       pSavepoint->tnt_savepoint->fk_deferred_count;
-                       }
+                       assert(p1 == SAVEPOINT_ROLLBACK);
+                       if (box_txn_rollback_to_savepoint(sv) != 0)
+                               goto abort_due_to_error;
+                       txn->fk_deferred_count = sv->fk_deferred_count;
                }
        }
 
@@ -2940,7 +2898,11 @@ case OP_CheckViewReferences: {
  * Otherwise, raise an error with appropriate error message.
  */
 case OP_TransactionBegin: {
-       if (sql_txn_begin() != 0)
+       if (in_txn()) {
+               diag_set(ClientError, ER_ACTIVE_TRANSACTION);
+               goto abort_due_to_error;
+       }
+       if (txn_begin() == NULL)
                goto abort_due_to_error;
        p->auto_commit = false  ;
        break;
@@ -2996,7 +2958,7 @@ case OP_TransactionRollback: {
  */
 case OP_TTransaction: {
        if (!box_txn()) {
-               if (sql_txn_begin() != 0)
+               if (txn_begin() == NULL)
                        goto abort_due_to_error;
        } else {
                p->anonymous_savepoint = sql_savepoint(p, NULL);
@@ -4843,7 +4805,6 @@ case OP_FkCounter: {
        if (((p->sql_flags & SQL_DeferFKs) != 0 || pOp->p1 != 0) &&
            !p->auto_commit) {
                struct txn *txn = in_txn();
-               assert(txn != NULL && txn->psql_txn != NULL);
                txn->fk_deferred_count += pOp->p2;
        } else {
                p->nFkConstraint += pOp->p2;
@@ -4867,7 +4828,6 @@ case OP_FkIfZero: {         /* jump */
        if (((p->sql_flags & SQL_DeferFKs) != 0 || pOp->p1 != 0) &&
            !p->auto_commit) {
                struct txn *txn = in_txn();
-               assert(txn != NULL && txn->psql_txn != NULL);
                if (txn->fk_deferred_count == 0)
                        goto jump_to_p2;
        } else {
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 8f16202ba..06f258805 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -172,15 +172,6 @@ struct SubProgram {
  */
 Vdbe *sqlVdbeCreate(Parse *);
 
-/**
- * Allocate and initialize SQL-specific struct which completes
- * original Tarantool's txn struct using region allocator.
- *
- * @retval NULL on OOM, new psql_txn struct on success.
- **/
-struct sql_txn *
-sql_alloc_txn();
-
 /**
  * Prepare given VDBE to execution: initialize structs connected
  * with transaction routine: autocommit mode, deferred foreign
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 5582d9506..3050f75b7 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -453,8 +453,7 @@ u32 sqlVdbeSerialGet(const unsigned char *, u32, Mem *);
 
 int sqlVdbeExec(Vdbe *);
 int sqlVdbeList(Vdbe *);
-int
-sql_txn_begin();
+
 Savepoint *
 sql_savepoint(Vdbe *p,
              const char *zName);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index bbbb99c70..b77fffaec 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -76,39 +76,12 @@ sqlVdbeCreate(Parse * pParse)
        return p;
 }
 
-struct sql_txn *
-sql_alloc_txn()
-{
-       struct sql_txn *txn = region_alloc_object(&fiber()->gc,
-                                                 struct sql_txn);
-       if (txn == NULL) {
-               diag_set(OutOfMemory, sizeof(struct sql_txn), "region",
-                        "struct sql_txn");
-               return NULL;
-       }
-       txn->pSavepoint = NULL;
-       return txn;
-}
-
 int
 sql_vdbe_prepare(struct Vdbe *vdbe)
 {
        assert(vdbe != NULL);
        struct txn *txn = in_txn();
        vdbe->auto_commit = txn == NULL;
-       if (txn != NULL) {
-               /*
-                * If transaction has been started in Lua, then
-                * sql_txn is NULL. On the other hand, it is not
-                * critical, since in Lua it is impossible to
-                * check FK violations, at least now.
-                */
-               if (txn->psql_txn == NULL) {
-                       txn->psql_txn = sql_alloc_txn();
-                       if (txn->psql_txn == NULL)
-                               return -1;
-               }
-       }
        return 0;
 }
 
@@ -1998,24 +1971,6 @@ sqlVdbeCheckFk(Vdbe * p, int deferred)
        return 0;
 }
 
-int
-sql_txn_begin()
-{
-       if (in_txn()) {
-               diag_set(ClientError, ER_ACTIVE_TRANSACTION);
-               return -1;
-       }
-       struct txn *ptxn = txn_begin(false);
-       if (ptxn == NULL)
-               return -1;
-       ptxn->psql_txn = sql_alloc_txn();
-       if (ptxn->psql_txn == NULL) {
-               box_txn_rollback();
-               return -1;
-       }
-       return 0;
-}
-
 Savepoint *
 sql_savepoint(MAYBE_UNUSED Vdbe *p, const char *zName)
 {
diff --git a/src/box/txn.c b/src/box/txn.c
index b57240846..aef655df2 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -221,8 +221,8 @@ txn_begin()
        txn->signature = -1;
        txn->engine = NULL;
        txn->engine_tx = NULL;
-       txn->psql_txn = NULL;
        txn->fk_deferred_count = 0;
+       stailq_create(&txn->savepoints);
        txn->fiber = NULL;
        fiber_set_txn(fiber(), txn);
        /* fiber_on_yield is initialized by engine on demand */
@@ -734,28 +734,53 @@ box_txn_alloc(size_t size)
                                    alignof(union natural_align));
 }
 
-box_txn_savepoint_t *
-box_txn_savepoint()
+struct txn_savepoint *
+txn_savepoint_new(struct txn *txn, const char *name)
 {
-       struct txn *txn = in_txn();
-       if (txn == NULL) {
-               diag_set(ClientError, ER_NO_TRANSACTION);
-               return NULL;
-       }
+       assert(txn == in_txn());
+       size_t svp_sz = sizeof(struct txn_savepoint);
+       int name_len = name != NULL ? strlen(name) : 0;
+       svp_sz += name_len;
        struct txn_savepoint *svp =
-               (struct txn_savepoint *) region_alloc_object(&txn->region,
-                                                       struct txn_savepoint);
+               (struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
        if (svp == NULL) {
-               diag_set(OutOfMemory, sizeof(*svp),
-                        "region", "struct txn_savepoint");
+               diag_set(OutOfMemory, svp_sz, "region", "svp");
                return NULL;
        }
        svp->stmt = stailq_last(&txn->stmts);
        svp->in_sub_stmt = txn->in_sub_stmt;
        svp->fk_deferred_count = txn->fk_deferred_count;
+       if (name != NULL)
+               memcpy(svp->name, name, name_len + 1);
+       else
+               svp->name[0] = 0;
+       stailq_add_tail_entry(&txn->savepoints, svp, link);
        return svp;
 }
 
+struct txn_savepoint *
+txn_savepoint_by_name(struct txn *txn, const char *name)
+{
+       assert(txn == in_txn());
+       struct txn_savepoint *sv;
+       stailq_foreach_entry(sv, &txn->savepoints, link) {
+               if (strcmp(sv->name, name) == 0)
+                       return sv;
+       }
+       return NULL;
+}
+
+box_txn_savepoint_t *
+box_txn_savepoint()
+{
+       struct txn *txn = in_txn();
+       if (txn == NULL) {
+               diag_set(ClientError, ER_NO_TRANSACTION);
+               return NULL;
+       }
+       return txn_savepoint_new(txn, NULL);
+}
+
 int
 box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 {
@@ -779,10 +804,43 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
                return -1;
        }
        txn_rollback_to_svp(txn, svp->stmt);
+       /* Discard from list all prior savepoints. */
+       struct stailq discard;
+       stailq_cut_tail(&txn->savepoints, &svp->link, &discard);
        txn->fk_deferred_count = svp->fk_deferred_count;
        return 0;
 }
 
+void
+txn_savepoint_release(struct txn_savepoint *svp)
+{
+       struct txn *txn = in_txn();
+       assert(txn != NULL);
+       /* Make sure that savepoint hasn't been released yet. */
+       assert(svp->in_sub_stmt == 0 ||
+              (stailq_entry(svp->stmt, struct txn_stmt, next)->space != NULL &&
+               stailq_entry(svp->stmt, struct txn_stmt, next)->row != NULL));
+       /*
+        * Discard current savepoint alongside with all
+        * created after it savepoints. To do so we should
+        * firstly find predecessor of the original node. Note
+        * that operation is currently available only for SQL.
+        */
+       struct stailq_entry *prev;
+       stailq_foreach(prev, &txn->savepoints) {
+               /*
+                * If savepoint to be removed is the first in the
+                * list, prev will be equal to NULL after
+                *iterations. So _cut_tail() is going to erase
+                * the whole list.
+                */
+               if (stailq_next(prev) == &svp->link)
+                       break;
+       }
+       struct stailq discard;
+       stailq_cut_tail(&txn->savepoints, prev, &discard);
+}
+
 static void
 txn_on_stop(struct trigger *trigger, void *event)
 {
diff --git a/src/box/txn.h b/src/box/txn.h
index f795cb76f..3d378ce29 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -43,7 +43,6 @@ extern "C" {
 
 /** box statistics */
 extern struct rmean *rmean_box;
-struct Savepoint;
 
 struct engine;
 struct space;
@@ -128,15 +127,21 @@ struct txn_savepoint {
         * state violating any number of deferred FK constraints.
         */
        uint32_t fk_deferred_count;
+       /** Organize savepoints into linked list. */
+       struct stailq_entry link;
+       /**
+        * Optional name of savepoint. If savepoint lacks
+        * name (i.e. anonymous savepoint available only by
+        * reference to the object), name[0] == ''. Otherwise,
+        * memory for name is reserved in the same memory chunk
+        * as struct txn_savepoint itself - name is placed
+        * right after structure (see txn_savepoint_new()).
+        */
+       char name[1];
 };
 
 extern double too_long_threshold;
 
-struct sql_txn {
-       /** List of active SQL savepoints. */
-       struct Savepoint *pSavepoint;
-};
-
 /**
  * An element of list of autogenerated ids, being returned as SQL
  * response metadata.
@@ -220,7 +225,7 @@ struct txn {
         * SQL specific property.
         */
        uint32_t fk_deferred_count;
-       struct sql_txn *psql_txn;
+       struct stailq savepoints;
 };
 
 static inline bool
@@ -466,6 +471,22 @@ txn_current_stmt(struct txn *txn)
        return stailq_entry(stmt, struct txn_stmt, next);
 }
 
+/**
+ * Allocate new savepoint object using region allocator.
+ * Savepoint is allowed to be anonymous (i.e. without
+ * name).
+ */
+struct txn_savepoint *
+txn_savepoint_new(struct txn *txn, const char *name);
+
+/** Find savepoint by its name in savepoint list. */
+struct txn_savepoint *
+txn_savepoint_by_name(struct txn *txn, const char *name);
+
+/** Remove given and all newer entries from savepoint list. */
+void
+txn_savepoint_release(struct txn_savepoint *svp);
+
 /**
  * FFI bindings: do not throw exceptions, do not accept extra
  * arguments
diff --git a/test/sql/savepoints.result b/test/sql/savepoints.result
index 78957173e..b5a0b7f46 100644
--- a/test/sql/savepoints.result
+++ b/test/sql/savepoints.result
@@ -58,6 +58,23 @@ release_sv();
 box.commit();
 ---
 ...
+release_sv_2 = function()
+    box.begin()
+    box.execute('SAVEPOINT t1;')
+    box.execute('SAVEPOINT t2;')
+    box.execute('SAVEPOINT t3;')
+    box.execute('RELEASE SAVEPOINT t2;')
+    local _, err = box.execute('ROLLBACK TO t1;')
+    assert(err == nil)
+end;
+---
+...
+release_sv_2();
+---
+...
+box.commit();
+---
+...
 release_sv_fail = function()
     box.begin()
     box.execute('SAVEPOINT t1;')
diff --git a/test/sql/savepoints.test.lua b/test/sql/savepoints.test.lua
index d96b04600..a4312c114 100644
--- a/test/sql/savepoints.test.lua
+++ b/test/sql/savepoints.test.lua
@@ -31,6 +31,18 @@ end;
 release_sv();
 box.commit();
 
+release_sv_2 = function()
+    box.begin()
+    box.execute('SAVEPOINT t1;')
+    box.execute('SAVEPOINT t2;')
+    box.execute('SAVEPOINT t3;')
+    box.execute('RELEASE SAVEPOINT t2;')
+    local _, err = box.execute('ROLLBACK TO t1;')
+    assert(err == nil)
+end;
+release_sv_2();
+box.commit();
+
 release_sv_fail = function()
     box.begin()
     box.execute('SAVEPOINT t1;')

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

See 5 comments below.

>>> diff --git a/src/box/txn.c b/src/box/txn.c
>>> index b57240846..451cc9eb1 100644
>>> --- a/src/box/txn.c
>>> +++ b/src/box/txn.c
>>> @@ -779,10 +807,28 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
>>> 		return -1;
>>> 	}
>>> 	txn_rollback_to_svp(txn, svp->stmt);
>>> +	/* Discard from list all prior savepoints. */

1. Again 'prior'. Should be 'newer', please.

>>> +	struct stailq discard;
>>> +	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
>>> 	txn->fk_deferred_count = svp->fk_deferred_count;
>>> 	return 0;
>>> }
>>>
>>> +void
>>> +txn_savepoint_release(struct txn_savepoint *svp)
>>> +{
>>> +	struct txn *txn = in_txn();
>>> +	assert(txn != NULL);
>>
>> 5. Please, add an assertion, that this savepoint is not released
>> yet. I.e. that its statement has non NULL space and row.
> 
> Ok, but we should also consider savepoint containing
> empty statements list (like SAVEPOINT t1; RELEASE SAVEPOINT t1;):
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 451cc9eb1..b112d3ad9 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -819,6 +819,11 @@ txn_savepoint_release(struct txn_savepoint *svp)
>  {
>         struct txn *txn = in_txn();
>         assert(txn != NULL);
> +       /* Make sure that savepoint hasn't been released yet. */
> +       assert(svp->in_sub_stmt == 0 ||

2. As far as I know, in_sub_stmt is always 0 between two statements.
(Unless they are in a trigger of another statement). This is because
txn_begin_stmt increments in_sub_stmt and txn_commit_stmt decrements.
This makes the assertion below always passing regardless of space and
row values, doesn't it? Not only for a first statement.

>>> diff --git a/src/box/txn.h b/src/box/txn.h
>>> index 87e880a6a..a14f3871a 100644
>>> --- a/src/box/txn.h
>>> +++ b/src/box/txn.h
>>> @@ -128,15 +127,12 @@ struct txn_savepoint {
>>> 	 * state violating any number of deferred FK constraints.
>>> 	 */
>>> 	uint32_t fk_deferred_count;
>>> +	struct stailq_entry next;
>>
>> 7. Why 'next'? It is current. 'next' is a part of
>> struct stailq_entry. Here you basically wrote 'next.next'.
>> We name queue members 'in_<queue_name>’.
> 
> However, ’next' prefix is used as well. Ofc I looked through
> source code before choosing a name :)
> 
> struct stailq_entry next_in_log; in struct txv
> struct stailq_entry next; in struct vy_squash
> struct stailq_entry next; in struct txn_stmt
> struct stailq_entry next; in struct applier_tx_row
> 
> Even in test/unit/stailq.c this member is called ’next’.
> 
> On second thought, I can say that ’next’ really seems to be
> a bit confusing. There’s also ‘link’ name:
> 
> struct stailq_entry link; in struct memtx_gc_task
> struct stailq_entry link; in truct autoinc_id_entry
> 
> IMHO ‘in_svps' sounds worse than just ‘link’. If you don't
> mind I'll rename it to link.

Thanks for the investigation. Ok, if 'next' is a really
household name here, then you can keep it. Up to you.

> Author: Nikita Pettik <korablev@tarantool.org>
> Date:   Fri Aug 2 19:40:55 2019 +0300
> 
>     txn: merge struct sql_txn into struct txn
>     
>     This procedure is processed in several steps. Firstly, we add name
>     to struct txn_savepoint since we should be capable of operating on named
>     savepoints (which in turn is SQL feature). Still, anonymous (in the sense
>     of name absence) savepoints are also valid. Then, we add list (as
>     implementation of stailq) of savepoints to struct txn: it allows us to
>     find savepoint by its name. Finally, we patch rollback to/release
>     savepoint routines: for rollback tail of the list containing savepoints
>     is cut (but subject of rollback routine remains in the list); for
>     release routine we cut tail including node being released.
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 246654cb7..0f4a160a7 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2840,69 +2836,31 @@ case OP_Savepoint: {
>         /* Assert that the p1 parameter is valid. Also that if there is no open
>          * transaction, then there cannot be any savepoints.
>          */
> -       assert(psql_txn->pSavepoint == NULL || box_txn());
> +       assert(stailq_empty(&txn->savepoints) || box_txn());
>         assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
>  
>         if (p1==SAVEPOINT_BEGIN) {
> -               /* Create a new savepoint structure. */
> -               pNew = sql_savepoint(p, zName);
> -               /* Link the new savepoint into the database handle's list. */
> -               pNew->pNext = psql_txn->pSavepoint;
> -               psql_txn->pSavepoint = pNew;
> +               /*
> +                * Savepoint is available by its name so we don't
> +                * care about object itself.
> +                */
> +               (void) txn_savepoint_new(txn, zName);

3. You still need to check for NULL in case of OOM.

>         } else {
>                 /* Find the named savepoint. If there is no such savepoint, then an
>                  * an error is returned to the user.
>                  */
> -               for(
> -                       pSavepoint = psql_txn->pSavepoint;
> -                       pSavepoint && sqlStrICmp(pSavepoint->zName, zName);
> -                       pSavepoint = pSavepoint->pNext
> -                       );
> -               if (!pSavepoint) {
> +               struct txn_savepoint *sv = txn_savepoint_by_name(txn, zName);
> +               if (sv == NULL) {
>                         diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
>                         goto abort_due_to_error;
> +               }
> +               if (p1 == SAVEPOINT_RELEASE) {
> +                       txn_savepoint_release(sv);
>                 } else {
> -
> -                       /* Determine whether or not this is a transaction savepoint. If so,
> -                        * and this is a RELEASE command, then the current transaction
> -                        * is committed.
> -                        */
> -                       int isTransaction = pSavepoint->pNext == 0;
> -                       if (isTransaction && p1==SAVEPOINT_RELEASE) {
> -                               if ((rc = sqlVdbeCheckFk(p, 1)) != 0)
> -                                       goto vdbe_return;
> -                               sqlVdbeHalt(p);
> -                               if (p->is_aborted)
> -                                       goto abort_due_to_error;
> -                       } else {
> -                               if (p1==SAVEPOINT_ROLLBACK)
> -                                       box_txn_rollback_to_savepoint(pSavepoint->tnt_savepoint);
> -                       }
> -
> -                       /* Regardless of whether this is a RELEASE or ROLLBACK, destroy all
> -                        * savepoints nested inside of the savepoint being operated on.
> -                        */
> -                       while (psql_txn->pSavepoint != pSavepoint) {
> -                               pTmp = psql_txn->pSavepoint;
> -                               psql_txn->pSavepoint = pTmp->pNext;
> -                               /*
> -                                * Since savepoints are stored in region, we do not
> -                                * have to destroy them
> -                                */
> -                       }
> -
> -                       /* If it is a RELEASE, then destroy the savepoint being operated on
> -                        * too. If it is a ROLLBACK TO, then set the number of deferred
> -                        * constraint violations present in the database to the value stored
> -                        * when the savepoint was created.
> -                        */
> -                       if (p1==SAVEPOINT_RELEASE) {
> -                               assert(pSavepoint == psql_txn->pSavepoint);
> -                               psql_txn->pSavepoint = pSavepoint->pNext;
> -                       } else {
> -                               txn->fk_deferred_count =
> -                                       pSavepoint->tnt_savepoint->fk_deferred_count;
> -                       }
> +                       assert(p1 == SAVEPOINT_ROLLBACK);
> +                       if (box_txn_rollback_to_savepoint(sv) != 0)
> +                               goto abort_due_to_error;
> +                       txn->fk_deferred_count = sv->fk_deferred_count;

4. Exactly the same assignment is in box_txn_rollback_to_savepoint. It
is not needed here anymore.

>                 }
>         }
>  
> diff --git a/src/box/txn.c b/src/box/txn.c
> index b57240846..aef655df2 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -734,28 +734,53 @@ box_txn_alloc(size_t size)
>                                     alignof(union natural_align));
>  }
>  
> -box_txn_savepoint_t *
> -box_txn_savepoint()
> +struct txn_savepoint *
> +txn_savepoint_new(struct txn *txn, const char *name)

5. Looks like this function allows duplicate names. Is it
done on purpose?

>  {
> -       struct txn *txn = in_txn();
> -       if (txn == NULL) {
> -               diag_set(ClientError, ER_NO_TRANSACTION);
> -               return NULL;
> -       }
> +       assert(txn == in_txn());
> +       size_t svp_sz = sizeof(struct txn_savepoint);
> +       int name_len = name != NULL ? strlen(name) : 0;
> +       svp_sz += name_len;
>         struct txn_savepoint *svp =
> -               (struct txn_savepoint *) region_alloc_object(&txn->region,
> -                                                       struct txn_savepoint);
> +               (struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
>         if (svp == NULL) {
> -               diag_set(OutOfMemory, sizeof(*svp),
> -                        "region", "struct txn_savepoint");
> +               diag_set(OutOfMemory, svp_sz, "region", "svp");
>                 return NULL;
>         }

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[n.pettik]

>>>> diff --git a/src/box/txn.c b/src/box/txn.c
>>>> index b57240846..451cc9eb1 100644
>>>> --- a/src/box/txn.c
>>>> +++ b/src/box/txn.c
>>>> @@ -779,10 +807,28 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
>>>> 		return -1;
>>>> 	}
>>>> 	txn_rollback_to_svp(txn, svp->stmt);
>>>> +	/* Discard from list all prior savepoints. */
> 
> 1. Again 'prior'. Should be 'newer', please.

Fixed:

diff --git a/src/box/txn.c b/src/box/txn.c
index 875d8adc9..399436a43 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -804,7 +804,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
                return -1;
        }
        txn_rollback_to_svp(txn, svp->stmt);
-       /* Discard from list all prior savepoints. */
+       /* Discard from list all newer savepoints. */
        struct stailq discard;
        stailq_cut_tail(&txn->savepoints, &svp->link, &discard);
        txn->fk_deferred_count = svp->fk_deferred_count;

>>>> +	struct stailq discard;
>>>> +	stailq_cut_tail(&txn->savepoints, &svp->next, &discard);
>>>> 	txn->fk_deferred_count = svp->fk_deferred_count;
>>>> 	return 0;
>>>> }
>>>> 
>>>> +void
>>>> +txn_savepoint_release(struct txn_savepoint *svp)
>>>> +{
>>>> +	struct txn *txn = in_txn();
>>>> +	assert(txn != NULL);
>>> 
>>> 5. Please, add an assertion, that this savepoint is not released
>>> yet. I.e. that its statement has non NULL space and row.
>> 
>> Ok, but we should also consider savepoint containing
>> empty statements list (like SAVEPOINT t1; RELEASE SAVEPOINT t1;):
>> 
>> diff --git a/src/box/txn.c b/src/box/txn.c
>> index 451cc9eb1..b112d3ad9 100644
>> --- a/src/box/txn.c
>> +++ b/src/box/txn.c
>> @@ -819,6 +819,11 @@ txn_savepoint_release(struct txn_savepoint *svp)
>> {
>>        struct txn *txn = in_txn();
>>        assert(txn != NULL);
>> +       /* Make sure that savepoint hasn't been released yet. */
>> +       assert(svp->in_sub_stmt == 0 ||
> 
> 2. As far as I know, in_sub_stmt is always 0 between two statements.
> (Unless they are in a trigger of another statement). This is because
> txn_begin_stmt increments in_sub_stmt and txn_commit_stmt decrements.
> This makes the assertion below always passing regardless of space and
> row values, doesn't it? Not only for a first statement.

Ok, then correct way to do this (I guess) is like in box_txn_rollback_to_savepoint().

@@ -817,9 +817,10 @@ txn_savepoint_release(struct txn_savepoint *svp)
        struct txn *txn = in_txn();
        assert(txn != NULL);
        /* Make sure that savepoint hasn't been released yet. */
-       assert(svp->in_sub_stmt == 0 ||
-              (stailq_entry(svp->stmt, struct txn_stmt, next)->space != NULL &&
-               stailq_entry(svp->stmt, struct txn_stmt, next)->row != NULL));
+       struct txn_stmt *stmt = svp->stmt == NULL ? NULL :
+                               stailq_entry(svp->stmt, struct txn_stmt, next);
+       assert(stmt == NULL || (stmt->space != NULL && stmt->row != NULL));
+       (void) stmt;
        /*
         * Discard current savepoint alongside with all
         * created after it savepoints. To do so we should

>> Author: Nikita Pettik <korablev@tarantool.org>
>> Date:   Fri Aug 2 19:40:55 2019 +0300
>> 
>>    txn: merge struct sql_txn into struct txn
>> 
>>    This procedure is processed in several steps. Firstly, we add name
>>    to struct txn_savepoint since we should be capable of operating on named
>>    savepoints (which in turn is SQL feature). Still, anonymous (in the sense
>>    of name absence) savepoints are also valid. Then, we add list (as
>>    implementation of stailq) of savepoints to struct txn: it allows us to
>>    find savepoint by its name. Finally, we patch rollback to/release
>>    savepoint routines: for rollback tail of the list containing savepoints
>>    is cut (but subject of rollback routine remains in the list); for
>>    release routine we cut tail including node being released.
>> 
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 246654cb7..0f4a160a7 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -2840,69 +2836,31 @@ case OP_Savepoint: {
>>        /* Assert that the p1 parameter is valid. Also that if there is no open
>>         * transaction, then there cannot be any savepoints.
>>         */
>> -       assert(psql_txn->pSavepoint == NULL || box_txn());
>> +       assert(stailq_empty(&txn->savepoints) || box_txn());
>>        assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
>> 
>>        if (p1==SAVEPOINT_BEGIN) {
>> -               /* Create a new savepoint structure. */
>> -               pNew = sql_savepoint(p, zName);
>> -               /* Link the new savepoint into the database handle's list. */
>> -               pNew->pNext = psql_txn->pSavepoint;
>> -               psql_txn->pSavepoint = pNew;
>> +               /*
>> +                * Savepoint is available by its name so we don't
>> +                * care about object itself.
>> +                */
>> +               (void) txn_savepoint_new(txn, zName);
> 
> 3. You still need to check for NULL in case of OOM.

Indeed, fixed:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 0f4a160a7..297a9b6c6 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2844,7 +2844,8 @@ case OP_Savepoint: {
                 * Savepoint is available by its name so we don't
                 * care about object itself.
                 */
-               (void) txn_savepoint_new(txn, zName);
+               if (txn_savepoint_new(txn, zName) == NULL)
+                       goto abort_due_to_error;
        } else {
                /* Find the named savepoint. If there is no such savepoint, then an
                 * an error is returned to the user.

>> -                       /* If it is a RELEASE, then destroy the savepoint being operated on
>> -                        * too. If it is a ROLLBACK TO, then set the number of deferred
>> -                        * constraint violations present in the database to the value stored
>> -                        * when the savepoint was created.
>> -                        */
>> -                       if (p1==SAVEPOINT_RELEASE) {
>> -                               assert(pSavepoint == psql_txn->pSavepoint);
>> -                               psql_txn->pSavepoint = pSavepoint->pNext;
>> -                       } else {
>> -                               txn->fk_deferred_count =
>> -                                       pSavepoint->tnt_savepoint->fk_deferred_count;
>> -                       }
>> +                       assert(p1 == SAVEPOINT_ROLLBACK);
>> +                       if (box_txn_rollback_to_savepoint(sv) != 0)
>> +                               goto abort_due_to_error;
>> +                       txn->fk_deferred_count = sv->fk_deferred_count;
> 
> 4. Exactly the same assignment is in box_txn_rollback_to_savepoint. It
> is not needed here anymore.

Removed:

@@ -2860,7 +2861,6 @@ case OP_Savepoint: {
                        assert(p1 == SAVEPOINT_ROLLBACK);
                        if (box_txn_rollback_to_savepoint(sv) != 0)
                                goto abort_due_to_error;
-                       txn->fk_deferred_count = sv->fk_deferred_count;
                }
        }

>> diff --git a/src/box/txn.c b/src/box/txn.c
>> index b57240846..aef655df2 100644
>> --- a/src/box/txn.c
>> +++ b/src/box/txn.c
>> @@ -734,28 +734,53 @@ box_txn_alloc(size_t size)
>>                                    alignof(union natural_align));
>> }
>> 
>> -box_txn_savepoint_t *
>> -box_txn_savepoint()
>> +struct txn_savepoint *
>> +txn_savepoint_new(struct txn *txn, const char *name)
> 
> 5. Looks like this function allows duplicate names. Is it
> done on purpose?

It’s an interesting question. I thought that it is OK. For instance,
PostgreSQL allows multiple savepoints with the same name:
https://www.postgresql.org/docs/8.1/sql-savepoint.html

‘’’
SQL requires a savepoint to be destroyed automatically when another
savepoint with the same name is established. In PostgreSQL, the old
savepoint is kept, though only the more recent one will be used when
rolling back or releasing. (Releasing the newer savepoint will cause the
older one to again become accessible to ROLLBACK TO SAVEPOINT
and RELEASE SAVEPOINT.) Otherwise, SAVEPOINT is fully SQL
conforming.

‘’'

Same for MS SQL:

https://docs.microsoft.com/en-us/sql/t-sql/language-elements/save-transaction-transact-sql?view=sql-server-2017

‘’’
Duplicate savepoint names are allowed in a transaction, but a
ROLLBACK TRANSACTION statement that specifies the savepoint
name will only roll the transaction back to the most recent
SAVE TRANSACTION using that name.

‘’'

Meanwhile MySQL and DB2 process name duplicates
according to ANSI:

17.5 <savepoint statement>

1) Let S be the <savepoint name>.
2) If S identifies an existing savepoint established within the current
    savepoint level, then that savepoint is destroyed.

MySQL:

https://dev.mysql.com/doc/refman/8.0/en/savepoint.html

‘’’
The SAVEPOINT statement sets a named transaction savepoint
with a name of identifier. If the current transaction has a savepoint
with the same name, the old savepoint is deleted and a new one is set.
‘''

In DB2 things are a bit complicated with ‘UNIQUE’ specifier,
but if it is omitted, savepoint with the same name is erased:

https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tpc/db2z_sql_savepoint.html

‘’’
If svpt-name identifies a savepoint that already exists within
the unit of recovery and the savepoint was not created with
the UNIQUE option, the existing savepoint is destroyed and
a new savepoint is created.

‘’'

But our current behaviour is even different from PosgreSQL
and MSSQL: they use most recent savepoint (i.e. they add
savepoints to the head of list), meanwhile we use oldest
(i.e. we add savepoint to the tail of list). Hence, release of
savepoint releases all savepoints starting from the very
first created with given name; same for rollback.

Currently, our doc says:
‘''

If a savepoint with the same name already exists, it is released
before the new savepoint is set.

‘’'

Which is obviously wrong (both on master and new branches):
we don’t release savepoints:

tarantool>     box.begin()
         >     box.execute('SAVEPOINT t1;')
         >     _, err = box.execute('SAVEPOINT t1;')
         >     print(err)
         >     _, err = box.execute('RELEASE SAVEPOINT t1;')
         >     print(err)
         >     _, err = box.execute('RELEASE SAVEPOINT t1;')
         >     print(err)
         >     _, err = box.execute('RELEASE SAVEPOINT t1;')
         >     print(err)
         >     box.commit();
nil
nil
nil
Can not rollback to savepoint: the savepoint does not exist
---
…

So, on the master branch duplicates are allowed. On the new branch:

tarantool>     box.begin()
         >     box.execute('SAVEPOINT t1;')
         >     _, err = box.execute('SAVEPOINT t1;')
         >     print(err)
         >     _, err = box.execute('RELEASE SAVEPOINT t1;')
         >     print(err)
         >     _, err = box.execute('RELEASE SAVEPOINT t1;')
         >     print(err)
         >     _, err = box.execute('RELEASE SAVEPOINT t1;')
         >     print(err)
         >     box.commit();
nil
nil
Can not rollback to savepoint: the savepoint does not exist
Can not rollback to savepoint: the savepoint does not exist
—
…

Let’s fix that and always release old savepoint as doc says
and ANSI declares. To do so, it’s better to use double
linked list - it allows to remove entry without additional iterations
over the list.

Diff to the current patch:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 14d6a2317..17ab19078 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2836,7 +2836,7 @@ case OP_Savepoint: {
        /* Assert that the p1 parameter is valid. Also that if there is no open
         * transaction, then there cannot be any savepoints.
         */
-       assert(stailq_empty(&txn->savepoints) || box_txn());
+       assert(rlist_empty(&txn->savepoints) || box_txn());
        assert(p1==SAVEPOINT_BEGIN||p1==SAVEPOINT_RELEASE||p1==SAVEPOINT_ROLLBACK);
 
        if (p1==SAVEPOINT_BEGIN) {
diff --git a/src/box/txn.c b/src/box/txn.c
index 3cfe4348f..e102c0501 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -222,7 +222,7 @@ txn_begin()
        txn->engine = NULL;
        txn->engine_tx = NULL;
        txn->fk_deferred_count = 0;
-       stailq_create(&txn->savepoints);
+       rlist_create(&txn->savepoints);
        txn->fiber = NULL;
        fiber_set_txn(fiber(), txn);
        /* fiber_on_yield is initialized by engine on demand */
@@ -754,7 +754,7 @@ txn_savepoint_new(struct txn *txn, const char *name)
                memcpy(svp->name, name, name_len + 1);
        else
                svp->name[0] = 0;
-       stailq_add_tail_entry(&txn->savepoints, svp, link);
+       rlist_add_entry(&txn->savepoints, svp, link);
        return svp;
 }
 
@@ -763,7 +763,7 @@ txn_savepoint_by_name(struct txn *txn, const char *name)
 {
        assert(txn == in_txn());
        struct txn_savepoint *sv;
-       stailq_foreach_entry(sv, &txn->savepoints, link) {
+       rlist_foreach_entry(sv, &txn->savepoints, link) {
                if (strcmp(sv->name, name) == 0)
                        return sv;
        }
@@ -805,8 +805,8 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
        }
        txn_rollback_to_svp(txn, svp->stmt);
        /* Discard from list all newer savepoints. */
-       struct stailq discard;
-       stailq_cut_tail(&txn->savepoints, &svp->link, &discard);
+       struct rlist discard = { };
+       rlist_cut_before(&discard, &txn->savepoints, &svp->link);
        txn->fk_deferred_count = svp->fk_deferred_count;
        return 0;
 }
@@ -823,23 +823,10 @@ txn_savepoint_release(struct txn_savepoint *svp)
        (void) stmt;
        /*
         * Discard current savepoint alongside with all
-        * created after it savepoints. To do so we should
-        * firstly find predecessor of the original node. Note
-        * that operation is currently available only for SQL.
+        * created after it savepoints.
         */
-       struct stailq_entry *prev;
-       stailq_foreach(prev, &txn->savepoints) {
-               /*
-                * If savepoint to be removed is the first in the
-                * list, prev will be equal to NULL after
-                * iterations. So _cut_tail() is going to erase
-                * the whole list.
-                */
-               if (stailq_next(prev) == &svp->link)
-                       break;
-       }
-       struct stailq discard;
-       stailq_cut_tail(&txn->savepoints, prev, &discard);
+       struct rlist discard = { };
+       rlist_cut_before(&discard, &txn->savepoints, rlist_next(&svp->link));
 }
 
 static void
diff --git a/src/box/txn.h b/src/box/txn.h
index 3d378ce29..da12feebf 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -128,7 +128,7 @@ struct txn_savepoint {
         */
        uint32_t fk_deferred_count;
        /** Organize savepoints into linked list. */
-       struct stailq_entry link;
+       struct rlist link;
        /**
         * Optional name of savepoint. If savepoint lacks
         * name (i.e. anonymous savepoint available only by
@@ -225,7 +225,8 @@ struct txn {
         * SQL specific property.
         */
        uint32_t fk_deferred_count;
-       struct stailq savepoints;
+       /** List of savepoints to find savepoint by name. */
+       struct rlist savepoints;
 };
 
 static inline bool


New follow-up patch:

    txn: erase old savepoint in case of name collision
    
    Name duplicates are allowed for savepoints (both in our SQL
    implementation and in ANSI specification). ANSI SQL states that previous
    savepoint should be deleted. What is more, our doc confirms this fact
    and says that "...it is released before the new savepoint is set."
    Unfortunately, it's not true - currently old savepoint remains in the
    list. For instance:
    
    SAVEPOINT t;
    SAVEPOINT t;
    RELEASE SAVEPOINT t;
    RELEASE SAVEPOINT t; -- no error is raised
    
    Let's fix this and remove old savepoint from the list.

diff --git a/src/box/txn.c b/src/box/txn.c
index e102c0501..2777608e1 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -750,10 +750,20 @@ txn_savepoint_new(struct txn *txn, const char *name)
        svp->stmt = stailq_last(&txn->stmts);
        svp->in_sub_stmt = txn->in_sub_stmt;
        svp->fk_deferred_count = txn->fk_deferred_count;
-       if (name != NULL)
+       if (name != NULL) {
+               /*
+                * If savepoint with given name already exists,
+                * erase it from the list. This has to be done
+                * in accordance with ANSI SQL compliance.
+                */
+               struct txn_savepoint *old_svp =
+                       txn_savepoint_by_name(txn, name);
+               if (old_svp != NULL)
+                       rlist_del(&old_svp->link);
                memcpy(svp->name, name, name_len + 1);
-       else
+       } else {
                svp->name[0] = 0;
+       }
        rlist_add_entry(&txn->savepoints, svp, link);
        return svp;
 }
diff --git a/test/sql/savepoints.result b/test/sql/savepoints.result
index b5a0b7f46..e48db302a 100644
--- a/test/sql/savepoints.result
+++ b/test/sql/savepoints.result
@@ -95,3 +95,27 @@ release_sv_fail();
 box.commit();
 ---
 ...
+-- Make sure that if the current transaction has a savepoint
+-- with the same name, the old savepoint is deleted and
+-- a new one is set. Note that no error should be raised.
+--
+collision_sv_2 = function()
+    box.begin()
+    box.execute('SAVEPOINT t1;')
+    box.execute('SAVEPOINT t2;')
+    local _,err = box.execute('SAVEPOINT t1;')
+    assert(err == nil)
+    box.execute('RELEASE SAVEPOINT t1;')
+    local _,err = box.execute('RELEASE SAVEPOINT t1;')
+    assert(err ~= nil)
+    local _, err = box.execute('ROLLBACK TO t2;')
+    assert(err == nil)
+end;
+---
+...
+collision_sv_2();
+---
+...
+box.commit();
+---
+...
diff --git a/test/sql/savepoints.test.lua b/test/sql/savepoints.test.lua
index a4312c114..99622a4db 100644
--- a/test/sql/savepoints.test.lua
+++ b/test/sql/savepoints.test.lua
@@ -56,3 +56,22 @@ release_sv_fail = function()
 end;
 release_sv_fail();
 box.commit();
+
+-- Make sure that if the current transaction has a savepoint
+-- with the same name, the old savepoint is deleted and
+-- a new one is set. Note that no error should be raised.
+--
+collision_sv_2 = function()
+    box.begin()
+    box.execute('SAVEPOINT t1;')
+    box.execute('SAVEPOINT t2;')
+    local _,err = box.execute('SAVEPOINT t1;')
+    assert(err == nil)
+    box.execute('RELEASE SAVEPOINT t1;')
+    local _,err = box.execute('RELEASE SAVEPOINT t1;')
+    assert(err ~= nil)
+    local _, err = box.execute('ROLLBACK TO t2;')
+    assert(err == nil)
+end;
+collision_sv_2();
+box.commit();

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! Mine is in a separate commit
on the branch (after "txn: merge struct sql_txn into struct txn"),
and below. Please, squash if you are ok with that.

Talking of savepoint duplicate names - it is a bug existing in 2.2.1
too. I think, we should backport the fix. Or not. Up to Kirill.

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

commit e955f4ad3ad4114a15f35594153ed66f3c23d661
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Aug 19 22:39:15 2019 +0200

    Review fix

diff --git a/src/box/txn.c b/src/box/txn.c
index bd024a73c..1002c2136 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -805,7 +805,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 	}
 	txn_rollback_to_svp(txn, svp->stmt);
 	/* Discard from list all newer savepoints. */
-	struct rlist discard = { };
+	RLIST_HEAD(discard);
 	rlist_cut_before(&discard, &txn->savepoints, &svp->link);
 	txn->fk_deferred_count = svp->fk_deferred_count;
 	return 0;
@@ -825,7 +825,7 @@ txn_savepoint_release(struct txn_savepoint *svp)
 	 * Discard current savepoint alongside with all
 	 * created after it savepoints.
 	 */
-	struct rlist discard = { };
+	RLIST_HEAD(discard);
 	rlist_cut_before(&discard, &txn->savepoints, rlist_next(&svp->link));
 }
 

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[n.pettik]

> On 19 Aug 2019, at 23:47, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the fixes! Mine is in a separate commit
> on the branch (after "txn: merge struct sql_txn into struct txn"),
> and below. Please, squash if you are ok with that.

Yep, thanks, squashed.

> Talking of savepoint duplicate names - it is a bug existing in 2.2.1
> too. I think, we should backport the fix. Or not. Up to Kirill.
> 
> ===============================================================
> 
> commit e955f4ad3ad4114a15f35594153ed66f3c23d661
> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Date:   Mon Aug 19 22:39:15 2019 +0200
> 
>    Review fix
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index bd024a73c..1002c2136 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -805,7 +805,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
> 	}
> 	txn_rollback_to_svp(txn, svp->stmt);
> 	/* Discard from list all newer savepoints. */
> -	struct rlist discard = { };
> +	RLIST_HEAD(discard);
> 	rlist_cut_before(&discard, &txn->savepoints, &svp->link);
> 	txn->fk_deferred_count = svp->fk_deferred_count;
> 	return 0;
> @@ -825,7 +825,7 @@ txn_savepoint_release(struct txn_savepoint *svp)
> 	 * Discard current savepoint alongside with all
> 	 * created after it savepoints.
> 	 */
> -	struct rlist discard = { };
> +	RLIST_HEAD(discard);
> 	rlist_cut_before(&discard, &txn->savepoints, rlist_next(&svp->link));
> }
> 
> 

[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn

[Vladislav Shpilevoy]

Thanks for the fixes!

LGTM. Kirill, please, proceed. Also take a look at my
comment about backport below.

On 21/08/2019 02:23, n.pettik wrote:
> 
> 
>> On 19 Aug 2019, at 23:47, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Hi! Thanks for the fixes! Mine is in a separate commit
>> on the branch (after "txn: merge struct sql_txn into struct txn"),
>> and below. Please, squash if you are ok with that.
> 
> Yep, thanks, squashed.
> 
>> Talking of savepoint duplicate names - it is a bug existing in 2.2.1
>> too. I think, we should backport the fix. Or not. Up to Kirill.
>>

[tarantool-patches] Re: [PATCH 0/3] Merge struct sql_txn into struct txn/savepoint

[Kirill Yukhin]

Hello,

On 07 Aug 18:13, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/move-sql-structs-from-txn
> 
> As a one of final steps of merging SQL and NoSQL codebases, it is
> required to squash struct sql_txn and struct txn/struct txn_savepoint.
> struct sql_txn was needed to operate on named savepoints. Hence, it
> contains name of savepoint and a link to next savepoint. This patch-set
> adds optional name of savepoint to struct txn_savepoint and orginizes
> txn_savepoints into list. Head of list is held in stuct txn. Iterating
> over list allows to find savepoint by its name. Finally, after this
> procedure is completed, we can remove struct sql_txn and struct Savepoint.
> 
> Nikita Pettik (3):
>   txn: move fk_deferred_count from psql_txn to txn
>   txn: merge struct sql_txn into struct txn
>   sql: use struct txn_savepoint as anonymous savepoint

I've checked the patchset into 2.2 and master.

--
Regards, Kirill Yukhin