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

fix(spanner/spannertest): support queries in ExecuteSql #3640

Merged
merged 9 commits into from Feb 4, 2021

Conversation

@olavloite
Copy link
Collaborator

@olavloite olavloite commented Feb 1, 2021

Normal queries from the Spanner client use the ExecuteStreamingSql method,
while DML statements use ExecuteSql. This distinction was also built into
spannertest where ExecuteSql would only support DML statements and required
a transaction to be specified. The session pool however uses ExecuteSql to
execute a simple SELECT 1 query without specifying any transaction. This
would cause a nil pointer dereference.

This PR introduces support for queries in the ExecuteSql method. The current
logic assumes that the statement is a query if the transaction is a single-
use read-only transaction.

Fixes #3639

Normal queries from the Spanner client use the ExecuteStreamingSql method,
while DML statements use ExecuteSql. This distinction was also built into
spannertest where ExecuteSql would only support DML statements and required
a transaction to be specified. The session pool however uses ExecuteSql to
execute a simple `SELECT 1` query without specifying any transaction. This
would cause a nil pointer dereference.

This PR introduces support for queries in the ExecuteSql method. The current
logic assumes that the statement is a query if the transaction is a single-
use read-only transaction.

Fixes #3639
@olavloite olavloite requested a review from dsymonds Feb 1, 2021
@olavloite olavloite requested review from skuruppu and as code owners Feb 1, 2021
@google-cla google-cla bot added the cla: yes label Feb 1, 2021
spanner/spannertest/inmem.go Outdated Show resolved Hide resolved
Loading
spanner/spannertest/inmem.go Outdated Show resolved Hide resolved
Loading
spanner/spannertest/integration_test.go Show resolved Hide resolved
Loading
spanner/spannertest/integration_test.go Show resolved Hide resolved
Loading
moneyease
Copy link

moneyease commented on c284847 Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks !

Loading

@olavloite
Copy link
Collaborator Author

@olavloite olavloite commented Feb 3, 2021

@skuruppu Apparently your review is also required in this case to allow merging.

Loading

@gcf-merge-on-green gcf-merge-on-green bot merged commit 8eede84 into master Feb 4, 2021
3 checks passed
Loading
@gcf-merge-on-green gcf-merge-on-green bot deleted the issue-3639 branch Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants