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: Support renaming objects depended on by views #10083

Open
a-robinson opened this issue Oct 19, 2016 · 18 comments
Open

sql: Support renaming objects depended on by views #10083

a-robinson opened this issue Oct 19, 2016 · 18 comments
Labels
A-schema-changes A-schema-descriptors Relating to SQL table/db descriptor handling. A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-semantics C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@a-robinson
Copy link
Contributor

a-robinson commented Oct 19, 2016

In our initial implementation of views, we store a mostly-unmodified text form of the query. This prevents us from allowing renames on the objects used in the query without doing query rewriting, which is quite difficult.

We plan to switch to a semantic encoding that would only rely on the IDs of objects in the query when possible (initial discussion of such an encoding is in #10055). Once that time has come, we should remove the restrictions on renaming dependent objects.

Opening this primarily for tracking.

Jira issue: CRDB-6143

@a-robinson a-robinson added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Oct 19, 2016
@a-robinson a-robinson added the S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. label Oct 28, 2016
@petermattis petermattis added this to the Q1 2017 milestone Feb 23, 2017
@petermattis
Copy link
Collaborator

@knz Is this feasible in the 1.0 timeframe? It doesn't seem necessary, but I know you're working towards this.

@knz
Copy link
Contributor

knz commented Feb 23, 2017

yes, this is feasible if we prioritize #12715 - that will give us a solution to this issue immediately.

@knz
Copy link
Contributor

knz commented Feb 23, 2017

I got #13781 submitted as groundwork for this.
The remaining step is to implement a transform function that rewrites the table expressions in the view definition to become numeric table/index references prior to storing the definition in the view descriptor.

@dianasaur323 dianasaur323 modified the milestones: 1.0, Q1 2017 Apr 14, 2017
@dianasaur323
Copy link
Contributor

Bringing into 1.0 milestone given the history of conversation, but if not necessary at this point, please move out into 1.1 milestone. Assigning @knz. Thanks!

@knz
Copy link
Contributor

knz commented Apr 17, 2017

I can look into it. It is certainly easier now than it was three months ago, but I need to check relative to other work items.

Unless we have a very strong need I would simply push it to 1.1 straight away. Our list of strong 1.0 issues is already large.

@dianasaur323
Copy link
Contributor

@knz let's move out.

@dianasaur323 dianasaur323 modified the milestones: 1.1, 1.0 Apr 18, 2017
@a-robinson
Copy link
Contributor Author

Agreed - features should already be frozen and this isn't of critical major importance.

@cuongdo
Copy link
Contributor

cuongdo commented Jul 7, 2017

@knz is this going to be resolved before 1.1 feature freeze?

@knz
Copy link
Contributor

knz commented Jul 7, 2017

Iv'e scheduled a meeting to discuss this next week.

@knz
Copy link
Contributor

knz commented Jul 7, 2017

It depends on #15388 too

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 Aug 8, 2017
@jordanlewis
Copy link
Member

Reopening this, as we'd like to make progress on it before doing a larger migration to store ids instead of names in stored descriptors.

@jordanlewis jordanlewis reopened this May 26, 2020
@jordanlewis jordanlewis assigned RichardJCai and unassigned knz May 26, 2020
@jordanlewis jordanlewis removed this from the 2.1 milestone May 26, 2020
@RichardJCai
Copy link
Contributor

I've been thinking about doing this without referring to ColumnIDs and it seems pretty tricky. With the way we store view queries right now, we could potentially have something like

CREATE TABLE t(x int, y int);
CREATE VIEW vx AS SELECT x, y AS z FROM T;
ALTER TABLE t RENAME COLUMN x TO z;

Our resulting view would be
SELECT z, y AS z FROM T

Postgres avoids this by renaming one of the z's to z_1 but if this happens inside a subquery, we'd have to track which to rename as z_1. It would be tricky to do especially since we don't force subqueries to use aliases unlike Postgres' views.

To support ids for #22212. If we want to update all views to use ids, we would have to perform a rename but we wouldn't have to deal with this variable capture issue.

@shermanCRL
Copy link
Contributor

We’ve got a customer request on this too, specifically that a table rename should not require dropping a view dependent on that table. (Let me know if this is the right issue.)

@ajwerner
Copy link
Contributor

ajwerner commented Jul 2, 2021

This is the right issue. We went through a bunch of discussions on this in the spring. The right want to do it will require machinery we don't currently have. See this long internal thread on the topic: https://cockroachlabs.slack.com/archives/C04U1BTF8/p1614712126092900.

The crux of the problem is that scoping in SQL is complex. We have one component in the whole system which understands it: the optbuilder. The problem is that what the optbuilder produces as it digests the statement throws away some information we'd need to rewrite the AST appropriately. The proposal in that thread is to have the optbuilder populate a mapping that as it builds the statement and then use that mapping to then rewrite the whole statement in a way that is name agnostic. It's not a small project but it does seem like something we need to do to unblock this issue. It'd be a generally very good thing.

@postamar
Copy link
Contributor

relates to #83233

@shermanCRL
Copy link
Contributor

Experienced by a customer as part of a table restore process, see internal thread.

@rimadeodhar
Copy link
Collaborator

Two issues related to this are being planned for the 23.2 release: #98606 and #98607

@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-changes A-schema-descriptors Relating to SQL table/db descriptor handling. A-sql-pgcompat Semantic compatibility with PostgreSQL A-sql-semantics C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

No branches or pull requests