From 8009fbcb1846d267d0cad6769cd3fcda7d838ddd Mon Sep 17 00:00:00 2001 From: Loc Nguyen <113083148+thailoc-dev@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:09:13 +0700 Subject: [PATCH] fix(oracle): recover object column panics on pinned connections --- agents/drivers/oracle-go/dbms_output.go | 12 +++ agents/drivers/oracle-go/main.go | 65 +++++++++++- agents/drivers/oracle-go/main_test.go | 128 ++++++++++++++++++++++++ agents/drivers/oracle-go/query_guard.go | 12 +++ 4 files changed, 215 insertions(+), 2 deletions(-) diff --git a/agents/drivers/oracle-go/dbms_output.go b/agents/drivers/oracle-go/dbms_output.go index f0efde01f0..cc0bf2285b 100644 --- a/agents/drivers/oracle-go/dbms_output.go +++ b/agents/drivers/oracle-go/dbms_output.go @@ -550,6 +550,9 @@ func (s *server) withStatementDBMSOutput(sqlText string, run func() error) []que s.statementConn = conn defer func() { s.statementConn = previous + if repinned := s.takeRepinnedStatementConn(); repinned != nil { + _ = repinned.Close() + } _ = conn.Close() }() session := s.statementDBMSOutputSession(conn) @@ -604,6 +607,9 @@ func (s *server) withPagedStatementDBMSOutput(sqlText string, run func() error) // so this function owns it. The statement's own cleanup has already closed // its rows (runPagedOracleSelect defers that), which is what lets the close // below return instead of waiting for a cursor that no one will close. + if repinned := s.takeRepinnedStatementConn(); repinned != nil { + _ = repinned.Close() + } _ = conn.Close() panic(recovered) } @@ -612,6 +618,12 @@ func (s *server) withPagedStatementDBMSOutput(sqlText string, run func() error) capture := s.ensureDBMSOutput(ctx, session) _ = run() messages := s.finishStatementDBMSOutput(ctx, session, capture, sqlText) + if repinned := s.takeRepinnedStatementConn(); repinned != nil { + // A driver panic poisoned the pinned connection and the statement's + // fallback ran on a replacement, so the page's cursor lives there. + _ = conn.Close() + conn = repinned + } return conn, messages } diff --git a/agents/drivers/oracle-go/main.go b/agents/drivers/oracle-go/main.go index 8c9eb04a92..2b41f12e76 100644 --- a/agents/drivers/oracle-go/main.go +++ b/agents/drivers/oracle-go/main.go @@ -660,6 +660,8 @@ type server struct { statementCtx context.Context statementCancel context.CancelFunc statementConn *sql.Conn + statementSchema string + repinnedStatementConn *sql.Conn plDebugMu sync.Mutex plDebugSessions map[string]*plDebugSession plAllocation *plDebugAllocation @@ -2028,6 +2030,17 @@ func (s *server) currentSchema() (string, error) { } return strings.ToUpper(schema), nil } + // The connection pinned for a statement's DBMS_OUTPUT capture carries the + // statement's CURRENT_SCHEMA the same way. + if s.statementConn != nil { + if err := s.repinPoisonedStatementConn(); err != nil { + return "", err + } + if err := s.statementConn.QueryRowContext(s.statementContext(), sqlText).Scan(&schema); err != nil { + return "", err + } + return strings.ToUpper(schema), nil + } db, err := s.requireDB() if err != nil { return "", err @@ -5133,8 +5146,10 @@ func (s *server) queryRowsWithOracleValueRewriteIfNeeded(sqlText string, timeout // A statement that keeps row locks while it runs is rewritten before the // first attempt: when such a statement panics after the rows were locked, // the locks stay behind and the retry below blocks on them until the query - // times out (issues #9344 / #9180). - if oracleSQLLocksRows(sqlText) { + // times out (issues #9344 / #9180). A manual transaction cannot retry at + // all: the panic leaves its session unusable and the transaction is rolled + // back (issue #13455). + if oracleSQLLocksRows(sqlText) || s.hasManualTransaction() { rewritten, err := rewriteOracleSelectSQL(sqlText, s.loadOracleColumnMeta, deferLOBs) if err == nil && rewritten != sqlText { return s.queryRowsWithTimeout(rewritten, nil, timeoutSecs) @@ -6131,6 +6146,9 @@ func (s *server) setSchema(schema string) error { } if s.statementConn != nil { _, err := s.statementConn.ExecContext(s.statementContext(), sqlText) + if err == nil { + s.statementSchema = schema + } return err } db, err := s.requireDB() @@ -6141,6 +6159,46 @@ func (s *server) setSchema(schema string) error { return err } +// repinPoisonedStatementConn replaces the statement's pinned connection once a +// driver panic has poisoned it, so the placeholder fallbacks of the same +// statement do not fail on the dead session (issue #13455). The fresh +// connection gets the statement's CURRENT_SCHEMA back; the statement's owner +// closes it together with the original one. +func (s *server) repinPoisonedStatementConn() error { + if s.manualTx != nil || s.statementConn == nil || !oracleConnPoisoned(s.statementConn) { + return nil + } + db, err := s.requireDB() + if err != nil { + return err + } + conn, err := db.Conn(s.statementContext()) + if err != nil { + return err + } + if s.statementSchema != "" { + if _, err := conn.ExecContext(s.statementContext(), "ALTER SESSION SET CURRENT_SCHEMA = "+quoteIdentifier(s.statementSchema)); err != nil { + _ = conn.Close() + return err + } + } + if s.repinnedStatementConn != nil { + _ = s.repinnedStatementConn.Close() + } + s.repinnedStatementConn = conn + s.statementConn = conn + return nil +} + +// takeRepinnedStatementConn hands a connection opened by +// repinPoisonedStatementConn to the statement's owner. +func (s *server) takeRepinnedStatementConn() *sql.Conn { + conn := s.repinnedStatementConn + s.repinnedStatementConn = nil + s.statementSchema = "" + return conn +} + func (s *server) queryRows(sqlText string, args []any) (*sql.Rows, error) { return s.queryRowsWithTimeout(sqlText, args, 0) } @@ -6149,6 +6207,9 @@ func (s *server) queryRowsWithTimeout(sqlText string, args []any, timeoutSecs in if _, err := s.requireDB(); err != nil { return nil, err } + if err := s.repinPoisonedStatementConn(); err != nil { + return nil, err + } ctx, cancel := context.WithCancel(s.statementContext()) var timer *time.Timer if timeoutSecs > 0 { diff --git a/agents/drivers/oracle-go/main_test.go b/agents/drivers/oracle-go/main_test.go index 6957ac6b8f..9a7cb88989 100644 --- a/agents/drivers/oracle-go/main_test.go +++ b/agents/drivers/oracle-go/main_test.go @@ -4329,6 +4329,134 @@ func TestOracleQueryRowsKeepsSTAsTextErrorWhenPlaceholderAlsoFails(t *testing.T) } } +// oracleViewSourceTxConn lets the scripted driver open the manual transaction +// the query guard wraps. +type oracleViewSourceTxConn struct{ *oracleViewSourceConn } + +func (c oracleViewSourceTxConn) BeginTx(context.Context, driver.TxOptions) (driver.Tx, error) { + return oracleViewSourceTx{}, nil +} + +type oracleViewSourceTx struct{} + +func (oracleViewSourceTx) Commit() error { return nil } +func (oracleViewSourceTx) Rollback() error { return nil } + +type oracleViewSourceTxDriver struct{ scripted *oracleViewSourceDriver } + +func (d oracleViewSourceTxDriver) Open(string) (driver.Conn, error) { + return oracleViewSourceTxConn{&oracleViewSourceConn{driver: d.scripted}}, nil +} + +func openGuardedOracleViewSourceTestDB(t *testing.T, steps []oracleViewSourceQueryStep) (*sql.DB, *oracleViewSourceDriver) { + t.Helper() + scripted := &oracleViewSourceDriver{steps: steps} + db := sql.OpenDB(oracleQueryConnector{oracleGuardTestConnector{oracleViewSourceTxDriver{scripted}}}) + t.Cleanup(func() { _ = db.Close() }) + return db, scripted +} + +func udtPanicFallbackSteps(rows [][]driver.Value) []oracleViewSourceQueryStep { + return []oracleViewSourceQueryStep{ + {exec: true, queryContains: `ALTER SESSION SET CURRENT_SCHEMA = "DBXT"`}, + {queryContains: "SELECT ID, G FROM T_UDT", args: []driver.Value{}, panicText: "simulated go-ora panic on object column"}, + // The poisoned pinned connection is replaced and gets the editor schema back. + {exec: true, queryContains: `ALTER SESSION SET CURRENT_SCHEMA = "DBXT"`}, + {queryContains: "CURRENT_SCHEMA", args: []driver.Value{}, columns: []string{"SCHEMA"}, rows: [][]driver.Value{{"DBXT"}}}, + { + queryContains: "ALL_TAB_COLUMNS", + args: []driver.Value{"DBXT", "T_UDT"}, + columns: []string{"COLUMN_NAME", "DATA_TYPE", "DATA_TYPE_OWNER"}, + rows: [][]driver.Value{{"ID", "NUMBER", nil}, {"G", "GEOM_T", "DBXT"}}, + }, + {queryContains: "''", args: []driver.Value{}, columns: []string{"ID", "G"}, rows: rows}, + } +} + +func TestExecuteQueryRetriesObjectColumnPanicOnFreshPinnedConnection(t *testing.T) { + steps := udtPanicFallbackSteps([][]driver.Value{{int64(1), ""}}) + db, scripted := openGuardedOracleViewSourceTestDB(t, steps) + s := newServer() + s.db = db + result, err := s.executeQuery(queryOptions{SQL: "SELECT ID, G FROM T_UDT", Schema: "DBXT", MaxRows: 10}) + if err != nil { + t.Fatalf("expected the placeholder fallback to recover the panic, got %v", err) + } + if !reflect.DeepEqual(result.Rows, [][]any{{int64(1), ""}}) { + t.Fatalf("unexpected rows: %#v", result.Rows) + } + if scripted.next != len(steps) { + t.Fatalf("expected all %d scripted steps, ran %d", len(steps), scripted.next) + } + if s.statementConn != nil || s.repinnedStatementConn != nil || s.statementSchema != "" || db.Stats().InUse != 0 { + t.Fatalf("statement leaked its pinned connections: %+v", db.Stats()) + } +} + +func TestExecuteQueryPageKeepsCursorOnReplacementPinnedConnection(t *testing.T) { + steps := udtPanicFallbackSteps([][]driver.Value{{int64(1), ""}, {int64(2), ""}}) + db, scripted := openGuardedOracleViewSourceTestDB(t, steps) + s := newServer() + s.db = db + result, err := s.executeQueryPage(queryOptions{SQL: "SELECT ID, G FROM T_UDT", Schema: "DBXT", MaxRows: 10}, 1) + if err != nil { + t.Fatalf("expected the placeholder fallback to recover the panic, got %v", err) + } + if !result.HasMore || result.SessionID == nil || !reflect.DeepEqual(result.Rows, [][]any{{int64(1), ""}}) { + t.Fatalf("unexpected first page: %+v", result) + } + if scripted.next != len(steps) { + t.Fatalf("expected all %d scripted steps, ran %d", len(steps), scripted.next) + } + next, err := s.fetchQueryPage(*result.SessionID, 1) + if err != nil || !reflect.DeepEqual(next.Rows, [][]any{{int64(2), ""}}) { + t.Fatalf("unexpected second page: %+v, %v", next, err) + } + s.closeQuerySession(*result.SessionID) + if s.statementConn != nil || s.repinnedStatementConn != nil || db.Stats().InUse != 0 { + t.Fatalf("paging session leaked its pinned connections: %+v", db.Stats()) + } +} + +func TestManualTransactionRewritesObjectColumnsBeforeTheFirstAttempt(t *testing.T) { + steps := []oracleViewSourceQueryStep{ + {exec: true, queryContains: `ALTER SESSION SET CURRENT_SCHEMA = "DBXT"`}, + {queryContains: "CURRENT_SCHEMA", args: []driver.Value{}, columns: []string{"SCHEMA"}, rows: [][]driver.Value{{"DBXT"}}}, + { + queryContains: "ALL_TAB_COLUMNS", + args: []driver.Value{"DBXT", "T_UDT"}, + columns: []string{"COLUMN_NAME", "DATA_TYPE", "DATA_TYPE_OWNER"}, + rows: [][]driver.Value{{"ID", "NUMBER", nil}, {"G", "GEOM_T", "DBXT"}}, + }, + {queryContains: "''", args: []driver.Value{}, columns: []string{"ID", "G"}, rows: [][]driver.Value{{int64(1), ""}}}, + {queryContains: "SELECT 1 FROM DUAL", args: []driver.Value{}, columns: []string{"1"}, rows: [][]driver.Value{{int64(1)}}}, + } + db, scripted := openGuardedOracleViewSourceTestDB(t, steps) + s := newServer() + s.db = db + if err := s.beginManualTransaction("DBXT"); err != nil { + t.Fatal(err) + } + // The raw statement would panic and take the transaction down with it, so it + // must never reach the driver. + result, err := s.executeQuery(queryOptions{SQL: "SELECT ID, G FROM T_UDT", MaxRows: 10}) + if err != nil { + t.Fatalf("expected the rewritten projection, got %v", err) + } + if !reflect.DeepEqual(result.Rows, [][]any{{int64(1), ""}}) { + t.Fatalf("unexpected rows: %#v", result.Rows) + } + if _, err := s.executeQuery(queryOptions{SQL: "SELECT 1 FROM DUAL", MaxRows: 10}); err != nil { + t.Fatalf("manual transaction became unusable: %v", err) + } + if scripted.next != len(steps) { + t.Fatalf("expected all %d scripted steps, ran %d", len(steps), scripted.next) + } + if err := s.rollbackManualTransaction(); err != nil { + t.Fatal(err) + } +} + func TestReadQuerySessionPageConvertsPanicToError(t *testing.T) { // A nil *sql.Rows makes rows.Next() panic with a nil pointer dereference, // which stands in for go-ora panicking while decoding an unknown type. diff --git a/agents/drivers/oracle-go/query_guard.go b/agents/drivers/oracle-go/query_guard.go index 19fb48a2e1..4db4a68574 100644 --- a/agents/drivers/oracle-go/query_guard.go +++ b/agents/drivers/oracle-go/query_guard.go @@ -2,6 +2,7 @@ package main import ( "context" + "database/sql" "database/sql/driver" "fmt" ) @@ -59,6 +60,17 @@ func (c *oracleQueryConnection) QueryContext(ctx context.Context, query string, return c.oracleThinConnection.QueryContext(ctx, query, args) } +// oracleConnPoisoned reports whether a driver panic already made conn unusable. +func oracleConnPoisoned(conn *sql.Conn) bool { + poisoned := false + _ = conn.Raw(func(driverConn any) error { + guarded, ok := driverConn.(*oracleQueryConnection) + poisoned = ok && guarded.failed != nil + return nil + }) + return poisoned +} + // Invalidate via Validator, not ErrBadConn: ErrBadConn would tell database/sql // to replay the original statement, even when it has already acquired locks or // invoked a function with side effects. The original panic remains diagnostic.