rows.Next() API is easy to misuse and hard to spot mistakes with, pgx can seem to successfully execute query but return no rows or fewer than expected

@jackc

I recently found a bug affecting many golang services at my work that suggests the rows.Next() API might be confusing and easy to use incorrectly. I suspect this will affect many others, so I'm raising this issue partly to make it searchable.

If your pgx-using code seems to execute a query successfully and does not return an error, but the returned result has zero rows or fewer rows than expected when you know the data really is in the database, misuse of the rows.Next() API in your code could be why.

rows.Next() can return false if there are no more rows or if an error occurred so you must always check for rows.Err() before returning.

Edit: @jackc informs me that this API is borrowed from database/sql and is provided for easy code conversion and compatibility. So that makes sense. It'd be good if the docs highlighted the hazards a bit better, and the higher-level V5 APIs look like a definite improvement.

I'll close this issue, but hopefully having it searchable will help others.

The following pattern looks fine

def doSomething(...) (someResultType*, err) {
  rows, err := conn.Query(...)
  if err != nil {
    return []someResultType{}, err
  }
  var results someResultType;
  while rows.Next() {
     addToResult(&rows, results);
  }
  return &results, nil
}

It's obviously and visibly checking for an error running the query. The natural assumption of the reader is that rows.Next() cannot fail, as it has no error to return. This would make sense if pgx like many other client drivers read the entire query result set and processed it before returning from Query().

In reality pgx defers result-set processing until after the query execution returns. So some errors can be returned from Query() but many will be delayed until result-set processing, e.g. incomplete result sets due to connection loss, result data not matching expected types, postgres error in the protocol stream instead of expected end-of-results marker, etc. But this is very much non-obvious to the reader of the code.

Since there's no error return value to check, this won't be spotted by casual code review of someone not closely familiar with the pgx API. Not can it be easily detected by common code scanners and static analyis tools, unless they're specially configured to "know" that any false return of pgx rows.Next() must be followed by a test for if rows.Err() != nil.

  while rows.Next() {
     addToResult(&rows, results);
  }
  if rows.Err() != nil {
    return nil, fmt.Errorf("while processing result rows in doSomething(): %w", rows.Err());
  }

Obviously this API cannot be changed now, and it's a convenient interface.

So it'd be helpful if examples and docs used this pattern:

// error will be wrapped into rows object
rows, _ := conn.Query(...)
while rows.Next() {
  // use result
}
if rows.Err() {
  return rows.Err()
}

so it's obvious that error checking must be done during the rows processing loop, and doing it on the query result is insufficient.
But it'd be helpful to add an alternative that forces error checking, and use that alternative in the docs.

An API addition for a variant that explicitly surfaces the error might also be quite helpful for clarity and exposure to static analysis tools. Users could then treat rows.Next() as a deprecated interface, and prefer the new rows.NextOrErr() or whatever, which returns both a "rows remaining" flag and an error.

The specific pattern in the code I found an issue with looked like this:

	rows, err := poolinstance.Query(ctx, sql, args...)
	if err != nil || rows == nil {
		return nil, err
	}
	defer rows.Close()

	if ok := rows.Next(); !ok {
		return nil, nil
	}

	someResult, err := scanRowsSomeResult(rows)
	if err != nil {
		return nil, err
	}

where scanRowsSomeResult(...) does

	if err := rows.Scan(...) ; err != nil {
		return nil, err
	}

When reviewing this code it was not at all obvious that this stanza

        if ok := rows.Next(); !ok {
		return nil, nil
	}

could cause the function to return nil, nil when there was an error executing the query or fetching results`, and needed to be

        if ok := rows.Next(); !ok {
		return nil, rows.Err()
	}

instead.

Someone mentioned that this may already be addressed in the v5 api. I didn't find a clear high level summary of API changes yet and don't see anything obvious based on https://github.com/jackc/pgx/blob/master/CHANGELOG.md#v500-september-17-2022 though.

Yes, I agree that the Query(), rows.Next(), rows.Err() sequence is too awkward and error prone for most uses. But that type interface is necessary to process rows one at at time. Even though it is usually best to buffer results before processing, a streaming interface needs to exist. In addition, it was designed to match database/sql as closely as possible.

But for most use cases the higher level interfaces in v5 should be used.

Someone mentioned that this may already be addressed in the v5 api.

https://github.com/jackc/pgx/blob/master/CHANGELOG.md#rows-result-helpers is probably what is being referred to.

Also, note that it is the recommended approach in the docs: https://pkg.go.dev/github.com/jackc/pgx/v5#hdr-Query_Interface.

For an even higher level abstraction take a look at https://pkg.go.dev/github.com/jackc/pgxutil#Select in my personal pgx helper library.

I'm happy to close this by the way; I've mostly posted it for search/discovery and visibility. It makes more sense if it's copying another API. I haven't used database/sql so I wasn't aware. I'll edit the original issue to reflect that parallel.

Higher-level V5 APIs improve on this already. Existing interface inherited from database/sql. Docs don't really discuss rows type, rows.Next() etc much at all, so not a ton to improve there.

docs: Emphasise need to call rows.Err() after rows.Next() returns false

I've raised #1708 with a small docs-only PR to make it a smidge more obvious. Though I suspect that people who read the docs on the API aren't the ones who'll hit the issue in the first place, it doesn't hurt to try.

The docs on conn.Query() already cover this pretty well so it's mostly just a short note and a pointer to those.

docs: Emphasise need to call rows.Err() after rows.Next() returns false

@jackc

Thanks very much @jackc

Navigation Menu

Repository navigation

rows.Next() API is easy to misuse and hard to spot mistakes with, pgx can seem to successfully execute query but return no rows or fewer than expected #1707

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

rows.Next() API is easy to misuse and hard to spot mistakes with, pgx can seem to successfully execute query but return no rows or fewer than expected #1707

Description

Activity

ringerc commented on Aug 9, 2023

ringerc commented on Aug 10, 2023

jackc commented on Aug 10, 2023

ringerc commented on Aug 10, 2023

ringerc commented on Aug 10, 2023

ringerc commented on Aug 10, 2023

ringerc commented on Aug 10, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions