Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v1 1/1] sql: fix fk violation for autoincremented field

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/10/21 11:40]:
> On 17.10.2019 22:20, Konstantin Osipov wrote:
> > * Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/10/17 17:36]:
> >> Fk constraints used to ignore incremented fields that are
> >> represented with NULL placeholders in a tuple during FK tests in
> >> the SQL frontend. A factual value is inserted in a tuple a bit
> >> later, during DML 'insert' operation on the server side.
> >>
> >> To work around this issue a new OP_NextSequenceValue opcode is
> >> introduced. This new operation retrieves a next sequence value
> >> (that will be factually inserted in a tuple later) to test it
> >> for fk constraint compatibility.
> > 
> > If you added a method to query the next sequence value, why don't
> > you set it in SQL? This way you don't have to query it, you
> > increment
> > and use it in SQL right away.
> 
> 1. "why don't you set it in SQL"
> I've spend much time to deal with this concept and now I think that is is not really good idea also.
> 
> Consider the following example:
> box.execute("CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT check (a > 0));")
> box.execute("INSERT OR IGNORE INTO t VALUES (null, 1), (null, -1), (null, 2);")
> - autoincrement_ids:
>   - 1
>   - 3
>   row_count: 2
> 
> 
> As you see, the generated ids are attached to the response only when the operation is done.
> 
> 2. Dealing with problem as proposed, if autoincremented ids were generated beforehand, i.e.
> (null, 1), (null, -1), (null, 2) -> (1, 1), (2, -1), (3, 2)
> we wouldn't attach them to the response during generation, because
> (for some reason, like ck constraint) some parts of the DML request could
> be declined.
> 
> At the same time in OP_IdxInsert (in this case) we have no information about the id is
> generated for the last tuple processed. (in the current Tarantool's architecture it is
> possible, because the generation is guaranteed to be performed during the tuple insertion
> - so we take a look for the sequence state).

OK, I get it, so INSERT IGNORE  + multiple-values insert breaks
the approach I suggested. Worth mentioning in the comment, I
guess. The tests you're coming up with are also very nice (would
be good to have them).

> 3. Unfortunately, my previously proposed approach is not correct also:
> box.execute("CREATE TABLE t1 (s1 INTEGER PRIMARY KEY);")
> box.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, FOREIGN KEY (s1) REFERENCES t1);")
> box.execute("INSERT INTO t2 VALUES (NULL), (NULL), (NULL);")
> 
> Looking for the next value of the sequence without advancing the sequence state is
> not correct, as you could imagine.

Choosing between the two evils, I think wrong autoinc ids output
in case of INSERT IGNORE is a lesser evil than failing
multi-values insert. 

> 4. You know, a completely correct solution for the problem of the ticket #4565 is evaluating
> fk constraints on the server side :(

OK, I get it. PeterG has always been the top notch "architecture bugs" finder, and this is one.
Until SQL is fully integrated we'll be stepping on other bugs like
these. Anyway, if you must fix this bug before 4, I think you
should choose 2 over 3.

-- 
Konstantin Osipov, Moscow, Russia