Error during Query inner exec is not observed until Close()

[jameshartig]

Is your feature request related to a problem? Please describe.
Not a problem necessarily but behavior that could be improved in my opinion.

When conn.Query calls PgConn.ExecParams or PgConn.ExecPrepared they return a *ResultReader that might be concluded and containing an error but the resulting Rows that is returned does not contain any error. Ideally an error would be returned from the Query call or at least an errored rows.

To be honest, this is hard to test since it requires precise timing of the server erroring. But we were able to reproduce this scenario:

1. conn.Query is called
2. PgConn.ExecParams or PgConn.ExecPrepared is called
3. PgConn.execExtendedSuffix is called
4. ResultReader.readUntilRowDescription is called
5. pgConn.receiveMessage returns a *pgproto3.ErrorResponse which calls ResultReader.concludeCommand
6. PgConn.ExecParams or PgConn.ExecPrepared now returns a concluded ResultReader with a stored err
7. conn.Query returns an unclosed *baseRows (without an err set) and no error
8. baseRows.Next() returns false
9. Err() returns non-nil

This was actually noticed because FieldDescriptions returned nil despite Err() returning nil which was confusing some code we have to map fields.

Describe the solution you'd like
I think conn.Query could check if the command is concluded and there was an error and then call rows.fatal.

Describe alternatives you've considered
There isn't necessarily anything wrong with the current behavior but I would just say that it was unexpected that an error was encountered and yet Err() returned nil for the rows.

Additional context
We are using this driver with Yugabyte. I don't think the code path above is yb-specific but we haven't tried to reproduce it with pg.

[jackc]

Hmm... That is a little tricky.

The only way I can think of for an error to be returned before the RowDescription message (which is how you would get a nil FieldDescriptions is for the query to be invalid. I was able to duplicate this behavior with the following test, but only in QueryExecModeExec.

func TestQueryReturnsErrorEarlier(t *testing.T) {
	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()

	pgxtest.RunWithQueryExecModes(ctx, t, defaultConnTestRunner, nil, func(ctx context.Context, t testing.TB, conn *pgx.Conn) {
		rows, err := conn.Query(context.Background(), "select * from foobar")
		require.Error(t, err)
		require.Error(t, rows.Err())
		rows.Close()
		require.Error(t, rows.Err())
	})
}

I took a quick look to see if it could be changed at the pgx.Conn.Query layer, but it doesn't look possible as pgconn.ResultReader does not expose the error aside from the return value of Close().

I'm not totally against adding an Err method to ResultReader, but the existing design is intentional. The pattern that pgx.Conn.Query uses was chosen to be as similar to database/sql as possible. But I believe that returning an error from pgx.Conn.Query and even the Rows.Err method is a sub-optimal design. It is a recurring foot-gun, especially for new users, to check the error returned from Query and assume that the query was successful. Neglecting to check Rows.Err after closing the rows is also a common mistake. As is checking Rows.Err before reading the entire result set and not realizing an error could occur later.

But the way pgconn.PgConn.Exec* and pgconn.ResultReader work remove these potential misuses. The pgconn.ResultReader must be closed and that is the only way to check for an error (i.e. after the results have been completely read). Unfortunately, a side effect of that is that FieldDescriptions can be nil before the calling code has any way to determine an error has occurred.

Would better / more documentation of this behavior be sufficient?

[jameshartig]

The only way I can think of for an error to be returned before the RowDescription message (which is how you would get a nil FieldDescriptions is for the query to be invalid.

This happened to us if the database was shutdown after receiving the query but before returning anything. The error that's returned is a shutdown error. It's very hard to reproduce by itself but when you're issuing hundreds or thousands of queries per second it will happen to several of them, at least from what we've seen in practice.

But I believe that returning an error from pgx.Conn.Query and even the Rows.Err method is a sub-optimal design. It is a recurring foot-gun, especially for new users, to check the error returned from Query and assume that the query was successful.

I agree. Unfortunately there's a lack of documentation in many places, including database/sql. We didn't realize that FieldDescriptions could ever return nil which ended up causing this issue for us in production. We now have a hacky check (below) to determine if there was an error but it just isn't exposed yet.

if rows != nil && err == nil && rows.FieldDescriptions() == nil {

If we didn't need the field descriptions then this isn't as big of a deal because we would just do the normal Next()->Scan()->Close() dance and somewhere along the way we would get the error.

The quickest solution would be to document that FieldDescriptions might be nil but I'm not really sure what could be done if you need them to handle the rows themselves other than just close the rows and give up.

Longer-term it might be good to clarify the states that could be returned from Query(). As it stands, there might be better clarification around err == nil doesn't necessarily mean the query did not fail it's just that it didn't fail early enough and that the only way to truly know if it failed is to call Close().

[jackc]

Longer-term it might be good to clarify the states that could be returned from Query(). As it stands, there might be better clarification around err == nil doesn't necessarily mean the query did not fail it's just that it didn't fail early enough and that the only way to truly know if it failed is to call Close().

This is already the first paragraph of the docs for the Conn.Query method:

// Query sends a query to the server and returns a Rows to read the results. Only errors encountered sending the query
// and initializing Rows will be returned. Err() on the returned Rows must be checked after the Rows is closed to
// determine if the query executed successfully.

However, I added some additional documentation to that method and to the Rows interface: 5f28621

[jameshartig]

Thanks @jackc I appreciate the clarifications.