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

sql: use memo metadata to add routines and UDTs to statement bundles #132147

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

DrewKimball
Copy link
Collaborator

opt: replace "udf" with "routine" in the metadata

This commit changes the naming of some methods and data structures used
in query plan metadata to refer to "routines" instead of "udfs". This
clarifies that stored procedures are also valid targets for metadata
tracking.

Epic: None

Release note: None

sql: use memo metadata to add routines to statement bundles

This commit updates the statement bundle logic to take advantage of the
information stored in the query plan metadata so that only relevant routines
are shown in schema.sql for a statement bundle. In addition, stored
procedures are now tracked in the metadata in addition to UDFs. This has
no impact on query-plan caching, since we currently do not cache plans
that invoke stored procedures.

Fixes #132142
Fixes #104976

Release note (bug fix): Fixed a bug that prevented the create statement
for a routine from being shown in a statement bundle. This happened when
the routine was created on a schema other than public. The bug has
existed since v23.1.

sql: use memo metadata to add UDTs to statement bundle

This commit modifies the statement bundle collection logic to use the
query plan metadata to determine which types to display in schema.sql.
This ensures that only types which are referenced by the query are shown.

Informs #104976

Release note: None

@DrewKimball DrewKimball added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Oct 8, 2024
@DrewKimball DrewKimball requested a review from a team October 8, 2024 02:37
@DrewKimball DrewKimball requested a review from a team as a code owner October 8, 2024 02:37
@DrewKimball DrewKimball requested review from mw5h and removed request for a team October 8, 2024 02:37
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Nice improvement! :lgtm:

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


pkg/sql/explain_bundle.go line 629 at r2 (raw file):

	}
	if mem.Metadata().HasUserDefinedRoutines() {
		// Get all relevant user-defined functions.

nit: s/functions/routines/.


pkg/sql/explain_bundle.go line 623 at r3 (raw file):

		}
	}
	// Get all user-defined types.

nit: maybe add "relevant" into the comment?


pkg/sql/opt/metadata.go line 1180 at r1 (raw file):

}

// TestingRoutineDepsEqual returns whether the UDF deps of the other Metadata are

nit: s/UDF deps/routine deps/.


pkg/sql/explain_bundle_test.go line 436 at r2 (raw file):

					reg = regexp.MustCompile("^foo")
					if reg.FindString(contents) != "" {
						return errors.Errorf("Found irrelevant function 'foo' in schema.sql")

nit: s/Found/found/.

This commit changes the naming of some methods and data structures used
in query plan metadata to refer to "routines" instead of "udfs". This
clarifies that stored procedures are also valid targets for metadata
tracking.

Epic: None

Release note: None
This commit updates the statement bundle logic to take advantage of the
information stored in the query plan metadata so that only relevant routines
are shown in `schema.sql` for a statement bundle. In addition, stored
procedures are now tracked in the metadata in addition to UDFs. This has
no impact on query-plan caching, since we currently do not cache plans
that invoke stored procedures.

Fixes cockroachdb#132142
Fixes cockroachdb#104976

Release note (bug fix): Fixed a bug that prevented the create statement
for a routine from being shown in a statement bundle. This happened when
the routine was created on a schema other than `public`. The bug has
existed since v23.1.
This commit modifies the statement bundle collection logic to use the
query plan metadata to determine which types to display in `schema.sql`.
This ensures that only types which are referenced by the query are shown.

Informs cockroachdb#104976

Release note: None
Copy link
Collaborator Author

@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.

TFTR!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mw5h)


pkg/sql/explain_bundle.go line 629 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/functions/routines/.

Done.


pkg/sql/explain_bundle.go line 623 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: maybe add "relevant" into the comment?

Done.


pkg/sql/opt/metadata.go line 1180 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/UDF deps/routine deps/.

Done.


pkg/sql/explain_bundle_test.go line 436 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/Found/found/.

Done.

@DrewKimball
Copy link
Collaborator Author

bors r+

craig bot pushed a commit that referenced this pull request Oct 29, 2024
132147: sql: use memo metadata to add routines and UDTs to statement bundles r=DrewKimball a=DrewKimball

#### opt: replace "udf" with "routine" in the metadata

This commit changes the naming of some methods and data structures used
in query plan metadata to refer to "routines" instead of "udfs". This
clarifies that stored procedures are also valid targets for metadata
tracking.

Epic: None

Release note: None

#### sql: use memo metadata to add routines to statement bundles

This commit updates the statement bundle logic to take advantage of the
information stored in the query plan metadata so that only relevant routines
are shown in `schema.sql` for a statement bundle. In addition, stored
procedures are now tracked in the metadata in addition to UDFs. This has
no impact on query-plan caching, since we currently do not cache plans
that invoke stored procedures.

Fixes #132142
Fixes #104976

Release note (bug fix): Fixed a bug that prevented the create statement
for a routine from being shown in a statement bundle. This happened when
the routine was created on a schema other than `public`. The bug has
existed since v23.1.

#### sql: use memo metadata to add UDTs to statement bundle

This commit modifies the statement bundle collection logic to use the
query plan metadata to determine which types to display in `schema.sql`.
This ensures that only types which are referenced by the query are shown.

Informs #104976

Release note: None

133365: roachtest: various minor perturbation cleanups r=kvoli a=andrewbaptist

Various minor cleanups to the perturbation tests. These changes don't impact anything functional but make it slightly easier to write new perturbations.

133699: build: delete `test_nogo_configured` r=rail a=rickystewart

This is unused.

Also correct a bad error message in `dev`.

Epic: none
Release note: None

Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Andrew Baptist <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@craig
Copy link
Contributor

craig bot commented Oct 29, 2024

Build failed (retrying...):

@craig craig bot merged commit ba71133 into cockroachdb:master Oct 29, 2024
22 of 23 checks passed
Copy link

blathers-crl bot commented Oct 29, 2024

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 #104976: branch-release-23.2, branch-release-24.1, branch-release-24.2.


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

Copy link

blathers-crl bot commented Oct 29, 2024

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.


error creating merge commit from 09c2e3d to blathers/backport-release-23.2-132147: 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 23.2.x failed. See errors above.


error creating merge commit from 09c2e3d to blathers/backport-release-24.1-132147: 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 24.1.x failed. See errors above.


error creating merge commit from 09c2e3d to blathers/backport-release-24.2-132147: 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 24.2.x failed. See errors above.


🦉 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-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2
Projects
None yet
3 participants