-
-
Notifications
You must be signed in to change notification settings - Fork 175
fix(#481): ALTER TABLE ADD COLUMN — new columns invisible to subsequent queries #495
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
base: main
Are you sure you want to change the base?
Changes from all commits
27a407f
3c1be50
e3d09e7
0160fc5
021f949
5b3b693
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,117 +1,195 @@ | ||
| package connection | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "fmt" | ||
|
|
||
| "github.com/goccy/googlesqlite" | ||
| ) | ||
|
|
||
| type Manager struct { | ||
| db *sql.DB | ||
| } | ||
|
|
||
| func NewManager(db *sql.DB) *Manager { | ||
| return &Manager{db: db} | ||
| } | ||
|
|
||
| func (m *Manager) Connection(ctx context.Context, projectID, datasetID string) (*Conn, error) { | ||
| if projectID == "" { | ||
| return nil, fmt.Errorf("invalid projectID. projectID is empty") | ||
| } | ||
| conn, err := m.db.Conn(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get connection: %w", err) | ||
| } | ||
| return &Conn{ | ||
| ProjectID: projectID, | ||
| DatasetID: datasetID, | ||
| Conn: conn, | ||
| }, nil | ||
| } | ||
|
|
||
| type Tx struct { | ||
| tx *sql.Tx | ||
| conn *Conn | ||
| committed bool | ||
| } | ||
|
|
||
| func (t *Tx) Tx() *sql.Tx { | ||
| return t.tx | ||
| } | ||
|
|
||
| func (t *Tx) RollbackIfNotCommitted() error { | ||
| if t.committed { | ||
| return nil | ||
| } | ||
| defer t.conn.Conn.Close() | ||
| return t.tx.Rollback() | ||
| } | ||
|
|
||
| func (t *Tx) Commit() error { | ||
| if err := t.tx.Commit(); err != nil { | ||
| return err | ||
| } | ||
| t.committed = true | ||
| t.conn.Conn.Close() | ||
| return nil | ||
| } | ||
|
|
||
| func (t *Tx) SetProjectAndDataset(projectID, datasetID string) { | ||
| t.conn.ProjectID = projectID | ||
| t.conn.DatasetID = datasetID | ||
| } | ||
|
|
||
| func (t *Tx) MetadataRepoMode() error { | ||
| if err := t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("failed to get *googlesqlite.Conn from %T", c) | ||
| } | ||
| _ = gsqlConn.SetNamePath([]string{}) | ||
| return nil | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to setup connection: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (t *Tx) ContentRepoMode() error { | ||
| if err := t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("failed to get *googlesqlite.Conn from %T", c) | ||
| } | ||
| if t.conn.DatasetID == "" { | ||
| _ = gsqlConn.SetNamePath([]string{t.conn.ProjectID}) | ||
| } else { | ||
| _ = gsqlConn.SetNamePath([]string{t.conn.ProjectID, t.conn.DatasetID}) | ||
| } | ||
| const maxNamePath = 3 // projectID and datasetID and tableID | ||
| gsqlConn.SetMaxNamePath(maxNamePath) | ||
| return nil | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to setup connection: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| type Conn struct { | ||
| ProjectID string | ||
| DatasetID string | ||
| Conn *sql.Conn | ||
| } | ||
|
|
||
| func (c *Conn) Begin(ctx context.Context) (*Tx, error) { | ||
| tx, err := c.Conn.BeginTx(ctx, nil) | ||
| if err != nil { | ||
| // The pooled connection is owned by the Tx once BeginTx succeeds and | ||
| // is released by Commit/RollbackIfNotCommitted. When BeginTx fails no | ||
| // Tx is created, so the connection must be returned to the pool here | ||
| // or it leaks for the lifetime of the process. | ||
| _ = c.Conn.Close() | ||
| return nil, err | ||
| } | ||
| return &Tx{tx: tx, conn: c}, nil | ||
| } | ||
| package connection | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "fmt" | ||
| "reflect" | ||
| "unsafe" | ||
|
|
||
| "github.com/goccy/googlesqlite" | ||
| ) | ||
|
|
||
| type Manager struct { | ||
| db *sql.DB | ||
| } | ||
|
|
||
| func NewManager(db *sql.DB) *Manager { | ||
| return &Manager{db: db} | ||
| } | ||
|
|
||
| func (m *Manager) Connection(ctx context.Context, projectID, datasetID string) (*Conn, error) { | ||
| if projectID == "" { | ||
| return nil, fmt.Errorf("invalid projectID. projectID is empty") | ||
| } | ||
| conn, err := m.db.Conn(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get connection: %w", err) | ||
| } | ||
| return &Conn{ | ||
| ProjectID: projectID, | ||
| DatasetID: datasetID, | ||
| Conn: conn, | ||
| }, nil | ||
| } | ||
|
|
||
| type Tx struct { | ||
| tx *sql.Tx | ||
| conn *Conn | ||
| committed bool | ||
| } | ||
|
|
||
| func (t *Tx) Tx() *sql.Tx { | ||
| return t.tx | ||
| } | ||
|
|
||
| func (t *Tx) RollbackIfNotCommitted() error { | ||
| if t.committed { | ||
| return nil | ||
| } | ||
| defer t.conn.Conn.Close() | ||
| return t.tx.Rollback() | ||
| } | ||
|
|
||
| func (t *Tx) Commit() error { | ||
| if err := t.tx.Commit(); err != nil { | ||
| return err | ||
| } | ||
| t.committed = true | ||
| t.conn.Conn.Close() | ||
| return nil | ||
| } | ||
|
|
||
| func (t *Tx) SetProjectAndDataset(projectID, datasetID string) { | ||
| t.conn.ProjectID = projectID | ||
| t.conn.DatasetID = datasetID | ||
| } | ||
|
|
||
| func (t *Tx) MetadataRepoMode() error { | ||
| if err := t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("failed to get *googlesqlite.Conn from %T", c) | ||
| } | ||
| _ = gsqlConn.SetNamePath([]string{}) | ||
| return nil | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to setup connection: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (t *Tx) ContentRepoMode() error { | ||
| if err := t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("failed to get *googlesqlite.Conn from %T", c) | ||
| } | ||
| if t.conn.ProjectID == "" { | ||
| return fmt.Errorf("invalid projectID. projectID is empty") | ||
| } | ||
| namePath := []string{t.conn.ProjectID} | ||
| if t.conn.DatasetID != "" { | ||
| namePath = append(namePath, t.conn.DatasetID) | ||
| } | ||
| _ = gsqlConn.SetNamePath(namePath) | ||
|
|
||
| const maxNamePath = 3 // projectID and datasetID and tableID | ||
| gsqlConn.SetMaxNamePath(maxNamePath) | ||
| return nil | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to setup connection: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| type Conn struct { | ||
| ProjectID string | ||
| DatasetID string | ||
| Conn *sql.Conn | ||
| } | ||
|
|
||
| // RawExec executes a raw SQLite statement directly on the inner connection, | ||
| // bypassing googlesqlite's ZetaSQL parser. Safe to call while a transaction | ||
| // is active (after Begin, before Commit/Rollback). | ||
| func (t *Tx) RawExec(ctx context.Context, query string, args ...interface{}) error { | ||
| var execErr error | ||
| if err := t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("unexpected driver connection type %T", c) | ||
| } | ||
| innerConn, err := innerSQLConn(gsqlConn) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, execErr = innerConn.ExecContext(ctx, query, args...) | ||
| return nil | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| return execErr | ||
| } | ||
|
|
||
| // RawQueryRow executes a raw SQLite query, bypassing ZetaSQL, and returns | ||
| // the single-row result. The caller must scan the row before the next call. | ||
| func (t *Tx) RawQueryRow(ctx context.Context, query string, args ...interface{}) (*sql.Row, error) { | ||
| var row *sql.Row | ||
| if err := t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("unexpected driver connection type %T", c) | ||
| } | ||
| innerConn, err := innerSQLConn(gsqlConn) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| row = innerConn.QueryRowContext(ctx, query, args...) | ||
| return nil | ||
| }); err != nil { | ||
| return nil, err | ||
| } | ||
| return row, nil | ||
| } | ||
|
|
||
| // innerSQLConn extracts the unexported *sql.Conn from a *googlesqlite.Conn via | ||
| // reflect. It validates the field name, kind, and exact type before the unsafe | ||
| // pointer reinterpretation so a layout change in googlesqlite surfaces as a | ||
| // clear error rather than a panic or memory corruption. | ||
| func innerSQLConn(gsqlConn *googlesqlite.Conn) (*sql.Conn, error) { | ||
| f := reflect.ValueOf(gsqlConn).Elem().FieldByName("conn") | ||
| if !f.IsValid() { | ||
| return nil, fmt.Errorf("googlesqlite.Conn has no 'conn' field") | ||
| } | ||
| wantType := reflect.TypeOf((*sql.Conn)(nil)) | ||
| if f.Kind() != reflect.Ptr || f.Type() != wantType { | ||
| return nil, fmt.Errorf("googlesqlite.Conn.conn has unexpected type %s (want %s)", f.Type(), wantType) | ||
| } | ||
| return *(**sql.Conn)(unsafe.Pointer(f.UnsafeAddr())), nil | ||
| } | ||
|
|
||
| // WithGSQLConn calls f with the underlying *googlesqlite.Conn. Use this to | ||
| // manipulate googlesqlite internals (e.g. the in-memory ZetaSQL catalog) | ||
| // from outside the package. | ||
| func (t *Tx) WithGSQLConn(f func(*googlesqlite.Conn) error) error { | ||
| return t.conn.Conn.Raw(func(c interface{}) error { | ||
| gsqlConn, ok := c.(*googlesqlite.Conn) | ||
| if !ok { | ||
| return fmt.Errorf("unexpected driver connection type %T", c) | ||
| } | ||
| return f(gsqlConn) | ||
| }) | ||
| } | ||
|
|
||
| func (c *Conn) Begin(ctx context.Context) (*Tx, error) { | ||
| tx, err := c.Conn.BeginTx(ctx, nil) | ||
| if err != nil { | ||
| // The pooled connection is owned by the Tx once BeginTx succeeds and | ||
| // is released by Commit/RollbackIfNotCommitted. When BeginTx fails no | ||
| // Tx is created, so the connection must be returned to the pool here | ||
| // or it leaks for the lifetime of the process. | ||
| _ = c.Conn.Close() | ||
| return nil, err | ||
| } | ||
| return &Tx{tx: tx, conn: c}, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -541,7 +541,7 @@ func (r *Repository) convertValueToCell(value interface{}, schema *bigqueryv2.Ta | |
|
|
||
| func (r *Repository) CreateOrReplaceTable(ctx context.Context, tx *connection.Tx, projectID, datasetID string, table *types.Table) error { | ||
| tx.SetProjectAndDataset(projectID, datasetID) | ||
| if err := tx.ContentRepoMode(); err != nil { | ||
| if err := tx.MetadataRepoMode(); err != nil { | ||
| return err | ||
| } | ||
|
Comment on lines
+544
to
546
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is part of the shared commits from PR #492 (fix for #470). The switch to |
||
| defer func() { | ||
|
|
@@ -577,7 +577,7 @@ func (r *Repository) AddTableData(ctx context.Context, tx *connection.Tx, projec | |
| return nil | ||
| } | ||
| tx.SetProjectAndDataset(projectID, datasetID) | ||
| if err := tx.ContentRepoMode(); err != nil { | ||
| if err := tx.MetadataRepoMode(); err != nil { | ||
| return err | ||
| } | ||
|
Comment on lines
+580
to
582
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above — this is part of the shared commits from PR #492. Addressed there. |
||
| defer func() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit
5b3b693. Extracted the reflect+unsafe access into a dedicatedinnerSQLConnhelper that validatesf.Kind() == reflect.Ptrandf.Type() == reflect.TypeOf((*sql.Conn)(nil))before the pointer reinterpretation. A type mismatch now returns a clear error instead of causing a panic or memory corruption.