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: renaming tables into a different parent database allows creating prohibited cross-database references #55709

Closed
jayshrivastava opened this issue Oct 19, 2020 · 4 comments · Fixed by #61741
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@jayshrivastava
Copy link
Contributor

jayshrivastava commented Oct 19, 2020

Describe the problem

Even with sql.cross_db_fks.enabled=false, one can move a table to another database such that there is a cross-database foreign key relation.

To Reproduce

root@:26257/otherdb> show cluster setting sql.cross_db_fks.enabled;
  sql.cross_db_fks.enabled
----------------------------
           false
(1 row)

Time: 0ms total (execution 0ms / network 0ms)

root@:26257/defaultdb> create database otherdb;
CREATE DATABASE

Time: 81ms total (execution 81ms / network 0ms)

root@:26257/defaultdb> create table t1(i int PRIMARY KEY); create table t2(j int, constraint t1_reference foreign key(j) references t1(i));
CREATE TABLE

Note: timings for multiple statements on a single line are not supported. See https://go.crdb.dev/issue-v/48180/v20.2.

root@:26257/defaultdb> alter table t2 rename to otherdb.public.t2;
NOTICE: renaming tables with a qualification is deprecated
HINT: use ALTER TABLE t2 RENAME TO t2 instead
RENAME TABLE

Time: 296ms total (execution 93ms / network 203ms)

root@:26257/defaultdb> use otherdb;
SET

Time: 0ms total (execution 0ms / network 0ms)

root@:26257/otherdb> select * from information_schema.tables where table_name = 't2';
  table_catalog | table_schema | table_name | table_type | is_insertable_into | version
----------------+--------------+------------+------------+--------------------+----------
  otherdb       | public       | t2         | BASE TABLE | YES                |       4
(1 row)

Time: 5ms total (execution 4ms / network 0ms)

Expected behavior
This should only work if sql.cross_db_fks.enabled is set to true.

Environment:

  • CockroachDB version [e.g. 20.2]

Epic: CRDB-1519

@jayshrivastava jayshrivastava added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Oct 19, 2020
@thoszhang
Copy link
Contributor

thoszhang commented Oct 19, 2020

This is also broken for type references and sequence owner references (gated by sql.cross_db_sequence_owners.enabled in the latter case):

demo@127.0.0.1:52326/defaultdb> create type typ as enum('hello');
CREATE TYPE

Time: 4ms total (execution 4ms / network 0ms)

demo@127.0.0.1:52326/defaultdb> create table tbl(a typ);
CREATE TABLE

Time: 40ms total (execution 13ms / network 27ms)

demo@127.0.0.1:52326/defaultdb> insert into tbl values ('hello');
INSERT 1

Time: 5ms total (execution 5ms / network 0ms)

demo@127.0.0.1:52326/defaultdb> create database otherdb;
CREATE DATABASE

Time: 3ms total (execution 3ms / network 0ms)

demo@127.0.0.1:52326/defaultdb> alter table tbl rename to otherdb.tbl;
NOTICE: renaming tables with a qualification is deprecated
HINT: use ALTER TABLE tbl RENAME TO tbl instead
RENAME TABLE

Time: 45ms total (execution 6ms / network 38ms)
demo@127.0.0.1:52536/defaultdb> create table t(a int);
CREATE TABLE

Time: 3ms total (execution 3ms / network 0ms)

demo@127.0.0.1:52536/defaultdb> create sequence s owned by t.a;
CREATE SEQUENCE

Time: 28ms total (execution 7ms / network 22ms)

demo@127.0.0.1:52536/defaultdb> create database otherdb;
CREATE DATABASE

Time: 3ms total (execution 3ms / network 0ms)

demo@127.0.0.1:52536/defaultdb> alter table t rename to otherdb.t;
NOTICE: renaming tables with a qualification is deprecated
HINT: use ALTER TABLE t RENAME TO t instead
RENAME TABLE

Time: 30ms total (execution 6ms / network 24ms)

Fortunately, for now we don't allow renaming tables that are referred to by views.

I'm worried about cross-db type references the most, since they're supposed to be disallowed from the start, and now we'll have to account for them during whatever migration we have coming up.

@thoszhang thoszhang changed the title sql: cluster setting sql.cross_db_fks.enabled does not prevent cross-db foreign key relations sql: renaming tables into a different parent database allows creating prohibited cross-database references Oct 19, 2020
@thoszhang
Copy link
Contributor

Linking to #54126 as the original issue.

@jayshrivastava
Copy link
Contributor Author

jayshrivastava commented Oct 20, 2020

Linking to #55709

craig bot pushed a commit that referenced this issue Oct 20, 2020
55459: kv: increase defaultRaftLogTruncationThreshold to 16MB r=nvanbenschoten a=nvanbenschoten

In v20.1, we increased the default max range size from 64MB to 512MB. However, we only doubled (258b965) the default raft log truncation threshold. This has the potential to exacerbate issues like #37906, because each range will now handle on average 8 times more write load, and will therefore be forced to truncate its log on average 4 times as frequently as it had previously.

This commit bumps the default raft log truncation to 16MB. It doesn't go as far as scaling the log truncation threshold by the max range size (either automatically or with a fixed 32MB default) because the cost of an individual log truncation is proportional to the log size and we don't want to make each log truncation significantly more disruptive. 16MB seems like a good middle ground and we know that we have customers already successfully running in production with this value.

55660: sql: add constraint name to constraint error r=yuzefovich a=alex-berger

Add constraint name to error for unique constraint, check constraintand foreign key constraint violations. Those constraint names are then propagated over the PostgreSQL wire protocol and will show up for example in JDBC exceptions (org.postgresql.util.PSQLException#getServerErrorMessage().getConstraint()). This commit improves PostgreSQL compatibility.

Release note: Improve PosgreSQL wire protocol compatibility by adding constraint name to sql errors.

55759: roachtest: don't expect node deaths in tpccbench r=nvanbenschoten a=nvanbenschoten

Closes #55542.

This was causing node deaths to be initially ignored in test
failures like #55542. The monitor was not watching when the
cluster was restarted, so there was no need to inform it of
the restart. Because of this, the monitor had an accumulated
"death quota" that allowed it to ignore the first few deaths
during the actual run of the test.

I tested this 5 times without issue.

55779: sql: disallow moving tables with user-defined types into a different DB r=lucy-zhang a=lucy-zhang

We disallow columns using user-defined types in a different database
from the table at creation time. However, we have a loophole where it's
possible to move the table to a different database using `RENAME` and
thus create a cross-database reference. This breaks backup/restore and
is generally unintended. This PR adds a check to disallow this.

Partially addresses #55709.
Related to #55772.

Release note (bug fix): Tables can no longer be moved to a different
database using `RENAME` if they have columns using user-defined types
(enums).

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: alex.berger@nexiot.ch <alex.berger@nexiot.ch>
Co-authored-by: Lucy Zhang <lucy@cockroachlabs.com>
@ajwerner
Copy link
Contributor

ajwerner commented Mar 5, 2021

@fqazi this would be a good one for you to take a look at. It pairs nicely with #58867.

fqazi added a commit to fqazi/cockroach that referenced this issue Mar 10, 2021
Fixes: cockroachdb#55709

Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.

Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.

Release justification: Low risk fix to avoid users from accidentally,
using deprecated functionality.
fqazi added a commit to fqazi/cockroach that referenced this issue Mar 11, 2021
Fixes: cockroachdb#55709

Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.

Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.

Release justification: Low risk fix to avoid users from accidentally,
using deprecated functionality.
craig bot pushed a commit that referenced this issue Mar 15, 2021
61741: sql: disallow table/view/sequence rename from making cross DB references r=fqazi a=fqazi

Fixes: #55709

Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.

Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
@craig craig bot closed this as completed in ad4374a Mar 15, 2021
fqazi added a commit to fqazi/cockroach that referenced this issue Mar 22, 2021
Fixes: cockroachdb#55709

Previously, disallowed cross DB references could be created
using a ALTER TABLE/VIEW/SEQUENCE rename operations. This
was inadequate because users could accidentally start using
deprecated functionality. To address this, this patch detects
such scenarios are returns an appropriate error.

Release note (bug fix): ALTER TABLE/VIEW/SEQUENCE can no longer
be used to incorrectly create cross DB references.

Release justification: Low risk fix to avoid users from accidentally,
using deprecated functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants