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

workload/schemachange: add support for CREATE FUNCTION #115357

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

chrisseto
Copy link
Contributor

@chrisseto chrisseto commented Nov 30, 2023

6e1ea5d workload/schemachange: add support for CREATE FUNCTION

This commit adds support for the CREATE FUNCTION DDL to the RSW.

While the space for possible UDF incantations is quite large, this
commit focused on permutations that are of interest to schema changes.
That is to say, the body of the UDF does not have much, if any,
variation but will be comprised of references to tables and UDTs.

Unfortunately, the introduction of UDFs in the descriptor graph has
aggravated an issue within DROP SCHEMA CASCADE.

This commit contains attempts to correct said issues to no avail. It's
possible that either the DSC or LSC are missing some logic for handling
UDFs that contain references to UDTs. Prior to this commit, errors
preventing the schema from being dropped would be thrown. After it,
expected errors are not thrown appropriately despite the checks being
hand verified for correctness.

Epic: CRDB-19168
Informs: CRDB-3265
Release note: None

@chrisseto chrisseto requested review from a team as code owners November 30, 2023 20:11
@chrisseto chrisseto requested review from herkolategan and srosenberg and removed request for a team November 30, 2023 20:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@chrisseto chrisseto changed the title workload/schemachange: add support for CREATE FUNCTION workload/schemachange: add support for CREATE FUNCTION Nov 30, 2023
@rafiss rafiss requested a review from fqazi December 4, 2023 15:11
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.

This is really nice and readable. Great work @chrisseto!

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @herkolategan, and @srosenberg)


pkg/workload/schemachange/operation_generator.go line 3771 at r4 (raw file):

func (og *operationGenerator) createFunction(ctx context.Context, tx pgx.Tx) (*opStmt, error) {
	tables, err := Collect(ctx, og, tx, pgx.RowTo[string], `SELECT quote_ident(schema_name) || '.' || quote_ident(table_name) FROM [SHOW TABLES] WHERE type = 'table'`)

Do we need to limit it to only tables? You could select from views / sequences too.


pkg/workload/schemachange/operation_generator.go line 3818 at r4 (raw file):

	}

	// TODO(chrisseto): There's no randomization across STRICT, VOLATILE,

Maybe as a TODO we could do sequence references too..like nextval(..)

Copy link
Contributor Author

@chrisseto chrisseto 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! 0 of 0 LGTMs obtained (waiting on @fqazi, @herkolategan, and @srosenberg)


pkg/workload/schemachange/operation_generator.go line 3771 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Do we need to limit it to only tables? You could select from views / sequences too.

Didn't even think about that! I've updated it to include views. sequences have been todo'd because they break DROP SEQUENCE.


pkg/workload/schemachange/operation_generator.go line 3818 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Maybe as a TODO we could do sequence references too..like nextval(..)

Done.

This commit adds support for the `CREATE FUNCTION` DDL to the RSW.

While the space for possible UDF incantations is quite large, this
commit focused on permutations that are of interest to schema changes.
That is to say, the body of the UDF does not have much, if any,
variation but will be comprised of references to tables and UDTs.

Unfortunately, the introduction of UDFs in the descriptor graph has
aggravated an issue within `DROP SCHEMA CASCADE`.

This commit contains attempts to correct said issues to no avail. It's
possible that either the DSC or LSC are missing some logic for handling
UDFs that contain references to UDTs. Prior to this commit, errors
preventing the schema from being dropped would be thrown. After it,
expected errors are not thrown appropriately despite the checks being
hand verified for correctness.

Epic: CRDB-19168
Informs: CRDB-3265
Release note: None
@chrisseto
Copy link
Contributor Author

bors r=fqazi

@craig
Copy link
Contributor

craig bot commented Dec 20, 2023

Build succeeded:

@craig craig bot merged commit 37c3279 into cockroachdb:master Dec 20, 2023
9 checks passed
@chrisseto chrisseto deleted the rsw-create-function branch December 20, 2023 15:52
craig bot pushed a commit that referenced this pull request Dec 26, 2023
116790: workload/schemachange: fix `CREATE SCHEMA` for the DSC r=rafiss a=chrisseto

### This PR is stacked on top of others!

Please consider reviewing those PRs first. This message will be removed once prerequisite PRs are merged and this one is rebased.

* 6e1ea5d is from #115357
* 230b588 is from #115358
* 4cc9b45 is from #115359
* be063ef is from #115360
* 4e89e34 is from #116788
* 95ffcf4 is from #116789

---

#### 89e761126789d2828e141339c34e739203da2310 workload/schemachange: fix `CREATE SCHEMA` for the DSC

Previously, `CREATE SCHEMA` would break if executed with the declarative
schema changer due to the inclusion of `AUTHORIZATION`.

This commit removes the `AUTHORIZATION` clause from `CREATE SCHEMA`
operations until it's support within the DSC.

Epic: none
Release note: None

Co-authored-by: Chris Seto <chriskseto@gmail.com>
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.

None yet

3 participants