Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org, gorcunov@gmail.com
Subject: Re: [Tarantool-patches] [PATCH 1/1] txn: destroy commit and rollback triggers
Date: Sun, 25 Apr 2021 11:44:48 +0300
Message-ID: <fa9828e4-c3c0-a725-e08d-4f9dbf2fb30c@tarantool.org> (raw)
In-Reply-To: <15101a51fcd1441813a384fef9f670150b522a3d.1619224125.git.v.shpilevoy@tarantool.org>



24.04.2021 03:29, Vladislav Shpilevoy пишет:
> They were not deleted ever. Worked fine for DDL, for which they
> were introduced on the first place, because these triggers are on
> the region memory.
>
> But didn't work when the triggers became available in the public
> API, because these are allocated on the heap. As a result, all the
> box.on_commit() and box.on_rollback() triggers leaked.
>
> The patch ensures all the on_commit/on_rollback triggers are
> destroyed. In order to mitigate the performance impact the
> internal triggers now clear themselves so as it wouldn't be
> necessary to walk their list in txn.c again.
>
> Another hack not to touch these triggers when possible makes their
> destruction happen only in txn_run_commit_triggers() and
> txn_run_rollback_triggers(). These are not called when a
> transaction does not have any triggers, and therefore these
> destroys don't cost anything. On the contrary with if they would
> be destroyed in txn_free() which is called always.
>
> In short: the patch does not affect the common path when the
> triggers are not used.
>
> Another option was to force all the triggers clear themselves. For
> example, in case of commit all the on_commit triggers must clear
> themselves, and the rollback triggers are destroyed. Vice versa
> when a rollback happens. But it didn't work because the Lua
> triggers all work via a common runner lbox_trigger_run(), which
> can't destroy its argument in most of the cases (space:on_replace,
> :before_replace, ...).

Hi! Thanks for  the patch!
Please find some comments below.

>
> Closes #6025
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-6025-box.on_commit-leak
> Issue: https://github.com/tarantool/tarantool/issues/6025
>
>   .../unreleased/gh-6025-box.on_commit-leak.md  |  3 +
>   src/box/alter.cc                              | 39 +++++++++++--
>   src/box/txn.c                                 | 26 +++++++--
>   src/box/txn_limbo.c                           |  2 +
>   src/box/vinyl.c                               |  2 +
>   .../gh-6025-box.on_commit-leak.test.lua       | 57 +++++++++++++++++++
>   6 files changed, 119 insertions(+), 10 deletions(-)
>   create mode 100644 changelogs/unreleased/gh-6025-box.on_commit-leak.md
>   create mode 100755 test/app-tap/gh-6025-box.on_commit-leak.test.lua
>

...

> 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
> @@ -54,7 +54,7 @@ static int
>   txn_on_yield(struct trigger *trigger, void *event);
>   
>   static void
> -txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers);
> +txn_run_rollback_triggers(struct txn *txn);
>   
>   static int
>   txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
> @@ -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.


>   
>   static void
> @@ -438,8 +438,9 @@ fail:

...

>   
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index c22bd6665..7d5ece81e 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c

...

> 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.


> 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?

> @@ -0,0 +1,57 @@
> +#!/usr/bin/env tarantool
> +
> +--
> +-- gh-6025: box.on_commit() and box.on_rollback() triggers always leaked.
> +--
> +
> +local tap = require('tap')
> +
> +--
> +-- Create a functional reference at the passed value. Not at the variable
> +-- keeping it in the caller, but at the value itself.
> +--
> +local function wrap_value(value)
> +    return function()
> +        assert(value == nil or value ~= nil)
> +    end
> +end
> +
> +local function test_on_commit_rollback(test)
> +    test:plan(2)
> +
> +    local s = box.schema.create_space('test')
> +    s:create_index('pk')
> +    local weak_ref = setmetatable({}, {__mode = 'v'})
> +
> +    -- If the triggers are deleted, the wrapped value reference must disappear
> +    -- and nothing should keep the value from collection from the weak table.
> +    local value = {}
> +    weak_ref[1] = value
> +    box.begin()
> +    s:replace{1}
> +    box.on_commit(wrap_value(value))
> +    box.on_rollback(wrap_value(value))
> +    box.commit()
> +    value = nil
> +    collectgarbage()
> +    test:isnil(weak_ref[1], 'on commit both triggers are deleted')
> +
> +    value = {}
> +    weak_ref[1] = value
> +    box.begin()
> +    s:replace{2}
> +    box.on_commit(wrap_value(value))
> +    box.on_rollback(wrap_value(value))
> +    box.rollback()
> +    value = nil
> +    collectgarbage()
> +    test:isnil(weak_ref[1], 'on rollback both triggers are deleted')
> +end
> +
> +box.cfg{}
> +
> +local test = tap.test('gh-6025-box.on_commit-leak')
> +test:plan(1)
> +test:test('delete triggers on commit and rollback', test_on_commit_rollback)
> +
> +os.exit(test:check() and 0 or 1)

-- 
Serge Petrenko


  reply	other threads:[~2021-04-25  8:44 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-24  0:29 Vladislav Shpilevoy via Tarantool-patches
2021-04-25  8:44 ` Serge Petrenko via Tarantool-patches [this message]
2021-04-25 15:47   ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa9828e4-c3c0-a725-e08d-4f9dbf2fb30c@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=sergepetrenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git