sql: un-break view descriptors by being more conservative about dependencies#17280
sql: un-break view descriptors by being more conservative about dependencies#17280knz merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
|
I don't know about the migration. It does seem like that's the right thing to do but I'm unsure of the costs of adding such migrations. I wonder if @bdarnell has thoughts there. Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. pkg/sql/logictest/testdata/logic_test/alter_table, line 330 at r1 (raw file):
Please add a test that demonstrates our blunt handling of this issue to make it crystal clear what's going on. Like, Comments from Reviewable |
495e093 to
3b7c810
Compare
|
Done, also added a migration. |
…dencies Prior to this patch, a view defined with ```sql CREATE VIEW vx AS SELECT k FROM (SELECT k, v FROM kv) ``` would only establish a dependency on column `k` of `kv`. Subsequently, `ALTER TABLE kv DROP COLUMN v` would be allowed without error and the view would become broken. There is a spectrum of solutions, see the discussion on cockroachdb#17269. This patch implements the simplest solution that still allows schema changes on depended-on tables: when a view is created, it now establishes a dependency on *all the columns that existed at the time the view is created*. New columns can be added without error; a subsequent patch can also address cockroachdb#10083 (renaming columns depended on by views) which is an orthogonal issue.
3b7c810 to
731ed39
Compare
|
I am merging this to enable progress on #15388. I feel the migration is benign since it does not change descriptor semantics -- whether it applies or not views will work the same, it's only a protection against further |
|
Many thanks @knz! I'm sorry for not considering cases like this. |
|
|
||
| // repopulateViewDeps ensures that each view V that depends on a table | ||
| // T depends on all columns in T. (#17269) | ||
| func repopulateViewDeps(ctx context.Context, r runner) error { |
There was a problem hiding this comment.
This is complicated enough that it really merits a test if possible, though.
There was a problem hiding this comment.
I can't figure a way to create an invalid view descriptor in package migration without exporting a bunch of stuff from sql and sqlbase. What would you suggest?
|
LGTM |
Prior to this patch, a view defined with
would only establish a dependency on column
kofkv. Subsequently,ALTER TABLE kv DROP COLUMN vwould be allowed without error and theview would become broken.
There is a spectrum of solutions, see the discussion on #17269.
This patch implements the simplest solution that still allows schema
changes on depended-on tables: when a view is created, it now
establishes a dependency on all the columns that existed at the time
the view is created. New columns can be added without error; a
subsequent patch can also address #10083 (renaming columns depended on
by views) which is an orthogonal issue.