multitenant: retain range splits after TRUNCATE for secondary tenants#93325
multitenant: retain range splits after TRUNCATE for secondary tenants#93325craig[bot] merged 1 commit intocockroachdb:masterfrom ecwall:multitenant_retain_splits_after_truncate
Conversation
|
This change is |
arulajmani
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @rafiss)
pkg/sql/truncate.go line 409 at r2 (raw file):
} nNodes := execCfg.NodeDescs.GetNodeDescriptorCount()
As you've correctly pointed out, this runs into #93348. Interestingly, what you're replacing here, also wasn't great -- the kv_node_status table doesn't actually account for node liveness. So, if a node was (say) decommissioned, we might still be using it in this calculation as long as its descriptor's TTL in gossip hadn't expired.
We should instead have been using the kv_node_liveness table to begin with. Which brings about a couple of questions -- do we envision making this table available to tenants? If we do, should we build this PR on top of that?
@knz do you have thoughts about this? Or are we happy with this general approach for now?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/truncate.go line 409 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
As you've correctly pointed out, this runs into #93348. Interestingly, what you're replacing here, also wasn't great -- the
kv_node_statustable doesn't actually account for node liveness. So, if a node was (say) decommissioned, we might still be using it in this calculation as long as its descriptor's TTL in gossip hadn't expired.We should instead have been using the
kv_node_livenesstable to begin with. Which brings about a couple of questions -- do we envision making this table available to tenants? If we do, should we build this PR on top of that?@knz do you have thoughts about this? Or are we happy with this general approach for now?
The SQL layer has no business inspecting nodes and the number of nodes to do this computation.
There should be a new KV endpoint that does this:
- enumerate the split points within the table/index key span
- return the list of current split points to the SQL layer
- also returns alongside the split points:
- a recommended number of splits (nSplits)
- a recommended expiration time (computed off
SplitByLoadMergeDelay)
The only remaining code in SQL should be to generate new split points (based on the KV returned details) and issue the AdminSplit/Scatter commands.
ecwall
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/truncate.go line 409 at r2 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
The SQL layer has no business inspecting nodes and the number of nodes to do this computation.
There should be a new KV endpoint that does this:
- enumerate the split points within the table/index key span
- return the list of current split points to the SQL layer
- also returns alongside the split points:
- a recommended number of splits (nSplits)
- a recommended expiration time (computed off
SplitByLoadMergeDelay)The only remaining code in SQL should be to generate new split points (based on the KV returned details) and issue the AdminSplit/Scatter commands.
I created #93549 and linked to it in a TODO.
knz
left a comment
There was a problem hiding this comment.
Reviewed 5 of 7 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:
|
bors r=knz |
|
Build succeeded: |
Fixes #69499
Fixes #82944
Existing split points are preserved after a TRUNCATE statement is executed by a secondary tenant.
Release note: None