Description
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.