Not clear if caller is required to close in case of QueryRow or if QueryRow properly closes in case of error

[joelpresence]

Hi,

Thanks for pgx - it's awesome and I'm really enjoying using it.

I'm trying to be paranoid about closing connections (good) to prevent potential resource leaks, but it's not clear to me if I'm required to call close after using pgx's QueryRow("").Scan() combination of methods?

For example, in your godoc example, I see:

var name string
var weight int64
err := conn.QueryRow("select name, weight from widgets where id=$1", 42).Scan(&name, &weight)
if err != nil {
    return err
}

However, when I look at the implementation of Row.Scan here, it's not clear to me that it closes the Rows object properly in case of error:

func (r *Row) Scan(dest ...interface{}) (err error) {
	rows := (*Rows)(r)

        // I'd expect to defer the close here
        // defer rows.Close()

	if rows.Err() != nil {
		return rows.Err()
	}

	if !rows.Next() {
		if rows.Err() == nil {
			return ErrNoRows
		}
		return rows.Err()
	}

	rows.Scan(dest...)
        // Why close here?  What happens if this is never reached due to an error above?
	rows.Close()
	return rows.Err()
}

I could totally be missing something, sorry if I am, but I don't see how rows is closed properly if there's an error returned before the close is reached at the bottom. Why not defer the close at the top of the Scan method implementation.

If the idiom is to write QueryRow("").Scan() then I don't ever access the underlying Rows object, so I have no real ability to close it cleanly.

What do you think?

Thanks!

[joelpresence]

For example, in the native database/sql golang pkg, the close is deferred right near the top of the method, here:

func (r *Row) Scan(dest ...interface{}) error {
  2462		if r.err != nil {
  2463			return r.err
  2464		}
  2465	
  2466		// TODO(bradfitz): for now we need to defensively clone all
  2467		// []byte that the driver returned (not permitting
  2468		// *RawBytes in Rows.Scan), since we're about to close
  2469		// the Rows in our defer, when we return from this function.
  2470		// the contract with the driver.Next(...) interface is that it
  2471		// can return slices into read-only temporary memory that's
  2472		// only valid until the next Scan/Close. But the TODO is that
  2473		// for a lot of drivers, this copy will be unnecessary. We
  2474		// should provide an optional interface for drivers to
  2475		// implement to say, "don't worry, the []bytes that I return
  2476		// from Next will not be modified again." (for instance, if
  2477		// they were obtained from the network anyway) But for now we
  2478		// don't care.
  2479		defer r.rows.Close()
  2480		for _, dp := range dest {
  2481			if _, ok := dp.(*RawBytes); ok {
  2482				return errors.New("sql: RawBytes isn't allowed on Row.Scan")
  2483			}
  2484		}
  2485	
  2486		if !r.rows.Next() {
  2487			if err := r.rows.Err(); err != nil {
  2488				return err
  2489			}
  2490			return ErrNoRows
  2491		}
  2492		err := r.rows.Scan(dest...)
  2493		if err != nil {
  2494			return err
  2495		}
  2496		// Make sure the query can be processed to completion with no errors.
  2497		if err := r.rows.Close(); err != nil {
  2498			return err
  2499		}
  2500	
  2501		return nil
  2502	}

[jackc]

func (r *Row) Scan(dest ...interface{}) (err error) {
	rows := (*Rows)(r)

	if rows.Err() != nil {
		return rows.Err()
	}

rows.Err() is only already set if there was an error sending the query. rows is already closed in this state.

	if !rows.Next() {
		if rows.Err() == nil {
			return ErrNoRows
		}
		return rows.Err()
	}

Close is called automatically by Next when all rows are read. If an error occurs Next calls Fatal which closes the rows.

	rows.Scan(dest...)
	rows.Close()
	return rows.Err()
}

[joelpresence]

Thanks @jackc . So then you're confirming that I do not need to close when I use the QueryRow().Scan() pattern?

Could I add this to the godoc or README to make it more explicit for others?

Thanks again!

[jackc]

Correct, you do not need to call Close when using QueryRow. But more than that, you can't -- Close is not defined on Row -- so I'm not sure what doc changes would be beneficial.

[joelpresence]

Fair enough. It's just that I was in the habit of either defer rows.Close() or defer tx.Rollback() so when it came to using QueryRow().Scan() it felt like something was missing and the godoc made no mention of why I could skip close when using this approach. But what you said made sense.

Now that I understand, I'm fine with leaving it out of the godoc if you prefer.

Closing. Thanks again for your help, much appreciated!

[Naidile-P-N]

Had the same query, probably we should mention that we don't have to explicitly close rows while using QueryRow

[mcshaman]

Same