diff --git a/CHANGELOG.md b/CHANGELOG.md index 28702cdb89..31f5f990f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Version history for saved queries, with **Restore This Version**. (#2505) - Git status letters, history and **Discard Changes…** for files in a linked SQL folder. (#2505) - Whether a materialized view can be refreshed concurrently, on its **Indexes** tab. (#2522) +- Invalid PostgreSQL indexes named on the table's **Indexes** tab. ### Changed @@ -370,6 +371,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Destination folder and the first database reading as one path in the backup result sheet. (#3046) - Only the last line of a failed backup's error shown, which on `pg_dump` is the hint rather than the cause. - Backup failure reported as an exit code alone when the tool wrote its message and exited at once. +- PostgreSQL export and column reorder script failing on an index a failed `CREATE INDEX CONCURRENTLY` left behind. +- PostgreSQL export and column reorder script failing on a foreign key that references a unique index. +- PostgreSQL exclusion constraints missing from exports and the DDL tab. +- PostgreSQL column reorder script dropping an index named like one of the table's check constraints. +- Invalid PostgreSQL index recreated on the target by Compare & Sync and **Copy To**. - Indent and Outdent named the wrong way round for Command-[ and Command-] in Settings > Keyboard. - Table, routine or type missing from the sidebar or Open Quickly when a period in its quoted name matched another's. - Show Previous Tab and Show Next Tab listed twice in the Window menu. diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLIndexQueries.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLIndexQueries.swift index 65a4c73624..dab879b987 100644 --- a/Plugins/PostgreSQLDriverPlugin/PostgreSQLIndexQueries.swift +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLIndexQueries.swift @@ -10,6 +10,47 @@ import TableProPluginKit nonisolated enum PostgreSQLIndexQueries { private static let logger = Logger(subsystem: "com.TablePro.PostgreSQLDriver", category: "IndexQueries") + static let restorableIndexPredicate = "(ix.indisvalid OR t.relkind = 'p') AND ix.indisready" + + static func standaloneIndexQuery(schema: String, table: String) -> String { + let schemaLiteral = PostgreSQLObjectQueries.quoteLiteral(schema) + let tableLiteral = PostgreSQLObjectQueries.quoteLiteral(table) + return """ + SELECT + i.relname AS index_name, + pg_catalog.pg_get_indexdef(ix.indexrelid) AS definition, + (\(restorableIndexPredicate)) AS is_restorable + FROM pg_catalog.pg_index ix + JOIN pg_catalog.pg_class i ON i.oid = ix.indexrelid + JOIN pg_catalog.pg_class t ON t.oid = ix.indrelid + JOIN pg_catalog.pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = \(schemaLiteral) + AND t.relname = \(tableLiteral) + AND NOT EXISTS ( + SELECT 1 FROM pg_catalog.pg_constraint con + WHERE con.conrelid = ix.indrelid + AND con.conindid = ix.indexrelid + AND con.contype IN ('p', 'u', 'x') + ) + ORDER BY i.relname + """ + } + + static func standaloneIndexes(rows: [[PluginCellValue]]) -> PostgreSQLStandaloneIndexes { + var definitions: [String] = [] + var invalidNames: [String] = [] + for row in rows { + guard let name = row[safe: 0]?.asText, + let definition = row[safe: 1]?.asText?.nilIfEmpty else { continue } + if PostgreSQLCatalogBoolean.isTrue(row[safe: 2]?.asText) { + definitions.append(definition) + } else { + invalidNames.append(name) + } + } + return PostgreSQLStandaloneIndexes(definitions: definitions, invalidNames: invalidNames) + } + /// One row per index, with its key parts in key order. /// /// A key part is read by position rather than by joining `pg_attribute` on `indkey`. An expression @@ -56,7 +97,8 @@ nonisolated enum PostgreSQLIndexQueries { JOIN pg_catalog.pg_attribute a ON a.attrelid = ix.indrelid AND a.attnum = ix.indkey[k.n - 1] ORDER BY k.n - )::text AS included_columns + )::text AS included_columns, + (\(restorableIndexPredicate)) AS is_valid FROM pg_catalog.pg_index ix JOIN pg_catalog.pg_class i ON i.oid = ix.indexrelid JOIN pg_catalog.pg_class t ON t.oid = ix.indrelid @@ -157,6 +199,11 @@ nonisolated struct PostgreSQLCatalogIndexDDL: Equatable { let whereClause: String? } +nonisolated struct PostgreSQLStandaloneIndexes: Equatable { + let definitions: [String] + let invalidNames: [String] +} + nonisolated enum PostgreSQLIndexRow { static func index( from row: [PluginCellValue], @@ -176,7 +223,8 @@ nonisolated enum PostgreSQLIndexRow { expressions: nonEmptyValues(row[safe: 7]?.asText), includedColumns: nonEmptyValues(row[safe: 8]?.asText), ddlMethodAndKeys: spelling?.methodAndKeys, - ddlWhereClause: spelling?.whereClause + ddlWhereClause: spelling?.whereClause, + isValid: row[safe: 9]?.asText.map { PostgreSQLCatalogBoolean.isTrue($0) } ) return (table, index) } diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+ColumnReorder.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+ColumnReorder.swift index 8ae22ad6bc..564b7a93c6 100644 --- a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+ColumnReorder.swift +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver+ColumnReorder.swift @@ -28,163 +28,28 @@ extension PostgreSQLPluginDriver { Set(parts.columnNames) == Set(desiredOrder), parts.columnNames.count == desiredOrder.count else { return nil } - let qualified = "\(quoteIdentifier(resolvedSchema)).\(quoteIdentifier(table))" - let staging = "\(quoteIdentifier(resolvedSchema)).\(quoteIdentifier("\(table)_tablepro_reorder"))" - let copyList = parts.copyableColumns.map { quoteIdentifier($0) }.joined(separator: ", ") - - let body = desiredOrder.compactMap { parts.columnDefinitions[$0] } - - /// The order here is the whole difficulty, and every step of it was measured against - /// PostgreSQL 17. The old table is renamed rather than dropped, so a foreign key in another - /// table keeps pointing at real rows while the copy runs. But a rename moves nothing else: - /// the staging table still owns every index name and every constraint name the original - /// had, and both live in the schema rather than on the table. Declaring the constraints - /// inside the `CREATE TABLE` therefore silently renames them, which shipped as `x_pkey1`, - /// `x_a_b_key1` and `x_c_check1`; creating an index before the staging table goes fails - /// outright with "relation already exists". So nothing that carries a name is created until - /// the staging table is dropped, and the staging table cannot be dropped until every - /// inbound foreign key has let go of it. - var statements: [String] = [] - statements.append("ALTER TABLE \(qualified) RENAME TO \(quoteIdentifier("\(table)_tablepro_reorder"))") - statements.append("CREATE TABLE \(qualified) (\n " + body.joined(separator: ",\n ") + "\n)") - statements.append(PostgreSQLVersionedStatements.copyRows( - into: qualified, - from: staging, - columnList: copyList, - capabilities: versionedCapabilities - )) - statements.append(contentsOf: parts.identityResets(qualified: qualified, quote: quoteIdentifier)) - statements.append(contentsOf: parts.inboundForeignKeyDrops) - /// A `serial` column's default still calls the sequence the staging table owns, so `DROP - /// TABLE` tries to take that sequence with it and PostgreSQL refuses, rolling the whole - /// script back. Measured: handing ownership to the rebuilt table first lets the drop - /// through, and the sequence keeps its original name. - statements.append(contentsOf: parts.serialSequenceHandovers) - statements.append("DROP TABLE \(staging)") - statements.append(contentsOf: parts.tableConstraints.map { "ALTER TABLE \(qualified) ADD \($0)" }) - statements.append(contentsOf: parts.outboundForeignKeys.map { "ALTER TABLE \(qualified) ADD \($0)" }) - statements.append(contentsOf: parts.inboundForeignKeyAdds) - statements.append(contentsOf: parts.indexes) - statements.append(contentsOf: parts.triggers) - statements.append(contentsOf: parts.triggerModes) - statements.append(contentsOf: parts.comments) - return PluginColumnReorderPlan( - statements: statements, + statements: parts.statements( + table: table, + schema: resolvedSchema, + desiredOrder: desiredOrder, + quote: quoteIdentifier, + capabilities: versionedCapabilities + ), isTransactional: true, cost: .tableRebuild, - caveats: parts.dependentViewCaveat + [ - String(localized: "Grants, row-level security policies, publications, extended statistics, partitioning and table inheritance are not carried over."), - String(localized: "An identity column keeps its value, but its sequence is recreated under a new name because the old table still holds the original name when the new one is created.") - ], + caveats: parts.caveats, isRunnable: false ) } - private struct RebuildParts { - var columnNames: [String] = [] - var columnDefinitions: [String: String] = [:] - var copyableColumns: [String] = [] - var identityColumns: [String] = [] - var tableConstraints: [String] = [] - var outboundForeignKeys: [String] = [] - var inboundForeignKeyDrops: [String] = [] - var inboundForeignKeyAdds: [String] = [] - var indexes: [String] = [] - var triggers: [String] = [] - var triggerModes: [String] = [] - var comments: [String] = [] - var dependentViews: [String] = [] - var serialSequenceHandovers: [String] = [] - - /// PostgreSQL binds a view to the table's OID, not its name, so a view follows the rename - /// onto the staging table and then refuses to let it be dropped. Measured: the rebuild - /// stops at `DROP TABLE` with "cannot drop table … because other objects depend on it" and - /// the whole transaction rolls back. Naming them here is what stops that being discovered - /// three quarters of the way through the script. - var dependentViewCaveat: [String] { - guard !dependentViews.isEmpty else { return [] } - return [ - String( - format: String( - localized: "Drop and recreate these views first, or the script stops when it drops the old table: %@." - ), - dependentViews.joined(separator: ", ") - ) - ] - } - - /// A new identity column starts its sequence at one, so it is wound forward to the rows the - /// copy just wrote. Without this the next insert collides with an existing key. - /// - /// `qualified` is already a quoted identifier pair, and `pg_get_serial_sequence` takes the - /// whole pair as one literal, so it is composed first and quoted once. A schema, table or - /// column name may legally contain an apostrophe or a backslash, and both land inside a - /// literal here. - func identityResets(qualified: String, quote: (String) -> String) -> [String] { - let relationLiteral = PostgreSQLObjectQueries.quoteLiteral(qualified) - return identityColumns.map { column in - let columnLiteral = PostgreSQLObjectQueries.quoteLiteral(column) - return """ - SELECT setval( - pg_get_serial_sequence(\(relationLiteral), \(columnLiteral)), - GREATEST(COALESCE((SELECT MAX(\(quote(column))) FROM \(qualified)), 0), 1), - true - ) - """ - } - } - } - - private func fetchRebuildParts(table: String, schema: String) async throws -> RebuildParts { + private func fetchRebuildParts(table: String, schema: String) async throws -> PostgreSQLTableRebuild { let tableLiteral = PostgreSQLObjectQueries.quoteLiteral(table) let schemaLiteral = PostgreSQLObjectQueries.quoteLiteral(schema) let caps = versionedCapabilities - var parts = RebuildParts() + var parts = PostgreSQLTableRebuild() - let identityClause = caps.hasIdentityColumns ? """ - CASE - WHEN a.attidentity = 'a' THEN ' GENERATED ALWAYS AS IDENTITY' - WHEN a.attidentity = 'd' THEN ' GENERATED BY DEFAULT AS IDENTITY' - ELSE '' - END || - """ : "" - let generatedClause = caps.hasGeneratedColumns ? """ - CASE - WHEN a.attgenerated = 's' THEN ' GENERATED ALWAYS AS (' || pg_get_expr(d.adbin, d.adrelid) || ') STORED' - WHEN a.attgenerated = 'v' THEN ' GENERATED ALWAYS AS (' || pg_get_expr(d.adbin, d.adrelid) || ') VIRTUAL' - ELSE '' - END || - """ : "" - let defaultGuard = [ - caps.hasIdentityColumns ? "AND a.attidentity = ''" : "", - caps.hasGeneratedColumns ? "AND a.attgenerated = ''" : "" - ].filter { !$0.isEmpty }.joined(separator: " ") - let identityFlag = caps.hasIdentityColumns ? "a.attidentity <> ''" : "false" - let generatedFlag = caps.hasGeneratedColumns ? "a.attgenerated <> ''" : "false" - - let columnRows = try await execute(query: """ - SELECT - a.attname, - quote_ident(a.attname) || ' ' || format_type(a.atttypid, a.atttypmod) || \(PostgreSQLSchemaQueries.columnCollateClause) || - \(identityClause) - \(generatedClause) - CASE WHEN a.attnotnull THEN ' NOT NULL' ELSE '' END || - CASE - WHEN a.atthasdef \(defaultGuard) - THEN ' DEFAULT ' || pg_get_expr(d.adbin, d.adrelid) - ELSE '' - END, - \(identityFlag), - \(generatedFlag) - FROM pg_attribute a - JOIN pg_class c ON c.oid = a.attrelid - JOIN pg_namespace n ON n.oid = c.relnamespace - LEFT JOIN pg_attrdef d ON d.adrelid = c.oid AND d.adnum = a.attnum - WHERE c.relname = \(tableLiteral) AND n.nspname = \(schemaLiteral) - AND a.attnum > 0 AND NOT a.attisdropped - ORDER BY a.attnum - """).rows + let columnRows = try await rebuildColumnRows(tableLiteral: tableLiteral, schemaLiteral: schemaLiteral) for row in columnRows { guard let name = row[safe: 0]?.asText, let definition = row[safe: 1]?.asText else { continue } @@ -242,17 +107,9 @@ extension PostgreSQLPluginDriver { /// The indexes a constraint owns come back with the constraint, so listing them again would /// fail on a duplicate name. - parts.indexes = try await textRows(""" - SELECT indexdef FROM pg_indexes - WHERE tablename = \(tableLiteral) AND schemaname = \(schemaLiteral) - AND indexname NOT IN ( - SELECT con.conname FROM pg_constraint con - JOIN pg_class c ON c.oid = con.conrelid - JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE c.relname = \(tableLiteral) AND n.nspname = \(schemaLiteral) - ) - ORDER BY indexname - """) + let standaloneIndexes = try await fetchStandaloneIndexes(table: table, schema: schema) + parts.indexes = standaloneIndexes.definitions + parts.invalidIndexes = standaloneIndexes.invalidNames parts.triggers = try await textRows(""" SELECT pg_get_triggerdef(t.oid, true) @@ -340,6 +197,53 @@ extension PostgreSQLPluginDriver { return parts } + private func rebuildColumnRows(tableLiteral: String, schemaLiteral: String) async throws -> [[PluginCellValue]] { + let caps = versionedCapabilities + let identityClause = caps.hasIdentityColumns ? """ + CASE + WHEN a.attidentity = 'a' THEN ' GENERATED ALWAYS AS IDENTITY' + WHEN a.attidentity = 'd' THEN ' GENERATED BY DEFAULT AS IDENTITY' + ELSE '' + END || + """ : "" + let generatedClause = caps.hasGeneratedColumns ? """ + CASE + WHEN a.attgenerated = 's' THEN ' GENERATED ALWAYS AS (' || pg_get_expr(d.adbin, d.adrelid) || ') STORED' + WHEN a.attgenerated = 'v' THEN ' GENERATED ALWAYS AS (' || pg_get_expr(d.adbin, d.adrelid) || ') VIRTUAL' + ELSE '' + END || + """ : "" + let defaultGuard = [ + caps.hasIdentityColumns ? "AND a.attidentity = ''" : "", + caps.hasGeneratedColumns ? "AND a.attgenerated = ''" : "" + ].filter { !$0.isEmpty }.joined(separator: " ") + let identityFlag = caps.hasIdentityColumns ? "a.attidentity <> ''" : "false" + let generatedFlag = caps.hasGeneratedColumns ? "a.attgenerated <> ''" : "false" + + return try await execute(query: """ + SELECT + a.attname, + quote_ident(a.attname) || ' ' || format_type(a.atttypid, a.atttypmod) || \(PostgreSQLSchemaQueries.columnCollateClause) || + \(identityClause) + \(generatedClause) + CASE WHEN a.attnotnull THEN ' NOT NULL' ELSE '' END || + CASE + WHEN a.atthasdef \(defaultGuard) + THEN ' DEFAULT ' || pg_get_expr(d.adbin, d.adrelid) + ELSE '' + END, + \(identityFlag), + \(generatedFlag) + FROM pg_attribute a + JOIN pg_class c ON c.oid = a.attrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + LEFT JOIN pg_attrdef d ON d.adrelid = c.oid AND d.adnum = a.attnum + WHERE c.relname = \(tableLiteral) AND n.nspname = \(schemaLiteral) + AND a.attnum > 0 AND NOT a.attisdropped + ORDER BY a.attnum + """).rows + } + private func textRows(_ query: String) async throws -> [String] { try await execute(query: query).rows.compactMap { $0[safe: 0]?.asText } } diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift index b8ebf698af..ba23f0c77e 100644 --- a/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLPluginDriver.swift @@ -408,18 +408,9 @@ class PostgreSQLPluginDriver: LibPQBackedDriver, @unchecked Sendable { ORDER BY a.attnum """ - let constraintsQuery = """ - SELECT - pg_get_constraintdef(con.oid, true) - FROM pg_constraint con - JOIN pg_class c ON c.oid = con.conrelid - JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE c.relname = \(tableLiteral) - AND n.nspname = \(schemaLiteral) - AND con.contype IN ('p', 'u', 'c') - ORDER BY - CASE con.contype WHEN 'p' THEN 0 WHEN 'u' THEN 1 WHEN 'c' THEN 2 END - """ + let constraintsQuery = PostgreSQLSchemaQueries.tableDDLConstraintsQuery( + schema: resolvedSchema, table: table + ) async let columnsResult = execute(query: columnsQuery) async let constraintsResult = execute(query: constraintsQuery) @@ -453,27 +444,19 @@ class PostgreSQLPluginDriver: LibPQBackedDriver, @unchecked Sendable { /// operator class, an `INCLUDE` list, a storage parameter, a partial predicate and a per-column /// sort direction verbatim. It also qualifies the table whatever `search_path` holds, so a dump /// spanning two schemas attaches each index to the right one. - /// - /// An index backing a constraint is excluded by `conindid` rather than by matching its name - /// against `conname`, which is how `pg_dump` does it: the names agree for a unique or primary - /// key constraint, but a CHECK constraint that happens to share an index's name would drop that - /// index from the dump. func fetchIndexDDL(table: String, schema: String?) async throws -> [String] { - let query = """ - SELECT pg_get_indexdef(ix.indexrelid) - FROM pg_index ix - JOIN pg_class c ON c.oid = ix.indrelid - JOIN pg_class i ON i.oid = ix.indexrelid - JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE c.relname = \(PostgreSQLObjectQueries.quoteLiteral(table)) - AND n.nspname = \(PostgreSQLObjectQueries.quoteLiteral(schema ?? core.currentSchema)) - AND NOT EXISTS ( - SELECT 1 FROM pg_constraint con WHERE con.conindid = ix.indexrelid - ) - ORDER BY i.relname - """ - let result = try await execute(query: query) - return result.rows.compactMap { $0[0].asText } + try await fetchStandaloneIndexes(table: table, schema: schema ?? core.currentSchema).definitions + } + + func fetchStandaloneIndexes(table: String, schema: String) async throws -> PostgreSQLStandaloneIndexes { + let query = PostgreSQLIndexQueries.standaloneIndexQuery(schema: schema, table: table) + let indexes = PostgreSQLIndexQueries.standaloneIndexes(rows: try await execute(query: query).rows) + if !indexes.invalidNames.isEmpty { + Self.logger.info( + "Left out \(indexes.invalidNames.count) invalid index(es) on \(table, privacy: .private(mask: .hash))" + ) + } + return indexes } func fetchTableMetadata(table: String, schema: String?) async throws -> PluginTableMetadata { diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLSchemaQueries.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLSchemaQueries.swift index 32ccb2453d..f5bc8758b9 100644 --- a/Plugins/PostgreSQLDriverPlugin/PostgreSQLSchemaQueries.swift +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLSchemaQueries.swift @@ -332,6 +332,23 @@ enum PostgreSQLSchemaQueries { """ } + static func tableDDLConstraintsQuery(schema: String, table: String) -> String { + let schemaLiteral = PostgreSQLObjectQueries.quoteLiteral(schema) + let tableLiteral = PostgreSQLObjectQueries.quoteLiteral(table) + return """ + SELECT + pg_get_constraintdef(con.oid, true) + FROM pg_constraint con + JOIN pg_class c ON c.oid = con.conrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE c.relname = \(tableLiteral) + AND n.nspname = \(schemaLiteral) + AND con.contype IN ('p', 'u', 'c', 'x') + ORDER BY + CASE con.contype WHEN 'p' THEN 0 WHEN 'u' THEN 1 WHEN 'c' THEN 2 ELSE 3 END + """ + } + /// Column introspection for one schema, read under `schemaRelativeReadPrefix(schema:)`. /// /// `declared_type` is what the column shows and `data_type` is what the app classifies by, which diff --git a/Plugins/PostgreSQLDriverPlugin/PostgreSQLTableRebuild.swift b/Plugins/PostgreSQLDriverPlugin/PostgreSQLTableRebuild.swift new file mode 100644 index 0000000000..b170f93f0a --- /dev/null +++ b/Plugins/PostgreSQLDriverPlugin/PostgreSQLTableRebuild.swift @@ -0,0 +1,135 @@ +// +// PostgreSQLTableRebuild.swift +// PostgreSQLDriverPlugin +// + +import Foundation + +struct PostgreSQLTableRebuild { + var columnNames: [String] = [] + var columnDefinitions: [String: String] = [:] + var copyableColumns: [String] = [] + var identityColumns: [String] = [] + var tableConstraints: [String] = [] + var outboundForeignKeys: [String] = [] + var inboundForeignKeyDrops: [String] = [] + var inboundForeignKeyAdds: [String] = [] + var indexes: [String] = [] + var invalidIndexes: [String] = [] + var triggers: [String] = [] + var triggerModes: [String] = [] + var comments: [String] = [] + var dependentViews: [String] = [] + var serialSequenceHandovers: [String] = [] + + /// The order here is the whole difficulty, and every step of it was measured against + /// PostgreSQL 17. The old table is renamed rather than dropped, so a foreign key in another + /// table keeps pointing at real rows while the copy runs. But a rename moves nothing else: + /// the staging table still owns every index name and every constraint name the original + /// had, and both live in the schema rather than on the table. Declaring the constraints + /// inside the `CREATE TABLE` therefore silently renames them, which shipped as `x_pkey1`, + /// `x_a_b_key1` and `x_c_check1`; creating an index before the staging table goes fails + /// outright with "relation already exists". So nothing that carries a name is created until + /// the staging table is dropped, and the staging table cannot be dropped until every + /// inbound foreign key has let go of it. + func statements( + table: String, + schema: String, + desiredOrder: [String], + quote: (String) -> String, + capabilities: PostgreSQLCapabilities + ) -> [String] { + let stagingName = "\(table)_tablepro_reorder" + let qualified = "\(quote(schema)).\(quote(table))" + let staging = "\(quote(schema)).\(quote(stagingName))" + let copyList = copyableColumns.map(quote).joined(separator: ", ") + let body = desiredOrder.compactMap { columnDefinitions[$0] } + + var statements: [String] = [] + statements.append("ALTER TABLE \(qualified) RENAME TO \(quote(stagingName))") + statements.append("CREATE TABLE \(qualified) (\n " + body.joined(separator: ",\n ") + "\n)") + statements.append(PostgreSQLVersionedStatements.copyRows( + into: qualified, + from: staging, + columnList: copyList, + capabilities: capabilities + )) + statements.append(contentsOf: identityResets(qualified: qualified, quote: quote)) + statements.append(contentsOf: inboundForeignKeyDrops) + /// A `serial` column's default still calls the sequence the staging table owns, so `DROP + /// TABLE` tries to take that sequence with it and PostgreSQL refuses, rolling the whole + /// script back. Measured: handing ownership to the rebuilt table first lets the drop + /// through, and the sequence keeps its original name. + statements.append(contentsOf: serialSequenceHandovers) + statements.append("DROP TABLE \(staging)") + statements.append(contentsOf: tableConstraints.map { "ALTER TABLE \(qualified) ADD \($0)" }) + statements.append(contentsOf: indexes) + statements.append(contentsOf: outboundForeignKeys.map { "ALTER TABLE \(qualified) ADD \($0)" }) + statements.append(contentsOf: inboundForeignKeyAdds) + statements.append(contentsOf: triggers) + statements.append(contentsOf: triggerModes) + statements.append(contentsOf: comments) + return statements + } + + var caveats: [String] { + dependentViewCaveat + invalidIndexCaveat + [ + String(localized: """ + Grants, row-level security policies, publications, extended statistics, partitioning and \ + table inheritance are not carried over. + """), + String(localized: """ + An identity column keeps its value, but its sequence is recreated under a new name because \ + the old table still holds the original name when the new one is created. + """) + ] + } + + /// PostgreSQL binds a view to the table's OID, not its name, so a view follows the rename + /// onto the staging table and then refuses to let it be dropped. Measured: the rebuild + /// stops at `DROP TABLE` with "cannot drop table … because other objects depend on it" and + /// the whole transaction rolls back. Naming them here is what stops that being discovered + /// three quarters of the way through the script. + private var dependentViewCaveat: [String] { + guard !dependentViews.isEmpty else { return [] } + return [ + String( + format: String( + localized: "Drop and recreate these views first, or the script stops when it drops the old table: %@." + ), + dependentViews.joined(separator: ", ") + ) + ] + } + + private var invalidIndexCaveat: [String] { + guard !invalidIndexes.isEmpty else { return [] } + return [ + String( + format: String(localized: "Invalid indexes are not recreated: %@."), + invalidIndexes.joined(separator: ", ") + ) + ] + } + + /// A new identity column starts its sequence at one, so it is wound forward to the rows the + /// copy just wrote. Without this the next insert collides with an existing key. + /// + /// `qualified` is already a quoted identifier pair, and `pg_get_serial_sequence` takes the + /// whole pair as one literal, so it is composed first and quoted once. A schema, table or + /// column name may legally contain an apostrophe or a backslash, and both land inside a + /// literal here. + private func identityResets(qualified: String, quote: (String) -> String) -> [String] { + let relationLiteral = PostgreSQLObjectQueries.quoteLiteral(qualified) + return identityColumns.map { column in + let columnLiteral = PostgreSQLObjectQueries.quoteLiteral(column) + return """ + SELECT setval( + pg_get_serial_sequence(\(relationLiteral), \(columnLiteral)), + GREATEST(COALESCE((SELECT MAX(\(quote(column))) FROM \(qualified)), 0), 1), + true + ) + """ + } + } +} diff --git a/Plugins/SQLExportPlugin/SQLExportPlugin.swift b/Plugins/SQLExportPlugin/SQLExportPlugin.swift index a4151b27c8..5b2b74f342 100644 --- a/Plugins/SQLExportPlugin/SQLExportPlugin.swift +++ b/Plugins/SQLExportPlugin/SQLExportPlugin.swift @@ -766,7 +766,8 @@ final class SQLExportPlugin: ObservableObject, ExportFormatPlugin, SettablePlugi to writer: SQLExportFileWriter, progress: PluginExportProgress ) async throws { - var emittedAnything = false + var emittedAnything = try await writeIndexPhase( + objects: sortedTables, dataSource: dataSource, to: writer, progress: progress) /// A driver that hands back the server's own CREATE statement has already declared these /// constraints inline, so adding them again names each one twice: MySQL and SQL Server /// reject the duplicate, and SQLite has no ADD CONSTRAINT to reject it with. The phase @@ -783,11 +784,6 @@ final class SQLExportPlugin: ObservableObject, ExportFormatPlugin, SettablePlugi } } - if try await writeIndexPhase( - objects: sortedTables, dataSource: dataSource, to: writer, progress: progress) { - emittedAnything = true - } - /// `setval` and `pg_get_serial_sequence` are PostgreSQL's own, so the sequence is only /// rewound on PostgreSQL. Every other engine reports its identity columns the same way and /// would take the statement as a syntax error. @@ -808,7 +804,7 @@ final class SQLExportPlugin: ObservableObject, ExportFormatPlugin, SettablePlugi } } - /// Writes each object's `CREATE INDEX` statements, after its rows and after the deferred + /// Writes each object's `CREATE INDEX` statements, after its rows and before the deferred /// foreign keys. /// /// That is where every engine's own dump tool puts them, and the reason is that a bulk load diff --git a/Plugins/TableProPluginKit/PluginIndexInfo.swift b/Plugins/TableProPluginKit/PluginIndexInfo.swift index 40da416732..96dfc6083a 100644 --- a/Plugins/TableProPluginKit/PluginIndexInfo.swift +++ b/Plugins/TableProPluginKit/PluginIndexInfo.swift @@ -33,6 +33,7 @@ public struct PluginIndexInfo: Codable, Sendable { /// `whereClause` as a `CREATE INDEX` on another schema has to write it, or nil to write /// `whereClause`. public let ddlWhereClause: String? + public let isValid: Bool? /// The signature published before key expressions, `INCLUDE` columns and the DDL spellings /// existed. Kept byte-identical and disfavoured so plugins built against an older PluginKit keep @@ -58,8 +59,10 @@ public struct PluginIndexInfo: Codable, Sendable { self.includedColumns = nil self.ddlMethodAndKeys = nil self.ddlWhereClause = nil + self.isValid = nil } + @_disfavoredOverload public init( name: String, columns: [String], @@ -84,5 +87,34 @@ public struct PluginIndexInfo: Codable, Sendable { self.includedColumns = includedColumns self.ddlMethodAndKeys = ddlMethodAndKeys self.ddlWhereClause = ddlWhereClause + self.isValid = nil + } + + public init( + name: String, + columns: [String], + isUnique: Bool = false, + isPrimary: Bool = false, + type: String = "BTREE", + columnPrefixes: [String: Int]? = nil, + whereClause: String? = nil, + expressions: [String]?, + includedColumns: [String]?, + ddlMethodAndKeys: String?, + ddlWhereClause: String?, + isValid: Bool? + ) { + self.name = name + self.columns = columns + self.isUnique = isUnique + self.isPrimary = isPrimary + self.type = type + self.columnPrefixes = columnPrefixes + self.whereClause = whereClause + self.expressions = expressions + self.includedColumns = includedColumns + self.ddlMethodAndKeys = ddlMethodAndKeys + self.ddlWhereClause = ddlWhereClause + self.isValid = isValid } } diff --git a/TablePro/Core/Compare/CompareMetadataService.swift b/TablePro/Core/Compare/CompareMetadataService.swift index 4b8c72228b..01ffd129f8 100644 --- a/TablePro/Core/Compare/CompareMetadataService.swift +++ b/TablePro/Core/Compare/CompareMetadataService.swift @@ -32,6 +32,14 @@ internal struct TableStructureRead: Sendable { internal let failure: String? internal var snapshot: TableStructureSnapshot? { + snapshot(indexes: indexes) + } + + internal var sourceSnapshot: TableStructureSnapshot? { + snapshot(indexes: indexes.filter { $0.isValid != false }) + } + + private func snapshot(indexes: [PluginIndexInfo]) -> TableStructureSnapshot? { guard failure == nil else { return nil } return TableStructureSnapshot.from( table: table, columns: columns, indexes: indexes, foreignKeys: foreignKeys, metadata: metadata diff --git a/TablePro/Core/Compare/CompareRunner+Data.swift b/TablePro/Core/Compare/CompareRunner+Data.swift index dd594ab43e..1aaeb5a3b3 100644 --- a/TablePro/Core/Compare/CompareRunner+Data.swift +++ b/TablePro/Core/Compare/CompareRunner+Data.swift @@ -179,7 +179,7 @@ internal extension CompareRunner { session.dataPlans.map { ($0.id, $0) }, uniquingKeysWith: { first, _ in first } ) let sourceSnapshots = Dictionary( - sourceReads.compactMap { $0.snapshot }.map { ($0.qualifiedName, $0) }, + sourceReads.compactMap { $0.sourceSnapshot }.map { ($0.qualifiedName, $0) }, uniquingKeysWith: { first, _ in first } ) diff --git a/TablePro/Core/Compare/CompareRunner.swift b/TablePro/Core/Compare/CompareRunner.swift index 384e6ff8b0..b028ca407a 100644 --- a/TablePro/Core/Compare/CompareRunner.swift +++ b/TablePro/Core/Compare/CompareRunner.swift @@ -299,7 +299,7 @@ internal struct CompareRunner { let targetTables = targetReads.filter { CompareTableKindClassifier.kind(of: $0.table) == .table } let sourceSnapshots = sourceTables.compactMap { - $0.snapshot?.droppingCatalogSpellings(ownSchema: context.source.schema) + $0.sourceSnapshot?.droppingCatalogSpellings(ownSchema: context.source.schema) } let targetSnapshots = targetTables.compactMap { $0.snapshot?.droppingCatalogSpellings(ownSchema: context.target.schema) diff --git a/TablePro/Core/MCP/MCPConnectionBridge+Schema.swift b/TablePro/Core/MCP/MCPConnectionBridge+Schema.swift index ed14adac40..6e8349ca2d 100644 --- a/TablePro/Core/MCP/MCPConnectionBridge+Schema.swift +++ b/TablePro/Core/MCP/MCPConnectionBridge+Schema.swift @@ -470,6 +470,7 @@ extension MCPConnectionBridge { "columns": .array(index.columns.map { .string($0) }), "is_unique": .bool(index.isUnique), "is_primary": .bool(index.isPrimary), + "is_valid": .bool(index.isValid), "type": .string(index.type) ] if let whereClause = index.whereClause, !whereClause.isEmpty { diff --git a/TablePro/Core/MCP/Protocol/Tools/MCPToolSchema.swift b/TablePro/Core/MCP/Protocol/Tools/MCPToolSchema.swift index 75fef39b23..9aa43dc52a 100644 --- a/TablePro/Core/MCP/Protocol/Tools/MCPToolSchema.swift +++ b/TablePro/Core/MCP/Protocol/Tools/MCPToolSchema.swift @@ -125,10 +125,11 @@ enum MCPToolSchema { "columns": array(String(localized: "Indexed columns in order"), of: stringItem), "is_unique": boolean(String(localized: "Whether the index enforces uniqueness")), "is_primary": boolean(String(localized: "Whether the index backs the primary key")), + "is_valid": boolean(String(localized: "False for an index the engine reports as invalid, which queries skip and exports leave out")), "type": string(String(localized: "Index type reported by the engine")), "where_clause": string(String(localized: "Partial index predicate")) ], - required: ["name", "columns", "is_unique", "is_primary", "type"], + required: ["name", "columns", "is_unique", "is_primary", "is_valid", "type"], allowsAdditional: true ) diff --git a/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift b/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift index 2370856f10..5d6afd98db 100644 --- a/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift +++ b/TablePro/Core/ObjectCopy/ObjectCopyPlanner.swift @@ -281,7 +281,7 @@ internal struct ObjectCopyPlanner { skipped.append(ObjectCopySkip(selection: selection, reason: Self.missingInSource)) continue } - guard read.snapshot != nil else { + guard read.sourceSnapshot != nil else { skipped.append(ObjectCopySkip(selection: selection, reason: read.failure ?? Self.unreadable)) continue } @@ -303,7 +303,7 @@ internal struct ObjectCopyPlanner { for selection in Self.orderedByDependency( scope.objects.filter { reads[$0] != nil }, reads: reads, effectiveSchema: sourceNamespace ) { - guard let read = reads[selection], let snapshot = read.snapshot else { continue } + guard let read = reads[selection], let snapshot = read.sourceSnapshot else { continue } let targetRead = match(selection, in: targetReads) let existsInTarget = targetRead != nil if existsInTarget, request.existingPolicy == .skip { diff --git a/TablePro/Core/Plugins/PluginManager.swift b/TablePro/Core/Plugins/PluginManager.swift index 25cf521337..5cffbe0e61 100644 --- a/TablePro/Core/Plugins/PluginManager.swift +++ b/TablePro/Core/Plugins/PluginManager.swift @@ -83,6 +83,9 @@ final class PluginManager: ObservableObject { /// the session already has open so nothing the app owns wraps a transaction the user opened. /// Both have defaults (nil and `.unknown`), so an already-built plugin keeps loading and /// answers them; the minimum stays where it is and no bulk re-release is needed. + /// + /// 33 also adds `isValid` to `PluginIndexInfo`, through an added initializer with the previous + /// full one disfavoured; nil means the driver does not report it. nonisolated static let currentPluginKitVersion = 33 /// Still 19, so every plugin already published for the previous release keeps loading. diff --git a/TablePro/Core/Plugins/PluginObjectMapping.swift b/TablePro/Core/Plugins/PluginObjectMapping.swift index c63586b328..3160f62264 100644 --- a/TablePro/Core/Plugins/PluginObjectMapping.swift +++ b/TablePro/Core/Plugins/PluginObjectMapping.swift @@ -48,7 +48,8 @@ extension IndexInfo { expressions: index.expressions, includedColumns: index.includedColumns, ddlMethodAndKeys: index.ddlMethodAndKeys, - ddlWhereClause: index.ddlWhereClause + ddlWhereClause: index.ddlWhereClause, + isValid: index.isValid ?? true ) } } diff --git a/TablePro/Models/Database/InvalidIndexNote.swift b/TablePro/Models/Database/InvalidIndexNote.swift new file mode 100644 index 0000000000..e2ab67fc48 --- /dev/null +++ b/TablePro/Models/Database/InvalidIndexNote.swift @@ -0,0 +1,21 @@ +// +// InvalidIndexNote.swift +// TablePro +// + +import Foundation + +internal struct InvalidIndexNote: Equatable { + internal let systemImage = "exclamationmark.triangle" + internal let text: String + + internal init?(indexes: [IndexInfo]) { + let names = indexes.filter { !$0.isValid }.map(\.name) + guard !names.isEmpty else { return nil } + let list = ListFormatter.localizedString(byJoining: names) + let format = names.count == 1 + ? String(localized: "%@ is invalid, so queries skip it and exports leave it out. Drop it, or rebuild it with REINDEX.") + : String(localized: "%@ are invalid, so queries skip them and exports leave them out. Drop them, or rebuild them with REINDEX.") + text = String(format: format, list) + } +} diff --git a/TablePro/Models/Query/QueryResult.swift b/TablePro/Models/Query/QueryResult.swift index ab9ce11782..3c6c38b016 100644 --- a/TablePro/Models/Query/QueryResult.swift +++ b/TablePro/Models/Query/QueryResult.swift @@ -348,6 +348,7 @@ struct IndexInfo: Identifiable, Hashable { /// `PluginIndexInfo.ddlMethodAndKeys` says why they differ from the fields. let ddlMethodAndKeys: String? let ddlWhereClause: String? + let isValid: Bool init( name: String, @@ -360,7 +361,8 @@ struct IndexInfo: Identifiable, Hashable { expressions: [String]? = nil, includedColumns: [String]? = nil, ddlMethodAndKeys: String? = nil, - ddlWhereClause: String? = nil + ddlWhereClause: String? = nil, + isValid: Bool = true ) { self.name = name self.columns = columns @@ -373,6 +375,7 @@ struct IndexInfo: Identifiable, Hashable { self.includedColumns = includedColumns self.ddlMethodAndKeys = ddlMethodAndKeys self.ddlWhereClause = ddlWhereClause + self.isValid = isValid } } diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index 375a17b054..ffce3b31ec 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -6356,12 +6356,18 @@ } } } + }, + "%@ are invalid, so queries skip them and exports leave them out. Drop them, or rebuild them with REINDEX." : { + }, "%@ cannot be scripted, because its definition is not a statement that recreates it." : { }, "%@ does not allow NULL, so its default cannot be NULL" : { + }, + "%@ is invalid, so queries skip it and exports leave it out. Drop it, or rebuild it with REINDEX." : { + }, "%d added" : { "localizations" : { @@ -62566,6 +62572,9 @@ } } } + }, + "False for an index the engine reports as invalid, which queries skip and exports leave out" : { + }, "expand" : { "extractionState" : "stale", @@ -75321,6 +75330,9 @@ } } } + }, + "Invalid indexes are not recreated: %@." : { + }, "hostname:%lld" : { "extractionState" : "stale", diff --git a/TablePro/Views/Structure/ConcurrentRefreshNoteView.swift b/TablePro/Views/Structure/StructureTabNoteView.swift similarity index 60% rename from TablePro/Views/Structure/ConcurrentRefreshNoteView.swift rename to TablePro/Views/Structure/StructureTabNoteView.swift index fa4c32cc28..e39455c4bd 100644 --- a/TablePro/Views/Structure/ConcurrentRefreshNoteView.swift +++ b/TablePro/Views/Structure/StructureTabNoteView.swift @@ -1,24 +1,26 @@ // -// ConcurrentRefreshNoteView.swift +// StructureTabNoteView.swift // TablePro // import SwiftUI -struct ConcurrentRefreshNoteView: View { - let note: MaterializedViewConcurrentRefreshNote +struct StructureTabNoteView: View { + let systemImage: String + let text: String + let identifier: String var body: some View { VStack(spacing: 0) { Divider() - Label(note.text, systemImage: note.systemImage) + Label(text, systemImage: systemImage) .font(.callout) .foregroundStyle(.secondary) .fixedSize(horizontal: false, vertical: true) .frame(maxWidth: .infinity, alignment: .leading) .padding(.horizontal, 8) .padding(.vertical, 6) - .accessibilityIdentifier("structure-concurrent-refresh-note") + .accessibilityIdentifier(identifier) } } } diff --git a/TablePro/Views/Structure/TableStructureView.swift b/TablePro/Views/Structure/TableStructureView.swift index 406b2b8e99..90f4cebdd8 100644 --- a/TablePro/Views/Structure/TableStructureView.swift +++ b/TablePro/Views/Structure/TableStructureView.swift @@ -367,10 +367,7 @@ struct TableStructureView: View { } } .safeAreaInset(edge: .bottom, spacing: 0) { - if objectKind == .materializedView, - let note = MaterializedViewConcurrentRefreshNote(state: session.concurrentRefresh) { - ConcurrentRefreshNoteView(note: note) - } + indexesTabNotes } case .foreignKeys: if shouldShowForeignKeysEmptyState { @@ -406,6 +403,26 @@ struct TableStructureView: View { } } + private var indexesTabNotes: some View { + VStack(spacing: 0) { + if let note = InvalidIndexNote(indexes: indexes) { + StructureTabNoteView( + systemImage: note.systemImage, + text: note.text, + identifier: "structure-invalid-index-note" + ) + } + if objectKind == .materializedView, + let note = MaterializedViewConcurrentRefreshNote(state: session.concurrentRefresh) { + StructureTabNoteView( + systemImage: note.systemImage, + text: note.text, + identifier: "structure-concurrent-refresh-note" + ) + } + } + } + /// Only offered where the add behind it can actually run. An engine that lists an object but /// cannot edit it shows the grid, so its real rows stay visible instead of being replaced by an /// empty state whose only affordance is disabled, and a view whose kind refuses the add never diff --git a/TableProTests/Core/Compare/TableStructureSnapshotIndexValidityTests.swift b/TableProTests/Core/Compare/TableStructureSnapshotIndexValidityTests.swift new file mode 100644 index 0000000000..f13686ecdf --- /dev/null +++ b/TableProTests/Core/Compare/TableStructureSnapshotIndexValidityTests.swift @@ -0,0 +1,135 @@ +// +// TableStructureSnapshotIndexValidityTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import TableProPluginKit +import Testing + +@Suite("Invalid indexes in Compare and Object Copy") +struct TableStructureSnapshotIndexValidityTests { + private static let table = PluginTableInfo(name: "orders", type: "TABLE", schema: "public", comment: nil) + + private static func index(_ name: String, column: String = "code", valid: Bool?) -> PluginIndexInfo { + PluginIndexInfo( + name: name, columns: [column], isUnique: true, expressions: nil, includedColumns: nil, + ddlMethodAndKeys: nil, ddlWhereClause: nil, isValid: valid + ) + } + + private static func read(_ indexes: [PluginIndexInfo]) -> TableStructureRead { + TableStructureRead( + table: table, + columns: [ + PluginColumnInfo(name: "code", dataType: "integer"), + PluginColumnInfo(name: "day", dataType: "date") + ], + indexes: indexes, + foreignKeys: [], + metadata: nil, + failure: nil + ) + } + + private static func addedIndexes(_ changes: [SchemaChange]) -> [String] { + changes.compactMap { change in + guard case .addIndex(let index) = change else { return nil } + return index.name + } + } + + private static func deletedIndexes(_ changes: [SchemaChange]) -> [String] { + changes.compactMap { change in + guard case .deleteIndex(let index) = change else { return nil } + return index.name + } + } + + @Test("A source read leaves out an invalid index and keeps every other one") + func sourceSnapshotDropsInvalidIndexes() throws { + let read = Self.read([ + Self.index("orders_code_key", valid: false), + Self.index("orders_day_key", column: "day", valid: true), + Self.index("orders_legacy", column: "day", valid: nil) + ]) + let source = try #require(read.sourceSnapshot) + let whole = try #require(read.snapshot) + let sourceNames = source.indexes.map(\.name) + let wholeNames = whole.indexes.map(\.name) + #expect(sourceNames == ["orders_day_key", "orders_legacy"]) + #expect(wholeNames == ["orders_code_key", "orders_day_key", "orders_legacy"]) + } + + @Test("An invalid source index is not added to the target and not rendered in its definition") + func invalidSourceIndexIsNotSynced() throws { + let source = try #require(Self.read([Self.index("orders_code_key", valid: false)]).sourceSnapshot) + let target = try #require(Self.read([]).snapshot) + + let result = StructureDiffEngine().compareTable(source: source, target: target) + + let definition = TableDefinitionRenderer.lines(for: source) + #expect(Self.addedIndexes(result.changes).isEmpty) + #expect(!definition.contains { $0.contains("orders_code_key") }) + } + + @Test("A target's invalid twin of a source index is left alone") + func targetInvalidTwinIsNotReAdded() throws { + let source = try #require(Self.read([Self.index("orders_code_key", valid: true)]).sourceSnapshot) + let target = try #require(Self.read([Self.index("orders_code_key", valid: false)]).snapshot) + + let result = StructureDiffEngine().compareTable(source: source, target: target) + + #expect(Self.addedIndexes(result.changes).isEmpty) + #expect(Self.deletedIndexes(result.changes).isEmpty) + } + + @Test("A target's invalid index the source does not have is still offered for dropping") + func orphanTargetInvalidIndexIsDropped() throws { + let source = try #require(Self.read([]).sourceSnapshot) + let target = try #require(Self.read([Self.index("orders_code_key", valid: false)]).snapshot) + + let result = StructureDiffEngine().compareTable(source: source, target: target) + + #expect(Self.deletedIndexes(result.changes) == ["orders_code_key"]) + } + + @Test("Object Copy creates no invalid index on the target") + func objectCopyLeavesInvalidIndexOut() throws { + let read = Self.read([ + Self.index("orders_code_key", valid: false), + Self.index("orders_day_key", column: "day", valid: true) + ]) + let snapshot = try #require(read.sourceSnapshot) + let draft = ObjectCopyTableDraft( + selection: ObjectCopySelection(kind: .table, name: "orders", schema: "public"), + read: read, + snapshot: snapshot, + targetSnapshot: nil, + existsInTarget: false, + sourceSchema: "public", + targetSchema: "archive", + targetServerVersion: nil, + request: ObjectCopyRequest( + source: Self.endpoint(), + destination: .existing(Self.endpoint()), + objects: [], + content: .structure, + existingPolicy: .skip + ) + ) + let targetIndexes = draft.targetStructure.indexes.map(\.name) + #expect(targetIndexes == ["orders_day_key"]) + } + + private static func endpoint() -> DatabaseEndpoint { + DatabaseEndpoint( + scope: DatabaseScope(connectionId: UUID(), database: "shop", schema: "public"), + connectionName: "PostgreSQL", + databaseType: .postgresql, + safeModeLevel: .silent, + color: .blue + ) + } +} diff --git a/TableProTests/Core/MCP/Protocol/Tools/MCPIndexEncodingTests.swift b/TableProTests/Core/MCP/Protocol/Tools/MCPIndexEncodingTests.swift index abf350bab0..4c75e244fc 100644 --- a/TableProTests/Core/MCP/Protocol/Tools/MCPIndexEncodingTests.swift +++ b/TableProTests/Core/MCP/Protocol/Tools/MCPIndexEncodingTests.swift @@ -36,4 +36,24 @@ struct MCPIndexEncodingTests { #expect(encoded["included_columns"] == nil) #expect(encoded["columns"]?.arrayValue?.compactMap(\.stringValue) == ["email"]) } + + @Test("An invalid index says so, and every other index reads as valid") + func validityIsEncoded() { + let invalid = MCPConnectionBridge.encode(index: IndexInfo( + name: "users_email_key", columns: ["email"], isUnique: true, isPrimary: false, type: "BTREE", + isValid: false + )) + let valid = MCPConnectionBridge.encode(index: IndexInfo( + name: "users_email", columns: ["email"], isUnique: false, isPrimary: false, type: "BTREE" + )) + #expect(invalid["is_valid"] == .bool(false)) + #expect(valid["is_valid"] == .bool(true)) + } + + @Test("The published index schema declares is_valid as a required boolean") + func schemaDeclaresValidity() { + let properties = MCPToolSchema.indexDefinition["properties"] + #expect(properties?["is_valid"]?["type"] == .string("boolean")) + #expect(MCPToolSchema.indexDefinition["required"]?.arrayValue?.contains(.string("is_valid")) == true) + } } diff --git a/TableProTests/Core/Plugins/PluginIndexMappingCoverageTests.swift b/TableProTests/Core/Plugins/PluginIndexMappingCoverageTests.swift index dc9a26d75c..d397277b1a 100644 --- a/TableProTests/Core/Plugins/PluginIndexMappingCoverageTests.swift +++ b/TableProTests/Core/Plugins/PluginIndexMappingCoverageTests.swift @@ -37,4 +37,19 @@ struct PluginIndexMappingCoverageTests { #expect(problems.contains { $0.hasPrefix("ddlMethodAndKeys:") }) #expect(problems.contains { $0.hasPrefix("ddlWhereClause:") }) } + + @Test("A mapper that reads every index as valid is caught") + func droppedValidityIsCaught() { + let fixtures = PluginStructureFixtures.indexes + let mapped = fixtures.map { + IndexInfo( + name: $0.name, columns: $0.columns, isUnique: $0.isUnique, isPrimary: $0.isPrimary, type: $0.type, + columnPrefixes: $0.columnPrefixes, whereClause: $0.whereClause, expressions: $0.expressions, + includedColumns: $0.includedColumns, ddlMethodAndKeys: $0.ddlMethodAndKeys, + ddlWhereClause: $0.ddlWhereClause + ) + } + let problems = StructureMappingCoverage.carryProblems(from: fixtures, to: mapped, appOnly: ["id"]) + #expect(problems == ["isValid: PluginIndexInfo has false, IndexInfo has true"]) + } } diff --git a/TableProTests/Helpers/PluginStructureFixtures.swift b/TableProTests/Helpers/PluginStructureFixtures.swift index 7c0e870a5b..5071cc2d1a 100644 --- a/TableProTests/Helpers/PluginStructureFixtures.swift +++ b/TableProTests/Helpers/PluginStructureFixtures.swift @@ -17,9 +17,9 @@ enum PluginStructureFixtures { ] static let indexes: [PluginIndexInfo] = [ - index("a", unique: true, primary: false, prefix: 1), - index("b", unique: false, primary: true, prefix: 2), - index("c", unique: false, primary: false, prefix: 3) + index("a", unique: true, primary: false, valid: true, prefix: 1), + index("b", unique: false, primary: true, valid: true, prefix: 2), + index("c", unique: false, primary: false, valid: false, prefix: 3) ] static let foreignKeys: [PluginForeignKeyInfo] = ["a", "b", "c"].map(foreignKey) @@ -67,7 +67,13 @@ enum PluginStructureFixtures { ) } - private static func index(_ suffix: String, unique: Bool, primary: Bool, prefix: Int) -> PluginIndexInfo { + private static func index( + _ suffix: String, + unique: Bool, + primary: Bool, + valid: Bool, + prefix: Int + ) -> PluginIndexInfo { PluginIndexInfo( name: "name-\(suffix)", columns: ["columns-\(suffix)"], @@ -79,7 +85,8 @@ enum PluginStructureFixtures { expressions: ["expressions-\(suffix)"], includedColumns: ["includedColumns-\(suffix)"], ddlMethodAndKeys: "ddlMethodAndKeys-\(suffix)", - ddlWhereClause: "ddlWhereClause-\(suffix)" + ddlWhereClause: "ddlWhereClause-\(suffix)", + isValid: valid ) } diff --git a/TableProTests/Plugins/PluginIndexInfoCodableTests.swift b/TableProTests/Plugins/PluginIndexInfoCodableTests.swift index 709eceb356..00692e6548 100644 --- a/TableProTests/Plugins/PluginIndexInfoCodableTests.swift +++ b/TableProTests/Plugins/PluginIndexInfoCodableTests.swift @@ -26,6 +26,7 @@ struct PluginIndexInfoCodableTests { #expect(decoded.includedColumns == nil) #expect(decoded.ddlMethodAndKeys == nil) #expect(decoded.ddlWhereClause == nil) + #expect(decoded.isValid == nil) } @Test("The published initializer leaves the new fields nil") @@ -35,8 +36,16 @@ struct PluginIndexInfoCodableTests { #expect(info.includedColumns == nil) #expect(info.ddlMethodAndKeys == nil) #expect(info.ddlWhereClause == nil) + #expect(info.isValid == nil) #expect(info.whereClause == "(a > 0)") + let spelled = PluginIndexInfo( + name: "ix", columns: ["a"], expressions: nil, includedColumns: nil, + ddlMethodAndKeys: "USING btree (a)", ddlWhereClause: nil + ) + #expect(spelled.ddlMethodAndKeys == "USING btree (a)") + #expect(spelled.isValid == nil) + let definition = PluginIndexDefinition(name: "ix", columns: ["a"], indexType: "HASH") #expect(definition.expressions == nil) #expect(definition.includedColumns == nil) @@ -54,9 +63,11 @@ struct PluginIndexInfoCodableTests { expressions: ["lower(email)"], includedColumns: ["name"], ddlMethodAndKeys: "USING btree (tenant_id, lower(email)) INCLUDE (name)", - ddlWhereClause: "(m = 'a'::src.mood)" + ddlWhereClause: "(m = 'a'::src.mood)", + isValid: false ) let decoded = try JSONDecoder().decode(PluginIndexInfo.self, from: JSONEncoder().encode(original)) + #expect(decoded.isValid == false) #expect(decoded.expressions == ["lower(email)"]) #expect(decoded.includedColumns == ["name"]) #expect(decoded.ddlMethodAndKeys == "USING btree (tenant_id, lower(email)) INCLUDE (name)") diff --git a/TableProTests/Plugins/PostgreSQLIndexReplayTests.swift b/TableProTests/Plugins/PostgreSQLIndexReplayTests.swift index 3ad184f8bf..d467f2b4fc 100644 --- a/TableProTests/Plugins/PostgreSQLIndexReplayTests.swift +++ b/TableProTests/Plugins/PostgreSQLIndexReplayTests.swift @@ -24,14 +24,45 @@ struct PostgreSQLIndexKeyPartTests { type: String = "btree", predicate: String? = nil, expressions: String = "{}", - included: String = "{}" + included: String = "{}", + valid: String = "t" ) -> [PluginCellValue] { [ .text(table), .text(name), .text(columns), .text(unique ? "true" : "false"), .text("false"), - .text(type), predicate.map(PluginCellValue.text) ?? .null, .text(expressions), .text(included) + .text(type), predicate.map(PluginCellValue.text) ?? .null, .text(expressions), .text(included), + .text(valid) ] } + @Test("Validity is read with the rule a dump applies, so an index on a partitioned table counts as valid") + func validityUsesTheDumpRule() { + for table in ["users", nil] { + let sql = PostgreSQLIndexQueries.indexList(schema: "public", table: table, capabilities: Self.modern) + #expect(sql.contains("((ix.indisvalid OR t.relkind = 'p') AND ix.indisready) AS is_valid")) + #expect(sql.contains("JOIN pg_catalog.pg_class t ON t.oid = ix.indrelid")) + } + } + + @Test("An invalid index decodes as invalid and a valid one as valid") + func validityIsDecoded() throws { + let invalid = try #require(PostgreSQLIndexRow.index( + from: Self.row(name: "users_email_key", columns: "{email}", unique: true, valid: "f"), ddl: [:] + )) + let valid = try #require(PostgreSQLIndexRow.index( + from: Self.row(name: "users_email", columns: "{email}"), ddl: [:] + )) + #expect(invalid.index.isValid == false) + #expect(valid.index.isValid == true) + } + + @Test("A row with no validity column reports none") + func missingValidityIsNotReported() throws { + let decoded = try #require(PostgreSQLIndexRow.index( + from: Array(Self.row(name: "users_email", columns: "{email}").prefix(9)), ddl: [:] + )) + #expect(decoded.index.isValid == nil) + } + @Test("Key parts stop at indnkeyatts from PostgreSQL 11, so INCLUDE columns are read separately") func keyCountFollowsCoveringIndexes() { let modern = PostgreSQLIndexQueries.indexList(schema: "public", table: nil, capabilities: Self.modern) diff --git a/TableProTests/Plugins/PostgreSQLLegacyCatalogQueryTests.swift b/TableProTests/Plugins/PostgreSQLLegacyCatalogQueryTests.swift index 2f67b859db..225ab3b91d 100644 --- a/TableProTests/Plugins/PostgreSQLLegacyCatalogQueryTests.swift +++ b/TableProTests/Plugins/PostgreSQLLegacyCatalogQueryTests.swift @@ -30,6 +30,8 @@ struct PostgreSQLLegacyCatalogQueryTests { PostgreSQLIndexQueries.indexList(schema: "public", table: nil, capabilities: legacy), PostgreSQLIndexQueries.indexDDLQuery(schema: "public", table: "orders"), PostgreSQLIndexQueries.indexDDLQuery(schema: "public", table: nil), + PostgreSQLIndexQueries.standaloneIndexQuery(schema: "public", table: "orders"), + PostgreSQLSchemaQueries.tableDDLConstraintsQuery(schema: "public", table: "orders"), PostgreSQLObjectQueries.triggerList(schema: "public", table: nil), PostgreSQLObjectQueries.userDefinedTypeList(schema: "public", identity: nil, capabilities: legacy), PostgreSQLSchemaQueries.checkConstraintsQuery(schema: "public", table: "t"), @@ -55,6 +57,8 @@ struct PostgreSQLLegacyCatalogQueryTests { PostgreSQLObjectQueries.triggerList(schema: hostile, table: hostile), PostgreSQLObjectQueries.routineList(schema: hostile, capabilities: Self.legacy), PostgreSQLSchemaQueries.checkConstraintsQuery(schema: hostile, table: hostile), + PostgreSQLSchemaQueries.tableDDLConstraintsQuery(schema: hostile, table: hostile), + PostgreSQLIndexQueries.standaloneIndexQuery(schema: hostile, table: hostile), PostgreSQLTableListing.query( schema: hostile, includeMaterializedViews: true, includeForeignTables: true ) diff --git a/TableProTests/Plugins/PostgreSQLLiteralQuotingTests.swift b/TableProTests/Plugins/PostgreSQLLiteralQuotingTests.swift index b768cdfb3a..887e037c00 100644 --- a/TableProTests/Plugins/PostgreSQLLiteralQuotingTests.swift +++ b/TableProTests/Plugins/PostgreSQLLiteralQuotingTests.swift @@ -42,6 +42,8 @@ struct PostgreSQLLiteralQuotingTests { PostgreSQLObjectQueries.userDefinedTypeList(schema: schema, identity: nil, capabilities: caps), PostgreSQLIndexQueries.indexList(schema: schema, table: table, capabilities: caps), PostgreSQLIndexQueries.indexDDLQuery(schema: schema, table: table), + PostgreSQLIndexQueries.standaloneIndexQuery(schema: schema, table: table), + PostgreSQLSchemaQueries.tableDDLConstraintsQuery(schema: schema, table: table), PostgreSQLForeignKeyQueries.foreignKeyList(schema: schema, table: table, capabilities: caps), PostgreSQLSequenceQueries.sequenceList( schema: schema, dependentOnTable: table, source: .sequencesView diff --git a/TableProTests/Plugins/PostgreSQLStandaloneIndexQueryTests.swift b/TableProTests/Plugins/PostgreSQLStandaloneIndexQueryTests.swift new file mode 100644 index 0000000000..a6faa61522 --- /dev/null +++ b/TableProTests/Plugins/PostgreSQLStandaloneIndexQueryTests.swift @@ -0,0 +1,71 @@ +// +// PostgreSQLStandaloneIndexQueryTests.swift +// TableProTests +// + +import Foundation +import TableProPluginKit +import Testing + +@Suite("PostgreSQL standalone index read") +struct PostgreSQLStandaloneIndexQueryTests { + private static let sql = PostgreSQLIndexQueries.standaloneIndexQuery(schema: "shop", table: "orders") + + @Test("An index is restorable under pg_dump's rule: valid, or on a partitioned table, and ready") + func restorableFollowsPgDump() { + #expect(Self.sql.contains("((ix.indisvalid OR t.relkind = 'p') AND ix.indisready) AS is_restorable")) + } + + @Test("Only an index a primary key, unique or exclusion constraint of this table owns is left to the table DDL") + func constraintOwnershipNamesTheTableAndTheKind() { + #expect(Self.sql.contains("con.conrelid = ix.indrelid")) + #expect(Self.sql.contains("con.conindid = ix.indexrelid")) + #expect(Self.sql.contains("con.contype IN ('p', 'u', 'x')")) + } + + @Test("The read is scoped to one table in one schema and ordered by name") + func scopedToOneTable() { + #expect(Self.sql.contains("WHERE n.nspname = 'shop'")) + #expect(Self.sql.contains("AND t.relname = 'orders'")) + #expect(Self.sql.contains("pg_catalog.pg_get_indexdef(ix.indexrelid) AS definition")) + #expect(Self.sql.hasSuffix("ORDER BY i.relname")) + } + + @Test("Restorable definitions are kept in order and invalid indexes are named apart") + func rowsSplitByRestorability() { + let indexes = PostgreSQLIndexQueries.standaloneIndexes(rows: [ + [.text("orders_code_idx"), .text("CREATE INDEX orders_code_idx ON shop.orders USING btree (code)"), .text("t")], + [.text("orders_code_key"), .text("CREATE UNIQUE INDEX orders_code_key ON shop.orders USING btree (code)"), .text("f")], + [.text("orders_day_idx"), .text("CREATE INDEX orders_day_idx ON shop.orders USING btree (day)"), .text("t")], + [.text("orders_code_idx_ccnew"), .text("CREATE INDEX orders_code_idx_ccnew ON shop.orders USING btree (code)"), .text("f")] + ]) + #expect(indexes.definitions == [ + "CREATE INDEX orders_code_idx ON shop.orders USING btree (code)", + "CREATE INDEX orders_day_idx ON shop.orders USING btree (day)" + ]) + #expect(indexes.invalidNames == ["orders_code_key", "orders_code_idx_ccnew"]) + } + + @Test("A row with no name or no definition is skipped") + func incompleteRowsAreSkipped() { + let indexes = PostgreSQLIndexQueries.standaloneIndexes(rows: [ + [.null, .text("CREATE INDEX a ON shop.orders USING btree (a)"), .text("t")], + [.text("b"), .null, .text("f")], + [.text("c"), .text(""), .text("t")] + ]) + #expect(indexes.definitions.isEmpty) + #expect(indexes.invalidNames.isEmpty) + } +} + +@Suite("PostgreSQL table DDL constraints read") +struct PostgreSQLTableDDLConstraintsQueryTests { + @Test("Exclusion constraints are written with the table, beside primary key, unique and check constraints") + func exclusionConstraintsAreIncluded() { + let sql = PostgreSQLSchemaQueries.tableDDLConstraintsQuery(schema: "shop", table: "bookings") + #expect(sql.contains("con.contype IN ('p', 'u', 'c', 'x')")) + #expect(sql.contains("CASE con.contype WHEN 'p' THEN 0 WHEN 'u' THEN 1 WHEN 'c' THEN 2 ELSE 3 END")) + #expect(sql.contains("c.relname = 'bookings'")) + #expect(sql.contains("n.nspname = 'shop'")) + } +} diff --git a/TableProTests/Plugins/PostgreSQLTableRebuildTests.swift b/TableProTests/Plugins/PostgreSQLTableRebuildTests.swift new file mode 100644 index 0000000000..b4bbaef378 --- /dev/null +++ b/TableProTests/Plugins/PostgreSQLTableRebuildTests.swift @@ -0,0 +1,85 @@ +// +// PostgreSQLTableRebuildTests.swift +// TableProTests +// + +import Foundation +import Testing + +@Suite("PostgreSQL column reorder rebuild script") +struct PostgreSQLTableRebuildTests { + private static let capabilities = PostgreSQLCapabilities(serverVersion: 170_011) + + private static func quote(_ identifier: String) -> String { + "\"\(identifier.replacingOccurrences(of: "\"", with: "\"\""))\"" + } + + private static func rebuild() -> PostgreSQLTableRebuild { + var parts = PostgreSQLTableRebuild() + parts.columnNames = ["id", "code"] + parts.columnDefinitions = ["id": "id integer NOT NULL", "code": "code integer"] + parts.copyableColumns = ["id", "code"] + parts.tableConstraints = ["CONSTRAINT parent_pkey PRIMARY KEY (id)"] + parts.indexes = ["CREATE UNIQUE INDEX parent_code_idx ON shop.parent USING btree (code)"] + parts.outboundForeignKeys = ["CONSTRAINT parent_self_fkey FOREIGN KEY (id) REFERENCES shop.parent(code)"] + parts.inboundForeignKeyDrops = ["ALTER TABLE shop.child DROP CONSTRAINT child_code_fkey"] + parts.inboundForeignKeyAdds = [ + "ALTER TABLE shop.child ADD CONSTRAINT child_code_fkey FOREIGN KEY (code) REFERENCES shop.parent(code)" + ] + parts.triggers = ["CREATE TRIGGER parent_audit AFTER INSERT ON shop.parent FOR EACH ROW EXECUTE FUNCTION audit()"] + return parts + } + + private static func statements(_ parts: PostgreSQLTableRebuild) -> [String] { + parts.statements( + table: "parent", schema: "shop", desiredOrder: ["code", "id"], quote: quote, capabilities: capabilities + ) + } + + private static func position(of needle: String, in statements: [String]) throws -> Int { + try #require(statements.firstIndex { $0.contains(needle) }, "\(needle) missing from the script") + } + + @Test("Indexes come back before any foreign key, which may reference a unique index rather than a constraint") + func indexesPrecedeForeignKeys() throws { + let statements = Self.statements(Self.rebuild()) + let index = try Self.position(of: "CREATE UNIQUE INDEX parent_code_idx", in: statements) + let outbound = try Self.position(of: "ADD CONSTRAINT parent_self_fkey", in: statements) + let inbound = try Self.position(of: "ADD CONSTRAINT child_code_fkey", in: statements) + #expect(index < outbound) + #expect(index < inbound) + } + + @Test("Indexes come back after the old table is gone and after the table's own constraints") + func indexesFollowTheDropAndTheConstraints() throws { + let statements = Self.statements(Self.rebuild()) + let index = try Self.position(of: "CREATE UNIQUE INDEX parent_code_idx", in: statements) + let drop = try Self.position(of: #"DROP TABLE "shop"."parent_tablepro_reorder""#, in: statements) + let primaryKey = try Self.position(of: "ADD CONSTRAINT parent_pkey", in: statements) + let trigger = try Self.position(of: "CREATE TRIGGER parent_audit", in: statements) + #expect(drop < index) + #expect(primaryKey < index) + #expect(index < trigger) + } + + @Test("The rebuilt table declares its columns in the order asked for") + func columnsFollowTheDesiredOrder() throws { + let statements = Self.statements(Self.rebuild()) + let create = try #require(statements.first { $0.hasPrefix(#"CREATE TABLE "shop"."parent""#) }) + #expect(create == "CREATE TABLE \"shop\".\"parent\" (\n code integer,\n id integer NOT NULL\n)") + } + + @Test("Invalid indexes are named in the caveats and never recreated") + func invalidIndexesAreNamed() { + var parts = Self.rebuild() + parts.invalidIndexes = ["parent_code_key", "parent_code_idx_ccnew"] + #expect(parts.caveats.contains { $0.contains("parent_code_key, parent_code_idx_ccnew") }) + #expect(!Self.statements(parts).contains { $0.contains("parent_code_key") }) + } + + @Test("A table with no invalid index adds no caveat for them") + func noInvalidIndexNoCaveat() { + let caveats = Self.rebuild().caveats + #expect(caveats.count == 2) + } +} diff --git a/TableProTests/Plugins/SQLExportIndexPhaseTests.swift b/TableProTests/Plugins/SQLExportIndexPhaseTests.swift index 9388fddfe5..2e57a03b19 100644 --- a/TableProTests/Plugins/SQLExportIndexPhaseTests.swift +++ b/TableProTests/Plugins/SQLExportIndexPhaseTests.swift @@ -13,15 +13,18 @@ struct SQLExportIndexPhaseTests { let databaseTypeId: String let indexDDL: [String: [String]] let failingTables: Set + let foreignKeys: [String: [PluginForeignKeyInfo]] init( databaseTypeId: String = "PostgreSQL", indexDDL: [String: [String]] = [:], - failingTables: Set = [] + failingTables: Set = [], + foreignKeys: [String: [PluginForeignKeyInfo]] = [:] ) { self.databaseTypeId = databaseTypeId self.indexDDL = indexDDL self.failingTables = failingTables + self.foreignKeys = foreignKeys } func streamRows(table: String, databaseName: String) -> AsyncThrowingStream { @@ -56,6 +59,10 @@ struct SQLExportIndexPhaseTests { } func fetchApproximateRowCount(table: String, databaseName: String) async throws -> Int? { nil } + + func fetchAllForeignKeys(databaseName: String) async throws -> [String: [PluginForeignKeyInfo]] { + foreignKeys + } } private enum StubError: Error { @@ -101,8 +108,8 @@ struct SQLExportIndexPhaseTests { #expect(try offset(of: "INSERT INTO", in: dump) < offset(of: "CREATE INDEX", in: dump)) } - @Test("Index statements are written after the deferred foreign keys") - func indexesFollowDeferredConstraints() async throws { + @Test("Index statements are written after every table is created") + func indexesFollowTheTables() async throws { let source = StubExportDataSource( indexDDL: ["orders": ["CREATE INDEX idx_orders_day ON orders (day)"]]) @@ -111,6 +118,24 @@ struct SQLExportIndexPhaseTests { #expect(try offset(of: "CREATE TABLE orders", in: dump) < offset(of: "CREATE INDEX", in: dump)) } + @Test("A unique index is written before the deferred foreign key that references it, as pg_dump orders them") + func indexesPrecedeDeferredForeignKeys() async throws { + let source = StubExportDataSource( + indexDDL: ["parent": ["CREATE UNIQUE INDEX parent_code_idx ON parent (code)"]], + foreignKeys: ["child": [PluginForeignKeyInfo( + name: "child_code_fkey", column: "code", referencedTable: "parent", referencedColumn: "code" + )]] + ) + + let (dump, _) = try await runExport(tables: [table("child"), table("parent")], dataSource: source) + + let index = try offset(of: "CREATE UNIQUE INDEX parent_code_idx", in: dump) + let foreignKey = try offset(of: "ADD CONSTRAINT \"child_code_fkey\"", in: dump) + let lastRow = try offset(of: "INSERT INTO \"parent\"", in: dump) + #expect(lastRow < index) + #expect(index < foreignKey) + } + @Test("Only the statements the driver reported are written, in the order it reported them") func statementsAreWrittenVerbatim() async throws { let source = StubExportDataSource(indexDDL: ["orders": [ diff --git a/TableProTests/Views/Structure/InvalidIndexNoteTests.swift b/TableProTests/Views/Structure/InvalidIndexNoteTests.swift new file mode 100644 index 0000000000..4663f9e460 --- /dev/null +++ b/TableProTests/Views/Structure/InvalidIndexNoteTests.swift @@ -0,0 +1,46 @@ +// +// InvalidIndexNoteTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("Invalid index note") +struct InvalidIndexNoteTests { + private static func index(_ name: String, valid: Bool = true) -> IndexInfo { + IndexInfo(name: name, columns: ["code"], isUnique: false, isPrimary: false, type: "BTREE", isValid: valid) + } + + @Test("No note when every index is valid") + func noNoteWhenAllValid() { + #expect(InvalidIndexNote(indexes: [Self.index("orders_pkey"), Self.index("orders_code_idx")]) == nil) + #expect(InvalidIndexNote(indexes: []) == nil) + } + + @Test("The note names the invalid index and none of the valid ones") + func namesOnlyTheInvalidIndex() throws { + let note = try #require(InvalidIndexNote(indexes: [ + Self.index("orders_code_idx"), + Self.index("orders_code_key", valid: false) + ])) + #expect(note.text.contains("orders_code_key")) + #expect(!note.text.contains("orders_code_idx")) + #expect(note.text.contains("orders_code_key is invalid")) + #expect(note.text.contains("REINDEX")) + #expect(note.systemImage == "exclamationmark.triangle") + } + + @Test("Several invalid indexes are listed in one note") + func listsEveryInvalidIndex() throws { + let note = try #require(InvalidIndexNote(indexes: [ + Self.index("orders_email_key", valid: false), + Self.index("orders_pkey"), + Self.index("orders_code_idx_ccnew", valid: false) + ])) + #expect(note.text.contains("orders_email_key")) + #expect(note.text.contains("orders_code_idx_ccnew are invalid")) + #expect(!note.text.contains("orders_pkey")) + } +} diff --git a/docs/databases/postgresql.mdx b/docs/databases/postgresql.mdx index 65cfd132c6..715fcbd69a 100644 --- a/docs/databases/postgresql.mdx +++ b/docs/databases/postgresql.mdx @@ -116,6 +116,10 @@ The Structure tab of a view, a materialized view or a foreign table offers only PostgreSQL refuses `INSERT`, `UPDATE` and `DELETE` on a materialized view, so its rows open read-only in the data grid. Edit the tables it reads from, then choose **Refresh Materialized View…**. +## Invalid indexes + +A `CREATE INDEX CONCURRENTLY` or `REINDEX CONCURRENTLY` that fails or is cancelled leaves an invalid index on the table. The **Indexes** tab names it, and exports leave it out as `pg_dump` does. [Table Structure](/features/table-structure#invalid-indexes) covers dropping or rebuilding it. + ## Cross-database tabs PostgreSQL has no in-place `USE`, so a tab bound to a database other than the connection's active one runs on a second connection opened for that database. It shares no temp tables, session variables, or open transaction with the query editor on the main connection: keep a multi-statement transaction or a `CREATE TEMP TABLE` on tabs bound to one database. Binding itself is on [Tabs](/features/tabs#where-a-tab-points). diff --git a/docs/external-api/mcp-tools.mdx b/docs/external-api/mcp-tools.mdx index bf1a4b592a..3f3da80e2a 100644 --- a/docs/external-api/mcp-tools.mdx +++ b/docs/external-api/mcp-tools.mdx @@ -78,7 +78,7 @@ The two `switch_` tools move what the user sees in TablePro. To run one statemen | `get_table_statistics` | `connection_id`, `table` (`database`, `schema`) | `table` plus whatever the engine records: `data_size_bytes`, `index_size_bytes`, `total_size_bytes`, `average_row_length`, `row_count`, `comment`, `engine`, `collation`, `created_at`, `updated_at` | | `get_database_statistics` | `connection_id` (`database`) | `databases[]` (`name`, `table_count`, `size_bytes`, `is_system_database`), sorted by name | -`describe_table` is the one call to make before writing SQL against an unfamiliar table. A column always carries `name`, `data_type`, `is_nullable` and `is_primary_key`, and picks up `is_generated`, `default_value`, `extra`, `comment` and `allowed_values` where the engine reports them; indexes and foreign keys work the same way. +`describe_table` is the one call to make before writing SQL against an unfamiliar table. A column always carries `name`, `data_type`, `is_nullable` and `is_primary_key`, and picks up `is_generated`, `default_value`, `extra`, `comment` and `allowed_values` where the engine reports them; indexes and foreign keys work the same way. An index always carries `name`, `columns`, `is_unique`, `is_primary`, `is_valid` and `type`. `is_valid` is `false` for a PostgreSQL index whose build did not finish, such as one a failed `CREATE INDEX CONCURRENTLY` left behind: queries skip it and `get_table_ddl` leaves it out. `include_row_counts` defaults to `false`. Counts come from engine statistics rather than `COUNT(*)`, and are fetched one table at a time, so `list_tables` skips them when the schema holds more than 200 objects. diff --git a/docs/features/import-export.mdx b/docs/features/import-export.mdx index aa85b44cbd..6d15cda653 100644 --- a/docs/features/import-export.mdx +++ b/docs/features/import-export.mdx @@ -124,7 +124,7 @@ As SQL, a results export writes `INSERT` statements only. A result set is the ou Structure (CREATE TABLE), Drop (DROP TABLE IF EXISTS), and Data (INSERT statements) are per-table checkboxes, and a multi-table export can mix them. - Structure carries the table's indexes. They are written after the rows, next to the deferred foreign keys, which is where `pg_dump` and `sqlite3 .dump` put them: a bulk load into an indexed table pays to maintain an index the restore is about to build anyway. On MySQL, MariaDB, ClickHouse, Trino and CockroachDB the server writes them inside `CREATE TABLE`, so they arrive with the table instead. A materialized view's indexes follow the view. + Structure carries the table's indexes. They are written after the rows and before the deferred foreign keys, the order `pg_dump` uses, so the rows load without index upkeep and a foreign key that references a unique index finds it in place. On MySQL, MariaDB, ClickHouse, Trino and CockroachDB the server writes them inside `CREATE TABLE`, so they arrive with the table instead. A materialized view's indexes follow the view. An invalid PostgreSQL index, which a failed `CREATE INDEX CONCURRENTLY` leaves behind, is left out, as `pg_dump` leaves it out. On PostgreSQL, Structure carries comments as well. Each table, view and materialized view gets its own `COMMENT ON` and one `COMMENT ON COLUMN` per commented column, directly after its `CREATE`. A foreign table is the exception: its comments are dropped, because the dump writes `CREATE TABLE` for it and PostgreSQL then refuses `COMMENT ON FOREIGN TABLE`. Where the server's own `CREATE TABLE` already carries the comment, MySQL and MariaDB among them, it arrives with the table instead. diff --git a/docs/features/table-structure.mdx b/docs/features/table-structure.mdx index 5ca9359680..8aa2caf2f1 100644 --- a/docs/features/table-structure.mdx +++ b/docs/features/table-structure.mdx @@ -111,6 +111,12 @@ Everything that runs goes to query history rather than the change queue. | **Unique** | Whether the index enforces uniqueness | | **Condition** | `WHERE` predicate for partial indexes (PostgreSQL, SQLite, libSQL, Cloudflare D1) | +### Invalid indexes + +A PostgreSQL `CREATE INDEX CONCURRENTLY` or `REINDEX CONCURRENTLY` that fails or is cancelled leaves an invalid index behind. It keeps its row in the list, and a line under the list names it. Queries skip it, but every write still updates it, and an invalid unique index still refuses duplicates. Delete the row and save to drop it, or run `REINDEX INDEX` on it in the SQL editor to build it again. + +The DDL tab, SQL exports and **Copy To** leave an invalid index out, the same as `pg_dump`, and [Compare & Sync](/features/compare-sync) never creates one on the target. An index on a partitioned table that is still waiting for a partition's index to be attached is not flagged, and exports keep it. + ### Materialized views A materialized view's indexes list, add and drop the same way a table's do. The line under the list says whether **Refresh Materialized View** can refresh it concurrently, which keeps other sessions reading the view while it runs: diff --git a/project.yml b/project.yml index ba1a157426..1b3fa67f0b 100644 --- a/project.yml +++ b/project.yml @@ -601,6 +601,7 @@ targets: - Plugins/PostgreSQLDriverPlugin/PostgreSQLSystemDatabases.swift - Plugins/PostgreSQLDriverPlugin/PostgreSQLTableListing.swift - Plugins/PostgreSQLDriverPlugin/PostgreSQLTableListingLadder.swift + - Plugins/PostgreSQLDriverPlugin/PostgreSQLTableRebuild.swift - Plugins/PostgreSQLDriverPlugin/PostgreSQLTextArray.swift - Plugins/PostgreSQLDriverPlugin/PostgreSQLTransactionStatement.swift - Plugins/PostgreSQLDriverPlugin/PostgreSQLTypeDefinition.swift diff --git a/scripts/check-postgres-index-dump-parity.sh b/scripts/check-postgres-index-dump-parity.sh new file mode 100755 index 0000000000..b872e63c10 --- /dev/null +++ b/scripts/check-postgres-index-dump-parity.sh @@ -0,0 +1,203 @@ +#!/usr/bin/env bash +# +# Check the PostgreSQL table and index DDL against pg_dump on a real server. +# +# A PostgreSQL dump writes a table's constraints inside its CREATE TABLE and every other index as a +# CREATE INDEX of its own. Both halves are catalog predicates copied by hand from pg_dump's +# getIndexes and getConstraints, and they have to agree with it and with each other: an invalid +# index a failed CREATE INDEX CONCURRENTLY left behind made the restore fail on its duplicates, a +# unique index a foreign key depends on went missing, and an exclusion constraint was in neither +# half. +# +# This builds every one of those shapes, asks the same predicates the plugin uses, runs pg_dump -s +# on each table, and compares the index names and the constraint names on both sides. +# +# Usage: +# scripts/check-postgres-index-dump-parity.sh [host] [port] [user] +# +# Needs psql, pg_dump at least as new as the server, and a PostgreSQL 11 or newer the user may +# create a database on. Takes about ten seconds, most of it waiting out two cancelled concurrent +# builds. Exits non-zero on a disagreement. + +set -uo pipefail + +HOST="${1:-127.0.0.1}" +PORT="${2:-5432}" +USER_NAME="${3:-postgres}" +PLUGIN="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/Plugins/PostgreSQLDriverPlugin" +INDEX_SOURCE="$PLUGIN/PostgreSQLIndexQueries.swift" +CONSTRAINT_SOURCE="$PLUGIN/PostgreSQLSchemaQueries.swift" +DATABASE="tablepro_index_dump_parity_check" + +for tool in psql pg_dump; do + command -v "$tool" > /dev/null || { + echo "$tool not found" >&2 + exit 3 + } +done +for source in "$INDEX_SOURCE" "$CONSTRAINT_SOURCE"; do + [ -f "$source" ] || { + echo "not found: $source" >&2 + exit 3 + } +done + +export PGOPTIONS="-c client_min_messages=warning" + +psql_do() { + psql -X -q -h "$HOST" -p "$PORT" -U "$USER_NAME" -d "$1" -v ON_ERROR_STOP=1 "${@:2}" +} + +if ! psql_do postgres -Atc "SELECT 1" > /dev/null 2>&1; then + echo "no PostgreSQL at $HOST:$PORT as $USER_NAME" >&2 + exit 3 +fi + +# The predicates as the plugin builds them, read out of the source so the two cannot drift. The +# Swift side interpolates the schema and the table; here they are bound per table instead. +RESTORABLE="(ix.indisvalid OR t.relkind = 'p') AND ix.indisready" +check_fragment() { + grep -qF "$2" "$1" || { + echo "FAIL: $1 no longer holds $2; update this script with the predicate" >&2 + exit 1 + } +} +check_fragment "$INDEX_SOURCE" "$RESTORABLE" +check_fragment "$INDEX_SOURCE" "con.conrelid = ix.indrelid" +check_fragment "$INDEX_SOURCE" "con.conindid = ix.indexrelid" +check_fragment "$INDEX_SOURCE" "con.contype IN ('p', 'u', 'x')" +check_fragment "$CONSTRAINT_SOURCE" "con.contype IN ('p', 'u', 'c', 'x')" + +VERSION="$(psql_do postgres -Atc "SHOW server_version")" +echo "Checking table and index DDL against $(pg_dump --version) and PostgreSQL $VERSION at $HOST:$PORT" + +psql_do postgres -c "DROP DATABASE IF EXISTS $DATABASE" > /dev/null +psql_do postgres -c "CREATE DATABASE $DATABASE" > /dev/null +trap 'psql -X -q -h "$HOST" -p "$PORT" -U "$USER_NAME" -d postgres -c "DROP DATABASE IF EXISTS $DATABASE" > /dev/null 2>&1' EXIT + +psql_do "$DATABASE" > /dev/null <<'SQL' +CREATE TABLE t (id int PRIMARY KEY, email text, code int); +INSERT INTO t VALUES (1, 'a@x', 1), (2, 'a@x', 2), (3, 'b@x', 3); +CREATE INDEX t_code_idx ON t (code); + +CREATE TABLE parent (id int, code int); +CREATE UNIQUE INDEX parent_code_idx ON parent (code); +CREATE TABLE child (code int REFERENCES parent (code)); + +CREATE TABLE s (id int, v int CONSTRAINT s_v_idx CHECK (v > 0), w int UNIQUE); +CREATE INDEX s_v_idx ON s (v); + +CREATE TABLE ex (r int4range, EXCLUDE USING gist (r WITH &&)); + +CREATE TABLE p (a int, b int) PARTITION BY RANGE (a); +CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (0) TO (10); +CREATE TABLE p2 PARTITION OF p FOR VALUES FROM (10) TO (20); +CREATE INDEX p_a_idx ON ONLY p (a); +CREATE INDEX p1_a_idx ON p1 (a); +ALTER INDEX p_a_idx ATTACH PARTITION p1_a_idx; +CREATE INDEX p_b_idx ON p (b); +SQL + +# A unique build over duplicate rows fails and leaves an index that is neither valid nor ready. +psql_do "$DATABASE" -c "CREATE UNIQUE INDEX CONCURRENTLY t_email_key ON t (email)" > /dev/null 2>&1 + +# A concurrent build or rebuild cancelled while it waits out an older snapshot leaves an index +# that is ready but not valid. +cancel_while_waiting() { + psql -X -q -h "$HOST" -p "$PORT" -U "$USER_NAME" -d "$DATABASE" \ + -c "BEGIN ISOLATION LEVEL REPEATABLE READ; SELECT 1; SELECT pg_sleep(4); COMMIT;" > /dev/null 2>&1 & + local holder=$! + sleep 1 + psql -X -q -h "$HOST" -p "$PORT" -U "$USER_NAME" -d "$DATABASE" \ + -c "SET statement_timeout = '1500ms'" -c "$1" > /dev/null 2>&1 + wait "$holder" +} +cancel_while_waiting "CREATE UNIQUE INDEX CONCURRENTLY t_code_key ON t (code)" +cancel_while_waiting "REINDEX INDEX CONCURRENTLY t_code_idx" + +INVALID="$(psql_do "$DATABASE" -Atc " + SELECT string_agg(i.relname, ' ' ORDER BY i.relname) + FROM pg_catalog.pg_index ix + JOIN pg_catalog.pg_class i ON i.oid = ix.indexrelid + WHERE NOT ix.indisvalid")" +for expected in t_email_key t_code_key t_code_idx_ccnew p_a_idx; do + case " $INVALID " in + *" $expected "*) ;; + *) + echo "setup did not leave $expected invalid (invalid: $INVALID), so this run proves nothing" >&2 + exit 2 + ;; + esac +done + +plugin_indexes() { + psql_do "$DATABASE" -Atc " + SELECT i.relname + FROM pg_catalog.pg_index ix + JOIN pg_catalog.pg_class i ON i.oid = ix.indexrelid + JOIN pg_catalog.pg_class t ON t.oid = ix.indrelid + JOIN pg_catalog.pg_namespace n ON n.oid = t.relnamespace + WHERE n.nspname = 'public' + AND t.relname = '$1' + AND NOT EXISTS ( + SELECT 1 FROM pg_catalog.pg_constraint con + WHERE con.conrelid = ix.indrelid + AND con.conindid = ix.indexrelid + AND con.contype IN ('p', 'u', 'x') + ) + AND ($RESTORABLE)" +} + +plugin_constraints() { + psql_do "$DATABASE" -Atc " + SELECT con.conname + FROM pg_constraint con + JOIN pg_class c ON c.oid = con.conrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE c.relname = '$1' + AND n.nspname = 'public' + AND con.contype IN ('p', 'u', 'c', 'x')" +} + +dump_of() { + pg_dump -h "$HOST" -p "$PORT" -U "$USER_NAME" -d "$DATABASE" -s -t "public.$1" +} + +dump_indexes() { + dump_of "$1" | sed -nE 's/^CREATE (UNIQUE )?INDEX ([^ ]+) ON .*/\2/p' | sort +} + +dump_constraints() { + dump_of "$1" \ + | sed -nE '/FOREIGN KEY/d; s/^ +ADD CONSTRAINT ([^ ]+) .*/\1/p; s/^ +CONSTRAINT ([^ ]+) CHECK .*/\1/p' \ + | sort +} + +TABLES="$(psql_do "$DATABASE" -Atc " + SELECT c.relname + FROM pg_catalog.pg_class c + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relkind IN ('r', 'p') + ORDER BY 1")" + +failures=0 +compare() { + local table="$1" kind="$2" plugin="$3" dump="$4" + if [ "$plugin" = "$dump" ]; then + printf 'ok %-7s %-11s %s\n' "$table" "$kind" "${plugin//$'\n'/ }" + else + printf 'FAIL %-7s %-11s plugin=[%s] pg_dump=[%s]\n' "$table" "$kind" "${plugin//$'\n'/ }" "${dump//$'\n'/ }" + failures=$((failures + 1)) + fi +} +for table in $TABLES; do + compare "$table" indexes "$(plugin_indexes "$table" | sort)" "$(dump_indexes "$table")" + compare "$table" constraints "$(plugin_constraints "$table" | sort)" "$(dump_constraints "$table")" +done + +if [ "$failures" -gt 0 ]; then + echo "$failures comparison(s) disagree with pg_dump" >&2 + exit 1 +fi + +echo "The table and index DDL agree with pg_dump on every shape."