sql: add support for CREATE OR REPLACE VIEW #47051

rohany · 2020-04-05T03:32:39Z

This PR adds support for the CREATE OR REPLACE VIEW
command by allowing the create view statement to optionally
overwrite an existing descriptor, rather than always write
a new descriptor k/v entry.

Release note (sql change): This PR adds support for the
CREATE OR REPLACE VIEW command.

cockroach-teamcity · 2020-04-05T03:32:46Z

This change is

RaduBerinde

Cool!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @rohany)

pkg/sql/create_view.go, line 79 at r1 (raw file):

	tKey, schemaID, err := getTableCreateParams(params, n.dbDesc.ID, isTemporary, viewName)
	if err != nil {
		if sqlbase.IsRelationAlreadyExistsError(err) {

[nit] we can invert this check and return err so the other logic isn't so nested

pkg/sql/create_view.go, line 82 at r1 (raw file):

			if n.ifNotExists {
				return nil
			} else if n.replace {

[nit] no need for else after return. Actually this could all be a big switch with four branches, all of which end in return

pkg/sql/create_view.go, line 112 at r1 (raw file):

	var id sqlbase.ID
	if replacingDesc != nil {

[nit] this could be coalesced with the if below, it has the same branches

pkg/sql/create_view.go, line 139 at r1 (raw file):

		oldDesc := &replacingDesc.ClusterVersion

		// Compare replacingDesc against its ClusterVersion to verify if

[nit] This block is a good candidate for moving into a helper function to make this function easier to read through

pkg/sql/create_view.go, line 169 at r1 (raw file):

		}

		// Remove this view from all tables that depend on it.

this comment seems wrong, this is about tables that the existing view depends on no?

pkg/sql/create_view.go, line 181 at r1 (raw file):

			}

			// Remove the back reference.

Is some of this code not the same with drop view code? (wondering if we can share anything)

blathers-crl · 2020-04-05T16:47:00Z

The "GitHub CI (Cockroach)" build has failed on 02c77fd47175da3fc1c1435986c647b1e5bafb52.

_{Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @RaduBerinde)

pkg/sql/create_view.go, line 82 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] no need for else after return. Actually this could all be a big switch with four branches, all of which end in return

oh thats clean -- i didn't realize you could switch on nothing.

pkg/sql/create_view.go, line 112 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] this could be coalesced with the if below, it has the same branches

it was actually unneeded.

pkg/sql/create_view.go, line 169 at r1 (raw file):

Previously, RaduBerinde wrote…

this comment seems wrong, this is about tables that the existing view depends on no?

Yup, this was backwards.

pkg/sql/create_view.go, line 181 at r1 (raw file):

Previously, RaduBerinde wrote…

Is some of this code not the same with drop view code? (wondering if we can share anything)

good call, was able to remove some duplication.

blathers-crl · 2020-04-05T18:58:38Z

❌ The GitHub CI (Cockroach) build has failed on abe167db.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

blathers-crl · 2020-04-05T19:24:21Z

❌ The GitHub CI (Cockroach) build has failed on abe167db.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

blathers-crl · 2020-04-05T21:42:26Z

❌ The GitHub CI (Cockroach) build has failed on 574437d7.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

knz

Reviewed 7 of 15 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, @RaduBerinde, and @rohany)

pkg/sql/create_view.go, line 322 at r2 (raw file):

	for i := range oldColumns {
		oldCol, newCol := &oldColumns[i], &newColumns[i]
		if oldCol.Name != newCol.Name {

I think you need to compare the visibility as well (hidden vs non-hidden). Otherwise the result of *-expansion can change.

rohany · 2020-04-06T14:23:18Z

Is it possible for views to have hidden columns? It doesn't seem like it.

knz · 2020-04-06T17:02:26Z

Is it possible for views to have hidden columns? It doesn't seem like it.

If it becomes possible tomorrow, you don't want this code to start failing silently.

rohany · 2020-04-06T17:11:53Z

valid point, updated.

RaduBerinde · 2020-04-06T18:43:33Z

Since we are reusing the descriptor ID, is it guaranteed that the Version in the descriptor will change? That is necessary for plan caching to handle the change correctly (via this code).

In any case, please add a test (in logic_test/prepare) where we have a view, we prepare a query using that view, we update the view, then execute the prepared query and verify that the results are consistent with the new view definition.

rohany · 2020-04-06T19:18:42Z

Since we are reusing the descriptor ID, is it guaranteed that the Version in the descriptor will change? That is necessary for plan caching to handle the change correctly (via this code).

Yeah, the version will be bumped. Added a test to verify this!

RaduBerinde · 2020-04-06T19:48:48Z

Test looks great, thanks!

rohany · 2020-04-10T01:40:21Z

Pinging for an "official" review action please!

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, @RaduBerinde, and @rohany)

pkg/sql/create_view.go, line 91 at r3 (raw file):

				return err
			}
			desc, err := params.p.Tables().getMutableTableVersionByID(params.ctx, id, params.p.txn)

Are there permissions we need to consider here? You shouldn't be allowed to edit a view you don't have write on, or something like that.

pkg/sql/create_view.go, line 96 at r3 (raw file):

			}
			if !desc.IsView() {
				return pgerror.Newf(pgcode.WrongObjectType, `"%s" is not a view`, viewName)

nit: You can use %q instead of "%s".

pkg/sql/create_view.go, line 115 at r3 (raw file):

	var newDesc *sqlbase.MutableTableDescriptor

	if replacingDesc != nil {

I would make this big block a function to make things a little cleaner, and add a comment. "replacingDesc != nil" doesn't really explain what's going on.

pkg/sql/create_view.go, line 147 at r3 (raw file):

			// Remove the back reference.
			desc.DependedOnBy = removeMatchingReferences(desc.DependedOnBy, replacingDesc.ID)

Doesn't this pointlessly drop and re-add the depend relationships if the view is edited but its dependencies not changed? This is the sort of thing that we're trying to cut down on with DDL performance improvements. They add up.

pkg/sql/create_view.go, line 149 at r3 (raw file):

			desc.DependedOnBy = removeMatchingReferences(desc.DependedOnBy, replacingDesc.ID)
			if err := params.p.writeSchemaChange(
				params.ctx, desc, sqlbase.InvalidMutationID, "updating view reference",

While you're here, can you make this job have a better name (and the one down below)? Just include the name of the view that is causing the update.

pkg/sql/create_view.go, line 222 at r3 (raw file):

		// TODO (lucy): Have more consistent/informative names for dependent jobs.
		if err := params.p.writeSchemaChange(
			params.ctx, backRefMutable, sqlbase.InvalidMutationID, "updating view reference",

This one should also be updated.

pkg/sql/create_view.go, line 292 at r3 (raw file):

}

func addResultColumns(

Add a comment on this.

pkg/sql/create_view.go, line 339 at r3 (raw file):

			)
		}
		if newCol.Hidden != oldCol.Hidden {

What about changing nullability? Or is that not an attribute for view columns?

jordanlewis · 2020-04-10T02:09:36Z

Oops, I didn't mean to mark this as approved yet.

jordanlewis

unapproved! sorry, haha

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, and @RaduBerinde)

pkg/sql/create_view.go, line 322 at r2 (raw file):

Previously, knz (kena) wrote…

I think you need to compare the visibility as well (hidden vs non-hidden). Otherwise the result of *-expansion can change.

Done.

pkg/sql/create_view.go, line 91 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Are there permissions we need to consider here? You shouldn't be allowed to edit a view you don't have write on, or something like that.

Good catch, added.

pkg/sql/create_view.go, line 96 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: You can use %q instead of "%s".

til

pkg/sql/create_view.go, line 115 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I would make this big block a function to make things a little cleaner, and add a comment. "replacingDesc != nil" doesn't really explain what's going on.

Done.

pkg/sql/create_view.go, line 147 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Doesn't this pointlessly drop and re-add the depend relationships if the view is edited but its dependencies not changed? This is the sort of thing that we're trying to cut down on with DDL performance improvements. They add up.

I thought this was going to be too tricky when first doing it but it turned out to be not bad. I've updated this to only do this write on tables that the new view doesn't reference anymore.

pkg/sql/create_view.go, line 149 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

While you're here, can you make this job have a better name (and the one down below)? Just include the name of the view that is causing the update.

Done.

pkg/sql/create_view.go, line 222 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This one should also be updated.

Done.

pkg/sql/create_view.go, line 292 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Add a comment on this.

Done.

pkg/sql/create_view.go, line 339 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

What about changing nullability? Or is that not an attribute for view columns?

Not sure, but theres no harm in adding a check for future proofing.

knz · 2020-04-10T13:12:24Z

(I won't be reviewing this further)

jordanlewis

mod the permissions test!

Just thinking about other random edge cases... will anything go wrong if other views depended on the old view? I think no, because the view will keep its id and all of the columns it used to have, and select * isn't allowed in views.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, @RaduBerinde, and @rohany)

pkg/sql/create_view.go, line 91 at r3 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Good catch, added.

Did you add a test?

Fixes cockroachdb#24897. This PR adds support for the CREATE OR REPLACE VIEW command by allowing the create view statement to optionally overwrite an existing descriptor, rather than always write a new descriptor k/v entry. Release note (sql change): This PR adds support for the CREATE OR REPLACE VIEW command.

rohany

Just thinking about other random edge cases... will anything go wrong if other views depended on the old view? I think no, because the view will keep its id and all of the columns it used to have, and select * isn't allowed in views.

Yeah, i think it will be ok. The checks we do (same column prefix) ensures that nothing goes wrong with the new view.

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @knz, @lucy-zhang, and @RaduBerinde)

pkg/sql/create_view.go, line 91 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Did you add a test?

Gah sorry, added.

rohany · 2020-04-11T15:12:18Z

bors r+

craig · 2020-04-11T16:48:10Z

Build succeeded

GitHub CI (Cockroach)

rohany requested review from jordanlewis, thoszhang and a team April 5, 2020 03:32

rohany requested a review from a team as a code owner April 5, 2020 03:32

rohany added this to Open PRs in SQL Features (Deprecated - use SQL Experience board) via automation Apr 5, 2020

RaduBerinde reviewed Apr 5, 2020

View reviewed changes

rohany force-pushed the create-or-replace-view branch from 02c77fd to abe167d Compare April 5, 2020 18:40

rohany commented Apr 5, 2020

View reviewed changes

rohany force-pushed the create-or-replace-view branch from abe167d to 574437d Compare April 5, 2020 21:00

knz reviewed Apr 6, 2020

View reviewed changes

rohany force-pushed the create-or-replace-view branch from 574437d to 99ce0a3 Compare April 6, 2020 17:11

rohany force-pushed the create-or-replace-view branch from 99ce0a3 to 180dc77 Compare April 6, 2020 19:18

jordanlewis approved these changes Apr 10, 2020

View reviewed changes

jordanlewis requested changes Apr 10, 2020

View reviewed changes

rohany force-pushed the create-or-replace-view branch from 180dc77 to a333b08 Compare April 10, 2020 02:56

rohany commented Apr 10, 2020

View reviewed changes

rohany force-pushed the create-or-replace-view branch from a333b08 to af04126 Compare April 10, 2020 03:02

rohany requested a review from jordanlewis April 11, 2020 01:52

rohany force-pushed the create-or-replace-view branch from af04126 to 9a9b2ea Compare April 11, 2020 02:07

jordanlewis approved these changes Apr 11, 2020

View reviewed changes

rohany force-pushed the create-or-replace-view branch from 9a9b2ea to 749351a Compare April 11, 2020 03:21

rohany commented Apr 11, 2020

View reviewed changes

craig bot merged commit 94b624e into cockroachdb:master Apr 11, 2020

SQL Features (Deprecated - use SQL Experience board) automation moved this from Open PRs to Done Apr 11, 2020

jseldess mentioned this pull request Jun 24, 2020

sql: add support for CREATE OR REPLACE VIEW cockroachdb/docs#7606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add support for CREATE OR REPLACE VIEW #47051

sql: add support for CREATE OR REPLACE VIEW #47051

rohany commented Apr 5, 2020

cockroach-teamcity commented Apr 5, 2020

RaduBerinde left a comment

blathers-crl bot commented Apr 5, 2020

rohany left a comment

blathers-crl bot commented Apr 5, 2020

blathers-crl bot commented Apr 5, 2020

blathers-crl bot commented Apr 5, 2020

knz left a comment

rohany commented Apr 6, 2020

knz commented Apr 6, 2020

rohany commented Apr 6, 2020

RaduBerinde commented Apr 6, 2020

rohany commented Apr 6, 2020

RaduBerinde commented Apr 6, 2020

rohany commented Apr 10, 2020

jordanlewis left a comment

jordanlewis commented Apr 10, 2020

jordanlewis left a comment

rohany left a comment

knz commented Apr 10, 2020

jordanlewis left a comment

rohany left a comment

rohany commented Apr 11, 2020

craig bot commented Apr 11, 2020

sql: add support for CREATE OR REPLACE VIEW #47051

sql: add support for CREATE OR REPLACE VIEW #47051

Conversation

rohany commented Apr 5, 2020

cockroach-teamcity commented Apr 5, 2020

RaduBerinde left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Apr 5, 2020

rohany left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Apr 5, 2020

blathers-crl bot commented Apr 5, 2020

blathers-crl bot commented Apr 5, 2020

knz left a comment

Choose a reason for hiding this comment

rohany commented Apr 6, 2020

knz commented Apr 6, 2020

rohany commented Apr 6, 2020

RaduBerinde commented Apr 6, 2020

rohany commented Apr 6, 2020

RaduBerinde commented Apr 6, 2020

rohany commented Apr 10, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis commented Apr 10, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

knz commented Apr 10, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

rohany commented Apr 11, 2020

craig bot commented Apr 11, 2020

Build succeeded