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

aankam-infoblox · 2024-12-18T19:54:49Z

This is to support read only database instance for performing read operations on the database. Once these changes are approved and merged then all the modules can use this functionality to implement read only database support to perform database read operations in their project.

davidhankins

Yeah, I agree with the semantic changes here although they appear odd.

To try and explain my reasoning;

I think the Transaction struct is ~poorly named - it can produce one transaction from a parent database in the current ctx - and deliver that same transaction to all callers in that ctx's call graph. This is more of a "context database state" structure than specifically a transaction. To wit, we use BeginFromContext(ctx) regularly in read-only functions deeply nested in the call graph of API calls to avoid "plumbing" (passing the transaction reference as an argument down a long call chain).

So it's fine that a caller asking for a transaction is instead given a read-only-database reference because that's actually what they're after ("the database reference for this api request, read or write").

Probably on the master branch we might want to think about renaming or providing a new interface to the database-context that tries to clear up some of the confusing terminology.

Just some minor nits on my part - let's get some unit tests drafted and see about merging this.

davidhankins · 2024-12-18T21:32:11Z

gorm/transaction.go

@@ -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) {


Let's add unit tests for this new function, see transaction_test.go

davidhankins · 2024-12-18T21:32:44Z

gorm/transaction.go

 	txn, ok := FromContext(ctx)
 	if !ok {
 		return nil, ErrCtxTxnMissing
 	}
+	if txn.readOnly == true {


We'll need to update the BeginFromContext tests for this change. The existing tests do not check that a transaction from the read-write parent is returned in the current cases, and we'll have to add a test to confirm the read-only database reference is returned on a read-only context.

davidhankins · 2024-12-18T21:33:35Z

gorm/transaction.go

@@ -220,6 +258,12 @@ func UnaryServerInterceptorTxn(txn *Transaction) grpc.UnaryServerInterceptor {
 		}()

 		ctx = NewContext(ctx, txn)
+		if len(readOnlyDB) > 0 {


Let's add a check for this to TestUnaryServerInterceptorTxn_success

davidhankins · 2024-12-18T21:41:01Z

gorm/transaction.go

@@ -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) {


Let's not export this unless there's a reason to? getReadOnlyDBInstance

With GetReadOnlyDB and BeginFromContext tested, I don't think there's much value in specifically testing this function, so I don't think a unit test for it alone is required.

akleinib · 2024-12-19T12:48:15Z

It would be good to document here how this is supposed to be used. Do we need to add something like the following everywhere to fall back on R/W replica?

txn, err := GetReadOnlyDB(ctx)

if errors.Is(err, ErrNoReadOnlyDB) {
   txn, err = BeginFromContext(ctx)
}

I believe it would be much better to implement this using the standard "option" pattern, e.g.:

func BeginFromContext(ctx context.Context, OPTIONS...) (*gorm.DB, error) { }

OPTIONS can be:

If none, current behavior,
Support of https://pkg.go.dev/database/sql#TxOptions,
From read-only replica, if it doesn't exist fall back to read/write,
From read-only replica, if it doesn't exist return an error,
...

This way we can support all the current and future use cases rather keep adding functions with new prototypes.

gorm/transaction.go

akleinib · 2024-12-19T12:19:57Z

gorm/transaction.go

+		return nil, ErrCtxTxnMissing
+	}
+	if txn.current != nil {
+		logger.Warnf("GetReadOnlyDB: Txn already initialized with read/write DB")


Maybe the caller would like to know that it asked for a read-only transaction and got a read-write one instead.

Extracting the logger from the context assumes this code is used with the gRPC gateway which might not be the case.

akleinib · 2024-12-19T12:29:47Z

gorm/transaction.go

+	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


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

No support for isolation level,

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).

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

akleinib · 2024-12-23T14:45:57Z

gorm/transaction.go

@@ -18,14 +19,24 @@ import (
 // This prevents collisions with keys defined in other packages.
 type ctxKey int

+// readOnlyDBKey is an unexported type and used to define a key for storing read only db instance in the context.


Rather than storing the read-only database in the context, cannot we store it in the Transaction structure like the existing database? A nil value for this variable indicates no read-only database.

akleinib · 2024-12-23T14:48:22Z

gorm/transaction.go

+
+type DatabaseOption func(*databaseOptions)
+
+// WithRODB returns clouser to set the readOnlyReplica flag


Typo clouser.

akleinib · 2024-12-23T14:53:25Z

gorm/transaction.go

+	} else if txn.readOnly == true {
+		logger.Warnf("BeginFromContext: requested: read-write DB, returns: read-only DB, reason: txn set to read only")
+		return getReadOnlyDBInstance(ctx)
+	}


Returning a R/W transaction instead of R-O transaction is ok since all the read-only operations will be executed by the R/W transaction. Doing the inverse doesn't work. This should return an error.

akleinib · 2024-12-23T14:53:56Z

gorm/transaction.go

 	txn, ok := FromContext(ctx)
 	if !ok {
 		return nil, ErrCtxTxnMissing
 	}
+	opts := toDatabaseOptions(options...)


I would keep the check of txn.parent before adding all this new code.

akleinib · 2024-12-23T14:56:02Z

gorm/transaction.go

+		if txn.current == nil {
+			return getReadOnlyDBInstance(ctx)
+		} else {
+			logger.Warnf("BeginFromContext: requested: read-only DB, returns: read-write DB, reason: read-write DB txn in use")


It might be better to provide a WithLogger() function people can use to provide their logger instance. This way all these messages are logged with the same logger as the rest of the application. If no logger is provided, we can use a global one.

I wonder if this should be at the UnaryServerInterceptor level rather than the Transaction level.

akleinib · 2024-12-23T14:57:06Z

gorm/transaction.go

+type DatabaseOption func(*databaseOptions)
+
+// WithRODB returns clouser to set the readOnlyReplica flag
+func WithRODB(readOnlyReplica bool) DatabaseOption {


We should add a "with function" for sql.TxOptions since we have existing use-cases.

akleinib · 2024-12-23T14:59:31Z

gorm/transaction.go

+	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


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

akleinib · 2024-12-23T15:02:45Z

gorm/transaction_test.go

+			if err != nil {
+				t.Error("Received an error beginning transaction")
+			}


err should be checked before anything else returned by a function.

akleinib · 2024-12-23T15:02:56Z

gorm/transaction_test.go

+			if txn2 != txn1 {
+				t.Error("Got a different txn than was opened before")
+			}
+			if err != nil {


davidhankins · 2024-12-23T16:33:16Z

gorm/transaction.go

 }

-func UnaryServerInterceptorTxn(txn *Transaction) grpc.UnaryServerInterceptor {
+func UnaryServerInterceptorTxn(txn *Transaction, readOnlyDB ...*gorm.DB) grpc.UnaryServerInterceptor {


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

davidhankins · 2024-12-23T17:39:51Z

gorm/transaction.go

+		if txn.current == nil {
+			return getReadOnlyDBInstance(ctx)
+		} else {
+			logger.Warnf("BeginFromContext: requested: read-only DB, returns: read-write DB, reason: read-write DB txn in use")


I wonder if this should be at the UnaryServerInterceptor level rather than the Transaction level.

davidhankins · 2024-12-23T17:41:20Z

gorm/transaction.go

+			return txn.current, nil
+		}
+	} else if txn.readOnly == true {
+		logger.Warnf("BeginFromContext: requested: read-write DB, returns: read-only DB, reason: txn set to read only")


I think this is normal now, and the log line can be removed.

promoting changes to add support for read only db instance

682bdca

davidhankins reviewed Dec 18, 2024

View reviewed changes

akleinib reviewed Dec 19, 2024

View reviewed changes

aankam-infoblox added 2 commits December 21, 2024 17:45

incorporated review comments

8f6932c

incorporated review comments

689e73c

akleinib reviewed Dec 23, 2024

View reviewed changes

davidhankins reviewed Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aankam-infoblox commented Dec 18, 2024

davidhankins left a comment

davidhankins Dec 18, 2024

davidhankins Dec 18, 2024

davidhankins Dec 18, 2024

davidhankins Dec 18, 2024

akleinib commented Dec 19, 2024

akleinib Dec 19, 2024

akleinib Dec 19, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

davidhankins Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

davidhankins Dec 23, 2024

davidhankins Dec 23, 2024

davidhankins Dec 23, 2024


		type DatabaseOption func(*databaseOptions)

		// WithRODB returns clouser to set the readOnlyReplica flag

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

Are you sure you want to change the base?

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

Conversation

aankam-infoblox commented Dec 18, 2024

davidhankins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akleinib commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment