sql: support views that depend on virtual views #39195
Conversation
|
This change is |
There was a problem hiding this comment.
LGTM but you're weakening a guarantee that we were previolusly providing: when the column of a dependency gets renamed, prior to this we guaranteed that dependent views would not break (either by blocking the rename, or disallowing the dependency outright). With your change the vtable/vview can rename its column and any dependent view will not notice.
In any case I'd recommend spelling this out in the release not.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
There was a problem hiding this comment.
I don't understand, it's not possible to rename a column of a virtual table or view, is it? Or are you saying if we make a change between versions? (I don't see how we could fix that, because we can't allow the dependent view to block the upgrade)
Reviewable status:
yes
The proper way is to make the view depend on other things structurally, using table/column IDs and not names. |
There was a problem hiding this comment.
I see. Note though that this is already the case for vtables. This change only concerns the one virtual view we have (crdb_internal.ranges).
The proper way is to make the view depend on other things structurally, using table/column IDs and not names.
But for that to work, we'd still need to keep old column IDs in the vtable/vview valid and populated on all subsequent versions no?
Reviewable status:
My previous comments only pertain to renames -- I was assuming that the columns would remain across versions. For column deletions, I have no good story to tell. |
When creating a view, we report a plan dependency on virtual views which causes a failure. This commit fixes the logic to avoid reporting a dependency in this case (just like we don't report dependencies on virtual tables). Fixes cockroachdb#38440. Release note (bug fix): crdb_internal.ranges can now be used inside views. Note that such views can become invalid in future releases if `crdb_internal.ranges` changes.
RaduBerinde
force-pushed
the
virtual-view-dep
branch
from
84c8c43
to
39253e4
Compare
|
Got it. Updated the release note. TFTR! bors r+ |
39195: sql: support views that depend on virtual views r=RaduBerinde a=RaduBerinde When creating a view, we report a plan dependency on virtual views which causes a failure. This commit fixes the logic to avoid reporting a dependency in this case (just like we don't report dependencies on virtual tables). Fixes #38440. Release note (bug fix): crdb_internal.ranges can now be used inside views. Note that such views can become invalid in future releases if `crdb_internal.ranges` changes. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Build succeeded |
When creating a view, we report a plan dependency on virtual
views which causes a failure. This commit fixes the logic to avoid
reporting a dependency in this case (just like we don't report
dependencies on virtual tables).
Fixes #38440.
Release note (bug fix): crdb_internal.ranges can now be used inside
views. Note that such views can become invalid in future releases if
crdb_internal.rangeschanges.