Skip to content

release-22.2.0: sql/sqlutil, descs: move TxnWithExecutor() and methods to sql.InternalExecutorFactory#90041

Closed
ZhouXing19 wants to merge 4 commits intocockroachdb:deprecate-release-22.2.0from
ZhouXing19:backport22.2.0-ie
Closed

release-22.2.0: sql/sqlutil, descs: move TxnWithExecutor() and methods to sql.InternalExecutorFactory#90041
ZhouXing19 wants to merge 4 commits intocockroachdb:deprecate-release-22.2.0from
ZhouXing19:backport22.2.0-ie

Conversation

@ZhouXing19
Copy link
Copy Markdown
Collaborator

Backport 3/3 commits from #89940 and 1/2 commits from #88295

/cc https://github.com/orgs/cockroachdb/teams/release


Having TxnWithExecutor() in descs.CollectionFactory is unnatural, and will bring dependency loop headaches. This commit is to move the same logic under sql.InternalExecutorFactory.

Also, if an internal executor is created with a txn binding to it, it should not be used to execute a statement with a nil txn.

link Epic CRDB-19135

Release note: None

Release justification: Bug fixes and low-risk updates to usages of internal executor.

Having `TxnWithExecutor()` in `descs.CollectionFactory` is unnatural, and will
bring dependency loop headaches. This commit is to add the same logic under
`InternalExecutorFactory`. We will remove
`descs.CollectionFactory.TxnWithExecutor()` in a future PR.

Release note: None
This is part of commit for a mechanical move of the `TxnWithExecutor()` function
from the `descs` pkg to the `sql` package. We now use the `descs.TxnManager`
interface for the same logic.

Release note: None
…ith new interfaces

We now use `descs.TxnManager.DescsWithTxn()` and
`.DescsTxnWithExecutor()` to replace `descs.CollectionFactory.Txn()`
and `.TxnWithExecutor()`.

Release note: None
If a internal executor is created with a txn binding to it, it should not be
used to execute statement with a nil txn.

Release note: None
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Oct 16, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@celiala
Copy link
Copy Markdown
Collaborator

celiala commented Feb 9, 2023

Closing this PR so I can delete the temporary "dot zero" branch.
This change was merged into the main release branch via #89940

@celiala celiala closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants