fix: SQL export/import round-trip for dropped statements and non-table objects#1264
Merged
Conversation
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.
Fixes two bugs that broke exporting a database to SQL and importing it back. Both come from #1114.
1. SQL import silently dropped statements
SQLFileParser.parseFile()fed its statement stream throughAsyncThrowingStream(bufferingPolicy: .bufferingNewest(8)). That is a dropping buffer, not backpressure. A detached parser task raced through the file while the consumer did one DB round-trip per statement, so whenever the 8-slot buffer filled, the oldest statement was discarded. The import ran a sparse, timing-dependent subset of the file. A re-imported export failed with errors likerelation "public.ses_email_events" does not existbecauseCREATE TABLEwas dropped while a laterCREATE INDEXon it survived. It also explains wrong "N statements" counts in the import preview.Rewrote
parseFileto use the pull-basedAsyncThrowingStream(unfolding:). AParseSessionholds the parse state and produces one statement pernext()call, so the consumer drives production: natural backpressure, no detached task racing ahead, bounded memory (~one 64 KB chunk), zero drops. The tokenizer logic is unchanged.Also fixed a second bug from the same area:
SqlFileImportSource.statements()never forwardeddialectto the parser, so PostgreSQL dumps were parsed as.genericand dollar-quoted function bodies were split at semicolons inside the body.2. SQL export emitted
DROP TABLEfor non-tablesThe export drop phase hardcoded
DROP TABLE IF EXISTSfor every object. Re-importing a dump that contained a materialized view failed with"daily_revenue" is not a table. HINT: Use DROP MATERIALIZED VIEW. Root cause was upstream:ExportServicecollapsed every object type to"table"or"view"before the export plugin saw it, so materialized views and foreign tables arrived labeled"table".ExportServicenow passes the real object type through.SQLExportPlugin.writeDropPhasepicks the matching keyword:DROP VIEW,DROP MATERIALIZED VIEW,DROP FOREIGN TABLE, orDROP TABLE. The data and finalization phases are unchanged.Known limitation, not addressed here: the create phase still emits
CREATE TABLEfor views and materialized views (pre-existing behavior), so a materialized view round-trips back as a regular table holding the snapshot data. Emitting realCREATE [MATERIALIZED] VIEW ... AS <query>is a larger change for a follow-up.Tests
slow_consumer_no_dropped_statements- 200 statements with a slow consumer; fails on the old dropping buffer, deterministic on the fix.count_statements_across_chunk_boundary- 4,000 statements spanning multiple read chunks.large_multi_row_insert_correctness(literal$0where\($0)was meant for the id column) - it fails onmainwhenever it actually runs in the suite.SQLFileParserTestssuite green across 3 consecutive runs; app builds;swiftlint --strictclean.The export plugin builds as a separate
.tablepluginbundle and the repo has no unit-test harness for plugin targets, so that change is build-verified.