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.

Sign up for GitHub

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

Jump to bottom

distsql: transition distsql physical planner to use SQLInstanceIDs instead of NodeIDs #49596

Closed
asubiotto opened this issue May 27, 2020 · 1 comment · Fixed by #75259
Closed

distsql: transition distsql physical planner to use SQLInstanceIDs instead of NodeIDs #49596

asubiotto opened this issue May 27, 2020 · 1 comment · Fixed by #75259
Assignees
@rharding6373
Labels
A-multitenancy Related to multi-tenancy A-sql-execution Relating to SQL execution. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-sql-queries SQL Queries Team

Comments

@asubiotto
Copy link
Contributor

The distsql physical planner currently uses roachpb.NodeIDs to identify nodes. With multitenancy work, use of roachpb.NodeID is slowly being phased out in favor of a SQLInstanceID (#48009). The distsql physical planner should be changed to use SQLInstanceIDs.

This work is not urgent since for phase 2, tenants will only be executing single-node queries and SQLInstanceIDs can be cast to NodeIDs for the time being, but it is cleanup that should be done after that phase.
@asubiotto asubiotto added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-sql-execution Relating to SQL execution. A-multitenancy Related to multi-tenancy labels May 27, 2020
@tbg
Copy link
Member

tbg commented May 27, 2020

For the record, in #48795 I'm adding code that makes sure that we never try to SetupFlow. I added that because in the same PR, one of my changes let to a flow being scheduled (since we used -1 for the NodeID in the NodeDescriptor, which I've now rectified to use the same fakeNodeID as the rest of the tenant).

@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
@RaduBerinde RaduBerinde self-assigned this Nov 11, 2021
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Jan 21, 2022
@rharding6373
distsql: transition distsql physical planner to use SQLInstanceIDs in…
17cd6ea 
…stead of NodeIDs

Refactors DistSQL and some related functions to use `SQLInstanceID`
instead of `NodeID` for multi-tenant support.

Fixes: cockroachdb#49596

Release note: None
@rharding6373 rharding6373 mentioned this issue Jan 21, 2022
Merged
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Jan 21, 2022
@rharding6373
distsql: transition distsql physical planner to use SQLInstanceIDs in…
62863f7 
…stead of NodeIDs

Refactors DistSQL and some related functions to use `SQLInstanceID`
instead of `NodeID` for multi-tenant support.

Fixes: cockroachdb#49596

Release note: None
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Jan 26, 2022
@rharding6373
distsql: transition distsql physical planner to use SQLInstanceIDs in…
2df3234 
…stead of NodeIDs

Refactors DistSQL and some related functions to use `SQLInstanceID`
instead of `NodeID` for multi-tenant support.

Fixes: cockroachdb#49596

Release note: None
craig bot pushed a commit that referenced this issue Jan 27, 2022
@yuzefovich @rharding6373 @jbowens
Merge #74866 #75223 #75259 #75557
9f76a47 
74866: colexechash: improve comments on the hash table r=yuzefovich a=yuzefovich

**colexechash: improve comments on the hash table**

Some comments around the hash table code have rotted, so this commit
refreshes them. It adds an extensive comment with an example of how the
hash table is used by the unordered distinct. Only a minor mechanical
change is done to the actual code in order to increase its clarity.

Release note: None

**colexechash: rename GroupID to ToCheckID**

This makes the purpose of this field more clear.

Release note: None

75223: changefeedccl: show topic name(s) for a created changefeed r=sherman-grewal a=sherman-grewal

changefeedccl: show topic name(s) for a created changefeed

Resolves #74468

Release note (enterprise change): Changefeeds will now
output the topic name(s) created by the Kafka sink.
Furthermore, these topic name(s) will be displayed in
the SHOW CHANGEFEED JOBS query.

75259: distsql: transition distsql physical planner to use SQLInstanceIDs instead of NodeIDs r=rharding6373 a=rharding6373

Refactors DistSQL and some related functions to use `SQLInstanceID`
instead of `NodeID` for multi-tenant support.

Fixes: #49596

Release note: None

75557: vendor: bump Pebble to a5c1766b568a r=nicktrav a=jbowens

```
a5c1766b compaction: use Equal in compaction iterator
060753c8 internal/rangekey: add merging, fragmenting iterator
ca9b4522 Revert "Revert "sstable: reduce allocations during index flushing""
66beca7b sstable: move checksum logic onto checksummer
176f9fbf sstable: add compressed/uncompressed fields to the dataBlockBuf
a241910a internal/manifest: fix Overlaps L0 expansion with exclusive-end
a4a6fe83 compaction: remove redundant TruncateAndFlushTo call
8540f24b sstable: re-work `TableFormat` enum values
b510eb6b db: refactor experimental writer API, expose on DB
```

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Sherman Grewal <[email protected]>
Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
@craig craig bot closed this as completed in 711e228 Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rharding6373 rharding6373

Labels
A-multitenancy Related to multi-tenancy A-sql-execution Relating to SQL execution. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-sql-queries SQL Queries Team
Projects
None yet
Milestone
No milestone
5 participants
@jlinder @tbg @rharding6373 @asubiotto @RaduBerinde