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

catalog: only clone descriptors when necessary #121579

Closed
annrpom opened this issue Apr 2, 2024 · 1 comment · Fixed by #127028
Closed

catalog: only clone descriptors when necessary #121579

annrpom opened this issue Apr 2, 2024 · 1 comment · Fixed by #127028
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-2 Issues/test failures with a fix SLA of 3 months T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@annrpom
Copy link
Contributor

annrpom commented Apr 2, 2024

This came up in https://github.com/cockroachlabs/support/issues/2870: we do an unconditional clone of descriptor protos in RunPostDeserializationChanges - which is an unnecessary expense in cases where no changes are needed.

We should find a way to avoid cloning in those cases.

Jira issue: CRDB-37351

Epic CRDB-24134

@annrpom annrpom added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Apr 2, 2024
@annrpom
Copy link
Contributor Author

annrpom commented Apr 2, 2024

revival - #105382!

@annrpom annrpom self-assigned this Apr 8, 2024
@exalate-issue-sync exalate-issue-sync bot added the P-2 Issues/test failures with a fix SLA of 3 months label Apr 17, 2024
@exalate-issue-sync exalate-issue-sync bot assigned fqazi and unassigned annrpom May 29, 2024
fqazi added a commit to fqazi/cockroach that referenced this issue Jul 30, 2024
Previously, we would copy table descriptors even if no
PostDeserialization changes would apply eagerly. This could lead to
extra over head when accessing a large number of tables in the
pg_catalog tables. To address this, this patch will make a copy only if
we need to modify the original descriptor because of a post
deserialization change.

Fixes: cockroachdb#121579

Release note: None
fqazi added a commit to fqazi/cockroach that referenced this issue Jul 31, 2024
Previously, we would copy table descriptors even if no
PostDeserialization changes would apply eagerly. This could lead to
extra over head when accessing a large number of tables in the
pg_catalog tables. To address this, this patch will make a copy only if
we need to modify the original descriptor because of a post
deserialization change.

Fixes: cockroachdb#121579

Release note: None
fqazi added a commit to fqazi/cockroach that referenced this issue Aug 1, 2024
Previously, we would copy table descriptors even if no
PostDeserialization changes would apply eagerly. This could lead to
extra over head when accessing a large number of tables in the
pg_catalog tables. To address this, this patch will make a copy only if
we need to modify the original descriptor because of a post
deserialization change.

Fixes: cockroachdb#121579

Release note: None
craig bot pushed a commit that referenced this issue Aug 2, 2024
127028: sql/catalog: avoid making copies of table descriptors r=fqazi a=fqazi

Previously, we would copy table descriptors even if no PostDeserialization changes would apply eagerly. This could lead to extra over head when accessing a large number of tables in the pg_catalog tables. To address this, this patch will make a copy only if we need to modify the original descriptor because of a post deserialization change.

Fixes: #121579

Release note: None

128128: bazel: never strip output binaries r=rail a=rickystewart

To date, binaries are stripped by default when building with Bazel. We skip stripping for release binaries (via `-c opt`) and if you're building with the `dev` configuration (many developers use this configuration as `dev doctor` guides you to). This has led to some confusion as these mechanisms are non-obvious, leading some to wonder why their binaries are (or are not) stripped. We simplify this by *never* stripping. Since release binaries are not stripped, they are unaffected by this change. Only developers building without the `dev` config and CI jobs that don't set `-c opt` would be affected.

Epic: CRDB-17171

Release note: None

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in 9e71096 Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-2 Issues/test failures with a fix SLA of 3 months T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants