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

Proposal: Callback function variants of existing methods for native pgx interface #821

Closed
jackc opened this issue Sep 5, 2020 · 2 comments

Comments

@jackc
Copy link
Owner

jackc commented Sep 5, 2020

A number of operations in pgx involve calling one method to start an operation and another to finish an operation. I'm considering adding simpler callback function variants for these operations.

In pgx transactions could be made simpler:

err := conn.BeginFunc(ctx, func(tx pgx.Tx) error {
	// Do stuff in the transaction
})
if err != nil {
	return err
}

as opposed to

tx, err := conn.Begin(ctx)
if err != nil {
	return err
}
defer tx.Rollback()

// Do stuff in the transaction

err = tx.Commit(ctx)
if err != nil {
	return err
}

It's shorter, and there are less places for errors to occur (such as forgetting the defer tx.Rollback()).

Similar changes could be made to BeginTx and to acquiring connections from pgxpool.

An even greater simplification might be possible with Query.

err := conn.QueryFunc(ctx,
	"select ...",
	[]interface{}{arg1, arg2, arg3},
	[]interface{}{&scan1, &scan2, &scan3},
	func(fields []pgproto3.FieldDescriptions) error {
		// Do stuff for each row
	},
)
if err != nil {
	return err
}

instead of

rows, err := conn.Query(ctx,
	"select ...",
	arg1, arg2, arg3,
)
if err != nil {
	return err
}
defer rows.Close()

for rows.Next() {
	err = rows.Scan(&scan1, &scan2, &scan3)
	if err != nil {
		return err
	}

	// Do stuff for each row
}

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

It is much smaller and eliminates multiple potential error conditions such as forgetting defer rows.Close(), forgetting to check the result of Scan, forgetting to check rows.Err(), and remembering to check rows.Err() but mistakenly using local var err when reporting an error.


These changes would be entirely backwards compatible as it would add new functions without changing or removing existing ones.

Any thoughts or concerns about these potential additions?

@paudley
Copy link
Contributor

paudley commented Sep 7, 2020 via email

@jackc
Copy link
Owner Author

jackc commented Dec 12, 2020

There hasn't been much feedback on this proposal, but what little there has been has been positive.

I added QueryFunc in e8f959e.

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

No branches or pull requests

2 participants