mysql: keep REPLACE INTO when generating a :copyfrom LOAD DATA - #4559
Open
hdimer wants to merge 1 commit into
Open
mysql: keep REPLACE INTO when generating a :copyfrom LOAD DATA#4559hdimer wants to merge 1 commit into
hdimer wants to merge 1 commit into
Conversation
A :copyfrom query written as REPLACE INTO generated LOAD DATA LOCAL INFILE '%s' INTO TABLE ..., dropping the REPLACE. MySQL's default for LOAD DATA is to skip rows that collide on a primary or unique key, so the upsert the query asked for silently became a skip. The parser already reports it (marino ast.InsertStmt.IsReplace), but nothing read the bit. Carry it on ast.InsertStmt, derive it from the raw statement in the compiler, add it to plugin.Query, and emit REPLACE INTO TABLE from the template. Fixes sqlc-dev#4339
hdimer
marked this pull request as ready for review
August 17, 2026 11:57
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 #4339.
The bug
A MySQL
:copyfromquery written asREPLACE INTOgenerated the same statement as anINSERT INTOone:MySQL's default for
LOAD DATAis to skip rows that collide on a primary or unique key (with a warning), so the upsert the query asked for silently became a skip. It generates without error, compiles, and only shows up as missing writes at runtime.The fix
marino already reports it as
ast.InsertStmt.IsReplace; nothing downstream read the bit. This carries it through:ast.InsertStmt.IsReplace, set from the parser indolphin.convertInsertStmt, and honored byInsertStmt.Format(mirroring how the siblingDefaultValuesis handled)compiler.Query.InsertIsReplace(), derived from the raw statement rather than stored, since bothQueryconstruction sites already keepRawStmtplugin.Query.insert_is_replace(new field 9, append-only), regenerated withbuf generatego-sql-driver-mysqlcopyfrom template, which now emitsREPLACE INTO TABLEThe generated doc comment is also branched: "Errors and duplicate keys are treated as warnings" is false on a REPLACE method, where collisions overwrite rather than skip. The non-REPLACE branch is byte-identical to the current text, so no existing golden moved.
Scope notes
REPLACE INTO, but only thego-sql-driver-mysqltemplate emitsLOAD DATA, and:copyfromis already confined to pgx and that driver.IsReplace,IgnoreErrandOnDuplicateas separate fields, so neitherINSERT IGNOREnorON DUPLICATE KEY UPDATEsets the bit, andInsertIsReplaceis read only inside the:copyfromguard in the template.INSERT ... ON DUPLICATE KEY UPDATEwith:copyfromhas the same silent-drop problem.validateCopyfromrejects postgres'ON CONFLICTviastmt.OnConflictClause, but dolphin puts MySQL's upsert inOnDuplicateKeyUpdate, which nothing checks, so it generates a plainLOAD DATAtoo. Rejecting it is a three-line guard, but it turns something that currently "works" into a hard error, so it seemed like your call rather than mine. Happy to add it here or in a separate PR.Testing
Coverage went into
internal/endtoend/testdata/copyfrom/mysqlrather than a new case directory, since its config is identical to the existing one and itsInsertValues/InsertSingleValuequeries act as controls that the flag does not leak onto sibling queries in the same package. Reverting either the converter line or the template line failsTestReplay/base/copyfrom/mysql; it also passes under thecoreanalyzercontext, so the compiler half is genuinely exercised.Ran locally against live PostgreSQL and MySQL:
go test --tags=examples -timeout 20m ./...(withsqlc-gen-jsonon$PATH, so the process-plugin goldens are not skipped),go build ./...,go vet ./...,gofmt,cd internal/endtoend/testdata && go build ./..., andbuf lint. The twogen/codegen.jsongoldens pick up"insert_is_replace": false, which is the JSON codegen emitting defaults.Before proposing the proto change I confirmed
buf generatereproduces the committedcodegen.pb.gobyte-for-byte on unmodified input, so the regenerated file is only the new field.One thing to flag honestly:
InsertStmt.Formatis included for consistency, but nothing exercises that path today (internal/x/expanderis unwired, andTestFormat's MySQL fingerprint round-trips throughFormaton both sides, so it cannot fail on a dropped keyword). Without it, an AST formatter that rendersREPLACE INTOasINSERT INTOwould reintroduce this exact bug one layer up whenever the expander does get wired in. Glad to drop it if you would rather keep the diff to the codegen path.I used an AI coding assistant while working on this. I reproduced the bug, ran the suite, and reviewed every line myself.