Skip to content

mysql: keep REPLACE INTO when generating a :copyfrom LOAD DATA - #4559

Open
hdimer wants to merge 1 commit into
sqlc-dev:mainfrom
hdimer:mysql-copyfrom-replace
Open

mysql: keep REPLACE INTO when generating a :copyfrom LOAD DATA#4559
hdimer wants to merge 1 commit into
sqlc-dev:mainfrom
hdimer:mysql-copyfrom-replace

Conversation

@hdimer

@hdimer hdimer commented Aug 17, 2026

Copy link
Copy Markdown

Fixes #4339.

The bug

A MySQL :copyfrom query written as REPLACE INTO generated the same statement as an INSERT INTO one:

LOAD DATA LOCAL INFILE '%s' INTO TABLE `locations` %s (id, name, address)

MySQL's default for LOAD DATA is 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 in dolphin.convertInsertStmt, and honored by InsertStmt.Format (mirroring how the sibling DefaultValues is handled)
  • compiler.Query.InsertIsReplace(), derived from the raw statement rather than stored, since both Query construction sites already keep RawStmt
  • plugin.Query.insert_is_replace (new field 9, append-only), regenerated with buf generate
  • the go-sql-driver-mysql copyfrom template, which now emits REPLACE INTO TABLE

The 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

  • MySQL only, deliberately. The flag is set only in the dolphin converter; SQLite also accepts REPLACE INTO, but only the go-sql-driver-mysql template emits LOAD DATA, and :copyfrom is already confined to pgx and that driver.
  • It cannot fire by accident. marino keeps IsReplace, IgnoreErr and OnDuplicate as separate fields, so neither INSERT IGNORE nor ON DUPLICATE KEY UPDATE sets the bit, and InsertIsReplace is read only inside the :copyfrom guard in the template.
  • Adjacent gap, left alone on purpose: INSERT ... ON DUPLICATE KEY UPDATE with :copyfrom has the same silent-drop problem. validateCopyfrom rejects postgres' ON CONFLICT via stmt.OnConflictClause, but dolphin puts MySQL's upsert in OnDuplicateKeyUpdate, which nothing checks, so it generates a plain LOAD DATA too. 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/mysql rather than a new case directory, since its config is identical to the existing one and its InsertValues / InsertSingleValue queries 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 fails TestReplay/base/copyfrom/mysql; it also passes under the coreanalyzer context, so the compiler half is genuinely exercised.

Ran locally against live PostgreSQL and MySQL: go test --tags=examples -timeout 20m ./... (with sqlc-gen-json on $PATH, so the process-plugin goldens are not skipped), go build ./..., go vet ./..., gofmt, cd internal/endtoend/testdata && go build ./..., and buf lint. The two gen/codegen.json goldens pick up "insert_is_replace": false, which is the JSON codegen emitting defaults.

Before proposing the proto change I confirmed buf generate reproduces the committed codegen.pb.go byte-for-byte on unmodified input, so the regenerated file is only the new field.

One thing to flag honestly: InsertStmt.Format is included for consistency, but nothing exercises that path today (internal/x/expander is unwired, and TestFormat's MySQL fingerprint round-trips through Format on both sides, so it cannot fail on a dropped keyword). Without it, an AST formatter that renders REPLACE INTO as INSERT INTO would 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.

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
hdimer marked this pull request as ready for review August 17, 2026 11:57
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.

Using REPLACE instead of INSERT with :copyfrom in MySQL doesn't add REPLACE to resulting LOAD DATA LOCAL INFILE query

1 participant