sql: re-vamp the view dependency analysis #17310

knz · 2017-07-30T15:36:33Z

Prior to this patch, view dependency analysis during CREATE VIEW was broken because it would only check dependencies after plan optimization, i.e. possibly after some dependency were lost. For example, with the queries:

CREATE VIEW v AS SELECT k FROM (SELECT k,v FROM kv) -- loses dependency on kv.v 
CREATE VIEW v AS SELECT k,v FROM kv WHERE FALSE -- loses dependency on kv

This patch addresses the issue as follows:

the dependencies are now collected during the initial construction of the query plan, before any optimization are applied. This way we ensure the dependency tracking is complete.
a migration is implemented that will fix any view descriptor and corresponding dependency information that was populated prior to this fix.

Fixes #17306.
Fixes #17290.
Replaces 74c7b0f (reverted via #17305)
Needed for #15388.

cockroach-teamcity · 2017-07-30T15:36:40Z

This change is

knz · 2017-07-31T14:05:59Z

okay so:

@a-robinson @benesch please have a look at my migration test and see if that is well aligned with the spirit of the thing
@jordanlewis @andreimatei @RaduBerinde one of you please have a look at the new dependency analysis code and provide feedback

a-robinson · 2017-07-31T14:56:41Z

I'm not sure dropping the view's dependencies and checking that the views don't work without them was really needed, but the test looks reasonable to me.

Reviewed 2 of 33 files at r1.
Review status: 2 of 33 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

benesch · 2017-07-31T19:57:01Z

pkg/migrations/migrations_test.go

+	defer s.Stopper().Stop(ctx)
+	e := s.Executor().(*sql.Executor)
+
+	t.Run("create test tables", func(t *testing.T) {


It's not clear to me that these subtests are providing any value, since they can't be run independently of one another. Plus, each subtest creates its own session, which adds quite a bit of boilerplate. @knz, does this exercise some codepath that wouldn't be executed if you used the same session throughout?

benesch · 2017-07-31T19:59:16Z

pkg/migrations/migrations_test.go

+	})
+
+	// Now, corrupt the descriptors by breaking their dependency information.
+	t.Run("break descriptors", func(t *testing.T) {


E.g., this will segfault if someone runs make test PKG=./pkg/migration TESTS=TestUpdateViewDependenciesMigration/break_descriptors. I feel like you might be better off with t.Logf("break descriptors").

knz · 2017-08-01T15:23:04Z

Test simplified thanks to generous suggestions by Jordan and Nikhil.

Review status: 1 of 29 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/migrations/migrations_test.go, line 539 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

It's not clear to me that these subtests are providing any value, since they can't be run independently of one another. Plus, each subtest creates its own session, which adds quite a bit of boilerplate. @knz, does this exercise some codepath that wouldn't be executed if you used the same session throughout?

Done.

pkg/migrations/migrations_test.go, line 598 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

E.g., this will segfault if someone runs make test PKG=./pkg/migration TESTS=TestUpdateViewDependenciesMigration/break_descriptors. I feel like you might be better off with t.Logf("break descriptors").

Done.

Comments from Reviewable

andreimatei · 2017-08-01T18:35:20Z

LGTM

The second "fixes" in the PR comment links to a PR. Is that what you wanted?

After we talked, I thought some more: for the future, I still think that having a plan construction mode where all optimizations are disabled, and then collecting the dependencies by visiting that, seems like a cleaner way than your proposed followed: the recording SchemaAccessor. You said that it wouldn't work because the optimizations change the typing of the view but a) I don't see why that's true and b) you don't have to do anything with the unoptimized plan besides collect the dependencies; you can construct a new, optimized plan afterwards for typing and whatever else.
One problem is see is the index dependencies (that's a thing, right?) if the index selection changes with optimizations. But perhaps we can build both the optimized and the unoptimized plans and merge the dependencies from both?
Does this make sense? I'm thinking that a plan creation mode without any optimizations could be more generally useful.

Review status: 0 of 19 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

pkg/sql/create.go, line 334 at r2 (raw file):

// table -- which index and columns are being depended upon.
type planDependencyInfo struct {
	desc *sqlbase.TableDescriptor

why is this a pointer?

pkg/sql/create.go, line 382 at r2 (raw file):

	// The traversal will update the NormalizableTableNames in-place,
	// so the changes are persisted in n.AsSource. We discard
	// the result of pretty-printing.

I was confused about what the reference to pretty-printing is telling me.
I'd say something like "we use FormatNode` as a tree visitor".

pkg/sql/create.go, line 398 at r2 (raw file):

				}
				// Persist the database prefix expansion.
				tn.DBNameOriginallyOmitted = false

nit: I don't know anything about anything, but this line suggests to me that DBNameOriginallyOmitted is badly named, since we're lying to it. Perhaps it should be named based on its effect, which I guess is something like "UseOriginalUnqualifiedWhenPrinting`.

pkg/sql/create.go, line 414 at r2 (raw file):

	// Request dependency tracking.
	defer func(prev planDependencies) { p.planDeps = prev }(p.planDeps)

I don't understand what this does and how it's "requesting" anything. You're restoring p.planDeps at the end?

pkg/sql/create.go, line 423 at r2 (raw file):

	}

	log.Infof(ctx, "collected view dependencies:\n%s", p.planDeps.String())

Infof seems high to me

pkg/sql/create.go, line 423 at r2 (raw file):

	}

	log.Infof(ctx, "collected view dependencies:\n%s", p.planDeps.String())

isn't p.planDeps empty here? So confused.

pkg/sql/create.go, line 464 at r2 (raw file):

// view descriptors created prior to fixing #17269 and #17306;
// it may also be used by a future "fsck" utility.
func RecomputeViewDependencies(ctx context.Context, txn *client.Txn, e *Executor) error {

bleah. I think we should move this function to another file that nobody needs to see, specific to migrations.

pkg/sql/create.go, line 669 at r2 (raw file):

		for _, dep := range updated.deps {
			// The logical plan constructor merely registered the dependencies.
			// It did not populate the "ID" field of TableDescriptor_Reference.

why didn't the ctor populate this field? Is it didn't know its own id?

pkg/sql/planner.go, line 77 at r2 (raw file):

	cancelChecker CancelChecker

	// planDeps, if non-nil, collects the table/view dependencies for this query.

please expand the comment saying something about the responsibilities of the node ctors when this is non-nil: to record what descriptors they're using.

And then get rid of this member soon :P

Comments from Reviewable

knz · 2017-08-01T22:50:45Z

Review status: 0 of 19 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

pkg/sql/create.go, line 334 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

why is this a pointer?

Added a clarifying comment.

pkg/sql/create.go, line 382 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I was confused about what the reference to pretty-printing is telling me.
I'd say something like "we use FormatNode` as a tree visitor".

Done.

pkg/sql/create.go, line 398 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: I don't know anything about anything, but this line suggests to me that DBNameOriginallyOmitted is badly named, since we're lying to it. Perhaps it should be named based on its effect, which I guess is something like "UseOriginalUnqualifiedWhenPrinting`.

I agree. Filed this separately as #17374.

pkg/sql/create.go, line 414 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I don't understand what this does and how it's "requesting" anything. You're restoring p.planDeps at the end?

Yes! This is a general pattern since this planNode constructor is really a visitor, and there may be a parent with a different idea of what planDeps should be. We do the same with avoidCachedDescriptors above for the same reason.

pkg/sql/create.go, line 423 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

isn't p.planDeps empty here? So confused.

Read the code above :)

pkg/sql/create.go, line 423 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Infof seems high to me

Indeed. Changed to VEventf.

pkg/sql/create.go, line 464 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

bleah. I think we should move this function to another file that nobody needs to see, specific to migrations.

I am going to refactor this code in the next review iteration to put all the view-related stuff in its own file. How does that sound?

pkg/sql/create.go, line 669 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

why didn't the ctor populate this field? Is it didn't know its own id?

Explained in the comment above the definition of the field.

pkg/sql/planner.go, line 77 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

please expand the comment saying something about the responsibilities of the node ctors when this is non-nil: to record what descriptors they're using.

And then get rid of this member soon :P

Done.

Comments from Reviewable

knz · 2017-08-01T23:21:19Z

Review status: 0 of 23 files reviewed at latest revision, 11 unresolved discussions.

pkg/sql/create.go, line 464 at r2 (raw file):

Previously, knz (kena) wrote…

I am going to refactor this code in the next review iteration to put all the view-related stuff in its own file. How does that sound?

Ok done already.

Comments from Reviewable

andreimatei · 2017-08-02T00:51:06Z

Reviewed 1 of 33 files at r2.
Review status: 1 of 23 files reviewed at latest revision, 12 unresolved discussions, some commit checks pending.

pkg/migration/sqlmigrations/migrations.go, line 420 at r3 (raw file):

// repopulateViewDeps ensures that each view V that depends on a table
// T depends on all columns in T. (#17269 #17306)

this comment talks about all columns but sql.RecomputeViewDeps doesn't (so I assume it just computes the correct cols). Which one is it? Or what does "all" mean?

pkg/migration/sqlmigrations/migrations.go, line 422 at r3 (raw file):

// T depends on all columns in T. (#17269 #17306)
func repopulateViewDeps(ctx context.Context, r runner) error {
	// System tables can only be modified by a privileged internal user.

I don't know what this comment about the privileged user is telling me. Is it about this code, or an assumption about the caller? I don't see any use anywhere around here.

pkg/migration/sqlmigrations/migrations.go, line 423 at r3 (raw file):

func repopulateViewDeps(ctx context.Context, r runner) error {
	// System tables can only be modified by a privileged internal user.
	// Retry a limited number of times because returning an error and letting

is this comment about retrying necessary (and talking about panicking at the wrong level - this code doesn't panic)? Nobody would be tempted to try something forever (particularly since this interface supports returning an error), would they?

pkg/sql/create.go, line 414 at r2 (raw file):

Previously, knz (kena) wrote…

Yes! This is a general pattern since this planNode constructor is really a visitor, and there may be a parent with a different idea of what planDeps should be. We do the same with avoidCachedDescriptors above for the same reason.

ummm isn't this horrible? If there were a parent which cares about planDeps, wouldn't it also care about the deps accumulated under this view? I don't know the answer to that question, so:
Can there really be a parent on top of a CREATE VIEW? If not, I'd suggest we find a way to assert that there is no such parent through the planner, and removing this code.

pkg/sql/create.go, line 669 at r2 (raw file):

Previously, knz (kena) wrote…

Explained in the comment above the definition of the field.

which field?

pkg/sql/views.go, line 35 at r3 (raw file):

	// desc is a reference to the descriptor for the table being
	// depended on. We keep a reference and not a copy because
	// dependency analysis does not need to modify the descriptor.

well but what about if someone else wants to modify the descriptor. If there's no good reason, save me from thinking about anything and make a copy of the proto. This is just used for schema operations, right? So performance doesn't matter.

pkg/sql/views.go, line 79 at r3 (raw file):

) (planDependencies, sqlbase.ResultColumns, error) {
	// To avoid races with ongoing schema changes to tables that the view
	// depends on, make sure we use the most recent versions of table

I spent a bunch of time being confused around here; I think this comment misses the point: what's important is not (only) that we use the current version of descriptors, but that the reads and writes of descriptors are transactional. The comment should reference saveNonmutationAndNotify which does the write.

Also, I think that function needs a comment about how the transaction would better have read the descriptor that it's writing. And to make it safe, I'd also change it to be a CPut and test the existing value. I'd also comment on the definition of avoidCachedDescriptors to highlight this use (which now might be the only use, after @vivekmenezes makes all caches timestamp-aware): the fact that the current txn is going to write descriptors and so it needs to make sure it actually reads them first.

We should also assert here that the transaction is SERIALIZABLE and refuse to allow creation of views in SNAPSHOT transactions :S

Comments from Reviewable

knz · 2017-08-02T09:02:11Z

Thank you for your insightful comment, especially the very valuable insight on avoidCachedDescriptors. I will investigate this further, I think we may have some more bugs lurking.

Review status: 0 of 23 files reviewed at latest revision, 12 unresolved discussions.

pkg/migration/sqlmigrations/migrations.go, line 420 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

this comment talks about all columns but sql.RecomputeViewDeps doesn't (so I assume it just computes the correct cols). Which one is it? Or what does "all" mean?

Moved the comment to RecomputeViewDependencies where it belongs.

pkg/migration/sqlmigrations/migrations.go, line 422 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I don't know what this comment about the privileged user is telling me. Is it about this code, or an assumption about the caller? I don't see any use anywhere around here.

The comment was stale, removed it. Thanks for noticing.

pkg/migration/sqlmigrations/migrations.go, line 423 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

is this comment about retrying necessary (and talking about panicking at the wrong level - this code doesn't panic)? Nobody would be tempted to try something forever (particularly since this interface supports returning an error), would they?

Yes you're right. I simplified the code instead.

pkg/sql/create.go, line 414 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ummm isn't this horrible? If there were a parent which cares about planDeps, wouldn't it also care about the deps accumulated under this view? I don't know the answer to that question, so:
Can there really be a parent on top of a CREATE VIEW? If not, I'd suggest we find a way to assert that there is no such parent through the planner, and removing this code.

I am going to respectfully and politely ignore this comment. This is really a common pattern in tree traversals. We can discuss more offline.

pkg/sql/create.go, line 669 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

which field?

Extended this comment to clarify.

pkg/sql/views.go, line 35 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

well but what about if someone else wants to modify the descriptor. If there's no good reason, save me from thinking about anything and make a copy of the proto. This is just used for schema operations, right? So performance doesn't matter.

Peter and I thoroughly checked together that referenced descriptors are not being modified anywhere, see the discussion on #17309.
There's absolutely no good reason to waste memory. We do the same in scanNode now.

pkg/sql/views.go, line 79 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I spent a bunch of time being confused around here; I think this comment misses the point: what's important is not (only) that we use the current version of descriptors, but that the reads and writes of descriptors are transactional. The comment should reference saveNonmutationAndNotify which does the write.

Also, I think that function needs a comment about how the transaction would better have read the descriptor that it's writing. And to make it safe, I'd also change it to be a CPut and test the existing value. I'd also comment on the definition of avoidCachedDescriptors to highlight this use (which now might be the only use, after @vivekmenezes makes all caches timestamp-aware): the fact that the current txn is going to write descriptors and so it needs to make sure it actually reads them first.

We should also assert here that the transaction is SERIALIZABLE and refuse to allow creation of views in SNAPSHOT transactions :S

Aha! This very comment reminds me why I love it when you review a PR.

I did not really understand what was this thing with "avoiding cached descriptors" before. Now I understand! Thank you!
I added an extensive explanatory comment on the definition of the avoidCachedDescriptor field to reflect this newly gained knowledge.

Also I understand that:

we indeed absolutely need to check that the transaction is SERIALIZABLE for DDL that update descriptors like CREATE VIEW but also anything else that updates descriptors in-place. I'll need to check that, we may have some bugs lurking!
we are probably not doing the right thing neither for views nor FKs that reference system tables (since the latter create an index and may modify the system descriptors); these txns need to be anchored on the system range with SetSystemConfigTrigger (they are not currently)

This is worth another commit on its own, which I might insert in-between master and this one. I will investigate further. Thank you for the patient explanation.

Comments from Reviewable

knz · 2017-08-02T18:07:44Z

Filed the two schema problems as separate issues #17404 #17405. They are quite important but can be addressed orthogonally to this PR.

The views in pg_catalog and information_schema provide introspection over the logical schema (pure SQL view) however when testing/debugging it is also important to see the descriptor IDs and also other fields of descriptors. This patch provides help for debugging with the following new virtual tables: - `crdb_internal.table_columns`: detailed column IDs and properties - `crdb_internal.table_indexes`: detailed index IDs and index properties - `crdb_internal.index_columns`: what columns are indexed and how - `crdb_internal.backward_dependencies`: which other descriptors each descriptor depends on - `crdb_internal.forward_dependencies`: which other descriptors each descriptor is depended on by These virtual tables are intended to display the contents of descriptors without any data transformation, so as to reveal any error in the descriptors, if any. Debugging/fsck tools can later use these to check schema validity.

knz · 2017-08-03T17:14:06Z

Rebased on top of #17422 to make the fix more obvious.

andreimatei · 2017-08-03T20:01:59Z

Review status: 0 of 30 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

Comments from Reviewable

... because it was incomplete (missing FK relationships). The two new tables `crdb_internal.forward_dependencies` and `crdb_internal.backward_dependencies` can be used instead.

Prior to this patch, view dependency analysis during CREATE VIEW was broken because it would only check dependencies after plan optimization, i.e. possibly after some dependency were lost. For example, with the queries: ```sql CREATE VIEW v AS SELECT k FROM (SELECT k,v FROM kv) -- loses dependency on kv.v CREATE VIEW v AS SELECT k,v FROM kv WHERE FALSE -- loses dependency on kv ``` This patch addresses the issue as follows: - the dependencies are now collected during the initial construction of the query plan, before any optimization are applied. This way we ensure the dependency tracking is complete. - a migration is implemented that will fix any view descriptor and corresponding dependency information that was populated prior to this fix.

knz · 2017-08-04T13:00:02Z

TFYR

andreimatei · 2017-09-21T22:32:22Z

Review status: 0 of 30 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

pkg/sql/views.go, line 79 at r3 (raw file):

Previously, knz (kena) wrote…

Aha! This very comment reminds me why I love it when you review a PR.

I did not really understand what was this thing with "avoiding cached descriptors" before. Now I understand! Thank you!
I added an extensive explanatory comment on the definition of the avoidCachedDescriptor field to reflect this newly gained knowledge.

Also I understand that:

we indeed absolutely need to check that the transaction is SERIALIZABLE for DDL that update descriptors like CREATE VIEW but also anything else that updates descriptors in-place. I'll need to check that, we may have some bugs lurking!

we are probably not doing the right thing neither for views nor FKs that reference system tables (since the latter create an index and may modify the system descriptors); these txns need to be anchored on the system range with SetSystemConfigTrigger (they are not currently)

This is worth another commit on its own, which I might insert in-between master and this one. I will investigate further. Thank you for the patient explanation.

This was further discussed in #17404

Comments from Reviewable

knz requested review from jordanlewis, andreimatei and a-robinson July 30, 2017 15:36

knz force-pushed the 20170730-fix-view-deps-more branch from 66fa9dc to d2699e3 Compare July 30, 2017 15:39

This was referenced Jul 31, 2017

sql: make scanNode embed a reference to the table descriptor, not a copy #17309

Merged

sql/parser: make NormalizableTableReference persist results of formatting #17315

Merged

knz force-pushed the 20170730-fix-view-deps-more branch 2 times, most recently from 04fd2a6 to 646c05d Compare July 31, 2017 13:49

benesch reviewed Jul 31, 2017

View reviewed changes

This was referenced Aug 1, 2017

sql: streaming results through pgwire #17019

Merged

sql: different schema update logic in tests and real sessions #17347

Closed

knz force-pushed the 20170730-fix-view-deps-more branch from 646c05d to 809b813 Compare August 1, 2017 15:21

knz force-pushed the 20170730-fix-view-deps-more branch 3 times, most recently from b5e81a7 to 285fb22 Compare August 1, 2017 16:18

knz mentioned this pull request Aug 1, 2017

sql/parser: rename "DBNameOriginallyOmitted" to "OmitDBNameDuringFormatting" #17374

Closed

knz force-pushed the 20170730-fix-view-deps-more branch from 285fb22 to 660c16f Compare August 1, 2017 22:50

knz force-pushed the 20170730-fix-view-deps-more branch from 660c16f to 19a8af5 Compare August 1, 2017 23:17

knz force-pushed the 20170730-fix-view-deps-more branch 2 times, most recently from a9affb5 to 83f873c Compare August 2, 2017 00:22

knz requested a review from a team as a code owner August 2, 2017 00:22

knz force-pushed the 20170730-fix-view-deps-more branch from 83f873c to a98b7f8 Compare August 2, 2017 09:00

knz force-pushed the 20170730-fix-view-deps-more branch from a98b7f8 to ceaf228 Compare August 2, 2017 10:20

This was referenced Aug 2, 2017

sql: CREATE TABLE/VIEW works on non-serializable transactions, but shouldnt #17404

Closed

sql: CREATE VIEW or FKs over system tables do not set the system config trigger #17405

Closed

knz force-pushed the 20170730-fix-view-deps-more branch from ceaf228 to 724d983 Compare August 3, 2017 11:39

knz mentioned this pull request Aug 3, 2017

sql: implement new crdb_internal virtual tables to inspect descriptors #17422

Merged

knz force-pushed the 20170730-fix-view-deps-more branch from 724d983 to e1dcadd Compare August 3, 2017 17:11

knz force-pushed the 20170730-fix-view-deps-more branch 2 times, most recently from 8b4efac to a4de72a Compare August 3, 2017 17:40

knz added 2 commits August 4, 2017 12:30

sql: remove the dependencies column from crdb_internal.create_statements

148eeaa

... because it was incomplete (missing FK relationships). The two new tables `crdb_internal.forward_dependencies` and `crdb_internal.backward_dependencies` can be used instead.

knz force-pushed the 20170730-fix-view-deps-more branch from a4de72a to e28811c Compare August 4, 2017 12:31

knz merged commit 824b975 into cockroachdb:master Aug 4, 2017

knz deleted the 20170730-fix-view-deps-more branch August 4, 2017 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: re-vamp the view dependency analysis #17310

sql: re-vamp the view dependency analysis #17310

knz commented Jul 30, 2017 •

edited

Loading

cockroach-teamcity commented Jul 30, 2017

knz commented Jul 31, 2017

a-robinson commented Jul 31, 2017

benesch Jul 31, 2017

benesch Jul 31, 2017

knz commented Aug 1, 2017

andreimatei commented Aug 1, 2017

knz commented Aug 1, 2017

knz commented Aug 1, 2017

andreimatei commented Aug 2, 2017

knz commented Aug 2, 2017

knz commented Aug 2, 2017

knz commented Aug 3, 2017

andreimatei commented Aug 3, 2017

knz commented Aug 4, 2017

andreimatei commented Sep 21, 2017

sql: re-vamp the view dependency analysis #17310

sql: re-vamp the view dependency analysis #17310

Conversation

knz commented Jul 30, 2017 • edited Loading

cockroach-teamcity commented Jul 30, 2017

knz commented Jul 31, 2017

a-robinson commented Jul 31, 2017

benesch Jul 31, 2017

Choose a reason for hiding this comment

benesch Jul 31, 2017

Choose a reason for hiding this comment

knz commented Aug 1, 2017

andreimatei commented Aug 1, 2017

knz commented Aug 1, 2017

knz commented Aug 1, 2017

andreimatei commented Aug 2, 2017

knz commented Aug 2, 2017

knz commented Aug 2, 2017

knz commented Aug 3, 2017

andreimatei commented Aug 3, 2017

knz commented Aug 4, 2017

andreimatei commented Sep 21, 2017

knz commented Jul 30, 2017 •

edited

Loading