storage: preserve original error on function fail

[kamenkremen]

For older Tarantool versions vshard uses func.call to execute functions.

If such a function raises an error during an uncommitted transaction, 'Transaction is active...' error is going to be raised, which will mask the original error.

This patch fixes that bug by checking the function call result. If the function returns an error and a transaction is still active, vshard now explicitly rolls back the transaction.

Fixes #614

NO_DOC=bugfix

[Serpentian]

Well done! I'm glad, you've figured out the reason 🎉

P.S. And please, don't forget to rebase)

[Serpentian]

Almost there! Well done

[Serpentian]

Small nits are left

[Serpentian]

Thank you for the patches! Well done

[kamenkremen]

I ran some performance tests, and here are the results.

Setup

Hardware

Intel(R) Xeon(R) Gold 6230 CPU @ 2.10GHz

Tarantool

Tarantool 2.11.0-0-g247a9a4183
Target: Linux-x86_64-Release
Build options: cmake . -DCMAKE_INSTALL_PREFIX=/usr/local -DENABLE_BACKTRACE=TRUE
Compiler: GNU-8.3.1
C_FLAGS: -fexceptions -funwind-tables -fasynchronous-unwind-tables -fno-common -fopenmp -msse2 -Wformat -Wformat-security -Werror=format-security -fstack-protector-strong -fPIC -fmacro-prefix-map=./tarantool=. -std=c11 -Wall -Wextra -Wno-gnu-alignof-expression -fno-gnu89-inline -Wno-cast-function-type
CXX_FLAGS: -fexceptions -funwind-tables -fasynchronous-unwind-tables -fno-common -fopenmp -msse2 -Wformat -Wformat-security -Werror=format-security -fstack-protector-strong -fPIC -fmacro-prefix-map=./tarantool=. -std=c++11 -Wall -Wextra -Wno-invalid-offsetof -Wno-gnu-alignof-expression -Wno-cast-function-type

2 shards, 2 replicas in one shard, 9 routers, 9 load generators, which are created as follows:

tarantool generate_load.lua \
            --bucket_count 30000 \
            --op_type replace \
            --warmup \
            --uri localhost:3305,localhost:3306,localhost:3307,localhost:3308,localhost:3309,localhost:3310,localhost:3311,localhost:3312,localhost:3313 \
            --fibers 50 \
            --ops 1000000 \

Mean of 20 iterations

The perf test is from https://github.com/tarantool/vshard-ee/pull/51, with registered replace function in schema.

• Before patch: 193721.8000 (100%)
• After only patch that preserves original error on function fail: 193125.2000 (99.7%)
• After whole patch: 175869.9000 (90.7%)

[Serpentian]

Let's do only that variant:

After only patch that preserves original error on function fail: 193125.2000 (99.7%)

The perf degradation is way too huge in the second variant. We'll leave #630 unresolved for now, since nobody complained about that yet.

[mrForza]

Thank you for the clean patch! 👍 Left some minor comments below

[Serpentian]

I can confirm, that @kamenkremen commit doesn't cause major perf degradation, so I remove do not merge lablel, we`re safe now.

However, @Gerold103, can we go even further and drop that piece of shit from net.box, which we don't need at all:

    local results = {...}
    for i = 1, select('#', ...) do
        if type(results[i]) == 'cdata' then
            results[i] = msgpack.decode(msgpack.encode(results[i]))
        end
    end
    return unpack(results)

This breaks backward compatibility of vshard.storage.call, but who cares, when it costs us 10% of performance. Here're my measurements. This perf bump will affect all requests, which return cdata (e.g. get, replace, insert). The only one not affected - select (it returns table). All tests (even cartridge's ones) works: #641

5 workers (load generators), 9 routers, 2 shards, 2 replicas in one shard. replaces.

Before the patches:
Average per iteration: 186312.8500 (100%)

After Ivan`s commit:
Average per iteration: 185511.4500 (99.6%)

After two commits (Ivan's + Mine: #641):
Average per iteration: 204105.0500 (109.6%)

Edit

Actually, we must fix it in core: tarantool/tarantool#11624 (comment). The inconsistency between versions is much more serious: the return type on router becomes cdata instead of table. Id rather return cdata everywhere and that's it. When we'll fix it in core, we can use the net.box.self.call` from there, but on all versions before it we should reimplement net.box.self.call, as it's done in #641, in order to avoid that perf degradation.

This way we have the following backward compatibility breakage: when cdata is returned on old vshard versions, vshard automatically did table from it, from now on cdata itself is returned. IMHO, it's ok.

[Gerold103]

I think it is fine to just drop netbox.self from vshard entirely. I can't imagine what can really break besides for people who were calling vshard.storage.call() locally on storage and were checking type() of results.

[Gerold103]

Thanks, this was a tricky topic in terms of perf and tiny details of behaviour.

[mrForza]

Thank you for the patch!