Skip to content

Conversation

@blathers-crl
Copy link

@blathers-crl blathers-crl bot commented May 29, 2025

Backport 1/1 commits from #147501 on behalf of @msbutler.


Previously, the auxillary job system tables were updated with an update stmt instead of a delete-then-insert approach to avoid an extra roundtrip to the kv layer. This optimization also included faulty error handling against a pk unique constraint violation. Specifically, these update paths would issue a delete if the internal executor returned an error on the update; however, the delete used the same txn as the original update, and the internal executor does not automatically clean up any uncommited data from the original update (i.e. via a savepoint rollback).

To avoid further complicating this optimization by wrapping the update in a savepoint, this commit simply reverts the update path to the original delete then insert pattern.

Epic: none

Release note: none


Release justification:

Previously, the auxillary job system tables were updated with an update stmt
instead of a delete-then-insert approach to avoid an extra roundtrip to the kv
layer. This optimization also included faulty error handling against a pk
unique constraint violation. Specifically, these update paths would issue a
delete if the internal executor returned an error on the update; however, the
delete used the same txn as the original update, and the internal executor does
not automatically clean up any uncommited data from the original update (i.e.
via a savepoint rollback).

To avoid further complicating this optimization by wrapping the update in a
savepoint, this commit simply reverts the update path to the original delete
then insert pattern.

Epic: none

Release note: none
@blathers-crl blathers-crl bot requested review from a team as code owners May 29, 2025 22:28
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-25.2-147501 branch from 4223a76 to 6a4e1fe Compare May 29, 2025 22:28
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels May 29, 2025
@blathers-crl blathers-crl bot removed the request for review from a team May 29, 2025 22:28
@blathers-crl blathers-crl bot added the blathers-backport This is a backport that Blathers created automatically. label May 29, 2025
@blathers-crl blathers-crl bot requested a review from kev-cao May 29, 2025 22:28
@blathers-crl
Copy link
Author

blathers-crl bot commented May 29, 2025

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • 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. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot requested a review from jeffswenson May 29, 2025 22:28
@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label May 29, 2025
@blathers-crl
Copy link
Author

blathers-crl bot commented May 29, 2025

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@jeffswenson jeffswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@msbutler msbutler merged commit 59aec01 into release-25.2 Jun 2, 2025
14 of 15 checks passed
@msbutler msbutler deleted the blathers/backport-release-25.2-147501 branch June 2, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. v25.2.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants