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 UDFs to statement bundle #104976

Closed
mgartner opened this issue Jun 15, 2023 · 2 comments · Fixed by #132147
Closed

sql: use memo metadata to add UDFs to statement bundle #104976

mgartner opened this issue Jun 15, 2023 · 2 comments · Fixed by #132147
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team

Comments

@mgartner
Copy link
Collaborator

mgartner commented Jun 15, 2023

We currently add UDFs to a statement bundle's schema.sql file with string matching:

// TODO: consider getting the udf sql body statements from the memo metadata.

Instead, we should use the metadata in the memo to determine which UDFs are relevant to the query.

As an added bonus, it would be great to ensure that UDFs referenced in table expressions are included in statement bundles. For example:

CREATE FUNCTION add_one(x INT) RETURNS INT IMMUTABLE LANGUAGE SQL AS 'SELECT x + 1';

CREATE TABLE t (
  a INT,
  b INT AS (add_one(a)) STORED
);

EXPLAIN ANALYZE (DEBUG) SELECT * FROM t;

The add_one UDF should be included in the statement bundle.

Jira issue: CRDB-28808

@mgartner mgartner added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jun 15, 2023
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Jun 15, 2023
@mgartner mgartner moved this to 23.2 Release in SQL Queries Jul 24, 2023
@michae2
Copy link
Collaborator

michae2 commented Sep 12, 2023

Might be done?

@msirek msirek moved this from 23.2 Release to 24.1 Release in SQL Queries Oct 10, 2023
@mgartner mgartner moved this from 24.1 Release to New Backlog in SQL Queries Nov 22, 2023
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 8, 2024
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.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 8, 2024
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
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Oct 29, 2024
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
craig bot pushed a commit that referenced this issue 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 <drewk@cockroachlabs.com>
Co-authored-by: Andrew Baptist <baptist@cockroachlabs.com>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
craig bot pushed a commit that referenced this issue 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.

133573: kv: redirect requests to raft leader who holds leader lease r=nvanbenschoten a=nvanbenschoten

Informs #132762.

This commit updates the lease status logic to redirect requests to the raft leader who holds a leader lease when evaluating a request on a follower. Previously, the follower would return a NotLeaseHolderError, but it would not include the lease in the response, so the client would not be immediately redirected to the leader. Instead, it would continue trying each replica in order, eventually throwing a `sending to all replicas failed` error.

Furthermore, since no lease was included, request proxying would never be used. This explains why the `failover/partial/lease-gateway` test was failing to recover. With this change, the test now observes recovery in in 7.650s.

| test                                         | lease=epoch (ms) | lease=expiration (ms) | lease=leader (ms) | parity with expiration |
|:---------------------------------------------|-----------------:|----------------------:|------------------:|:----------------------:|
| failover/partial/lease-gateway               | 8,589            | 19,327           | 7,650            | ✔                      |

**Key _(comparing leader vs. expiration)_**:
✔ = parity
❌ = minor regression
❌❌ = major regression
❌❌❌ = unavailability

To further demonstrate that this change works as expected, the commit also ports over two unit test that exercise request proxying to run with leader leases. Without this change, they fail. With it, they pass.

----

Release note: None

133691: sql/schemachanger: Support enum types for DSC impl of ALTER COLUMN … ALTER TYPE r=spilchen a=spilchen

Previously, attempts to alter a column's type to an enum were blocked due to failures during the backfill process, resulting in the error: "cannot resolve types in DistSQL by name." This change addresses that issue by ensuring all type names are fully resolved when populating the USING expression for the alteration.

Additionally, I've corrected an error message for column type conversions involving an enum type without a USING clause. The previous message included a hint with an unhydrated type name, which was confusing for users. The hint used to look like this:
```
HINT: You might need to specify "USING: x::`@100117".`
```

This fix now provides a more meaningful hint by displaying the correct type name.

Epic: CRDB-25314
Closes #132936
Release note: none

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Andrew Baptist <baptist@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
@craig craig bot closed this as completed in 3fb9f91 Oct 29, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in SQL Queries Oct 29, 2024
Copy link

blathers-crl bot commented Oct 29, 2024

Based on the specified backports for linked PR #132147, I applied the following new label(s) to this issue: branch-release-23.2, branch-release-24.1, branch-release-24.2. 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.

@blathers-crl blathers-crl bot added branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 labels Oct 29, 2024
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Nov 25, 2024
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.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Nov 25, 2024
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
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Nov 25, 2024
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.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Nov 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants