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

backfill: avoid redundant calls to MakeIndexKeyPrefix #137798

Closed
rafiss opened this issue Dec 19, 2024 · 1 comment · Fixed by #139256
Closed

backfill: avoid redundant calls to MakeIndexKeyPrefix #137798

rafiss opened this issue Dec 19, 2024 · 1 comment · Fixed by #139256
Assignees
@annrpom
Labels
branch-release-25.1 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@rafiss
Copy link
Collaborator

rafiss commented Dec 19, 2024 •
edited by cockroach-jira-scripts
Loading

The ingestIndexEntriesChunk function encodes the row for the new secondary index. It calls into EncodeSecondaryIndexes here:

cockroach/pkg/sql/backfill/backfill.go

Lines 980 to 990 in 2bf463b

return rowenc.EncodeSecondaryIndexes(
ctx,
ib.evalCtx.Codec,
tableDesc,
ib.indexesToEncode,
ib.colIdxMap,
ib.rowVals,
buffer,
false, /* includeEmpty */
&ib.muBoundAccount.boundAccount,
)

Each key of the secondary index then calls MakeIndexKeyPrefix, which will be identical for every row. We should be able to avoid this redundant encoding work by computing this ahead of time and passing it in as a parameter.

cockroach/pkg/sql/rowenc/index_encoding.go

Line 1297 in 354444d

secondaryIndexKeyPrefix := MakeIndexKeyPrefix(codec, tableDesc.GetID(), secondaryIndex.GetID())

This profile shows that the overhead of this is not huge, but also not negligible:
profile.pb.gz

Jira issue: CRDB-45747
@rafiss rafiss added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Dec 19, 2024
@annrpom annrpom mentioned this issue Jan 21, 2025
Merged
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 22, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
6537f0c 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 24, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
57f62d4 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 27, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
774bc22 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 29, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
2dec4f9 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 29, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
c3d5a9b 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 30, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
b3803f6 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 30, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
9152c31 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 31, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
895ee09 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
annrpom added a commit to annrpom/cockroach that referenced this issue Jan 31, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
906d5b9 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
craig bot pushed a commit that referenced this issue Jan 31, 2025
@annrpom @taroface @andrewbaptist
Merge #139256 #139365 #140071
4bbafad 
139256: sql/rowenc: reduce index key prefix calls r=annrpom a=annrpom

### sql/rowenc: reduce index key prefix calls
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: [CRDB-42901](https://cockroachlabs.atlassian.net/browse/CRDB-42901)
Fixes: #137798

Release note: None

---

### sql/rowexec: run BenchmarkIndexBackfill on-disk
Epic: none
Release note: None


139365: docgen: update COPY TO STDOUT portions of SQL diagram r=mgartner a=taroface

Updated `COPY` diagram so that it better reflects the `COPY TO` syntax that was added in 23.1 (this PR will be backported to every version up until 23.1). These changes accompany the docs updates in cockroachdb/docs#19310
 
Current diagram:

<img width="647" alt="image" src="https://github.com/user-attachments/assets/eb70b831-3473-40aa-823f-5432a8cf2698" />

New diagram:

<img width="829" alt="image" src="https://github.com/user-attachments/assets/314e1345-cec8-4a72-9be2-fb4b2f2885ee" />

Epic: none
Release note: none
Release justification: non-production code change

140071: storage: covert SizeSpec to protobuf r=RaduBerinde a=andrewbaptist

This commit moves SizeSpec to the storagepb directory and converts it toa protobuf. Additionally it pulls shared storage into the global StorageConfig file.

Epic: none

Release note: None

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Ryan Kuo <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
craig bot pushed a commit that referenced this issue Jan 31, 2025
@annrpom
Merge #139256 #140167
311ed57 
139256: sql/rowenc: reduce index key prefix calls r=annrpom a=annrpom

### sql/rowenc: reduce index key prefix calls
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: [CRDB-42901](https://cockroachlabs.atlassian.net/browse/CRDB-42901)
Fixes: #137798

Release note: None

---

### sql/rowexec: run BenchmarkIndexBackfill on-disk
Epic: none
Release note: None


140167: sql: account for duplicate partition names in subzone span gen r=annrpom a=annrpom

This patch fixes a bug during GenerateSubzoneSpans that does not
correctly distinguish between partitions of the same name across
indexes. The issue was introduced when we relaxed restrictions on
partition name reuse across indexes of a table (#39332).

Epic: [CRDB-43310](https://cockroachlabs.atlassian.net/browse/CRDB-43310)
Fixes: #128692

Release note (bug fix): Configuring replication controls on a
partition name of an index that is not unique across all indexes
will correctly impact only that partition.

Co-authored-by: Annie Pompa <[email protected]>
craig bot pushed a commit that referenced this issue Jan 31, 2025
@annrpom @msbutler
@rytaft @mgartner @andrewbaptist
Merge #139256 #139955 #140057 #140065 #140203
281a25b 
139256: sql/rowenc: reduce index key prefix calls r=annrpom a=annrpom

### sql/rowenc: reduce index key prefix calls
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: [CRDB-42901](https://cockroachlabs.atlassian.net/browse/CRDB-42901)
Fixes: #137798

Release note: None

---

### sql/rowexec: run BenchmarkIndexBackfill on-disk
Epic: none
Release note: None


139955: crosscluster/logical: use large test pool r=rickystewart a=msbutler

This package spins up several TestServers. We've also seen many tests timeout, likely due to resource exhaustion.

Informs #138277
Informs #139673

Release note: none

140057: sqlsmith: randomly generate auto partial stats in sqlsmith tests r=rytaft a=rytaft

Epic: None

Release note: None

140065: opt: add `optimizer_min_row_count` session setting r=mgartner a=mgartner

Informs #64570
Informs #130201

Release note (sql change): The `optimizer_min_row_count` session setting
has been added which sets a lower bound on row count estimates for
relational expressions during query planning. A value of zero, which is
the default, indicates no lower bound. Note that if this is set to a
value greater than zero, a row count of zero can still be estimated for
expressions with a cardinality of zero, e.g., for a contradictory
filter. Setting this to a value higher than 0, such as 1, may yield
better query plans in some cases, such as when statistics are frequently
stale and inaccurate.


140203: server: split join_list to its own file r=RaduBerinde a=andrewbaptist

Previously StoreSpec and JoinListType were in the same file. This commit moves JoinListType and its test to a separate file.

Epic: CRDB-41111

Release note: None

Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
@craig craig bot closed this as completed in e29b21b Jan 31, 2025
@blathers-crl blathers[crl]
Copy link

blathers-crl bot commented Jan 31, 2025

Based on the specified backports for linked PR #139256, I applied the following new label(s) to this issue: branch-release-25.1. Please adjust the labels as needed to match the branches actually affected by this issue, including adding any known older branches.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

annrpom added a commit to annrpom/cockroach that referenced this issue Jan 31, 2025
@annrpom
sql/rowenc: reduce index key prefix calls
896340b 
This patch removes redundant calls to `MakeIndexKeyPrefix` during
the construction of `IndexEntry`s by saving each first-time call in a
map that we can later lookup. Previously, we would make this call
for each row; however, as the prefix (table id + index id) for a
particular index remains the same, we do not need to do any
recomputation.

Epic: CRDB-42901
Fixes: cockroachdb#137798

Release note: None
@annrpom annrpom mentioned this issue Jan 31, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@annrpom annrpom

Labels
branch-release-25.1 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql/rowenc: reduce index key prefix calls annrpom/cockroach
2 participants
@rafiss @annrpom