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.

gorm/transaction.go

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.

gorm/transaction.go

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.

gorm/transaction.go

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.

gorm/transaction.go

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.

gorm/transaction_test.go

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.

gorm/transaction.go

akleinib · 2025-01-07T15:05:17Z

gorm/transaction.go

+				txn.txOpts = &sql.TxOptions{}
+				txn.txOpts.ReadOnly = opts.txOpts.ReadOnly
+				txn.txOpts.Isolation = opts.txOpts.Isolation


Cannot we use the following instead?

txn.txOpts = &sql.TxOptions{*opts.txOpts}

(see above comment I think we might want to just store opts directly rather than have two fields)

gorm/transaction.go

gorm/transaction_test.go

akleinib · 2025-01-07T15:13:18Z

gorm/transaction_test.go

+	}
+	mock.ExpectBegin()
+	mock.ExpectCommit()
+	dbROMock.ExpectBegin()


We don't expect a read-only transaction to be started.

ExpectationsWereMet() isn't called on dbROMock unlike mock.

akleinib · 2025-01-07T15:30:58Z

gorm/transaction_test.go

+	mock.ExpectBegin()
+	mock.ExpectCommit()
+	dbROMock.ExpectBegin()


Don't we expect no R/W transaction being used and a R-O transaction being committed?

davidhankins · 2025-01-07T18:21:41Z

gorm/transaction.go

 	current         *gorm.DB
+	txOpts          *sql.TxOptions


Consider using e.g. currentOpts *databaseOptions rather than separately txOpts and currentDB etc.

I think you could more directly compare "current and prior options" that way; just keep the options value from when the transaction was started and compare options to options.

davidhankins · 2025-01-07T18:22:25Z

gorm/transaction.go

 	afterCommitHook []func(context.Context)
 }

+type databaseOptions struct {
+	readOnlyReplica bool


Consider using dbType here (so you can have notset / readonly / readwrite as a trinary instead of boolean value).

davidhankins · 2025-01-07T18:30:50Z

gorm/transaction.go

+// getReadOnlyDBInstance returns the read only db txn if RO DB available otherwise it returns read/write db txn
+func getReadOnlyDBTxn(ctx context.Context, opts *databaseOptions, txn *Transaction) (*gorm.DB, error) {
+	var db *gorm.DB
+	if txn.parentRO == nil {


Try to restructure the if / else / if / if / else / ... chain here as a switch {.

You might have to have nested levels of switch { or some if conditions inside cases, but I think it would read better e.g.

switch { case txn.parentRO == nil: return ... case opts.txOpts != nil: ... handle case where txn.txOpts is or isn't nil (nested if) case txn.txOpts != nil: ... at this point you know opts.txOpts is nil. default: ... }

davidhankins · 2025-01-07T18:31:19Z

gorm/transaction.go

+				txn.txOpts = &sql.TxOptions{}
+				txn.txOpts.ReadOnly = opts.txOpts.ReadOnly
+				txn.txOpts.Isolation = opts.txOpts.Isolation


(see above comment I think we might want to just store opts directly rather than have two fields)

gorm/transaction_test.go

davidhankins · 2025-01-08T16:32:58Z

gorm/transaction.go

+	default:
+		// We should error in two cases 1. We should error if read-only DB requested with read-write txn
+		// 2. If no txn options provided in previous call but provided in subsequent call
+		if opts.txOpts != nil {
+			if opts.txOpts.ReadOnly == false || txn.currentOpts.database != dbNotSet {
+				return nil, ErrCtxTxnOptMismatch
+			}
+			txnOpts := *opts.txOpts
+			txn.currentOpts.txOpts = &txnOpts
+		}
+	}
+	if txn.current != nil {


you can collapse these two and dedent a bit as well;

case opts.txOpts != nil: if opts.txOpts.ReadOnly == false ... case txn.current != nil: return txn.current, nil

i quite like this layout, thanks for updating. ^_^

update: you can't move the txn.current != nil test into the switch, sorry about that.

davidhankins · 2025-01-08T16:34:16Z

gorm/transaction.go

+	case txn.parentRO == nil:
+		return getReadWriteDBTxn(ctx, opts, txn)
+	case opts.txOpts != nil && txn.currentOpts.txOpts != nil:
+		if opts.txOpts.ReadOnly != txn.currentOpts.txOpts.ReadOnly || opts.txOpts.Isolation != txn.currentOpts.txOpts.Isolation {


can we simplify to *opts.txOpts != *txn.currentOpts.txOpts?

davidhankins

LGTM, but we'll wait and see if Arnaud is available to confirm his questions are addressed.

davidhankins · 2025-01-08T19:09:29Z

gorm/transaction_test.go

+					t.Error("Did not receive a transaction from context")
+				}
+				// Case: Transaction begin is idempotent
+				if txn1 != txn4 {


i think we previous checked that txn1 != nil so you could omit the txn4 == nil case if you wanted to, but it works either way and you might want the more precise error message for the nil case, so not blocking.

promoting changes to add support for read only db instance

682bdca

davidhankins reviewed Dec 18, 2024

View reviewed changes

gorm/transaction.go Outdated Show resolved Hide resolved

gorm/transaction.go Outdated Show resolved Hide resolved

gorm/transaction.go Outdated Show resolved Hide resolved

gorm/transaction.go Outdated Show resolved Hide resolved

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

incorporated review comments

0dc2e3d

akleinib reviewed Jan 7, 2025

View reviewed changes

davidhankins reviewed Jan 7, 2025

View reviewed changes

incorporated the review comments

1b2f990

akleinib reviewed Jan 8, 2025

View reviewed changes

davidhankins reviewed Jan 8, 2025

View reviewed changes

aankam-infoblox added 2 commits January 8, 2025 13:25

incorporated the review comments

a0621ec

fixed the lint issues

f274246

davidhankins approved these changes Jan 8, 2025

View reviewed changes

akleinib approved these changes Jan 8, 2025

View reviewed changes

davidhankins merged commit d104685 into infobloxopen:v0.25 Jan 8, 2025
2 checks passed

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

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

davidhankins Dec 23, 2024

akleinib Dec 23, 2024

akleinib Dec 23, 2024

davidhankins Dec 23, 2024

davidhankins Dec 23, 2024

davidhankins Dec 23, 2024

akleinib Jan 7, 2025

davidhankins Jan 7, 2025

akleinib Jan 7, 2025

akleinib Jan 7, 2025

davidhankins Jan 7, 2025

davidhankins Jan 7, 2025

davidhankins Jan 7, 2025

davidhankins Jan 7, 2025

davidhankins Jan 8, 2025

davidhankins Jan 8, 2025

davidhankins Jan 8, 2025

davidhankins left a comment

davidhankins Jan 8, 2025

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

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

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

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

davidhankins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment