Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: request option to prevent splitting rows in responses #73618

Closed
erikgrinaker opened this issue Dec 8, 2021 · 0 comments · Fixed by #74602
Closed

kvserver: request option to prevent splitting rows in responses #73618

erikgrinaker opened this issue Dec 8, 2021 · 0 comments · Fixed by #74602
Assignees
Labels
A-kv-server Relating to the KV-level RPC server C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@erikgrinaker
Copy link
Contributor

SQL rows can be split across multiple KV pairs. During range splits, we take this into account at the MVCC level to prevent splitting a row across multiple ranges by decoding the KV pairs:

cockroach/pkg/storage/mvcc.go

Lines 3881 to 3892 in 2e038a1

if _, tenID, err := keys.DecodeTenantPrefix(it.UnsafeKey().Key); err == nil {
if _, _, err := keys.MakeSQLCodec(tenID).DecodeTablePrefix(it.UnsafeKey().Key); err == nil {
// The first key in this range represents a row in a SQL table. Advance the
// minSplitKey past this row to avoid the problems described above.
firstRowKey, err := keys.EnsureSafeSplitKey(it.Key().Key)
if err != nil {
return nil, err
}
// Allow a split key before other rows in the same table.
minSplitKey = firstRowKey.PrefixEnd()
}
}

However, when doing limited scans (with MaxSpanRequestKeys or TargetBytes limits), the response can end up returning a partial row, causing the client to either have to discard it or run another request to complete the row. This is particularly relevant for the streaming client API in #68430.

We should add a Header parameter that prevents these limits from splitting rows. Since these limits are now strict limits (i.e. we will discard the last value that exceeds TargetBytes), this implies that we'll discard these partial rows to keep below the limits unless it is the first row in the result. This needs to be plumbed down into MVCC where the limits are applied:

// Check if we actually exceeded the limit.
if size >= p.targetBytes || (p.targetBytesAvoidExcess && size+nextSize > p.targetBytes) {
p.curExcluded = true
p.resumeReason = roachpb.RESUME_BYTE_LIMIT
return false
}

Note that even though this option will currently only apply to Scan and ReverseScan requests, we add it to Header since that is where the related limits are configured.

@erikgrinaker erikgrinaker added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-server Relating to the KV-level RPC server T-kv-replication labels Dec 8, 2021
@erikgrinaker erikgrinaker self-assigned this Dec 8, 2021
@craig craig bot closed this as completed in ebda0ec Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-server Relating to the KV-level RPC server C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant