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: add setting to gate cross-db reference, default to disallow #54126

Closed
ajwerner opened this issue Sep 9, 2020 · 7 comments
Closed

sql: add setting to gate cross-db reference, default to disallow #54126

ajwerner opened this issue Sep 9, 2020 · 7 comments
Assignees
@RaduBerinde
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Sep 9, 2020 •
edited
Loading

Is your feature request related to a problem? Please describe.

As we move towards postgres compliance, we want to encourage the use of user-defined schemas to arrange the relations of an application. In postgres there are no cross database references or queries. In cockroach, prior to 20.2, databases and schemas were functionally equivalent. We've reached a general consensus that we'd like to move towards a world where cross-database references are not allowed. This will simplify integrity checking and backup/restore. Cross database references have also been a source of bugs and odd behavior (#51090).

Describe the solution you'd like
To move towards strict deprecation in 21.1, we'll need to announce our intention and provide some push for users to change their behavior in 20.2. The idea is that we'll create a cluster setting to disallow creating new cross-database by default and gate it behind a cluster or setting setting.

Note that databases without user-defined schema can be re-parented as schemas in other databases.

Additional context

In the 21.1 we'd like to provide a mechanism to forcibly prevent updating to 21.2 with any cross database references. If that's a goal, we need to socialize the idea early.
@blathers-crl blathers-crl bot added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Sep 9, 2020
@ajwerner
Copy link
Contributor Author

ajwerner commented Sep 9, 2020

cc @vy-ton

@thoszhang thoszhang added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. branch-release-20.2 labels Sep 15, 2020
@cockroachdb cockroachdb deleted a comment from blathers-crl bot Sep 15, 2020
@RaduBerinde RaduBerinde mentioned this issue Sep 15, 2020
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 15, 2020
@RaduBerinde
sql: disallow cross-database foreign keys
94e1754 
This change disallows cross-database foreign key references and adds
a cluster setting to allow them (false by default).

Informs cockroachdb#54126.

Release note (sql change): creating cross-database foreign key
references is now disallowed (and can be re-enabled via a cluster
setting).
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 15, 2020
@RaduBerinde
sql: disallow cross-database foreign keys
2ea6868 
This change disallows cross-database foreign key references and adds
a cluster setting to allow them (false by default).

Informs cockroachdb#54126.

Release note (sql change): creating cross-database foreign key
references is now disallowed (and can be re-enabled via a cluster
setting).
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 15, 2020
@RaduBerinde
sql: disallow cross-database foreign keys
65d957e 
This change disallows cross-database foreign key references and adds
a cluster setting to allow them (false by default).

Informs cockroachdb#54126.

Release note (sql change): creating cross-database foreign key
references is now disallowed (and can be re-enabled via a cluster
setting).
craig bot pushed a commit that referenced this issue Sep 16, 2020
@RaduBerinde
Merge #54407
68ff6b1 
54407: sql: disallow cross-database foreign keys r=RaduBerinde a=RaduBerinde

This change disallows cross-database foreign key references and adds
a cluster setting to allow them (false by default).

Informs #54126.

Release note (sql change): creating cross-database foreign key
references is now disallowed (and can be re-enabled via a cluster
setting).

Co-authored-by: Radu Berinde <[email protected]>
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 16, 2020
@RaduBerinde
sql: disallow cross-database views
bc77011 
This change disallows views from referring to tables in other
databases (except `system`) and adds a cluster setting to allow them
(false by default).

Informs cockroachdb#54126.

Release note (sql change): creating views that refer to tables in
other databases is now disallowed (and can be re-enabled via a cluster
setting).
@RaduBerinde RaduBerinde mentioned this issue Sep 16, 2020
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 17, 2020
@RaduBerinde
sql: disallow cross-database views
a4d19fd 
This change disallows views from referring to tables in other
databases (except `system`) and adds a cluster setting to allow them
(false by default).

Informs cockroachdb#54126.

Release note (sql change): creating views that refer to tables in
other databases is now disallowed (and can be re-enabled via a cluster
setting).
craig bot pushed a commit that referenced this issue Sep 17, 2020
@arulajmani @RaduBerinde
@mgartner @otan
Merge #54188 #54475 #54480 #54488
6bf81ee 
54188: sql: add framework for testing only descriptor validation r=otan a=arulajmani

This patch adds a new field on the `ExecutorTestingKnobs` called
`TestingDescriptorValidation`. This creates the structure to perform
certain table descriptor validation only during testing scenarios, i.e,
when this knob is turned on. This is achieved by intercepting the
context at the `conn_executor` level to include a value that indicates
to the validation functions if extra validation should be performed.
By intercepting the context at this level, instead of somewhere closer
to the Validate calls, we avoid having to find and intercept the context
in all possible codepaths -- which was getting quite messy.

On the validation side, this patch adds two to be filled functions --
validateCrossReferencesIfTesting and validateTableIfTesting. We don't
validate cross references for mutable descriptors, so providing these
two functions allows us to preserve this pattern and cover both
mutable/immutable descriptors in testing only validation. The functions
are currently no-ops and will be filled in in upcoming patches.

Currently, the testing knob is turned on by default for all test
servers. If you're writing a test and don't want to opt into this
validation for some reason, there's a
`DisableTestingDescriptorValidation` on server args that can be set.

Release note: None

54475: sql: disallow cross-database views r=RaduBerinde a=RaduBerinde

This change disallows views from referring to tables in other
databases (except `system`) and adds a cluster setting to allow them
(false by default).

Informs #54126.

Release note (sql change): creating views that refer to tables in
other databases is now disallowed (and can be re-enabled via a cluster
setting).

54480: sql: re-enable partial index enum test r=mgartner a=mgartner

This commit re-enables a test that was previously commented-out because
of issue #52539. The PR #54434 fixed the issue, so now this test passes.

Release note: None

54488: pgwire: cleanup notice sending usage r=knz a=otan

* Replace the `flushBeforeCloseFuncs` closure with the raw data types that
  need to be flushed.
* Rename SendClientNotice to BufferClientNotice to clear up semantics.
* Rename AppendNotice to BufferNotice.

This was desired in #49190.

Release note: None



Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 17, 2020
@RaduBerinde
sql: disallow cross-database foreign keys
121abbe 
This change disallows cross-database foreign key references and adds
a cluster setting to allow them (false by default).

Informs cockroachdb#54126.

Release note (sql change): creating cross-database foreign key
references is now disallowed (and can be re-enabled via a cluster
setting).
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 17, 2020
@RaduBerinde
sql: disallow cross-database views
b8cace1 
This change disallows views from referring to tables in other
databases (except `system`) and adds a cluster setting to allow them
(false by default).

Informs cockroachdb#54126.

Release note (sql change): creating views that refer to tables in
other databases is now disallowed (and can be re-enabled via a cluster
setting).
@RaduBerinde RaduBerinde mentioned this issue Sep 17, 2020
Merged
@ajwerner
Copy link
Contributor Author

Maybe on your list but we should add sequence ownership to the set of things being disallowed.

@RaduBerinde
Copy link
Member

Cross-database foreign keys and views that refer to other databases (except system.) are now disallowed by default in 20.2.

@RaduBerinde
Copy link
Member

Maybe on your list but we should add sequence ownership to the set of things being disallowed.

Oh, just noticed this. I'll try to get that in as well.

@RaduBerinde RaduBerinde reopened this Sep 17, 2020
@RaduBerinde
Copy link
Member

Actually, that is only relevant for views, correct? It already works through the same mechanism (the sequence is a view dependency). I will add a test for that.

@RaduBerinde
Copy link
Member

Oh, no, you are talking about OWNED BY I believe, I'll look at that.

@RaduBerinde RaduBerinde mentioned this issue Sep 17, 2020
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 18, 2020
@RaduBerinde
sql: don't allow cross-database sequence owners
3f99be8 
A sequence can be "owned" by a column. This change disallows setting
owners from tables in other databases and adds a cluster setting to
allow them (false by default).

Informs cockroachdb#54126.

Release note (sql change): creating sequences that are OWNED BY
columns in tables in other databases is now disallowed (and can be
re-enabled via a cluster setting).
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Sep 18, 2020
@RaduBerinde
sql: don't allow cross-database sequence owners
daa60bc 
A sequence can be "owned" by a column. This change disallows setting
owners from tables in other databases and adds a cluster setting to
allow them (false by default).

Informs cockroachdb#54126.

Release note (sql change): creating sequences that are OWNED BY
columns in tables in other databases is now disallowed (and can be
re-enabled via a cluster setting).
@RaduBerinde RaduBerinde mentioned this issue Sep 18, 2020
Merged
craig bot pushed a commit that referenced this issue Sep 18, 2020
@adityamaru @mgartner
@otan @RaduBerinde
Merge #54450 #54517 #54519 #54531
84d37a7 
54450: userfile: add userfile telemetry r=pbardea a=adityamaru

Adds telemetry to userfile upload, delete, list, and counts how many
times userfile is used as the import source.

Informs: #53431

Release note: None

54517: sql: backfill partial inverted indexes r=rytaft a=mgartner

This commit adds support for backfilling partial inverted indexes.
After the backfill is complete, the number of entries in the index are
verified to be correct.

Release note: None

54519: sql: implement CREATE EXTENSION syntax r=solongordon a=otan

Refs #51424, #51137 

Release note (sql change): Add support for the CREATE EXTENSION syntax.
This no-ops for PostGIS, and gives unimplemented errors for extensions
we do not yet support but may plan to.

54531: sql: don't allow cross-database sequence owners r=RaduBerinde a=RaduBerinde

#### sql: add more tests for cross-db views

Follow-up to #54475, a few more tests around views:
 - reference to sequence from another database
 - CREATE OR REPLACE

Release note: None

#### sql: don't allow cross-database sequence owners

A sequence can be "owned" by a column. This change disallows setting
owners from tables in other databases and adds a cluster setting to
allow them (false by default).

Informs #54126.

Release note (sql change): creating sequences that are OWNED BY
columns in tables in other databases is now disallowed (and can be
re-enabled via a cluster setting).


Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@RaduBerinde
Copy link
Member

Cross-database foreign keys, views that refer to other databases (except system), and cross-database sequence owners are now disallowed by default in 20.2.

This was referenced Oct 19, 2020
Closed
Open
@ajwerner ajwerner mentioned this issue Sep 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RaduBerinde RaduBerinde

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jordanlewis @ajwerner @thoszhang @RaduBerinde