Hi Nikita!
Please, consider comments below.
WBR, Alexander Turenko.
> +test:do_execsql_test(
> + "xfer-oprimization-1.8",
> ...
> +test:do_execsql_test(
> + "xfer-oprimization-1.10",
> ...
> +test:do_execsql_test(
> + "xfer-oprimization-1.12",
> ...
> +test:do_execsql_test(
> + "xfer-oprimization-1.16",
> +test:do_execsql_test(
> + "xfer-oprimization-1.18",
oprimization -> optimization
Is already fixed.
It seems that review questions from the last Nikita email in the thread
was not fixed or answered (at least some of them).
> + sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc,
> + pSrc->tnum, space_ptr_reg);
Why do you open source space for write?
I changed it to 'vdbe_emit_open_cursor' which now is used everywhere for both reading and writing (internally it uses 'OP_OpenWrite')
How your changes to xferCompatibleIndex and empty check enabling
condition is motivated? It is hard to understand it without more
detailed description.
We open cursors on PK for both tables, additional check that we have opened 'em on PK isn't redundunt.
What's about empty check enabling: I only want to add some cases where we don't demand empty destination table for xferOptimization to be used. Thus it will be used a little bit more frequently (it's faster than inserting non-raw data for about 13%).
---
EOF.
On Thu, Jun 28, 2018 at 01:18:39PM +0300, Alexander Turenko wrote:
> On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote:
> > > @@ -1737,8 +1744,10 @@ xferOptimization(Parse * pParse, /*
> > Parser context */
> > > if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> > > if (pDest->iPKey >= 0)
> > > onError = pDest->keyConf;
> > > - if (onError == ON_CONFLICT_ACTION_DEFAULT)
> > > + if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> > > onError = ON_CONFLICT_ACTION_ABORT;
> > > + confl_action_default = 1;
> > Why do you need this variable at all? I mean, DEFAULT always
> > is an alias to ABORT, isn’t it?
> > Yes, it is, but there's a little difference between directly specified
> > ABORT for an
> > insert stmt (INSERT OR ABORT) and just INSERT without any specified
> > error action
> > (ABORT specified by the internals). When you directly specify it ABORT
> > is a higher priority
> > action than in case there's a column with REPLACE error action. Thus we
> > can even insert
> > not in the empty destination table.
>
> If an user asks for ABORT explicitly we should make abort, I think.
>
> As I understood the extra variable appears due to the fact than we can
> have per-column conflict clauses in CREATE TABLE and per-table clause
> with INSERT OR ABORT. The latter should have precedence, I think.
>
> I don't sure whether something (behaviour? code?) should be different
> from SQLite here in light of #2963 changes. Kirill, Nikita, can you
> comment, please?
>
> WBR, Alexander Turenko.
>