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

QA: SELECT from view with circular reference crashes node #98999

Closed
msirek opened this issue Mar 19, 2023 · 3 comments · Fixed by #99174
Closed

QA: SELECT from view with circular reference crashes node #98999

msirek opened this issue Mar 19, 2023 · 3 comments · Fixed by #99174
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-qa S-1 High impact: many users impacted, serious risk of high unavailability or data loss T-sql-queries SQL Queries Team

Comments

@msirek
Copy link
Contributor

msirek commented Mar 19, 2023

From star expressions in view definitions QA,

Creating a view with a circular reference to itself causes a stack overflow and crashes the node:

CREATE TABLE films (id int PRIMARY KEY, title text, kind text, classification CHAR(1), year int, language text, studio text, runtime float, country_code CHAR(2));
CREATE VIEW comedies AS
    SELECT *
    FROM films
    WHERE kind = 'Comedy';
CREATE VIEW pg_comedies AS
    SELECT *
    FROM comedies
    WHERE classification = 'PG';
CREATE OR REPLACE VIEW comedies AS
    SELECT *
    FROM pg_comedies;

EXPLAIN SELECT * FROM pg_comedies;
ERROR: unexpected EOF
warning: error retrieving the transaction status: connection closed unexpectedly: conn closed
warning: connection lost!
opening new connection: all session settings will be lost
warning: error retrieving the database name: failed to connect to `host=localhost user=root database=`: dial error (dial tcp 127.0.0.1:26257: connect: connection refused)

I230319 21:26:41.260793 1 util/log/flags.go:211  [-] 1  stderr capture started
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc02e3ba388 stack=[0xc02e3ba000, 0xc04e3ba000]
fatal error: stack overflow

runtime stack:
runtime.throw({0x59e7120?, 0x9f342a0?})
        GOROOT/src/runtime/panic.go:1047 +0x5d fp=0x7f966a94c278 sp=0x7f966a94c248 pc=0x48f7dd
runtime.newstack()
        GOROOT/src/runtime/stack.go:1103 +0x5cc fp=0x7f966a94c430 sp=0x7f966a94c278 pc=0x4a9ccc
runtime.morestack()
        GOROOT/src/runtime/asm_amd64.s:570 +0x8b fp=0x7f966a94c438 sp=0x7f966a94c430 pc=0x4c204b

In Postgres, the above returns an error:

ERROR:  infinite recursion detected in rules for relation "pg_comedies"

Jira issue: CRDB-25645

@msirek msirek added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-qa labels Mar 19, 2023
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Mar 19, 2023
@rharding6373
Copy link
Collaborator

Great find. This does not appear to be a * specific bug, as I've been able to repro Mark's example using column names instead of *. In UDFs we disallow UDFs to call other UDFs because of the circular dependency problem. I don't think we can disable the same for views though since it may break user workloads.

@rharding6373 rharding6373 self-assigned this Mar 21, 2023
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 21, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies.

Fixes: cockroachdb#98999
Epic: none

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "infinite recursion detected in rules for
relation" instead of crashing the node.
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 22, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "infinite recursion detected in rules for
relation" instead of crashing the node.
@chengxiong-ruan
Copy link
Contributor

This would worth a gentle TA or at least let customers know. Maybe not worth a TA because I think it only crash the gateway node and a restart of the node would recover things.

rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 26, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "infinite recursion detected in rules for
relation" instead of crashing the node.
@mgartner mgartner added the S-1 High impact: many users impacted, serious risk of high unavailability or data loss label Mar 27, 2023
@rharding6373
Copy link
Collaborator

Discussed the prospect of a TA with the SQl Queries team, and our conclusion is that we don't need one, since as you say it is recoverable, and since it only manifests with a bad schema design users with circular dependencies would have found them quickly (and can fix the issue by correcting the schema).

rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 27, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "cyclic view dependency for
relation" instead of crashing the node. This bug is present since at
least 21.1.
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 27, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "cyclic view dependency for
relation" instead of crashing the node. This bug is present since at
least 21.1.
craig bot pushed a commit that referenced this issue Mar 30, 2023
99174: sql: fix circular dependencies in views r=rharding6373 a=rharding6373

This change fixes node crashes that could happen due to stack overflow if views were created with circular dependencies.

Fixes: #98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies, CRDB returns the error "cyclic view dependency for relation" instead of crashing the node. This bug is present since at least 21.1.

99624: testutils: add infrastructure for reusable test fixtures r=RaduBerinde a=RaduBerinde

Certain storage-related tests/benchmarks generate fixtures and attempt to reuse them across invocations. This is important because fixtures can be large and slow to generate; but more importantly the generation is non-deterministic and we want to use the exact same fixture when comparing benchmark data.

Currently the tests achieve this by using a `.gitignore`d subdirectory inside the source tree. This does not work with bazel (which executes the test in a sandbox).

This commit addresses the issue by adding new test infrastructure for reusable fixtures. We use the user's `.cache` directory instead of the source tree (specifically $HOME/.cache/crdb-test-fixtures/...). For bazel, we make sure the path is available (and writable) in the sandbox and we pass the path to the test through an env var.

Fixes #83599.

Release note: None
Epic: none

99699: spanconfig: integrate SpanConfigBounds with the Store and KVSubscriber r=ajwerner a=arulajmani

This patch integrates `SpanConfigBounds` with the `KVSubscriber` and
`spanconfig.Store`. The `spanconfig.Store` used by the `KVSubscriber`
now has a handle to the global tenant capability state. It uses this to
clamp any secondary tenant span configs that are not in conformance
before returning them.

By default, clamping of secondary tenant span configurations is turned
off. It can be enabled using the `spanconfig.bounds.enabled` cluster
setting. The setting is hidden.

Fixes: #99689
Informs #99911

Release note: None

99759: roachtest: set lower min range_max_bytes in multitenant_distsql test r=rharding6373 a=rharding6373

The multitenant_distsql roachtest relies on a smaller range size than the new default min of range_max_bytes (64MiB) due to performance and resource limitations. We set the COCKROACH_MIN_RANGE_MAX_BYTES to allow the test to use the smaller range.

Fixes: #99626
Epic: None

Release note: None

99813: spanconfig: do not fatal in NeedsSplit and ComputeSplitKey r=irfansharif a=arulajmani

See individual commits for details. 

Fixes #97336.

99839: schemachanger: Annotate all tables if ALTER TABLE IF EXISTS on non-existent table r=Xiang-Gu a=Xiang-Gu

Previously, if table `t` does not exist, `ALTER TABLE IF EXISTS t` will only mark `t` as non-existent. This is inadequate because for stmt like `ALTER TABLE IF EXISTS t ADD FOREIGN KEY REFERENCES t_other` we will not touch `t_other` and the validation logic will later complain that `t_other` is not fully resolved nor marked as non-existent. This commit fixes it by marking all tables in this ALTER TABLE stmt as non-existent if the `t` is non-existent, so we can pass the validation.

Fixes issues discovered in #99185
Epic: None

99865: roachtest: fix query used to get job status in backup/mixed-version r=srosenberg a=renatolabs

At the moment, we only query job status in mixed-version state (so we should always use `system.jobs`). However, the code added in this commit should continue to work once we start developing 23.2, as we're checking that the cluster version is at least 23.1 before using `crdb_internal.system_jobs`.

Epic: none

Release note: None

99878: jobs: change job_info.info_key to string r=dt a=dt

Release note: none.
Epic: none.

Hopefully we get this one in now before it is released and harder to change later. I think if we go with bytes, we'll spend the next several years typing convert_to over and over, or forgetting to and then typing it, when debugging.

Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: David Taylor <[email protected]>
@craig craig bot closed this as completed in ee16008 Mar 30, 2023
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 30, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "cyclic view dependency for
relation" instead of crashing the node. This bug is present since at
least 21.1.

Release justification: Fixes a bug that can cause the gateway node to
crash if there are self-referencing views.
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 30, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "cyclic view dependency for
relation" instead of crashing the node. This bug is present since at
least 21.1.

Release justification: Fixes a bug that can cause the gateway node to
crash if there are self-referencing views.
rharding6373 added a commit to rharding6373/cockroach that referenced this issue Mar 30, 2023
This change fixes node crashes that could happen due to stack overflow
if views were created with circular dependencies. It also fixes a few
places in the schema changer where there were problems dropping views
with circular dependencies.

Fixes: cockroachdb#98999
Epic: none
Co-authored-by: [email protected]

Release note (bug fix): If views are created with circular dependencies,
CRDB returns the error "cyclic view dependency for
relation" instead of crashing the node. This bug is present since at
least 21.1.

Release justification: Fixes a bug that can cause the gateway node to
crash if there are self-referencing views.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
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. O-qa S-1 High impact: many users impacted, serious risk of high unavailability or data loss T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants