Hello! Thanks for review, Diff is in the end!
вт, 31 июл. 2018 г. в 16:29, Alexander Turenko
<alexander.turenko@xxxxxxxxxxxxx>:
Hi!
Thanks for updates.
Answered inline.
WBR, Alexander Turenko.
+ /*
+ * Xfer optimization is unable to correctly insert data
+ * in case there's a conflict action other than
+ * explicit *_ABORT. This is the reason we want to only
+ * run it if the destination table is initially empty.
+ * That block generates code to make that determination.
+ */
+ if (!(onError == ON_CONFLICT_ACTION_ABORT &&
+ !is_err_action_default)) {
Do you mean that:
1. The optimization non-empty table case correctly works only with ABORT
conflict action (default or explicit).
2. The default conflict action can be overwritten on per-column basis, so
'default abort' can really be replace or other conflict action.
If so, your description doesn't give this information.
To more on that, can we check per-column conflict actions instead of check
whether the conflict action default or explicit? This would enable more
cases
with non-empty tables for the optimization. And this would look less
confusing,
IMHO.
Well, basically, you're right. But the thing is that we're going to remove
column conflict actions, thus, I suppose, it doesn't make sense.
Nikita, Correct me if I'm wrong please.
I recommend to comment such things in the code or commit message (as you
think it is more appropriate). It allows to decrease review iterations
at least :) I don't insist, though.
I have one more question on that. It seems that SQLite has this
optimization
working with ROLLBACK conflict action. We cannot doing so, because of some
problem? Did this problem described / trackerized somewhere? Or something
changes underhood and makes this impossible? Are we know exact reason or
just
observing it does not work?
I investigated that a little. OP_IdxInsert was changed so that we can
process
ABORT, FAIL or IGNORE. I took it into account in newer version hence
it is also used in these cases even when the destination table is not empty.
So I understand it as follows: sqlite3's OP_IdxInsert just copy rows and
cannot handle non-trivial conflict actions, but tarantool's OP_IdxInsert
can handle it. However we have some non-investigated problems with
ROLLBACK and REPLACE as well as per-column conflict actions.
I think that is is worth to investigate it a little deeper and file
issue(s), but this is not the blocker for this patch.
sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ switch (onError) {
+ case ON_CONFLICT_ACTION_IGNORE:
+ sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE);
+ break;
+ case ON_CONFLICT_ACTION_FAIL:
+ sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL);
+ break;
+ default:
+ sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ break;
+ }
OPFLAG_NCHANGE is independent with other flags I think.
+local function do_xfer_test(test, test_func, test_name, func, exp, opts)
+ local opts = opts or {}
+ local exp_xfer_count = opts.exp_xfer_count
+ local before = box.sql.debug().sql_xfer_count
+ local ok, result = pcall(test_func, test, test_name, func, exp)
+ local after = box.sql.debug().sql_xfer_count
+ if exp_xfer_count ~= nil then
+ ok = ok and test:is(after - before, exp_xfer_count,
+ test_name .. '-xfer-count')
+ end
+ return ok
end
I missed that do_execsql_test don't return a result and do_catchsql_test
returns {0} or {1}. So just don't same `ok` and `result` and remove
pcall.
By the way, we have 4 spaces indent for Lua accorsing to Lua Style
Guide.