fix(plugins): import a SQL Server file with no GO line a statement at a time and bound the import failure alert - #3125
Merged
Conversation
… a time and bound the import failure alert
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
This branch was successfully deployed
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.
Follow-up to #3122, which is merged. A review of #3122 found three defects in it; this fixes them and a fourth found while fixing the second.
What was wrong
1. A SQL Server file with no
GOline imported as one batch. #3122 made the import parser read a SQL Server file the way sqlcmd does, and sqlcmd sends a file with noGOline as one batch. Every SQL Server dump TablePro wrote up to 0.75 is exactly that:;-terminated statements, noGO. Sent as one batch, such a dump fails at compile time and runs none of it:INSERTis compiled against the old table before theDROPruns.On main before #3122 the same files imported statement by statement and worked.
2. The import failure alert laid out the whole failing batch. The statement a stopped import reports is now a whole batch, up to 64 Mi units.
TransferReportViewput all of it in a wrappingNSTextView, and TextKit lays out a paragraph whole on the main thread. Measured with a probe that builds the same alert: a 15 M unit batch holding one 5 M unit line blocked the main thread for 67 s and reached a 2.7 GB footprint. A single huge failing statement already did this before #3122.3. The GO terminator was per-export state on a shared plugin instance.
SQLExportPlugin.statementEndwas set at the start ofexport()and read by every later phase, after many suspension points.PluginManagerhands every window the same instance, so a MySQL export started in another window during a SQL Server export dropped theGOlines from the rest of the SQL Server dump, and the reverse putGOlines into a MySQL dump.4. The failure alert's text joined every error of the batch. A SQL Server batch raises one error per failed statement and the driver keeps up to 1,000. Measured live: 1,500 duplicate-key
INSERTs in one batch came back as 1,001 errors, 166,934 units of text.NSAlertgrows to fit its informative text: with 1,000 lines it measured 54,135 points tall, its Done button far off the screen.What changed
SQLFileParsersettles how to read a SQL Server file before handing out its first statement. A file that holds aGOline (read lexically, so one inside a literal, a bracketed name or a comment does not count) is read in batches as fix(plugins): run SQL Server imports as GO batches and end each statement of a SQL Server dump with GO #3122 does. A file with none is read a statement at a time with the statement grammar, as 0.75 read it, soMERGEkeeps its;and aBEGIN...ENDroutine body stays whole. The check is a count-only pass of the same lexer in batch mode that stops at the firstGOline, so it cannot disagree with the reading. Measured on a 200 MB file with noGOline (release build): about 2 s before the first statement.executeBatch, which maps the server's line onto the file, and a dropped comment line would shift that mapping.TransferReportViewshows at most the first 10,000 units of a report, cut on a character boundary with an ellipsis. With the cap the same 15 M unit batch shows in 0.1 s at 87 MB. Copy Details still copies all of it.PluginExportDataSource.dumpStatementEnd), and nothing about it is stored on the plugin.check-mssql-sql-import.sh: a no-GOdump with a view now has to import statement by statement, plus a new case for a dump that recreates a table wider. ADECLAREscript with noGOline runs statement by statement, and the same script ending inGOkeeps its variable. The cut-length case gained aGOline so it still exercises the cut.check-mssql-merge-terminator.shimports every text twice, as written (a statement at a time) and with aGOline after it (as a batch), so both readings keep aMERGE's;against the server.databases/mssql.mdxno longer says a dump with noGOline imports as a batch, says how to run such a file as one batch (end it withGO), and lists the limitation below.features/import-export.mdxsays the same and describes the capped alert.A GO-less script that relies on batch scope, such as
DECLARE @xon one statement and@xread in the next, or anIF ... BEGIN ... ENDblock outside a routine, runs statement by statement again, as it did before #3122. OneGOline anywhere makes the whole file a batch script. The editor is unchanged and still runs such a script as one batch.Reading only TablePro's own old dumps (by their
-- TablePro SQL Exportheader) statement by statement, and every other GO-less file as one batch, was considered and rejected: a;-terminated SQL Server dump from another tool also imported on 0.75 and would fail again with Msg 111 or Msg 207.In a file with no
GOline, a procedure, function or trigger whose body is not wrapped inBEGIN...ENDkeeps only the first statement of its body, and the rest runs on its own, silently. That is what 0.75 did with the same file, and nothing in such a file says where the body ends. The docs now list it with the fix (wrap the body, or give every statement aGOline). Dumps written since #3122 carryGOlines and are not affected.Verification
verify.sh build: PASS.verify.sh plugins(AllPlugins): PASS. Both on the final commit.shellcheck --severity=warningon both scripts: clean.verify.sh teston the final commit, rebased on fix(mcp): let SQL Server scripts past the execution gate and keep GO lines out of single statements #3124, over the 22 suites that own the changed types: 243 of 243 passed (SQLFileParserBatchTests, SQLFileParserTests, SQLFileParserPLSQLTests, SQLScriptTextTests, SQLServerImportBatchTests, SqlFileImportSourceCleanupTests, TransferFailureReportTests, TransferAlertWindowOwnershipTests, SQLExportBatchSeparatorTests and 13 other SQL export and import suites).SQLFileParser.swiftandSQLExportPlugin.swiftput back to main and the view cap removed, 19 cases failed in 5 suites: every new parser, import, export-concurrency and view-cap test. With the error cap removed,manyErrorsAreCutInTheAlertButCopiedWholefailed. A parser harness with statement comments turned off fails the comment case.verify.sh linton every changed Swift file: 0 violations.verify.sh docs: PASS.pr_sql_import_go_batches:scripts/check-mssql-sql-import.shon this branch: 30 of 30 checks pass. The same script built against main's parser: 7 fail, with Msg 111 on the no-GOdump and Msg 207 "Invalid column name 'b'" on the wider table.scripts/check-mssql-merge-terminator.sh: 26 texts, 0 failures, each imported both ways.GOlines, the same as when it runs alone (10 before), and the MySQL dump written during a SQL Server export has none (10 before).exportsSharingThePluginKeepTheirOwnStatementEndsdoes the same deterministically, holding the first export inside its table fetch while the second runs to its end.Review
Codex is unavailable (usage limit), so the diff was read by
Skill(code-review)at high effort. Acted on: the error text is now capped by length as well as by lines, the MERGE check covers both import readings, a redundantprivateinside aprivate extensionis gone, and the limitation above is documented. Not acted on:GOline and once to run it, and the import dialog's statement count does the same. Measured at about 2 s per 200 MB (release build), against minutes to import the same file into SQL Server. A file with aGOline stops the scan at its first one.GOline still fails where the bad chunk is.SQLExportPlugin(exportSpansContainers,emittedSequenceNames, the failure lists,metadataWarnings,tablesUnorderedByCycle) have the same sharing flaw. It predates fix(plugins): run SQL Server imports as GO batches and end each statement of a SQL Server dump with GO #3122 and is left for its own change.Not covered
NSOpenPanelthat no suite drives today. The alert caps are tested on the realTransferReportViewand on the text the alert is given.SqlFileImportSourceCleanupTests/retryCleansUpItsOwnDecompressedFilefailed in one of the runs alongside the parser suites, passed alone (3 of 3) and passed in the final run. It lists every.sqlfile in the shared temporary directory, so files other suites write there at the same time look like leaks. It does that on main too; it is not changed here.Found while fixing #3078 (#3105).