upgrades: fix txn retry bug in upgrade batching #104752
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes: cockroachdb#104653 Release note (bug fix): fixes a bug where a txn retry during the backfill of the jobs info table could result in job rows being missed
|
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 |
dt
approved these changes
Jun 12, 2023
|
blathers backport 23.1 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 947e34f to blathers/backport-release-23.1-104752: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
stevendanna
approved these changes
Jun 12, 2023
|
bors r=dt,stevendanna |
|
bors r- |
|
Canceled. |
|
bors r+ p=999 single-on |
|
Timed out. |
|
bors r+ single on |
|
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: {"message":"You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
|
bors r+ single on |
|
Build failed: |
|
bors r+ single on |
|
Build succeeded: |
stevendanna
added a commit
to stevendanna/cockroach
that referenced
this pull request
Jul 7, 2023
In cockroachdb#105750 we split the backfill of the job_type column across multiple transactions. This introduced a bug in which we would modify the resumeAfter variable that controlled the batching before the transaction succeeded. In the face of a transaction retry, this would result in some rows not having their job_type column populated. This was caught in nightly tests that attempted to use the crdb_internal.system_jobs virtual index on the job_type column. Here, we apply the same fix that we applied in cockroachdb#104752 for the same type of bug. Fixes cockroachdb#106347 Fixes cockroachdb#106246 Release note (bug fix): Fixes a bug where a transaction retry during the backfill of the job_type column in the jobs table could result some job records with no job_type value.
craig bot
pushed a commit
that referenced
this pull request
Jul 7, 2023
106236: admission: avoid recursive grant chain r=irfansharif a=irfansharif Fixes #105185. Fixes #105613. In #97599 we introduced a non-blocking admission interface for below-raft, replication admission control. When doing so, we unintentionally violated the 'requester' interface -- when 'requester.granted()' is invoked, the granter expects to admit a single queued request. The code layering made it so that after granting one, when doing post-hoc token adjustments, if we observed the granter was exhausted but now no longer was so, we'd try to grant again. This resulted in admitting work recursively, with a callstack as deep as the admit chain. Not only is that undesirable design-wise, it also triggered panics in the granter that wasn't expecting more than one request being admitted. Recursively we were incrementing the grant chain index, which overflowed (it was in int8, so happened readily with long enough admit chains), after which we panic-ed when using a negative index to access an array. We add a test that fails without the changes. The failure can also be triggered by applying the diff below (which reverts to the older, buggy behavior): ``` dev test pkg/kv/kvserver -f TestFlowControlGranterAdmitOneByOne -v --show-logs ``` ```diff diff --git i/pkg/util/admission/granter.go w/pkg/util/admission/granter.go index ba42213c375..7c526fbb3d8 100644 --- i/pkg/util/admission/granter.go +++ w/pkg/util/admission/granter.go `@@` -374,7 +374,7 `@@` func (cg *kvStoreTokenChildGranter) storeWriteDone( func (cg *kvStoreTokenChildGranter) storeReplicatedWorkAdmittedLocked( originalTokens int64, admittedInfo storeReplicatedWorkAdmittedInfo, ) (additionalTokens int64) { - return cg.parent.storeReplicatedWorkAdmittedLocked(cg.workClass, originalTokens, admittedInfo, false /* canGrantAnother */) + return cg.parent.storeReplicatedWorkAdmittedLocked(cg.workClass, originalTokens, admittedInfo, true /* canGrantAnother */) } ``` Release note: None 106378: upgrades: fix txn retry bug in upgrade batching r=adityamaru a=stevendanna In #105750 we split the backfill of the job_type column across multiple transactions. This introduced a bug in which we would modify the resumeAfter variable that controlled the batching before the transaction succeeded. In the face of a transaction retry, this would result in some rows not having their job_type column populated. This was caught in nightly tests that attempted to use the crdb_internal.system_jobs virtual index on the job_type column. Here, we apply the same fix that we applied in #104752 for the same type of bug. Fixes #106347 Fixes #106246 Release note (bug fix): Fixes a bug where a transaction retry during the backfill of the job_type column in the jobs table could result some job records with no job_type value. 106408: ci: remove `lint` job from GitHub CI r=rail a=rickystewart With `staticcheck` and `unused` working identically under `lint` in Bazel and `make` now, it's time! Delete this file so that GitHub CI lint stops running. This is the *last* GitHub CI job. :) Now only Bazel builds and tests will run on PR's. Epic: CRDB-15060 Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Steven Danna <danna@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
blathers-crl bot
pushed a commit
that referenced
this pull request
Jul 7, 2023
In #105750 we split the backfill of the job_type column across multiple transactions. This introduced a bug in which we would modify the resumeAfter variable that controlled the batching before the transaction succeeded. In the face of a transaction retry, this would result in some rows not having their job_type column populated. This was caught in nightly tests that attempted to use the crdb_internal.system_jobs virtual index on the job_type column. Here, we apply the same fix that we applied in #104752 for the same type of bug. Fixes #106347 Fixes #106246 Release note (bug fix): Fixes a bug where a transaction retry during the backfill of the job_type column in the jobs table could result some job records with no job_type value.
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #104545 we broke up the txn that is responsible for backfill the
system.job_infotable as part of an upgrade. That diff had a bug where a txn retry inside thedb.Txnclosure could result in us skipping rows to backfill. The consequence of this is that some jobs will not have their payload and progress copied over from thesystem.jobstable to thesystem.job_infotable. This is bad because once the cluster is fully upgraded, the job system will only consult thesystem.job_infotable during execution. When it does so, the job is destined to fail as there will be no payload or progress entry corresponding to that job.Fixes: #104653
Release note (bug fix): fixes a bug where a txn retry during the backfill of the jobs info table could result in job rows being missed