From 8a6c868f68237e81212ff80bdb1e11d55a82df28 Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Thu, 6 Jun 2024 11:52:44 -0700 Subject: [PATCH] Allowing ForeignKeyCollection.GetMatchingKey to take a parameter controlling whether an unresolved FK will match with a resolved FK or not. --- .../doltcore/doltdb/foreign_key_coll.go | 24 +++++--- go/libraries/doltcore/merge/merge_schema.go | 10 +++- .../enginetest/dolt_transaction_queries.go | 58 +++++++++++++++++++ 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/go/libraries/doltcore/doltdb/foreign_key_coll.go b/go/libraries/doltcore/doltdb/foreign_key_coll.go index 6d19c62c90..a931b2ac0b 100644 --- a/go/libraries/doltcore/doltdb/foreign_key_coll.go +++ b/go/libraries/doltcore/doltdb/foreign_key_coll.go @@ -470,25 +470,33 @@ OuterLoop: } // GetMatchingKey gets the ForeignKey defined over the parent and child columns. If the given foreign key is resolved, -// then both resolved and unresolved keys are checked for a match. If the given foreign key is unresolved, then ONLY -// unresolved keys may be found. +// then both resolved and unresolved keys are checked for a match. If the given foreign key is unresolved, then it +// will match with unresolved keys, and may also match with resolved keys if |matchUnresolvedKeyToResolvedKey| is +// set to true. // -// This discrepancy is due to the primary uses for this function. It is assumed that the ForeignKeyCollection is an +// When |matchUnresolvedKeyToResolvedKey| is false, it is assumed that the ForeignKeyCollection is an // ancestor collection compared to the collection that the given key comes from. Therefore, the key found in the // ancestor will usually be the unresolved version of the given key, hence the comparison is valid. However, if the // given key is unresolved, it is treated as a new key, which cannot be matched to a resolved key that previously // existed. // -// The given schema map is keyed by table name, and is used in the event that the given key is resolved and any keys in -// the collection are unresolved. A "dirty resolution" is performed, which matches the column names to tags, and then a -// standard tag comparison is performed. If a table or column is not in the map, then the foreign key is ignored. -func (fkc *ForeignKeyCollection) GetMatchingKey(fk ForeignKey, allSchemas map[string]schema.Schema) (ForeignKey, bool) { +// When the ForeignKeyCollection is NOT an ancestor collection, |matchUnresolvedKeyToResolvedKey| can be set to true +// to enable |fk| to match against both resolved and unresolved keys in the ForeignKeyCollection. This is necessary +// when one session commits a table with resolved foreign keys, but another open session still has an unresolved +// version of the foreign key. When the open session commits, it needs to merge it's root with the root that has +// the resolved foreign key, so it's unresolved foreign key needs to be able to match a resolved key in this case. +// +// The given schema map, |allSchemas|, is keyed by table name, and is used in the event that the given key is resolved +// and any keys in the collection are unresolved. A "dirty resolution" is performed, which matches the column names to +// tags, and then a standard tag comparison is performed. If a table or column is not in the map, then the foreign key +// is ignored. +func (fkc *ForeignKeyCollection) GetMatchingKey(fk ForeignKey, allSchemas map[string]schema.Schema, matchUnresolvedKeyToResolvedKey bool) (ForeignKey, bool) { if !fk.IsResolved() { // The given foreign key is unresolved, so we only look for matches on unresolved keys OuterLoopUnresolved: for _, existingFk := range fkc.foreignKeys { // For unresolved keys, the table name is important (column tags are globally unique, column names are not) - if existingFk.IsResolved() || + if (!matchUnresolvedKeyToResolvedKey && existingFk.IsResolved()) || fk.TableName != existingFk.TableName || fk.ReferencedTableName != existingFk.ReferencedTableName || len(fk.UnresolvedFKDetails.TableColumns) != len(existingFk.UnresolvedFKDetails.TableColumns) || diff --git a/go/libraries/doltcore/merge/merge_schema.go b/go/libraries/doltcore/merge/merge_schema.go index 8bc39dfa13..7e5a8ab067 100644 --- a/go/libraries/doltcore/merge/merge_schema.go +++ b/go/libraries/doltcore/merge/merge_schema.go @@ -909,7 +909,11 @@ func foreignKeysInCommon(ourFKs, theirFKs, ancFKs *doltdb.ForeignKeyCollection, common, _ = doltdb.NewForeignKeyCollection() err = ourFKs.Iter(func(ours doltdb.ForeignKey) (stop bool, err error) { - theirs, ok := theirFKs.GetMatchingKey(ours, ancSchs) + // Since we aren't using an ancestor root here, pass true for the + // matchUnresolvedKeyToResolvedKey parameter. This allows us to match + // resolved FKs with both resolved and unresolved FKs in theirFKs. + // See GetMatchingKey's documentation for more info. + theirs, ok := theirFKs.GetMatchingKey(ours, ancSchs, true) if !ok { return false, nil } @@ -919,7 +923,7 @@ func foreignKeysInCommon(ourFKs, theirFKs, ancFKs *doltdb.ForeignKeyCollection, return false, err } - anc, ok := ancFKs.GetMatchingKey(ours, ancSchs) + anc, ok := ancFKs.GetMatchingKey(ours, ancSchs, false) if !ok { // FKs added on both branch with different defs conflicts = append(conflicts, FKConflict{ @@ -981,7 +985,7 @@ func foreignKeysInCommon(ourFKs, theirFKs, ancFKs *doltdb.ForeignKeyCollection, func fkCollSetDifference(fkColl, ancestorFkColl *doltdb.ForeignKeyCollection, ancSchs map[string]schema.Schema) (d *doltdb.ForeignKeyCollection, err error) { d, _ = doltdb.NewForeignKeyCollection() err = fkColl.Iter(func(fk doltdb.ForeignKey) (stop bool, err error) { - _, ok := ancestorFkColl.GetMatchingKey(fk, ancSchs) + _, ok := ancestorFkColl.GetMatchingKey(fk, ancSchs, false) if !ok { err = d.AddKeys(fk) } diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go b/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go index 8d7507dc70..0ad20648c2 100755 --- a/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_transaction_queries.go @@ -755,6 +755,64 @@ var DoltTransactionTests = []queries.TransactionTest{ }, }, }, + { + // https://github.com/dolthub/dolt/issues/7956 + Name: "Merge unresolved FKs after resolved FKs were committed", + Assertions: []queries.ScriptTestAssertion{ + { + Query: "/* client a */ SET @@foreign_key_checks=0;", + SkipResultsCheck: true, + }, + { + Query: "/* client a */ SET @@autocommit=0;", + SkipResultsCheck: true, + }, + { + // Create a table for the FK to reference + Query: "/* client a */ CREATE TABLE ref (id varchar(100) PRIMARY KEY, status int);", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + // Create a table with an FK + Query: "/* client a */ CREATE TABLE t (id int, ref_id varchar(100), FOREIGN KEY (ref_id) REFERENCES ref(id));", + Expected: []sql.Row{{types.NewOkResult(0)}}, + }, + { + Query: "/* client a */ COMMIT;", + Expected: []sql.Row{}, + }, + { + // Turn @@foreign_key_checks back on in client a + Query: "/* client a */ SET @@foreign_key_checks=1;", + Expected: []sql.Row{{}}, + }, + { + // Reference the table with an unresolved FK, so that it gets loaded and resolved + Query: "/* client a */ UPDATE t SET ref_id = 42 where ref_id > 100000;", + Expected: []sql.Row{{types.OkResult{Info: plan.UpdateInfo{}}}}, + }, + { + // Make any change in client b's session + Query: "/* client b */ create table foo (i int);", + Expected: []sql.Row{{types.OkResult{RowsAffected: 0}}}, + }, + { + // Client a still has an unresolved FK at this point + Query: "/* client a */ COMMIT;", + Expected: []sql.Row{}, + }, + { + // Assert that client a can see the schema with the foreign key constraints still present + Query: "/* client a */ show create table t;", + Expected: []sql.Row{{"t", "CREATE TABLE `t` (\n `id` int,\n `ref_id` varchar(100),\n KEY `ref_id` (`ref_id`),\n CONSTRAINT `t_ibfk_1` FOREIGN KEY (`ref_id`) REFERENCES `ref` (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + { + // Assert that client b can see the schema with the foreign key constraints still present + Query: "/* client b */ show create table t;", + Expected: []sql.Row{{"t", "CREATE TABLE `t` (\n `id` int,\n `ref_id` varchar(100),\n KEY `ref_id` (`ref_id`),\n CONSTRAINT `t_ibfk_1` FOREIGN KEY (`ref_id`) REFERENCES `ref` (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin"}}, + }, + }, + }, } var DoltConflictHandlingTests = []queries.TransactionTest{