From c8ee8e75af65fefd9976146b49e238eb068e01c7 Mon Sep 17 00:00:00 2001 From: "Mr.Web" Date: Tue, 27 Oct 2026 11:20:49 +0100 Subject: [PATCH 01/13] fix(copy): turn off foreign key checks while Copy To writes rows --- .../Core/ObjectCopy/ObjectCopyRunner.swift | 188 ++++++++++-------- .../ObjectCopyTransactionTests.swift | 56 +++++- apple/changelog.d/0.fixed.md | 1 + 3 files changed, 166 insertions(+), 79 deletions(-) create mode 100644 apple/changelog.d/0.fixed.md diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index f9208a3a5b..4625af5b2b 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -406,6 +406,34 @@ internal struct ObjectCopyRunner { return ranAny } + /// Foreign key checks off for the rows, on the connection that writes them, as `mysqldump` does. + /// + /// Tables are copied in name order, so a child can arrive before its parent. Set outside the + /// transaction because SQLite ignores the pragma inside one. Put back before the lease ends, + /// shielded like a rollback, since the connection returns to the pool. An engine that refuses + /// the statement (Postgres without superuser) keeps its checks, as it did before. + nonisolated private static func withForeignKeysRelaxed( + _ plugin: any PluginDatabaseDriver, + _ body: () async throws -> T + ) async throws -> T { + for statement in plugin.foreignKeyDisableStatements() ?? [] { + _ = try? await plugin.execute(query: statement) + } + func restore() async { + for statement in plugin.foreignKeyEnableStatements() ?? [] { + _ = try? await TaskCancellationShield.run { try await plugin.execute(query: statement) } + } + } + do { + let value = try await body() + await restore() + return value + } catch { + await restore() + throw error + } + } + /// Each statement shielded like a rollback: the connection goes back to the pool after this /// lease, and a setting left behind would apply to whatever leases it next. nonisolated private static func closeSettings( @@ -501,54 +529,56 @@ internal struct ObjectCopyRunner { guard let targetPlugin = CompareMetadataService.pluginDriver(from: targetDriver) else { throw ObjectCopyError.refused(Self.noTargetDriver) } - let owner = await Self.transactionOwner(of: targetDriver) - let usesTransaction = owner.opensTransaction - if usesTransaction { try await targetPlugin.beginTransaction(mode: .readWrite) } - - var result = DataResult() - result.joinedSessionTransaction = owner == .session - let cleared = await Self.execute( - clearGroups, on: targetPlugin, errorHandling: errorHandling, progress: progress - ) - result.outcomes += cleared.outcomes.filter { $0.error != nil } - result.cancelled = cleared.cancelled - result.stopped = cleared.stopped + return try await Self.withForeignKeysRelaxed(targetPlugin) { + let owner = await Self.transactionOwner(of: targetDriver) + let usesTransaction = owner.opensTransaction + if usesTransaction { try await targetPlugin.beginTransaction(mode: .readWrite) } - if !result.stopped { - let copied = await Self.stream( - steps, - from: sourceScope, - into: targetPlugin, - manager: manager, - sourceType: sourceType, - targetType: targetType, - errorHandling: errorHandling, - progress: progress + var result = DataResult() + result.joinedSessionTransaction = owner == .session + let cleared = await Self.execute( + clearGroups, on: targetPlugin, errorHandling: errorHandling, progress: progress ) - result.outcomes += copied.outcomes - result.cancelled = result.cancelled || copied.cancelled - result.stopped = copied.stopped - result.rowsCopied = copied.rowsCopied - } + result.outcomes += cleared.outcomes.filter { $0.error != nil } + result.cancelled = cleared.cancelled + result.stopped = cleared.stopped + + if !result.stopped { + let copied = await Self.stream( + steps, + from: sourceScope, + into: targetPlugin, + manager: manager, + sourceType: sourceType, + targetType: targetType, + errorHandling: errorHandling, + progress: progress + ) + result.outcomes += copied.outcomes + result.cancelled = result.cancelled || copied.cancelled + result.stopped = copied.stopped + result.rowsCopied = copied.rowsCopied + } - guard usesTransaction else { return result } - guard result.stopped, errorHandling != .stopAndCommit else { - /// A commit that fails leaves the transaction open, and the clears are inside it, - /// so it is rolled back rather than left for the lease to decide. - do { - try await targetPlugin.commitTransaction() - } catch { - try? await targetPlugin.rollbackTransaction() - throw error + guard usesTransaction else { return result } + guard result.stopped, errorHandling != .stopAndCommit else { + /// A commit that fails leaves the transaction open, and the clears are inside it, + /// so it is rolled back rather than left for the lease to decide. + do { + try await targetPlugin.commitTransaction() + } catch { + try? await targetPlugin.rollbackTransaction() + throw error + } + return result } + try? await targetPlugin.rollbackTransaction() + /// The target is back where it started, rows and all, so nothing may be reported + /// as copied and no table may be reported as done. + result.outcomes = result.outcomes.filter { $0.error != nil } + result.rowsCopied = 0 return result } - try? await targetPlugin.rollbackTransaction() - /// The target is back where it started, rows and all, so nothing may be reported - /// as copied and no table may be reported as done. - result.outcomes = result.outcomes.filter { $0.error != nil } - result.rowsCopied = 0 - return result } } @@ -729,46 +759,48 @@ internal struct ObjectCopyRunner { guard let targetPlugin = CompareMetadataService.pluginDriver(from: targetDriver) else { throw ObjectCopyError.refused(Self.noTargetDriver) } - let owner = await Self.transactionOwner(of: targetDriver) - let usesTransaction = wrapsInTransaction && owner.opensTransaction - if usesTransaction { - try await targetPlugin.beginTransaction(mode: .readWrite) - } - do { - let outcome = try await copier.copy( - from: sourcePlugin, - to: targetPlugin - ) { rows in - progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) - } - guard usesTransaction else { - /// Nothing of the copy's own to roll back, so every batch already flushed is - /// in the target, or pending in the transaction the session already held, - /// whether the user stopped or not. - return CopiedRows( - outcome: ObjectCopyRowCopier.Outcome( - inserted: outcome.inserted, - cancelled: outcome.cancelled, - committed: outcome.inserted - ), - joinedSessionTransaction: owner == .session - ) - } - if outcome.cancelled { - try? await targetPlugin.rollbackTransaction() - } else { - try await targetPlugin.commitTransaction() - } - return CopiedRows(outcome: outcome, joinedSessionTransaction: false) - } catch { + return try await Self.withForeignKeysRelaxed(targetPlugin) { + let owner = await Self.transactionOwner(of: targetDriver) + let usesTransaction = wrapsInTransaction && owner.opensTransaction if usesTransaction { - if errorHandling == .stopAndCommit { - try? await targetPlugin.commitTransaction() - } else { + try await targetPlugin.beginTransaction(mode: .readWrite) + } + do { + let outcome = try await copier.copy( + from: sourcePlugin, + to: targetPlugin + ) { rows in + progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) + } + guard usesTransaction else { + /// Nothing of the copy's own to roll back, so every batch already flushed is + /// in the target, or pending in the transaction the session already held, + /// whether the user stopped or not. + return CopiedRows( + outcome: ObjectCopyRowCopier.Outcome( + inserted: outcome.inserted, + cancelled: outcome.cancelled, + committed: outcome.inserted + ), + joinedSessionTransaction: owner == .session + ) + } + if outcome.cancelled { try? await targetPlugin.rollbackTransaction() + } else { + try await targetPlugin.commitTransaction() + } + return CopiedRows(outcome: outcome, joinedSessionTransaction: false) + } catch { + if usesTransaction { + if errorHandling == .stopAndCommit { + try? await targetPlugin.commitTransaction() + } else { + try? await targetPlugin.rollbackTransaction() + } } + throw error } - throw error } } } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index 80bf80c55e..0083fcbc89 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -38,11 +38,16 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { private var statements: [String] = [] private var transactions: [String] = [] private var state: PluginSessionTransactionState + private let relaxesForeignKeys: Bool - init(sessionState: PluginSessionTransactionState) { + init(sessionState: PluginSessionTransactionState, relaxesForeignKeys: Bool = false) { state = sessionState + self.relaxesForeignKeys = relaxesForeignKeys } + func foreignKeyDisableStatements() -> [String]? { relaxesForeignKeys ? ["FK OFF"] : nil } + func foreignKeyEnableStatements() -> [String]? { relaxesForeignKeys ? ["FK ON"] : nil } + var executed: [String] { lock.withLock { statements } } var transactionEvents: [String] { lock.withLock { transactions } } @@ -152,4 +157,53 @@ struct ObjectCopyTransactionTests { #expect(driver.transactionEvents.isEmpty) #expect(result.pendingInSessionTransaction) } + + /// Tables arrive in name order, so a child can be written before its parent. The checks have to + /// be off on the connection that writes the rows, and back on before it returns to the pool. + @Test("The data phase writes rows with foreign key checks off and restores them") + func dataPhaseRelaxesForeignKeys() async throws { + let selection = ObjectCopySelection(kind: .table, name: "buildings", schema: nil) + let step = ObjectCopyTableStep( + selection: selection, + dropStatements: [], + sequenceStatements: [], + createStatements: [], + truncateStatements: [], + columns: [], + primaryKeyColumns: ["id"], + sourceQuery: "SELECT \"id\" FROM \"buildings\"", + targetTable: "buildings", + targetSchema: nil, + estimatedRows: nil, + copiesData: true, + copiesIdentityColumn: false, + note: nil + ) + let request = ObjectCopyRequest( + source: endpoint("app"), + destination: .existing(endpoint("staging")), + objects: [selection], + content: .data, + existingPolicy: .replace, + errorHandling: .stopAndRollback, + wrapEachTableInTransaction: true + ) + let plan = ObjectCopyPlan( + request: request, + createsDatabase: false, + tableSteps: [step], + definitionSteps: [], + schemaStatements: [] + ) + let plugin = RecordingCopyDriver(sessionState: .idle, relaxesForeignKeys: true) + let adapter = PluginDriverAdapter( + connection: TestFixtures.makeConnection(type: .duckdb), pluginDriver: plugin + ) + let runner = ObjectCopyRunner(manager: StubMetadataProvider(driver: adapter), gate: AlwaysAllowGate()) + + _ = try await runner.run(plan, progress: ObjectCopyProgress(progress: Progress())) + + #expect(plugin.executed.first == "FK OFF") + #expect(plugin.executed.last == "FK ON") + } } diff --git a/apple/changelog.d/0.fixed.md b/apple/changelog.d/0.fixed.md new file mode 100644 index 0000000000..d5a62f2d20 --- /dev/null +++ b/apple/changelog.d/0.fixed.md @@ -0,0 +1 @@ +Copy To failed with foreign key errors when a child table was copied before its parent. From 0899278ffc49a0746e20c706a4f45882466831d8 Mon Sep 17 00:00:00 2001 From: "Mr.Web" Date: Tue, 27 Oct 2026 11:20:58 +0100 Subject: [PATCH 02/13] chore(changelog): name the fragment after the PR --- apple/changelog.d/{0.fixed.md => 3859.fixed.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename apple/changelog.d/{0.fixed.md => 3859.fixed.md} (100%) diff --git a/apple/changelog.d/0.fixed.md b/apple/changelog.d/3859.fixed.md similarity index 100% rename from apple/changelog.d/0.fixed.md rename to apple/changelog.d/3859.fixed.md From 4bd09faf3865b97cdb54f37dbd9e20e8dc26f030 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 19:12:29 +0700 Subject: [PATCH 03/13] fix(copy): write rows with foreign key checks off and put back what the session had --- .../CloudflareD1Plugin.swift | 2 + .../CloudflareD1PluginDriver.swift | 11 - .../LibSQLPluginDriver.swift | 19 +- .../MySQLPluginDriver+SessionScope.swift | 3 +- .../MySQLSessionScopePlan.swift | 4 + .../MySQLSessionScopeTests.swift | 18 +- .../PostgreSQLPluginDriver+SessionScope.swift | 3 + .../PostgreSQLPluginDriver.swift | 10 +- .../PostgreSQLSessionScopeSQLTests.swift | 22 + .../SQLiteDriverPlugin/SQLitePlugin.swift | 10 + .../SQLiteSessionScopeTests.swift | 96 ++++ .../PluginDatabaseDriver.swift | 2 + .../PluginSessionScope.swift | 4 + .../SQLiteSessionScope.swift | 69 +++ .../Core/Compare/CompareMetadataService.swift | 32 +- .../Core/Compare/CompareRunner+Data.swift | 20 +- .../TablePro/Core/Compare/CompareRunner.swift | 90 +++- .../Core/DataWrite/DataWriteExecutor.swift | 22 + .../Core/ObjectCopy/ObjectCopyPlan.swift | 19 +- .../ObjectCopyPlanner+SourceDefinitions.swift | 80 ++- .../Core/ObjectCopy/ObjectCopyPlanner.swift | 112 ++++- .../Core/ObjectCopy/ObjectCopyRunner.swift | 241 ++++----- .../ObjectCopy/ObjectCopyTableDraft.swift | 3 + ...ginMetadataRegistry+RegistryDefaults.swift | 2 +- .../TablePro/Resources/Localizable.xcstrings | 4 + .../ObjectCopy/CopyObjectsConfigureView.swift | 14 + .../SchemaSyncScriptBuilderTests.swift | 159 ++++++ .../DataWrite/DataWriteExecutorTests.swift | 39 +- .../Core/ObjectCopy/ObjectCopyPlanTests.swift | 145 ++++++ .../ObjectCopyPlannerOrderingTests.swift | 97 +++- .../ObjectCopyPlannerStructureReadTests.swift | 109 +++++ .../ObjectCopyTransactionTests.swift | 460 +++++++++++++----- apple/changelog.d/3859.added.md | 1 + apple/changelog.d/3859.fixed.1.md | 1 + apple/changelog.d/3859.fixed.2.md | 1 + apple/changelog.d/3859.fixed.3.md | 1 + apple/changelog.d/3859.fixed.4.md | 1 + apple/changelog.d/3859.fixed.5.md | 1 + apple/changelog.d/3859.fixed.md | 2 +- 39 files changed, 1593 insertions(+), 336 deletions(-) create mode 100644 apple/Plugins/SQLiteDriverTests/SQLiteSessionScopeTests.swift create mode 100644 apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift create mode 100644 apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerStructureReadTests.swift create mode 100644 apple/changelog.d/3859.added.md create mode 100644 apple/changelog.d/3859.fixed.1.md create mode 100644 apple/changelog.d/3859.fixed.2.md create mode 100644 apple/changelog.d/3859.fixed.3.md create mode 100644 apple/changelog.d/3859.fixed.4.md create mode 100644 apple/changelog.d/3859.fixed.5.md diff --git a/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1Plugin.swift b/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1Plugin.swift index 215ac5cb6e..cff710c29a 100644 --- a/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1Plugin.swift +++ b/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1Plugin.swift @@ -28,6 +28,8 @@ final class CloudflareD1Plugin: NSObject, TableProPlugin, DriverPlugin { static let supportsSSL = false static let isDownloadable = true static let supportsImport = false + // D1 runs each request in a transaction of its own, where user queries cannot change foreign key enforcement. + static let supportsForeignKeyDisable = false static let supportsSchemaEditing = true static let supportsTriggers = true static let supportsDatabaseTriggerBrowse = true diff --git a/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1PluginDriver.swift b/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1PluginDriver.swift index 9a5f766972..1a6ba5844c 100644 --- a/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1PluginDriver.swift +++ b/apple/Plugins/CloudflareD1DriverPlugin/CloudflareD1PluginDriver.swift @@ -42,7 +42,6 @@ final class CloudflareD1PluginDriver: PluginDatabaseDriver, @unchecked Sendable [ .parameterizedQueries, .alterTableDDL, - .foreignKeyToggle, .truncateTable, .cancelQuery, .schemaCompare, @@ -545,16 +544,6 @@ final class CloudflareD1PluginDriver: PluginDatabaseDriver, @unchecked Sendable return "DROP VIEW IF EXISTS \(quoted);\nCREATE VIEW \(quoted) AS\nSELECT * FROM table_name;" } - // MARK: - Foreign Key Checks - - func foreignKeyDisableStatements() -> [String]? { - ["PRAGMA foreign_keys = OFF"] - } - - func foreignKeyEnableStatements() -> [String]? { - ["PRAGMA foreign_keys = ON"] - } - // MARK: - Table Operations func truncateTableStatements(table: String, schema: String?, cascade: Bool) -> [String]? { diff --git a/apple/Plugins/LibSQLDriverPlugin/LibSQLPluginDriver.swift b/apple/Plugins/LibSQLDriverPlugin/LibSQLPluginDriver.swift index 9860cdb4d4..e8bb9c6300 100644 --- a/apple/Plugins/LibSQLDriverPlugin/LibSQLPluginDriver.swift +++ b/apple/Plugins/LibSQLDriverPlugin/LibSQLPluginDriver.swift @@ -45,7 +45,6 @@ final class LibSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { var base: PluginCapabilities = [ .parameterizedQueries, .alterTableDDL, - .foreignKeyToggle, .truncateTable, .cancelQuery, .schemaCompare, @@ -54,6 +53,7 @@ final class LibSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { if isLocalMode { base.insert(.transactions) base.insert(.batchExecute) + base.insert(.foreignKeyToggle) } return base } @@ -581,12 +581,25 @@ final class LibSQLPluginDriver: PluginDatabaseDriver, @unchecked Sendable { // MARK: - Foreign Key Checks + // Remote runs every statement on a stream of its own, so a pragma is gone before the next statement runs. func foreignKeyDisableStatements() -> [String]? { - ["PRAGMA foreign_keys = OFF"] + isLocalMode ? ["PRAGMA foreign_keys = OFF"] : nil } func foreignKeyEnableStatements() -> [String]? { - ["PRAGMA foreign_keys = ON"] + isLocalMode ? ["PRAGMA foreign_keys = ON"] : nil + } + + // MARK: - Session Scope + + func openSessionScope(for purpose: PluginSessionPurpose) async throws -> PluginSessionScope? { + guard isLocalMode else { return nil } + return try await SQLiteSessionScope.open(for: purpose) { try await self.execute(query: $0) } + } + + func closeSessionScope(_ scope: PluginSessionScope) async throws { + guard isLocalMode else { return } + try await SQLiteSessionScope.close(scope) { try await self.execute(query: $0) } } // MARK: - Table Operations diff --git a/apple/Plugins/MySQLDriverPlugin/MySQLPluginDriver+SessionScope.swift b/apple/Plugins/MySQLDriverPlugin/MySQLPluginDriver+SessionScope.swift index 3478a2ec44..04c6eca032 100644 --- a/apple/Plugins/MySQLDriverPlugin/MySQLPluginDriver+SessionScope.swift +++ b/apple/Plugins/MySQLDriverPlugin/MySQLPluginDriver+SessionScope.swift @@ -9,7 +9,8 @@ import TableProMySQLCore import TableProPluginKit internal extension MySQLPluginDriver { - /// Databend has no `sql_mode` to hold, and a server before 5.0.3 cannot filter `SHOW` with `WHERE`. + /// Databend has no `sql_mode` or foreign key checks to hold, and a server before 5.0.3 cannot filter `SHOW` with + /// `WHERE`. func openSessionScope(for purpose: PluginSessionPurpose) async throws -> PluginSessionScope? { let names = MySQLSessionScopePlan.savedNames(for: purpose) guard !flavor.isDatabend, diff --git a/apple/Plugins/MySQLDriverPlugin/MySQLSessionScopePlan.swift b/apple/Plugins/MySQLDriverPlugin/MySQLSessionScopePlan.swift index 291959a2dc..6bca4c79ea 100644 --- a/apple/Plugins/MySQLDriverPlugin/MySQLSessionScopePlan.swift +++ b/apple/Plugins/MySQLDriverPlugin/MySQLSessionScopePlan.swift @@ -35,6 +35,8 @@ internal enum MySQLSessionScopePlan { return preservedNames case .verbatimRead, .verbatimWrite: return verbatimNames + case .relaxedForeignKeys: + return ["foreign_key_checks"] @unknown default: return [] } @@ -57,6 +59,8 @@ internal enum MySQLSessionScopePlan { PluginSessionSetting(name: "sql_mode", value: writeMode(saved: saved)), PluginSessionSetting(name: "time_zone", value: "+00:00"), ] + case .relaxedForeignKeys: + return [PluginSessionSetting(name: "foreign_key_checks", value: "OFF")] @unknown default: return nil } diff --git a/apple/Plugins/MySQLDriverTests/MySQLSessionScopeTests.swift b/apple/Plugins/MySQLDriverTests/MySQLSessionScopeTests.swift index 67741e4379..ea60bb8367 100644 --- a/apple/Plugins/MySQLDriverTests/MySQLSessionScopeTests.swift +++ b/apple/Plugins/MySQLDriverTests/MySQLSessionScopeTests.swift @@ -4,6 +4,7 @@ // import Foundation +import TableProMySQLCore import TableProPluginKit import Testing @@ -105,6 +106,21 @@ struct MySQLSessionScopeTests { #expect(MySQLSessionScopePlan.writeMode(saved: saved) == writeMode) } + /// `SET SESSION foreign_key_checks = 'OFF'` and `'ON'` measured on MySQL 8.4.13, inside a transaction too. + @Test("Relaxing foreign keys saves foreign_key_checks alone, turns it off, and closing turns it back on") + func relaxedForeignKeysPlan() { + let saved = [PluginSessionSetting(name: "foreign_key_checks", value: "ON")] + #expect(MySQLSessionScopePlan.savedNames(for: .relaxedForeignKeys) == ["foreign_key_checks"]) + let applied = MySQLSessionScopePlan.applied(for: .relaxedForeignKeys, saved: saved) + #expect(applied == [PluginSessionSetting(name: "foreign_key_checks", value: "OFF")]) + #expect(applied?.compactMap { MySQLSessionStatements.assignment(name: $0.name, value: $0.value) } == [ + "SET SESSION foreign_key_checks = 'OFF'", + ]) + let restore = MySQLSessionScopePlan.restore(saved: saved, current: applied ?? [], isTransactionOpen: true) + #expect(restore.statements == ["SET SESSION foreign_key_checks = 'ON'"]) + #expect(restore.isComplete) + } + @Test("A write scope with no saved mode adds only NO_AUTO_VALUE_ON_ZERO, and reading ignores the session's mode") func writeModeWithoutSavedMode() { #expect(MySQLSessionScopePlan.writeMode(saved: []) == "NO_AUTO_VALUE_ON_ZERO") @@ -216,7 +232,7 @@ struct MySQLSessionScopeTests { host: "", port: 0, username: "", password: "", database: "", additionalFields: ["driverVariant": MySQLServerFlavor.databendVariant] )) - for purpose in [PluginSessionPurpose.preserve, .verbatimRead, .verbatimWrite] { + for purpose in [PluginSessionPurpose.preserve, .verbatimRead, .verbatimWrite, .relaxedForeignKeys] { #expect(try await driver.openSessionScope(for: purpose) == nil) } let routine = PluginRoutineInfo(name: "cleanup", kind: .procedure) diff --git a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift index 2d30cbd3b3..73dd899b7d 100644 --- a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift +++ b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift @@ -24,6 +24,9 @@ internal extension PostgreSQLPluginDriver { case .verbatimWrite: // What the read pins reads back the same under any settings; nil would make the host drop the read too. return PluginSessionScope(purpose: .verbatimWrite, saved: [], applied: []) + case .relaxedForeignKeys: + // `session_replication_role = replica` would also stop the target's user triggers, and needs a superuser. + return nil @unknown default: return nil } diff --git a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift index f92bf287f3..369188a3df 100644 --- a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift +++ b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift @@ -122,11 +122,17 @@ class PostgreSQLPluginDriver: LibPQBackedDriver, @unchecked Sendable { // MARK: - Foreign Keys func foreignKeyDisableStatements() -> [String]? { - ["SET session_replication_role = replica"] + [Self.settingReplicationRole("replica")] } func foreignKeyEnableStatements() -> [String]? { - ["SET session_replication_role = DEFAULT"] + [Self.settingReplicationRole("DEFAULT")] + } + + /// Setting it takes a superuser or a `SET` grant, and a bare refusal aborts the transaction it runs in. The block + /// swallows the refusal and keeps the transaction, and an allowed `SET` outlives it (measured on 17.11 and 18.6). + private static func settingReplicationRole(_ value: String) -> String { + "DO $$ BEGIN SET session_replication_role = \(value); EXCEPTION WHEN insufficient_privilege THEN NULL; END $$" } /// A duplicated database arrives with `public` alone, so every other schema its tables are diff --git a/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift b/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift index 1829120d69..b39c229f05 100644 --- a/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift +++ b/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift @@ -175,4 +175,26 @@ struct PostgreSQLSessionScopeSQLTests { func readScopeRefusesOtherwise(state: PluginSessionTransactionState) { #expect(!PostgreSQLSessionScopeSQL.canOpenReadScope(in: state)) } + + private static func unconnectedDriver() -> PostgreSQLPluginDriver { + PostgreSQLPluginDriver(config: DriverConnectionConfig(host: "", port: 0, username: "", password: "", database: "")) + } + + /// Measured on PostgreSQL 17.11 and 18.6: for a role without the privilege the block answers DO and the + /// transaction keeps its rows, where the bare `SET` aborts it. + @Test("The foreign key switch swallows a refused SET, both ways") + func foreignKeySwitchSwallowsRefusal() { + let driver = Self.unconnectedDriver() + #expect(driver.foreignKeyDisableStatements() == [ + "DO $$ BEGIN SET session_replication_role = replica; EXCEPTION WHEN insufficient_privilege THEN NULL; END $$", + ]) + #expect(driver.foreignKeyEnableStatements() == [ + "DO $$ BEGIN SET session_replication_role = DEFAULT; EXCEPTION WHEN insufficient_privilege THEN NULL; END $$", + ]) + } + + @Test("Relaxing foreign keys opens no scope, since replica would also stop the target's user triggers") + func relaxedForeignKeysOpensNoScope() async throws { + #expect(try await Self.unconnectedDriver().openSessionScope(for: .relaxedForeignKeys) == nil) + } } diff --git a/apple/Plugins/SQLiteDriverPlugin/SQLitePlugin.swift b/apple/Plugins/SQLiteDriverPlugin/SQLitePlugin.swift index bd2222677e..09956c3069 100644 --- a/apple/Plugins/SQLiteDriverPlugin/SQLitePlugin.swift +++ b/apple/Plugins/SQLiteDriverPlugin/SQLitePlugin.swift @@ -290,6 +290,16 @@ final class SQLitePluginDriver: PluginDatabaseDriver, @unchecked Sendable { ["PRAGMA foreign_keys = ON"] } + // MARK: - Session Scope + + func openSessionScope(for purpose: PluginSessionPurpose) async throws -> PluginSessionScope? { + try await SQLiteSessionScope.open(for: purpose) { try await self.execute(query: $0) } + } + + func closeSessionScope(_ scope: PluginSessionScope) async throws { + try await SQLiteSessionScope.close(scope) { try await self.execute(query: $0) } + } + // MARK: - User Query func executeBoundedQuery(query: String, rowCap: Int) async throws -> PluginQueryResult? { diff --git a/apple/Plugins/SQLiteDriverTests/SQLiteSessionScopeTests.swift b/apple/Plugins/SQLiteDriverTests/SQLiteSessionScopeTests.swift new file mode 100644 index 0000000000..67f9456c1f --- /dev/null +++ b/apple/Plugins/SQLiteDriverTests/SQLiteSessionScopeTests.swift @@ -0,0 +1,96 @@ +// +// SQLiteSessionScopeTests.swift +// SQLiteDriverTests +// + +import Foundation +import TableProPluginKit +import Testing + +struct SQLiteSessionScopeTests { + private func withDriver(_ body: (SQLitePluginDriver) async throws -> Void) async throws { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("sqlite-scope-\(UUID().uuidString).sqlite") + defer { try? FileManager.default.removeItem(at: url) } + let driver = SQLitePluginDriver(config: DriverConnectionConfig( + host: "", port: 0, username: "", password: "", database: url.path + )) + try await driver.connect() + defer { driver.disconnect() } + try await body(driver) + } + + private func foreignKeys(_ driver: SQLitePluginDriver) async throws -> String? { + try await driver.execute(query: "PRAGMA foreign_keys").rows.first?.first?.asText + } + + @Test("Closing a preserve scope turns foreign keys back off after the statements inside turned them on") + func preserveRestoresForeignKeys() async throws { + try await withDriver { driver in + #expect(try await foreignKeys(driver) == "0") + let scope = try #require(try await driver.openSessionScope(for: .preserve)) + #expect(scope.applied.isEmpty) + _ = try await driver.execute(query: "PRAGMA foreign_keys = ON") + try await driver.closeSessionScope(scope) + #expect(try await foreignKeys(driver) == "0") + } + } + + @Test("A preserve scope that cannot put foreign keys back inside a transaction says so") + func preserveCloseInsideTransactionThrows() async throws { + try await withDriver { driver in + let scope = try #require(try await driver.openSessionScope(for: .preserve)) + _ = try await driver.execute(query: "PRAGMA foreign_keys = ON") + _ = try await driver.execute(query: "BEGIN") + await #expect(throws: (any Error).self) { try await driver.closeSessionScope(scope) } + _ = try await driver.execute(query: "ROLLBACK") + try await driver.closeSessionScope(scope) + #expect(try await foreignKeys(driver) == "0") + } + } + + @Test("Relaxing turns foreign keys off for the writes and closing turns them back on") + func relaxTurnsForeignKeysOffAndBack() async throws { + try await withDriver { driver in + _ = try await driver.execute(query: "CREATE TABLE parent (id INTEGER PRIMARY KEY)") + _ = try await driver.execute(query: "CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent (id))") + _ = try await driver.execute(query: "PRAGMA foreign_keys = ON") + let scope = try #require(try await driver.openSessionScope(for: .relaxedForeignKeys)) + #expect(scope.applied == [PluginSessionSetting(name: "foreign_keys", value: "0")]) + #expect(try await foreignKeys(driver) == "0") + _ = try await driver.execute(query: "INSERT INTO child VALUES (1, 7)") + try await driver.closeSessionScope(scope) + #expect(try await foreignKeys(driver) == "1") + } + } + + /// sqlite.org/pragma.html#pragma_foreign_keys: the pragma is a no-op while a transaction is open. + @Test("Relaxing inside a transaction opens no scope and leaves foreign keys on") + func relaxInsideTransactionIsNil() async throws { + try await withDriver { driver in + _ = try await driver.execute(query: "PRAGMA foreign_keys = ON") + _ = try await driver.execute(query: "BEGIN") + #expect(try await driver.openSessionScope(for: .relaxedForeignKeys) == nil) + #expect(try await foreignKeys(driver) == "1") + _ = try await driver.execute(query: "ROLLBACK") + } + } + + @Test("Relaxing a connection whose foreign keys are already off opens no scope") + func relaxWhenAlreadyOffIsNil() async throws { + try await withDriver { driver in + let scope = try await driver.openSessionScope(for: .relaxedForeignKeys) + let value = try await foreignKeys(driver) + #expect(scope == nil) + #expect(value == "0") + } + } + + @Test("The verbatim purposes open no scope", arguments: [PluginSessionPurpose.verbatimRead, .verbatimWrite]) + func verbatimPurposesAreNil(_ purpose: PluginSessionPurpose) async throws { + try await withDriver { driver in + let scope = try await driver.openSessionScope(for: purpose) + #expect(scope == nil) + } + } +} diff --git a/apple/Plugins/TableProPluginKit/PluginDatabaseDriver.swift b/apple/Plugins/TableProPluginKit/PluginDatabaseDriver.swift index ed59378451..2105e052b9 100644 --- a/apple/Plugins/TableProPluginKit/PluginDatabaseDriver.swift +++ b/apple/Plugins/TableProPluginKit/PluginDatabaseDriver.swift @@ -570,7 +570,9 @@ public protocol PluginDatabaseDriver: AnyObject, Sendable { // Table operations (optional — return nil to use app-level fallback) func truncateTableStatements(table: String, schema: String?, cascade: Bool) -> [String]? func dropObjectStatement(name: String, objectType: String, schema: String?, cascade: Bool) -> String? + /// Turns the session's foreign key checks off. Runs outside a transaction, on the connection that writes. func foreignKeyDisableStatements() -> [String]? + /// Turns them on whatever they were before; a caller that needs the previous value back holds a session scope. func foreignKeyEnableStatements() -> [String]? /// `UPDATE SET WHERE ` in this engine's own form, or nil for that form. The host diff --git a/apple/Plugins/TableProPluginKit/PluginSessionScope.swift b/apple/Plugins/TableProPluginKit/PluginSessionScope.swift index de64f78517..199a979805 100644 --- a/apple/Plugins/TableProPluginKit/PluginSessionScope.swift +++ b/apple/Plugins/TableProPluginKit/PluginSessionScope.swift @@ -18,6 +18,10 @@ public enum PluginSessionPurpose: Sendable, Equatable { /// Rows read under `verbatimRead` are written back: the target side of a copy. A driver pins the same /// representation for both purposes, so a value read under one is written under the other unchanged. case verbatimWrite + + /// Rows arrive in an order the target's foreign keys may refuse. The driver turns off the checks it can for this + /// session, changing nothing else the writes do, and closing puts back what it saved. Nil when it cannot. + case relaxedForeignKeys } /// One session setting, named and spelled the way its driver reads and writes it. A nil value is SQL `NULL`. diff --git a/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift b/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift new file mode 100644 index 0000000000..a7f0ac244c --- /dev/null +++ b/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift @@ -0,0 +1,69 @@ +// +// SQLiteSessionScope.swift +// TableProPluginKit +// + +import Foundation + +/// The session scopes of SQLite and libSQL's local file, which run every statement on the one connection the driver +/// holds. Only `PRAGMA foreign_keys` is held there; the verbatim purposes open none. +public enum SQLiteSessionScope { + private static let foreignKeys = "foreign_keys" + private static let on = "1" + private static let off = "0" + + public static func open( + for purpose: PluginSessionPurpose, + execute: (String) async throws -> PluginQueryResult + ) async throws -> PluginSessionScope? { + switch purpose { + case .preserve: + guard let saved = try await readForeignKeys(execute) else { return nil } + return PluginSessionScope(purpose: purpose, saved: [saved], applied: []) + case .relaxedForeignKeys: + guard let saved = try await readForeignKeys(execute), saved.value == on else { return nil } + _ = try await execute(assignment(off)) + // The pragma is a no-op while a transaction is open, and says so only when read back. + guard try await readForeignKeys(execute)?.value == off else { return nil } + let applied = PluginSessionSetting(name: foreignKeys, value: off) + return PluginSessionScope(purpose: purpose, saved: [saved], applied: [applied]) + case .verbatimRead, .verbatimWrite: + return nil + } + } + + public static func close( + _ scope: PluginSessionScope, + execute: (String) async throws -> PluginQueryResult + ) async throws { + guard let saved = scope.saved.first(where: { $0.name == foreignKeys })?.value, + try await readForeignKeys(execute)?.value != saved + else { return } + _ = try await execute(assignment(saved)) + guard try await readForeignKeys(execute)?.value == saved else { + throw SQLiteSessionScopeError.transactionOpen + } + } + + private static func readForeignKeys( + _ execute: (String) async throws -> PluginQueryResult + ) async throws -> PluginSessionSetting? { + switch try await execute("PRAGMA foreign_keys").rows.first?[safe: 0]?.asText?.lowercased() { + case "1", "true": return PluginSessionSetting(name: foreignKeys, value: on) + case "0", "false": return PluginSessionSetting(name: foreignKeys, value: off) + default: return nil + } + } + + private static func assignment(_ value: String) -> String { + "PRAGMA foreign_keys = \(value == on ? "ON" : "OFF")" + } +} + +private enum SQLiteSessionScopeError: LocalizedError { + case transactionOpen + + var errorDescription: String? { + String(localized: "Foreign key enforcement could not be put back while a transaction is open.") + } +} diff --git a/apple/TablePro/Core/Compare/CompareMetadataService.swift b/apple/TablePro/Core/Compare/CompareMetadataService.swift index 278ee0ee29..308c4a927b 100644 --- a/apple/TablePro/Core/Compare/CompareMetadataService.swift +++ b/apple/TablePro/Core/Compare/CompareMetadataService.swift @@ -104,7 +104,7 @@ internal struct CompareMetadataService { /// `names` narrows the read to the objects the caller already knows it wants, matched without /// regard to case because engines disagree on identifier folding. A comparison passes nil and /// reads the whole scope; a copy of one table would otherwise pay four round trips for every - /// other table in the database. + /// other table in the database. Names that cover every table still take the whole-scope reads. /// /// `profile` says which of the four reads this caller actually looks at. A data comparison /// pairs tables and reads their rows, so the indexes and the table metadata it used to fetch @@ -124,20 +124,35 @@ internal struct CompareMetadataService { return try await manager.withMetadataDriver(scope: endpoint.scope) { driver in guard let plugin = Self.pluginDriver(from: driver) else { return [] } - let tables = try await plugin.fetchTables(schema: schema).filter { table in - guard wanted?.contains(table.name.lowercased()) ?? true, - !CompareTableKindClassifier.isSequence(table) else { return false } + let listed = try await plugin.fetchTables(schema: schema).filter { table in + guard !CompareTableKindClassifier.isSequence(table) else { return false } let kind = CompareTableKindClassifier.kind(of: table) return kind == .table || includeViews } return try await Self.read( - tables: tables, schema: schema, profile: profile, - narrowed: wanted != nil, databaseType: databaseType, + tables: listed.filter { wanted?.contains($0.name.lowercased()) ?? true }, + schema: schema, profile: profile, + narrowed: Self.narrows(listed, to: wanted), databaseType: databaseType, indexedKinds: indexedKinds, using: plugin ) } } + /// Whether `wanted` leaves out a table `listed` offers, which is when the whole-scope reads + /// would fetch more than the caller keeps. A Copy To of every table took the per-table reads + /// anyway: 52 statements for 8 MySQL tables, measured, against 5 in bulk. A foreign table and + /// a sequence are never offered for a copy, and a view adds little to a bulk read, so none of + /// them counts. + nonisolated internal static func narrows(_ listed: [PluginTableInfo], to wanted: Set?) -> Bool { + guard let wanted else { return false } + return listed.contains { table in + CompareTableKindClassifier.kind(of: table) == .table + && !CompareTableKindClassifier.isForeign(table) + && !CompareTableKindClassifier.isSequence(table) + && !wanted.contains(table.name.lowercased()) + } + } + /// Both sides at once. /// /// A comparison is two independent reads and used to run them one after the other, so the wall @@ -219,8 +234,9 @@ internal struct CompareMetadataService { /// connection queue behind each other. Its gate was `supportsConnectionPooling`, which answers /// whether a *second* connection is safe, not whether one connection can run two statements. /// - /// `narrowed` says the caller asked for specific tables, so the whole-schema queries would read - /// the rest of the database to throw it away. Those callers keep the per-table reads. + /// `narrowed` says the caller wants only some of the scope's tables (`narrows`), so the + /// whole-schema queries would read the rest of the database to throw it away. Those callers + /// keep the per-table reads. nonisolated internal static func read( tables: [PluginTableInfo], schema: String?, diff --git a/apple/TablePro/Core/Compare/CompareRunner+Data.swift b/apple/TablePro/Core/Compare/CompareRunner+Data.swift index 374101fa37..4227d26368 100644 --- a/apple/TablePro/Core/Compare/CompareRunner+Data.swift +++ b/apple/TablePro/Core/Compare/CompareRunner+Data.swift @@ -122,8 +122,26 @@ internal extension CompareRunner { byTable.append((plan, statements)) } guard !byTable.isEmpty else { return [] } + return Self.orderedAcrossTables( + byTable, + sourceSnapshots: session.sourceSnapshots, + in: Self.referenceNamespace(of: context.source) + ) + } - let foreignKeys = CompareRunner.foreignKeyMap(from: session.sourceSnapshots) + static func orderedAcrossTables( + _ byTable: [(plan: DataComparePlan, statements: DataSyncStatements)], + sourceSnapshots: [String: TableStructureSnapshot], + in namespace: ReferenceNamespace + ) -> [SyncStatement] { + var foreignKeys: [String: [PluginForeignKeyInfo]] = [:] + for entry in byTable { + // A listing that leaves the schema off keys the snapshot by bare name, while the plan takes the endpoint's. + guard let snapshot = sourceSnapshots[entry.plan.id] ?? sourceSnapshots[entry.plan.table] else { continue } + let dependencies = dependencies(of: snapshot, ownSchema: entry.plan.schema, in: namespace) + guard !dependencies.isEmpty else { continue } + foreignKeys[entry.plan.id] = dependencies + } let nodes = byTable.map { ForeignKeyTopologicalSort.Table(name: $0.plan.table, schema: $0.plan.schema) } let parentFirst = ForeignKeyTopologicalSort .ordered(nodes, foreignKeysByTable: foreignKeys, childrenFirst: false) diff --git a/apple/TablePro/Core/Compare/CompareRunner.swift b/apple/TablePro/Core/Compare/CompareRunner.swift index 8f796c9035..3de7b182d5 100644 --- a/apple/TablePro/Core/Compare/CompareRunner.swift +++ b/apple/TablePro/Core/Compare/CompareRunner.swift @@ -435,7 +435,12 @@ internal struct CompareRunner { let sourceDefined = selected .filter { $0.identity.kind != .table } .map { (result: $0, action: session.action(for: $0)) } - let foreignKeys = Self.foreignKeyMap(from: snapshots) + let foreignKeys = Self.structureForeignKeys( + source: snapshots, + in: Self.referenceNamespace(of: context.source), + target: session.targetSnapshots, + in: Self.referenceNamespace(of: context.target) + ) return try await DatabaseManager.shared.withMetadataDriver(scope: context.target.scope) { driver in guard let plugin = CompareMetadataService.pluginDriver(from: driver) else { @@ -492,28 +497,81 @@ internal struct CompareRunner { } } + // MARK: - Foreign keys + + // What a catalog writes into `referencedSchema` for a key inside the endpoint: MySQL's database, PostgreSQL's schema. + internal struct ReferenceNamespace { + internal let name: String? + internal let slot: EngineNamespaceSlot + + internal init(endpoint: DatabaseEndpoint, supportsSchemas: Bool, supportsDatabases: Bool) { + self.name = ObjectCopyNamespace.name( + for: endpoint, supportsSchemas: supportsSchemas, supportsDatabases: supportsDatabases + ) + self.slot = EngineNamespaceSlot(supportsSchemas: supportsSchemas, supportsDatabases: supportsDatabases) + } + + // A database name's case follows `lower_case_table_names`; a schema name is exact. + internal func names(_ schema: String) -> Bool { + guard let name else { return false } + return slot == .database ? schema.lowercased() == name.lowercased() : schema == name + } + } + + internal static func referenceNamespace(of endpoint: DatabaseEndpoint) -> ReferenceNamespace { + ReferenceNamespace( + endpoint: endpoint, + supportsSchemas: PluginManager.shared.supportsSchemaSwitching(for: endpoint.databaseType), + supportsDatabases: PluginManager.shared.supportsDatabaseSwitching(for: endpoint.databaseType) + ) + } + + // A dropped table exists only on the target, so its keys are read there. + internal static func structureForeignKeys( + source: [String: TableStructureSnapshot], + in sourceNamespace: ReferenceNamespace, + target: [String: TableStructureSnapshot], + in targetNamespace: ReferenceNamespace + ) -> [String: [PluginForeignKeyInfo]] { + let targetOnly = target.filter { source[$0.key] == nil } + return foreignKeyMap(from: source, in: sourceNamespace) + .merging(foreignKeyMap(from: targetOnly, in: targetNamespace)) { sourceKeys, _ in sourceKeys } + } + internal static func foreignKeyMap( - from snapshots: [String: TableStructureSnapshot] + from snapshots: [String: TableStructureSnapshot], + in namespace: ReferenceNamespace ) -> [String: [PluginForeignKeyInfo]] { var map: [String: [PluginForeignKeyInfo]] = [:] for snapshot in snapshots.values { - let dependencies = snapshot.foreignKeys.compactMap { foreignKey -> PluginForeignKeyInfo? in - guard let column = foreignKey.columns.first, - let referencedColumn = foreignKey.referencedColumns.first else { return nil } - let referencedSchema = foreignKey.referencedSchema ?? snapshot.schema - /// The sort qualifies `referencedTable` with `referencedSchema` itself, so - /// pre-qualifying here produced `public.public.orders` and dropped every edge. - return PluginForeignKeyInfo( - name: foreignKey.name, - column: column, - referencedTable: foreignKey.referencedTable, - referencedColumn: referencedColumn, - referencedSchema: referencedSchema - ) - } + let dependencies = dependencies(of: snapshot, ownSchema: snapshot.schema, in: namespace) guard !dependencies.isEmpty else { continue } map[snapshot.qualifiedName] = dependencies } return map } + + internal static func dependencies( + of snapshot: TableStructureSnapshot, + ownSchema: String?, + in namespace: ReferenceNamespace + ) -> [PluginForeignKeyInfo] { + snapshot.foreignKeys.compactMap { foreignKey -> PluginForeignKeyInfo? in + guard let column = foreignKey.columns.first, + let referencedColumn = foreignKey.referencedColumns.first else { return nil } + var referencedSchema = foreignKey.referencedSchema ?? ownSchema + // The listing leaves that namespace off its tables, so a key into it points at the child's own schema. + if let named = referencedSchema, namespace.names(named) { + referencedSchema = ownSchema + } + // The sort qualifies `referencedTable` with `referencedSchema`; pre-qualifying gave `public.public.orders`. + return PluginForeignKeyInfo( + name: foreignKey.name, + column: column, + referencedTable: foreignKey.referencedTable, + referencedColumn: referencedColumn, + referencedSchema: referencedSchema + ) + } + } } diff --git a/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift b/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift index c347256c19..0232e0b93e 100644 --- a/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift +++ b/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift @@ -120,6 +120,8 @@ enum DataWriteExecutor { } var sideStatements: [String] = [] + // An epilogue sets a fixed value, so the scope is what gives SQLite back the foreign_keys it had. + let preserved = plan.prologue.isEmpty ? nil : await openPreservingScope(on: driver) func drainEpilogue() async { for statement in plan.epilogue { @@ -132,6 +134,7 @@ enum DataWriteExecutor { ) } } + await closePreservingScope(preserved, on: driver) } for statement in plan.prologue { @@ -239,6 +242,25 @@ enum DataWriteExecutor { return DataWriteRun(results: results, sideStatements: sideStatements) } + private static func openPreservingScope(on driver: DatabaseDriver) async -> PluginSessionScope? { + do { + return try await driver.openSessionScope(for: .preserve) + } catch { + logger.warning("Save runs without saving the session settings: \(error.publicLogShape, privacy: .public)") + return nil + } + } + + // Shielded like a rollback: a cancelled save still has to leave the session as it found it. + private static func closePreservingScope(_ scope: PluginSessionScope?, on driver: DatabaseDriver) async { + guard let scope else { return } + do { + try await TaskCancellationShield.run { try await driver.closeSessionScope(scope) } + } catch { + logger.error("Restoring the session settings after the save failed: \(error.publicLogShape, privacy: .public)") + } + } + private static func rowsFound( by lookup: RowLookup, for step: DataWriteStep, diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index 127c8315fd..9f1bfa8dd4 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -107,6 +107,10 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { internal var id: String { selection.id } + /// Emptied before its rows go back in. A step that copies no data never is: clearing it deleted + /// every row the target had and wrote nothing in their place. + internal var clearsTarget: Bool { copiesData && !truncateStatements.isEmpty } + /// What the DDL phase runs for this table. The truncate is deliberately absent: it belongs to /// the data phase's transaction. internal var ddl: [SyncStatement] { dropStatements + sequenceStatements + createStatements } @@ -224,6 +228,11 @@ internal struct ObjectCopyPlan: Sendable { /// so the run may write that DDL under the target's write scope. A definition read as the /// session is runs as the session is, the way it did before scopes. internal let isStructureReadPinned: Bool + /// True when the plan empties tables and every foreign key in the target that points at one of + /// them comes from a table the plan empties too. Only then can the clears run with the target's + /// checks off: a key from any other table would be left pointing at rows that are gone. False + /// whenever the target's keys could not be read. + internal let clearsAreSelfContained: Bool internal init( request: ObjectCopyRequest, @@ -232,7 +241,8 @@ internal struct ObjectCopyPlan: Sendable { definitionSteps: [ObjectCopyDefinitionStep], schemaStatements: [SyncStatement] = [], skipped: [ObjectCopySkip] = [], - isStructureReadPinned: Bool = false + isStructureReadPinned: Bool = false, + clearsAreSelfContained: Bool = false ) { self.request = request self.createsDatabase = createsDatabase @@ -241,6 +251,7 @@ internal struct ObjectCopyPlan: Sendable { self.schemaStatements = schemaStatements self.skipped = skipped self.isStructureReadPinned = isStructureReadPinned + self.clearsAreSelfContained = clearsAreSelfContained } /// Shown above the script, for the caveats the copy cannot do anything about: a key column the @@ -334,13 +345,9 @@ internal struct ObjectCopyPlan: Sendable { } /// Emptying the tables a data-only replace appends to, children first so a foreign key holds. - /// - /// Only a table whose rows are going back in. A step that copies no data is not in `dataSteps`, - /// so clearing it deletes every row the target had and writes nothing in their place: the - /// review said only that the two sides shared no writable column, and the run reported success. internal var clearGroups: [ObjectCopyStatementGroup] { tableSteps.reversed() - .filter { $0.copiesData && !$0.truncateStatements.isEmpty } + .filter(\.clearsTarget) .map { ObjectCopyStatementGroup($0.selection, $0.truncateStatements) } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift index 2a6879073b..5ca1121ade 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift @@ -34,7 +34,7 @@ internal struct ObjectCopyCreationScopes: Sendable { } internal extension ObjectCopyPlanner { - nonisolated private static let creationLogger = Logger(subsystem: "com.TablePro", category: "ObjectCopyPlanner") + nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "ObjectCopyPlanner") nonisolated static func definitionBuild( for input: ObjectCopyDefinitionInput, @@ -152,12 +152,12 @@ internal extension ObjectCopyPlanner { let wanted = Set(names.map { $0.lowercased() }) let scopes = await VerbatimTransferScopes.open(reading: SessionScopeHandle(plugin)) do { - let tables = try await plugin.fetchTables(schema: schema).filter { wanted.contains($0.name.lowercased()) } + let listed = try await plugin.fetchTables(schema: schema) let reads = try await CompareMetadataService.read( - tables: tables, + tables: listed.filter { wanted.contains($0.name.lowercased()) }, schema: schema, profile: .structure, - narrowed: true, + narrowed: CompareMetadataService.narrows(listed, to: wanted), databaseType: databaseType, indexedKinds: SourceObjectIndexes.carriedKinds(on: databaseType), using: plugin @@ -170,6 +170,76 @@ internal extension ObjectCopyPlanner { } } + /// Whether emptying `tables` breaks only keys between tables the copy empties too. Each key the + /// whole-scope read finds is matched the way `ForeignKeyTopologicalSort` keys its nodes, and a + /// read that is unavailable or fails answers false, which keeps the target's checks on. + nonisolated static func clearsAreSelfContained( + clearing tables: [String], + schema: String?, + namespace: String?, + slot: EngineNamespaceSlot, + using plugin: any PluginDatabaseDriver + ) async throws -> Bool { + guard !tables.isEmpty, plugin.providesBulkForeignKeyFetch else { return false } + let foreignKeys: [String: [PluginForeignKeyInfo]] + do { + foreignKeys = try await plugin.fetchAllForeignKeys(schema: schema) + } catch { + try rethrowCancellation(error) + logger.warning("Target foreign keys unreadable: \(error.publicLogShape, privacy: .public)") + return false + } + /// PostgreSQL reads a nil schema as the session's own, and its keys name that schema. + let namespace = namespace ?? plugin.currentSchema + let cleared = Set(tables.map { ForeignKeyTopologicalSort.Table(name: $0, schema: namespace).identifier }) + return foreignKeys.allSatisfy { table, keys in + cleared.contains(ForeignKeyTopologicalSort.Table(name: table, schema: namespace).identifier) + || !keys.contains { key in + let parent = ForeignKeyTopologicalSort.Table( + name: key.referencedTable, + schema: referencedSchema(of: key, readIn: namespace, slot: slot) ?? namespace + ) + return cleared.contains(parent.identifier) + } + } + } + + /// The key with its referenced schema spelled the way `namespace` spells it, where the two name + /// one container. + nonisolated static func pointedInto( + _ key: PluginForeignKeyInfo, + namespace: String?, + slot: EngineNamespaceSlot + ) -> PluginForeignKeyInfo { + let schema = referencedSchema(of: key, readIn: namespace, slot: slot) + guard schema != key.referencedSchema else { return key } + return PluginForeignKeyInfo( + name: key.name, + column: key.column, + referencedTable: key.referencedTable, + referencedColumn: key.referencedColumn, + referencedDatabase: key.referencedDatabase, + referencedSchema: schema, + onDelete: key.onDelete, + onUpdate: key.onUpdate + ) + } + + /// A MySQL key names its database as the server spells it, and a server with + /// `lower_case_table_names` 1 or 2 takes `Regest` and `regest` as one database, so on the + /// database slot a spelling that differs only by case is the namespace itself. PostgreSQL + /// schemas `App` and `app` are two schemas, so the schema slot never folds. + nonisolated static func referencedSchema( + of key: PluginForeignKeyInfo, + readIn namespace: String?, + slot: EngineNamespaceSlot + ) -> String? { + guard slot == .database, let referenced = key.referencedSchema, let namespace, + ObjectCopyNamespace.isSame(referenced, namespace) + else { return key.referencedSchema } + return namespace + } + nonisolated static func creationScopes( for selections: [ObjectCopySelection], triggerTables: [String], @@ -260,7 +330,7 @@ internal extension ObjectCopyPlanner { result.scopes[selection.id] = try await fetch() } catch { try rethrowCancellation(error) - creationLogger.warning( + logger.warning( "Creation settings read failed for \(selection.kind.rawValue, privacy: .public): \(error.publicLogShape, privacy: .public)" ) result.unreadable.insert(selection.id) diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index 448133d1ca..bbeb12ca67 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -42,8 +42,8 @@ internal struct ObjectCopyPlanner { try refuseUpFront(request) /// One pass per source namespace. A database-level copy on PostgreSQL spans every schema, - /// and each schema's tables have to be read, ordered and written in their own scope: one - /// read against a nil schema answers only whatever the connection is currently on. + /// and each schema's tables have to be read and written in their own scope: one read + /// against a nil schema answers only whatever the connection is currently on. var skipped: [ObjectCopySkip] = [] var tableSteps: [ObjectCopyTableStep] = [] var definitionSteps: [ObjectCopyDefinitionStep] = [] @@ -99,6 +99,7 @@ internal struct ObjectCopyPlanner { passes.append(ScopePass( scope: scope, sourceEndpoint: sourceEndpoint, + sourceNamespace: ObjectCopyNamespace.name(for: sourceEndpoint), targetEndpoint: targetEndpoint, sourceReads: sourceReads, targetObjects: targetObjects, @@ -106,6 +107,9 @@ internal struct ObjectCopyPlanner { skipped: tableSkips )) } + let parentsFirst = Self.orderAcrossScopes( + &passes, slot: EngineNamespaceSlot(databaseType: request.source.databaseType) + ) /// Every source scope of a copy to a chosen target lands in the one schema that was chosen, /// so the names its indexes take are allocated across scopes, not afresh for each. @@ -144,6 +148,7 @@ internal struct ObjectCopyPlanner { skipped: &skipped ) } + tableSteps.sort { parentsFirst($0.selection, $1.selection) } return ObjectCopyPlan( request: request, @@ -152,8 +157,59 @@ internal struct ObjectCopyPlanner { definitionSteps: definitionSteps, schemaStatements: try await buildSchemaStatements(request, namespaces: targetNamespaces), skipped: skipped, - isStructureReadPinned: isStructureReadPinned + isStructureReadPinned: isStructureReadPinned, + clearsAreSelfContained: try await clearsAreSelfContained(request, tableSteps: tableSteps) + ) + } + + /// Settles one dependency order over every scope's tables and puts each pass's drafts in it. + /// + /// Sorted one scope at a time, a PostgreSQL database with `app.orders` referencing `core.users` + /// created `orders` first, because `app` was the first scope. The drafts are reordered before + /// any step is built, because a sequence several tables share is created under the first. + private static func orderAcrossScopes( + _ passes: inout [ScopePass], + slot: EngineNamespaceSlot + ) -> (ObjectCopySelection, ObjectCopySelection) -> Bool { + var reads: [ObjectCopySelection: TableStructureRead] = [:] + for draft in passes.flatMap(\.drafts) { + reads[draft.selection] = draft.read + } + let ordered = orderedByDependency( + passes.map { (namespace: $0.sourceNamespace, tables: $0.drafts.map(\.selection)) }, + reads: reads, + slot: slot ) + let rank = Dictionary( + ordered.enumerated().map { ($0.element, $0.offset) }, uniquingKeysWith: { first, _ in first } + ) + let parentsFirst = { (lhs: ObjectCopySelection, rhs: ObjectCopySelection) in + (rank[lhs] ?? .max) < (rank[rhs] ?? .max) + } + for index in passes.indices { + passes[index].drafts.sort { parentsFirst($0.selection, $1.selection) } + } + return parentsFirst + } + + /// Asked of the target only when the plan empties a table, and in one whole-scope read. + private func clearsAreSelfContained( + _ request: ObjectCopyRequest, + tableSteps: [ObjectCopyTableStep] + ) async throws -> Bool { + let cleared = tableSteps.filter(\.clearsTarget).map(\.targetTable) + guard !cleared.isEmpty else { return false } + /// Every clear is in `request.target`: a duplicate creates its tables, so it empties none. + let target = request.target + let schema = target.schema?.nilIfEmpty + let namespace = ObjectCopyNamespace.name(for: target) + let slot = EngineNamespaceSlot(databaseType: target.databaseType) + return try await manager.withMetadataDriver(scope: targetScope(request, endpoint: target)) { driver in + guard let plugin = CompareMetadataService.pluginDriver(from: driver) else { return false } + return try await Self.clearsAreSelfContained( + clearing: cleared, schema: schema, namespace: namespace, slot: slot, using: plugin + ) + } } private func sourceTableReads( @@ -231,10 +287,14 @@ internal struct ObjectCopyPlanner { private struct ScopePass { let scope: Scope let sourceEndpoint: DatabaseEndpoint + /// The engine's own namespace, not the endpoint's schema. MySQL reports no schema on a + /// table while its foreign keys carry the database name, so ordering by the schema alone + /// matched no edge and fell through to alphabetical order. + let sourceNamespace: String? let targetEndpoint: DatabaseEndpoint let sourceReads: [TableStructureRead] let targetObjects: [String: ObjectCopySelection] - let drafts: [ObjectCopyTableDraft] + var drafts: [ObjectCopyTableDraft] let skipped: [ObjectCopySkip] } @@ -412,20 +472,13 @@ internal struct ObjectCopyPlanner { } guard !reads.isEmpty else { return [] } - /// The engine's own namespace, not the endpoint's schema. MySQL reports no schema on a - /// table while its foreign keys carry the database name, so ordering by the schema alone - /// matched no edge and fell through to alphabetical order. - let sourceNamespace = ObjectCopyNamespace.name(for: sourceEndpoint) - /// Seeded from the user's own order, not from `reads.keys`. Swift seeds Dictionary hashing - /// per process, so taking the keys gave the tables with no foreign key between them a - /// different tie-break on every launch: the approved script, the progress order and the - /// outcome list were all shuffled differently for the same copy. + /// In the user's own order, not `reads.keys`: Swift seeds Dictionary hashing per process, so + /// the tie-break between unrelated tables changed on every launch and shuffled the approved + /// script. `orderAcrossScopes` sorts them once every scope is read. var drafts: [ObjectCopyTableDraft] = [] let targetServerVersion = manager.driver(for: request.target.connectionId)?.serverVersion - for selection in Self.orderedByDependency( - scope.objects.filter { reads[$0] != nil }, reads: reads, effectiveSchema: sourceNamespace - ) { - guard let read = reads[selection], let snapshot = read.sourceSnapshot else { continue } + for selection in scope.objects { + guard let read = reads.removeValue(forKey: selection), let snapshot = read.sourceSnapshot else { continue } let targetRead = match(selection, in: targetReads) let existsInTarget = targetRead != nil if existsInTarget, request.existingPolicy == .skip { @@ -1047,28 +1100,35 @@ internal struct ObjectCopyPlanner { /// Parents before children, so a foreign key in a `CREATE TABLE` finds the table it points at. /// A cycle keeps whatever order the sort settles on and fails at the server, which is the same /// answer Compare & Sync gives it. - /// `effectiveSchema` is the scope the tables were read in, and it is load-bearing. + /// + /// Every scope goes into one sort, because a key may point into another scope's table. Each + /// scope's `namespace` is the one its tables were read in, and it is load-bearing. /// `fetchTables` returns `PluginTableInfo.schema == nil` on MySQL and PostgreSQL while their /// foreign keys carry a non-nil `referencedSchema`, so nodes built from the table's own schema /// were keyed `child` while the dependency named `public.parent`. No edge ever matched, every /// table looked independent, and the sort fell through to alphabetical order: a child could be /// created before its parent and the server rejected it. nonisolated internal static func orderedByDependency( - _ selections: [ObjectCopySelection], + _ scopes: [(namespace: String?, tables: [ObjectCopySelection])], reads: [ObjectCopySelection: TableStructureRead], - effectiveSchema: String? + slot: EngineNamespaceSlot ) -> [ObjectCopySelection] { + let selections = scopes.flatMap { $0.tables } guard selections.count > 1 else { return selections } var foreignKeysByTable: [String: [PluginForeignKeyInfo]] = [:] var nodes: [ForeignKeyTopologicalSort.Table] = [] var bySortKey: [String: ObjectCopySelection] = [:] - for selection in selections { - guard let read = reads[selection] else { continue } - let schema = read.table.schema ?? selection.schema ?? effectiveSchema - let node = ForeignKeyTopologicalSort.Table(name: read.table.name, schema: schema) - nodes.append(node) - foreignKeysByTable[node.identifier] = read.foreignKeys - bySortKey[node.identifier] = selection + for scope in scopes { + for selection in scope.tables { + guard let read = reads[selection] else { continue } + let schema = read.table.schema ?? selection.schema ?? scope.namespace + let node = ForeignKeyTopologicalSort.Table(name: read.table.name, schema: schema) + nodes.append(node) + foreignKeysByTable[node.identifier] = read.foreignKeys.map { + pointedInto($0, namespace: node.schema, slot: slot) + } + bySortKey[node.identifier] = selection + } } var emitted: Set = [] diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index 4625af5b2b..aebfa8eaa9 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -165,11 +165,10 @@ internal struct ObjectCopyRunner { /// the table it points at and a trigger before the table that owns it. One parent-first /// pass had every DROP rejected by the constraint below it. /// - /// Both phases run under one `withMetadataDriver` call and, where the engine has them, - /// with foreign key checks off and a transaction around them. Split across two calls, a - /// Stop between them or a failing first CREATE left every later object dropped with - /// nothing put back. - if !plan.schemaStatements.isEmpty || !plan.cleanupGroups.isEmpty || !plan.creationGroups.isEmpty { + /// Both phases run under one `withMetadataDriver` call, in a transaction or with foreign + /// key checks off. Split across two calls, a Stop between them or a failing first CREATE + /// left every later object dropped with nothing put back. + if !(plan.schemaStatements + plan.cleanupStatements + plan.creationStatements).isEmpty { let result = try await runStructure(plan, request: request, progress: progress) outcomes += result.outcomes cancelled = cancelled || result.cancelled @@ -185,16 +184,20 @@ internal struct ObjectCopyRunner { /// A run that promises a rollback empties them inside the data phase's own transaction /// instead, because a DELETE that has already committed cannot be taken back by anything /// that fails afterwards. Only a run that promises nothing clears here. + var uncleared: Set = [] if !cancelled, !plan.clearGroups.isEmpty, !plan.clearsInsideDataTransaction { let result = try await runDDL(plan.clearGroups, request: request, progress: progress) - outcomes += result.outcomes.filter { $0.error != nil } + let failures = result.outcomes.filter { $0.error != nil } + outcomes += failures + // A table that kept its own rows would get the source's on top of them. + uncleared = Set(failures.map(\.id)) cancelled = cancelled || result.cancelled pendingInSessionTransaction = pendingInSessionTransaction || result.joinedSessionTransaction if result.stopped { return finished() } } if !cancelled, request.content.includesData { - let dataOutcomes = try await runData(plan, request: request, progress: progress) + let dataOutcomes = try await runData(plan, skipping: uncleared, request: request, progress: progress) outcomes += dataOutcomes.outcomes cancelled = cancelled || dataOutcomes.cancelled rowsCopied = dataOutcomes.rowsCopied @@ -237,8 +240,8 @@ internal struct ObjectCopyRunner { /// /// A replacement is destructive only in the moment between its DROP and its CREATE, so the two /// have to be one unit as far as the engine can make them: a transaction where DDL is - /// transactional, and otherwise at least one connection lease with foreign key checks off so - /// the ordering cannot fail half way. + /// transactional and the run promises a rollback, and otherwise at least one connection lease + /// with foreign key checks off so the ordering cannot fail half way. private func runStructure( _ plan: ObjectCopyPlan, request: ObjectCopyRequest, @@ -259,40 +262,36 @@ internal struct ObjectCopyRunner { throw ObjectCopyError.refused(Self.noTargetDriver) } let owner = await Self.transactionOwner(of: driver) - let usesTransaction = hasCleanup && plugin.supportsTransactionalDDL && owner.opensTransaction - let relaxesForeignKeys = hasCleanup && !usesTransaction + // The other two settings keep what ran, and PostgreSQL commits an aborted transaction as a ROLLBACK. + let usesTransaction = hasCleanup && errorHandling == .stopAndRollback + && plugin.supportsTransactionalDDL && owner.opensTransaction if usesTransaction { try await plugin.beginTransaction(mode: .readWrite) } - if relaxesForeignKeys { - for statement in plugin.foreignKeyDisableStatements() ?? [] { - _ = try? await plugin.execute(query: statement) - } - } - var result = try await Self.executeTableDDL( - schemaStatements: schemaStatements, - groups: tableGroups, - on: plugin, - pinsSession: pinsSession, - errorHandling: errorHandling, - progress: progress - ) - /// Outside the write scope, because a routine or trigger records the session's - /// `sql_mode`: it gets its own creation settings, or else the target's. - if !result.stopped { - let definitions = await Self.execute( - definitionGroups, on: plugin, errorHandling: errorHandling, progress: progress + var result: DDLResult = try await Self.withForeignKeysRelaxed( + on: plugin, when: hasCleanup && !usesTransaction && owner != .session + ) { + var ddl = try await Self.executeTableDDL( + schemaStatements: schemaStatements, + groups: tableGroups, + on: plugin, + pinsSession: pinsSession, + errorHandling: errorHandling, + progress: progress ) - result.outcomes += definitions.outcomes - result.cancelled = definitions.cancelled - result.stopped = definitions.stopped + /// Outside the write scope, because a routine or trigger records the session's + /// `sql_mode`: it gets its own creation settings, or else the target's. + if !ddl.stopped { + let definitions = await Self.execute( + definitionGroups, on: plugin, errorHandling: errorHandling, progress: progress + ) + ddl.outcomes += definitions.outcomes + ddl.cancelled = definitions.cancelled + ddl.stopped = definitions.stopped + } + return ddl } result.joinedSessionTransaction = owner == .session - if relaxesForeignKeys { - for statement in plugin.foreignKeyEnableStatements() ?? [] { - _ = try? await plugin.execute(query: statement) - } - } guard usesTransaction else { return result } guard result.stopped else { try await plugin.commitTransaction() @@ -406,30 +405,36 @@ internal struct ObjectCopyRunner { return ranAny } - /// Foreign key checks off for the rows, on the connection that writes them, as `mysqldump` does. - /// - /// Tables are copied in name order, so a child can arrive before its parent. Set outside the - /// transaction because SQLite ignores the pragma inside one. Put back before the lease ends, - /// shielded like a rollback, since the connection returns to the pool. An engine that refuses - /// the statement (Postgres without superuser) keeps its checks, as it did before. + /// Parents already come first, but a cycle, a self reference whose child row comes first, or a + /// source row that already breaks a key is written only with the target's checks off. nonisolated private static func withForeignKeysRelaxed( - _ plugin: any PluginDatabaseDriver, + on plugin: any PluginDatabaseDriver, + when relaxes: Bool, _ body: () async throws -> T ) async throws -> T { - for statement in plugin.foreignKeyDisableStatements() ?? [] { - _ = try? await plugin.execute(query: statement) + guard relaxes else { return try await body() } + let opened: PluginSessionScope? + do { + opened = try await plugin.openSessionScope(for: .relaxedForeignKeys) + } catch { + logger.warning("Copying with foreign key checks on: \(error.publicLogShape, privacy: .public)") + opened = nil } - func restore() async { - for statement in plugin.foreignKeyEnableStatements() ?? [] { - _ = try? await TaskCancellationShield.run { try await plugin.execute(query: statement) } + guard let scope = opened else { return try await body() } + // Shielded like a rollback: the connection goes back to the pool after this lease. + func close() async { + do { + try await TaskCancellationShield.run { try await plugin.closeSessionScope(scope) } + } catch { + logger.error("Restoring foreign key checks failed: \(error.publicLogShape, privacy: .public)") } } do { let value = try await body() - await restore() + await close() return value } catch { - await restore() + await close() throw error } } @@ -488,11 +493,13 @@ internal struct ObjectCopyRunner { private func runData( _ plan: ObjectCopyPlan, + skipping uncleared: Set, request: ObjectCopyRequest, progress: ObjectCopyProgress ) async throws -> DataResult { guard plan.clearsInsideDataTransaction else { - return await runIndependentTables(plan, request: request, progress: progress) + let steps = plan.dataSteps.filter { !uncleared.contains($0.id) } + return await runIndependentTables(steps, request: request, progress: progress) } return try await runClearedTables(plan, request: request, progress: progress) } @@ -514,6 +521,7 @@ internal struct ObjectCopyRunner { ) async throws -> DataResult { let manager = self.manager let clearGroups = plan.clearGroups + let clearsAreSelfContained = plan.clearsAreSelfContained let steps = plan.dataSteps let sourceType = request.source.databaseType let targetType = request.target.databaseType @@ -529,21 +537,27 @@ internal struct ObjectCopyRunner { guard let targetPlugin = CompareMetadataService.pluginDriver(from: targetDriver) else { throw ObjectCopyError.refused(Self.noTargetDriver) } - return try await Self.withForeignKeysRelaxed(targetPlugin) { - let owner = await Self.transactionOwner(of: targetDriver) - let usesTransaction = owner.opensTransaction - if usesTransaction { try await targetPlugin.beginTransaction(mode: .readWrite) } + let owner = await Self.transactionOwner(of: targetDriver) + let usesTransaction = owner.opensTransaction + if usesTransaction { try await targetPlugin.beginTransaction(mode: .readWrite) } - var result = DataResult() - result.joinedSessionTransaction = owner == .session + var result = DataResult() + result.joinedSessionTransaction = owner == .session + func clear() async { let cleared = await Self.execute( clearGroups, on: targetPlugin, errorHandling: errorHandling, progress: progress ) result.outcomes += cleared.outcomes.filter { $0.error != nil } result.cancelled = cleared.cancelled result.stopped = cleared.stopped + } - if !result.stopped { + // A table outside the copy that references a cleared one keeps its own ON DELETE rule. + if !clearsAreSelfContained { await clear() } + if !result.stopped { + try await Self.withForeignKeysRelaxed(on: targetPlugin, when: owner != .session) { + if clearsAreSelfContained { await clear() } + guard !result.stopped else { return } let copied = await Self.stream( steps, from: sourceScope, @@ -559,26 +573,26 @@ internal struct ObjectCopyRunner { result.stopped = copied.stopped result.rowsCopied = copied.rowsCopied } + } - guard usesTransaction else { return result } - guard result.stopped, errorHandling != .stopAndCommit else { - /// A commit that fails leaves the transaction open, and the clears are inside it, - /// so it is rolled back rather than left for the lease to decide. - do { - try await targetPlugin.commitTransaction() - } catch { - try? await targetPlugin.rollbackTransaction() - throw error - } - return result + guard usesTransaction else { return result } + guard result.stopped, errorHandling != .stopAndCommit else { + /// A commit that fails leaves the transaction open, and the clears are inside it, + /// so it is rolled back rather than left for the lease to decide. + do { + try await targetPlugin.commitTransaction() + } catch { + try? await targetPlugin.rollbackTransaction() + throw error } - try? await targetPlugin.rollbackTransaction() - /// The target is back where it started, rows and all, so nothing may be reported - /// as copied and no table may be reported as done. - result.outcomes = result.outcomes.filter { $0.error != nil } - result.rowsCopied = 0 return result } + try? await targetPlugin.rollbackTransaction() + /// The target is back where it started, rows and all, so nothing may be reported + /// as copied and no table may be reported as done. + result.outcomes = result.outcomes.filter { $0.error != nil } + result.rowsCopied = 0 + return result } } @@ -653,14 +667,14 @@ internal struct ObjectCopyRunner { /// Both scopes are open at once, which the planner has already established is safe: two scopes /// on one connection are refused unless both route to the pool. private func runIndependentTables( - _ plan: ObjectCopyPlan, + _ steps: [ObjectCopyTableStep], request: ObjectCopyRequest, progress: ObjectCopyProgress ) async -> DataResult { var result = DataResult() let targetType = request.target.databaseType - for step in plan.dataSteps { + for step in steps { if progress.isCancelled || Task.isCancelled { result.cancelled = true result.stopped = true @@ -759,48 +773,47 @@ internal struct ObjectCopyRunner { guard let targetPlugin = CompareMetadataService.pluginDriver(from: targetDriver) else { throw ObjectCopyError.refused(Self.noTargetDriver) } - return try await Self.withForeignKeysRelaxed(targetPlugin) { - let owner = await Self.transactionOwner(of: targetDriver) - let usesTransaction = wrapsInTransaction && owner.opensTransaction - if usesTransaction { - try await targetPlugin.beginTransaction(mode: .readWrite) - } - do { - let outcome = try await copier.copy( - from: sourcePlugin, - to: targetPlugin - ) { rows in + let owner = await Self.transactionOwner(of: targetDriver) + let usesTransaction = wrapsInTransaction && owner.opensTransaction + if usesTransaction { + try await targetPlugin.beginTransaction(mode: .readWrite) + } + do { + let outcome = try await Self.withForeignKeysRelaxed( + on: targetPlugin, when: owner != .session + ) { + try await copier.copy(from: sourcePlugin, to: targetPlugin) { rows in progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) } - guard usesTransaction else { - /// Nothing of the copy's own to roll back, so every batch already flushed is - /// in the target, or pending in the transaction the session already held, - /// whether the user stopped or not. - return CopiedRows( - outcome: ObjectCopyRowCopier.Outcome( - inserted: outcome.inserted, - cancelled: outcome.cancelled, - committed: outcome.inserted - ), - joinedSessionTransaction: owner == .session - ) - } - if outcome.cancelled { - try? await targetPlugin.rollbackTransaction() + } + guard usesTransaction else { + /// Nothing of the copy's own to roll back, so every batch already flushed is + /// in the target, or pending in the transaction the session already held, + /// whether the user stopped or not. + return CopiedRows( + outcome: ObjectCopyRowCopier.Outcome( + inserted: outcome.inserted, + cancelled: outcome.cancelled, + committed: outcome.inserted + ), + joinedSessionTransaction: owner == .session + ) + } + if outcome.cancelled { + try? await targetPlugin.rollbackTransaction() + } else { + try await targetPlugin.commitTransaction() + } + return CopiedRows(outcome: outcome, joinedSessionTransaction: false) + } catch { + if usesTransaction { + if errorHandling == .stopAndCommit { + try? await targetPlugin.commitTransaction() } else { - try await targetPlugin.commitTransaction() - } - return CopiedRows(outcome: outcome, joinedSessionTransaction: false) - } catch { - if usesTransaction { - if errorHandling == .stopAndCommit { - try? await targetPlugin.commitTransaction() - } else { - try? await targetPlugin.rollbackTransaction() - } + try? await targetPlugin.rollbackTransaction() } - throw error } + throw error } } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift index 5b354b0923..2a9fc5183a 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift @@ -16,6 +16,8 @@ import TableProPluginKit /// each run over the whole list. internal struct ObjectCopyTableDraft { let selection: ObjectCopySelection + /// Kept for the dependency order, which waits until every scope has been read. + let read: TableStructureRead let snapshot: TableStructureSnapshot /// The same table said in the target's own types, which is what the target driver is handed. /// Identical to `snapshot` whenever the two engines share a type family, apart from an index the @@ -54,6 +56,7 @@ internal struct ObjectCopyTableDraft { request: ObjectCopyRequest ) { self.selection = selection + self.read = read self.snapshot = snapshot self.rowScope = request.rowScope(for: selection) self.sourceSchema = sourceSchema diff --git a/apple/TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift b/apple/TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift index 9c5c0ebc69..383f3e0822 100644 --- a/apple/TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift +++ b/apple/TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift @@ -903,7 +903,7 @@ extension PluginMetadataRegistry { supportsSSH: false, supportsSSL: false, supportsCascadeDrop: false, - supportsForeignKeyDisable: true, + supportsForeignKeyDisable: false, supportsReadOnlyMode: true, supportsQueryProgress: false, requiresReconnectForDatabaseSwitch: false, diff --git a/apple/TablePro/Resources/Localizable.xcstrings b/apple/TablePro/Resources/Localizable.xcstrings index 422eaccc99..a1b9eb19a6 100644 --- a/apple/TablePro/Resources/Localizable.xcstrings +++ b/apple/TablePro/Resources/Localizable.xcstrings @@ -84261,6 +84261,10 @@ } } }, + "Foreign key enforcement could not be put back while a transaction is open." : { + "extractionState" : "manual", + "generatesSymbol" : false + }, "Foreign key must have at least one column, a referenced table, and a referenced column" : { "localizations" : { "fr" : { diff --git a/apple/TablePro/Views/ObjectCopy/CopyObjectsConfigureView.swift b/apple/TablePro/Views/ObjectCopy/CopyObjectsConfigureView.swift index 5fa786da36..8b87811a4e 100644 --- a/apple/TablePro/Views/ObjectCopy/CopyObjectsConfigureView.swift +++ b/apple/TablePro/Views/ObjectCopy/CopyObjectsConfigureView.swift @@ -6,6 +6,7 @@ // import SwiftUI +import TableProPluginKit internal struct CopyObjectsConfigureView: View { @ObservedObject internal var session: ObjectCopySession @@ -35,6 +36,7 @@ internal struct CopyObjectsConfigureView: View { destinationSection contentSection existingSection + errorHandlingSection Spacer(minLength: 0) } .frame(maxWidth: .infinity, alignment: .leading) @@ -124,6 +126,18 @@ internal struct CopyObjectsConfigureView: View { } } + private var errorHandlingSection: some View { + labelled(String(localized: "On error")) { + Picker(String(localized: "On error"), selection: $session.errorHandling) { + Text("Stop and roll back").tag(ImportErrorHandling.stopAndRollback) + Text("Stop and keep what ran").tag(ImportErrorHandling.stopAndCommit) + Text("Skip and continue").tag(ImportErrorHandling.skipAndContinue) + } + .labelsHidden() + .accessibilityIdentifier("copy-objects-error-handling") + } + } + @ViewBuilder private func labelled(_ title: String, @ViewBuilder content: () -> some View) -> some View { VStack(alignment: .leading, spacing: 6) { diff --git a/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift b/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift index 4ffe1cfa8a..33fb6109ba 100644 --- a/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift +++ b/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift @@ -5,6 +5,7 @@ import Foundation import TableProPluginKit +import Testing import XCTest @testable import TablePro @@ -324,6 +325,164 @@ final class SchemaSyncScriptBuilderTests: XCTestCase { } } +// The listing leaves the table's schema nil while each key's `referencedSchema` names the endpoint's +// namespace: MySQL's database, PostgreSQL's schema. +@MainActor +struct CompareForeignKeyOrderTests { + private func endpoint(database: String, schema: String?, type: DatabaseType) -> DatabaseEndpoint { + DatabaseEndpoint( + scope: DatabaseScope(connectionId: UUID(), database: database, schema: schema), + connectionName: "server", + databaseType: type, + safeModeLevel: .silent, + color: .blue + ) + } + + private func mysql(_ database: String) -> CompareRunner.ReferenceNamespace { + CompareRunner.ReferenceNamespace( + endpoint: endpoint(database: database, schema: nil, type: .mysql), + supportsSchemas: false, + supportsDatabases: true + ) + } + + private func postgres(_ schema: String) -> CompareRunner.ReferenceNamespace { + CompareRunner.ReferenceNamespace( + endpoint: endpoint(database: "app", schema: schema, type: .postgresql), + supportsSchemas: true, + supportsDatabases: true + ) + } + + private func table( + _ name: String, + schema: String? = nil, + referencing parent: String? = nil, + in referencedSchema: String? = nil + ) -> TableStructureSnapshot { + let foreignKeys = parent.map { parent in + [EditableForeignKeyDefinition( + id: UUID(), name: "fk_\(name)_\(parent)", columns: ["\(parent)_id"], + referencedTable: parent, referencedColumns: ["id"], referencedSchema: referencedSchema, + onDelete: .noAction, onUpdate: .noAction + )] + } ?? [] + return TableStructureSnapshot(name: name, schema: schema, columns: [], foreignKeys: foreignKeys) + } + + private func keyed(_ snapshots: TableStructureSnapshot...) -> [String: TableStructureSnapshot] { + Dictionary(uniqueKeysWithValues: snapshots.map { ($0.qualifiedName, $0) }) + } + + private func rows( + for table: String, + schema: String? = nil + ) -> (plan: DataComparePlan, statements: DataSyncStatements) { + let qualified = schema.map { "\($0).\(table)" } ?? table + return ( + DataComparePlan(table: table, schema: schema, columns: [], scope: DataTableScope(), isEnabled: true), + DataSyncStatements( + inserts: [SyncStatement(sql: "INSERT \(qualified)", objectName: qualified, summary: "")], + deletes: [SyncStatement(sql: "DELETE \(qualified)", objectName: qualified, summary: "")] + ) + ) + } + + @Test(arguments: ["regest", "Regest"]) + func mysqlCreatesAParentBeforeItsChild(database: String) { + let estates = table("estates") + let buildings = table("buildings", referencing: "estates", in: "regest") + let foreignKeys = CompareRunner.structureForeignKeys( + source: keyed(buildings, estates), in: mysql(database), target: [:], in: mysql("regest_copy") + ) + + let ordered = SchemaSyncScriptBuilder.order( + operations: [.createTable(buildings), .createTable(estates)], foreignKeysByTable: foreignKeys + ) + + let names = ordered.map(\.tableName) + #expect(names == ["estates", "buildings"]) + } + + @Test + func mysqlDropsAChildBeforeItsParentFromTheTargetsKeys() { + let estates = table("estates") + let buildings = table("buildings", referencing: "estates", in: "regest_copy") + let foreignKeys = CompareRunner.structureForeignKeys( + source: [:], in: mysql("regest"), target: keyed(buildings, estates), in: mysql("regest_copy") + ) + + let ordered = SchemaSyncScriptBuilder.order( + operations: [.dropTable(name: "estates", schema: nil), .dropTable(name: "buildings", schema: nil)], + foreignKeysByTable: foreignKeys + ) + + let names = ordered.map(\.tableName) + #expect(names == ["buildings", "estates"]) + } + + @Test + func mysqlInsertsAParentFirstAndDeletesItLast() { + let statements = CompareRunner.orderedAcrossTables( + [rows(for: "buildings"), rows(for: "estates")], + sourceSnapshots: keyed(table("buildings", referencing: "estates", in: "regest"), table("estates")), + in: mysql("regest") + ) + + let sql = statements.map(\.sql) + #expect(sql == ["INSERT estates", "INSERT buildings", "DELETE buildings", "DELETE estates"]) + } + + // The plan takes the endpoint's schema, so its id is qualified while the snapshot's key is bare. + @Test + func postgresInsertsAParentFirstWhenTheListingLeavesTheSchemaOff() { + let statements = CompareRunner.orderedAcrossTables( + [rows(for: "buildings", schema: "public"), rows(for: "estates", schema: "public")], + sourceSnapshots: keyed(table("buildings", referencing: "estates", in: "public"), table("estates")), + in: postgres("public") + ) + + let sql = statements.map(\.sql) + #expect(sql == [ + "INSERT public.estates", "INSERT public.buildings", "DELETE public.buildings", "DELETE public.estates", + ]) + } + + @Test + func postgresKeysIntoTheTablesOwnSchemaKeepTheParentFirst() { + let estates = table("estates", schema: "public") + let buildings = table("buildings", schema: "public", referencing: "estates", in: "public") + let foreignKeys = CompareRunner.structureForeignKeys( + source: keyed(buildings, estates), in: postgres("public"), target: [:], in: postgres("public") + ) + + let ordered = SchemaSyncScriptBuilder.order( + operations: [.createTable(buildings), .createTable(estates)], foreignKeysByTable: foreignKeys + ) + + let identifiers = ordered.map(\.tableIdentifier) + #expect(identifiers == ["public.estates", "public.buildings"]) + } + + // `app` and `App` are two PostgreSQL schemas, so a key into the other one is no edge here. + @Test + func postgresSchemaNamesFoldOnlyWhenTheyMatchExactly() { + let estates = table("estates") + let buildings = table("buildings", referencing: "estates", in: "app") + let foreignKeys = CompareRunner.structureForeignKeys( + source: keyed(buildings, estates), in: postgres("App"), target: [:], in: postgres("App") + ) + + let ordered = SchemaSyncScriptBuilder.order( + operations: [.createTable(buildings), .createTable(estates)], foreignKeysByTable: foreignKeys + ) + + let names = ordered.map(\.tableName) + #expect(names == ["buildings", "estates"]) + } +} + final class SyncSafetyClassifierTests: XCTestCase { private let classifier = SyncSafetyClassifier() diff --git a/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift b/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift index c5cda01873..025d91d38f 100644 --- a/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift +++ b/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift @@ -22,11 +22,11 @@ private final class CountingDriver: PluginDriverStub, @unchecked Sendable { /// What the session answers when the executor asks what it is already holding. let sessionState: PluginSessionTransactionState - /// Every call in the order it arrived: statement text, plus "BEGIN", "COMMIT" and "ROLLBACK". + /// Every call in the order it arrived: statement text, plus "BEGIN", "COMMIT", "ROLLBACK" and + /// "open" or "close" with a session scope's purpose. private(set) var trace: [String] = [] private var statementCount = 0 - var executed: [String] { trace.filter { !["BEGIN", "COMMIT", "ROLLBACK"].contains($0) } } var didCommit: Bool { trace.contains("COMMIT") } var didRollBack: Bool { trace.contains("ROLLBACK") } @@ -62,6 +62,15 @@ private final class CountingDriver: PluginDriverStub, @unchecked Sendable { func sessionTransactionState() async -> PluginSessionTransactionState { sessionState } + func openSessionScope(for purpose: PluginSessionPurpose) async throws -> PluginSessionScope? { + trace.append("open \(purpose)") + return PluginSessionScope(purpose: purpose, saved: [], applied: []) + } + + func closeSessionScope(_ scope: PluginSessionScope) async throws { + trace.append("close \(scope.purpose)") + } + func beginTransaction(mode: PluginTransactionAccessMode) async throws { trace.append("BEGIN") } func commitTransaction() async throws { trace.append("COMMIT") } func rollbackTransaction() async throws { @@ -193,7 +202,8 @@ struct DataWriteExecutorTests { #expect(counting.didCommit) } - @Test("The foreign-key disable runs before the transaction, and the re-enable after it") + /// SQLite opens with foreign_keys off, and the epilogue alone would leave it on. + @Test("The foreign-key disable runs before the transaction and the re-enable after it, inside a preserving scope") func togglesRunOutsideTheTransaction() async throws { let counting = CountingDriver(affectedRows: 1) _ = try await DataWriteExecutor.run( @@ -206,15 +216,17 @@ struct DataWriteExecutorTests { ) #expect(counting.trace == [ + "open preserve", "PRAGMA foreign_keys = OFF", "BEGIN", "UPDATE \"t\" SET \"b\" = 0", "COMMIT", "PRAGMA foreign_keys = ON", + "close preserve", ]) } - @Test("Foreign keys are re-enabled after a rollback too") + @Test("Foreign keys are re-enabled and the session's settings put back after a rollback too") func togglesRunAfterRollback() async throws { let counting = CountingDriver(affectedRows: 1, failOnStatement: 2) await #expect(throws: (any Error).self) { @@ -228,8 +240,16 @@ struct DataWriteExecutorTests { ) } - #expect(counting.didRollBack) - #expect(counting.trace.last == "PRAGMA foreign_keys = ON") + #expect(counting.trace.first == "open preserve") + #expect(Array(counting.trace.suffix(3)) == ["ROLLBACK", "PRAGMA foreign_keys = ON", "close preserve"]) + } + + @Test("A plan without a prologue leaves the session's settings alone") + func planWithoutPrologueOpensNoScope() async throws { + let counting = CountingDriver(affectedRows: 1) + _ = try await DataWriteExecutor.run(plan(expectedRowCount: 1), on: driver(counting)) + + #expect(counting.trace == ["BEGIN", "UPDATE \"t\" SET \"b\" = 0", "COMMIT"]) } @Test @@ -247,7 +267,12 @@ struct DataWriteExecutorTests { try await DataWriteExecutor.run(required, on: driver(counting)) } - #expect(counting.executed == ["SET IDENTITY_INSERT [dbo].[t] ON", "SET IDENTITY_INSERT [dbo].[t] OFF"]) + #expect(counting.trace == [ + "open preserve", + "SET IDENTITY_INSERT [dbo].[t] ON", + "SET IDENTITY_INSERT [dbo].[t] OFF", + "close preserve", + ]) #expect(!counting.didCommit) } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift index bd62b9c081..fee0129dc0 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift @@ -19,6 +19,35 @@ private final class TruncatingEngineDriver: PluginDriverStub, @unchecked Sendabl } } +private final class TargetForeignKeyDriver: PluginDriverStub, @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + private let bulk: Bool + private let answer: Result<[String: [PluginForeignKeyInfo]], any Error> + private let sessionSchema: String? + + init( + bulk: Bool = true, + sessionSchema: String? = nil, + _ answer: Result<[String: [PluginForeignKeyInfo]], any Error> + ) { + self.bulk = bulk + self.sessionSchema = sessionSchema + self.answer = answer + } + + var reads: Int { lock.withLock { count } } + + var currentSchema: String? { sessionSchema } + + var providesBulkForeignKeyFetch: Bool { bulk } + + func fetchAllForeignKeys(schema: String?) async throws -> [String: [PluginForeignKeyInfo]] { + lock.withLock { count += 1 } + return try answer.get() + } +} + struct ObjectCopyPlanTests { private func endpoint( _ database: String, @@ -140,6 +169,122 @@ struct ObjectCopyPlanTests { #expect(!(plan([step("orders", truncates: false)]).clearsInsideDataTransaction)) } + // MARK: - Clears with checks off + + private func key(to table: String, in schema: String? = "shop") -> PluginForeignKeyInfo { + PluginForeignKeyInfo( + name: "fk_\(table)", + column: "\(table)_id", + referencedTable: table, + referencedColumn: "id", + referencedDatabase: nil, + referencedSchema: schema + ) + } + + private func selfContained( + _ cleared: [String], + namespace: String? = "shop", + slot: EngineNamespaceSlot = .database, + on driver: TargetForeignKeyDriver + ) async throws -> Bool { + try await ObjectCopyPlanner.clearsAreSelfContained( + clearing: cleared, schema: nil, namespace: namespace, slot: slot, using: driver + ) + } + + /// A key between two cleared tables is refilled by the copy, and one out of a cleared table + /// points at a parent nothing empties. + @Test + func clearsThatOnlyReferenceEachOtherAreSelfContained() async throws { + let driver = TargetForeignKeyDriver(.success([ + "buildings": [key(to: "estates")], + "estates": [key(to: "users")], + "employees": [key(to: "employees")] + ])) + + let contained = try await selfContained(["estates", "buildings"], on: driver) + + #expect(contained) + #expect(driver.reads == 1) + } + + /// Measured on MySQL 8.4: `DELETE FROM estates` with checks off left an unselected `contracts` + /// pointing at rows that were gone. + @Test + func aTableTheCopyLeavesAloneThatReferencesAClearedOneIsNotSelfContained() async throws { + let driver = TargetForeignKeyDriver(.success([ + "buildings": [key(to: "estates")], + "contracts": [key(to: "estates")] + ])) + + let contained = try await selfContained(["estates", "buildings"], on: driver) + + #expect(!contained) + } + + /// The key names the database as the server spells it, which a `lower_case_table_names` 1 + /// server lets differ from the endpoint's spelling. Missing that match would relax the clears. + @Test + func aKeyIntoTheSameDatabaseSpelledInAnotherCaseStillCounts() async throws { + let driver = TargetForeignKeyDriver(.success(["contracts": [key(to: "estates", in: "shop")]])) + + let contained = try await selfContained(["estates"], namespace: "Shop", on: driver) + + #expect(!contained) + } + + /// PostgreSQL reads a nil schema as the session's, and the keys it returns name that schema. + @Test + func aTargetWithNoSchemaIsMatchedInTheSessionsOwn() async throws { + let driver = TargetForeignKeyDriver( + sessionSchema: "public", .success(["items": [key(to: "orders", in: "public")]]) + ) + + let contained = try await selfContained(["orders"], namespace: nil, slot: .schema, on: driver) + + #expect(!contained) + } + + @Test + func aDriverWithoutTheWholeScopeReadIsNeverSelfContained() async throws { + let driver = TargetForeignKeyDriver(bulk: false, .success([:])) + + let contained = try await selfContained(["estates"], on: driver) + + #expect(!contained) + #expect(driver.reads == 0, "a per-table read of every key in the target is not asked for") + } + + @Test + func aFailedReadIsNeverSelfContained() async throws { + let driver = TargetForeignKeyDriver(.failure(CocoaError(.fileReadNoPermission))) + + let contained = try await selfContained(["estates"], on: driver) + + #expect(!contained) + } + + @Test + func aCancelledReadCancelsThePlan() async { + let driver = TargetForeignKeyDriver(.failure(CancellationError())) + + await #expect(throws: CancellationError.self) { + _ = try await self.selfContained(["estates"], on: driver) + } + } + + @Test + func aPlanWithNothingToEmptyIsNotSelfContained() async throws { + let driver = TargetForeignKeyDriver(.success([:])) + + let contained = try await selfContained([], on: driver) + + #expect(!contained) + #expect(driver.reads == 0) + #expect(!plan([step("orders")]).clearsAreSelfContained, "the plan's own default") + } + /// The script is what the user approves, so it has to be in the order the run uses. Shown /// against each table instead, it said the parent was emptied after the child had been filled. @Test diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift index 83e1162625..efacdbea60 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift @@ -48,6 +48,15 @@ struct ObjectCopyPlannerOrderingTests { ) } + private func parentsFirst( + _ selections: [ObjectCopySelection], + reads: [ObjectCopySelection: TableStructureRead] + ) -> [ObjectCopySelection] { + ObjectCopyPlanner.orderedByDependency( + [(namespace: "public", tables: selections)], reads: reads, slot: .schema + ) + } + @Test func aParentIsCreatedBeforeItsChild() { let orders = selection("orders") @@ -57,9 +66,7 @@ struct ObjectCopyPlannerOrderingTests { customers: read("customers") ] - let ordered = ObjectCopyPlanner.orderedByDependency( - [orders, customers], reads: reads, effectiveSchema: "public" - ) + let ordered = parentsFirst([orders, customers], reads: reads) #expect(ordered.map(\.name) == ["customers", "orders"]) } @@ -76,14 +83,68 @@ struct ObjectCopyPlannerOrderingTests { customers: read("customers") ] - let ordered = ObjectCopyPlanner.orderedByDependency( - [orders, customers], reads: reads, effectiveSchema: "public" - ) + let ordered = parentsFirst([orders, customers], reads: reads) #expect(ordered.map(\.name) == ["customers", "orders"], "the edge has to survive the schema the table itself does not report") } + /// Sorted one scope at a time, a PostgreSQL database copy created `app.orders` before the + /// `core.users` it references, because `app` was the first scope. + @Test + func aParentInAnotherScopeIsCreatedFirst() { + let orders = selection("orders", schema: "app") + let users = selection("users", schema: "core") + let reads: [ObjectCopySelection: TableStructureRead] = [ + orders: read("orders", referencing: ["users"], referencedSchema: "core"), + users: read("users") + ] + + let ordered = ObjectCopyPlanner.orderedByDependency( + [(namespace: "app", tables: [orders]), (namespace: "core", tables: [users])], + reads: reads, + slot: .schema + ) + + #expect(ordered.map(\.qualifiedName) == ["core.users", "app.orders"]) + } + + /// A MySQL key names its database the way the server spells it. On a server with + /// `lower_case_table_names` 1 the endpoint can say `Regest` while the key says `regest`. + @Test + func aDatabaseSpelledInAnotherCaseIsTheSameDatabase() { + let buildings = selection("buildings", schema: nil) + let estates = selection("estates", schema: nil) + let reads: [ObjectCopySelection: TableStructureRead] = [ + buildings: read("buildings", referencing: ["estates"], referencedSchema: "regest"), + estates: read("estates") + ] + + let ordered = ObjectCopyPlanner.orderedByDependency( + [(namespace: "Regest", tables: [buildings, estates])], reads: reads, slot: .database + ) + + #expect(ordered.map(\.name) == ["estates", "buildings"]) + } + + /// PostgreSQL schemas `App` and `app` are two schemas, so a key into `App` is no edge in `app`. + /// Without one the tie-break puts `orders` first. + @Test + func aSchemaSpelledInAnotherCaseIsAnotherSchema() { + let orders = selection("orders", schema: "app") + let users = selection("users", schema: "app") + let reads: [ObjectCopySelection: TableStructureRead] = [ + orders: read("orders", referencing: ["users"], referencedSchema: "App"), + users: read("users") + ] + + let ordered = ObjectCopyPlanner.orderedByDependency( + [(namespace: "app", tables: [users, orders])], reads: reads, slot: .schema + ) + + #expect(ordered.map(\.name) == ["orders", "users"]) + } + /// A cycle cannot be ordered, and dropping one of its tables would be worse than letting the /// server refuse the second CREATE, so every input still comes back exactly once. @Test @@ -95,9 +156,7 @@ struct ObjectCopyPlannerOrderingTests { second: read("b", referencing: ["a"]) ] - let ordered = ObjectCopyPlanner.orderedByDependency( - [first, second], reads: reads, effectiveSchema: "public" - ) + let ordered = parentsFirst([first, second], reads: reads) #expect(Set(ordered.map(\.name)) == ["a", "b"]) #expect(ordered.count == 2) @@ -106,9 +165,7 @@ struct ObjectCopyPlannerOrderingTests { @Test func aSingleTableIsReturnedUnchanged() { let only = selection("orders") - #expect(ObjectCopyPlanner.orderedByDependency( - [only], reads: [only: read("orders")], effectiveSchema: "public" - ).map(\.name) == ["orders"]) + #expect(parentsFirst([only], reads: [only: read("orders")]).map(\.name) == ["orders"]) } @Test @@ -302,9 +359,7 @@ struct ObjectCopyPlannerOrderingTests { let unread = names.map { selection($0) } let reads: [ObjectCopySelection: TableStructureRead] = [placed: read("customers")] - let ordered = ObjectCopyPlanner.orderedByDependency( - [placed] + unread, reads: reads, effectiveSchema: "public" - ) + let ordered = parentsFirst([placed] + unread, reads: reads) #expect(ordered.map(\.name) == ["customers"] + names) } @@ -320,12 +375,8 @@ struct ObjectCopyPlannerOrderingTests { var backwards: [ObjectCopySelection: TableStructureRead] = [:] for (selection, name) in zip(selections, names).reversed() { backwards[selection] = read(name) } - let first = ObjectCopyPlanner.orderedByDependency( - selections, reads: forwards, effectiveSchema: "public" - ) - let second = ObjectCopyPlanner.orderedByDependency( - selections, reads: backwards, effectiveSchema: "public" - ) + let first = parentsFirst(selections, reads: forwards) + let second = parentsFirst(selections, reads: backwards) #expect(first.map(\.name) == second.map(\.name)) #expect(Set(first.map(\.name)) == Set(names)) @@ -344,9 +395,7 @@ struct ObjectCopyPlannerOrderingTests { audit: read("audit") ] - let ordered = ObjectCopyPlanner.orderedByDependency( - [orders, audit, customers], reads: reads, effectiveSchema: "public" - ) + let ordered = parentsFirst([orders, audit, customers], reads: reads) guard let parent = ordered.firstIndex(of: customers), let child = ordered.firstIndex(of: orders) else { diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerStructureReadTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerStructureReadTests.swift new file mode 100644 index 0000000000..947d035fc1 --- /dev/null +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerStructureReadTests.swift @@ -0,0 +1,109 @@ +// +// ObjectCopyPlannerStructureReadTests.swift +// TableProTests +// +// What reading the source costs before the review shows. A selection of +// every table takes the whole-schema reads; a strict subset keeps the +// per-table ones. +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +private final class BulkReadCountingDriver: PluginDriverStub, @unchecked Sendable { + private let lock = NSLock() + private var log: [String] = [] + private let listed: [PluginTableInfo] + + init(_ names: [String]) { + listed = names.map { PluginTableInfo(name: $0, type: "TABLE", schema: nil, comment: nil) } + } + + var calls: [String] { lock.withLock { log } } + + private func record(_ call: String) { + lock.withLock { log.append(call) } + } + + private var column: [PluginColumnInfo] { [PluginColumnInfo(name: "id", dataType: "int")] } + + var providesBulkColumnFetch: Bool { true } + var providesBulkIndexFetch: Bool { true } + var providesBulkForeignKeyFetch: Bool { true } + var providesBulkTableMetadataFetch: Bool { true } + + func fetchTables(schema: String?) async throws -> [PluginTableInfo] { listed } + + func fetchAllColumns(schema: String?) async throws -> [String: [PluginColumnInfo]] { + record("fetchAllColumns") + return Dictionary(uniqueKeysWithValues: listed.map { ($0.name, column) }) + } + + func fetchAllIndexes(schema: String?) async throws -> [String: [PluginIndexInfo]] { + record("fetchAllIndexes") + return [:] + } + + func fetchAllForeignKeys(schema: String?) async throws -> [String: [PluginForeignKeyInfo]] { + record("fetchAllForeignKeys") + return [:] + } + + func fetchAllTableMetadata(schema: String?) async throws -> [String: PluginTableMetadata] { + record("fetchAllTableMetadata") + return [:] + } + + func fetchColumns(table: String, schema: String?) async throws -> [PluginColumnInfo] { + record("fetchColumns \(table)") + return column + } +} + +struct ObjectCopyPlannerStructureReadTests { + /// Every name handed over, in another case than the server's, still covers the scope. + @Test + func aSelectionOfEveryTableReadsTheWholeSchema() async throws { + let driver = BulkReadCountingDriver(["estates", "buildings", "users"]) + + let read = try await ObjectCopyPlanner.pinnedTableReads( + names: ["Estates", "buildings", "users"], schema: nil, databaseType: .mysql, using: driver + ) + + #expect(read.reads.map(\.table.name) == ["estates", "buildings", "users"]) + #expect(driver.calls.filter { $0 == "fetchAllColumns" }.count == 1) + #expect(!driver.calls.contains { $0.hasPrefix("fetchColumns") }) + } + + @Test + func aStrictSubsetKeepsThePerTableReads() async throws { + let driver = BulkReadCountingDriver(["estates", "buildings", "users"]) + + let read = try await ObjectCopyPlanner.pinnedTableReads( + names: ["estates"], schema: nil, databaseType: .mysql, using: driver + ) + + #expect(read.reads.map(\.table.name) == ["estates"]) + #expect(!driver.calls.contains("fetchAllColumns")) + #expect(driver.calls.contains("fetchColumns estates")) + #expect(!driver.calls.contains("fetchColumns users")) + } + + /// A copy never offers a foreign table or a sequence, and leaving a view out costs a bulk read + /// little, so none of them makes the selection a subset. + @Test + func onlyATableLeftOutNarrowsTheRead() { + let listed = [ + PluginTableInfo(name: "orders", type: "TABLE", schema: nil, comment: nil), + PluginTableInfo(name: "recent", type: "VIEW", schema: nil, comment: nil), + PluginTableInfo(name: "remote", type: "FOREIGN TABLE", schema: nil, comment: nil), + PluginTableInfo(name: "orders_id_seq", type: "SEQUENCE", schema: nil, comment: nil) + ] + + #expect(!CompareMetadataService.narrows(listed, to: ["orders"])) + #expect(CompareMetadataService.narrows(listed, to: ["recent"])) + #expect(!CompareMetadataService.narrows(listed, to: nil), "a comparison names nothing and reads the scope") + } +} diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index 0083fcbc89..a31311cea4 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -14,74 +14,183 @@ import Foundation import TableProPluginKit import Testing -@MainActor -private final class StubMetadataProvider: ScopedMetadataProviding { - private let driver: DatabaseDriver +private struct StatementFailure: Error {} - init(driver: DatabaseDriver) { - self.driver = driver - } +private final class CopyEventLog: @unchecked Sendable { + private let lock = NSLock() + private var entries: [String] = [] - func withMetadataDriver( - scope: DatabaseScope, - workload: MetadataConnectionPool.Workload, - _ body: @Sendable @escaping (DatabaseDriver) async throws -> T - ) async throws -> T { - try await body(driver) - } + var events: [String] { lock.withLock { entries } } - func browseScope(for connectionId: UUID) -> DatabaseScope? { nil } + func record(_ event: String) { + lock.withLock { entries.append(event) } + } } private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { - private let lock = NSLock() - private var statements: [String] = [] - private var transactions: [String] = [] - private var state: PluginSessionTransactionState - private let relaxesForeignKeys: Bool + private let name: String + private let log: CopyEventLog + private let sessionState: PluginSessionTransactionState + private let failing: Set + private let failsInserts: Bool + private let cancelsOnInsert: Bool - init(sessionState: PluginSessionTransactionState, relaxesForeignKeys: Bool = false) { - state = sessionState - self.relaxesForeignKeys = relaxesForeignKeys + init( + _ name: String, + log: CopyEventLog, + sessionState: PluginSessionTransactionState = .idle, + failing: Set = [], + failsInserts: Bool = false, + cancelsOnInsert: Bool = false + ) { + self.name = name + self.log = log + self.sessionState = sessionState + self.failing = failing + self.failsInserts = failsInserts + self.cancelsOnInsert = cancelsOnInsert } - func foreignKeyDisableStatements() -> [String]? { relaxesForeignKeys ? ["FK OFF"] : nil } - func foreignKeyEnableStatements() -> [String]? { relaxesForeignKeys ? ["FK ON"] : nil } - - var executed: [String] { lock.withLock { statements } } - var transactionEvents: [String] { lock.withLock { transactions } } - var capabilities: PluginCapabilities { [] } var supportsTransactions: Bool { true } var supportsTransactionalDDL: Bool { true } - func sessionTransactionState() async -> PluginSessionTransactionState { lock.withLock { state } } + private func record(_ event: String) { + log.record("\(name) \(event)") + } + + func sessionTransactionState() async -> PluginSessionTransactionState { sessionState } + + func openSessionScope(for purpose: PluginSessionPurpose) async throws -> PluginSessionScope? { + record("open \(purpose)") + guard purpose == .relaxedForeignKeys else { return nil } + return PluginSessionScope(purpose: purpose, saved: [], applied: []) + } + + func closeSessionScope(_ scope: PluginSessionScope) async throws { + record(Task.isCancelled ? "close \(scope.purpose) while cancelled" : "close \(scope.purpose)") + } func execute(query: String) async throws -> PluginQueryResult { - lock.withLock { statements.append(query) } - return PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 0, executionTime: 0) + record(query) + if failing.contains(query) { throw StatementFailure() } + return PluginQueryResult( + columns: ["id"], columnTypeNames: [], rows: [[.text("1")]], rowsAffected: 1, executionTime: 0 + ) + } + + func executeParameterized(query: String, parameters: [PluginCellValue]) async throws -> PluginQueryResult { + record("insert") + if cancelsOnInsert { + withUnsafeCurrentTask { task in + guard let task else { return } + task.cancel() + } + } + if failsInserts { throw StatementFailure() } + return PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 1, executionTime: 0) + } + + func beginTransaction() async throws { record("begin") } + func commitTransaction() async throws { record("commit") } + func rollbackTransaction() async throws { record("rollback") } +} + +@MainActor +private final class StubMetadataProvider: ScopedMetadataProviding { + private let drivers: [UUID: DatabaseDriver] + + init(drivers: [UUID: DatabaseDriver]) { + self.drivers = drivers } - func beginTransaction() async throws { lock.withLock { transactions.append("begin") } } - func commitTransaction() async throws { lock.withLock { transactions.append("commit") } } - func rollbackTransaction() async throws { lock.withLock { transactions.append("rollback") } } + func withMetadataDriver( + scope: DatabaseScope, + workload: MetadataConnectionPool.Workload, + _ body: @Sendable @escaping (DatabaseDriver) async throws -> T + ) async throws -> T { + guard let driver = drivers[scope.connectionId] else { + throw ObjectCopyError.refused("No driver for \(scope.connectionId)") + } + return try await body(driver) + } + + func browseScope(for connectionId: UUID) -> DatabaseScope? { nil } } @MainActor struct ObjectCopyTransactionTests { - private func endpoint(_ database: String) -> DatabaseEndpoint { + private let sourceScope = DatabaseScope(connectionId: UUID(), database: "app", schema: nil) + private let targetScope = DatabaseScope(connectionId: UUID(), database: "staging", schema: nil) + + // Different engines, so the copier opens no scope of its own and the log holds only the runner's. + private let sourceType = DatabaseType.postgresql + private let targetType = DatabaseType.mysql + + private func endpoint(_ scope: DatabaseScope, _ type: DatabaseType) -> DatabaseEndpoint { DatabaseEndpoint( - scope: DatabaseScope(connectionId: UUID(), database: database, schema: nil), - connectionName: "server", - databaseType: .duckdb, + scope: scope, + connectionName: scope.database, + databaseType: type, safeModeLevel: .silent, color: .blue ) } + private func request( + _ selections: [ObjectCopySelection], + content: ObjectCopyContent, + errorHandling: ImportErrorHandling = .stopAndRollback + ) -> ObjectCopyRequest { + ObjectCopyRequest( + source: endpoint(sourceScope, sourceType), + destination: .existing(endpoint(targetScope, targetType)), + objects: selections, + content: content, + existingPolicy: .replace, + errorHandling: errorHandling, + wrapEachTableInTransaction: true + ) + } + + private func dataStep(_ table: String, clears: Bool = false) -> ObjectCopyTableStep { + ObjectCopyTableStep( + selection: ObjectCopySelection(kind: .table, name: table, schema: nil), + dropStatements: [], + sequenceStatements: [], + createStatements: [], + truncateStatements: clears + ? [SyncStatement(sql: "DELETE FROM \(table)", objectName: table, summary: "clear")] + : [], + columns: ["id"], + primaryKeyColumns: ["id"], + sourceQuery: "SELECT id FROM \(table)", + targetTable: table, + targetSchema: nil, + estimatedRows: nil, + copiesData: true, + copiesIdentityColumn: false, + note: nil + ) + } + + private func dataPlan( + _ steps: [ObjectCopyTableStep], + errorHandling: ImportErrorHandling = .stopAndRollback, + clearsAreSelfContained: Bool = false + ) -> ObjectCopyPlan { + ObjectCopyPlan( + request: request(steps.map(\.selection), content: .data, errorHandling: errorHandling), + createsDatabase: false, + tableSteps: steps, + definitionSteps: [], + clearsAreSelfContained: clearsAreSelfContained + ) + } + /// A replacement: the drop and the create are one unit, which is what makes the structure phase /// want a transaction of its own. - private func replacementPlan() -> ObjectCopyPlan { + private func replacementPlan(errorHandling: ImportErrorHandling = .stopAndRollback) -> ObjectCopyPlan { let selection = ObjectCopySelection(kind: .table, name: "orders", schema: nil) let step = ObjectCopyTableStep( selection: selection, @@ -93,7 +202,7 @@ struct ObjectCopyTransactionTests { truncateStatements: [], columns: [], primaryKeyColumns: ["id"], - sourceQuery: "SELECT \"id\" FROM \"orders\"", + sourceQuery: "SELECT id FROM orders", targetTable: "orders", targetSchema: nil, estimatedRows: nil, @@ -101,50 +210,57 @@ struct ObjectCopyTransactionTests { copiesIdentityColumn: false, note: nil ) - let request = ObjectCopyRequest( - source: endpoint("app"), - destination: .existing(endpoint("staging")), - objects: [selection], - content: .structure, - existingPolicy: .replace, - errorHandling: .stopAndRollback, - wrapEachTableInTransaction: true - ) return ObjectCopyPlan( - request: request, + request: request([selection], content: .structure, errorHandling: errorHandling), createsDatabase: false, tableSteps: [step], - definitionSteps: [], - schemaStatements: [] + definitionSteps: [] ) } - private func run(sessionState: PluginSessionTransactionState) async throws -> (ObjectCopyRunResult, RecordingCopyDriver) { - let plugin = RecordingCopyDriver(sessionState: sessionState) - let connection = TestFixtures.makeConnection(type: .duckdb) - let adapter = PluginDriverAdapter(connection: connection, pluginDriver: plugin) - let runner = ObjectCopyRunner(manager: StubMetadataProvider(driver: adapter), gate: AlwaysAllowGate()) - let result = try await runner.run(replacementPlan(), progress: ObjectCopyProgress(progress: Progress())) - return (result, plugin) + private func run( + _ plan: ObjectCopyPlan, + source: RecordingCopyDriver, + target: RecordingCopyDriver + ) async throws -> ObjectCopyRunResult { + let provider = StubMetadataProvider(drivers: [ + sourceScope.connectionId: PluginDriverAdapter( + connection: TestFixtures.makeConnection(type: sourceType), pluginDriver: source + ), + targetScope.connectionId: PluginDriverAdapter( + connection: TestFixtures.makeConnection(type: targetType), pluginDriver: target + ), + ]) + let runner = ObjectCopyRunner(manager: provider, gate: AlwaysAllowGate()) + return try await runner.run(plan, progress: ObjectCopyProgress(progress: Progress())) } + // MARK: - Structure + @Test("An idle session lets the copy open its own transaction") func idleSessionKeepsTheCopysOwnTransaction() async throws { - let (result, driver) = try await run(sessionState: .idle) + let log = CopyEventLog() + + let result = try await run( + replacementPlan(), source: RecordingCopyDriver("source", log: log), target: RecordingCopyDriver("target", log: log) + ) - #expect(driver.transactionEvents == ["begin", "commit"]) - #expect(driver.executed == ["DROP TABLE orders;", "CREATE TABLE orders (id INTEGER);"]) + #expect(log.events == [ + "target begin", "target DROP TABLE orders;", "target CREATE TABLE orders (id INTEGER);", "target commit", + ]) #expect(result.pendingInSessionTransaction == false) } /// The statements still run: they join the transaction the user opened, and only the user can /// end it. Opening one over it is what aborted it on DuckDB. - @Test("A session holding a transaction is joined rather than wrapped") + @Test("A session holding a transaction is joined rather than wrapped, with its keys as they are") func openSessionTransactionIsJoined() async throws { - let (result, driver) = try await run(sessionState: .inTransaction) + let log = CopyEventLog() + let target = RecordingCopyDriver("target", log: log, sessionState: .inTransaction) - #expect(driver.transactionEvents.isEmpty) - #expect(driver.executed == ["DROP TABLE orders;", "CREATE TABLE orders (id INTEGER);"]) + let result = try await run(replacementPlan(), source: RecordingCopyDriver("source", log: log), target: target) + + #expect(log.events == ["target DROP TABLE orders;", "target CREATE TABLE orders (id INTEGER);"]) #expect(result.pendingInSessionTransaction) } @@ -152,58 +268,184 @@ struct ObjectCopyTransactionTests { /// so the copy sends none. @Test("A session holding locks is joined too") func sessionLocksAreJoined() async throws { - let (result, driver) = try await run(sessionState: .holdsSessionLocks) + let log = CopyEventLog() + let target = RecordingCopyDriver("target", log: log, sessionState: .holdsSessionLocks) + + let result = try await run(replacementPlan(), source: RecordingCopyDriver("source", log: log), target: target) - #expect(driver.transactionEvents.isEmpty) + #expect(!log.events.contains("target begin")) #expect(result.pendingInSessionTransaction) } - /// Tables arrive in name order, so a child can be written before its parent. The checks have to - /// be off on the connection that writes the rows, and back on before it returns to the pool. - @Test("The data phase writes rows with foreign key checks off and restores them") - func dataPhaseRelaxesForeignKeys() async throws { - let selection = ObjectCopySelection(kind: .table, name: "buildings", schema: nil) - let step = ObjectCopyTableStep( - selection: selection, - dropStatements: [], - sequenceStatements: [], - createStatements: [], - truncateStatements: [], - columns: [], - primaryKeyColumns: ["id"], - sourceQuery: "SELECT \"id\" FROM \"buildings\"", - targetTable: "buildings", - targetSchema: nil, - estimatedRows: nil, - copiesData: true, - copiesIdentityColumn: false, - note: nil + /// PostgreSQL commits an aborted transaction as a ROLLBACK, so a run that keeps going past a + /// failure would report tables that were never created. + @Test( + "Only a run that promises a rollback puts the structure in a transaction", + arguments: [ImportErrorHandling.skipAndContinue, .stopAndCommit] + ) + func structureWithoutRollbackHasNoTransaction(errorHandling: ImportErrorHandling) async throws { + let log = CopyEventLog() + + _ = try await run( + replacementPlan(errorHandling: errorHandling), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) ) - let request = ObjectCopyRequest( - source: endpoint("app"), - destination: .existing(endpoint("staging")), - objects: [selection], - content: .data, - existingPolicy: .replace, - errorHandling: .stopAndRollback, - wrapEachTableInTransaction: true + + #expect(log.events == [ + "target open relaxedForeignKeys", + "target DROP TABLE orders;", + "target CREATE TABLE orders (id INTEGER);", + "target close relaxedForeignKeys", + ]) + } + + // MARK: - Rows + + @Test("Rows are written with the target's keys relaxed inside its transaction, and the source left alone") + func rowsAreRelaxedOnTheTargetInsideItsTransaction() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) ) - let plan = ObjectCopyPlan( - request: request, - createsDatabase: false, - tableSteps: [step], - definitionSteps: [], - schemaStatements: [] + + #expect(result.rowsCopied == 1) + #expect(log.events == [ + "target begin", + "target open relaxedForeignKeys", + "source SELECT id FROM buildings", + "target insert", + "target close relaxedForeignKeys", + "target commit", + ]) + } + + /// Relaxed, the DELETE would leave rows in a table outside the copy pointing at nothing. + @Test("A clear that a table outside the copy references runs before the keys are relaxed") + func referencedClearRunsWithKeysOn() async throws { + let log = CopyEventLog() + + _ = try await run( + dataPlan([dataStep("buildings", clears: true)]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) + ) + + #expect(log.events == [ + "target begin", + "target DELETE FROM buildings", + "target open relaxedForeignKeys", + "source SELECT id FROM buildings", + "target insert", + "target close relaxedForeignKeys", + "target commit", + ]) + } + + /// A self reference or a cycle refuses its own DELETE on MySQL with the checks on. + @Test("Clears that only the copy's own tables reference run with the keys relaxed") + func selfContainedClearRunsRelaxed() async throws { + let log = CopyEventLog() + + _ = try await run( + dataPlan([dataStep("buildings", clears: true)], clearsAreSelfContained: true), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) + ) + + #expect(log.events == [ + "target begin", + "target open relaxedForeignKeys", + "target DELETE FROM buildings", + "source SELECT id FROM buildings", + "target insert", + "target close relaxedForeignKeys", + "target commit", + ]) + } + + @Test("A copy that throws puts the target's keys back before rolling back") + func failedCopyRestoresKeys() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, failsInserts: true) ) - let plugin = RecordingCopyDriver(sessionState: .idle, relaxesForeignKeys: true) - let adapter = PluginDriverAdapter( - connection: TestFixtures.makeConnection(type: .duckdb), pluginDriver: plugin + + #expect(result.failedCount == 1) + #expect(log.events == [ + "target begin", + "target open relaxedForeignKeys", + "source SELECT id FROM buildings", + "target insert", + "target close relaxedForeignKeys", + "target rollback", + ]) + } + + @Test("A stopped copy puts the target's keys back outside the cancellation") + func cancelledCopyRestoresKeys() async throws { + let log = CopyEventLog() + let plan = dataPlan([dataStep("buildings")]) + let source = RecordingCopyDriver("source", log: log) + let target = RecordingCopyDriver("target", log: log, cancelsOnInsert: true) + + let result = try await Task { try await run(plan, source: source, target: target) }.value + + #expect(result.cancelled) + #expect(log.events == [ + "target begin", + "target open relaxedForeignKeys", + "source SELECT id FROM buildings", + "target insert", + "target close relaxedForeignKeys", + "target rollback", + ]) + } + + @Test("Rows joining the session's own transaction keep its keys as they are") + func sessionTransactionIsNotRelaxed() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, sessionState: .inTransaction) ) - let runner = ObjectCopyRunner(manager: StubMetadataProvider(driver: adapter), gate: AlwaysAllowGate()) - _ = try await runner.run(plan, progress: ObjectCopyProgress(progress: Progress())) + #expect(result.pendingInSessionTransaction) + #expect(log.events == ["source SELECT id FROM buildings", "target insert"]) + } + + /// The table still holds its own rows, so the source's would be appended beside them. + @Test("Under Skip and continue a table whose clear failed gets no rows") + func failedClearGetsNoRows() async throws { + let log = CopyEventLog() + let target = RecordingCopyDriver("target", log: log, failing: ["DELETE FROM buildings"]) + + let result = try await run( + dataPlan( + [dataStep("estates", clears: true), dataStep("buildings", clears: true)], + errorHandling: .skipAndContinue + ), + source: RecordingCopyDriver("source", log: log), + target: target + ) - #expect(plugin.executed.first == "FK OFF") - #expect(plugin.executed.last == "FK ON") + #expect(result.failures.map { $0.outcome.selection.name } == ["buildings"]) + #expect(result.rowsCopied == 1) + #expect(log.events == [ + "target DELETE FROM buildings", + "target DELETE FROM estates", + "target open relaxedForeignKeys", + "source SELECT id FROM estates", + "target insert", + "target close relaxedForeignKeys", + ]) } } diff --git a/apple/changelog.d/3859.added.md b/apple/changelog.d/3859.added.md new file mode 100644 index 0000000000..16c7fb1157 --- /dev/null +++ b/apple/changelog.d/3859.added.md @@ -0,0 +1 @@ +Copy To can stop and keep what ran, or skip a failing object and continue. diff --git a/apple/changelog.d/3859.fixed.1.md b/apple/changelog.d/3859.fixed.1.md new file mode 100644 index 0000000000..0122ae3c72 --- /dev/null +++ b/apple/changelog.d/3859.fixed.1.md @@ -0,0 +1 @@ +Compare & Sync on MySQL and PostgreSQL wrote tables in name order instead of parents first. diff --git a/apple/changelog.d/3859.fixed.2.md b/apple/changelog.d/3859.fixed.2.md new file mode 100644 index 0000000000..e6ea911bc0 --- /dev/null +++ b/apple/changelog.d/3859.fixed.2.md @@ -0,0 +1 @@ +SQL import and Ignore foreign key checks left a SQLite connection enforcing foreign keys afterwards. diff --git a/apple/changelog.d/3859.fixed.3.md b/apple/changelog.d/3859.fixed.3.md new file mode 100644 index 0000000000..8cf7ae481a --- /dev/null +++ b/apple/changelog.d/3859.fixed.3.md @@ -0,0 +1 @@ +Copy To read a whole database one table at a time before showing the review. diff --git a/apple/changelog.d/3859.fixed.4.md b/apple/changelog.d/3859.fixed.4.md new file mode 100644 index 0000000000..7340a49a21 --- /dev/null +++ b/apple/changelog.d/3859.fixed.4.md @@ -0,0 +1 @@ +Duplicating a PostgreSQL database failed when a table referenced one in another schema. diff --git a/apple/changelog.d/3859.fixed.5.md b/apple/changelog.d/3859.fixed.5.md new file mode 100644 index 0000000000..59d9def6fe --- /dev/null +++ b/apple/changelog.d/3859.fixed.5.md @@ -0,0 +1 @@ +Disabling foreign key checks on PostgreSQL without superuser failed the import or aborted the open transaction. diff --git a/apple/changelog.d/3859.fixed.md b/apple/changelog.d/3859.fixed.md index d5a62f2d20..329492e5b9 100644 --- a/apple/changelog.d/3859.fixed.md +++ b/apple/changelog.d/3859.fixed.md @@ -1 +1 @@ -Copy To failed with foreign key errors when a child table was copied before its parent. +Copy To stopped with a foreign key error on MySQL tables that reference each other or themselves. From ef8e76b2cc17a577cc621065113e5bc0a35e4824 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 20:38:30 +0700 Subject: [PATCH 04/13] fix(copy): keep foreign key checks on for clears and skip, and match names exactly --- .../TablePro/Core/Compare/CompareRunner.swift | 7 +- .../Core/ObjectCopy/ObjectCopyPlan.swift | 21 +-- .../ObjectCopyPlanner+SourceDefinitions.swift | 74 +-------- .../Core/ObjectCopy/ObjectCopyPlanner.swift | 40 +---- .../Core/ObjectCopy/ObjectCopyRunner.swift | 53 +++---- .../TableProMobile/Localizable.xcstrings | 4 + .../SchemaSyncScriptBuilderTests.swift | 13 +- .../ObjectCopyDDLSessionTests.swift | 14 +- .../Core/ObjectCopy/ObjectCopyPlanTests.swift | 150 +----------------- .../ObjectCopyPlannerOrderingTests.swift | 19 ++- .../ObjectCopySessionScopeTests.swift | 7 +- .../ObjectCopyTransactionTests.swift | 41 +---- 12 files changed, 92 insertions(+), 351 deletions(-) diff --git a/apple/TablePro/Core/Compare/CompareRunner.swift b/apple/TablePro/Core/Compare/CompareRunner.swift index 3de7b182d5..f867d957ba 100644 --- a/apple/TablePro/Core/Compare/CompareRunner.swift +++ b/apple/TablePro/Core/Compare/CompareRunner.swift @@ -502,19 +502,16 @@ internal struct CompareRunner { // What a catalog writes into `referencedSchema` for a key inside the endpoint: MySQL's database, PostgreSQL's schema. internal struct ReferenceNamespace { internal let name: String? - internal let slot: EngineNamespaceSlot internal init(endpoint: DatabaseEndpoint, supportsSchemas: Bool, supportsDatabases: Bool) { self.name = ObjectCopyNamespace.name( for: endpoint, supportsSchemas: supportsSchemas, supportsDatabases: supportsDatabases ) - self.slot = EngineNamespaceSlot(supportsSchemas: supportsSchemas, supportsDatabases: supportsDatabases) } - // A database name's case follows `lower_case_table_names`; a schema name is exact. + // Exact: on a case-sensitive MySQL server `Shop` and `shop` are two databases. internal func names(_ schema: String) -> Bool { - guard let name else { return false } - return slot == .database ? schema.lowercased() == name.lowercased() : schema == name + schema == name } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index 9f1bfa8dd4..4d3c18fa0f 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -107,10 +107,6 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { internal var id: String { selection.id } - /// Emptied before its rows go back in. A step that copies no data never is: clearing it deleted - /// every row the target had and wrote nothing in their place. - internal var clearsTarget: Bool { copiesData && !truncateStatements.isEmpty } - /// What the DDL phase runs for this table. The truncate is deliberately absent: it belongs to /// the data phase's transaction. internal var ddl: [SyncStatement] { dropStatements + sequenceStatements + createStatements } @@ -228,11 +224,6 @@ internal struct ObjectCopyPlan: Sendable { /// so the run may write that DDL under the target's write scope. A definition read as the /// session is runs as the session is, the way it did before scopes. internal let isStructureReadPinned: Bool - /// True when the plan empties tables and every foreign key in the target that points at one of - /// them comes from a table the plan empties too. Only then can the clears run with the target's - /// checks off: a key from any other table would be left pointing at rows that are gone. False - /// whenever the target's keys could not be read. - internal let clearsAreSelfContained: Bool internal init( request: ObjectCopyRequest, @@ -241,8 +232,7 @@ internal struct ObjectCopyPlan: Sendable { definitionSteps: [ObjectCopyDefinitionStep], schemaStatements: [SyncStatement] = [], skipped: [ObjectCopySkip] = [], - isStructureReadPinned: Bool = false, - clearsAreSelfContained: Bool = false + isStructureReadPinned: Bool = false ) { self.request = request self.createsDatabase = createsDatabase @@ -251,7 +241,6 @@ internal struct ObjectCopyPlan: Sendable { self.schemaStatements = schemaStatements self.skipped = skipped self.isStructureReadPinned = isStructureReadPinned - self.clearsAreSelfContained = clearsAreSelfContained } /// Shown above the script, for the caveats the copy cannot do anything about: a key column the @@ -345,9 +334,13 @@ internal struct ObjectCopyPlan: Sendable { } /// Emptying the tables a data-only replace appends to, children first so a foreign key holds. + /// + /// Only a table whose rows are going back in. A step that copies no data is not in `dataSteps`, + /// so clearing it deletes every row the target had and writes nothing in their place: the + /// review said only that the two sides shared no writable column, and the run reported success. internal var clearGroups: [ObjectCopyStatementGroup] { tableSteps.reversed() - .filter(\.clearsTarget) + .filter { $0.copiesData && !$0.truncateStatements.isEmpty } .map { ObjectCopyStatementGroup($0.selection, $0.truncateStatements) } } @@ -361,7 +354,7 @@ internal struct ObjectCopyPlan: Sendable { internal var clearsInsideDataTransaction: Bool { !clearGroups.isEmpty && request.wrapEachTableInTransaction - && request.errorHandling != .skipAndContinue + && request.errorHandling == .stopAndRollback } /// The definitions held back until the rows are in: a trigger fires on the copy itself, and a diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift index 5ca1121ade..7e7b11a27c 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner+SourceDefinitions.swift @@ -34,7 +34,7 @@ internal struct ObjectCopyCreationScopes: Sendable { } internal extension ObjectCopyPlanner { - nonisolated private static let logger = Logger(subsystem: "com.TablePro", category: "ObjectCopyPlanner") + nonisolated private static let creationLogger = Logger(subsystem: "com.TablePro", category: "ObjectCopyPlanner") nonisolated static func definitionBuild( for input: ObjectCopyDefinitionInput, @@ -170,76 +170,6 @@ internal extension ObjectCopyPlanner { } } - /// Whether emptying `tables` breaks only keys between tables the copy empties too. Each key the - /// whole-scope read finds is matched the way `ForeignKeyTopologicalSort` keys its nodes, and a - /// read that is unavailable or fails answers false, which keeps the target's checks on. - nonisolated static func clearsAreSelfContained( - clearing tables: [String], - schema: String?, - namespace: String?, - slot: EngineNamespaceSlot, - using plugin: any PluginDatabaseDriver - ) async throws -> Bool { - guard !tables.isEmpty, plugin.providesBulkForeignKeyFetch else { return false } - let foreignKeys: [String: [PluginForeignKeyInfo]] - do { - foreignKeys = try await plugin.fetchAllForeignKeys(schema: schema) - } catch { - try rethrowCancellation(error) - logger.warning("Target foreign keys unreadable: \(error.publicLogShape, privacy: .public)") - return false - } - /// PostgreSQL reads a nil schema as the session's own, and its keys name that schema. - let namespace = namespace ?? plugin.currentSchema - let cleared = Set(tables.map { ForeignKeyTopologicalSort.Table(name: $0, schema: namespace).identifier }) - return foreignKeys.allSatisfy { table, keys in - cleared.contains(ForeignKeyTopologicalSort.Table(name: table, schema: namespace).identifier) - || !keys.contains { key in - let parent = ForeignKeyTopologicalSort.Table( - name: key.referencedTable, - schema: referencedSchema(of: key, readIn: namespace, slot: slot) ?? namespace - ) - return cleared.contains(parent.identifier) - } - } - } - - /// The key with its referenced schema spelled the way `namespace` spells it, where the two name - /// one container. - nonisolated static func pointedInto( - _ key: PluginForeignKeyInfo, - namespace: String?, - slot: EngineNamespaceSlot - ) -> PluginForeignKeyInfo { - let schema = referencedSchema(of: key, readIn: namespace, slot: slot) - guard schema != key.referencedSchema else { return key } - return PluginForeignKeyInfo( - name: key.name, - column: key.column, - referencedTable: key.referencedTable, - referencedColumn: key.referencedColumn, - referencedDatabase: key.referencedDatabase, - referencedSchema: schema, - onDelete: key.onDelete, - onUpdate: key.onUpdate - ) - } - - /// A MySQL key names its database as the server spells it, and a server with - /// `lower_case_table_names` 1 or 2 takes `Regest` and `regest` as one database, so on the - /// database slot a spelling that differs only by case is the namespace itself. PostgreSQL - /// schemas `App` and `app` are two schemas, so the schema slot never folds. - nonisolated static func referencedSchema( - of key: PluginForeignKeyInfo, - readIn namespace: String?, - slot: EngineNamespaceSlot - ) -> String? { - guard slot == .database, let referenced = key.referencedSchema, let namespace, - ObjectCopyNamespace.isSame(referenced, namespace) - else { return key.referencedSchema } - return namespace - } - nonisolated static func creationScopes( for selections: [ObjectCopySelection], triggerTables: [String], @@ -330,7 +260,7 @@ internal extension ObjectCopyPlanner { result.scopes[selection.id] = try await fetch() } catch { try rethrowCancellation(error) - logger.warning( + creationLogger.warning( "Creation settings read failed for \(selection.kind.rawValue, privacy: .public): \(error.publicLogShape, privacy: .public)" ) result.unreadable.insert(selection.id) diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index bbeb12ca67..f068f45069 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -107,9 +107,7 @@ internal struct ObjectCopyPlanner { skipped: tableSkips )) } - let parentsFirst = Self.orderAcrossScopes( - &passes, slot: EngineNamespaceSlot(databaseType: request.source.databaseType) - ) + let parentsFirst = Self.orderAcrossScopes(&passes) /// Every source scope of a copy to a chosen target lands in the one schema that was chosen, /// so the names its indexes take are allocated across scopes, not afresh for each. @@ -157,8 +155,7 @@ internal struct ObjectCopyPlanner { definitionSteps: definitionSteps, schemaStatements: try await buildSchemaStatements(request, namespaces: targetNamespaces), skipped: skipped, - isStructureReadPinned: isStructureReadPinned, - clearsAreSelfContained: try await clearsAreSelfContained(request, tableSteps: tableSteps) + isStructureReadPinned: isStructureReadPinned ) } @@ -168,8 +165,7 @@ internal struct ObjectCopyPlanner { /// created `orders` first, because `app` was the first scope. The drafts are reordered before /// any step is built, because a sequence several tables share is created under the first. private static func orderAcrossScopes( - _ passes: inout [ScopePass], - slot: EngineNamespaceSlot + _ passes: inout [ScopePass] ) -> (ObjectCopySelection, ObjectCopySelection) -> Bool { var reads: [ObjectCopySelection: TableStructureRead] = [:] for draft in passes.flatMap(\.drafts) { @@ -177,8 +173,7 @@ internal struct ObjectCopyPlanner { } let ordered = orderedByDependency( passes.map { (namespace: $0.sourceNamespace, tables: $0.drafts.map(\.selection)) }, - reads: reads, - slot: slot + reads: reads ) let rank = Dictionary( ordered.enumerated().map { ($0.element, $0.offset) }, uniquingKeysWith: { first, _ in first } @@ -192,26 +187,6 @@ internal struct ObjectCopyPlanner { return parentsFirst } - /// Asked of the target only when the plan empties a table, and in one whole-scope read. - private func clearsAreSelfContained( - _ request: ObjectCopyRequest, - tableSteps: [ObjectCopyTableStep] - ) async throws -> Bool { - let cleared = tableSteps.filter(\.clearsTarget).map(\.targetTable) - guard !cleared.isEmpty else { return false } - /// Every clear is in `request.target`: a duplicate creates its tables, so it empties none. - let target = request.target - let schema = target.schema?.nilIfEmpty - let namespace = ObjectCopyNamespace.name(for: target) - let slot = EngineNamespaceSlot(databaseType: target.databaseType) - return try await manager.withMetadataDriver(scope: targetScope(request, endpoint: target)) { driver in - guard let plugin = CompareMetadataService.pluginDriver(from: driver) else { return false } - return try await Self.clearsAreSelfContained( - clearing: cleared, schema: schema, namespace: namespace, slot: slot, using: plugin - ) - } - } - private func sourceTableReads( _ request: ObjectCopyRequest, endpoint: DatabaseEndpoint, @@ -1110,8 +1085,7 @@ internal struct ObjectCopyPlanner { /// created before its parent and the server rejected it. nonisolated internal static func orderedByDependency( _ scopes: [(namespace: String?, tables: [ObjectCopySelection])], - reads: [ObjectCopySelection: TableStructureRead], - slot: EngineNamespaceSlot + reads: [ObjectCopySelection: TableStructureRead] ) -> [ObjectCopySelection] { let selections = scopes.flatMap { $0.tables } guard selections.count > 1 else { return selections } @@ -1124,9 +1098,7 @@ internal struct ObjectCopyPlanner { let schema = read.table.schema ?? selection.schema ?? scope.namespace let node = ForeignKeyTopologicalSort.Table(name: read.table.name, schema: schema) nodes.append(node) - foreignKeysByTable[node.identifier] = read.foreignKeys.map { - pointedInto($0, namespace: node.schema, slot: slot) - } + foreignKeysByTable[node.identifier] = read.foreignKeys bySortKey[node.identifier] = selection } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index aebfa8eaa9..8b602baa73 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -253,6 +253,8 @@ internal struct ObjectCopyRunner { let scope = request.target.scope let hasCleanup = !plan.cleanupGroups.isEmpty let schemaStatements = plan.schemaStatements + // A MySQL CREATE TABLE refuses a key to a table not created yet, which a cycle always has. + let writesTables = tableGroups.contains { !$0.statements.isEmpty } let pinsSession = plan.isStructureReadPinned && VerbatimTransferScopes.isSameEngine( source: request.source.databaseType, target: request.target.databaseType ) @@ -267,10 +269,10 @@ internal struct ObjectCopyRunner { && plugin.supportsTransactionalDDL && owner.opensTransaction if usesTransaction { try await plugin.beginTransaction(mode: .readWrite) } - var result: DDLResult = try await Self.withForeignKeysRelaxed( - on: plugin, when: hasCleanup && !usesTransaction && owner != .session + var result = try await Self.withForeignKeysRelaxed( + on: plugin, when: writesTables && !usesTransaction && owner != .session ) { - var ddl = try await Self.executeTableDDL( + try await Self.executeTableDDL( schemaStatements: schemaStatements, groups: tableGroups, on: plugin, @@ -278,17 +280,16 @@ internal struct ObjectCopyRunner { errorHandling: errorHandling, progress: progress ) - /// Outside the write scope, because a routine or trigger records the session's - /// `sql_mode`: it gets its own creation settings, or else the target's. - if !ddl.stopped { - let definitions = await Self.execute( - definitionGroups, on: plugin, errorHandling: errorHandling, progress: progress - ) - ddl.outcomes += definitions.outcomes - ddl.cancelled = definitions.cancelled - ddl.stopped = definitions.stopped - } - return ddl + } + /// Outside the write scope, because a routine or trigger records the session's + /// `sql_mode`: it gets its own creation settings, or else the target's. + if !result.stopped { + let definitions = await Self.execute( + definitionGroups, on: plugin, errorHandling: errorHandling, progress: progress + ) + result.outcomes += definitions.outcomes + result.cancelled = definitions.cancelled + result.stopped = definitions.stopped } result.joinedSessionTransaction = owner == .session @@ -521,7 +522,6 @@ internal struct ObjectCopyRunner { ) async throws -> DataResult { let manager = self.manager let clearGroups = plan.clearGroups - let clearsAreSelfContained = plan.clearsAreSelfContained let steps = plan.dataSteps let sourceType = request.source.databaseType let targetType = request.target.databaseType @@ -543,21 +543,17 @@ internal struct ObjectCopyRunner { var result = DataResult() result.joinedSessionTransaction = owner == .session - func clear() async { - let cleared = await Self.execute( - clearGroups, on: targetPlugin, errorHandling: errorHandling, progress: progress - ) - result.outcomes += cleared.outcomes.filter { $0.error != nil } - result.cancelled = cleared.cancelled - result.stopped = cleared.stopped - } + // Checks stay on for the clears: a table outside the copy that references a cleared one + // keeps its own ON DELETE rule, and MySQL cannot prove no other database holds one. + let cleared = await Self.execute( + clearGroups, on: targetPlugin, errorHandling: errorHandling, progress: progress + ) + result.outcomes += cleared.outcomes.filter { $0.error != nil } + result.cancelled = cleared.cancelled + result.stopped = cleared.stopped - // A table outside the copy that references a cleared one keeps its own ON DELETE rule. - if !clearsAreSelfContained { await clear() } if !result.stopped { try await Self.withForeignKeysRelaxed(on: targetPlugin, when: owner != .session) { - if clearsAreSelfContained { await clear() } - guard !result.stopped else { return } let copied = await Self.stream( steps, from: sourceScope, @@ -779,8 +775,9 @@ internal struct ObjectCopyRunner { try await targetPlugin.beginTransaction(mode: .readWrite) } do { + // Past a skipped failure a child's parent rows can be missing, and only the checks say so. let outcome = try await Self.withForeignKeysRelaxed( - on: targetPlugin, when: owner != .session + on: targetPlugin, when: owner != .session && errorHandling != .skipAndContinue ) { try await copier.copy(from: sourcePlugin, to: targetPlugin) { rows in progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) diff --git a/apple/TableProMobile/TableProMobile/Localizable.xcstrings b/apple/TableProMobile/TableProMobile/Localizable.xcstrings index 5e685cb14e..d20e9d9398 100644 --- a/apple/TableProMobile/TableProMobile/Localizable.xcstrings +++ b/apple/TableProMobile/TableProMobile/Localizable.xcstrings @@ -10363,6 +10363,10 @@ }, "Folder Deleted" : { + }, + "Foreign key enforcement could not be put back while a transaction is open." : { + "extractionState" : "manual", + "generatesSymbol" : false }, "Foreign Keys" : { "localizations" : { diff --git a/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift b/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift index 33fb6109ba..08291e640a 100644 --- a/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift +++ b/apple/TableProTests/Core/Compare/SchemaSyncScriptBuilderTests.swift @@ -389,12 +389,12 @@ struct CompareForeignKeyOrderTests { ) } - @Test(arguments: ["regest", "Regest"]) - func mysqlCreatesAParentBeforeItsChild(database: String) { + @Test + func mysqlCreatesAParentBeforeItsChild() { let estates = table("estates") let buildings = table("buildings", referencing: "estates", in: "regest") let foreignKeys = CompareRunner.structureForeignKeys( - source: keyed(buildings, estates), in: mysql(database), target: [:], in: mysql("regest_copy") + source: keyed(buildings, estates), in: mysql("regest"), target: [:], in: mysql("regest_copy") ) let ordered = SchemaSyncScriptBuilder.order( @@ -405,6 +405,13 @@ struct CompareForeignKeyOrderTests { #expect(names == ["estates", "buildings"]) } + /// A case-sensitive server holds `Shop` and `shop` side by side, so a key into one is not a key into the other. + @Test + func mysqlDatabasesThatDifferOnlyInCaseStayApart() { + #expect(mysql("Shop").names("Shop")) + #expect(!mysql("Shop").names("shop")) + } + @Test func mysqlDropsAChildBeforeItsParentFromTheTargetsKeys() { let estates = table("estates") diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift index 535157f743..7f091e100b 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift @@ -200,7 +200,9 @@ struct ObjectCopyDDLSessionTests { let result = try await run(plan(tables: [tableStep()]), on: driver) #expect(result.failedCount == 0) - #expect(driver.events == ["open verbatimWrite", createTable, "close verbatimWrite"]) + #expect(driver.events == [ + "open relaxedForeignKeys", "open verbatimWrite", createTable, "close verbatimWrite", "close relaxedForeignKeys" + ]) } @Test("A failed CREATE TABLE still closes the write scope") @@ -210,7 +212,9 @@ struct ObjectCopyDDLSessionTests { let result = try await run(plan(tables: [tableStep()]), on: driver) #expect(result.failedCount == 1) - #expect(driver.events == ["open verbatimWrite", createTable, "close verbatimWrite"]) + #expect(driver.events == [ + "open relaxedForeignKeys", "open verbatimWrite", createTable, "close verbatimWrite", "close relaxedForeignKeys" + ]) } /// MariaDB renders a TIMESTAMP default in the session's zone, so DDL read as the session is @@ -221,7 +225,7 @@ struct ObjectCopyDDLSessionTests { _ = try await run(plan(tables: [tableStep()], isStructureReadPinned: false), on: driver) - #expect(driver.events == [createTable]) + #expect(driver.events == ["open relaxedForeignKeys", createTable, "close relaxedForeignKeys"]) } @Test("A copy between engines creates its tables under the session as it is") @@ -230,7 +234,7 @@ struct ObjectCopyDDLSessionTests { _ = try await run(plan(from: .postgresql, tables: [tableStep()]), on: driver) - #expect(driver.events == [createTable]) + #expect(driver.events == ["open relaxedForeignKeys", createTable, "close relaxedForeignKeys"]) } // MARK: - Reading the source tables @@ -289,7 +293,7 @@ struct ObjectCopyDDLSessionTests { #expect(result.failedCount == 0) #expect(driver.events == [ - "open verbatimWrite", createTable, "close verbatimWrite", + "open relaxedForeignKeys", "open verbatimWrite", createTable, "close verbatimWrite", "close relaxedForeignKeys", saveMode, setMode, createProcedure, restoreMode ]) } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift index fee0129dc0..aedfff83c8 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlanTests.swift @@ -19,35 +19,6 @@ private final class TruncatingEngineDriver: PluginDriverStub, @unchecked Sendabl } } -private final class TargetForeignKeyDriver: PluginDriverStub, @unchecked Sendable { - private let lock = NSLock() - private var count = 0 - private let bulk: Bool - private let answer: Result<[String: [PluginForeignKeyInfo]], any Error> - private let sessionSchema: String? - - init( - bulk: Bool = true, - sessionSchema: String? = nil, - _ answer: Result<[String: [PluginForeignKeyInfo]], any Error> - ) { - self.bulk = bulk - self.sessionSchema = sessionSchema - self.answer = answer - } - - var reads: Int { lock.withLock { count } } - - var currentSchema: String? { sessionSchema } - - var providesBulkForeignKeyFetch: Bool { bulk } - - func fetchAllForeignKeys(schema: String?) async throws -> [String: [PluginForeignKeyInfo]] { - lock.withLock { count += 1 } - return try answer.get() - } -} - struct ObjectCopyPlanTests { private func endpoint( _ database: String, @@ -157,6 +128,11 @@ struct ObjectCopyPlanTests { [step("orders")], request: request(errorHandling: .skipAndContinue) ).clearsInsideDataTransaction)) + /// PostgreSQL commits a transaction a failure aborted as a ROLLBACK, which keeps nothing. + #expect(!(plan( + [step("orders")], + request: request(errorHandling: .stopAndCommit) + ).clearsInsideDataTransaction)) #expect(!(plan( [step("orders")], request: request(wrapEachTableInTransaction: false) @@ -169,122 +145,6 @@ struct ObjectCopyPlanTests { #expect(!(plan([step("orders", truncates: false)]).clearsInsideDataTransaction)) } - // MARK: - Clears with checks off - - private func key(to table: String, in schema: String? = "shop") -> PluginForeignKeyInfo { - PluginForeignKeyInfo( - name: "fk_\(table)", - column: "\(table)_id", - referencedTable: table, - referencedColumn: "id", - referencedDatabase: nil, - referencedSchema: schema - ) - } - - private func selfContained( - _ cleared: [String], - namespace: String? = "shop", - slot: EngineNamespaceSlot = .database, - on driver: TargetForeignKeyDriver - ) async throws -> Bool { - try await ObjectCopyPlanner.clearsAreSelfContained( - clearing: cleared, schema: nil, namespace: namespace, slot: slot, using: driver - ) - } - - /// A key between two cleared tables is refilled by the copy, and one out of a cleared table - /// points at a parent nothing empties. - @Test - func clearsThatOnlyReferenceEachOtherAreSelfContained() async throws { - let driver = TargetForeignKeyDriver(.success([ - "buildings": [key(to: "estates")], - "estates": [key(to: "users")], - "employees": [key(to: "employees")] - ])) - - let contained = try await selfContained(["estates", "buildings"], on: driver) - - #expect(contained) - #expect(driver.reads == 1) - } - - /// Measured on MySQL 8.4: `DELETE FROM estates` with checks off left an unselected `contracts` - /// pointing at rows that were gone. - @Test - func aTableTheCopyLeavesAloneThatReferencesAClearedOneIsNotSelfContained() async throws { - let driver = TargetForeignKeyDriver(.success([ - "buildings": [key(to: "estates")], - "contracts": [key(to: "estates")] - ])) - - let contained = try await selfContained(["estates", "buildings"], on: driver) - - #expect(!contained) - } - - /// The key names the database as the server spells it, which a `lower_case_table_names` 1 - /// server lets differ from the endpoint's spelling. Missing that match would relax the clears. - @Test - func aKeyIntoTheSameDatabaseSpelledInAnotherCaseStillCounts() async throws { - let driver = TargetForeignKeyDriver(.success(["contracts": [key(to: "estates", in: "shop")]])) - - let contained = try await selfContained(["estates"], namespace: "Shop", on: driver) - - #expect(!contained) - } - - /// PostgreSQL reads a nil schema as the session's, and the keys it returns name that schema. - @Test - func aTargetWithNoSchemaIsMatchedInTheSessionsOwn() async throws { - let driver = TargetForeignKeyDriver( - sessionSchema: "public", .success(["items": [key(to: "orders", in: "public")]]) - ) - - let contained = try await selfContained(["orders"], namespace: nil, slot: .schema, on: driver) - - #expect(!contained) - } - - @Test - func aDriverWithoutTheWholeScopeReadIsNeverSelfContained() async throws { - let driver = TargetForeignKeyDriver(bulk: false, .success([:])) - - let contained = try await selfContained(["estates"], on: driver) - - #expect(!contained) - #expect(driver.reads == 0, "a per-table read of every key in the target is not asked for") - } - - @Test - func aFailedReadIsNeverSelfContained() async throws { - let driver = TargetForeignKeyDriver(.failure(CocoaError(.fileReadNoPermission))) - - let contained = try await selfContained(["estates"], on: driver) - - #expect(!contained) - } - - @Test - func aCancelledReadCancelsThePlan() async { - let driver = TargetForeignKeyDriver(.failure(CancellationError())) - - await #expect(throws: CancellationError.self) { - _ = try await self.selfContained(["estates"], on: driver) - } - } - - @Test - func aPlanWithNothingToEmptyIsNotSelfContained() async throws { - let driver = TargetForeignKeyDriver(.success([:])) - - let contained = try await selfContained([], on: driver) - - #expect(!contained) - #expect(driver.reads == 0) - #expect(!plan([step("orders")]).clearsAreSelfContained, "the plan's own default") - } - /// The script is what the user approves, so it has to be in the order the run uses. Shown /// against each table instead, it said the parent was emptied after the child had been filled. @Test diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift index efacdbea60..b845fb5af4 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift @@ -53,7 +53,7 @@ struct ObjectCopyPlannerOrderingTests { reads: [ObjectCopySelection: TableStructureRead] ) -> [ObjectCopySelection] { ObjectCopyPlanner.orderedByDependency( - [(namespace: "public", tables: selections)], reads: reads, slot: .schema + [(namespace: "public", tables: selections)], reads: reads ) } @@ -102,29 +102,28 @@ struct ObjectCopyPlannerOrderingTests { let ordered = ObjectCopyPlanner.orderedByDependency( [(namespace: "app", tables: [orders]), (namespace: "core", tables: [users])], - reads: reads, - slot: .schema + reads: reads ) #expect(ordered.map(\.qualifiedName) == ["core.users", "app.orders"]) } - /// A MySQL key names its database the way the server spells it. On a server with - /// `lower_case_table_names` 1 the endpoint can say `Regest` while the key says `regest`. + /// A case-sensitive MySQL server holds `Shop` and `shop` side by side, so a key into `shop` is + /// no edge in `Shop`. Without one the tie-break puts `buildings` first. @Test - func aDatabaseSpelledInAnotherCaseIsTheSameDatabase() { + func aDatabaseSpelledInAnotherCaseIsAnotherDatabase() { let buildings = selection("buildings", schema: nil) let estates = selection("estates", schema: nil) let reads: [ObjectCopySelection: TableStructureRead] = [ - buildings: read("buildings", referencing: ["estates"], referencedSchema: "regest"), + buildings: read("buildings", referencing: ["estates"], referencedSchema: "shop"), estates: read("estates") ] let ordered = ObjectCopyPlanner.orderedByDependency( - [(namespace: "Regest", tables: [buildings, estates])], reads: reads, slot: .database + [(namespace: "Shop", tables: [buildings, estates])], reads: reads ) - #expect(ordered.map(\.name) == ["estates", "buildings"]) + #expect(ordered.map(\.name) == ["buildings", "estates"]) } /// PostgreSQL schemas `App` and `app` are two schemas, so a key into `App` is no edge in `app`. @@ -139,7 +138,7 @@ struct ObjectCopyPlannerOrderingTests { ] let ordered = ObjectCopyPlanner.orderedByDependency( - [(namespace: "app", tables: [users, orders])], reads: reads, slot: .schema + [(namespace: "app", tables: [users, orders])], reads: reads ) #expect(ordered.map(\.name) == ["orders", "users"]) diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift index c89b4dd285..0c0ac4a3c2 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift @@ -356,7 +356,10 @@ struct ObjectCopySessionScopeTests { #expect(result.rowsCopied == 2) #expect(source.events == ["open verbatimRead", "stream", "close verbatimRead"]) - #expect(target.events == ["begin", "open verbatimWrite", "insert", "close verbatimWrite", "commit"]) + #expect(target.events == [ + "begin", "open relaxedForeignKeys", "open verbatimWrite", "insert", "close verbatimWrite", + "close relaxedForeignKeys", "commit" + ]) } @MainActor @Test("The runner opens no scope for a copy between engines") @@ -368,6 +371,6 @@ struct ObjectCopySessionScopeTests { #expect(result.rowsCopied == 2) #expect(source.events == ["stream"]) - #expect(target.events == ["begin", "insert", "commit"]) + #expect(target.events == ["begin", "open relaxedForeignKeys", "insert", "close relaxedForeignKeys", "commit"]) } } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index a31311cea4..892440c98f 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -176,15 +176,13 @@ struct ObjectCopyTransactionTests { private func dataPlan( _ steps: [ObjectCopyTableStep], - errorHandling: ImportErrorHandling = .stopAndRollback, - clearsAreSelfContained: Bool = false + errorHandling: ImportErrorHandling = .stopAndRollback ) -> ObjectCopyPlan { ObjectCopyPlan( request: request(steps.map(\.selection), content: .data, errorHandling: errorHandling), createsDatabase: false, tableSteps: steps, - definitionSteps: [], - clearsAreSelfContained: clearsAreSelfContained + definitionSteps: [] ) } @@ -323,9 +321,9 @@ struct ObjectCopyTransactionTests { ]) } - /// Relaxed, the DELETE would leave rows in a table outside the copy pointing at nothing. - @Test("A clear that a table outside the copy references runs before the keys are relaxed") - func referencedClearRunsWithKeysOn() async throws { + /// Relaxed, a DELETE would leave rows in a table outside the copy pointing at nothing. + @Test("Clears run with the target's keys on, before the rows relax them") + func clearsRunWithKeysOn() async throws { let log = CopyEventLog() _ = try await run( @@ -345,28 +343,6 @@ struct ObjectCopyTransactionTests { ]) } - /// A self reference or a cycle refuses its own DELETE on MySQL with the checks on. - @Test("Clears that only the copy's own tables reference run with the keys relaxed") - func selfContainedClearRunsRelaxed() async throws { - let log = CopyEventLog() - - _ = try await run( - dataPlan([dataStep("buildings", clears: true)], clearsAreSelfContained: true), - source: RecordingCopyDriver("source", log: log), - target: RecordingCopyDriver("target", log: log) - ) - - #expect(log.events == [ - "target begin", - "target open relaxedForeignKeys", - "target DELETE FROM buildings", - "source SELECT id FROM buildings", - "target insert", - "target close relaxedForeignKeys", - "target commit", - ]) - } - @Test("A copy that throws puts the target's keys back before rolling back") func failedCopyRestoresKeys() async throws { let log = CopyEventLog() @@ -422,8 +398,9 @@ struct ObjectCopyTransactionTests { #expect(log.events == ["source SELECT id FROM buildings", "target insert"]) } - /// The table still holds its own rows, so the source's would be appended beside them. - @Test("Under Skip and continue a table whose clear failed gets no rows") + /// The table still holds its own rows, so the source's would be appended beside them. Past that + /// failure the rows keep the target's keys, since a child's parent rows can be missing. + @Test("Under Skip and continue a table whose clear failed gets no rows, and none are relaxed") func failedClearGetsNoRows() async throws { let log = CopyEventLog() let target = RecordingCopyDriver("target", log: log, failing: ["DELETE FROM buildings"]) @@ -442,10 +419,8 @@ struct ObjectCopyTransactionTests { #expect(log.events == [ "target DELETE FROM buildings", "target DELETE FROM estates", - "target open relaxedForeignKeys", "source SELECT id FROM estates", "target insert", - "target close relaxedForeignKeys", ]) } } From 0282e5bd31da36ee00ecef82c25d2e00c2c99f08 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 22:48:10 +0700 Subject: [PATCH 05/13] fix(copy): restore foreign keys when a SQLite relax fails and roll back a failed structure phase --- .../SQLiteSessionScopeTests.swift | 62 +++++++++++++++++++ .../SQLiteSessionScope.swift | 13 +++- .../Core/ObjectCopy/ObjectCopyRunner.swift | 36 +++++++---- .../ObjectCopyTransactionTests.swift | 24 +++++++ 4 files changed, 122 insertions(+), 13 deletions(-) create mode 100644 apple/Packages/TableProCore/Tests/TableProPluginKitTests/SQLiteSessionScopeTests.swift diff --git a/apple/Packages/TableProCore/Tests/TableProPluginKitTests/SQLiteSessionScopeTests.swift b/apple/Packages/TableProCore/Tests/TableProPluginKitTests/SQLiteSessionScopeTests.swift new file mode 100644 index 0000000000..c404506a45 --- /dev/null +++ b/apple/Packages/TableProCore/Tests/TableProPluginKitTests/SQLiteSessionScopeTests.swift @@ -0,0 +1,62 @@ +import Foundation +import TableProPluginKit +import Testing + +private struct ReadFailure: Error {} + +// The connection's foreign_keys as each statement leaves it, and the read-back that follows the assignment. +private final class ScriptedConnection: @unchecked Sendable { + private let lock = NSLock() + private var value = "1" + private var reads = 0 + private let secondRead: Result + private var log: [String] = [] + + init(secondRead: Result) { + self.secondRead = secondRead + } + + var foreignKeys: String { lock.withLock { value } } + var statements: [String] { lock.withLock { log } } + + func execute(_ query: String) throws -> PluginQueryResult { + try lock.withLock { + log.append(query) + switch query { + case "PRAGMA foreign_keys = ON": value = "1" + case "PRAGMA foreign_keys = OFF": value = "0" + default: + reads += 1 + let answer = reads == 2 ? try secondRead.get() : value + return PluginQueryResult( + columns: ["foreign_keys"], columnTypeNames: [], rows: [[.text(answer)]], rowsAffected: 0, executionTime: 0 + ) + } + return PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 0, executionTime: 0) + } + } +} + +struct SQLiteSessionScopeTests { + @Test("A relax whose read-back fails turns foreign keys back on before it throws") + func failedReadBackRestores() async { + let connection = ScriptedConnection(secondRead: .failure(ReadFailure())) + + await #expect(throws: ReadFailure.self) { + _ = try await SQLiteSessionScope.open(for: .relaxedForeignKeys) { try connection.execute($0) } + } + + #expect(connection.foreignKeys == "1") + #expect(connection.statements.last == "PRAGMA foreign_keys = ON") + } + + @Test("A relax whose read-back is unreadable turns foreign keys back on and opens no scope") + func unreadableReadBackRestores() async throws { + let connection = ScriptedConnection(secondRead: .success("unknown")) + + let scope = try await SQLiteSessionScope.open(for: .relaxedForeignKeys) { try connection.execute($0) } + + #expect(scope == nil) + #expect(connection.foreignKeys == "1") + } +} diff --git a/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift b/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift index a7f0ac244c..903279fb45 100644 --- a/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift +++ b/apple/Plugins/TableProPluginKit/SQLiteSessionScope.swift @@ -23,8 +23,19 @@ public enum SQLiteSessionScope { case .relaxedForeignKeys: guard let saved = try await readForeignKeys(execute), saved.value == on else { return nil } _ = try await execute(assignment(off)) + // No scope reaches the caller past here, so nothing else would turn them back on. + let readBack: PluginSessionSetting? + do { + readBack = try await readForeignKeys(execute) + } catch { + _ = try? await execute(assignment(on)) + throw error + } // The pragma is a no-op while a transaction is open, and says so only when read back. - guard try await readForeignKeys(execute)?.value == off else { return nil } + guard readBack?.value == off else { + _ = try? await execute(assignment(on)) + return nil + } let applied = PluginSessionSetting(name: foreignKeys, value: off) return PluginSessionScope(purpose: purpose, saved: [saved], applied: [applied]) case .verbatimRead, .verbatimWrite: diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index 8b602baa73..14c2df8031 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -269,17 +269,24 @@ internal struct ObjectCopyRunner { && plugin.supportsTransactionalDDL && owner.opensTransaction if usesTransaction { try await plugin.beginTransaction(mode: .readWrite) } - var result = try await Self.withForeignKeysRelaxed( - on: plugin, when: writesTables && !usesTransaction && owner != .session - ) { - try await Self.executeTableDDL( - schemaStatements: schemaStatements, - groups: tableGroups, - on: plugin, - pinsSession: pinsSession, - errorHandling: errorHandling, - progress: progress - ) + var result: DDLResult + do { + result = try await Self.withForeignKeysRelaxed( + on: plugin, when: writesTables && !usesTransaction && owner != .session + ) { + try await Self.executeTableDDL( + schemaStatements: schemaStatements, + groups: tableGroups, + on: plugin, + pinsSession: pinsSession, + errorHandling: errorHandling, + progress: progress + ) + } + } catch { + // The pool never ends a transaction, so the next lease would inherit an aborted one. + if usesTransaction { try? await plugin.rollbackTransaction() } + throw error } /// Outside the write scope, because a routine or trigger records the session's /// `sql_mode`: it gets its own creation settings, or else the target's. @@ -295,7 +302,12 @@ internal struct ObjectCopyRunner { guard usesTransaction else { return result } guard result.stopped else { - try await plugin.commitTransaction() + do { + try await plugin.commitTransaction() + } catch { + try? await plugin.rollbackTransaction() + throw error + } return result } try? await plugin.rollbackTransaction() diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index 892440c98f..1b79a9c67f 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -298,6 +298,30 @@ struct ObjectCopyTransactionTests { ]) } + @Test("A schema that cannot be created rolls the structure transaction back before the lease returns") + func failedSchemaRollsBack() async throws { + let log = CopyEventLog() + let createSchema = "CREATE SCHEMA staging" + let base = replacementPlan() + let plan = ObjectCopyPlan( + request: base.request, + createsDatabase: false, + tableSteps: base.tableSteps, + definitionSteps: [], + schemaStatements: [SyncStatement(sql: createSchema, objectName: "staging", summary: "schema")] + ) + + await #expect(throws: (any Error).self) { + _ = try await run( + plan, + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, failing: [createSchema]) + ) + } + + #expect(log.events == ["target begin", "target \(createSchema)", "target rollback"]) + } + // MARK: - Rows @Test("Rows are written with the target's keys relaxed inside its transaction, and the source left alone") From 5542ce2f96a9e00b7cc18d422ea1b457ef8bab7c Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 23:12:38 +0700 Subject: [PATCH 06/13] fix(copy): relax keys only for a whole copy in one transaction, and keep each batch under the other settings --- .../Core/ObjectCopy/ObjectCopyPlan.swift | 17 ++- .../Core/ObjectCopy/ObjectCopyPlanner.swift | 57 ++++++++- .../Core/ObjectCopy/ObjectCopyRowCopier.swift | 55 ++++---- .../Core/ObjectCopy/ObjectCopyRunner.swift | 101 +++++---------- .../ObjectCopyDDLSessionTests.swift | 3 +- .../ObjectCopyPlannerOrderingTests.swift | 68 ++++++++++ .../ObjectCopySessionScopeTests.swift | 3 +- .../ObjectCopyTransactionTests.swift | 121 +++++++++++++++++- apple/changelog.d/3859.changed.md | 1 + apple/changelog.d/3859.fixed.6.md | 1 + 10 files changed, 320 insertions(+), 107 deletions(-) create mode 100644 apple/changelog.d/3859.changed.md create mode 100644 apple/changelog.d/3859.fixed.6.md diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index 4d3c18fa0f..c29880f856 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -224,6 +224,8 @@ internal struct ObjectCopyPlan: Sendable { /// so the run may write that DDL under the target's write scope. A definition read as the /// session is runs as the session is, the way it did before scopes. internal let isStructureReadPinned: Bool + /// `ObjectCopyPlanner.keysStayInside`: whether the target's foreign key checks may be off. + internal let keysStayInside: Bool internal init( request: ObjectCopyRequest, @@ -232,7 +234,8 @@ internal struct ObjectCopyPlan: Sendable { definitionSteps: [ObjectCopyDefinitionStep], schemaStatements: [SyncStatement] = [], skipped: [ObjectCopySkip] = [], - isStructureReadPinned: Bool = false + isStructureReadPinned: Bool = false, + keysStayInside: Bool = false ) { self.request = request self.createsDatabase = createsDatabase @@ -241,6 +244,7 @@ internal struct ObjectCopyPlan: Sendable { self.schemaStatements = schemaStatements self.skipped = skipped self.isStructureReadPinned = isStructureReadPinned + self.keysStayInside = keysStayInside } /// Shown above the script, for the caveats the copy cannot do anything about: a key column the @@ -352,9 +356,14 @@ internal struct ObjectCopyPlan: Sendable { /// A run that promises nothing keeps them in a phase of their own, where one table's failure /// does not reach the tables already copied. internal var clearsInsideDataTransaction: Bool { - !clearGroups.isEmpty - && request.wrapEachTableInTransaction - && request.errorHandling == .stopAndRollback + !clearGroups.isEmpty && dataRunsInOneTransaction + } + + /// Stop and roll back promises the target ends as it was, so the clears and every table's rows + /// are one transaction. The other two keep what ran, and PostgreSQL commits an aborted + /// transaction as a ROLLBACK, so their batches commit as they go. + internal var dataRunsInOneTransaction: Bool { + request.wrapEachTableInTransaction && request.errorHandling == .stopAndRollback } /// The definitions held back until the rows are in: a trigger fires on the copy itself, and a diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index f068f45069..f3620100d8 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -108,6 +108,18 @@ internal struct ObjectCopyPlanner { )) } let parentsFirst = Self.orderAcrossScopes(&passes) + let drafts = passes.flatMap(\.drafts) + let keysStayInside = Self.keysStayInside( + passes.map { (namespace: $0.sourceNamespace, tables: $0.drafts.map(\.selection)) }, + reads: Dictionary(drafts.map { ($0.selection, $0.read) }, uniquingKeysWith: { first, _ in first }), + rows: Dictionary( + drafts.filter(\.copiesData).map { draft in + let scope = draft.rowScope + return (draft.selection, !VerbatimTransferScopes.filtersRows(scope) && scope?.rowLimit == nil) + }, + uniquingKeysWith: { first, _ in first } + ) + ) /// Every source scope of a copy to a chosen target lands in the one schema that was chosen, /// so the names its indexes take are allocated across scopes, not afresh for each. @@ -155,7 +167,8 @@ internal struct ObjectCopyPlanner { definitionSteps: definitionSteps, schemaStatements: try await buildSchemaStatements(request, namespaces: targetNamespaces), skipped: skipped, - isStructureReadPinned: isStructureReadPinned + isStructureReadPinned: isStructureReadPinned, + keysStayInside: keysStayInside ) } @@ -1095,8 +1108,7 @@ internal struct ObjectCopyPlanner { for scope in scopes { for selection in scope.tables { guard let read = reads[selection] else { continue } - let schema = read.table.schema ?? selection.schema ?? scope.namespace - let node = ForeignKeyTopologicalSort.Table(name: read.table.name, schema: schema) + let node = sortNode(selection, read: read, namespace: scope.namespace) nodes.append(node) foreignKeysByTable[node.identifier] = read.foreignKeys bySortKey[node.identifier] = selection @@ -1121,6 +1133,45 @@ internal struct ObjectCopyPlanner { return result } + /// Whether every key a copied table holds points at a table in the copy, and, for a table that + /// gets rows, at one that gets all of its rows. `rows` holds each table that gets rows, true when + /// it gets every one. + /// + /// Only then may the target's checks be off: turning them back on checks nothing already + /// written, so a child copied without its parent's rows would keep pointing at nothing. + nonisolated internal static func keysStayInside( + _ scopes: [(namespace: String?, tables: [ObjectCopySelection])], + reads: [ObjectCopySelection: TableStructureRead], + rows: [ObjectCopySelection: Bool] + ) -> Bool { + var nodes: [(selection: ObjectCopySelection, read: TableStructureRead, node: ForeignKeyTopologicalSort.Table)] = [] + for scope in scopes { + for selection in scope.tables { + guard let read = reads[selection] else { return false } + nodes.append((selection, read, sortNode(selection, read: read, namespace: scope.namespace))) + } + } + let present = Set(nodes.map(\.node.identifier)) + let filled = Set(nodes.filter { rows[$0.selection] == true }.map(\.node.identifier)) + return nodes.allSatisfy { entry in + let parents = rows[entry.selection] == nil ? present : filled + return entry.read.foreignKeys.allSatisfy { key in + let parent = ForeignKeyTopologicalSort.Table( + name: key.referencedTable, schema: key.referencedSchema ?? entry.node.schema + ) + return parents.contains(parent.identifier) + } + } + } + + nonisolated private static func sortNode( + _ selection: ObjectCopySelection, + read: TableStructureRead, + namespace: String? + ) -> ForeignKeyTopologicalSort.Table { + ForeignKeyTopologicalSort.Table(name: read.table.name, schema: read.table.schema ?? selection.schema ?? namespace) + } + /// A view selects from a table, a routine calls a view, and a trigger hangs off a table, so the /// kinds run in that order. Within a kind the user's own order is kept. nonisolated internal static func orderedByKind(_ selections: [ObjectCopySelection]) -> [ObjectCopySelection] { diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift index 2277b22a2b..329303b753 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift @@ -38,9 +38,15 @@ internal struct ObjectCopyRowCopier: Sendable { internal struct Outcome: Sendable { internal let inserted: Int internal let cancelled: Bool - /// What is in the target whatever happens next. Zero where the caller holds a transaction - /// it can roll back, and every flushed batch where it does not. - internal var committed: Int = 0 + } + + /// A table that failed after some of its batches were written. Outside a transaction those stay + /// in the target, and a count that left them out would send a retry over them. + internal struct PartialFailure: LocalizedError { + internal let written: Int + internal let underlying: any Error + + internal var errorDescription: String? { underlying.localizedDescription } } internal let step: ObjectCopyTableStep @@ -92,29 +98,33 @@ internal struct ObjectCopyRowCopier: Sendable { var inserted = 0 - while let element = try await stream.next() { + do { + while let element = try await stream.next() { + if Task.isCancelled { return Outcome(inserted: inserted, cancelled: true) } + guard case .rows(let rows) = element else { continue } + for row in rows { + let values = try aligned(row) + let rowBytes = budget.byteCount(of: values) + guard let batch = filler.append(values, bytes: rowBytes) else { continue } + inserted += try await write(batch, generator: generator, driver: targetDriver) + onProgress(inserted) + if Task.isCancelled { return Outcome(inserted: inserted, cancelled: true) } + } + } + + /// Checked again here. Cancellation can reach an `AsyncThrowingStream` as an ordinary end + /// of stream, or land after its last element, and the loop then falls out with rows still + /// pending: writing them committed a batch the user had already stopped and reported the + /// table as copied. if Task.isCancelled { return Outcome(inserted: inserted, cancelled: true) } - guard case .rows(let rows) = element else { continue } - for row in rows { - let values = try aligned(row) - let rowBytes = budget.byteCount(of: values) - guard let batch = filler.append(values, bytes: rowBytes) else { continue } + if let batch = filler.take() { inserted += try await write(batch, generator: generator, driver: targetDriver) onProgress(inserted) - if Task.isCancelled { return Outcome(inserted: inserted, cancelled: true) } } + return Outcome(inserted: inserted, cancelled: Task.isCancelled) + } catch where inserted > 0 && !(error is CancellationError) { + throw PartialFailure(written: inserted, underlying: error) } - - /// Checked again here. Cancellation can reach an `AsyncThrowingStream` as an ordinary end - /// of stream, or land after its last element, and the loop then falls out with rows still - /// pending: writing them committed a batch the user had already stopped and reported the - /// table as copied. - if Task.isCancelled { return Outcome(inserted: inserted, cancelled: true) } - if let batch = filler.take() { - inserted += try await write(batch, generator: generator, driver: targetDriver) - onProgress(inserted) - } - return Outcome(inserted: inserted, cancelled: Task.isCancelled) } // MARK: - Server side @@ -139,9 +149,6 @@ internal struct ObjectCopyRowCopier: Sendable { /// worse-looking number; over-reporting one is a false claim about their data. let inserted = result.rowsAffected onProgress(inserted) - /// `committed` is left at zero, as the streamed path leaves it: whether these rows survive - /// is the caller's transaction to decide, and it is the caller that fills it in when there - /// is no transaction to roll back. return Outcome(inserted: inserted, cancelled: false) } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index 14c2df8031..5a10ead31f 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -253,8 +253,11 @@ internal struct ObjectCopyRunner { let scope = request.target.scope let hasCleanup = !plan.cleanupGroups.isEmpty let schemaStatements = plan.schemaStatements - // A MySQL CREATE TABLE refuses a key to a table not created yet, which a cycle always has. - let writesTables = tableGroups.contains { !$0.statements.isEmpty } + // A MySQL DROP refuses a table another references, and a CREATE a key to a table not there + // yet, which a cycle always has. Past a replacement, a key to a table outside the copy keeps + // the checks, so it still fails when that table is missing. + let dropsTables = plan.cleanupGroups.contains { !$0.statements.isEmpty } + let relaxesKeys = tableGroups.contains { !$0.statements.isEmpty } && (dropsTables || plan.keysStayInside) let pinsSession = plan.isStructureReadPinned && VerbatimTransferScopes.isSameEngine( source: request.source.databaseType, target: request.target.databaseType ) @@ -272,7 +275,7 @@ internal struct ObjectCopyRunner { var result: DDLResult do { result = try await Self.withForeignKeysRelaxed( - on: plugin, when: writesTables && !usesTransaction && owner != .session + on: plugin, when: relaxesKeys && !usesTransaction && owner != .session ) { try await Self.executeTableDDL( schemaStatements: schemaStatements, @@ -510,24 +513,24 @@ internal struct ObjectCopyRunner { request: ObjectCopyRequest, progress: ObjectCopyProgress ) async throws -> DataResult { - guard plan.clearsInsideDataTransaction else { + guard plan.dataRunsInOneTransaction else { let steps = plan.dataSteps.filter { !uncleared.contains($0.id) } return await runIndependentTables(steps, request: request, progress: progress) } - return try await runClearedTables(plan, request: request, progress: progress) + return try await runInOneTransaction(plan, request: request, progress: progress) } - /// The clear and every row it makes room for, in one transaction on the target. + /// Every clear and every row, in one transaction on the target. /// /// Emptying a table is reversible only while the transaction that emptied it is still open, so /// the clears cannot run in a phase of their own once the run has promised a rollback. They /// still all run before any table is filled, which is what keeps a cascading foreign key from /// taking rows out of a table the user never selected. /// - /// One transaction for every table is the price of that promise, and it is charged only here: - /// a copy with nothing to empty keeps its per-table transactions, where one table's failure - /// leaves the tables already copied alone. - private func runClearedTables( + /// The rows relax the target's checks only here, where a failure takes every one of them back: + /// committed table by table, a cycle's first table would keep rows pointing at a second that + /// then failed. + private func runInOneTransaction( _ plan: ObjectCopyPlan, request: ObjectCopyRequest, progress: ObjectCopyProgress @@ -539,6 +542,7 @@ internal struct ObjectCopyRunner { let targetType = request.target.databaseType let sourceScope = request.source.scope let errorHandling = request.errorHandling + let keysStayInside = plan.keysStayInside /// A lease this phase cannot take is a run-level failure with no object to pin it on, and /// it is thrown for the same reason the DDL phases throw it: swallowed into a stopped @@ -565,7 +569,7 @@ internal struct ObjectCopyRunner { result.stopped = cleared.stopped if !result.stopped { - try await Self.withForeignKeysRelaxed(on: targetPlugin, when: owner != .session) { + try await Self.withForeignKeysRelaxed(on: targetPlugin, when: usesTransaction && keysStayInside) { let copied = await Self.stream( steps, from: sourceScope, @@ -584,7 +588,7 @@ internal struct ObjectCopyRunner { } guard usesTransaction else { return result } - guard result.stopped, errorHandling != .stopAndCommit else { + guard result.stopped else { /// A commit that fails leaves the transaction open, and the clears are inside it, /// so it is rolled back rather than left for the lease to decide. do { @@ -671,9 +675,10 @@ internal struct ObjectCopyRunner { return result } - /// Each table is its own unit of work, so one that fails leaves the ones already copied alone. - /// Both scopes are open at once, which the planner has already established is safe: two scopes - /// on one connection are refused unless both route to the pool. + /// Each batch commits as it is written, so a failure keeps what ran before it, and the target's + /// checks stay on: past a stop or a skipped failure a child's parent rows can be missing. Both + /// scopes are open at once, which the planner has already established is safe: two scopes on + /// one connection are refused unless both route to the pool. private func runIndependentTables( _ steps: [ObjectCopyTableStep], request: ObjectCopyRequest, @@ -689,32 +694,27 @@ internal struct ObjectCopyRunner { break } progress.startObject(step.qualifiedTargetName) - let wrapsInTransaction = request.wrapEachTableInTransaction - && request.errorHandling != .skipAndContinue do { let copied = try await copyRows( step, request: request, targetType: targetType, - wrapsInTransaction: wrapsInTransaction, completedBefore: result.rowsCopied, progress: progress ) let outcome = copied.outcome result.joinedSessionTransaction = result.joinedSessionTransaction || copied.joinedSessionTransaction - /// A cancelled table that rolled back neither counts as copied nor reads as an - /// object that succeeded. On a target without transactions nothing rolled back, so - /// the batches already flushed are in the target and saying otherwise hides them - /// from a user about to retry and double the rows. + /// Nothing rolled back, so the batches already flushed are in the target, and + /// saying otherwise hides them from a user about to retry and double the rows. guard !outcome.cancelled else { result.cancelled = true result.stopped = true - if outcome.committed > 0 { - result.rowsCopied += outcome.committed + if outcome.inserted > 0 { + result.rowsCopied += outcome.inserted result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: outcome.committed, error: nil + selection: step.selection, rowsCopied: outcome.inserted, error: nil )) } progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) @@ -735,8 +735,11 @@ internal struct ObjectCopyRunner { Self.logger.error( "Copy rows failed for \(step.qualifiedTargetName, privacy: .public): \(error.publicLogShape, privacy: .public)" ) + let written = (error as? ObjectCopyRowCopier.PartialFailure)?.written ?? 0 + result.rowsCopied += written + progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: 0, error: error.localizedDescription + selection: step.selection, rowsCopied: written, error: error.localizedDescription )) guard request.errorHandling == .skipAndContinue else { result.stopped = true @@ -757,7 +760,6 @@ internal struct ObjectCopyRunner { _ step: ObjectCopyTableStep, request: ObjectCopyRequest, targetType: DatabaseType, - wrapsInTransaction: Bool, completedBefore: Int, progress: ObjectCopyProgress ) async throws -> CopiedRows { @@ -766,7 +768,6 @@ internal struct ObjectCopyRunner { let copier = ObjectCopyRowCopier( step: step, sourceDatabaseType: request.source.databaseType, targetDatabaseType: targetType ) - let errorHandling = request.errorHandling /// The injected manager on both sides. Reaching for the singleton on the target bypassed /// the caller's own connections and routing, which is the whole point of injecting one. @@ -782,48 +783,10 @@ internal struct ObjectCopyRunner { throw ObjectCopyError.refused(Self.noTargetDriver) } let owner = await Self.transactionOwner(of: targetDriver) - let usesTransaction = wrapsInTransaction && owner.opensTransaction - if usesTransaction { - try await targetPlugin.beginTransaction(mode: .readWrite) - } - do { - // Past a skipped failure a child's parent rows can be missing, and only the checks say so. - let outcome = try await Self.withForeignKeysRelaxed( - on: targetPlugin, when: owner != .session && errorHandling != .skipAndContinue - ) { - try await copier.copy(from: sourcePlugin, to: targetPlugin) { rows in - progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) - } - } - guard usesTransaction else { - /// Nothing of the copy's own to roll back, so every batch already flushed is - /// in the target, or pending in the transaction the session already held, - /// whether the user stopped or not. - return CopiedRows( - outcome: ObjectCopyRowCopier.Outcome( - inserted: outcome.inserted, - cancelled: outcome.cancelled, - committed: outcome.inserted - ), - joinedSessionTransaction: owner == .session - ) - } - if outcome.cancelled { - try? await targetPlugin.rollbackTransaction() - } else { - try await targetPlugin.commitTransaction() - } - return CopiedRows(outcome: outcome, joinedSessionTransaction: false) - } catch { - if usesTransaction { - if errorHandling == .stopAndCommit { - try? await targetPlugin.commitTransaction() - } else { - try? await targetPlugin.rollbackTransaction() - } - } - throw error + let outcome = try await copier.copy(from: sourcePlugin, to: targetPlugin) { rows in + progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) } + return CopiedRows(outcome: outcome, joinedSessionTransaction: owner == .session) } } } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift index 7f091e100b..c7236c8a93 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift @@ -180,7 +180,8 @@ struct ObjectCopyDDLSessionTests { createsDatabase: false, tableSteps: tables, definitionSteps: definitions, - isStructureReadPinned: isStructureReadPinned + isStructureReadPinned: isStructureReadPinned, + keysStayInside: true ) } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift index b845fb5af4..b4df23ccb7 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift @@ -404,4 +404,72 @@ struct ObjectCopyPlannerOrderingTests { #expect(parent < child) #expect(ordered.count == 3) } + + // MARK: - Keys inside the copy + + private func keysStayInside( + _ selections: [ObjectCopySelection], + reads: [ObjectCopySelection: TableStructureRead], + rows: [ObjectCopySelection: Bool] + ) -> Bool { + ObjectCopyPlanner.keysStayInside([(namespace: "public", tables: selections)], reads: reads, rows: rows) + } + + @Test + func aCycleCopiedWholeKeepsItsKeysInside() { + let orders = selection("orders", schema: nil) + let customers = selection("customers", schema: nil) + let reads = [ + orders: read("orders", referencing: ["customers"]), + customers: read("customers", referencing: ["orders", "customers"]) + ] + + #expect(keysStayInside([orders, customers], reads: reads, rows: [orders: true, customers: true])) + } + + @Test + func aChildWithoutItsParentPointsOutside() { + let orders = selection("orders") + + #expect(!keysStayInside([orders], reads: [orders: read("orders", referencing: ["customers"])], rows: [orders: true])) + } + + @Test + func aParentMissingRowsLeavesItsChildOutside() { + let orders = selection("orders") + let customers = selection("customers") + let reads = [orders: read("orders", referencing: ["customers"]), customers: read("customers")] + + #expect(!keysStayInside([orders, customers], reads: reads, rows: [orders: true, customers: false])) + #expect(!keysStayInside([orders, customers], reads: reads, rows: [orders: true])) + } + + /// A table that gets no rows only needs its parent created, not filled. + @Test + func structureNeedsOnlyTheParentInTheCopy() { + let orders = selection("orders") + let customers = selection("customers") + let reads = [orders: read("orders", referencing: ["customers"]), customers: read("customers")] + + #expect(keysStayInside([orders, customers], reads: reads, rows: [:])) + } + + @Test + func aKeyIntoAnotherSchemaNeedsThatSchemasTable() { + let orders = selection("orders", schema: "app") + let users = selection("users", schema: "core") + let reads = [ + orders: read("orders", tableSchema: "app", referencing: ["users"], referencedSchema: "core"), + users: read("users", tableSchema: "core") + ] + + #expect(ObjectCopyPlanner.keysStayInside( + [(namespace: "app", tables: [orders]), (namespace: "core", tables: [users])], + reads: reads, + rows: [orders: true, users: true] + )) + #expect(!ObjectCopyPlanner.keysStayInside( + [(namespace: "app", tables: [orders])], reads: reads, rows: [orders: true] + )) + } } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift index 0c0ac4a3c2..cce310c0d9 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift @@ -331,7 +331,8 @@ struct ObjectCopySessionScopeTests { ), createsDatabase: false, tableSteps: [step()], - definitionSteps: [] + definitionSteps: [], + keysStayInside: true ) let provider = TwoConnectionProvider(drivers: [ sourceScope.connectionId: PluginDriverAdapter( diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index 1b79a9c67f..81b58e2bde 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -33,7 +33,10 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { private let sessionState: PluginSessionTransactionState private let failing: Set private let failsInserts: Bool + private let failsInsertsAfter: Int? private let cancelsOnInsert: Bool + private let rowCount: Int + private var inserts = 0 init( _ name: String, @@ -41,14 +44,18 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { sessionState: PluginSessionTransactionState = .idle, failing: Set = [], failsInserts: Bool = false, - cancelsOnInsert: Bool = false + failsInsertsAfter: Int? = nil, + cancelsOnInsert: Bool = false, + rowCount: Int = 1 ) { self.name = name self.log = log self.sessionState = sessionState self.failing = failing self.failsInserts = failsInserts + self.failsInsertsAfter = failsInsertsAfter self.cancelsOnInsert = cancelsOnInsert + self.rowCount = rowCount } var capabilities: PluginCapabilities { [] } @@ -75,7 +82,11 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { record(query) if failing.contains(query) { throw StatementFailure() } return PluginQueryResult( - columns: ["id"], columnTypeNames: [], rows: [[.text("1")]], rowsAffected: 1, executionTime: 0 + columns: ["id"], + columnTypeNames: [], + rows: Array(repeating: [.text("1")], count: rowCount), + rowsAffected: rowCount, + executionTime: 0 ) } @@ -87,7 +98,8 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { task.cancel() } } - if failsInserts { throw StatementFailure() } + inserts += 1 + if failsInserts || inserts > failsInsertsAfter ?? .max { throw StatementFailure() } return PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 1, executionTime: 0) } @@ -176,13 +188,15 @@ struct ObjectCopyTransactionTests { private func dataPlan( _ steps: [ObjectCopyTableStep], - errorHandling: ImportErrorHandling = .stopAndRollback + errorHandling: ImportErrorHandling = .stopAndRollback, + keysStayInside: Bool = true ) -> ObjectCopyPlan { ObjectCopyPlan( request: request(steps.map(\.selection), content: .data, errorHandling: errorHandling), createsDatabase: false, tableSteps: steps, - definitionSteps: [] + definitionSteps: [], + keysStayInside: keysStayInside ) } @@ -298,6 +312,45 @@ struct ObjectCopyTransactionTests { ]) } + /// MySQL refuses a CREATE whose key names a missing table only while the checks are on. + @Test("A new table relaxes the target's checks only when its keys stay inside the copy", arguments: [true, false]) + func creationRelaxesOnlyInsideTheCopy(keysStayInside: Bool) async throws { + let log = CopyEventLog() + let base = replacementPlan(errorHandling: .stopAndCommit) + let created = base.tableSteps.map { step in + ObjectCopyTableStep( + selection: step.selection, + dropStatements: [], + sequenceStatements: [], + createStatements: step.createStatements, + truncateStatements: [], + columns: [], + primaryKeyColumns: step.primaryKeyColumns, + sourceQuery: step.sourceQuery, + targetTable: step.targetTable, + targetSchema: nil, + estimatedRows: nil, + copiesData: false, + copiesIdentityColumn: false, + note: nil + ) + } + let plan = ObjectCopyPlan( + request: base.request, + createsDatabase: false, + tableSteps: created, + definitionSteps: [], + keysStayInside: keysStayInside + ) + + _ = try await run(plan, source: RecordingCopyDriver("source", log: log), target: RecordingCopyDriver("target", log: log)) + + let create = "target CREATE TABLE orders (id INTEGER);" + #expect(log.events == (keysStayInside + ? ["target open relaxedForeignKeys", create, "target close relaxedForeignKeys"] + : [create])) + } + @Test("A schema that cannot be created rolls the structure transaction back before the lease returns") func failedSchemaRollsBack() async throws { let log = CopyEventLog() @@ -345,6 +398,64 @@ struct ObjectCopyTransactionTests { ]) } + /// Turning the checks back on checks nothing already written, so a child copied without its + /// parent's rows would keep pointing at nothing. + @Test("Rows whose keys point outside the copy keep the target's checks on") + func keysOutsideTheCopyKeepTheChecks() async throws { + let log = CopyEventLog() + + _ = try await run( + dataPlan([dataStep("buildings")], keysStayInside: false), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) + ) + + #expect(log.events == [ + "target begin", "source SELECT id FROM buildings", "target insert", "target commit", + ]) + } + + /// Committed table by table, a cycle's first table kept rows pointing at a second that then failed. + @Test("Stop and roll back writes every table's rows in one transaction") + func rollbackRunsTheRowsInOneTransaction() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("estates"), dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) + ) + + #expect(result.rowsCopied == 2) + #expect(log.events == [ + "target begin", + "target open relaxedForeignKeys", + "source SELECT id FROM estates", + "target insert", + "source SELECT id FROM buildings", + "target insert", + "target close relaxedForeignKeys", + "target commit", + ]) + } + + /// PostgreSQL commits an aborted transaction as a ROLLBACK, so a table wrapped in one lost + /// every batch written ahead of its failure. + @Test("Stop and keep what ran commits each batch with the checks on, and counts what a failure kept") + func stopAndKeepCommitsEachBatch() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("buildings")], errorHandling: .stopAndCommit), + source: RecordingCopyDriver("source", log: log, rowCount: SQLWriteBatchBudget.maximumRows + 1), + target: RecordingCopyDriver("target", log: log, failsInsertsAfter: 1) + ) + + #expect(result.rowsCopied == SQLWriteBatchBudget.maximumRows) + #expect(result.failures.map { $0.outcome.rowsCopied } == [SQLWriteBatchBudget.maximumRows]) + #expect(log.events == ["source SELECT id FROM buildings", "target insert", "target insert"]) + } + /// Relaxed, a DELETE would leave rows in a table outside the copy pointing at nothing. @Test("Clears run with the target's keys on, before the rows relax them") func clearsRunWithKeysOn() async throws { diff --git a/apple/changelog.d/3859.changed.md b/apple/changelog.d/3859.changed.md new file mode 100644 index 0000000000..acd11055bb --- /dev/null +++ b/apple/changelog.d/3859.changed.md @@ -0,0 +1 @@ +Copy To's Stop and roll back now takes back the rows of every table, not only the one that failed. diff --git a/apple/changelog.d/3859.fixed.6.md b/apple/changelog.d/3859.fixed.6.md new file mode 100644 index 0000000000..c8fc16bddc --- /dev/null +++ b/apple/changelog.d/3859.fixed.6.md @@ -0,0 +1 @@ +After a Copy To into PostgreSQL failed creating a schema, the next Copy To or Compare on that database could fail. From 8761437b14c435caf87cf5a43e597d433c329bba Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 23:24:06 +0700 Subject: [PATCH 07/13] fix(copy): keep checks on for tables the target keeps, and count what a no-transaction target kept --- .../Core/ObjectCopy/ObjectCopyPlan.swift | 17 ++- .../Core/ObjectCopy/ObjectCopyPlanner.swift | 49 ++++---- .../Core/ObjectCopy/ObjectCopyRunner.swift | 115 +++++++++--------- .../ObjectCopyDDLSessionTests.swift | 2 +- .../ObjectCopyPlannerOrderingTests.swift | 52 +++++--- .../ObjectCopySessionScopeTests.swift | 2 +- .../ObjectCopyTransactionTests.swift | 61 ++++++++-- 7 files changed, 185 insertions(+), 113 deletions(-) diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index c29880f856..e8d5c33468 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -207,6 +207,16 @@ internal struct ObjectCopyPartialNote: Identifiable, Sendable { internal var id: String { selection.id } } +/// Whether the copy's foreign keys point only at tables it writes, which is when the target's checks +/// may be off: turning them back on checks nothing already written. +internal struct ObjectCopyKeysInside: Sendable, Equatable { + /// Every key of a table the copy creates names a table in the copy. + internal var created = false + /// Every table that gets rows is one the copy creates, so the target holds the source's keys + /// and no others, and each names a table that gets all of its rows. + internal var rows = false +} + internal struct ObjectCopyPlan: Sendable { internal let request: ObjectCopyRequest internal let createsDatabase: Bool @@ -224,8 +234,7 @@ internal struct ObjectCopyPlan: Sendable { /// so the run may write that DDL under the target's write scope. A definition read as the /// session is runs as the session is, the way it did before scopes. internal let isStructureReadPinned: Bool - /// `ObjectCopyPlanner.keysStayInside`: whether the target's foreign key checks may be off. - internal let keysStayInside: Bool + internal let keysInside: ObjectCopyKeysInside internal init( request: ObjectCopyRequest, @@ -235,7 +244,7 @@ internal struct ObjectCopyPlan: Sendable { schemaStatements: [SyncStatement] = [], skipped: [ObjectCopySkip] = [], isStructureReadPinned: Bool = false, - keysStayInside: Bool = false + keysInside: ObjectCopyKeysInside = ObjectCopyKeysInside() ) { self.request = request self.createsDatabase = createsDatabase @@ -244,7 +253,7 @@ internal struct ObjectCopyPlan: Sendable { self.schemaStatements = schemaStatements self.skipped = skipped self.isStructureReadPinned = isStructureReadPinned - self.keysStayInside = keysStayInside + self.keysInside = keysInside } /// Shown above the script, for the caveats the copy cannot do anything about: a key column the diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index f3620100d8..0ca3dd2791 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -109,9 +109,10 @@ internal struct ObjectCopyPlanner { } let parentsFirst = Self.orderAcrossScopes(&passes) let drafts = passes.flatMap(\.drafts) - let keysStayInside = Self.keysStayInside( + let keysInside = Self.keysInside( passes.map { (namespace: $0.sourceNamespace, tables: $0.drafts.map(\.selection)) }, reads: Dictionary(drafts.map { ($0.selection, $0.read) }, uniquingKeysWith: { first, _ in first }), + created: Set(drafts.filter(\.writesStructure).map(\.selection)), rows: Dictionary( drafts.filter(\.copiesData).map { draft in let scope = draft.rowScope @@ -168,7 +169,7 @@ internal struct ObjectCopyPlanner { schemaStatements: try await buildSchemaStatements(request, namespaces: targetNamespaces), skipped: skipped, isStructureReadPinned: isStructureReadPinned, - keysStayInside: keysStayInside + keysInside: keysInside ) } @@ -1133,35 +1134,41 @@ internal struct ObjectCopyPlanner { return result } - /// Whether every key a copied table holds points at a table in the copy, and, for a table that - /// gets rows, at one that gets all of its rows. `rows` holds each table that gets rows, true when - /// it gets every one. - /// - /// Only then may the target's checks be off: turning them back on checks nothing already - /// written, so a child copied without its parent's rows would keep pointing at nothing. - nonisolated internal static func keysStayInside( + /// `created` holds each table whose structure the copy writes, and `rows` each table that gets + /// rows, true when it gets every one. Keys are read from the source, which is what the target + /// holds only for a table the copy creates: one it keeps may carry keys of its own. + nonisolated internal static func keysInside( _ scopes: [(namespace: String?, tables: [ObjectCopySelection])], reads: [ObjectCopySelection: TableStructureRead], + created: Set, rows: [ObjectCopySelection: Bool] - ) -> Bool { - var nodes: [(selection: ObjectCopySelection, read: TableStructureRead, node: ForeignKeyTopologicalSort.Table)] = [] + ) -> ObjectCopyKeysInside { + var nodes: [(selection: ObjectCopySelection, keys: [PluginForeignKeyInfo], node: ForeignKeyTopologicalSort.Table)] = [] for scope in scopes { for selection in scope.tables { - guard let read = reads[selection] else { return false } - nodes.append((selection, read, sortNode(selection, read: read, namespace: scope.namespace))) + guard let read = reads[selection] else { return ObjectCopyKeysInside() } + nodes.append((selection, read.foreignKeys, sortNode(selection, read: read, namespace: scope.namespace))) + } + } + func pointsWithin( + _ keys: [PluginForeignKeyInfo], from node: ForeignKeyTopologicalSort.Table, _ tables: Set + ) -> Bool { + keys.allSatisfy { key in + tables.contains(ForeignKeyTopologicalSort.Table( + name: key.referencedTable, schema: key.referencedSchema ?? node.schema + ).identifier) } } let present = Set(nodes.map(\.node.identifier)) let filled = Set(nodes.filter { rows[$0.selection] == true }.map(\.node.identifier)) - return nodes.allSatisfy { entry in - let parents = rows[entry.selection] == nil ? present : filled - return entry.read.foreignKeys.allSatisfy { key in - let parent = ForeignKeyTopologicalSort.Table( - name: key.referencedTable, schema: key.referencedSchema ?? entry.node.schema - ) - return parents.contains(parent.identifier) + return ObjectCopyKeysInside( + created: nodes.filter { created.contains($0.selection) }.allSatisfy { + pointsWithin($0.keys, from: $0.node, present) + }, + rows: nodes.filter { rows[$0.selection] != nil }.allSatisfy { + created.contains($0.selection) && pointsWithin($0.keys, from: $0.node, filled) } - } + ) } nonisolated private static func sortNode( diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index 5a10ead31f..1d26647b00 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -257,7 +257,7 @@ internal struct ObjectCopyRunner { // yet, which a cycle always has. Past a replacement, a key to a table outside the copy keeps // the checks, so it still fails when that table is missing. let dropsTables = plan.cleanupGroups.contains { !$0.statements.isEmpty } - let relaxesKeys = tableGroups.contains { !$0.statements.isEmpty } && (dropsTables || plan.keysStayInside) + let relaxesKeys = tableGroups.contains { !$0.statements.isEmpty } && (dropsTables || plan.keysInside.created) let pinsSession = plan.isStructureReadPinned && VerbatimTransferScopes.isSameEngine( source: request.source.databaseType, target: request.target.databaseType ) @@ -502,9 +502,47 @@ internal struct ObjectCopyRunner { var stopped = false /// True when the rows went into a transaction the session already held. var joinedSessionTransaction = false - /// Only what was committed. A cancelled table rolls its rows back, so counting what the - /// copier inserted reported rows the target never kept. + /// Every row written, a stopped or failed table's included. A rollback takes the count back + /// with the rows; a target that opened no transaction keeps both, and leaving them out + /// would send a retry over them. var rowsCopied = 0 + + mutating func recordCopy(of step: ObjectCopyTableStep, inserted: Int) { + rowsCopied += inserted + outcomes.append(ObjectCopyObjectOutcome(selection: step.selection, rowsCopied: inserted, error: nil)) + } + + /// Stop reaching the driver mid-stream is the user stopping, not the copy failing, and the + /// two produce different notifications. + mutating func recordStop(of step: ObjectCopyTableStep, inserted: Int, progress: ObjectCopyProgress) { + cancelled = true + stopped = true + if inserted > 0 { recordCopy(of: step, inserted: inserted) } + progress.setRowsForCurrentObject(0, completedBefore: rowsCopied) + } + + /// False when the run stops here. + mutating func recordFailure( + of step: ObjectCopyTableStep, + _ error: any Error, + errorHandling: ImportErrorHandling, + progress: ObjectCopyProgress + ) -> Bool { + ObjectCopyRunner.logger.error( + "Copy rows failed for \(step.qualifiedTargetName, privacy: .public): \(error.publicLogShape, privacy: .public)" + ) + let written = (error as? ObjectCopyRowCopier.PartialFailure)?.written ?? 0 + rowsCopied += written + progress.setRowsForCurrentObject(0, completedBefore: rowsCopied) + outcomes.append(ObjectCopyObjectOutcome( + selection: step.selection, rowsCopied: written, error: error.localizedDescription + )) + guard errorHandling == .skipAndContinue else { + stopped = true + return false + } + return true + } } private func runData( @@ -542,7 +580,7 @@ internal struct ObjectCopyRunner { let targetType = request.target.databaseType let sourceScope = request.source.scope let errorHandling = request.errorHandling - let keysStayInside = plan.keysStayInside + let relaxesRows = plan.keysInside.rows /// A lease this phase cannot take is a run-level failure with no object to pin it on, and /// it is thrown for the same reason the DDL phases throw it: swallowed into a stopped @@ -569,7 +607,7 @@ internal struct ObjectCopyRunner { result.stopped = cleared.stopped if !result.stopped { - try await Self.withForeignKeysRelaxed(on: targetPlugin, when: usesTransaction && keysStayInside) { + try await Self.withForeignKeysRelaxed(on: targetPlugin, when: usesTransaction && relaxesRows) { let copied = await Self.stream( steps, from: sourceScope, @@ -602,7 +640,9 @@ internal struct ObjectCopyRunner { try? await targetPlugin.rollbackTransaction() /// The target is back where it started, rows and all, so nothing may be reported /// as copied and no table may be reported as done. - result.outcomes = result.outcomes.filter { $0.error != nil } + result.outcomes = result.outcomes.filter { $0.error != nil }.map { + ObjectCopyObjectOutcome(selection: $0.selection, rowsCopied: 0, error: $0.error) + } result.rowsCopied = 0 return result } @@ -645,31 +685,15 @@ internal struct ObjectCopyRunner { } } guard !outcome.cancelled else { - result.cancelled = true - result.stopped = true - progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) + result.recordStop(of: step, inserted: outcome.inserted, progress: progress) break } - result.rowsCopied += outcome.inserted - result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: outcome.inserted, error: nil - )) + result.recordCopy(of: step, inserted: outcome.inserted) } catch is CancellationError { - result.cancelled = true - result.stopped = true - progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) + result.recordStop(of: step, inserted: 0, progress: progress) break } catch { - logger.error( - "Copy rows failed for \(step.qualifiedTargetName, privacy: .public): \(error.publicLogShape, privacy: .public)" - ) - result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: 0, error: error.localizedDescription - )) - guard errorHandling == .skipAndContinue else { - result.stopped = true - break - } + guard result.recordFailure(of: step, error, errorHandling: errorHandling, progress: progress) else { break } } } return result @@ -706,45 +730,18 @@ internal struct ObjectCopyRunner { let outcome = copied.outcome result.joinedSessionTransaction = result.joinedSessionTransaction || copied.joinedSessionTransaction - /// Nothing rolled back, so the batches already flushed are in the target, and - /// saying otherwise hides them from a user about to retry and double the rows. guard !outcome.cancelled else { - result.cancelled = true - result.stopped = true - if outcome.inserted > 0 { - result.rowsCopied += outcome.inserted - result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: outcome.inserted, error: nil - )) - } - progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) + result.recordStop(of: step, inserted: outcome.inserted, progress: progress) break } - result.rowsCopied += outcome.inserted - result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: outcome.inserted, error: nil - )) + result.recordCopy(of: step, inserted: outcome.inserted) } catch is CancellationError { - /// Stop reaching the driver mid-stream is the user stopping, not the copy failing, - /// and the two produce different notifications. - result.cancelled = true - result.stopped = true - progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) + result.recordStop(of: step, inserted: 0, progress: progress) break } catch { - Self.logger.error( - "Copy rows failed for \(step.qualifiedTargetName, privacy: .public): \(error.publicLogShape, privacy: .public)" - ) - let written = (error as? ObjectCopyRowCopier.PartialFailure)?.written ?? 0 - result.rowsCopied += written - progress.setRowsForCurrentObject(0, completedBefore: result.rowsCopied) - result.outcomes.append(ObjectCopyObjectOutcome( - selection: step.selection, rowsCopied: written, error: error.localizedDescription - )) - guard request.errorHandling == .skipAndContinue else { - result.stopped = true - break - } + guard result.recordFailure( + of: step, error, errorHandling: request.errorHandling, progress: progress + ) else { break } } } return result diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift index c7236c8a93..cc8368d46c 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyDDLSessionTests.swift @@ -181,7 +181,7 @@ struct ObjectCopyDDLSessionTests { tableSteps: tables, definitionSteps: definitions, isStructureReadPinned: isStructureReadPinned, - keysStayInside: true + keysInside: ObjectCopyKeysInside(created: true, rows: true) ) } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift index b4df23ccb7..fbf619938b 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift @@ -407,12 +407,15 @@ struct ObjectCopyPlannerOrderingTests { // MARK: - Keys inside the copy - private func keysStayInside( + private func keysInside( _ selections: [ObjectCopySelection], reads: [ObjectCopySelection: TableStructureRead], + created: Set? = nil, rows: [ObjectCopySelection: Bool] - ) -> Bool { - ObjectCopyPlanner.keysStayInside([(namespace: "public", tables: selections)], reads: reads, rows: rows) + ) -> ObjectCopyKeysInside { + ObjectCopyPlanner.keysInside( + [(namespace: "public", tables: selections)], reads: reads, created: created ?? Set(selections), rows: rows + ) } @Test @@ -424,34 +427,43 @@ struct ObjectCopyPlannerOrderingTests { customers: read("customers", referencing: ["orders", "customers"]) ] - #expect(keysStayInside([orders, customers], reads: reads, rows: [orders: true, customers: true])) + let inside = keysInside([orders, customers], reads: reads, rows: [orders: true, customers: true]) + + #expect(inside == ObjectCopyKeysInside(created: true, rows: true)) } @Test func aChildWithoutItsParentPointsOutside() { let orders = selection("orders") - #expect(!keysStayInside([orders], reads: [orders: read("orders", referencing: ["customers"])], rows: [orders: true])) + let inside = keysInside([orders], reads: [orders: read("orders", referencing: ["customers"])], rows: [orders: true]) + + #expect(inside == ObjectCopyKeysInside(created: false, rows: false)) } + /// A table that gets no rows only needs its parent created, not filled. @Test - func aParentMissingRowsLeavesItsChildOutside() { + func aParentMissingRowsLeavesOnlyTheRowsOutside() { let orders = selection("orders") let customers = selection("customers") let reads = [orders: read("orders", referencing: ["customers"]), customers: read("customers")] - #expect(!keysStayInside([orders, customers], reads: reads, rows: [orders: true, customers: false])) - #expect(!keysStayInside([orders, customers], reads: reads, rows: [orders: true])) + #expect(keysInside([orders, customers], reads: reads, rows: [orders: true, customers: false]) + == ObjectCopyKeysInside(created: true, rows: false)) + #expect(keysInside([orders, customers], reads: reads, rows: [orders: true]) + == ObjectCopyKeysInside(created: true, rows: false)) + #expect(keysInside([orders, customers], reads: reads, rows: [:]) + == ObjectCopyKeysInside(created: true, rows: true)) } - /// A table that gets no rows only needs its parent created, not filled. + /// The source's keys say nothing about a table the target keeps, which may reference a parent the copy never touches. @Test - func structureNeedsOnlyTheParentInTheCopy() { + func aTableTheTargetKeepsLeavesTheRowsOutside() { let orders = selection("orders") - let customers = selection("customers") - let reads = [orders: read("orders", referencing: ["customers"]), customers: read("customers")] - #expect(keysStayInside([orders, customers], reads: reads, rows: [:])) + let inside = keysInside([orders], reads: [orders: read("orders")], created: [], rows: [orders: true]) + + #expect(inside == ObjectCopyKeysInside(created: true, rows: false)) } @Test @@ -463,13 +475,17 @@ struct ObjectCopyPlannerOrderingTests { users: read("users", tableSchema: "core") ] - #expect(ObjectCopyPlanner.keysStayInside( + let whole = ObjectCopyPlanner.keysInside( [(namespace: "app", tables: [orders]), (namespace: "core", tables: [users])], reads: reads, + created: [orders, users], rows: [orders: true, users: true] - )) - #expect(!ObjectCopyPlanner.keysStayInside( - [(namespace: "app", tables: [orders])], reads: reads, rows: [orders: true] - )) + ) + let alone = ObjectCopyPlanner.keysInside( + [(namespace: "app", tables: [orders])], reads: reads, created: [orders], rows: [orders: true] + ) + + #expect(whole == ObjectCopyKeysInside(created: true, rows: true)) + #expect(alone == ObjectCopyKeysInside(created: false, rows: false)) } } diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift index cce310c0d9..d469f19cc2 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopySessionScopeTests.swift @@ -332,7 +332,7 @@ struct ObjectCopySessionScopeTests { createsDatabase: false, tableSteps: [step()], definitionSteps: [], - keysStayInside: true + keysInside: ObjectCopyKeysInside(created: true, rows: true) ) let provider = TwoConnectionProvider(drivers: [ sourceScope.connectionId: PluginDriverAdapter( diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index 81b58e2bde..e11dfb627b 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -36,6 +36,7 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { private let failsInsertsAfter: Int? private let cancelsOnInsert: Bool private let rowCount: Int + private let transactional: Bool private var inserts = 0 init( @@ -46,7 +47,8 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { failsInserts: Bool = false, failsInsertsAfter: Int? = nil, cancelsOnInsert: Bool = false, - rowCount: Int = 1 + rowCount: Int = 1, + transactional: Bool = true ) { self.name = name self.log = log @@ -56,11 +58,12 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { self.failsInsertsAfter = failsInsertsAfter self.cancelsOnInsert = cancelsOnInsert self.rowCount = rowCount + self.transactional = transactional } var capabilities: PluginCapabilities { [] } - var supportsTransactions: Bool { true } - var supportsTransactionalDDL: Bool { true } + var supportsTransactions: Bool { transactional } + var supportsTransactionalDDL: Bool { transactional } private func record(_ event: String) { log.record("\(name) \(event)") @@ -189,14 +192,14 @@ struct ObjectCopyTransactionTests { private func dataPlan( _ steps: [ObjectCopyTableStep], errorHandling: ImportErrorHandling = .stopAndRollback, - keysStayInside: Bool = true + keysInside: Bool = true ) -> ObjectCopyPlan { ObjectCopyPlan( request: request(steps.map(\.selection), content: .data, errorHandling: errorHandling), createsDatabase: false, tableSteps: steps, definitionSteps: [], - keysStayInside: keysStayInside + keysInside: ObjectCopyKeysInside(created: keysInside, rows: keysInside) ) } @@ -314,7 +317,7 @@ struct ObjectCopyTransactionTests { /// MySQL refuses a CREATE whose key names a missing table only while the checks are on. @Test("A new table relaxes the target's checks only when its keys stay inside the copy", arguments: [true, false]) - func creationRelaxesOnlyInsideTheCopy(keysStayInside: Bool) async throws { + func creationRelaxesOnlyInsideTheCopy(keysInside: Bool) async throws { let log = CopyEventLog() let base = replacementPlan(errorHandling: .stopAndCommit) let created = base.tableSteps.map { step in @@ -340,13 +343,13 @@ struct ObjectCopyTransactionTests { createsDatabase: false, tableSteps: created, definitionSteps: [], - keysStayInside: keysStayInside + keysInside: ObjectCopyKeysInside(created: keysInside) ) _ = try await run(plan, source: RecordingCopyDriver("source", log: log), target: RecordingCopyDriver("target", log: log)) let create = "target CREATE TABLE orders (id INTEGER);" - #expect(log.events == (keysStayInside + #expect(log.events == (keysInside ? ["target open relaxedForeignKeys", create, "target close relaxedForeignKeys"] : [create])) } @@ -405,7 +408,7 @@ struct ObjectCopyTransactionTests { let log = CopyEventLog() _ = try await run( - dataPlan([dataStep("buildings")], keysStayInside: false), + dataPlan([dataStep("buildings")], keysInside: false), source: RecordingCopyDriver("source", log: log), target: RecordingCopyDriver("target", log: log) ) @@ -456,6 +459,46 @@ struct ObjectCopyTransactionTests { #expect(log.events == ["source SELECT id FROM buildings", "target insert", "target insert"]) } + @Test("A rolled back table reports none of the batches it wrote before failing") + func rolledBackFailureCountsNothing() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log, rowCount: SQLWriteBatchBudget.maximumRows + 1), + target: RecordingCopyDriver("target", log: log, failsInsertsAfter: 1) + ) + + #expect(result.rowsCopied == 0) + #expect(result.failures.map { $0.outcome.rowsCopied } == [0]) + #expect(log.events.last == "target rollback") + } + + /// Nothing can be taken back there, so leaving the rows out of the count would send a retry over them. + @Test("A target with no transactions counts what a stopped or failed table wrote") + func untransactionalTargetCountsWhatItKept() async throws { + let log = CopyEventLog() + + let stopped = try await Task { + try await run( + dataPlan([dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, cancelsOnInsert: true, transactional: false) + ) + }.value + let failed = try await run( + dataPlan([dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log, rowCount: SQLWriteBatchBudget.maximumRows + 1), + target: RecordingCopyDriver("target", log: log, failsInsertsAfter: 1, transactional: false) + ) + + #expect(stopped.cancelled) + #expect(stopped.rowsCopied == 1) + #expect(failed.rowsCopied == SQLWriteBatchBudget.maximumRows) + #expect(!log.events.contains("target begin")) + #expect(!log.events.contains("target open relaxedForeignKeys")) + } + /// Relaxed, a DELETE would leave rows in a table outside the copy pointing at nothing. @Test("Clears run with the target's keys on, before the rows relax them") func clearsRunWithKeysOn() async throws { From b31f22114d6faacd4cdb99595765fbcc15d790a0 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 23:41:42 +0700 Subject: [PATCH 08/13] fix(copy): check every key before a relaxed copy commits, lease the target first, and count a thrown cancellation --- .../Core/ObjectCopy/ObjectCopyPlan.swift | 13 ++- .../Core/ObjectCopy/ObjectCopyPlanner.swift | 39 ++++++- .../Core/ObjectCopy/ObjectCopyRowCopier.swift | 4 +- .../Core/ObjectCopy/ObjectCopyRunner.swift | 67 +++++++++--- .../ObjectCopy/ObjectCopyTableDraft.swift | 1 + .../TablePro/Resources/Localizable.xcstrings | 3 + .../ObjectCopyPlannerOrderingTests.swift | 47 ++++++++ .../ObjectCopyTransactionTests.swift | 103 +++++++++++++++++- 8 files changed, 253 insertions(+), 24 deletions(-) diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index e8d5c33468..c1ac8a8975 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -61,6 +61,9 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { /// Set when this table is in the plan but part of it cannot run, so the sheet can say why /// before the user presses Copy. internal let note: String? + /// One count per foreign key this table is created with. Nil when a key cannot be checked, + /// which keeps the target's checks on for the rows. + internal let keyChecks: [ObjectCopyKeyCheck]? /// Spelled out rather than left to the memberwise init so the three fields a same-engine copy /// never sets can default. Every one of them describes something only a crossing or a @@ -83,8 +86,10 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { coercer: CrossEngineValueCoercer? = nil, serverSideInsert: SyncStatement? = nil, hasRowFilter: Bool = false, - note: String? + note: String?, + keyChecks: [ObjectCopyKeyCheck]? = [] ) { + self.keyChecks = keyChecks self.selection = selection self.dropStatements = dropStatements self.sequenceStatements = sequenceStatements @@ -207,6 +212,12 @@ internal struct ObjectCopyPartialNote: Identifiable, Sendable { internal var id: String { selection.id } } +/// Counts the rows of a table that point at nothing through one foreign key. +internal struct ObjectCopyKeyCheck: Sendable, Equatable { + internal let constraint: String + internal let sql: String +} + /// Whether the copy's foreign keys point only at tables it writes, which is when the target's checks /// may be off: turning them back on checks nothing already written. internal struct ObjectCopyKeysInside: Sendable, Equatable { diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index 0ca3dd2791..ef3649b56e 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -109,7 +109,7 @@ internal struct ObjectCopyPlanner { } let parentsFirst = Self.orderAcrossScopes(&passes) let drafts = passes.flatMap(\.drafts) - let keysInside = Self.keysInside( + var keysInside = Self.keysInside( passes.map { (namespace: $0.sourceNamespace, tables: $0.drafts.map(\.selection)) }, reads: Dictionary(drafts.map { ($0.selection, $0.read) }, uniquingKeysWith: { first, _ in first }), created: Set(drafts.filter(\.writesStructure).map(\.selection)), @@ -160,6 +160,7 @@ internal struct ObjectCopyPlanner { ) } tableSteps.sort { parentsFirst($0.selection, $1.selection) } + keysInside.rows = keysInside.rows && tableSteps.filter(\.copiesData).allSatisfy { $0.keyChecks != nil } return ObjectCopyPlan( request: request, @@ -542,7 +543,8 @@ internal struct ObjectCopyPlanner { coercer: draft.coercer, serverSideInsert: draft.copiesData ? serverSide[draft.selection.id] : nil, hasRowFilter: VerbatimTransferScopes.filtersRows(draft.rowScope), - note: draft.note + note: draft.note, + keyChecks: statements.keyChecks ) } } @@ -781,6 +783,9 @@ internal struct ObjectCopyPlanner { ddl.create = try builder.build( operations: [.createTable(input.snapshot)], foreignKeysByTable: [:] ) + ddl.keyChecks = Self.keyChecks( + for: input.snapshot, schema: input.targetSchema, driver: plugin, databaseType: driver.connection.type + ) } if input.emptiesFirst { ddl.truncate = Self.emptyStatements( @@ -864,6 +869,36 @@ internal struct ObjectCopyPlanner { ) } + /// Turning the target's checks back on checks nothing already written, so a copy that wrote + /// with them off counts what each key would have refused before it commits. Nil when a key's + /// columns do not pair up, since that one could not be checked. + nonisolated static func keyChecks( + for snapshot: TableStructureSnapshot, + schema: String?, + driver: any PluginDatabaseDriver, + databaseType: DatabaseType + ) -> [ObjectCopyKeyCheck]? { + let quote = driver.quoteIdentifier + let child = SchemaQualifiedName.render(name: snapshot.name, schema: schema, databaseType: databaseType, quote: quote) + var checks: [ObjectCopyKeyCheck] = [] + for key in snapshot.foreignKeys { + guard !key.columns.isEmpty, key.columns.count == key.referencedColumns.count else { return nil } + let parent = SchemaQualifiedName.render( + name: key.referencedTable, schema: key.referencedSchema ?? schema, databaseType: databaseType, quote: quote + ) + // A key with any column NULL holds nothing, the way MATCH SIMPLE reads it. + let present = key.columns.map { "c.\(quote($0)) IS NOT NULL" }.joined(separator: " AND ") + let matches = zip(key.columns, key.referencedColumns) + .map { "p.\(quote($1)) = c.\(quote($0))" } + .joined(separator: " AND ") + checks.append(ObjectCopyKeyCheck( + constraint: key.name, + sql: "SELECT COUNT(*) FROM \(child) c WHERE \(present) AND NOT EXISTS (SELECT 1 FROM \(parent) p WHERE \(matches))" + )) + } + return checks + } + /// What empties a table before its rows are written. /// /// DELETE whenever the run promises to roll the table back. TRUNCATE commits implicitly on the diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift index 329303b753..2ed4008ce1 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRowCopier.swift @@ -122,7 +122,9 @@ internal struct ObjectCopyRowCopier: Sendable { onProgress(inserted) } return Outcome(inserted: inserted, cancelled: Task.isCancelled) - } catch where inserted > 0 && !(error is CancellationError) { + } catch is CancellationError { + return Outcome(inserted: inserted, cancelled: true) + } catch where inserted > 0 { throw PartialFailure(written: inserted, underlying: error) } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index 1d26647b00..f58632d2a6 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -276,7 +276,7 @@ internal struct ObjectCopyRunner { do { result = try await Self.withForeignKeysRelaxed( on: plugin, when: relaxesKeys && !usesTransaction && owner != .session - ) { + ) { _ in try await Self.executeTableDDL( schemaStatements: schemaStatements, groups: tableGroups, @@ -426,9 +426,9 @@ internal struct ObjectCopyRunner { nonisolated private static func withForeignKeysRelaxed( on plugin: any PluginDatabaseDriver, when relaxes: Bool, - _ body: () async throws -> T + _ body: (_ relaxed: Bool) async throws -> T ) async throws -> T { - guard relaxes else { return try await body() } + guard relaxes else { return try await body(false) } let opened: PluginSessionScope? do { opened = try await plugin.openSessionScope(for: .relaxedForeignKeys) @@ -436,7 +436,7 @@ internal struct ObjectCopyRunner { logger.warning("Copying with foreign key checks on: \(error.publicLogShape, privacy: .public)") opened = nil } - guard let scope = opened else { return try await body() } + guard let scope = opened else { return try await body(false) } // Shielded like a rollback: the connection goes back to the pool after this lease. func close() async { do { @@ -446,7 +446,7 @@ internal struct ObjectCopyRunner { } } do { - let value = try await body() + let value = try await body(true) await close() return value } catch { @@ -607,7 +607,7 @@ internal struct ObjectCopyRunner { result.stopped = cleared.stopped if !result.stopped { - try await Self.withForeignKeysRelaxed(on: targetPlugin, when: usesTransaction && relaxesRows) { + try await Self.withForeignKeysRelaxed(on: targetPlugin, when: usesTransaction && relaxesRows) { relaxed in let copied = await Self.stream( steps, from: sourceScope, @@ -622,6 +622,11 @@ internal struct ObjectCopyRunner { result.cancelled = result.cancelled || copied.cancelled result.stopped = copied.stopped result.rowsCopied = copied.rowsCopied + // A row read while the source was changing, or one the source already broke, + // fails here rather than committing past keys nothing checks again. + if relaxed, !result.stopped { + await Self.checkKeys(of: steps, on: targetPlugin, into: &result, progress: progress) + } } } @@ -769,17 +774,19 @@ internal struct ObjectCopyRunner { /// The injected manager on both sides. Reaching for the singleton on the target bypassed /// the caller's own connections and routing, which is the whole point of injecting one. let manager = self.manager - return try await manager.withMetadataDriver(scope: sourceScope, workload: .bulk) { sourceDriver in - guard let sourcePlugin = CompareMetadataService.pluginDriver(from: sourceDriver) else { - throw ObjectCopyError.refused(Self.noSourceDriver) + // Target first, as `runInOneTransaction` takes them: two copies taking the two leases in + // opposite orders could each hold the one the other waits for. + return try await manager.withMetadataDriver(scope: targetScope, workload: .bulk) { targetDriver in + guard let targetPlugin = CompareMetadataService.pluginDriver(from: targetDriver) else { + throw ObjectCopyError.refused(Self.noTargetDriver) } + let owner = await Self.transactionOwner(of: targetDriver) return try await manager.withMetadataDriver( - scope: targetScope, workload: .bulk - ) { targetDriver in - guard let targetPlugin = CompareMetadataService.pluginDriver(from: targetDriver) else { - throw ObjectCopyError.refused(Self.noTargetDriver) + scope: sourceScope, workload: .bulk + ) { sourceDriver in + guard let sourcePlugin = CompareMetadataService.pluginDriver(from: sourceDriver) else { + throw ObjectCopyError.refused(Self.noSourceDriver) } - let owner = await Self.transactionOwner(of: targetDriver) let outcome = try await copier.copy(from: sourcePlugin, to: targetPlugin) { rows in progress.setRowsForCurrentObject(rows, completedBefore: completedBefore) } @@ -788,6 +795,38 @@ internal struct ObjectCopyRunner { } } + /// Stops at the first key that fails, since the rollback that follows takes back every table. + nonisolated private static func checkKeys( + of steps: [ObjectCopyTableStep], + on driver: any PluginDatabaseDriver, + into result: inout DataResult, + progress: ObjectCopyProgress + ) async { + for step in steps { + for check in step.keyChecks ?? [] { + if progress.isCancelled || Task.isCancelled { + result.cancelled = true + result.stopped = true + return + } + let failure: String + do { + let counted = try await driver.execute(query: check.sql).rows.first?.first?.asText + guard let broken = counted.flatMap(Int.init), broken > 0 else { continue } + failure = String( + format: String(localized: "Rows in %1$@ break the foreign key %2$@."), + step.qualifiedTargetName, check.constraint + ) + } catch { + failure = error.localizedDescription + } + result.outcomes.append(ObjectCopyObjectOutcome(selection: step.selection, rowsCopied: 0, error: failure)) + result.stopped = true + return + } + } + } + // MARK: - Authorization private func authorizationRequest(for plan: ObjectCopyPlan) -> OperationRequest { diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift index 2a9fc5183a..47b187f3fc 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift @@ -235,6 +235,7 @@ internal struct ObjectCopyTableDDL: Sendable { var drop: [SyncStatement] = [] var create: [SyncStatement] = [] var truncate: [SyncStatement] = [] + var keyChecks: [ObjectCopyKeyCheck]? } internal enum ObjectCopyError: LocalizedError { diff --git a/apple/TablePro/Resources/Localizable.xcstrings b/apple/TablePro/Resources/Localizable.xcstrings index a1b9eb19a6..e34544636d 100644 --- a/apple/TablePro/Resources/Localizable.xcstrings +++ b/apple/TablePro/Resources/Localizable.xcstrings @@ -152433,6 +152433,9 @@ } } } + }, + "Rows in %1$@ break the foreign key %2$@." : { + }, "Rows in %@ changed after they were compared. Compare again before generating the script." : { "localizations" : { diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift index fbf619938b..8572262700 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyPlannerOrderingTests.swift @@ -488,4 +488,51 @@ struct ObjectCopyPlannerOrderingTests { #expect(whole == ObjectCopyKeysInside(created: true, rows: true)) #expect(alone == ObjectCopyKeysInside(created: false, rows: false)) } + + // MARK: - Key checks + + private func key( + _ name: String, _ columns: [String], references table: String, _ referenced: [String] + ) -> EditableForeignKeyDefinition { + EditableForeignKeyDefinition( + id: UUID(), name: name, columns: columns, referencedTable: table, referencedColumns: referenced, + referencedSchema: nil, onDelete: .noAction, onUpdate: .noAction + ) + } + + private func keyChecks(_ keys: [EditableForeignKeyDefinition]) -> [ObjectCopyKeyCheck]? { + ObjectCopyPlanner.keyChecks( + for: TableStructureSnapshot(name: "lines", columns: [], foreignKeys: keys), + schema: nil, + driver: QuotingDriver(), + databaseType: .sqlite + ) + } + + @Test + func aCompositeKeyIsCheckedOnEveryColumnAndSkipsNulls() { + let checks = keyChecks([key("fk_order", ["order_id", "shop_id"], references: "orders", ["id", "shop_id"])]) + + #expect(checks == [ObjectCopyKeyCheck( + constraint: "fk_order", + sql: #"SELECT COUNT(*) FROM "lines" c WHERE c."order_id" IS NOT NULL AND c."shop_id" IS NOT NULL "# + + #"AND NOT EXISTS (SELECT 1 FROM "orders" p WHERE p."id" = c."order_id" AND p."shop_id" = c."shop_id")"# + )]) + } + + @Test + func aSelfReferenceIsCheckedAgainstItsOwnTable() { + let checks = keyChecks([key("fk_parent", ["parent_id"], references: "lines", ["id"])]) + + #expect(checks?.first?.sql.contains(#"FROM "lines" c"#) == true) + #expect(checks?.first?.sql.contains(#"FROM "lines" p"#) == true) + } + + @Test + func aKeyWhoseColumnsDoNotPairCannotBeChecked() { + #expect(keyChecks([key("fk_bad", ["a", "b"], references: "orders", ["id"])]) == nil) + #expect(keyChecks([]) == []) + } } + +private final class QuotingDriver: PluginDriverStub, @unchecked Sendable {} diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index e11dfb627b..8fe6df2b94 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -37,6 +37,8 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { private let cancelsOnInsert: Bool private let rowCount: Int private let transactional: Bool + private let brokenRows: Int + private let throwsCancellationAfter: Int? private var inserts = 0 init( @@ -48,7 +50,9 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { failsInsertsAfter: Int? = nil, cancelsOnInsert: Bool = false, rowCount: Int = 1, - transactional: Bool = true + transactional: Bool = true, + brokenRows: Int = 0, + throwsCancellationAfter: Int? = nil ) { self.name = name self.log = log @@ -59,6 +63,8 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { self.cancelsOnInsert = cancelsOnInsert self.rowCount = rowCount self.transactional = transactional + self.brokenRows = brokenRows + self.throwsCancellationAfter = throwsCancellationAfter } var capabilities: PluginCapabilities { [] } @@ -84,6 +90,11 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { func execute(query: String) async throws -> PluginQueryResult { record(query) if failing.contains(query) { throw StatementFailure() } + if query.hasPrefix("SELECT COUNT(*)") { + return PluginQueryResult( + columns: ["count"], columnTypeNames: [], rows: [[.text("\(brokenRows)")]], rowsAffected: 0, executionTime: 0 + ) + } return PluginQueryResult( columns: ["id"], columnTypeNames: [], @@ -102,6 +113,7 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { } } inserts += 1 + if inserts > throwsCancellationAfter ?? .max { throw CancellationError() } if failsInserts || inserts > failsInsertsAfter ?? .max { throw StatementFailure() } return PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 1, executionTime: 0) } @@ -114,9 +126,11 @@ private final class RecordingCopyDriver: PluginDriverStub, @unchecked Sendable { @MainActor private final class StubMetadataProvider: ScopedMetadataProviding { private let drivers: [UUID: DatabaseDriver] + private let leases: CopyEventLog? - init(drivers: [UUID: DatabaseDriver]) { + init(leases: CopyEventLog? = nil, drivers: [UUID: DatabaseDriver]) { self.drivers = drivers + self.leases = leases } func withMetadataDriver( @@ -127,6 +141,7 @@ private final class StubMetadataProvider: ScopedMetadataProviding { guard let driver = drivers[scope.connectionId] else { throw ObjectCopyError.refused("No driver for \(scope.connectionId)") } + leases?.record("lease \(scope.database)") return try await body(driver) } @@ -168,7 +183,9 @@ struct ObjectCopyTransactionTests { ) } - private func dataStep(_ table: String, clears: Bool = false) -> ObjectCopyTableStep { + private func dataStep( + _ table: String, clears: Bool = false, keyChecks: [ObjectCopyKeyCheck] = [] + ) -> ObjectCopyTableStep { ObjectCopyTableStep( selection: ObjectCopySelection(kind: .table, name: table, schema: nil), dropStatements: [], @@ -185,7 +202,8 @@ struct ObjectCopyTransactionTests { estimatedRows: nil, copiesData: true, copiesIdentityColumn: false, - note: nil + note: nil, + keyChecks: keyChecks ) } @@ -236,9 +254,10 @@ struct ObjectCopyTransactionTests { private func run( _ plan: ObjectCopyPlan, source: RecordingCopyDriver, - target: RecordingCopyDriver + target: RecordingCopyDriver, + leases: CopyEventLog? = nil ) async throws -> ObjectCopyRunResult { - let provider = StubMetadataProvider(drivers: [ + let provider = StubMetadataProvider(leases: leases, drivers: [ sourceScope.connectionId: PluginDriverAdapter( connection: TestFixtures.makeConnection(type: sourceType), pluginDriver: source ), @@ -499,6 +518,78 @@ struct ObjectCopyTransactionTests { #expect(!log.events.contains("target open relaxedForeignKeys")) } + @Test("A relaxed copy checks every key it wrote before it commits") + func relaxedCopyChecksKeysBeforeCommit() async throws { + let log = CopyEventLog() + let check = ObjectCopyKeyCheck(constraint: "fk_estate", sql: "SELECT COUNT(*) FROM buildings") + + let result = try await run( + dataPlan([dataStep("buildings", keyChecks: [check])]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log) + ) + + #expect(result.rowsCopied == 1) + #expect(log.events == [ + "target begin", + "target open relaxedForeignKeys", + "source SELECT id FROM buildings", + "target insert", + "target SELECT COUNT(*) FROM buildings", + "target close relaxedForeignKeys", + "target commit", + ]) + } + + /// A child read after its parent while the source was changing, or one the source already broke. + @Test("A row a key refuses fails the copy and takes every table back") + func brokenKeyRollsTheCopyBack() async throws { + let log = CopyEventLog() + let check = ObjectCopyKeyCheck(constraint: "fk_estate", sql: "SELECT COUNT(*) FROM buildings") + + let result = try await run( + dataPlan([dataStep("estates"), dataStep("buildings", keyChecks: [check])]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, brokenRows: 2) + ) + + #expect(result.rowsCopied == 0) + #expect(result.failures.map { $0.outcome.selection.name } == ["buildings"]) + #expect(log.events.suffix(2) == ["target close relaxedForeignKeys", "target rollback"]) + } + + @Test( + "Both paths lease the target before the source", + arguments: [ImportErrorHandling.stopAndRollback, .stopAndCommit] + ) + func leasesTheTargetFirst(errorHandling: ImportErrorHandling) async throws { + let log = CopyEventLog() + let leases = CopyEventLog() + + _ = try await run( + dataPlan([dataStep("buildings")], errorHandling: errorHandling), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log), + leases: leases + ) + + #expect(leases.events == ["lease staging", "lease app"]) + } + + @Test("A driver that throws a cancellation keeps the count of the batches before it") + func thrownCancellationKeepsTheCount() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("buildings")], errorHandling: .stopAndCommit), + source: RecordingCopyDriver("source", log: log, rowCount: SQLWriteBatchBudget.maximumRows + 1), + target: RecordingCopyDriver("target", log: log, throwsCancellationAfter: 1) + ) + + #expect(result.cancelled) + #expect(result.rowsCopied == SQLWriteBatchBudget.maximumRows) + } + /// Relaxed, a DELETE would leave rows in a table outside the copy pointing at nothing. @Test("Clears run with the target's keys on, before the rows relax them") func clearsRunWithKeysOn() async throws { From ecc539528c7caf97e03c4bd952f168ecc4229210 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 27 Oct 2026 23:53:24 +0700 Subject: [PATCH 09/13] fix(copy): report the rows of a table the target keeps through a rollback --- .../Core/ObjectCopy/ObjectCopyPlan.swift | 7 +++++- .../Core/ObjectCopy/ObjectCopyPlanner.swift | 3 ++- .../Core/ObjectCopy/ObjectCopyRunner.swift | 14 +++++++---- .../ObjectCopy/ObjectCopyTableDraft.swift | 6 +++++ .../ObjectCopyTransactionTests.swift | 23 +++++++++++++++++-- 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index 36f0997dc8..0d1be56987 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -65,6 +65,9 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { /// One count per foreign key this table is created with. Nil when a key cannot be checked, /// which keeps the target's checks on for the rows. internal let keyChecks: [ObjectCopyKeyCheck]? + /// False when the target keeps this table's rows through a ROLLBACK, so a rolled back run still + /// reports them. + internal let rollsBack: Bool /// Spelled out rather than left to the memberwise init so the three fields a same-engine copy /// never sets can default. Every one of them describes something only a crossing or a @@ -88,9 +91,11 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { serverSideInsert: SyncStatement? = nil, hasRowFilter: Bool = false, note: String?, - keyChecks: [ObjectCopyKeyCheck]? = [] + keyChecks: [ObjectCopyKeyCheck]? = [], + rollsBack: Bool = true ) { self.keyChecks = keyChecks + self.rollsBack = rollsBack self.selection = selection self.dropStatements = dropStatements self.sequenceStatements = sequenceStatements diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index fffbd6bf9a..ad8c0235c4 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -554,7 +554,8 @@ internal struct ObjectCopyPlanner { serverSideInsert: draft.copiesData ? serverSide[draft.selection.id] : nil, hasRowFilter: VerbatimTransferScopes.filtersRows(draft.rowScope), note: draft.note, - keyChecks: statements.keyChecks + keyChecks: statements.keyChecks, + rollsBack: draft.rollsBack ) } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index f58632d2a6..02af992af2 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -643,12 +643,16 @@ internal struct ObjectCopyRunner { return result } try? await targetPlugin.rollbackTransaction() - /// The target is back where it started, rows and all, so nothing may be reported - /// as copied and no table may be reported as done. - result.outcomes = result.outcomes.filter { $0.error != nil }.map { - ObjectCopyObjectOutcome(selection: $0.selection, rowsCopied: 0, error: $0.error) + /// Nothing may be reported as copied that the rollback took back, and no such table as + /// done. A table the engine keeps through a ROLLBACK still holds its rows, and leaving + /// them out would send a retry over them. + let kept = Set(steps.filter { !$0.rollsBack }.map(\.id)) + result.outcomes = result.outcomes.compactMap { outcome in + if kept.contains(outcome.id) { return outcome } + guard let error = outcome.error else { return nil } + return ObjectCopyObjectOutcome(selection: outcome.selection, rowsCopied: 0, error: error) } - result.rowsCopied = 0 + result.rowsCopied = result.outcomes.filter { kept.contains($0.id) }.reduce(0) { $0 + $1.rowsCopied } return result } } diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift index 47b187f3fc..3fd3ed1724 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyTableDraft.swift @@ -43,6 +43,8 @@ internal struct ObjectCopyTableDraft { /// The `WHERE` and row limit the user set on this table, or nil for every row. let rowScope: PluginExportRowScope? let note: String? + /// False for a MySQL table whose storage engine keeps its rows through a ROLLBACK, such as MyISAM. + let rollsBack: Bool init( selection: ObjectCopySelection, @@ -82,6 +84,10 @@ internal struct ObjectCopyTableDraft { ) self.targetStructure = translation.snapshot self.isCrossEngine = translation.translated + self.rollsBack = !DataSyncTransactionality.cannotRollBack( + storageEngine: writesStructure ? translation.snapshot.engine : targetSnapshot?.engine, + databaseType: request.target.databaseType + ) /// Only a run that writes the structure has anything to report about it. Appending into a /// table the target already has changes no type, so a list of conversions would describe /// DDL that is not going to run. diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index 8fe6df2b94..b597ede675 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -184,7 +184,7 @@ struct ObjectCopyTransactionTests { } private func dataStep( - _ table: String, clears: Bool = false, keyChecks: [ObjectCopyKeyCheck] = [] + _ table: String, clears: Bool = false, keyChecks: [ObjectCopyKeyCheck] = [], rollsBack: Bool = true ) -> ObjectCopyTableStep { ObjectCopyTableStep( selection: ObjectCopySelection(kind: .table, name: table, schema: nil), @@ -203,7 +203,8 @@ struct ObjectCopyTransactionTests { copiesData: true, copiesIdentityColumn: false, note: nil, - keyChecks: keyChecks + keyChecks: keyChecks, + rollsBack: rollsBack ) } @@ -558,6 +559,24 @@ struct ObjectCopyTransactionTests { #expect(log.events.suffix(2) == ["target close relaxedForeignKeys", "target rollback"]) } + /// MySQL keeps a MyISAM table's rows through a ROLLBACK. + @Test("A table the target keeps through a rollback still reports its rows") + func tableThatCannotRollBackKeepsItsCount() async throws { + let log = CopyEventLog() + + let result = try await run( + dataPlan([dataStep("audit", rollsBack: false), dataStep("buildings")]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, failsInsertsAfter: 1) + ) + + #expect(log.events.last == "target rollback") + #expect(result.rowsCopied == 1) + #expect(result.outcomes.map { "\($0.selection.name) \($0.rowsCopied) \($0.error == nil)" } == [ + "audit 1 true", "buildings 0 false", + ]) + } + @Test( "Both paths lease the target before the source", arguments: [ImportErrorHandling.stopAndRollback, .stopAndCommit] From d2027a3163abd989421020189069d5af1c5f2784 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 28 Oct 2026 00:04:48 +0700 Subject: [PATCH 10/13] fix(copy): keep the checks on when the target account cannot run the key checks --- .../Core/ObjectCopy/ObjectCopyRunner.swift | 24 ++++++++++++++++++- .../ObjectCopyTransactionTests.swift | 23 ++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift index 02af992af2..7c758ee83d 100644 --- a/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift +++ b/apple/TablePro/Core/ObjectCopy/ObjectCopyRunner.swift @@ -607,7 +607,12 @@ internal struct ObjectCopyRunner { result.stopped = cleared.stopped if !result.stopped { - try await Self.withForeignKeysRelaxed(on: targetPlugin, when: usesTransaction && relaxesRows) { relaxed in + // Every table here was just created, so the checks cost nothing yet. One the account + // may not run, such as a SELECT on a write-only login, keeps the target's checks on. + let checkable = usesTransaction && relaxesRows + ? await Self.keyChecksRun(steps, on: targetPlugin) + : false + try await Self.withForeignKeysRelaxed(on: targetPlugin, when: checkable) { relaxed in let copied = await Self.stream( steps, from: sourceScope, @@ -799,6 +804,23 @@ internal struct ObjectCopyRunner { } } + nonisolated private static func keyChecksRun( + _ steps: [ObjectCopyTableStep], + on driver: any PluginDatabaseDriver + ) async -> Bool { + for check in steps.flatMap({ $0.keyChecks ?? [] }) { + do { + _ = try await driver.execute(query: check.sql) + } catch { + logger.warning( + "Copying with foreign key checks on, \(check.constraint, privacy: .public) cannot be checked: \(error.publicLogShape, privacy: .public)" + ) + return false + } + } + return true + } + /// Stops at the first key that fails, since the rollback that follows takes back every table. nonisolated private static func checkKeys( of steps: [ObjectCopyTableStep], diff --git a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift index b597ede675..ab8b8205a5 100644 --- a/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift +++ b/apple/TableProTests/Core/ObjectCopy/ObjectCopyTransactionTests.swift @@ -533,6 +533,7 @@ struct ObjectCopyTransactionTests { #expect(result.rowsCopied == 1) #expect(log.events == [ "target begin", + "target SELECT COUNT(*) FROM buildings", "target open relaxedForeignKeys", "source SELECT id FROM buildings", "target insert", @@ -542,6 +543,28 @@ struct ObjectCopyTransactionTests { ]) } + /// A write-only login copies parents first with the checks on, as it did before they could be relaxed. + @Test("A key the account cannot check keeps the target's checks on") + func uncheckableKeyKeepsTheChecks() async throws { + let log = CopyEventLog() + let check = ObjectCopyKeyCheck(constraint: "fk_estate", sql: "SELECT COUNT(*) FROM buildings") + + let result = try await run( + dataPlan([dataStep("buildings", keyChecks: [check])]), + source: RecordingCopyDriver("source", log: log), + target: RecordingCopyDriver("target", log: log, failing: [check.sql]) + ) + + #expect(result.rowsCopied == 1) + #expect(log.events == [ + "target begin", + "target SELECT COUNT(*) FROM buildings", + "source SELECT id FROM buildings", + "target insert", + "target commit", + ]) + } + /// A child read after its parent while the source was changing, or one the source already broke. @Test("A row a key refuses fails the copy and takes every table back") func brokenKeyRollsTheCopyBack() async throws { From c5366397e85262b87c0ca840e665f034ecaeb789 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 28 Oct 2026 00:15:08 +0700 Subject: [PATCH 11/13] fix(postgresql): put a setting saved inside a transaction back for that transaction only --- .../PostgreSQLPluginDriver+SessionScope.swift | 5 ++++- .../PostgreSQLSessionScopeSQL.swift | 6 +++--- .../PostgreSQLSessionScopeSQLTests.swift | 9 +++++++++ apple/changelog.d/3859.fixed.7.md | 1 + 4 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 apple/changelog.d/3859.fixed.7.md diff --git a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift index 73dd899b7d..a816628f70 100644 --- a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift +++ b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift @@ -96,10 +96,13 @@ internal extension PostgreSQLPluginDriver { let names = saved.map(\.name) let rows = try await core.executeSessionSetting(PostgreSQLSessionScopeSQL.captureStatement(names: names)) let current = PostgreSQLSessionScopeSQL.settings(fromRows: rows.map { $0.map(\.asText) }, names: names) + // Inside a transaction a saved value can be one SET LOCAL gave it, and set back session-wide it would + // outlive that transaction: a `replica` role kept skipping triggers and foreign keys after the commit. + let local = await core.sessionTransactionState() == .inTransaction var refused: [String] = [] for setting in PostgreSQLSessionScopeSQL.changedSettings(saved: saved, current: current) { - guard let statement = PostgreSQLSessionScopeSQL.setStatement([setting]) else { continue } + guard let statement = PostgreSQLSessionScopeSQL.setStatement([setting], local: local) else { continue } do { _ = try await core.executeSessionSetting(statement) } catch { diff --git a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift index 65d30ab1f2..5db9ced595 100644 --- a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift +++ b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift @@ -83,13 +83,13 @@ internal enum PostgreSQLSessionScopeSQL { /// One statement, so the server applies every setting or none: a refused one rolls back the statement's /// implicit transaction, or the savepoint the connection holds around it inside a block. A nil value binds SQL - /// `NULL`, which `set_config` takes as a reset. - static func setStatement(_ settings: [PluginSessionSetting]) -> PostgreSQLBoundStatement? { + /// `NULL`, which `set_config` takes as a reset. `local` holds the values only until the transaction ends. + static func setStatement(_ settings: [PluginSessionSetting], local: Bool = false) -> PostgreSQLBoundStatement? { guard !settings.isEmpty else { return nil } var calls: [String] = [] var parameters: [PluginCellValue] = [] for (index, setting) in settings.enumerated() { - calls.append("pg_catalog.set_config($\(index * 2 + 1), $\(index * 2 + 2), false)") + calls.append("pg_catalog.set_config($\(index * 2 + 1), $\(index * 2 + 2), \(local))") parameters.append(.text(setting.name)) parameters.append(setting.value.map { PluginCellValue.text($0) } ?? .null) } diff --git a/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift b/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift index b39c229f05..6a800449fc 100644 --- a/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift +++ b/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift @@ -102,6 +102,15 @@ struct PostgreSQLSessionScopeSQLTests { #expect(statement.parameters == [.text("extra_float_digits"), .null]) } + /// A value saved inside a transaction may be a `SET LOCAL` one, which must not outlive it. + @Test("Inside a transaction the values go back for that transaction only") + func localSetStatementEndsWithTheTransaction() throws { + let statement = try #require(PostgreSQLSessionScopeSQL.setStatement( + [Self.setting("session_replication_role", "replica")], local: true + )) + #expect(statement.sql == "SELECT pg_catalog.set_config($1, $2, true)") + } + @Test("No settings, no statement") func emptySetIsNil() { #expect(PostgreSQLSessionScopeSQL.setStatement([]) == nil) diff --git a/apple/changelog.d/3859.fixed.7.md b/apple/changelog.d/3859.fixed.7.md new file mode 100644 index 0000000000..c08cc7542d --- /dev/null +++ b/apple/changelog.d/3859.fixed.7.md @@ -0,0 +1 @@ +An SQL import inside an open PostgreSQL transaction could keep a SET LOCAL value after the transaction ended. From 5fcab4bfd520d579a87b0fe9182b1ec94b2527b9 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 28 Oct 2026 00:22:39 +0700 Subject: [PATCH 12/13] revert(postgresql): restore saved settings session-wide, as the session-level value is not readable under SET LOCAL --- .../PostgreSQLPluginDriver+SessionScope.swift | 5 +---- .../PostgreSQLSessionScopeSQL.swift | 6 +++--- .../PostgreSQLSessionScopeSQLTests.swift | 9 --------- apple/changelog.d/3859.fixed.7.md | 1 - 4 files changed, 4 insertions(+), 17 deletions(-) delete mode 100644 apple/changelog.d/3859.fixed.7.md diff --git a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift index a816628f70..73dd899b7d 100644 --- a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift +++ b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+SessionScope.swift @@ -96,13 +96,10 @@ internal extension PostgreSQLPluginDriver { let names = saved.map(\.name) let rows = try await core.executeSessionSetting(PostgreSQLSessionScopeSQL.captureStatement(names: names)) let current = PostgreSQLSessionScopeSQL.settings(fromRows: rows.map { $0.map(\.asText) }, names: names) - // Inside a transaction a saved value can be one SET LOCAL gave it, and set back session-wide it would - // outlive that transaction: a `replica` role kept skipping triggers and foreign keys after the commit. - let local = await core.sessionTransactionState() == .inTransaction var refused: [String] = [] for setting in PostgreSQLSessionScopeSQL.changedSettings(saved: saved, current: current) { - guard let statement = PostgreSQLSessionScopeSQL.setStatement([setting], local: local) else { continue } + guard let statement = PostgreSQLSessionScopeSQL.setStatement([setting]) else { continue } do { _ = try await core.executeSessionSetting(statement) } catch { diff --git a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift index 5db9ced595..65d30ab1f2 100644 --- a/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift +++ b/apple/Plugins/PostgreSQLDriverPlugin/PostgreSQLSessionScopeSQL.swift @@ -83,13 +83,13 @@ internal enum PostgreSQLSessionScopeSQL { /// One statement, so the server applies every setting or none: a refused one rolls back the statement's /// implicit transaction, or the savepoint the connection holds around it inside a block. A nil value binds SQL - /// `NULL`, which `set_config` takes as a reset. `local` holds the values only until the transaction ends. - static func setStatement(_ settings: [PluginSessionSetting], local: Bool = false) -> PostgreSQLBoundStatement? { + /// `NULL`, which `set_config` takes as a reset. + static func setStatement(_ settings: [PluginSessionSetting]) -> PostgreSQLBoundStatement? { guard !settings.isEmpty else { return nil } var calls: [String] = [] var parameters: [PluginCellValue] = [] for (index, setting) in settings.enumerated() { - calls.append("pg_catalog.set_config($\(index * 2 + 1), $\(index * 2 + 2), \(local))") + calls.append("pg_catalog.set_config($\(index * 2 + 1), $\(index * 2 + 2), false)") parameters.append(.text(setting.name)) parameters.append(setting.value.map { PluginCellValue.text($0) } ?? .null) } diff --git a/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift b/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift index 6a800449fc..b39c229f05 100644 --- a/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift +++ b/apple/Plugins/PostgreSQLDriverTests/PostgreSQLSessionScopeSQLTests.swift @@ -102,15 +102,6 @@ struct PostgreSQLSessionScopeSQLTests { #expect(statement.parameters == [.text("extra_float_digits"), .null]) } - /// A value saved inside a transaction may be a `SET LOCAL` one, which must not outlive it. - @Test("Inside a transaction the values go back for that transaction only") - func localSetStatementEndsWithTheTransaction() throws { - let statement = try #require(PostgreSQLSessionScopeSQL.setStatement( - [Self.setting("session_replication_role", "replica")], local: true - )) - #expect(statement.sql == "SELECT pg_catalog.set_config($1, $2, true)") - } - @Test("No settings, no statement") func emptySetIsNil() { #expect(PostgreSQLSessionScopeSQL.setStatement([]) == nil) diff --git a/apple/changelog.d/3859.fixed.7.md b/apple/changelog.d/3859.fixed.7.md deleted file mode 100644 index c08cc7542d..0000000000 --- a/apple/changelog.d/3859.fixed.7.md +++ /dev/null @@ -1 +0,0 @@ -An SQL import inside an open PostgreSQL transaction could keep a SET LOCAL value after the transaction ended. From 03b4ac9c58f50f72ff1f972c74b50e7898eb93a7 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Wed, 28 Oct 2026 00:25:39 +0700 Subject: [PATCH 13/13] fix(datawrite): save no session settings inside the user's transaction --- .../Core/DataWrite/DataWriteExecutor.swift | 7 ++++++- ...ginSessionTransactionState+Execution.swift | 12 +++++++++++ .../DataWrite/DataWriteExecutorTests.swift | 20 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift b/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift index 0232e0b93e..d0bf6b9918 100644 --- a/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift +++ b/apple/TablePro/Core/DataWrite/DataWriteExecutor.swift @@ -121,7 +121,12 @@ enum DataWriteExecutor { var sideStatements: [String] = [] // An epilogue sets a fixed value, so the scope is what gives SQLite back the foreign_keys it had. - let preserved = plan.prologue.isEmpty ? nil : await openPreservingScope(on: driver) + // Inside the user's transaction the pragma does nothing, and PostgreSQL would put a SET LOCAL value + // back session-wide, past the transaction: there the toggles run as they always did. + var preserved: PluginSessionScope? + if !plan.prologue.isEmpty, !(await driver.heldSessionTransactionState()).holdsUserTransaction { + preserved = await openPreservingScope(on: driver) + } func drainEpilogue() async { for statement in plan.epilogue { diff --git a/apple/TablePro/Core/Services/Execution/PluginSessionTransactionState+Execution.swift b/apple/TablePro/Core/Services/Execution/PluginSessionTransactionState+Execution.swift index cbc1b84be1..11512c99bd 100644 --- a/apple/TablePro/Core/Services/Execution/PluginSessionTransactionState+Execution.swift +++ b/apple/TablePro/Core/Services/Execution/PluginSessionTransactionState+Execution.swift @@ -37,6 +37,18 @@ internal extension PluginSessionTransactionState { } } + /// Whether the session is inside a transaction the user opened, aborted or not. + var holdsUserTransaction: Bool { + switch self { + case .inTransaction, .abortedTransaction: + return true + case .idle, .holdsSessionLocks, .unknown: + return false + @unknown default: + return false + } + } + /// What the user has to be told about the transaction this session is holding, or nil when it /// is holding none. /// diff --git a/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift b/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift index 9eade500d1..5234fead9b 100644 --- a/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift +++ b/apple/TableProTests/Core/DataWrite/DataWriteExecutorTests.swift @@ -256,6 +256,26 @@ struct DataWriteExecutorTests { #expect(Array(counting.trace.suffix(3)) == ["ROLLBACK", "PRAGMA foreign_keys = ON", "close preserve"]) } + /// PostgreSQL would put a `SET LOCAL` value back session-wide, where it outlives the user's transaction. + @Test( + "A save inside the user's transaction saves no session settings to put back", + arguments: [PluginSessionTransactionState.inTransaction, .abortedTransaction] + ) + func saveInsideTheUsersTransactionOpensNoScope(state: PluginSessionTransactionState) async throws { + let counting = CountingDriver(affectedRows: 1, sessionState: state) + _ = try? await DataWriteExecutor.run( + plan( + expectedRowCount: 1, + prologue: ["SET session_replication_role = replica"], + epilogue: ["SET session_replication_role = origin"] + ), + on: driver(counting) + ) + + #expect(!counting.trace.contains("open preserve")) + #expect(counting.trace.contains("SET session_replication_role = origin")) + } + @Test("A plan without a prologue leaves the session's settings alone") func planWithoutPrologueOpensNoScope() async throws { let counting = CountingDriver(affectedRows: 1)