Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

:copyfrom for MySQL LOAD DATA INFILE #2220

Closed
wants to merge 1 commit into from
Closed

Conversation

Jille
Copy link
Contributor

@Jille Jille commented Apr 20, 2023

Similar to PostgreSQL :copyfrom support, this adds support for MySQL's LOAD DATA INFILE.

Usage is the same as PostgreSQL: by writing an INSERT INTO query with :copyfrom, and we'll generate a method that internally calls LOAD DATA INFILE.

The only MySQL driver I found that supports LOAD DATA LOCAL INFILE at all is github.com/go-sql-driver/mysql. In able to support other drivers that might add support in the future, the sql_package config setting must be set to go-sql-driver/mysql.

@Jille Jille force-pushed the loaddata2 branch 5 times, most recently from b906c49 to 759860f Compare April 25, 2023 07:38
@kyleconroy kyleconroy added this to the v1.19.0 milestone Apr 27, 2023
@kyleconroy
Copy link
Collaborator

@Jille I'm assuming this PR is still in draft because of some upstream mysql driver issues?

@kyleconroy kyleconroy removed this from the v1.19.0 milestone Jun 8, 2023
@Jille
Copy link
Contributor Author

Jille commented Jun 8, 2023

Yeah, I need go-sql-driver/mysql to expose the *time.Location to encode correctly. But the maintainers don't seem to want to expose it in a way sqlc can use it.

How would you feel about an extra argument to the :loaddata methods that receives the *time.Location with the timezone configured for the connection? It's ugly, but I'm not sure I see another way yet.

@kyleconroy
Copy link
Collaborator

How would you feel about an extra argument to the :loaddata methods that receives the *time.Location with the timezone configured for the connection? It's ugly, but I'm not sure I see another way yet.

I don't think that will play nicely with the query_parameter_limit configuration option. We could create a separate struct that has a location as a field? Not sure honestly.

This enables the :copyfrom query annotation for people using
go-sql-driver/mysql that transforms it into a LOAD DATA LOCAL INFILE.

We don't have a way to get the timezone from the connection, so I've
simply blocked people from using time.Times in their copyfrom.

issue sqlc-dev#2179
@Jille
Copy link
Contributor Author

Jille commented Jun 25, 2023

We haven't found a way to expose the timezone from go-sql-driver/mysql yet. Let's move ahead with this and simply block time.Time values for now.

@Jille Jille marked this pull request as ready for review June 25, 2023 21:18
Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small changes, but other than that I think this is ready to go. I wanted to generate static queries, but it turns out that LOAD DATA doesn't work with parameters. Maybe something we can do in the future.

@@ -186,6 +186,8 @@ import (
{{define "copyfromCode"}}
{{if .SQLDriver.IsPGX }}
{{- template "copyfromCodePgx" .}}
{{else}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of {{else}}, let's check for the MySQL driver explicitly.

Comment on lines +26 to +28
// {{.MethodName}} uses MySQL's LOAD DATA LOCAL INFILE and is not atomic. Errors and duplicate keys are treated as warnings and insertion will continue, even without an error for some cases.
// Use this in a transaction and use SHOW WARNINGS to check for any problems and roll back if you want to.
// This is a MySQL limitation, not sqlc. Check the documentation for more information: https://dev.mysql.com/doc/refman/8.0/en/load-data.html#load-data-error-handling
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is too wide. Can we edit it to be this instead?

// {{.MethodName}} uses MySQL's LOAD DATA LOCAL INFILE and is not atomic.
//
// Errors and duplicate keys are treated as warnings and insertion will
// continue, even without an error for some cases.  Use this in a transaction
// and use SHOW WARNINGS to check for any problems and roll back if you want to.
//
// Check the documentation for more information:
// https://dev.mysql.com/doc/refman/8.0/en/load-data.html#load-data-error-handling

mysql.RegisterReaderHandler(rh, func() io.Reader { return pr })
defer mysql.DeregisterReaderHandler(rh)
go convertRowsFor{{.MethodName}}(pw, {{.Arg.Name}})
result, err := {{if (not $.EmitMethodsWithDBArgument)}}q.{{end}}db.ExecContext(ctx, fmt.Sprintf("LOAD DATA LOCAL INFILE '%s' INTO TABLE {{.TableIdentifierForMySQL}} %s ({{range $index, $name := .Arg.ColumnNames}}{{if gt $index 0}}, {{end}}{{$name}}{{end}})", "Reader::" + rh, mysqltsv.Escaping))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment here explaining that LOAD DATA can't be used with parameters and link to this part of the docs

The LOAD DATA INFILE statement reads rows from a text file into a table at a very high speed. The file name must be given as a literal string.

@kyleconroy kyleconroy added this to the v1.20.0 milestone Jul 28, 2023
@kyleconroy
Copy link
Collaborator

Going to get this ready to merge in #2545. Thanks for the initial implementation :)

@kyleconroy
Copy link
Collaborator

Closed via #2545

@kyleconroy kyleconroy closed this Jul 30, 2023
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.

2 participants