storage/concurrency: add support for non-transactional writes to#44975
storage/concurrency: add support for non-transactional writes to#44975craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)
pkg/storage/concurrency/lock_table.go, line 556 at r1 (raw file):
waitForTxn = l.reservation.txn waitForTs = l.reservation.ts if !findDistinguished && l.distinguishedWaiter.txn != nil &&
Consider adding the following method to avoid all of these new nil-txn checks:
func (g *lockTableGuardImpl) isTxn(txn *enginepb.TxnMeta) bool {
return g.txn != nil && g.txn.ID == txn.ID
}
You could probably eliminate a few existing checks as well.
pkg/storage/concurrency/lock_table.go, line 716 at r1 (raw file):
waitForTs = l.reservation.ts reservedBySelfTxn = g.txn != nil && g.txn.ID == waitForTxn.ID if g.txn == nil && sa == spanset.SpanReadWrite && l.reservation.seqNum > g.seqNum {
The sa == spanset.SpanReadWrite is interesting. Is it needed? Is the corresponding sa == spanset.SpanReadOnly case already doing the same thing below (i.e. ignoring the reservation). Should it?
pkg/storage/concurrency/testdata/lock_table, line 1569 at r1 (raw file):
res: req: 28, txn: 00000000-0000-0000-0000-000000000003, ts: 0.000000010,0 local: num=0
Could you call guard-state r=req27 and guard-state r=req28 here and then print again?
This creates room for new concurrency_manager testdata. Large diff, but just an outdent. We should probably get this in because cockroachdb#44975 so that we can put those new test cases in a new file.
44965: backup,import: count system table rows as rows r=dt a=dt Previously we counted any KVs belonging to non-user tables as 'system records' rather than as 'rows' or 'index entries' the way they are counted for regular tables. This however meant that BACKUP or RESTORE would confusingly say that it had backed up 0 rows from a given system table when it in fact backed up its rows normally, unless one separately looked at the 'system record' count. Even if one does so, that is also confusing since it doesn't distinguish rows from index entries so the count does not match. Given that these counts are rolled up by table anyway the distinction gains us little. Release note: none. 45010: storage/concurrency: move lock_table testdata to directory r=nvanbenschoten a=nvanbenschoten This creates room for new concurrency_manager testdata. There is a large diff, but it's just an outdent. We should probably get this in because #44975 so that we can put those new test cases in a new file. Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/concurrency/lock_table.go, line 556 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Consider adding the following method to avoid all of these new nil-txn checks:
func (g *lockTableGuardImpl) isTxn(txn *enginepb.TxnMeta) bool { return g.txn != nil && g.txn.ID == txn.ID }You could probably eliminate a few existing checks as well.
Done
pkg/storage/concurrency/lock_table.go, line 716 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
The
sa == spanset.SpanReadWriteis interesting. Is it needed? Is the correspondingsa == spanset.SpanReadOnlycase already doing the same thing below (i.e. ignoring the reservation). Should it?
It isn't needed for correctness, but non-transactional and transactional reads are handled in the same way later. So this piece of code is meant to only handle non-transactional writes since they have specialized behavior related to not making reservations and only waiting for reservations in some cases. I've added a code comment.
pkg/storage/concurrency/testdata/lock_table, line 1569 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Could you call
guard-state r=req27andguard-state r=req28here and then
Done
nvb
left a comment
There was a problem hiding this comment.
testdata/lock_table/non_txn file. Once that's done, feel free to merge and I'll rebase #44885 on top of this.
Reviewed 3 of 3 files at r2.
Reviewable status:
lockTable. And updated the datadriven and randomized tests to exercise non-transactional writes. Release note: None
sumeerbhola
left a comment
There was a problem hiding this comment.
this will need to be rebased on #45010, and I think you'll probably want to pull the new test cases into a testdata/lock_table/non_txn file.
Done
Reviewable status:
|
bors r+ |
Build succeeded |
lockTable.
And updated the datadriven and randomized tests to exercise
non-transactional writes.
Release note: None