Skip to content

Conversation

@mgartner
Copy link
Collaborator

@mgartner mgartner commented Dec 3, 2025

bench/rttananlysis: document directions for running tests

These tests have a non-conventional construction that make it hard to
determine how they can be run. Each subset of tests is not a typical Go
function like func TestAThing(t *testing.T) { ... } that can be easily
searched for. A few comments have been added to explain how the tests
can be run.

Release note: None

bench/rttanalysis: add test case with an after trigger

Release note: None

opt: route descriptor lookups for AFTER triggers through cache

This commit applies the fix in #144217 for BEFORE triggers to AFTER
triggers. See that PR for more details.

Fixes #144211

Release note (performance improvement): After triggers now perform the
descriptor lookup for TG_TABLE_SCHEMA against a cache. This can
significantly reduce trigger planning latency.

opt: warn about performance of (*optCatalog).FullyQualifiedName

Release note: None

@mgartner mgartner requested review from DrewKimball and fqazi December 3, 2025 21:45
@mgartner mgartner requested review from a team as code owners December 3, 2025 21:45
@mgartner mgartner requested a review from a team December 3, 2025 21:45
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner added backport-25.2.x Flags PRs that need to be backported to 25.2 backport-25.3.x Flags PRs that need to be backported to 25.3 backport-25.4.x Flags PRs that need to be backported to 25.4 labels Dec 3, 2025
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@fqazi reviewed 2 of 2 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks! It might also be worth adding a comment to FullyQualifiedName to warn people away from using it over methods that go through the cache.

@DrewKimball reviewed 2 of 2 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)

@mgartner
Copy link
Collaborator Author

mgartner commented Dec 4, 2025

It might also be worth adding a comment to FullyQualifiedName to warn people away from using it over methods that go through the cache.

The interface already mentions this:

// - this call may involve a database operation so it shouldn't be used in
// performance sensitive paths;

I copied these notes to the opt catalog implementation too.

These tests have a non-conventional construction that make it hard to
determine how they can be run. Each subset of tests is not a typical Go
function like `func TestAThing(t *testing.T) { ... }` that can be easily
searched for. A few comments have been added to explain how the tests
can be run.

Release note: None
This commit applies the fix in cockroachdb#144217 for `BEFORE` triggers to `AFTER`
triggers. See that PR for more details.

Fixes cockroachdb#144211

Release note (performance improvement): After triggers now perform the
descriptor lookup for `TG_TABLE_SCHEMA` against a cache. This can
significantly reduce trigger planning latency.
@mgartner
Copy link
Collaborator Author

mgartner commented Dec 5, 2025

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 5, 2025

@craig craig bot merged commit 7331b39 into cockroachdb:master Dec 5, 2025
24 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Dec 5, 2025

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


Issue #144211: branch-release-25.3, branch-release-25.4.


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

@blathers-crl
Copy link

blathers-crl bot commented Dec 5, 2025

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from 5f69f7b to blathers/backport-release-25.2-158708: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 25.2.x failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

error creating merge commit from 364a865 to blathers/backport-release-25.3-158708: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 25.3.x failed. See errors above.


💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-25.2.x Flags PRs that need to be backported to 25.2 backport-25.3.x Flags PRs that need to be backported to 25.3 backport-25.4.x Flags PRs that need to be backported to 25.4 backport-failed target-release-26.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql: triggers do expensive system table lookups

4 participants