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

Upgrade to pgx v5 #99

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Upgrade to pgx v5 #99

wants to merge 7 commits into from

Conversation

jschaf
Copy link
Owner

@jschaf jschaf commented Jan 2, 2024

#74

- For pggen row structs, implement pgx.RowScanner.
- Remove the typeResolver, since we're punting type resolution to the pgx.Conn
  type map for now.

#74
@@ -263,7 +263,7 @@ func (c CompositeInitDeclarer) Declare(string) (string, error) {
// of a composite type as a generic array: []interface{}. Necessary because we
// can only set pgtype.CompositeType from a []interface{}.
//
// Revisit after https://github.com/jackc/pgtype/pull/100 to see if we can
// Revisit after https://github.com/jackc/pgx/v5/pgtype/pull/100 to see if we can

Choose a reason for hiding this comment

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

Broken text replace

@DavidArchibald
Copy link

Hey! I was interested in getting this at least sort of working and put together a set of minimal fixes here: mypricehealth@ec3a10c

The main things fixed are that the complete removal of {{- range .Declarers}}{{- "\n\n" -}}{{ .Declare $.PkgPath }}{{ end -}} ended up removing all the struct declarations, that the pgtype.XArray types are now pgtype.FlatArray[X] or maybe pgtype.Array[X] types, and that EmitScanColumn didn't scan into sub fields.

This was enough to at least get the generation of some of the examples folder building. I might continue working on some of this but I'm wondering how much interest there is in collaboration here. There's definitely some there's a non insignificant remaining work to get it beaten into shape. I couldn't quite get it to working enough that internal/pg/query.sql.go works for example.

@kirk-anchor
Copy link

kirk-anchor commented Mar 26, 2024

@DavidArchibald I am interested in upgrading to pgx-v5. Do your changes remove support for composite types or arrays of composite types? joe/pgxv5...mypricehealth:pggen:joe/pgxv5

@DavidArchibald
Copy link

@kirk-anchor on the tip of my branch I was able to get all the code I personally needed generating which included generating structs for all the queries I needed, arrays included. I don't know if I tested composite types explicitly so I guess I'd recommend trying it out and seeing if it works for your queries.

If it's not working I'd appreciate a simple reproduction and then I'd probably be able to fix it. Though just to be explicit, I whipped this up in an afternoon and I'd actually be surprised if my set of fixes didn't in of themselves include multiple problems. What I can say is that I audited all the code I generated and it all worked for the queries I was writing but that's it. I'm aware of a handful of issues but they were in types I didn't personally use and I couldn't figure out a straightforward migration from v4 -> v5.

@kirk-anchor
Copy link

@DavidArchibald When I use a composite type, it tries to reference the now removed q.types field.

func (q *DBQuerier) InsertMyTable(ctx context.Context, myRows []MyTable) (pgconn.CommandTag, error) {
	ctx = context.WithValue(ctx, QueryName{}, "InsertMyTable")
	cmdTag, err := q.conn.Exec(ctx, insertMyTableSQL, q.types.newMyTableArrayInit(myRows))
	if err != nil {
		return pgconn.CommandTag{}, fmt.Errorf("exec query InsertMyTable: %w", err)
	}
	return cmdTag, err
}

I have queries like this.

-- name: InsertMyTable :exec
INSERT INTO my_table (my_col1, my_col2, my_col3)
SELECT
    my_col1,
    my_col2,
    my_col3
FROM
    unnest(pggen.arg('my_rows')::my_table[]);

-- name: GetMyTable :many
SELECT
    my_table
FROM
    my_table;

This way I have a common MyTable struct type to use on all queries.

	InsertMyTable(ctx context.Context, myRows []MyTable) (pgconn.CommandTag, error)
	GetMyTable(ctx context.Context) ([]MyTable, error)

@kirk-anchor
Copy link

Looks like on pgx-v5, I can get the composite type with dt, err := conn.LoadType(ctx, "my_table")
https://github.com/jackc/pgx/blob/60a01d044a5b3f65b9eea866954fdeea1e7d3f00/pgtype/composite_test.go#L231

@jschaf Do you plan to continue development? I may see if adding composite types to the fork will get my queries working. Arrays use Go generics now so arrays of composites should work too. Do you plan to upstream the changes from the fork or take another approach?

@kirk-anchor
Copy link

I was able to get the composite type example queries I provided working. Also fixed some bugs. mypricehealth/pggen@joe/pgxv5...kirk-anchor:pggen:pgxv5

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.

3 participants