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

promoting changes to add support for read only db instance #429

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 48 additions & 4 deletions gorm/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"reflect"
"sync"

"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
"github.com/infobloxopen/atlas-app-toolkit/rpc/errdetails"
"github.com/jinzhu/gorm"
"google.golang.org/grpc"
Expand All @@ -17,15 +18,18 @@ import (
// ctxKey is an unexported type for keys defined in this package.
// This prevents collisions with keys defined in other packages.
type ctxKey int
type readOnlyDBKey int

// txnKey is the key for `*Transaction` values in `context.Context`.
// It is unexported; clients use NewContext and FromContext
// instead of using this key directly.
var txnKey ctxKey
var roDBKey readOnlyDBKey

var (
ErrCtxTxnMissing = errors.New("Database transaction for request missing in context")
ErrCtxTxnNoDB = errors.New("Transaction in context, but DB is nil")
ErrNoReadOnlyDB = errors.New("No read-only DB")
)

// NewContext returns a new Context that carries value txn.
Expand All @@ -39,13 +43,33 @@ func FromContext(ctx context.Context) (txn *Transaction, ok bool) {
return
}

// GetReadOnlyDB returns the read only db instance stored in the ctx if there is no txn in use with read/write database
func GetReadOnlyDB(ctx context.Context) (*gorm.DB, error) {
logger := ctxlogrus.Extract(ctx)
txn, ok := FromContext(ctx)
if !ok {
return nil, ErrCtxTxnMissing
}
if txn.current != nil {
logger.Warnf("GetReadOnlyDB: Txn already initialized with read/write DB")

Choose a reason for hiding this comment

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

  1. Maybe the caller would like to know that it asked for a read-only transaction and got a read-write one instead.
  2. Extracting the logger from the context assumes this code is used with the gRPC gateway which might not be the case.

return txn.current, nil
}
dbRO, ok := ctx.Value(roDBKey).(*gorm.DB)
if !ok {
return nil, ErrNoReadOnlyDB
}
txn.readOnly = true
return dbRO, nil
}

// Transaction serves as a wrapper around `*gorm.DB` instance.
// It works as a singleton to prevent an application of creating more than one
// transaction instance per incoming request.
type Transaction struct {
mu sync.Mutex
parent *gorm.DB
current *gorm.DB
readOnly bool
afterCommitHook []func(context.Context)
}

Expand All @@ -57,15 +81,29 @@ func (t *Transaction) AddAfterCommitHook(hooks ...func(context.Context)) {
t.afterCommitHook = append(t.afterCommitHook, hooks...)
}

// BeginFromContext will extract transaction wrapper from context and start new transaction.
// GetReadOnlyDBInstance returns the read only database instance stored in the ctx
func GetReadOnlyDBInstance(ctx context.Context) (*gorm.DB, error) {
dbRO, ok := ctx.Value(roDBKey).(*gorm.DB)
if !ok {
return nil, ErrNoReadOnlyDB
}
return dbRO, nil
}

// BeginFromContext will extract transaction wrapper from context and start new transaction if transaction is not set to read only otherwise it will return read only database instance

Choose a reason for hiding this comment

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

Not using an actual transaction for read-only operations introduces two problems:

  1. No support for isolation level,
  2. Performance (see Statements are executed more quickly in a transaction block, because transaction start/commit requires significant CPU and disk activity. in https://www.postgresql.org/docs/15/sql-begin.html).

Choose a reason for hiding this comment

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

This point is not addressed. The code still returns the "database" and not a transaction.

// As result new instance of `*gorm.DB` will be returned.
// Error will be returned in case either transaction or db connection info is missing in context.
// Gorm specific error can be checked by `*gorm.DB.Error`.
func BeginFromContext(ctx context.Context) (*gorm.DB, error) {
logger := ctxlogrus.Extract(ctx)
txn, ok := FromContext(ctx)
if !ok {
return nil, ErrCtxTxnMissing
}
if txn.readOnly == true {
logger.Warnf("BeginFromContext: Read Only DB instance already in use!")
return GetReadOnlyDBInstance(ctx)
}
if txn.parent == nil {
return nil, ErrCtxTxnNoDB
}
Expand Down Expand Up @@ -173,12 +211,12 @@ func (t *Transaction) Commit(ctx context.Context) error {
// Client is responsible to call `txn.Begin()` to open transaction.
// If call of grpc.UnaryHandler returns with an error the transaction
// is aborted, otherwise committed.
func UnaryServerInterceptor(db *gorm.DB) grpc.UnaryServerInterceptor {
func UnaryServerInterceptor(db *gorm.DB, readOnlyDB ...*gorm.DB) grpc.UnaryServerInterceptor {
txn := &Transaction{parent: db}
return UnaryServerInterceptorTxn(txn)
return UnaryServerInterceptorTxn(txn, readOnlyDB...)
}

func UnaryServerInterceptorTxn(txn *Transaction) grpc.UnaryServerInterceptor {
func UnaryServerInterceptorTxn(txn *Transaction, readOnlyDB ...*gorm.DB) grpc.UnaryServerInterceptor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should plan for the future here and use a similar WithReadOnlyDB(roDB) interface here, so we can e.g. WithLogger() as well.

return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
// Deep copy is necessary as a tansaction should be created per request.
txn := &Transaction{parent: txn.parent, afterCommitHook: txn.afterCommitHook}
Expand Down Expand Up @@ -220,6 +258,12 @@ func UnaryServerInterceptorTxn(txn *Transaction) grpc.UnaryServerInterceptor {
}()

ctx = NewContext(ctx, txn)
if len(readOnlyDB) > 0 {
dbRO := readOnlyDB[0]
if dbRO != nil {
ctx = context.WithValue(ctx, roDBKey, dbRO)
}
}
resp, err = handler(ctx, req)

return resp, err
Expand Down