Skip to content

sql: views do not track their dependencies properly #17269

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

Closed
knz opened this issue Jul 27, 2017 · 7 comments
Closed

sql: views do not track their dependencies properly #17269

knz opened this issue Jul 27, 2017 · 7 comments
Assignees
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone

Comments

@knz
Copy link
Contributor

knz commented Jul 27, 2017

It is possible to break a view by removing a column from the table it depends on:

CREATE TABLE kv(k INT, v INT);
CREATE VIEW v1 AS SELECT k FROM (SELECT k,v FROM kv) AS t;
ALTER TABLE kv DROP COLUMN v;
SELECT * FROM v1; -- fails with `pq: column 2 does not exist`

The problem is that the view tracks dependencies based on "needed columns", which is a result of logical plan optimization. A column that is named by the query but not used by the view results becomes "unneeded" and is thus not tracked as dependency. When it is deleted from the source table, the view plan cannot be constructed any more.

cc @cockroachdb/sql-planning @cockroachdb/sql-async

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 27, 2017
@knz knz added this to the 1.1 milestone Jul 27, 2017
@knz knz self-assigned this Jul 27, 2017
@knz
Copy link
Contributor Author

knz commented Jul 27, 2017

After some brainstorm with @RaduBerinde and @jordanlewis there are multiple solutions. To compare them we need first to establish what the solution must solve:

  1. the problem at hand above (columns being dropped)
  2. not break views when columns are added
  3. enable views using star expansion (e.g. create view v as select * from t) sql: Support view queries with star expansions #10028
  4. enable renaming columns/tables safely sql: Support renaming objects depended on by views #10083 sql: make view descriptors depend on table/columns by IDs #15388

Here are the solutions we found so far, I am giving them "small names" for the table below.

  • blunt-nochange: stop tracking individual column dependencies. If a view b depends on table a, forbid any changes to a's schema (either dropping, adding or renaming columns)
  • blunt-nodrop: ditto blunt-nochange, but allow adding or renaming columns (not dropping). Use table references in view queries to allow renames. This is a subset of the changes in sql: make view descriptors depend on table/columns by IDs #15388.
  • recompile-views-sc. Whenever a table's schema is updated, iterate over all the views that depend on it and try to compile their view query. If that fails, reject the schema change.
  • plan-to-sql: create a way to generate valid SQL text from a logical plan. Use that in views: after the view plan is prepared during CREATE VIEW, store the plan in the descriptor. Convert the plan back to SQL only in SHOW CREATE VIEW.
  • table-refs: replace the table names in the view query by numeric table references. Allow drops by creating fake null columns in stored view queries. This is the approach that would fix the bug in sql: make view descriptors depend on table/columns by IDs #15388
  • track-named-columns: during planning, mark columns that are named in the view query with all simplifications disabled - partial evaluation of expressions, render expression nullification etc must be disabled - to ensure that all the names present in the original SQL text of the view are also recognized/marked as being used by this new algorithm. Then use this information to construct the view dependencies.
  • edit-ast: during plan simplification, any time an expression is elided from the plan also edit the AST to remove it from there. Whenever a star is expanded, edit the AST with the expansion.
solution \ problem solved 1 2 3 4 Notes Eng effort
blunt-nochange yes yes yes yes Not pleasant for users! S
recompile-views-sc yes yes no no S
blunt-nodrop yes yes no yes M
table-refs yes yes yes yes Adds complexity in logical plan semantics M
track-named-columns yes yes yes yes Will be less "precise" wrt dep tracking than the current code. L
edit-ast yes yes yes yes XL
plan-to-sql yes yes yes yes Some plans may no have any valid SQL repr XL

I had started as part of the work in #15388 to implement the solution "table-refs" however @jordanlewis is of the opinion we should invest the extra time into solution "track-named-columns" instead because it brings us closer to what pg does without making our logical planning / optimizations more complicated.

@knz
Copy link
Contributor Author

knz commented Jul 27, 2017

From a strategic perspective:

Would this suggest we need to implement blunt-nochange or blunt-nodrop in the time being?

@knz
Copy link
Contributor Author

knz commented Jul 27, 2017

Note that "not solving this bug" is rather unfortunately not an option: this is a path to make the entire schema of a database unusable (broken views / no way to fix it without cluster downtime).

@knz
Copy link
Contributor Author

knz commented Jul 27, 2017

(For reference: "What would postgres do?" - postgres has a plan-to-sql solution.)

@vivekmenezes
Copy link
Contributor

as a work around the user can drop the view and recreate.

DROP view t;
CREATE VIEW v1 AS SELECT k FROM (SELECT k FROM kv) AS t;

The application will see errors until the new view is created.

knz added a commit to knz/cockroach that referenced this issue Jul 28, 2017
…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.
knz added a commit to knz/cockroach that referenced this issue Jul 28, 2017
…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.
@knz knz modified the milestones: 1.2, 1.1 Jul 28, 2017
@knz
Copy link
Contributor Author

knz commented Jul 28, 2017

Moving to 1.2 since #17280 alleviates / works around the issue.

@knz knz modified the milestones: 2.0, 1.1, 2.1 Jan 22, 2018
@knz knz added the A-schema-descriptors Relating to SQL table/db descriptor handling. label Apr 28, 2018
@knz
Copy link
Contributor Author

knz commented Aug 23, 2018

The original problem mentioned in this issue was actually durably solved by #17280.

What remains now is the opposite problem, filed separately: #29021.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

2 participants