Skip to content

fix(sidebar): keep a Replace copy's clearing inside the transaction it promises to roll back - #2570

Merged
datlechin merged 1 commit into
mainfrom
fix/object-copy-review
Aug 29, 2026
Merged

fix(sidebar): keep a Replace copy's clearing inside the transaction it promises to roll back#2570
datlechin merged 1 commit into
mainfrom
fix/object-copy-review

Conversation

@datlechin

Copy link
Copy Markdown
Member

Purpose

A review of the object copy feature (#2567, #2568) after it landed. Twelve findings, three of which lose the user's data, plus one the review itself missed.

Nothing here is released yet, so per CLAUDE.md the fixes fold into the existing Copy To and Duplicate Database entry rather than adding Fixed lines of their own.

Data loss

A Replace deleted the target's rows outside the transaction that promised to put them back

ObjectCopyTableStep.truncateStatements documented the rule: the clear "runs inside the same transaction as this table's rows, so a copy that fails or is stopped puts the target's own rows back. Run ahead of the transaction it deleted them for good while rolling only the new rows back."

The runner ran it ahead of the transaction. plan.clearGroups went through its own runDDL call, which opens its own lease and no transaction, and only then did the data phase start opening one transaction per table.

Data only, existing policy Replace, error handling Stop and roll back:

  1. DELETE FROM target.orders and DELETE FROM target.order_items commit.
  2. Table two fails, or the user presses Stop.
  3. copyRows rolls back the INSERTs it opened a transaction for.
  4. The target's original rows are gone and nothing put them back.

The two requirements that collided here are both real. Every table has to be emptied before any is filled, or the first parent DELETE meets child rows that are still there and a cascading key takes rows out of a table the user never selected. And the clear has to be undoable by whatever fails after it.

One transaction satisfies both. ObjectCopyPlan.clearsInsideDataTransaction is where the decision lives: when there is something to empty and the run promises a rollback, the clears and every table's rows run in one transaction on the target, with the source leased per table. A copy with nothing to empty is untouched and keeps its per-table transactions, where one table's failure leaves the tables already copied alone.

The same database was accepted as its own target

sameObjectRefusal compared DatabaseEndpoint.id, which spells the schema into the identity.

connection database schema id
Source, from a right-click on the database row A app none `A
Target, chosen in the picker A app public `A

The ids differ, so both refuseUpFront and the sheet's own check passed. ObjectCopyPlanner.plan then resolved the source to app.public and the target to app.public, and with Replace the run dropped and recreated every table in public before streaming rows from the table it had just emptied.

An endpoint that names no schema stands for whichever schema its objects turn out to be in, so it overlaps every schema of its database rather than none of them. The refusal now compares that: same connection, same database, and either side's schema absent or equal. Two schemas of one database stay a valid pair, which is what the schema-scoped endpoint exists for.

A table with no writable column in common was emptied and never refilled

emptiesFirst did not depend on any column pair resolving, while copiesData did.

Data only, Replace, two tables. orders shares its columns. legacy_audit exists in the target with entirely different column names, so writableColumnPairs returns empty. legacy_audit was excluded from dataSteps and included in clearGroups. plan.isEmpty was false because orders had work, so the run went ahead, deleted every row of the target's legacy_audit and wrote nothing back. The review sheet said only "The source and the target share no writable column."

emptiesFirst now requires copiesData, and clearGroups filters on it too, so the plan cannot emit a clear for a table that is not also in dataSteps.

Correctness

Duplicate Database never created the schemas it qualified its tables with

No CREATE SCHEMA existed anywhere under Core/ObjectCopy. A PostgreSQL database with public and sales, duplicated: CREATE DATABASE app_copy succeeds and gives the new database public alone, then CREATE TABLE "sales"."invoices" fails with schema "sales" does not exist. Under the default Stop and roll back the whole structure phase then rolls back, leaving a database that holds nothing and cannot be retried without deleting it first. Every schema other than public was unreachable.

createSchemaStatement(name:) is a new PluginDatabaseDriver requirement with a nil default, which is additive and ABI safe. PostgreSQL implements it. The statements run first in the structure phase and throw rather than being attributed to an object: nothing can be created if the schema naming it is not there, and reporting one cause as N per-object failures helps nobody. A driver that answers nil leaves the copy exactly as it was rather than being handed DDL the server will reject.

A rolled-back structure phase reported the objects it had undone

Self.execute appends a success outcome per object as it goes. On a failure at table eight, runStructure rolled the transaction back and returned all seven earlier successes, so succeededCount was 7 over a target where nothing was written. Same path on a Stop.

The result sheet dropped one of two error messages

ObjectCopyObjectOutcome.id is selection.id, and one selection produces an outcome per phase. Under Skip and continue a table whose CREATE fails also fails in the data phase with "relation does not exist", so ForEach(failures) saw two rows with the same identifier and rendered undefined results. ObjectCopyRunResult.failedCount already deduplicates by id for exactly this reason, which is why the ids have to collide. failures gives each one a positional identity instead.

A lease failure said nothing

Found while writing the above rather than in the review. A withMetadataDriver failure in the new data path was caught and turned into a stopped result with no outcomes, so firstError was nil and the sheet reported a copy that did nothing without saying why. It throws now, the way the DDL phases already do. A failing commit rolls back rather than leaving the transaction for the lease to close.

The approved script was not the order that ran

scriptText put each table's truncate with that table's data step, so a data-only Replace of orders and order_items read as DELETE FROM orders; SELECT … FROM orders; DELETE FROM order_items; SELECT …. The runner issues both DELETEs, children first, before either SELECT. A user reasoning about a cascade from what they were asked to approve reached the opposite conclusion from what the run does. The clears are now one block ahead of the reads.

The plan order depended on Swift's hash seed

orderedByDependency was seeded with Array(reads.keys). The topological sort tie-breaks alphabetically, so the main path was stable, but the tail it appends for tables the sort could not place followed the dictionary's order, and so did which selection won when two resolved to one sort key. Seeded from scope.objects, which already carries the user's own order.

The object checkbox inverted on any write

set: { _ in session.toggle(object) }

The setter discarded the value SwiftUI handed it. Any write of the value already held is then a change: a re-render that re-applies the current state, an accessibility setValue, or a second delivery while the list diffs under a search keystroke each took an object out of the copy or put one in. session.setSelected(object, $0) cannot desynchronise.

Efficiency and clarity

  • sourceDefinitions fetched every view body, routine body and trigger body before the loop applied canCopyDefinition, which is loop invariant and false for every object of a cross-namespace copy. On a MySQL prod to staging copy with 200 routines that was 200 round trips whose results were discarded one line later, plus one identical skip row per object. The check is hoisted above the fetch and writes one scope-level reason.
  • existingTargetObjects read the full target catalog once per source scope, and did it for data-only copies where buildDefinitionSteps returns before touching it. Now gated on content.includesStructure and memoized per target endpoint, so twelve schemas into one target read it once.
  • retargeted evaluated a loop-invariant guard inside its per-key map; orderedByDependency closed with an O(n²) result.contains, which is 125,000 equality checks on a 500-table copy.
  • sourceDefinitions built a whole throwaway ObjectCopyRequest that shadowed its parameter so it could read back the sourceEndpoint it was already handed.

Reviewed and rejected

One finding said the PostgreSQL setval in fetchDependentSequences had been unqualified in #2568 and so broke SQL export and the Structure view, which share the method.

The CREATE SEQUENCE beside it was always unqualified. #2568 made the pair agree; before it, a dump created the sequence in the restoring session's search_path and then repositioned a different one, or errored. The current form is the correct one, and the change is left alone.

Success criteria

verify.sh generate: PASS
verify.sh build:    PASS
verify.sh docs:     PASS  (check-writing-style.sh and check-docs-against-source.py both clean)
swiftlint lint --strict on TablePro: 0 violations

Tests, all suites in the blast radius:

117 executed, 117 passed, 0 failed

ObjectCopyPlanTests is new and covers the plan-level invariants: a table that copies no rows is never emptied, tables are emptied children first, when the clears join the data transaction and when they do not, the script empties everything before it reads anything, schema statements lead the DDL, and every failure keeps an identity of its own while the summary still counts the object once. ObjectCopyEligibilityTests gains the four refusal cases, ObjectCopyPlannerOrderingTests two ordering cases, ObjectCopySessionTests one for the binding.

The ABI check is additive:

+ public func createSchemaStatement(name: Swift::String) -> Swift::String?

One symbol added, zero removed, a requirement with a default. No currentPluginKitVersion bump and no plugin re-release.

Not verified

verify.sh plugins fails locally, with both of its two errors coming from an @TaskLocal macro expansion inside the pinned oracle-nio fork. That is a known local toolchain problem unrelated to this change; CI compiles the aggregate. The PostgreSQL plugin is the only driver edited here and it compiled as part of the passing app build, and the PluginKit addition is a defaulted requirement, so no plugin has to implement anything.

No TableProUITests coverage. The copy sheet needs two live connections with real objects on both sides, which the isolated UI fixture does not provide and which would not be deterministic.

PostgreSQLPluginDriver is over the type_body_length warning at 1149 lines and these six lines make it slightly longer. It is pre-existing, Plugins/ is outside .swiftlint.yml's included:, and splitting that class is its own change.

@mintlify

mintlify Bot commented Aug 29, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 29, 2026, 6:24 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit e409d4b into main Aug 29, 2026
14 checks passed
@datlechin
datlechin deleted the fix/object-copy-review branch August 29, 2026 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant