fix(sidebar): keep a Replace copy's clearing inside the transaction it promises to roll back - #2570
Merged
Merged
Conversation
…t promises to roll back
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.mdthe fixes fold into the existingCopy To and Duplicate Databaseentry rather than addingFixedlines of their own.Data loss
A Replace deleted the target's rows outside the transaction that promised to put them back
ObjectCopyTableStep.truncateStatementsdocumented 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.clearGroupswent through its ownrunDDLcall, 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:
DELETE FROM target.ordersandDELETE FROM target.order_itemscommit.copyRowsrolls back the INSERTs it opened a transaction for.The two requirements that collided here are both real. Every table has to be emptied before any is filled, or the first parent
DELETEmeets 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.clearsInsideDataTransactionis 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
sameObjectRefusalcomparedDatabaseEndpoint.id, which spells the schema into the identity.idAappAapppublicThe ids differ, so both
refuseUpFrontand the sheet's own check passed.ObjectCopyPlanner.planthen resolved the source toapp.publicand the target toapp.public, and with Replace the run dropped and recreated every table inpublicbefore 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
emptiesFirstdid not depend on any column pair resolving, whilecopiesDatadid.Data only, Replace, two tables.
ordersshares its columns.legacy_auditexists in the target with entirely different column names, sowritableColumnPairsreturns empty.legacy_auditwas excluded fromdataStepsand included inclearGroups.plan.isEmptywas false becauseordershad work, so the run went ahead, deleted every row of the target'slegacy_auditand wrote nothing back. The review sheet said only "The source and the target share no writable column."emptiesFirstnow requirescopiesData, andclearGroupsfilters on it too, so the plan cannot emit a clear for a table that is not also indataSteps.Correctness
Duplicate Database never created the schemas it qualified its tables with
No
CREATE SCHEMAexisted anywhere underCore/ObjectCopy. A PostgreSQL database withpublicandsales, duplicated:CREATE DATABASE app_copysucceeds and gives the new databasepublicalone, thenCREATE TABLE "sales"."invoices"fails withschema "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 thanpublicwas unreachable.createSchemaStatement(name:)is a newPluginDatabaseDriverrequirement with anildefault, 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.executeappends a success outcome per object as it goes. On a failure at table eight,runStructurerolled the transaction back and returned all seven earlier successes, sosucceededCountwas 7 over a target where nothing was written. Same path on a Stop.The result sheet dropped one of two error messages
ObjectCopyObjectOutcome.idisselection.id, and one selection produces an outcome per phase. Under Skip and continue a table whoseCREATEfails also fails in the data phase with "relation does not exist", soForEach(failures)saw two rows with the same identifier and rendered undefined results.ObjectCopyRunResult.failedCountalready deduplicates by id for exactly this reason, which is why the ids have to collide.failuresgives each one a positional identity instead.A lease failure said nothing
Found while writing the above rather than in the review. A
withMetadataDriverfailure in the new data path was caught and turned into a stopped result with no outcomes, sofirstErrorwas 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
scriptTextput each table's truncate with that table's data step, so a data-only Replace ofordersandorder_itemsread asDELETE FROM orders; SELECT … FROM orders; DELETE FROM order_items; SELECT …. The runner issues bothDELETEs, children first, before eitherSELECT. 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
orderedByDependencywas seeded withArray(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 fromscope.objects, which already carries the user's own order.The object checkbox inverted on any write
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
sourceDefinitionsfetched every view body, routine body and trigger body before the loop appliedcanCopyDefinition, which is loop invariant and false for every object of a cross-namespace copy. On a MySQLprodtostagingcopy 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.existingTargetObjectsread the full target catalog once per source scope, and did it for data-only copies wherebuildDefinitionStepsreturns before touching it. Now gated oncontent.includesStructureand memoized per target endpoint, so twelve schemas into one target read it once.retargetedevaluated a loop-invariant guard inside its per-keymap;orderedByDependencyclosed with an O(n²)result.contains, which is 125,000 equality checks on a 500-table copy.sourceDefinitionsbuilt a whole throwawayObjectCopyRequestthat shadowed its parameter so it could read back thesourceEndpointit was already handed.Reviewed and rejected
One finding said the PostgreSQL
setvalinfetchDependentSequenceshad been unqualified in #2568 and so broke SQL export and the Structure view, which share the method.The
CREATE SEQUENCEbeside it was always unqualified. #2568 made the pair agree; before it, a dump created the sequence in the restoring session'ssearch_pathand then repositioned a different one, or errored. The current form is the correct one, and the change is left alone.Success criteria
Tests, all suites in the blast radius:
ObjectCopyPlanTestsis 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.ObjectCopyEligibilityTestsgains the four refusal cases,ObjectCopyPlannerOrderingTeststwo ordering cases,ObjectCopySessionTestsone for the binding.The ABI check is additive:
One symbol added, zero removed, a requirement with a default. No
currentPluginKitVersionbump and no plugin re-release.Not verified
verify.sh pluginsfails locally, with both of its two errors coming from an@TaskLocalmacro expansion inside the pinnedoracle-niofork. 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
TableProUITestscoverage. 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.PostgreSQLPluginDriveris over thetype_body_lengthwarning at 1149 lines and these six lines make it slightly longer. It is pre-existing,Plugins/is outside.swiftlint.yml'sincluded:, and splitting that class is its own change.