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

Reusing prepared statements throws runtime errors #17

Closed
grzm opened this issue Aug 19, 2019 · 4 comments
Closed

Reusing prepared statements throws runtime errors #17

grzm opened this issue Aug 19, 2019 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@grzm
Copy link

grzm commented Aug 19, 2019

When using prepared statements for queries, subsequent execution of the statement throws runtime errors.

	var res bool

	stmt, err := connDB.PrepareContext(ctx, "SELECT true AS res")
	if err != nil {
		os.Exit(1)
	}

        // first call
	rows, err := stmt.QueryContext(ctx)
	if err != nil {
		os.Exit(1)
	}

	for rows.Next() {
		testLogger.Info("first use of stmt: %t", res) // succeeds
	}

        // second call
	rows, err = stmt.QueryContext(ctx)
	if err != nil {
		os.Exit(1)
	}

	for rows.Next() {    // <- throws
		testLogger.Info("second use of stmt: %t", res)
	}

For comparison, this code works with PostgreSQL (using the lib/pq driver), and this pattern of reusing a prepared statement for a query is used in the Prometheus generic database free/sql_exporter exporter.

It looks like the row description might be getting lost on subsequent calls, as the number of columns returned in the second call (from rows.Columns()) is 0.

I've added tests which exhibit this behavior.

grzm@0cdacf8

@huebnerr huebnerr added the bug Something isn't working label Aug 20, 2019
@huebnerr
Copy link
Collaborator

Thank you for finding this, @grzm. We'll work on repro'ing the error, incorporating your test into the suite, and keep you updated. Hopefully this can be fixed in short order.

@huebnerr
Copy link
Collaborator

huebnerr commented Aug 20, 2019

@grzm I've made a fix to the code that passes this test. Please verify that I've correctly translated your test into a new unit test. Note that in yours, the value was never Scan()'d, but I've made a change in this one to Scan and validate the cursor state.

func TestStmtReuseBug(t *testing.T) {
	connDB := openConnection(t)
	defer closeConnection(t, connDB)

	var res bool

	stmt, err := connDB.PrepareContext(ctx, "SELECT true AS res")
	assertNoErr(t, err)

	// first call
	rows, err := stmt.QueryContext(ctx)
	assertNoErr(t, err)

	defer rows.Close()

	assertNext(t, rows)
	assertNoErr(t, rows.Scan(&res))
	assertEqual(t, res, true)
	assertNoNext(t, rows)

	// second call
	rows, err = stmt.QueryContext(ctx)
	assertNoErr(t, err)

	defer rows.Close()

	assertNext(t, rows)
	assertNoErr(t, rows.Scan(&res))
	assertEqual(t, res, true)
	assertNoNext(t, rows)
}
`

@grzm
Copy link
Author

grzm commented Aug 20, 2019

@huebnerr Cheers! I applied your patch https://github.com/huebnerr/vertica-sql-go/commit/399816b3f198fb58f26dbab9cd135c59b0c7a72f and it resolved this particular issue.

Note that in yours, the value was never Scan()

Thanks for catching that. Copy/paste error. That snippet was just to demonstrate in the comment. I did have full tests in the patch I linked to (grzm@0cdacf8), where I did indeed call rows.Scan ;)

Thanks for the quick turnaround!

@huebnerr
Copy link
Collaborator

No problem at all. Thank you for catching it, @grzm ! I'll go ahead and close the issue. @sitingren will merge the PR. #18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants