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

Add Scan and Value to pgtype.FlatArray, pgtype.Array, pgtype.Range, and pgtype.Multirange #2020

Closed
wants to merge 5 commits into from

Conversation

jackc
Copy link
Owner

@jackc jackc commented May 19, 2024

This allows using the types directly without *Map.SQLScanner.

These methods were previously not implemented because they all require a *Map to encode or decode their underlying elements. The new implementations use an internal default *Map. This means that they may not be able to directly support custom types, but normal types like pgtype.FlatArray[string] will work as expected.

@stephenafamo
Copy link
Contributor

stephenafamo commented May 19, 2024

Amazing work @jackc

Aside from normal types like pgtype.FlatArray[string], will this also work with types that implement database/sql.Scanner and driver.Valuer themselves?

@jackc
Copy link
Owner Author

jackc commented May 19, 2024

@stephenafamo Normal types that implement Scanner and Valuer were already supported. The issue this works around is the difficulty of implementing them on generic container types like arrays and ranges.

@stephenafamo
Copy link
Contributor

To be clear, what I meant is an array (or other container) of types that implement scanner/valuer

@gspeicher
Copy link

@stephenafamo I have not tested this branch directly, but the code examples in #1779, upon which this changeset appears to be based, does indeed work for FlatArray[T], Multirange[T], and Range[T] where T is a type supported by pgx such as pgtype.Date.

Copy link

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@jackc
Copy link
Owner Author

jackc commented May 21, 2024

To be clear, what I meant is an array (or other container) of types that implement scanner/valuer

@stephenafamo I'd assumed it would work. But I added a test:

func TestPGTypeFlatArrayOfScanner(t *testing.T) {
	testWithAllQueryExecModes(t, func(t *testing.T, db *sql.DB) {
		var names pgtype.FlatArray[sql.NullString]

		err := db.QueryRow("select array['John', 'Jane']::text[]").Scan(&names)
		require.NoError(t, err)
		require.Equal(t, pgtype.FlatArray[sql.NullString]{{String: "John", Valid: true}, {String: "Jane", Valid: true}}, names)

		var n int
		err = db.QueryRow("select cardinality($1::text[])", names).Scan(&n)
		require.NoError(t, err)
		require.EqualValues(t, 2, n)

		err = db.QueryRow("select null::text[]").Scan(&names)
		require.NoError(t, err)
		require.Nil(t, names)
	})
}

And it fails pretty horribly. Stack overflow / infinite recursion. All types must be registered on the connection for it to work. It just so happens all the tests I originally wrote were of types that are registered by default. 🤦

In the PostgreSQL protocol, the OID / data type of each result column is always available. The entire pgx scanning system is built around having the data type of what is being decoded. But the database/sql.Scanner interface does not provide underlying type information. In our case all we get is a string. While a concrete type can parse the string because it knows its own type, a generic type cannot.

I thought I had worked around that issue, but apparently I have not. I'm beginning to think it might not be possible, at least without an excessive amount of complexity or special cases.

@jackc
Copy link
Owner Author

jackc commented May 21, 2024

I'm tired of fighting with database/sql's scanning system. Instead, I've made a proposal to allow drivers to override Scan entirely. If this is accepted there would be complete parity of type support between pgx and database/sql. It also could improve database/sql performance.

The proposal is here: golang/go#67546. I think it would be simple to implement in Go and simple for drivers to support.

@jackc jackc closed this May 21, 2024
@stephenafamo
Copy link
Contributor

Thanks for all the work @jackc

I hope the proposal moves forward quickly, perhaps it can be in go1.24

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.

None yet

4 participants