[インデックス 11409] ファイルの概要

このコミットは、Go言語の標準ライブラリである database/sql パッケージにおける Tx.Query メソッドのバグを修正するものです。具体的には、トランザクション内で実行されるクエリが、結果セット（Rows）が完全に処理される前に、関連するプリペアドステートメント（Stmt）を誤って閉じてしまう問題を解決します。

コミット

commit bcb976c5b277b30dab6e771659c01bddec8c8a72
Author: Blake Mizerany <blake.mizerany@gmail.com>
Date:   Wed Jan 25 17:49:30 2012 -0800

    database/sql: fix Tx.Query
    
    Fixes #2784
    
    R=golang-dev, bradfitz
    CC=golang-dev
    https://golang.org/cl/5574073
---
 src/pkg/database/sql/sql.go      |  7 +++++--
 src/pkg/database/sql/sql_test.go | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/pkg/database/sql/sql.go b/src/pkg/database/sql/sql.go
index 70499b9a95..7e226b17dc 100644
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -556,8 +556,11 @@ func (tx *Tx) Query(query string, args ...interface{}) (*Rows, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer stmt.Close()
-	return stmt.Query(args...)
+	rows, err := stmt.Query(args...)
+	if err == nil {
+		rows.closeStmt = stmt
+	}
+	return rows, err
 }
 
 // QueryRow executes a query that is expected to return at most one row.
diff --git a/src/pkg/database/sql/sql_test.go b/src/pkg/database/sql/sql_test.go
index 3fb137eb24..08db6d38ff 100644
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -311,6 +311,40 @@ func TestTxStmt(t *testing.T) {
 	}
 }
 
+// Issue: http://golang.org/issue/2784
+// This test didn't fail before because we got luckly with the fakedb driver.
+// It was failing, and now not, in github.com/bradfitz/go-sql-test
+func TestTxQuery(t *testing.T) {
+	db := newTestDB(t, "")
+	defer closeDB(t, db)
+	exec(t, db, "CREATE|t1|name=string,age=int32,dead=bool")
+	exec(t, db, "INSERT|t1|name=Alice")
+
+	tx, err := db.Begin()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer tx.Rollback()
+
+	r, err := tx.Query("SELECT|t1|name|")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !r.Next() {
+		if r.Err() != nil {
+			t.Fatal(r.Err())
+		}
+		t.Fatal("expected one row")
+	}
+
+	var x string
+	err = r.Scan(&x)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 // Tests fix for issue 2542, that we release a lock when querying on
 // a closed connection.
 func TestIssue2542Deadlock(t *testing.T) {

GitHub上でのコミットページへのリンク

https://github.com/golang/go/commit/bcb976c5b277b30dab6e771659c01bddec8c8a72

元コミット内容

database/sql: fix Tx.Query
Fixes #2784
R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/5574073

変更の背景

このコミットは、Go言語の database/sql パッケージにおける Tx.Query メソッドのバグを修正するために行われました。コミットメッセージに Fixes #2784 とあるように、Issue 2784で報告された問題に対応しています。

当時の Tx.Query の実装では、内部的にプリペアドステートメント（Stmt）を作成し、その Stmt に対してクエリを実行していました。しかし、defer stmt.Close() という記述があったため、Tx.Query メソッドが終了する際に、結果セット（Rows）が完全に読み取られる前に Stmt が閉じられてしまう可能性がありました。

database/sql パッケージの設計では、Rows オブジェクトがクエリ結果をイテレートする間、関連する Stmt や基盤となるデータベース接続をオープンに保つ必要があります。Stmt が prematurely に閉じられると、Rows からデータを読み取ろうとした際にエラーが発生したり、予期せぬ動作を引き起こしたりする可能性がありました。

追加されたテストケース TestTxQuery は、この問題を再現するために書かれました。テストコードのコメントにもあるように、以前は「fakedb driver のおかげでたまたま失敗しなかった」が、github.com/bradfitz/go-sql-test では失敗していた、という状況が示唆されています。これは、特定のデータベースドライバーやテスト環境において、このバグが顕在化していたことを意味します。

（注記：コミットが2012年のものであり、当時のIssueトラッカーの記録が古いため、現在のWeb検索では golang/go リポジトリのIssue 2784に関する直接的な情報は得られませんでした。検索結果は golang/vscode-go の異なるIssueを指していました。これは、Issue番号が異なるリポジトリ間で重複したり、時間の経過とともに情報が整理されたりするためによくあることです。しかし、コミットメッセージとコードの変更内容から、問題の性質は明確に読み取れます。）

前提知識の解説

このコミットを理解するためには、Go言語の database/sql パッケージの基本的な概念を理解しておく必要があります。

• database/sql パッケージ: Go言語でSQLデータベースを操作するための標準パッケージです。データベースドライバーとアプリケーションの間の抽象化レイヤーを提供します。
• DB (データベース接続プール): sql.Open で取得されるオブジェクトで、データベースへの接続プールを管理します。
• Tx (トランザクション): DB.Begin() または DB.BeginTx() で開始されるデータベーストランザクションを表すオブジェクトです。トランザクション内の操作は、Commit() または Rollback() が呼び出されるまで、アトミックに扱われます。
• Stmt (プリペアドステートメント): DB.Prepare() または Tx.Prepare() で作成されるプリペアドステートメントを表すオブジェクトです。SQLクエリを事前に準備（プリコンパイル）しておくことで、繰り返し実行するクエリのパフォーマンスを向上させたり、SQLインジェクション攻撃を防いだりするのに役立ちます。Stmt は Close() メソッドを持ち、リソースを解放します。
• Query メソッド: DB または Tx オブジェクトに対してSQLクエリを実行し、結果セット（Rows）を返すメソッドです。
• Rows (結果セット): Query メソッドが返すオブジェクトで、クエリの結果行をイテレートするために使用されます。Next() メソッドで次の行に進み、Scan() メソッドで現在の行のデータをGoの変数にスキャンします。Rows は Close() メソッドを持ち、結果セットに関連するリソースを解放します。Rows が閉じられると、関連する Stmt も閉じられるべきです。
• defer ステートメント: Go言語のキーワードで、defer に続く関数呼び出しを、その関数がリターンする直前に実行するようにスケジュールします。リソースの解放（ファイル、ロック、データベース接続など）によく使用されます。

技術的詳細

このコミットの核心は、Tx.Query メソッドにおける Stmt のライフサイクル管理の修正にあります。

修正前の Tx.Query メソッドの関連部分は以下のようになっていました。

func (tx *Tx) Query(query string, args ...interface{}) (*Rows, error) {
	stmt, err := tx.Prepare(query)
	if err != nil {
		return nil, err
	}
	defer stmt.Close() // ここが問題
	return stmt.Query(args...)
}

ここで問題となっていたのは defer stmt.Close() です。 defer ステートメントは、Tx.Query 関数が終了する直前に stmt.Close() を実行するようにスケジュールします。しかし、stmt.Query(args...) が *Rows オブジェクトを返した後、Tx.Query 関数がすぐにリターンしてしまうと、Rows オブジェクトがまだアクティブな状態で stmt.Close() が呼び出されてしまう可能性がありました。

database/sql パッケージの設計では、Rows オブジェクトは、その結果セットを読み取る間、関連する Stmt オブジェクト（および基盤となるデータベース接続）がオープンであることを期待します。Stmt が Rows の読み取りが完了する前に閉じられてしまうと、Rows.Next() や Rows.Scan() を呼び出した際に「ステートメントが閉じられている」といったエラーが発生したり、データが正しく取得できなかったりする原因となります。

この修正では、defer stmt.Close() を削除し、代わりに Rows オブジェクトに Stmt オブジェクトへの参照を保持させるように変更しました。

修正後のコードは以下のようになります。

func (tx *Tx) Query(query string, args ...interface{}) (*Rows, error) {
	stmt, err := tx.Prepare(query)
	if err != nil {
		return nil, err
	}
	// defer stmt.Close() は削除された
	rows, err := stmt.Query(args...)
	if err == nil {
		rows.closeStmt = stmt // Rows オブジェクトに Stmt への参照を保持させる
	}
	return rows, err
}

この変更により、Stmt は Tx.Query メソッドが終了してもすぐに閉じられることはありません。代わりに、stmt.Query() が返す Rows オブジェクトが、その内部フィールド closeStmt を通じて Stmt への参照を保持します。

Rows オブジェクトは、その Close() メソッドが呼び出されたとき（または、すべての行が読み取られたとき、あるいはエラーが発生したときなど、Rows のライフサイクルが終了したとき）に、closeStmt に保持されている Stmt を適切に閉じます。これにより、Stmt のライフサイクルが Rows のライフサイクルと同期され、Rows がアクティブな間は Stmt がオープンに保たれることが保証されます。

この修正は、database/sql パッケージの堅牢性を高め、トランザクション内でクエリを実行する際の潜在的なバグを防ぐ上で非常に重要です。

コアとなるコードの変更箇所

src/pkg/database/sql/sql.go ファイルの Tx.Query メソッドが変更されました。

--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -556,8 +556,11 @@ func (tx *Tx) Query(query string, args ...interface{}) (*Rows, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer stmt.Close()
-	return stmt.Query(args...)
+	rows, err := stmt.Query(args...)
+	if err == nil {
+		rows.closeStmt = stmt
+	}
+	return rows, err
 }

また、この修正の正しさを検証するために、src/pkg/database/sql/sql_test.go に TestTxQuery という新しいテストケースが追加されました。

--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -311,6 +311,40 @@ func TestTxStmt(t *testing.T) {
 	}
 }
 
+// Issue: http://golang.org/issue/2784
+// This test didn't fail before because we got luckly with the fakedb driver.
+// It was failing, and now not, in github.com/bradfitz/go-sql-test
+func TestTxQuery(t *testing.T) {
+	db := newTestDB(t, "")
+	defer closeDB(t, db)
+	exec(t, db, "CREATE|t1|name=string,age=int32,dead=bool")
+	exec(t, db, "INSERT|t1|name=Alice")
+
+	tx, err := db.Begin()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer tx.Rollback()
+
+	r, err := tx.Query("SELECT|t1|name|")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !r.Next() {
+		if r.Err() != nil {
+			t.Fatal(r.Err())
+		}
+		t.Fatal("expected one row")
+	}
+
+	var x string
+	err = r.Scan(&x)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
 // Tests fix for issue 2542, that we release a lock when querying on
 // a closed connection.
 func TestIssue2542Deadlock(t *testing.T) {

コアとなるコードの解説

変更の核心は、Tx.Query メソッド内で defer stmt.Close() を削除し、代わりに rows.closeStmt = stmt という行を追加した点です。

• defer stmt.Close() の削除:

  • 以前は、Tx.Query 関数が終了する際に、内部で作成されたプリペアドステートメント stmt が defer によって閉じられるようにスケジュールされていました。
  • しかし、stmt.Query() が返す Rows オブジェクトがまだアクティブな状態で、Tx.Query がリターンしてしまうと、Rows が結果を読み取る前に stmt が閉じられてしまう可能性がありました。
  • Rows は、そのライフサイクル中に stmt がオープンであることを前提としているため、これは問題を引き起こす可能性がありました。

• rows, err := stmt.Query(args...):

  • これは、プリペアドステートメント stmt を使って実際にクエリを実行し、結果セット rows を取得する部分です。

• if err == nil { rows.closeStmt = stmt }:

  • クエリの実行が成功した場合（err が nil の場合）にのみ、この処理が実行されます。
  • rows.closeStmt = stmt は、Rows オブジェクトの内部フィールド closeStmt に、現在使用している Stmt オブジェクトへの参照を代入しています。
  • この closeStmt フィールドは、Rows オブジェクトが自身の Close() メソッドが呼び出されたときに、関連する Stmt を適切に閉じるために使用されます。
  • これにより、Stmt のライフサイクルが Rows のライフサイクルに結びつけられ、Rows が結果セットを処理している間は Stmt が確実にオープンに保たれるようになります。Rows が閉じられたときに初めて Stmt も閉じられるため、リソースの適切な管理とバグの回避が実現されます。

この修正により、Tx.Query を使用してトランザクション内でクエリを実行する際に、結果セットの読み取り中にプリペアドステートメントが予期せず閉じられるという問題が解決されました。

関連リンク

• GitHubコミットページ: https://github.com/golang/go/commit/bcb976c5b277b30dab6e771659c01bddec8c8a72
• Go CL (Code Review): https://golang.org/cl/5574073 (現在はGoの新しいコードレビューシステムにリダイレクトされる可能性があります)

参考にした情報源リンク

• 上記のGitHubコミット情報
• Go言語 database/sql パッケージのドキュメント (当時のバージョンに準拠)
• Go言語の defer ステートメントに関する一般的な知識
• database/sql パッケージにおける Rows と Stmt のライフサイクル管理に関する一般的な知識
• Issue 2784に関するWeb検索は、当時の正確な情報を見つけることができませんでした。これは、Issueトラッカーの変更や、古いIssueがアーカイブされることによるものです。しかし、コミットメッセージとコードの変更内容から、問題の性質は明確に理解できました。