[DNM] kv/kvclient: ref count LeafTxns and disallow RootTxn use #99269
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
This does not work yet because DistSQL uses a single call into `GetLeafTxnInputState` to construct multiple LeafTxn instances.
nvanbenschoten
force-pushed
the
nvanbenschoten/refCountLeafTxn
branch
from
af8d243
to
7dde115
Compare
@yuzefovich let me know if you'd like to talk about this. Do we know how many LeafTxns we're planning to instantiate when calling |
There was a problem hiding this comment.
Do we know how many LeafTxns we're planning to instantiate when calling
GetLeafTxnInputStateOrRejectClient?
This seems tractable. If we create a LeafTxn, then we will make exactly one for the whole flow, and we will create an extra one for each streamer in the flow. Are you envisioning changing the signature of this method to take in the expected number of LeafTxns that will be created?
A possible complication is that if we have a query that will result in other "inner" queries (e.g. via the internal executor or apply join iterations), then each of those "inner" queries will call RooTxn.GetLeafTxnInputStateOrRejectClient).
Also, can we trust that each LeafTxn will result in exactly one call to
UpdateRootWithLeafFinalState?
This is not true, but also seems tractable. We call UpdateRootWithLeafFinalState for each execinfrapb.ProducerMetadata object that has LeafTxnFinalState set. Such an object is generated by each DistSQL processor that performs reads using the LeafTxn, so the total number of objects depends on 1) number of nodes, and 2) types of processors in each flow.
I think we'd need to iterate over the FlowSpecs for the whole distributed plan and count the number of "reader" processors in each flow. Again, things will be more complicated if we need to know in advance the number of execinfrapb.ProducerMetadata objects that will be created by "inner" nested plans.
However, it seems like we don't need to consider the "inner" nested queries.
Reviewable status:
complete! 0 of 0 LGTMs obtained
99378: kv/kvclient: remove GetLeafTxnInputState{OrRejectClient} split r=arulajmani a=nvanbenschoten
Informs #99269.
This PR includes two refactors with no production behavior changes. They combine to make the reference counting attempted in #99269 easier to implement.
### kv/kvclient: introduce txn.GetReadSeqNum
This commit introduces a new `txn.GetReadSeqNum` method.
It then uses this method to avoid unnecessarily heavyweight calls into GetLeafTxnInputState. It turns out that this removes all production calls into `GetLeafTxnInputState`.
### kv/kvclient: remove GetLeafTxnInputState{OrRejectClient} split
This commit removes the split between the `txn.GetLeafTxnInputState` and `txn.GetLeafTxnInputStateOrRejectClient` methods, consolidating on a single method that behaves like the old `txn.GetLeafTxnInputStateOrRejectClient` method behaved. As of the previous commit, there were no remaining production callers of `txn.GetLeafTxnInputState`.
This split between the two methods was introduced in d8630d0, which was a targeted fix that did not questions whether all callers of `GetLeafTxnInputState` (`GetTxnCoordMeta` at the time) could be switched to have this new behavior. It may have also been more difficult to switch all callers over before c00ea84 and 0461b00.
In consolidating this logic, we also remove the `kv.TxnStatusOpt` enum and simplify the `kv.TxnSender` interface.
Release note: None
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
This does not work yet because DistSQL uses a single call into
GetLeafTxnInputStateto construct multiple LeafTxn instances.