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

Cluster setting sql.defaults.disallow_full_table_scans.enabled prevents index creation #137404

Closed
smcvey opened this issue Dec 13, 2024 · 2 comments · Fixed by #137681
Closed
Assignees
Labels
branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-1 Issues/test failures with a fix SLA of 1 month T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@smcvey
Copy link
Contributor

smcvey commented Dec 13, 2024

Describe the problem

If full table scans are disabled, it prevents the creation of new indexes on tables.

To Reproduce

SET CLUSTER SETTING sql.defaults.disallow_full_table_scans.enabled = true;
CREATE TABLE t (id UUID PRIMARY KEY);
CREATE INDEX ON t (id);

This produces the error message:

ERROR: verify-idx-count: index "t_id_idx" cannot be used for this query
SQLSTATE: 42809
HINT: try overriding the `disallow_full_table_scans` or increasing the `large_full_scan_rows` cluster/session settings

Despite the hint, overriding either of these variables in the session has no effect. The cluster setting needs overridden, which can't be done by an ordinary user.

Expected behavior
The setting is intended to prevent users/applications from accidentally running expensive full table scans. However, it should not prevent the creation of an index which requires, by definition, a full table scan.

Environment:

  • CockroachDB version: master

Jira issue: CRDB-45564

https://cockroachdb.zendesk.com/agent/tickets/24794

@smcvey smcvey added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Dec 13, 2024
Copy link

blathers-crl bot commented Dec 13, 2024

Hi @smcvey, please add branch-* labels to identify which branch(es) this C-bug affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@smcvey smcvey added the O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs label Dec 13, 2024
@Dedej-Bergin Dedej-Bergin added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Dec 13, 2024
@exalate-issue-sync exalate-issue-sync bot added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Dec 13, 2024
@rafiss
Copy link
Collaborator

rafiss commented Dec 16, 2024

Let's add an exception for disallow_full_table_scans similar to how we did for statement_timeout in 8f01b63. It's also a good time to check for any other timeouts or blocks that should not apply to non-session-bound internal executor jobs.

@exalate-issue-sync exalate-issue-sync bot added the P-1 Issues/test failures with a fix SLA of 1 month label Dec 17, 2024
spilchen added a commit to spilchen/cockroach that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: cockroachdb#137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
spilchen added a commit to spilchen/cockroach that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: cockroachdb#137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
spilchen added a commit to spilchen/cockroach that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: cockroachdb#137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
@rimadeodhar rimadeodhar added branch-master Failures and bugs on the master branch. branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 labels Dec 18, 2024
craig bot pushed a commit that referenced this issue Dec 18, 2024
129706: opt: type check VALUES rows after building them r=DrewKimball a=DrewKimball

The type of a RECORD-returning UDF is not fully resolved until after it is fully built within the optbuilder. Therefore, a VALUES expression with a UDF can only determine its column types after building its rows. We accounted for this in #103078, but that fix was incomplete - building the row resolves the type of the UDF, but does not update the type annotation for any expressions that wrap it (for example, a COALESCE).

This commit fully fixes the type resolution bug by type checking the row once again after it is built. This ensures that the UDF's resolved type is correctly propagated to parent expressions.

Fixes #117101

Release note (bug fix): Fixed a bug that would cause an internal error when the result of a RECORD-returning UDF was wrapped by another expression (such as COALESCE) within a VALUES clause.

137681: sql: Internal sessions exempt from disallow_full_table_scans setting r=spilchen a=spilchen

Previously, enabling the cluster setting sql.defaults.disallow_full_table_scans.enabled prevented the creation of indexes due to the internal table scans required during the process. This change ensures that the setting is bypassed for internal operations, while still applying to user queries.

Epic: None
Closes: #137404
Release note (bug fix): Internal scans are now exempt from the sql.defaults.disallow_full_table_scans.enabled setting, allowing index creation even when the cluster setting is active.

Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>
@craig craig bot closed this as completed in 0024f4f Dec 18, 2024
blathers-crl bot pushed a commit that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: #137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
blathers-crl bot pushed a commit that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: #137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
blathers-crl bot pushed a commit that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: #137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
blathers-crl bot pushed a commit that referenced this issue Dec 18, 2024
Previously, enabling the cluster setting
sql.defaults.disallow_full_table_scans.enabled prevented the creation of
indexes due to the internal table scans required during the process.
This change ensures that the setting is bypassed for internal
operations, while still applying to user queries.

Epic: None
Closes: #137404
Release note (bug fix): Internal scans are now exempt from the
sql.defaults.disallow_full_table_scans.enabled setting, allowing index
creation even when the cluster setting is active.
michae2 added a commit to michae2/cockroach that referenced this issue Dec 23, 2024
Now that cockroachdb#137681 explicitly turns off `disallow_full_table_scans` for
the internal executor, we no longer need to look at executorType in the
connExecutor check. This makes the connExecutor check match the
execBuilder check added in cockroachdb#71317.

Informs: cockroachdb#137404

Release note: None
craig bot pushed a commit that referenced this issue Dec 23, 2024
137949: sql: remove executorType condition from disallow_full_table_scans check r=yuzefovich,mgartner a=michae2

Now that #137681 explicitly turns off `disallow_full_table_scans` for the internal executor, we no longer need to look at executorType in the connExecutor check. This makes the connExecutor check match the execBuilder check added in #71317.

Informs: #137404

Release note: None

Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
michae2 added a commit to michae2/cockroach that referenced this issue Dec 24, 2024
We have various checks for disallow_full_table_scans:
1. in connExecutor we fail the statement with an error if it uses a full
   scan
2. in execbuild we fail the statement with an error if there is an index
   hint that causes a full scan
3. in the coster we add hugeCost to full scans, to try and avoid them

We don't want disallow_full_table_scans to apply to anything done by
internal executors. (1) was already checking whether this connExecutor
was internal. (2) and (3) were not, which led to cockroachdb#137404. cockroachdb#137681 fixed
this by setting disallow_full_table_scans in `NewInternalSessionData`,
but we only use `NewInternalSessionData` for some uses of the internal
executor.

This commit explicitly checks `SessionData().Internal` in (2) and (3) to
match (1), so that we don't get any of the disallow_full_table_scans
behavior for any use of the internal executor, including populating
virtual tables.

This commit also adds `SessionData().Internal` to the memo staleness
check.

This behavior can be observed with the following diff, which adds an
index hint to the `crdb_internal.system_jobs` virtual table SQL:

```
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
index 80a5342b56a..99ccb8f1611 100644
--- a/pkg/sql/crdb_internal.go
+++ b/pkg/sql/crdb_internal.go
@@ -975,7 +975,7 @@ SELECT
 DISTINCT(id), status, created, payload.value AS payload, progress.value AS progress,
 created_by_type, created_by_id, claim_session_id, claim_instance_id, num_runs, last_run, job_type
 FROM
-system.jobs AS j
+system.jobs@jobs_status_created_idx AS j
 LEFT JOIN system.job_info AS progress ON j.id = progress.job_id AND progress.info_key = 'legacy_progress'
 INNER JOIN system.job_info AS payload ON j.id = payload.job_id AND payload.info_key = 'legacy_payload'
 `
```

With this diff, the following SQL hits (2) and fails with an error, even
though it uses the internal executor and thus should not error:

```
SET disallow_full_table_scans = on;
SELECT * FROM crdb_internal.jobs;
```

With this commit we no longer fail, matching the behavior without the
index hint.

Informs: cockroachdb#137404
Informs: cockroachdb#123783

Release note: None
michae2 added a commit to michae2/cockroach that referenced this issue Dec 28, 2024
We have various checks for disallow_full_table_scans:
1. in connExecutor we fail the statement with an error if it uses a full
   scan
2. in execbuild we fail the statement with an error if there is an index
   hint that causes a full scan
3. in the coster we add hugeCost to full scans, to try and avoid them

We don't want disallow_full_table_scans to apply to anything done by
internal executors. (1) was already checking whether this connExecutor
was internal. (2) and (3) were not, which led to cockroachdb#137404. cockroachdb#137681 fixed
this by setting disallow_full_table_scans in `NewInternalSessionData`,
but we only use `NewInternalSessionData` for some uses of the internal
executor.

This commit explicitly checks `SessionData().Internal` in (2) and (3) to
match (1), so that we don't get any of the disallow_full_table_scans
behavior for any use of the internal executor, including populating
virtual tables.

This commit also adds `SessionData().Internal` to the memo staleness
check.

This behavior can be observed with the following diff, which adds an
index hint to the `crdb_internal.system_jobs` virtual table SQL:

```
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
index 80a5342b56a..99ccb8f1611 100644
--- a/pkg/sql/crdb_internal.go
+++ b/pkg/sql/crdb_internal.go
@@ -975,7 +975,7 @@ SELECT
 DISTINCT(id), status, created, payload.value AS payload, progress.value AS progress,
 created_by_type, created_by_id, claim_session_id, claim_instance_id, num_runs, last_run, job_type
 FROM
-system.jobs AS j
+system.jobs@jobs_status_created_idx AS j
 LEFT JOIN system.job_info AS progress ON j.id = progress.job_id AND progress.info_key = 'legacy_progress'
 INNER JOIN system.job_info AS payload ON j.id = payload.job_id AND payload.info_key = 'legacy_payload'
 `
```

With this diff, the following SQL hits (2) and fails with an error, even
though it uses the internal executor and thus should not error:

```
SET disallow_full_table_scans = on;
SELECT * FROM crdb_internal.jobs;
```

With this commit we no longer fail, matching the behavior without the
index hint.

Informs: cockroachdb#137404
Informs: cockroachdb#123783

Release note: None
michae2 added a commit to michae2/cockroach that referenced this issue Jan 2, 2025
We have various checks for disallow_full_table_scans:
1. in connExecutor we fail the statement with an error if it uses a full
   scan
2. in execbuild we fail the statement with an error if there is an index
   hint that causes a full scan
3. in the coster we add hugeCost to full scans, to try and avoid them

We don't want disallow_full_table_scans to apply to anything done by
internal executors. (1) was already checking whether this connExecutor
was internal. (2) and (3) were not, which led to cockroachdb#137404. cockroachdb#137681 fixed
this by setting disallow_full_table_scans in `NewInternalSessionData`,
but we only use `NewInternalSessionData` for some uses of the internal
executor.

This commit explicitly checks `SessionData().Internal` in (2) and (3) to
match (1), so that we don't get any of the disallow_full_table_scans
behavior for any use of the internal executor, including populating
virtual tables.

This commit also adds `SessionData().Internal` to the memo staleness
check.

This behavior can be observed with the following diff, which adds an
index hint to the `crdb_internal.system_jobs` virtual table SQL:

```
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
index 80a5342b56a..99ccb8f1611 100644
--- a/pkg/sql/crdb_internal.go
+++ b/pkg/sql/crdb_internal.go
@@ -975,7 +975,7 @@ SELECT
 DISTINCT(id), status, created, payload.value AS payload, progress.value AS progress,
 created_by_type, created_by_id, claim_session_id, claim_instance_id, num_runs, last_run, job_type
 FROM
-system.jobs AS j
+system.jobs@jobs_status_created_idx AS j
 LEFT JOIN system.job_info AS progress ON j.id = progress.job_id AND progress.info_key = 'legacy_progress'
 INNER JOIN system.job_info AS payload ON j.id = payload.job_id AND payload.info_key = 'legacy_payload'
 `
```

With this diff, the following SQL hits (2) and fails with an error, even
though it uses the internal executor and thus should not error:

```
SET disallow_full_table_scans = on;
SELECT * FROM crdb_internal.jobs;
```

With this commit we no longer fail, matching the behavior without the
index hint.

Informs: cockroachdb#137404
Informs: cockroachdb#123783

Release note: None
michae2 added a commit to michae2/cockroach that referenced this issue Jan 2, 2025
We have various checks for disallow_full_table_scans:
1. in connExecutor we fail the statement with an error if it uses a full
   scan
2. in execbuild we fail the statement with an error if there is an index
   hint that causes a full scan
3. in the coster we add hugeCost to full scans, to try and avoid them

We don't want disallow_full_table_scans to apply to anything done by
internal executors. (1) was already checking whether this connExecutor
was internal. (2) and (3) were not, which led to cockroachdb#137404. cockroachdb#137681 fixed
this by setting disallow_full_table_scans in `NewInternalSessionData`,
but we only use `NewInternalSessionData` for some uses of the internal
executor.

This commit explicitly checks `SessionData().Internal` in (2) and (3) to
match (1), so that we don't get any of the disallow_full_table_scans
behavior for any use of the internal executor, including populating
virtual tables.

This commit also adds `SessionData().Internal` to the memo staleness
check.

This behavior can be observed with the following diff, which adds an
index hint to the `crdb_internal.system_jobs` virtual table SQL:

```
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
index 80a5342b56a..99ccb8f1611 100644
--- a/pkg/sql/crdb_internal.go
+++ b/pkg/sql/crdb_internal.go
@@ -975,7 +975,7 @@ SELECT
 DISTINCT(id), status, created, payload.value AS payload, progress.value AS progress,
 created_by_type, created_by_id, claim_session_id, claim_instance_id, num_runs, last_run, job_type
 FROM
-system.jobs AS j
+system.jobs@jobs_status_created_idx AS j
 LEFT JOIN system.job_info AS progress ON j.id = progress.job_id AND progress.info_key = 'legacy_progress'
 INNER JOIN system.job_info AS payload ON j.id = payload.job_id AND payload.info_key = 'legacy_payload'
 `
```

With this diff, the following SQL hits (2) and fails with an error, even
though it uses the internal executor and thus should not error:

```
SET disallow_full_table_scans = on;
SELECT * FROM crdb_internal.jobs;
```

With this commit we no longer fail, matching the behavior without the
index hint.

Informs: cockroachdb#137404
Informs: cockroachdb#123783

Release note: None
craig bot pushed a commit that referenced this issue Jan 3, 2025
137961: opt: don't apply disallow_full_table_scans to internal executors r=yuzefovich a=michae2

We have various checks for disallow_full_table_scans:
1. in connExecutor we fail the statement with an error if it uses a full scan
2. in execbuild we fail the statement with an error if there is an index hint that causes a full scan
3. in the coster we add hugeCost to full scans, to try and avoid them

We don't want disallow_full_table_scans to apply to anything done by internal executors. (1) was already checking whether this connExecutor was internal. (2) and (3) were not, which led to #137404. #137681 fixed this by setting disallow_full_table_scans in `NewInternalSessionData`, but we only use `NewInternalSessionData` for some uses of the internal executor.

This commit explicitly checks `SessionData().Internal` in (2) and (3) to match (1), so that we don't get any of the disallow_full_table_scans behavior for any use of the internal executor, including populating virtual tables.

This commit also adds `SessionData().Internal` to the memo staleness check.

This behavior can be observed with the following diff, which adds an index hint to the `crdb_internal.system_jobs` virtual table SQL:

```
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
index 80a5342b56a..99ccb8f1611 100644
--- a/pkg/sql/crdb_internal.go
+++ b/pkg/sql/crdb_internal.go
`@@` -975,7 +975,7 `@@` SELECT
 DISTINCT(id), status, created, payload.value AS payload, progress.value AS progress,
 created_by_type, created_by_id, claim_session_id, claim_instance_id, num_runs, last_run, job_type
 FROM
-system.jobs AS j
+system.jobs@jobs_status_created_idx AS j
 LEFT JOIN system.job_info AS progress ON j.id = progress.job_id AND progress.info_key = 'legacy_progress'
 INNER JOIN system.job_info AS payload ON j.id = payload.job_id AND payload.info_key = 'legacy_payload'
 `
```

With this diff, the following SQL hits (2) and fails with an error, even though it uses the internal executor and thus should not error:

```
SET disallow_full_table_scans = on;
SELECT * FROM crdb_internal.jobs;
```

With this commit we no longer fail, matching the behavior without the index hint.

Informs: #137404
Informs: #123783

Release note: None

Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-1 Issues/test failures with a fix SLA of 1 month T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants