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

release-23.1: sql: fix CREATE AS [SHOW CREATE FUNCTION] job failure #108161

Closed
wants to merge 10 commits into from
Closed

release-23.1: sql: fix CREATE AS [SHOW CREATE FUNCTION] job failure #108161

wants to merge 10 commits into from

Conversation

ecwall
Copy link
Contributor

@ecwall ecwall commented Aug 4, 2023

Backport:

Please see individual PRs for details.

/cc @cockroachdb/release

Release justification: CREATE AS bug fix.

rafiss and others added 9 commits August 4, 2023 06:51
After changing the sessiondata to stop using zero-value defaults, a test
started failing. It works when using a zero sessiondata, which deserves
more investigation.

Release note: None
Prevent zero-values from being used for session vars when creating an InternalDB.

Release note: None
Now that the internal executor is using a non-zero session data, that
means it uses the declarative schema changer by default. The upgrade
manager had a few assumptions in it that were specific to the old schema
changer:

- The PGAttributeNum field should only be set if it differs from the
  column descriptor ID.
- Check the DeclarativeSchemaChangerState when waiting for migrations to
  finish.

Release note: None
This change makes debugging easier by including the invalid JSON
in that caused JSONBMarshalToMessage to fail.

Release note: None
…ssageToJSON

Fixes #107823

This PR makes `VectorizeExecMode`'s protobuf and JSON string representations
the same to prevent errors like this when `SessionData.VectorizeMode` has a
non-zero value:
```
unknown value "on" for enum cockroach.sql.sessiondatapb.VectorizeExecMode
```

Release note: None
@ecwall ecwall requested review from a team as code owners August 4, 2023 11:28
@ecwall ecwall requested a review from a team August 4, 2023 11:28
@ecwall ecwall requested review from a team as code owners August 4, 2023 11:28
@blathers-crl
Copy link

blathers-crl bot commented Aug 4, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@ecwall ecwall requested review from rhu713, jayshrivastava, nkodali and mgartner and removed request for a team August 4, 2023 11:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ecwall ecwall removed request for a team, jayshrivastava and nkodali August 4, 2023 11:31
@ecwall ecwall requested review from rafiss and removed request for mgartner, a team and rhu713 August 4, 2023 11:31
Fixes #106268

Previously `CREATE TABLE AS`/`CREATE MATERIALIZED VIEW AS` sourcing from
`SHOW CREATE FUNCTION <function>` generated a failing schema change job with a
`unknown function: f(): function undefined` error because the job runs in the
system database and functions cannot be referenced cross database.

This PR fixes this by using the original user session (which includes the
database) in the schema change job.

Release note (bug fix): Fixes the schema changer job when CREATE AS sources
from SHOW CREATE FUNCTION:
CREATE TABLE t AS SELECT * FROM [SHOW CREATE FUNCTION f];
@yuzefovich
Copy link
Member

Backporting #101486 seems scary, and to me the bug with CREATE AS [SHOW CREATE FUNCTION] doesn't seem worth the risk, so I'd vote to not merge this.

@ecwall
Copy link
Contributor Author

ecwall commented Aug 4, 2023

Backporting #101486 seems scary, and to me the bug with CREATE AS [SHOW CREATE FUNCTION] doesn't seem worth the risk, so I'd vote to not merge this.

Closing this PR in favor of #108195

@ecwall ecwall closed this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants