[tarantool-patches] Re: [PATCH 2/5] schema: add new system space for FK constraints

["n.pettik" <korablev@tarantool.org>]

Firstly, I don’t understand some of your fixes:

@@ -3855,11 +3853,9 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
                memset(fkey, 0, sizeof(*fkey));
                fkey->def = fk_def;
                fkey->index_id = fk_index->def->iid;
+               rlist_add_entry(&child_space->child_fkey, fkey, child_link);
+               rlist_add_entry(&parent_space->parent_fkey, fkey, parent_link);
                if (old_tuple == NULL) {
-                       rlist_add_entry(&child_space->child_fkey, fkey,
-                                       child_link);
-                       rlist_add_entry(&parent_space->parent_fkey, fkey,
-                                       parent_link);
                        struct trigger *on_rollback =
                                txn_alter_trigger_new(on_create_fkey_rollback,
                                                      fkey);
@@ -3868,10 +3864,6 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
                        struct fkey *old_fk =
                                fkey_grab_by_name(&child_space->child_fkey,
                                                  fk_def->name);
-                       rlist_add_entry(&child_space->child_fkey, fkey,
-                                       child_link);
-                       rlist_add_entry(&parent_space->parent_fkey, fkey,
-                                       parent_link);

In case of replace we must firstly remove entry and only then insert new one.
It is easy to check that the way you suggest doesn’t work (just modify test you provided):

box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b INT UNIQUE);")
t = box.space._fk_constraint:select{}[1]:totable()
errinj = box.error.injection
errinj.set("ERRINJ_WAL_IO", true)
-- Make constraint reference B field instead of id.
t[9] = {2}
box.space._fk_constraint:replace(t)
box.sql.execute("INSERT INTO t3 VALUES (1, 2, 2)”)

- No error is raised.

If I discard your diff it works fine (i.e. "FOREIGN KEY constraint failed” error is raised).

>>    schema: add new system space for FK constraints
>>        This patch introduces new system space to persist foreign keys
>>    constraints. Format of the space:
>>        _fk_constraint (space id = 358)
>>        [<contraint name> STR, <parent id> UINT, <child id> UINT,
> 
> 1. Typo: contraint.

Fixed.

>> +/** Return old FK and release memory for the new one. */
>> +static void
>> +on_replace_fkey_rollback(struct trigger *trigger, void *event)
>> +{
>> +	(void) event;
>> +	struct fkey *fk = (struct fkey *)trigger->data;
>> +	struct space *parent = space_by_id(fk->def->parent_id);
>> +	struct space *child = space_by_id(fk->def->child_id);
>> +	struct fkey *old_fkey = fkey_grab_by_name(&child->child_fkey,
>> +						  fk->def->name);
>> +	fkey_delete(old_fkey);
>> +	rlist_add_entry(&child->child_fkey, fk, child_link);
>> +	rlist_add_entry(&parent->parent_fkey, fk, parent_link);
> 
> 
> 2. In the comment you said that this function restores the old fk and
> deletes the new one. But on the code we see the line:
> fkey_delete(old_fkey).
> 
> Secondly, you got old_fkey from child->child_fkey, but earlier, in
> on_replace trigger, you had removed old_fkey from child_fkey list
> and put here new fkey. So here actually old_fkey == fkey (I tested, it is).
> 
> I think, it can be fixed quite simple: you have ability to fetch the new
> fkey from child_list and have no the latter for the old one since it is
> unlinked already. So lets pass here old_fkey as trigger->data and fetch
> the new fkey from the list.

Thx for catch. Fixed:

+++ b/src/box/alter.cc
@@ -3678,14 +3678,14 @@ static void
 on_replace_fkey_rollback(struct trigger *trigger, void *event)
 {
        (void) event;
-       struct fkey *fk = (struct fkey *)trigger->data;
-       struct space *parent = space_by_id(fk->def->parent_id);
-       struct space *child = space_by_id(fk->def->child_id);
-       struct fkey *old_fkey = fkey_grab_by_name(&child->child_fkey,
-                                                 fk->def->name);
-       fkey_delete(old_fkey);
-       rlist_add_entry(&child->child_fkey, fk, child_link);
-       rlist_add_entry(&parent->parent_fkey, fk, parent_link);
+       struct fkey *old_fk = (struct fkey *)trigger->data;
+       struct space *parent = space_by_id(old_fk->def->parent_id);
+       struct space *child = space_by_id(old_fk->def->child_id);
+       struct fkey *new_fkey = fkey_grab_by_name(&child->child_fkey,
+                                                 old_fk->def->name);
+       fkey_delete(new_fkey);
+       rlist_add_entry(&child->child_fkey, old_fk, child_link);
+       rlist_add_entry(&parent->parent_fkey, old_fk, parent_link);
 }
 
 /** On rollback of drop simply return back FK to DD. */
@@ -3866,7 +3866,7 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
                                                  fk_def->name);
                        struct trigger *on_rollback =
                                txn_alter_trigger_new(on_replace_fkey_rollback,
-                                                     fkey);
+                                                     old_fk);

> 
> The test for this bug is below:
> 
> 	box.sql.execute("CREATE TABLE t1 (id PRIMARY KEY, a REFERENCES t1, b INT);")
> 	t = box.space._fk_constraint:select{}[1]:totable()
> 	errinj = box.error.injection
> 	errinj.set("ERRINJ_WAL_IO", true)
> 	box.space._fk_constraint:replace(t)

Added to the next patch:

+++ b/test/sql/foreign-keys.test.lua
@@ -160,5 +160,31 @@ box.space._fk_constraint:select{}
 box.sql.execute('DROP TABLE tc')
 box.sql.execute('DROP TABLE tp')
 
+-- Tests which are aimed at verifying work of commit/rollback
+-- triggers on _fk_constraint space.
+--
+box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b INT UNIQUE);")
+t = box.space._fk_constraint:select{}[1]:totable()
+errinj = box.error.injection
+errinj.set("ERRINJ_WAL_IO", true)
+-- Make constraint reference B field instead of id.
+t[9] = {2}
+box.space._fk_constraint:replace(t)
+errinj.set("ERRINJ_WAL_IO", false)
+box.sql.execute("INSERT INTO t3 VALUES (1, 2, 2);")
+errinj.set("ERRINJ_WAL_IO", true)
+box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) REFERENCES t3;")
+errinj.set("ERRINJ_WAL_IO", false)
+box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
+box.sql.execute("DELETE FROM t3;")
+box.snapshot()
+box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) REFERENCES t3;")
+box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
+errinj.set("ERRINJ_WAL_IO", true)
+box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
+box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
+errinj.set("ERRINJ_WAL_IO", false)
+box.sql.execute("DROP TABLE t3;")
+

+++ b/test/sql/foreign-keys.result
@@ -356,5 +356,85 @@ box.sql.execute('DROP TABLE tc')
 box.sql.execute('DROP TABLE tp')
 ---
 ...
+-- Tests which are aimed at verifying work of commit/rollback
+-- triggers on _fk_constraint space.
+--
+box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b INT UNIQUE);")
+---
+...
+t = box.space._fk_constraint:select{}[1]:totable()
+---
+...
+errinj = box.error.injection
+---
+...
+errinj.set("ERRINJ_WAL_IO", true)
+---
+- ok
+...
+-- Make constraint reference B field instead of id.
+t[9] = {2}
+---
+...
+box.space._fk_constraint:replace(t)
+---
+- error: Failed to write to disk
+...
+errinj.set("ERRINJ_WAL_IO", false)
+---
+- ok
+...
+box.sql.execute("INSERT INTO t3 VALUES (1, 2, 2);")
+---
+- error: FOREIGN KEY constraint failed
+...
+errinj.set("ERRINJ_WAL_IO", true)
+---
+- ok
+...
+box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) REFERENCES t3;")
+---
+- error: Failed to write to disk
+...
+errinj.set("ERRINJ_WAL_IO", false)
+---
+- ok
+...
+box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
+---
+...
+box.sql.execute("DELETE FROM t3;")
+---
+...
+box.snapshot()
+---
+- ok
+...
+box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) REFERENCES t3;")
+---
+...
+box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
+---
+- error: FOREIGN KEY constraint failed
+...
+errinj.set("ERRINJ_WAL_IO", true)
+---
+- ok
+...
+box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
+---
+- error: Failed to write to disk
+...
+box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
+---
+- error: FOREIGN KEY constraint failed
+...
+errinj.set("ERRINJ_WAL_IO", false)
+---
+- ok
+...
+box.sql.execute("DROP TABLE t3;")
+---
+...

> But it crashes on the next commit. On the current one this function is
> unreachable.
> 
> 	Process 51167 stopped
> 	* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
> 
>> +/**
>> + * ANSI SQL doesn't allow list of referenced fields to contain
>> + * duplicates. Firstly, we try to follow the easiest way:
>> + * if all referenced fields numbers are less than 63, we can
>> + * use bit mask. Otherwise, fall through slow check where we
>> + * use O(field_cont^2) simple nested cycle iterations.
>> + */
>> +static void
>> +fkey_links_check_duplicates(struct fkey_def *fk_def)
>> +{
>> +	uint64_t field_mask = 0;
>> +	for (uint32_t i = 0; i < fk_def->field_count; ++i) {
>> +		uint32_t parent_field = fk_def->links[i].parent_field;
>> +		if (parent_field > 63)
>> +			goto slow_check;
>> +		if (field_mask & (((uint64_t) 1) << parent_field))
>> +			goto error;
>> +		column_mask_set_fieldno(&field_mask, parent_field);
> 
> 3. Looks like column_mask API is not applicable here since you
> anyway still use raw bit setting. I have removed column_mask API
> from there on the branch.

Yep, thx for fix.