sql: Internal sessions exempt from disallow_full_table_scans setting #137681
+73
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
rafiss
added
backport-23.2.x
rafiss
approved these changes
Dec 18, 2024
| @@ -440,6 +440,27 @@ CREATE INVERTED INDEX ON opclasses(c DESC) | |||
| statement ok | |||
| CREATE INVERTED INDEX ON opclasses(a DESC, c) | |||
|
|
|||
| # Regression test for GH issue #137404 | |||
There was a problem hiding this comment.
these tests are great!
we could add an additional one here, similar to what we have for statement_timeout:
cockroach/pkg/sql/opt/exec/execbuilder/testdata/execute_internally_builtin
Lines 219 to 238 in 8f01b63
There was a problem hiding this comment.
Good suggestion. Thanks.
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
force-pushed
the
gh-137404/241218/0757/disallow-full-ts-fix
branch
from
117a82b to
0024f4f
Compare
spilchen
commented
Dec 18, 2024
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rafiss)
| @@ -440,6 +440,27 @@ CREATE INVERTED INDEX ON opclasses(c DESC) | |||
| statement ok | |||
| CREATE INVERTED INDEX ON opclasses(a DESC, c) | |||
|
|
|||
| # Regression test for GH issue #137404 | |||
There was a problem hiding this comment.
Good suggestion. Thanks.
|
TFTR! bors r+ |
michae2
approved these changes
Dec 18, 2024
There was a problem hiding this comment.
LGTM, but I thought this check for executorType meant that internal executor was already exempt from disallow_full_table_scans?
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:
|
I didn't know about that exclusion. Since we have a simple test case, we should check with a debugger to see how it's possible to reach this condition for the verify-idx operation. |
This was referenced Dec 18, 2024
|
We never reach line 3195: cockroach/pkg/sql/conn_executor_exec.go Lines 3194 to 3195 in f26c138 The previous condition is false for CREATE INDEX. The only flags set are:
Should I revert this fix and work on getting the |
I'd prefer to leave to the SQL Queries team to decide how to tackle that (cc @michae2). In the meantime, I think the fix you've made here is a robust way of addressing the issue, and it uses the same pattern we have for statement_timeout so i don't think we need to change it. |
If we never reach line 3195, then how is |
|
Without this fix, CREATE INDEX fails at this point: cockroach/pkg/sql/opt/exec/execbuilder/relational.go Lines 604 to 605 in a2db312 It's driving a full index scan of the form Lines 2104 to 2111 in e6a5104 In cockroach/pkg/sql/conn_executor_exec.go Lines 3187 to 3195 in 503d914 I don't believe the statements in |
|
I see, thanks. I didn't realized we also checked disallow_full_table_scans in execbuild when the index is forced. And that check doesn't look at the executorType. I'm going to open a follow-up PR removing the executorType part, since this fix is cleaner and the executorType part doesn't work in execbuild anyway. |
michae2
added a commit
to michae2/cockroach
that referenced
this pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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 pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.