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: set planner database scope of SchemaChanger to descriptor db #54410

Closed
wants to merge 1 commit into from

Conversation

otan
Copy link
Contributor

@otan otan commented Sep 15, 2020

Resolves #53819. Unsure about whether we should backport this.

Release note (bug fix): Previously, using builtins which referenced
other tables (e.g. nextval) in a CREATE TABLE AS or CREATE MATERIALIZED
VIEW would error. This has been resolved.

@otan otan requested review from thoszhang and a team September 15, 2020 17:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@otan
Copy link
Contributor Author

otan commented Sep 15, 2020

hmm, this fails some tenant tests, but unsure why / how this happens

@ajwerner
Copy link
Contributor

This feels closely related to #51090

@ajwerner
Copy link
Contributor

which we did not successfully address in this 20.2 :(

Copy link
Contributor

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Andrew said what I was going to say. What are the semantics here if the current database, the parent database of the table, and the parent database of the sequence aren't the same?

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan)


pkg/sql/schema_changer.go, line 256 at r1 (raw file):

		// which relies on the planner scope being the same as the scope that ran the initial schema
		// query.
		db, err := catalogkv.GetDatabaseDescByID(ctx, txn, keys.SystemSQLCodec, table.GetParentID())

I think you need the codec on sc.execCfg. Also, you want to use catalogkv.MustGetDatabaseDescByID() here. GetDatabaseDescByID is sort of deprecated.

@ajwerner
Copy link
Contributor

I think the right thing to do is what’s proposed in the linked issue. We’ve done the hard part for 20.2 (make table ids stable). Now we can fix this, at least moving forward. It’s a little bit less clear what to do with existing sequences.

Release note (bug fix): Previously, using builtins which referenced
other tables (e.g. nextval) in a CREATE TABLE AS or CREATE MATERIALIZED
VIEW would error. This has been resolved.
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.

sql: CREATE MATERIALIZED VIEW does not use the right name resolution scope for its scalar expressions
4 participants