[SERVER-40200] Address "query_exec" dependency on "transaction" library. Created: 18/Mar/19  Updated: 29/Oct/23  Resolved: 27/Mar/19

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 4.1.10

Type: Improvement Priority: Major - P3
Reporter: Justin Seyster Assignee: Janna Golden
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-39676 Investigate possibility of adapting T... Closed
Backwards Compatibility: Fully Compatible
Sprint: Sharding 2019-03-25, Sharding 2019-04-08
Participants:

 Description   

Planned changes to the TransactionHistoryIterator will require it to call into the query_lib's getExecutorFind() function. However, query_lib already has a dependency on the transaction library, so this call would create a dependency cycle.

It looks like this dependency is the result of UpdateStage::assertDocStillBelongsToNode(). Is there some way we can break that dependency so that the transaction library can safely depend on query_lib?



 Comments   
Comment by Githook User [ 27/Mar/19 ]

Author:

{'email': 'golden.janna@gmail.com', 'name': 'jannaerin', 'username': 'jannaerin'}

Message: SERVER-40200 remove query_exec lib dependency on transaction library
Branch: master
https://github.com/mongodb/mongo/commit/ea0b5bc2587157daf9e31890feb8b0209ce4e62d

Comment by Siyuan Zhou [ 19/Mar/19 ]

Agreed. States like whether we are inside a transaction or not should be accessed without linking against the whole transaction library. Currently, the member variables of operation context and its decorations serve this purpose. Maybe in the future we need to go further than that to pull out more states, following how sharding pulled out the sharding API for upper level usage. The implementation of transaction shouldn't be linked anyway though.

Comment by Kaloian Manassiev [ 19/Mar/19 ]

Assigning to janna.golden to see whether this dependency on the transactions library could be abstracted.

I kind of think though, that being able to access the active transaction's state from within an update is not architecturally wrong. Perhaps the parts of the transactions library, which use DBDirectClient can be pulled out?

Comment by Siyuan Zhou [ 18/Mar/19 ]

kaloian.manassiev, this is the issue we discussed the other day and it turned out to be blocking the change stream work. I think this ticket is what we need. Transaction is implemented by writing and reading the metadata in transaction table, so it's natural to have TransactionHistoryIterator to depend on query. Currently, DBDirectClient is used, so the dependency cycle already exists but it's just hidden.

The only place where query needs transaction is here. That logic needs to know whether we are in a transaction or retryable write but it doesn't depend on the implementation of transaction. There are perhaps other ways to achieve this. For example, we can check the read concern: snapshot to know whether users intend to run a transaction. Checking the transaction number will tell us whether a session is used. Checking the opCtx->getWriteUnitOfWork() can tell whether the transaction is active or not. Hope we can remove the cycle somehow.

Comment by Kaloian Manassiev [ 18/Mar/19 ]

It doesn't seem right that the transactions library should have a dependency on query. Transactions is a very low level concept and if anything, query should have dependency on it.

What is the scenario for adding this dependency?

Generated at Thu Feb 08 04:54:19 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.