refactor: DB transactions context #2513

nasdf · 2024-04-10T00:38:55Z

Relevant issue(s)

Resolves #2512
Resolves #2516

Description

This PR moves the db transactions to the context.

Notable Changes:

moved txn_db.go to store.go
~~replaced explicitTxnDB and implicitTxnDb with store~~
~~added db/session.go to simplify setting context~~
added db/context.go to manage context values

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

fredcarle · 2024-04-10T01:58:47Z

db/session/session.go

+type contextKey string
+
+const (
+	TxnContextKey = contextKey("txn")


suggestion: To avoid allocation on the key, you can simply use

type TxnContextKey struct{}

https://pkg.go.dev/context#WithValue

fredcarle · 2024-04-11T16:11:16Z

db/collection.go

 	if err != nil {
 		return nil, err
 	}
-
+	txn := mustGetContextTxn(ctx)


thought: It's a little weird that whe need to extract the txn from the context and the next function call passes the context and the transaction.

question: Why not leave it to the function that needs the txn to get it from the context?

Someone had a preference for this in the standup discussion. If a function requires a transaction it should remain as a parameter and not come from context.

fredcarle · 2024-04-11T16:28:19Z

db/context.go

+//
+// If a transactions exists on the context it will be made explicit,
+// otherwise a new implicit transaction will be created.
+func ensureContextTxn(ctx context.Context, db transactionDB, readOnly bool) (context.Context, error) {


suggestion: There is a lot of ensureContextTxn followed by mustGetContextTxn. To avoid having to type cast everytime, I would suggest returning datastore.Txn here.

func ensureContextTxn(ctx context.Context, db transactionDB, readOnly bool) (context.Context, datastore.Txn, error)

fredcarle · 2024-04-11T16:40:33Z

db/store.go

+
+var _ client.Store = (*store)(nil)
+
+type store struct {


question: Can you explain why you prefer wrapping the db instead of using it directly for the methods bellow?

Good point. There's no need for a separate struct anymore. Would you prefer these functions remain in store.go or would it be better to move them into db.go?

store.go is fine with me. We could even add documentation at the top saying that the fonction within implement the client.Store interface for db.

fredcarle · 2024-04-11T16:48:49Z

http/handler_ccip.go

@@ -35,7 +35,7 @@ type CCIPResponse struct {

 // ExecCCIP handles GraphQL over Cross Chain Interoperability Protocol requests.
 func (c *ccipHandler) ExecCCIP(rw http.ResponseWriter, req *http.Request) {
-	store := req.Context().Value(storeContextKey).(client.Store)
+	db := req.Context().Value(dbContextKey).(client.DB)


thought: There is nothing wrong with keeping the more restrictive type casting of client.Store since that is really what is needed here.

fredcarle · 2024-04-11T17:12:46Z

net/server.go

@@ -349,15 +353,14 @@ func (s *server) syncIndexedDocs(
 	ctx context.Context,
 	col client.Collection,
 	docID client.DocID,
+	txn datastore.Txn,


todo: Remove unused param.

fredcarle

I like the changes. I'll just wait for the comments to be resolved before approving.

I'm wondering if we should remove the txn from function params when the context already holds it. Probably out of scope for this PR but could be done shortly after.

Edit: Will bring it up in the standup.

jsimnz · 2024-04-11T19:43:15Z

I'm not a huge fan of this PR, not sure if its the specific implementation or the general concept (txn via context). This was my concern when discussing it in the ACP discord thread regarding the use of txn via the context making the depedency more opaque. It can make following the call stack of a given transaction harder to reason about and easier to mess up.

Heres an example in the basicImport func

It has the following function signature

It uses the txn as an argument to a follow-up DB call (note the private method being called which specifically has the txn argument)

Then makes a call to the a public function which doesn't use the txn

.

It is not 100% clear the txn scope the last call is using, since the calling function (basicImport) is explicitly given a txn.

Before, the last line was col.WithTxn(txn).Create(...) which makes it very obvious the txn being used.

--

Theres a possible solution here:
All actual DB/Collection functionality is implemented in the private funcs (which we basicallly do now), all private funcs only call private funcs, and private funcs take txn as a input args.

in fact after reviewing this, and the existing (develop) implementation, there is actually a bug where some call paths don't respect transaction scopes correctly because of the implicit txn creation on the Collection public calls.

Oddly enough, this PR actually fixes that without intending to because of the shared context.

Perhaps my issue isn't necessarily with the transaction context propagation, but the inconsistency we have between implicit and explicit transactions and how they are handled, both on the db object and the Collection object.

Happy to chat more during the dev call today (cc @nasdf @fredcarle )

islamaliev · 2024-04-11T20:39:51Z

db/context.go

+
+// SetContextTxn returns a new context with the txn value set.
+func SetContextTxn(ctx context.Context, txn datastore.Txn) context.Context {
+	return context.WithValue(ctx, txnContextKey{}, txn)


todo: I think we need to check here if there is already an existing transaction in the context. I there is one, current code will override it allowing potentially to leak the old transaction that was not discarded yet.

The ensureContextTxn makes sure we don't overwrite an existing transaction. There is a case within net/server.go where we need the functionality to overwrite the transaction.

not sure if having ensureContextTxn is enough in this case. SetContextTxn is a public method and can be called from anywhere. If ensureContextTxn is required to call before calling this one, why not to make it part of it? Otherwise I see no link between the two.

I'm not seeing the issue here. The lifetime of the transaction is managed from the caller. If the caller overwrites a transaction and forgets to discard it, then the bug is in the caller's code.

if the intention is to put this responsibility on the caller's side, it's fine, but then it needs to be documented.

I've updated the function docs to be more clear.

fredcarle

LGTM

nasdf added 3 commits April 9, 2024 10:59

move db txn to context. add db session helper

23fc4ec

move db session to subpackage. replace collection.WithTxn

19b7db3

remove lens registry WithTxn

447d956

nasdf added the area/db-system Related to the core system related components of the DB label Apr 10, 2024

nasdf added this to the DefraDB v0.11 milestone Apr 10, 2024

nasdf self-assigned this Apr 10, 2024

nasdf changed the title ~~refactor: DB session~~ refactor: DB transactions context Apr 10, 2024

fredcarle reviewed Apr 10, 2024

View reviewed changes

nasdf and others added 4 commits April 10, 2024 10:31

ensure internal db methods have correct transaction context

3fa4b97

cli and http context transaction fixes

2c6b92f

fix cli db context

e4ecbd8

Merge branch 'develop' into nasdf/refactor/db-session

999e864

nasdf marked this pull request as ready for review April 10, 2024 18:39

replace db session with simpler context api

caa3afa

nasdf requested a review from a team April 10, 2024 20:32

fredcarle reviewed Apr 11, 2024

View reviewed changes

return transaction from ensureContextTxn

3673d05

fredcarle reviewed Apr 11, 2024

View reviewed changes

nasdf added 3 commits April 11, 2024 09:52

merge db.store funcs into db

fbed9d0

preserve client.Store type in cli

73219a9

preserve client.Store type in http

f65a767

fredcarle reviewed Apr 11, 2024

View reviewed changes

nasdf added 2 commits April 11, 2024 10:19

review fixes

5962357

fix ensureContextTxn

743ddcf

islamaliev reviewed Apr 11, 2024

View reviewed changes

update SetContextTxn docs

83e0d7a

Merge branch 'develop' into nasdf/refactor/db-session

8d88d12

nasdf requested a review from fredcarle April 12, 2024 16:39

fredcarle approved these changes Apr 12, 2024

View reviewed changes

nasdf merged commit ef228b8 into sourcenetwork:develop Apr 12, 2024
26 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: DB transactions context #2513

refactor: DB transactions context #2513

nasdf commented Apr 10, 2024 •

edited

Loading

fredcarle Apr 10, 2024

fredcarle Apr 11, 2024

nasdf Apr 11, 2024

fredcarle Apr 11, 2024

fredcarle Apr 11, 2024

nasdf Apr 11, 2024

fredcarle Apr 11, 2024

fredcarle Apr 11, 2024

fredcarle Apr 11, 2024

fredcarle left a comment •

edited

Loading

jsimnz commented Apr 11, 2024

islamaliev Apr 11, 2024

nasdf Apr 11, 2024

islamaliev Apr 11, 2024

nasdf Apr 11, 2024 •

edited

Loading

islamaliev Apr 12, 2024

nasdf Apr 12, 2024

fredcarle left a comment

refactor: DB transactions context #2513

refactor: DB transactions context #2513

Conversation

nasdf commented Apr 10, 2024 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

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

fredcarle left a comment • edited Loading

Choose a reason for hiding this comment

jsimnz commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasdf Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

nasdf commented Apr 10, 2024 •

edited

Loading

fredcarle left a comment •

edited

Loading

nasdf Apr 11, 2024 •

edited

Loading