[tarantool-patches] Re: [PATCH 2/3] txn: merge struct sql_txn into struct txn
n.pettik
korablev at tarantool.org
Thu Aug 15 14:04:00 MSK 2019
> On 10 Aug 2019, at 00:02, Vladislav Shpilevoy <v.shpilevoy at 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 at 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;')
More information about the Tarantool-patches
mailing list