Skip to content

fix(plugins): import a SQL Server file with no GO line a statement at a time and bound the import failure alert - #3125

Merged
datlechin merged 1 commit into
mainfrom
fix/sql-import-go-less-scripts
Sep 24, 2026
Merged

datlechin merged 1 commit into
mainfrom
fix/sql-import-go-less-scripts

Conversation

@datlechin

Copy link
Copy Markdown
Member

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 GO line imported as one batch. #3122 made the import parser read a SQL Server file the way sqlcmd does, and sqlcmd sends a file with no GO line as one batch. Every SQL Server dump TablePro wrote up to 0.75 is exactly that: ;-terminated statements, no GO. Sent as one batch, such a dump fails at compile time and runs none of it:

  • a view, routine or trigger after the tables fails with Msg 111, "'CREATE VIEW' must be the first statement in a query batch";
  • a dump that drops and recreates a table wider than the one in the database fails with Msg 207, because the INSERT is compiled against the old table before the DROP runs.

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. TransferReportView put all of it in a wrapping NSTextView, 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.statementEnd was set at the start of export() and read by every later phase, after many suspension points. PluginManager hands every window the same instance, so a MySQL export started in another window during a SQL Server export dropped the GO lines from the rest of the SQL Server dump, and the reverse put GO lines 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. NSAlert grows to fit its informative text: with 1,000 lines it measured 54,135 points tall, its Done button far off the screen.

What changed

  • SQLFileParser settles how to read a SQL Server file before handing out its first statement. A file that holds a GO line (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, so MERGE keeps its ; and a BEGIN...END routine body stays whole. The check is a count-only pass of the same lexer in batch mode that stops at the first GO line, so it cannot disagree with the reading. Measured on a 200 MB file with no GO line (release build): about 2 s before the first statement.
  • A SQL Server statement read that way keeps the comments written inside it, as a batch does. The import sends it through executeBatch, which maps the server's line onto the file, and a dropped comment line would shift that mapping.
  • TransferReportView shows 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.
  • The failure alert's text holds at most five error lines and 1,000 units, then "Copy Details copies the rest." The alert is 423 points tall with 1,000 errors.
  • The statement end comes from the data source each export writes from (PluginExportDataSource.dumpStatementEnd), and nothing about it is stored on the plugin.
  • check-mssql-sql-import.sh: a no-GO dump with a view now has to import statement by statement, plus a new case for a dump that recreates a table wider. A DECLARE script with no GO line runs statement by statement, and the same script ending in GO keeps its variable. The cut-length case gained a GO line so it still exercises the cut.
  • check-mssql-merge-terminator.sh imports every text twice, as written (a statement at a time) and with a GO line after it (as a batch), so both readings keep a MERGE's ; against the server.
  • Docs: databases/mssql.mdx no longer says a dump with no GO line imports as a batch, says how to run such a file as one batch (end it with GO), and lists the limitation below. features/import-export.mdx says the same and describes the capped alert.
  • CHANGELOG: the fix(plugins): run SQL Server imports as GO batches and end each statement of a SQL Server dump with GO #3122 line about dumps no longer claims 0.75 imports failed (they did not), and one Fixed line for the alert hang, which 0.75 already had for a huge single statement. Defects 1, 3 and 4 are regressions in unreleased fix(plugins): run SQL Server imports as GO batches and end each statement of a SQL Server dump with GO #3122 work, so they get no line of their own.

A GO-less script that relies on batch scope, such as DECLARE @x on one statement and @x read in the next, or an IF ... BEGIN ... END block outside a routine, runs statement by statement again, as it did before #3122. One GO line 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 Export header) 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 GO line, a procedure, function or trigger whose body is not wrapped in BEGIN...END keeps 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 a GO line). Dumps written since #3122 carry GO lines and are not affected.

Verification

  • verify.sh build: PASS. verify.sh plugins (AllPlugins): PASS. Both on the final commit. shellcheck --severity=warning on both scripts: clean.
  • verify.sh test on 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).
  • Red runs. With SQLFileParser.swift and SQLExportPlugin.swift put 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, manyErrorsAreCutInTheAlertButCopiedWhole failed. A parser harness with statement comments turned off fails the comment case.
  • verify.sh lint on every changed Swift file: 0 violations. verify.sh docs: PASS.
  • Live, against SQL Server (Azure SQL Edge 15) in database pr_sql_import_go_batches:
    • scripts/check-mssql-sql-import.sh on this branch: 30 of 30 checks pass. The same script built against main's parser: 7 fail, with Msg 111 on the no-GO dump 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.
  • The reviewer's concurrency probe, rebuilt from these sources: the SQL Server dump written while a MySQL export ran has 20 GO lines, the same as when it runs alone (10 before), and the MySQL dump written during a SQL Server export has none (10 before). exportsSharingThePluginKeepTheirOwnStatementEnds does 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 redundant private inside a private extension is gone, and the limitation above is documented. Not acted on:

  • Header-based reading of GO-less files: rejected for the reason above.
  • The import now reads a GO-less file twice, once for a GO line 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 a GO line stops the scan at its first one.
  • A byte the chosen encoding cannot decode now fails a GO-less SQL Server file before its first statement, where main ran every statement before the bad chunk. A file with an early GO line still fails where the bad chunk is.
  • The other per-export fields on the shared 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

  • No UI automation. The import flows here need a SQL Server connection, which a UI test cannot provide, and an import starts from an NSOpenPanel that no suite drives today. The alert caps are tested on the real TransferReportView and on the text the alert is given.
  • SqlFileImportSourceCleanupTests/retryCleansUpItsOwnDecompressedFile failed in one of the runs alongside the parser suites, passed alone (3 of 3) and passed in the final run. It lists every .sql file 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).

@mintlify

mintlify Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

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

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 24, 2026, 6:49 PM

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

This branch was successfully deployed

1 active deployment
staging - docs — f9b66a7d Deployed Sep 24, 2026 by mintlify[bot]
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