Re: [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the review!

>> diff --git a/src/box/txn.c b/src/box/txn.c
>> index 03b39e0de..4dfe1ad82 100644
>> --- a/src/box/txn.c
>> +++ b/src/box/txn.c
>> @@ -150,7 +150,7 @@ txn_rollback_one_stmt(struct txn *txn, struct txn_stmt *stmt)
>>       if (txn->engine != NULL && stmt->space != NULL)
>>           engine_rollback_statement(txn->engine, txn, stmt);
>>       if (stmt->has_triggers)
>> -        txn_run_rollback_triggers(txn, &stmt->on_rollback);
>> +        txn_run_rollback_triggers(txn);
>>   }
> 
> 
> 1. This piece ran stmt->on_rollback, and you run txn->on_rollback now.
>    These are not the same, for example check txn_rollback_to_svp below.

Indeed, I added a bug here, thanks for noticing. In v2 I removed
these run_rollback/run_commit functions. For the statement triggers
I added an assertion that they don't have a 'destroy' method. Because
if they would have, on commit we would need to walk the statement
list and for each statement destroy its rollback triggers, which would
add notable complexity.

>> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
>> index b53e97593..d381179d0 100644
>> --- a/src/box/vinyl.c
>> +++ b/src/box/vinyl.c
> ...
> 
> 
> 2. There's also applier_txn_rollback_cb() in applier.cc
>    You should probably  clear it as  well.

In v2 I deleted all trigger clears.

>> diff --git a/test/app-tap/gh-6025-box.on_commit-leak.test.lua b/test/app-tap/gh-6025-box.on_commit-leak.test.lua
>> new file mode 100755
>> index 000000000..ec4e8b099
>> --- /dev/null
>> +++ b/test/app-tap/gh-6025-box.on_commit-leak.test.lua
> 
> 3. The test's cool, I wouldn't have come up with it myself.
>    Shouldn't it be in box-tap though?

Yes, this is about box, and I didn't think of box-tap. Moved there now.

I've sent v2 since the patch is too different now and the diff would
look not very easy to understand.