diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cf3ee409..c289662c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Parse error on any MongoDB filter written in shell syntax, such as `db.orders.find({status: 1})`. - MongoDB `.sort()` and `.projection()` silently ignored when written with unquoted keys. +- Compare & Sync unable to drop an overloaded PostgreSQL routine, or any trigger. +- PostgreSQL sequence DDL naming the schema it was read from, in SQL export and the structure editor. ## [0.69.0] - 2026-08-27 diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift index 65e220ca9..15de11ad1 100644 --- a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift @@ -396,6 +396,19 @@ class PostgreSQLPluginDriver: LibPQBackedDriver, @unchecked Sendable { "DROP TRIGGER IF EXISTS \(quoteIdentifier(name)) ON \(qualifiedTable(table, schema: schema))" } + /// PostgreSQL allows `f(integer)` and `f(text)` in one schema, so a drop that names only `f` + /// is ambiguous and the server refuses it. + func generateDropRoutineSQL( + name: String, + signature: String?, + schema: String?, + isFunction: Bool + ) -> String? { + let keyword = isFunction ? "FUNCTION" : "PROCEDURE" + let arguments = (signature ?? "").trimmingCharacters(in: .whitespacesAndNewlines) + return "DROP \(keyword) IF EXISTS \(qualifiedTable(name, schema: schema))\(arguments)" + } + var providesBulkForeignKeyFetch: Bool { true } func fetchAllForeignKeys(schema: String?) async throws -> [String: [PluginForeignKeyInfo]] { @@ -757,13 +770,17 @@ class PostgreSQLPluginDriver: LibPQBackedDriver, @unchecked Sendable { let cycle = row[5].asText == "t" ? " CYCLE" : "" let lastValue = row.count > 6 ? row[6].asText : nil let quotedSeqName = quoteIdentifier(seqName) - let escapedSchemaForLiteral = escapeStringLiteral(schemaName) let escapedSeqForLiteral = escapeStringLiteral(seqName) var ddl = "CREATE SEQUENCE \(quotedSeqName) INCREMENT BY \(incrementBy)" + " MINVALUE \(minVal) MAXVALUE \(maxVal)" + " START WITH \(startVal)\(cycle);" + /// Unqualified, so it names the sequence the line above created rather than the one it + /// was read from. `setval` takes a `regclass`, which resolves through `search_path`, and + /// the `CREATE SEQUENCE` beside it is already schema-relative. Spelling the source's own + /// schema here made the pair disagree: run against another schema it repositioned the + /// original sequence, and against another database it named one that was not there. if let last = lastValue, !last.isEmpty, Int64(last) != nil { - ddl += "\nSELECT pg_catalog.setval('\"\(escapedSchemaForLiteral)\".\"\(escapedSeqForLiteral)\"', \(last), true);" + ddl += "\nSELECT pg_catalog.setval('\"\(escapedSeqForLiteral)\"', \(last), true);" } return (name: seqName, ddl: ddl) } diff --git a/Plugins/TableProPluginKit/PluginDatabaseDriver.swift b/Plugins/TableProPluginKit/PluginDatabaseDriver.swift index d1022ecab..21fabf430 100644 --- a/Plugins/TableProPluginKit/PluginDatabaseDriver.swift +++ b/Plugins/TableProPluginKit/PluginDatabaseDriver.swift @@ -241,6 +241,7 @@ public protocol PluginDatabaseDriver: AnyObject, Sendable { func createTriggerTemplate(table: String, schema: String?) -> String? func fetchTriggerDefinition(name: String, table: String, schema: String?) async throws -> String? func generateDropTriggerSQL(name: String, table: String, schema: String?) -> String? + func generateDropRoutineSQL(name: String, signature: String?, schema: String?, isFunction: Bool) -> String? var triggerEditUsesReplace: Bool { get } var supportsTransactionalDDL: Bool { get } @@ -315,6 +316,18 @@ public extension PluginDatabaseDriver { func createTriggerTemplate(table: String, schema: String?) -> String? { nil } func fetchTriggerDefinition(name: String, table: String, schema: String?) async throws -> String? { nil } func generateDropTriggerSQL(name: String, table: String, schema: String?) -> String? { nil } + + /// How this engine drops a routine, given that only some of them accept an argument list. + /// + /// PostgreSQL requires one to tell `f(integer)` from `f(text)`, and MySQL rejects one outright, + /// so a caller cannot spell this itself. Returning nil means the caller's own qualified + /// `DROP FUNCTION schema.name` is right for this engine. + func generateDropRoutineSQL( + name: String, + signature: String?, + schema: String?, + isFunction: Bool + ) -> String? { nil } var triggerEditUsesReplace: Bool { false } var supportsTransactionalDDL: Bool { false } diff --git a/TablePro/Core/Compare/SourceObjectSyncBuilder.swift b/TablePro/Core/Compare/SourceObjectSyncBuilder.swift index 6274f918d..90431703d 100644 --- a/TablePro/Core/Compare/SourceObjectSyncBuilder.swift +++ b/TablePro/Core/Compare/SourceObjectSyncBuilder.swift @@ -52,9 +52,9 @@ internal struct SourceObjectSyncBuilder { private func dropStatements(for result: CompareObjectResult, isReplacement: Bool) -> [SyncStatement] { guard let keyword = dropKeyword(for: result.identity.kind) else { return [] } - let name = qualified(result.identity) + let sql = dialectDrop(for: result.identity) ?? "DROP \(keyword) \(qualified(result.identity))" return [SyncStatement( - sql: "DROP \(keyword) \(name);", + sql: terminated(sql), objectName: result.identity.displayName, summary: isReplacement ? String( @@ -69,6 +69,29 @@ internal struct SourceObjectSyncBuilder { )] } + /// A routine and a trigger are not addressed by name alone on every engine. PostgreSQL needs an + /// overloaded routine's argument list and spells a trigger drop `DROP TRIGGER name ON table`, + /// while MySQL rejects the argument list and takes no `ON`. Only the driver knows which, so the + /// bare qualified name is the fallback rather than the rule. + private func dialectDrop(for identity: CompareObjectIdentity) -> String? { + switch identity.kind { + case .procedure, .function: + return targetDriver.generateDropRoutineSQL( + name: identity.name, + signature: identity.signature, + schema: identity.schema, + isFunction: identity.kind == .function + ) + case .trigger: + guard let table = identity.signature, !table.isEmpty else { return nil } + return targetDriver.generateDropTriggerSQL( + name: identity.name, table: table, schema: identity.schema + ) + case .view, .materializedView, .table, .sequence: + return nil + } + } + private func dropKeyword(for kind: CompareObjectKind) -> String? { switch kind { case .view: return "VIEW" diff --git a/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift b/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift index f0e634e7c..eb73adb92 100644 --- a/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift +++ b/TablePro/Core/ObjectCopy/ObjectCopyPlan.swift @@ -17,6 +17,12 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { internal let selection: ObjectCopySelection /// Runs first, and only when the user chose to replace a table the target already has. internal let dropStatements: [SyncStatement] + /// The sequences this table's own defaults name, created before it. + /// + /// A PostgreSQL `SERIAL` column's default is `nextval('seq'::regclass)`. Copied without its + /// sequence, the table either refuses to be created or arrives with a default pointing at + /// nothing, and the first insert fails. + internal let sequenceStatements: [SyncStatement] internal let createStatements: [SyncStatement] /// Empties a table the copy is about to append to, for a data-only replace where there is no /// DROP and CREATE to clear it. @@ -45,7 +51,7 @@ internal struct ObjectCopyTableStep: Identifiable, Sendable { /// 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 + createStatements } + internal var ddl: [SyncStatement] { dropStatements + sequenceStatements + createStatements } internal var qualifiedTargetName: String { guard let targetSchema, !targetSchema.isEmpty else { return targetTable } @@ -146,7 +152,7 @@ internal struct ObjectCopyPlan: Sendable { } internal var creationGroups: [ObjectCopyStatementGroup] { - tableSteps.map { ObjectCopyStatementGroup($0.selection, $0.createStatements) } + tableSteps.map { ObjectCopyStatementGroup($0.selection, $0.sequenceStatements + $0.createStatements) } + definitionSteps.filter { !$0.runsAfterData } .map { ObjectCopyStatementGroup($0.selection, $0.createStatements) } } diff --git a/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index 1518955aa..7312b1d10 100644 --- a/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -286,6 +286,9 @@ internal struct ObjectCopyPlanner { return ObjectCopyTableStep( selection: draft.selection, dropStatements: statements.drop, + sequenceStatements: Self.sequenceStatements( + parts?.sequences ?? [], table: draft.targetTable + ), createStatements: statements.create, truncateStatements: statements.truncate, columns: draft.targetColumns, @@ -304,6 +307,7 @@ internal struct ObjectCopyPlanner { private struct SourceParts: Sendable { let query: String let estimatedRows: Int? + let sequences: [String] } /// One scoped call for every table, because each `withMetadataDriver` either leases a pooled @@ -315,32 +319,78 @@ internal struct ObjectCopyPlanner { /// The source's own spellings, which are not always the target's: a case-insensitive /// match can pair `Orders.UserID` with `orders.userid`, and quoting the source's spelling /// into the target's INSERT names a column that engine does not have. - /// Only the tables whose rows are actually copied. Teradata implements the row estimate as - /// `SELECT COUNT(*)`, so preparing every draft made reviewing a structure-only copy scan - /// every table it named. - let inputs = drafts.filter(\.copiesData).map { - (id: $0.selection.id, table: $0.snapshot.name, schema: $0.sourceSchema, columns: $0.sourceColumns) + let inputs = drafts.map { + ( + id: $0.selection.id, + table: $0.snapshot.name, + schema: $0.sourceSchema, + columns: $0.sourceColumns, + copiesData: $0.copiesData, + writesStructure: $0.writesStructure + ) } - guard !inputs.isEmpty else { return [:] } + guard inputs.contains(where: { $0.copiesData || $0.writesStructure }) else { return [:] } return try await manager.withMetadataDriver(scope: endpoint.scope, workload: .bulk) { driver in guard let plugin = CompareMetadataService.pluginDriver(from: driver) else { throw ObjectCopyError.refused(Self.noSourceDriver) } var parts: [String: SourceParts] = [:] + /// A sequence several tables default from is created once, under the first of them, + /// which the dependency order has already put ahead of the rest. + var claimedSequences: Set = [] for input in inputs { try Task.checkCancellation() - let query = ObjectCopySelectQuery.build( - columns: input.columns, table: input.table, schema: input.schema, driver: plugin - ) - let estimate = try? await plugin.fetchApproximateRowCount( - table: input.table, schema: input.schema + var query = "" + var estimatedRows: Int? + /// Only the tables whose rows are actually copied. Teradata implements the row + /// estimate as `SELECT COUNT(*)`, so preparing every draft made reviewing a + /// structure-only copy scan every table it named. + if input.copiesData { + query = ObjectCopySelectQuery.build( + columns: input.columns, table: input.table, schema: input.schema, driver: plugin + ) + estimatedRows = (try? await plugin.fetchApproximateRowCount( + table: input.table, schema: input.schema + )) ?? nil + } + var sequences: [String] = [] + if input.writesStructure { + let found = (try? await plugin.fetchDependentSequences( + table: input.table, schema: input.schema + )) ?? [] + for sequence in found + where claimedSequences.insert(sequence.name.lowercased()).inserted { + sequences += SQLStatementScanner.allStatements(in: sequence.ddl) + } + } + parts[input.id] = SourceParts( + query: query, estimatedRows: estimatedRows, sequences: sequences ) - parts[input.id] = SourceParts(query: query, estimatedRows: estimate ?? nil) } return parts } } + /// A copied table's default names its sequence, so the sequence has to be there before the + /// `CREATE TABLE` runs. + /// + /// PostgreSQL renders a `SERIAL` column as `integer ... DEFAULT nextval('orders_id_seq')`, and + /// the driver keeps that text verbatim. Copied without the sequence it names, the table either + /// fails to be created at all or, where the source's own sequence happens to be reachable, is + /// created sharing it, so the copy and the original hand out the same keys. + nonisolated internal static func sequenceStatements( + _ sql: [String], + table: String + ) -> [SyncStatement] { + sql.map { statement in + SyncStatement( + sql: statement.hasSuffix(";") ? statement : statement + ";", + objectName: table, + summary: String(format: String(localized: "Create the sequences %@ defaults from"), table) + ) + } + } + private func buildTargetDDL( _ drafts: [ObjectCopyTableDraft], request: ObjectCopyRequest, @@ -583,7 +633,10 @@ internal struct ObjectCopyPlanner { kind: target.kind, schema: targetSchema ?? target.schema, name: target.name, - signature: target.signature + /// A trigger's owning table travels in the same slot a routine's + /// argument list does, which is the shape `triggerReads` already uses, + /// and is what lets PostgreSQL spell `DROP TRIGGER name ON table`. + signature: target.signature ?? target.owner ), status: .onlyInTarget ) diff --git a/TableProTests/Core/Compare/SourceObjectSyncBuilderTests.swift b/TableProTests/Core/Compare/SourceObjectSyncBuilderTests.swift new file mode 100644 index 000000000..2a6d41d68 --- /dev/null +++ b/TableProTests/Core/Compare/SourceObjectSyncBuilderTests.swift @@ -0,0 +1,148 @@ +// +// SourceObjectSyncBuilderTests.swift +// TableProTests +// +// A routine and a trigger are not addressed by name alone on every engine, so +// the drop the builder writes has to come from the target driver rather than +// from a keyword and a qualified name. +// + +@testable import TablePro +import TableProPluginKit +import XCTest + +/// Shared through a refining protocol rather than a base class on purpose. A conformance is +/// witnessed where it is declared, so a subclass method cannot take over a requirement its +/// superclass already satisfied from the protocol's own default, and both stubs would answer nil. +private protocol DropStubDriver: PluginDatabaseDriver {} + +private extension DropStubDriver { + func connect() async throws {} + func disconnect() {} + func execute(query: String) async throws -> PluginQueryResult { + PluginQueryResult(columns: [], columnTypeNames: [], rows: [], rowsAffected: 0, executionTime: 0) + } + func quoteIdentifier(_ name: String) -> String { "\"\(name)\"" } + func fetchTables(schema: String?) async throws -> [PluginTableInfo] { [] } + func fetchColumns(table: String, schema: String?) async throws -> [PluginColumnInfo] { [] } + func fetchIndexes(table: String, schema: String?) async throws -> [PluginIndexInfo] { [] } + func fetchForeignKeys(table: String, schema: String?) async throws -> [PluginForeignKeyInfo] { [] } + func fetchTableDDL(table: String, schema: String?) async throws -> String { "" } + func fetchViewDefinition(view: String, schema: String?) async throws -> String { "" } + func fetchTableMetadata(table: String, schema: String?) async throws -> PluginTableMetadata { + PluginTableMetadata(tableName: table) + } + func fetchDatabases() async throws -> [String] { [] } + func fetchDatabaseMetadata(_ database: String) async throws -> PluginDatabaseMetadata { + PluginDatabaseMetadata(name: database) + } + + func qualified(_ name: String, _ schema: String?) -> String { + guard let schema, !schema.isEmpty else { return quoteIdentifier(name) } + return "\(quoteIdentifier(schema)).\(quoteIdentifier(name))" + } +} + +/// Spells both drops its own way, the way PostgreSQL does. +private final class DialectDropDriver: DropStubDriver, @unchecked Sendable { + func generateDropRoutineSQL( + name: String, + signature: String?, + schema: String?, + isFunction: Bool + ) -> String? { + let keyword = isFunction ? "FUNCTION" : "PROCEDURE" + return "DROP \(keyword) IF EXISTS \(qualified(name, schema))\(signature ?? "")" + } + + func generateDropTriggerSQL(name: String, table: String, schema: String?) -> String? { + "DROP TRIGGER IF EXISTS \(quoteIdentifier(name)) ON \(qualified(table, schema))" + } +} + +/// Takes neither an argument list nor an `ON`, the way MySQL does, and so inherits both defaults. +private final class PlainDropDriver: DropStubDriver, @unchecked Sendable {} + +final class SourceObjectSyncBuilderTests: XCTestCase { + private func drop( + _ identity: CompareObjectIdentity, + driver: any PluginDatabaseDriver + ) -> String? { + SourceObjectSyncBuilder(targetDriver: driver) + .build(for: CompareObjectResult(identity: identity, status: .onlyInTarget), action: .drop) + .first?.sql + } + + /// Two overloads are two routines, and a drop that names only `f` is refused as ambiguous. + func testARoutineDropCarriesItsArgumentListWhereTheEngineNeedsOne() { + XCTAssertEqual( + drop( + CompareObjectIdentity( + kind: .function, schema: "public", name: "total", signature: "(integer)" + ), + driver: DialectDropDriver() + ), + "DROP FUNCTION IF EXISTS \"public\".\"total\"(integer);" + ) + } + + func testAProcedureDropUsesTheProcedureKeyword() { + XCTAssertEqual( + drop( + CompareObjectIdentity( + kind: .procedure, schema: "public", name: "rebuild", signature: "()" + ), + driver: DialectDropDriver() + ), + "DROP PROCEDURE IF EXISTS \"public\".\"rebuild\"();" + ) + } + + /// The owning table travels in the signature slot, which is what lets the driver write the `ON`. + func testATriggerDropNamesTheTableThatOwnsIt() { + XCTAssertEqual( + drop( + CompareObjectIdentity( + kind: .trigger, schema: "public", name: "audit", signature: "orders" + ), + driver: DialectDropDriver() + ), + "DROP TRIGGER IF EXISTS \"audit\" ON \"public\".\"orders\";" + ) + } + + /// Nothing to hang the `ON` off, so the bare qualified name is all that can be written. + func testATriggerWithNoOwnerFallsBackToTheQualifiedName() { + XCTAssertEqual( + drop( + CompareObjectIdentity(kind: .trigger, schema: "public", name: "audit"), + driver: DialectDropDriver() + ), + "DROP TRIGGER \"public\".\"audit\";" + ) + } + + /// An engine that rejects the argument list keeps the plain drop it has always had. + func testAnEngineWithoutADialectDropKeepsTheQualifiedName() { + XCTAssertEqual( + drop( + CompareObjectIdentity( + kind: .function, schema: "shop", name: "total", signature: "(integer)" + ), + driver: PlainDropDriver() + ), + "DROP FUNCTION \"shop\".\"total\";" + ) + } + + /// A view is addressed by name on every engine, so it must not be routed through either hook. + func testAViewDropIsUnchanged() { + XCTAssertEqual( + drop( + CompareObjectIdentity(kind: .view, schema: "public", name: "recent"), + driver: DialectDropDriver() + ), + "DROP VIEW \"public\".\"recent\";" + ) + } +} diff --git a/TableProTests/Core/ObjectCopy/ObjectCopyRowCopierTests.swift b/TableProTests/Core/ObjectCopy/ObjectCopyRowCopierTests.swift index cf239a448..fc9cae702 100644 --- a/TableProTests/Core/ObjectCopy/ObjectCopyRowCopierTests.swift +++ b/TableProTests/Core/ObjectCopy/ObjectCopyRowCopierTests.swift @@ -80,6 +80,7 @@ final class ObjectCopyRowCopierTests: XCTestCase { ObjectCopyTableStep( selection: ObjectCopySelection(kind: .table, name: "orders", schema: schema), dropStatements: [], + sequenceStatements: [], createStatements: [], truncateStatements: [], columns: columns, diff --git a/TableProTests/Core/ObjectCopy/ObjectCopySequenceStatementTests.swift b/TableProTests/Core/ObjectCopy/ObjectCopySequenceStatementTests.swift new file mode 100644 index 000000000..4c76d84c0 --- /dev/null +++ b/TableProTests/Core/ObjectCopy/ObjectCopySequenceStatementTests.swift @@ -0,0 +1,77 @@ +// +// ObjectCopySequenceStatementTests.swift +// TableProTests +// +// A copied table's default names its sequence, so the sequence has to exist +// before the CREATE TABLE that references it runs. +// + +@testable import TablePro +import XCTest + +final class ObjectCopySequenceStatementTests: XCTestCase { + /// The driver returns the create and the reposition as one block, and the runner sends one + /// statement per `execute`, so the block has to arrive already split. + func testAMultiStatementSequenceDDLBecomesOneStatementEach() { + let statements = ObjectCopyPlanner.sequenceStatements( + [ + "CREATE SEQUENCE \"orders_id_seq\" INCREMENT BY 1 MINVALUE 1 MAXVALUE 100 START WITH 1", + "SELECT pg_catalog.setval('\"orders_id_seq\"', 42, true)" + ], + table: "orders" + ) + XCTAssertEqual(statements.map(\.sql), [ + "CREATE SEQUENCE \"orders_id_seq\" INCREMENT BY 1 MINVALUE 1 MAXVALUE 100 START WITH 1;", + "SELECT pg_catalog.setval('\"orders_id_seq\"', 42, true);" + ]) + } + + func testAlreadyTerminatedSQLIsNotTerminatedTwice() { + XCTAssertEqual( + ObjectCopyPlanner.sequenceStatements(["CREATE SEQUENCE \"s\";"], table: "orders") + .map(\.sql), + ["CREATE SEQUENCE \"s\";"] + ) + } + + /// Named after the table, because the progress list and the outcome report are both grouped by + /// the object the user selected. + func testTheStatementIsAttributedToTheTableThatNeedsIt() { + let statement = ObjectCopyPlanner.sequenceStatements( + ["CREATE SEQUENCE \"s\""], table: "orders" + ).first + XCTAssertEqual(statement?.objectName, "orders") + } + + func testATableWithNoSequencesProducesNothing() { + XCTAssertTrue(ObjectCopyPlanner.sequenceStatements([], table: "orders").isEmpty) + } + + /// Ahead of the CREATE TABLE whose default references them, and behind the DROP that a + /// replacement runs first. + func testSequencesRunBeforeTheTableTheyBelongTo() { + let step = ObjectCopyTableStep( + selection: ObjectCopySelection(kind: .table, name: "orders", schema: "public"), + dropStatements: [SyncStatement(sql: "DROP TABLE \"orders\";", objectName: "orders", summary: "")], + sequenceStatements: ObjectCopyPlanner.sequenceStatements( + ["CREATE SEQUENCE \"orders_id_seq\""], table: "orders" + ), + createStatements: [SyncStatement(sql: "CREATE TABLE \"orders\";", objectName: "orders", summary: "")], + truncateStatements: [], + columns: ["id"], + primaryKeyColumns: ["id"], + sourceQuery: "SELECT \"id\" FROM \"public\".\"orders\"", + targetTable: "orders", + targetSchema: "public", + estimatedRows: nil, + copiesData: false, + copiesIdentityColumn: false, + note: nil + ) + XCTAssertEqual(step.ddl.map(\.sql), [ + "DROP TABLE \"orders\";", + "CREATE SEQUENCE \"orders_id_seq\";", + "CREATE TABLE \"orders\";" + ]) + } +}