feat(golang): support custom enum slices for pgx/v5

[toqueteos]

⚠️ EDIT(2023/07/30 22:55 GMT+2): This PR now only contains the pgx/v5 changes.

Currently queries that use arrays of custom enum types fail (in both pgx/v4 & pgx/v5) as shown in #1256

The error looks like this:
failed to encode args[1]: unable to encode []MyCustomEnumType{\"some_value\"} into text format for unknown type (OID 16877): cannot find encode plan

There's a somewhat easy solution to this issue by modifying the generated code for custom enum types to also generate implementations of EncodeText and DecodeText.

Example: #1256 (comment)

I didn't see any pluralize helper func in the codegen/golang folder so I'm using {{.Name}}s as the type name, better alternatives are welcomed!

[toqueteos]

I forced-pushed a new commit with some fixes

• Fixed the broken tests due to test-data not being properly updated (I didn't run make regen initially, my bad)
• Added a new dependency (github.com/gertd/go-pluralize) to handle pluralization
• Made code generation specific for pgx/v4 & pgx/v5
  • pgx/v4 works by implementing EncodeText & DecodeText (found it in Executing query that takes an array of a custom enum type with pgx/v4 fails #1256 (comment))
  • pgx/v5 works by calling LoadType for every enum name in the new generated EnumNames slice (found it in sqlc generated enum type not support in pgx query #2144 (comment) , see example below)

func LoadTypes(ctx context.Context, conn *pgx.Conn) error {
	for _, enum := range sqlcgeneratedpackage.EnumNames {
		t, err := conn.LoadType(ctx, enum) // register enum
		if err != nil {
			return err
		}
		conn.TypeMap().RegisterType(t)

		ta, err := conn.LoadType(ctx, "_"+enum) // register []enum
		if err != nil {
			return err
		}
		conn.TypeMap().RegisterType(ta)
	}

	return nil
}

// ...

poolCfg, err := pgxpool.ParseConfig(dsn)
if err != nil {
	return nil, fmt.Errorf("could not parse db dsn: %w", err)
}
poolCfg.AfterConnect = LoadTypes

[toqueteos]

So test pipeline is failing now due to pgx/v4 missing the pgtype import (I'll see how to cleanly add that one in).

I'm mostly interested in the changes for pgx/v5 so I may split this PR into two if the import takes me more time than expected.

Also I spotted that the toLower FuncMap func is no longer needed in template.tmpl so I'll get rid of that.

[toqueteos]

There we go, both issues are now fixed.

I also rebased with latest main as the project now lives under the sqlc-dev umbrella (congrats!) and there were some conflicts due to imports being renamed.

[andrewmbenton]

Thanks for this. It looks like a longstanding issue.

I haven't dug in too deeply yet but at a high level I think we should avoid the pluralize dependency, and scope this down to just add support for pgx/v5 which should hopefully require fewer template changes?

But speaking of that, I don't actually see any calls to LoadType() and the only test output that seems to have changed for pgx/v5 tests is in models, so is this actually working for pgx/v5? Or maybe the EncodeText and DecodeText additions make this work for both v4 and v5? If that's the case then I think we should consider keeping the single implementation that supports both, and you could remove the (currently unused) EnumNames slice from the template.

[toqueteos]

Thanks for this. It looks like a longstanding issue.

I haven't dug in too deeply yet but at a high level I think we should avoid the pluralize dependency, and scope this down to just add support for pgx/v5 which should hopefully require fewer template changes?

But speaking of that, I don't actually see any calls to LoadType() and the only test output that seems to have changed for pgx/v5 tests is in models, so is this actually working for pgx/v5? Or maybe the EncodeText and DecodeText additions make this work for both v4 and v5? If that's the case then I think we should consider keeping the single implementation that supports both, and you could remove the (currently unused) EnumNames slice from the template.

Hi Andrew!

So, the EncodeText and DecodeText additions only work with pgx/v4 (those interfaces do not exist anymore in pgx/v5).

It is true that I didn't update any examples or tests to actually prove it's being used.

The pluralize dependency is only required for the pgx/v4 code and just to generate proper type names.

As for the pgx/v5 specific code (the EnumNames string slice) ideally it would only be called withing examples and or tests as that's only meant to be used in application code as we don't know what sqlc users might want to do in their AfterConnect func.

It's not really that useful, it's more like a DX thing as users can already build a list of enum type names by themselves, but it's handier if sqlc builds it for you.

I think it may be probably best if I split this PR into two with updated tests and example code showcasing it's usage.

[andrewmbenton]

The pluralize dependency is only required for the pgx/v4 code and just to generate proper type names.

Yep understood. We just really don't want to add more dependencies unless it's strictly necessary. And in this case it is a nice-to-have rather than need-to-have I think.

As for the pgx/v5 specific code (the EnumNames string slice) ideally it would only be called withing examples and or tests as that's only meant to be used in application code as we don't know what sqlc users might want to do in their AfterConnect func.

It's not really that useful, it's more like a DX thing as users can already build a list of enum type names by themselves, but it's handier if sqlc builds it for you.

I think it may be probably best if I split this PR into two with updated tests and example code showcasing it's usage.

Yeah I think splitting into one thing that minimally gets things working for pgx/v5 is a good place to start, since that should be easier for us to merge. Then we can come back to what to do about pgx/v4.

[toqueteos]

@andrewmbenton Done, I just force pushed a new commit with just the pgx/v5 changes.
I also changed this PR's title to reflect that.

There's a new enums example folder (the first one to use pgx/v5) with some tests for this scenario.

toq@local ~/code/sqlc/examples/enums $ dotenv -- go test -v --tags=examples ./...
=== RUN   TestUsers
    db_test.go:14: db: postgres://postgres:mysecretpassword@localhost:5432/dinotest?sslmode=disable
    db_test.go:49: [{1 John Doe 42 large} {2 Dirk Sample 23 large} {3 Susan Foo 19 small} {4 Alex Brown 25 medium}]
    db_test.go:56: [{1 John Doe 42 large} {2 Dirk Sample 23 large}]
--- PASS: TestUsers (0.02s)
PASS
ok  	github.com/sqlc-dev/sqlc/examples/enums/postgresql	0.025s

[toqueteos]

Rebased with latest main and fixed the PostgreSQLPgx call (should be PostgreSQLPgxV4) in examples/batch/postgresql/db_test.go

[toqueteos]

/cc @andrewmbenton Did you have time to check the pgx/v5 specific changes? Am I missing something?

[toqueteos]

Gentle ping @andrewmbenton

Haven't heard from anybody since Aug 5, are there any blockers for this?

[andrewmbenton]

Thanks for the prod, just coming back to this.

I think I'm still a little confused about what the best solution is for pgx/v5. If the only thing sqlc does is generate a list of enum type names for use in pgx registration, then it's kind of a minor convenience (DX like you said) with the downside that sqlc output now has more surface area.

Is it not the case that sqlc could generate a proper encoding/scanning implementation for each type, such that registration wouldn't be necessary at all and enum slices would just work? I can see that it's not recommended ("While pgtype will often still work with unregistered types it is highly recommended that all types be registered due to an improvement in performance and the elimination of certain edge cases."), but I'm wondering if generating the encoding/scanning is a plausible path forward?

If the only proper solution for pgx/v5 is to load and register the enum types before querying, then I think I wouldn't recommend changing sqlc output and instead would write some documentation to explain (probably in this section: https://docs.sqlc.dev/en/stable/reference/datatypes.html#arrays).

[david-alza]

@andrewmbenton think it would be an overreach for sqlc to set up a hook or something to call from pgx to register these types? It's easy enough to write a load/register function, but it could be convenient if sqlc would set these up for you (based on the driver you're using).

You'd still have to know to call this when connecting to your DB of course.

[andrewmbenton]

@andrewmbenton think it would be an overreach for sqlc to set up a hook or something to call from pgx to register these types? It's easy enough to write a load/register function, but it could be convenient if sqlc would set these up for you (based on the driver you're using).

Not necessarily an overreach, just a question of whether the value gained is worth the addition of more go templates. If all we are doing is adding a list of custom type names to a go package so that users can more-easily iterate over the list then it doesn't really seem too valuable. But if we provided the entire LoadTypes(...) func from this comment above maybe that's worth it?

[kyleconroy]

Also see code in #2116