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.

Sign up for GitHub

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

Jump to bottom

sql: deprecate cross-database references between catalog elements #55791

Open
thoszhang opened this issue Oct 21, 2020 · 5 comments
Open

sql: deprecate cross-database references between catalog elements #55791

thoszhang opened this issue Oct 21, 2020 · 5 comments
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@thoszhang
Copy link
Contributor

thoszhang commented Oct 21, 2020 •
edited by cockroach-jira-scripts
Loading

As part of moving toward a more Postgres-compatible world where databases are isolated from each other, we're going to stop supporting references between catalog elements (i.e., tables, types, and schemas) in different databases. The following kinds of references will only be permitted for tables/types in the same database:

In 20.2 we introduced cluster settings to disallow creating new cross-database references for foreign keys, sequence owners, and views by default (#54126).

The proposal is that in 21.1 we'll always disallow the creation of new cross-database references, and have a mechanism to prevent upgrading to 21.2 until users have removed all their existing cross-database references by either moving tables or removing the cross-database schema elements. Specifically, we'd use the long-running migration framework, and have a potentially very long running migration that continually checks whether users have finished removing their cross-database references, and only completes successfully upon confirming that they have.

Other concerns:

Epic: CRDB-1519

Jira issue: CRDB-3631
@thoszhang thoszhang added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Oct 21, 2020
@thoszhang
Copy link
Contributor Author

The current thinking is that we'll start the migration for #55793 automatically after the user has set some flag (via cluster setting) and implement all this as one long-running migration.

@ajwerner
Copy link
Contributor

One important consideration for this task is the interaction with RESTORE of a backup which includes such references. What is the user story? Do we reject them?

cc @mwang1026

@mwang1026
Copy link

Thanks for the ping. Let me bring this to the team to discuss. My initial thought is to invalidate them with some sort of warning after the fact, but want to talk through other options too (e.g. invalidation is an option, otherwise error if we find them).

@mwang1026 mwang1026 mentioned this issue Nov 10, 2020
Closed
@ericharmeling ericharmeling mentioned this issue Nov 10, 2020
Closed
@thoszhang thoszhang mentioned this issue Dec 4, 2020
Closed
@vy-ton vy-ton mentioned this issue Dec 10, 2020
Closed
@rafiss rafiss mentioned this issue Feb 8, 2021
Merged
@solongordon solongordon mentioned this issue Feb 12, 2021
Open
@jlinder jlinder added the T-sql-schema-deprecated Use T-sql-foundations instead label Jun 16, 2021
@postamar
Copy link
Contributor

@ajwerner should we add a cluster upgrade precondition for 23.1 which checks that the cross-database cluster setting is disabled, to consider this done?

@ajwerner
Copy link
Contributor

Steps:

  1. Add a migration and pre-condition to make sure these things don't exist
    • Migration makes cluster setting meaningless
  2. Block restores of such references
  3. Add gated validation that ensures that such references do not exist

@shermanCRL shermanCRL mentioned this issue Feb 9, 2023
Open
@rafiss rafiss mentioned this issue Mar 22, 2023
Merged
msirek pushed a commit to msirek/cockroach that referenced this issue Mar 23, 2023
randgen: disable generation of REGNAMESPACE type expressions
85b666e 
Casting an OID to REGNAMESPACE runs this SQL using the internal executor:
```
SELECT pg_namespace.oid, nspname FROM pg_catalog.pg_namespace WHERE oid = $1
```
When the `GenerateConstrainedScans` rule is disabled, this returns no
rows and so doesn't apply the cast. The same SQL, not run via an
internal executor, but also with `GenerateConstrainedScans` disabled,
behaves correctly. Disabling `GenerateConstrainedScans` all the time
prevents the cluster from starting.

The reason a full scan of pg_namespace doesn't find the OID is because it
only checks for schemas in the current database:
https://github.com/cockroachdb/cockroach/blob/7b341ffa678e4a22416e2274351fd56f415f5421/pkg/sql/virtual_schema.go#L602
https://github.com/cockroachdb/cockroach/blob/d10c3dd42c3dc40cad82792a30ae47fd2a663f43/pkg/sql/pg_catalog.go#L2099-L2106
https://github.com/cockroachdb/cockroach/blob/a7e9c4a68b81436d1f9382518d4267f74cbdac94/pkg/sql/information_schema.go#L2295-L2296

Whereas with a constrained scan, all descriptors are searched:
https://github.com/cockroachdb/cockroach/blob/af6a72a622ae05f3733b5db637403b3eaa9455f1/pkg/sql/catalog/descs/descriptor.go#L168-L175

The correct result is from the constrained scan, because we currently
allow cross-database references. Once cockroachdb#55791 is fixed, both results
should match.

The fix alternatives are:
1. disallow disabling of the `GenerateConstrainedScans` rule for internal SQL
2. turn off generation of REGNAMESPACE expressions in randgen to avoid hitting this problem in tests.

The 2nd fix alternative is implemented to avoid losing any of the
rule-disabling test coverage provided by the
`testing_optimizer_disable_rule_probability` setting.

Fixes cockroachdb#98322

Release note: None
craig bot pushed a commit that referenced this issue Apr 5, 2023
@xinhaoz @rickystewart
Merge #99168 #100652 #100760
530100f 
99168: randgen: disable generation of REGNAMESPACE type expressions r=msirek a=msirek

Casting an OID to REGNAMESPACE runs this SQL using the internal executor:
```
SELECT pg_namespace.oid, nspname FROM pg_catalog.pg_namespace WHERE oid = $1
```
When the `GenerateConstrainedScans` rule is disabled, this returns no
rows and so doesn't apply the cast. The same SQL, not run via an
internal executor, but also with `GenerateConstrainedScans` disabled,
behaves correctly. Disabling `GenerateConstrainedScans` all the time
prevents the cluster from starting.

The reason a full scan of pg_namespace doesn't find the OID is because it
only checks for schemas in the current database:
https://github.com/cockroachdb/cockroach/blob/7b341ffa678e4a22416e2274351fd56f415f5421/pkg/sql/virtual_schema.go#L602
https://github.com/cockroachdb/cockroach/blob/d10c3dd42c3dc40cad82792a30ae47fd2a663f43/pkg/sql/pg_catalog.go#L2099-L2106
https://github.com/cockroachdb/cockroach/blob/a7e9c4a68b81436d1f9382518d4267f74cbdac94/pkg/sql/information_schema.go#L2295-L2296

Whereas with a constrained scan, all descriptors are searched:
https://github.com/cockroachdb/cockroach/blob/af6a72a622ae05f3733b5db637403b3eaa9455f1/pkg/sql/catalog/descs/descriptor.go#L168-L175

The correct result is from the constrained scan, because we currently
allow cross-database references. Once #55791 is fixed, both results
should match.

The fix alternatives are:
1. disallow disabling of the `GenerateConstrainedScans` rule for internal SQL
2. turn off generation of REGNAMESPACE expressions in randgen to avoid hitting this problem in tests.

The 2nd fix alternative is implemented to avoid losing any of the
rule-disabling test coverage provided by the
`testing_optimizer_disable_rule_probability` setting.

Fixes #98322

100652: cluster-ui: fix cached data invalidation on timescale change r=xinhaoz a=xinhaoz

In a prior change, we moved the invalidation of cached data depending on the timescale to the local storage saga for CC. This was so invaldiation would occur after updating the cache. The local storage saga created for the time scale action was not hooked up to fire after the action, thus the data would not have been invalidated. This commit properly subscribes the saga to the update time scale action in CC.

Epic: none

Release note: None

100760: build: make sure toolchain has correct arguments for linking shared lib r=rail a=rickystewart

`geos` has been failing to build without this change since #100313. This fixes it.

Epic: none
Release note: None

Co-authored-by: Mark Sirek <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Apr 5, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Apr 5, 2023
randgen: disable generation of REGNAMESPACE type expressions
cebe846 
Casting an OID to REGNAMESPACE runs this SQL using the internal executor:
```
SELECT pg_namespace.oid, nspname FROM pg_catalog.pg_namespace WHERE oid = $1
```
When the `GenerateConstrainedScans` rule is disabled, this returns no
rows and so doesn't apply the cast. The same SQL, not run via an
internal executor, but also with `GenerateConstrainedScans` disabled,
behaves correctly. Disabling `GenerateConstrainedScans` all the time
prevents the cluster from starting.

The reason a full scan of pg_namespace doesn't find the OID is because it
only checks for schemas in the current database:
https://github.com/cockroachdb/cockroach/blob/7b341ffa678e4a22416e2274351fd56f415f5421/pkg/sql/virtual_schema.go#L602
https://github.com/cockroachdb/cockroach/blob/d10c3dd42c3dc40cad82792a30ae47fd2a663f43/pkg/sql/pg_catalog.go#L2099-L2106
https://github.com/cockroachdb/cockroach/blob/a7e9c4a68b81436d1f9382518d4267f74cbdac94/pkg/sql/information_schema.go#L2295-L2296

Whereas with a constrained scan, all descriptors are searched:
https://github.com/cockroachdb/cockroach/blob/af6a72a622ae05f3733b5db637403b3eaa9455f1/pkg/sql/catalog/descs/descriptor.go#L168-L175

The correct result is from the constrained scan, because we currently
allow cross-database references. Once #55791 is fixed, both results
should match.

The fix alternatives are:
1. disallow disabling of the `GenerateConstrainedScans` rule for internal SQL
2. turn off generation of REGNAMESPACE expressions in randgen to avoid hitting this problem in tests.

The 2nd fix alternative is implemented to avoid losing any of the
rule-disabling test coverage provided by the
`testing_optimizer_disable_rule_probability` setting.

Fixes #98322

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Apr 5, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Apr 5, 2023
randgen: disable generation of REGNAMESPACE type expressions
367b8dc 
Casting an OID to REGNAMESPACE runs this SQL using the internal executor:
```
SELECT pg_namespace.oid, nspname FROM pg_catalog.pg_namespace WHERE oid = $1
```
When the `GenerateConstrainedScans` rule is disabled, this returns no
rows and so doesn't apply the cast. The same SQL, not run via an
internal executor, but also with `GenerateConstrainedScans` disabled,
behaves correctly. Disabling `GenerateConstrainedScans` all the time
prevents the cluster from starting.

The reason a full scan of pg_namespace doesn't find the OID is because it
only checks for schemas in the current database:
https://github.com/cockroachdb/cockroach/blob/7b341ffa678e4a22416e2274351fd56f415f5421/pkg/sql/virtual_schema.go#L602
https://github.com/cockroachdb/cockroach/blob/d10c3dd42c3dc40cad82792a30ae47fd2a663f43/pkg/sql/pg_catalog.go#L2099-L2106
https://github.com/cockroachdb/cockroach/blob/a7e9c4a68b81436d1f9382518d4267f74cbdac94/pkg/sql/information_schema.go#L2295-L2296

Whereas with a constrained scan, all descriptors are searched:
https://github.com/cockroachdb/cockroach/blob/af6a72a622ae05f3733b5db637403b3eaa9455f1/pkg/sql/catalog/descs/descriptor.go#L168-L175

The correct result is from the constrained scan, because we currently
allow cross-database references. Once #55791 is fixed, both results
should match.

The fix alternatives are:
1. disallow disabling of the `GenerateConstrainedScans` rule for internal SQL
2. turn off generation of REGNAMESPACE expressions in randgen to avoid hitting this problem in tests.

The 2nd fix alternative is implemented to avoid losing any of the
rule-disabling test coverage provided by the
`testing_optimizer_disable_rule_probability` setting.

Fixes #98322

Release note: None
@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
Assignees
No one assigned
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@jlinder @ajwerner @postamar @thoszhang @mwang1026