Skip to content

Commit

Permalink
Merge #123780
Browse files Browse the repository at this point in the history
123780: opt: don't apply huge cost for full scans of virtual tables r=yuzefovich a=yuzefovich

This commit fixes - what I believe is - an oversight where we applied the huge cost to the plans with full scans of virtual tables when `disallow_full_table_scans` variable is set. The intent behind this penalty is to make plans with full scans prohibitively expensive so that other plans (without full scans) would be preferred, and if there is no other plan, then the query would error out. Implementation of this approach was incomplete for virtual tables: namely, we'd apply the cost penalty, but then we would not actually error out the query. Additionally, given that we don't have stats on virtual tables, we would apply the cost penalty to all full scans of virtual tables.

As a result, we would either pick a plan without the full scan (that might actually be worse) or would allow the plan with the full scan to get executed still. We would also favor plans with fewer full scans of virtual tables (which might also be worse than had we not applied the cost penalty). This commit adjusts the code to not apply the cost penalty to full scans of virtual tables.

This change is gated based on the new session variable `optimizer_apply_full_scan_penalty_to_virtual_tables` which defaults to `false` on master. We can discuss on the backport PRs whether it should be default to `true` there to require users to opt in for this PR to have an effect.

Fixes: #122708.

Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
  • Loading branch information
craig[bot] and yuzefovich committed May 9, 2024
2 parents cd17692 + 1bb9662 commit e0951b9
Show file tree
Hide file tree
Showing 17 changed files with 154 additions and 32 deletions.
2 changes: 2 additions & 0 deletions pkg/sql/conn_executor_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -2335,6 +2335,8 @@ func (ex *connExecutor) makeExecPlan(ctx context.Context, planner *planner) erro
if hasLargeScan {
// We don't execute the statement if:
// - plan contains a full table or full index scan.
// TODO(#123783): this currently doesn't apply to full scans
// of virtual tables.
// - the session setting disallows full table/index scans.
// - the scan is considered large.
// - the query is not an internal query.
Expand Down
4 changes: 4 additions & 0 deletions pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -3395,6 +3395,10 @@ func (m *sessionDataMutator) SetDisallowFullTableScans(val bool) {
m.data.DisallowFullTableScans = val
}

func (m *sessionDataMutator) SetOptimizerApplyFullScanPenaltyToVirtualTables(val bool) {
m.data.OptimizerApplyFullScanPenaltyToVirtualTables = val
}

func (m *sessionDataMutator) SetAlterColumnTypeGeneral(val bool) {
m.data.AlterColumnTypeGeneralEnabled = val
}
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/explain_bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,7 @@ func (c *stmtEnvCollector) PrintSessionSettings(w io.Writer, sv *settings.Values
{sessionSetting: "null_ordered_last"},
{sessionSetting: "on_update_rehome_row_enabled", clusterSetting: onUpdateRehomeRowEnabledClusterMode, convFunc: boolToOnOff},
{sessionSetting: "opt_split_scan_limit"},
{sessionSetting: "optimizer_apply_full_scan_penalty_to_virtual_tables", convFunc: boolToOnOff},
{sessionSetting: "optimizer_use_forecasts", convFunc: boolToOnOff},
{sessionSetting: "optimizer_use_histograms", clusterSetting: optUseHistogramsClusterMode, convFunc: boolToOnOff},
{sessionSetting: "optimizer_use_multicol_stats", clusterSetting: optUseMultiColStatsClusterMode, convFunc: boolToOnOff},
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/logictest/testdata/logic_test/information_schema
Original file line number Diff line number Diff line change
Expand Up @@ -6157,6 +6157,7 @@ on_update_rehome_row_enabled on
opt_split_scan_limit 2048
optimizer on
optimizer_always_use_histograms on
optimizer_apply_full_scan_penalty_to_virtual_tables off
optimizer_hoist_uncorrelated_equality_subqueries on
optimizer_merge_joins_enabled on
optimizer_prove_implication_with_virtual_computed_columns on
Expand Down
3 changes: 3 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/pg_catalog
Original file line number Diff line number Diff line change
Expand Up @@ -2886,6 +2886,7 @@ null_ordered_last off N
on_update_rehome_row_enabled on NULL NULL NULL string
opt_split_scan_limit 2048 NULL NULL NULL string
optimizer_always_use_histograms on NULL NULL NULL string
optimizer_apply_full_scan_penalty_to_virtual_tables off NULL NULL NULL string
optimizer_hoist_uncorrelated_equality_subqueries on NULL NULL NULL string
optimizer_merge_joins_enabled on NULL NULL NULL string
optimizer_prove_implication_with_virtual_computed_columns on NULL NULL NULL string
Expand Down Expand Up @@ -3070,6 +3071,7 @@ null_ordered_last off N
on_update_rehome_row_enabled on NULL user NULL on on
opt_split_scan_limit 2048 NULL user NULL 2048 2048
optimizer_always_use_histograms on NULL user NULL on on
optimizer_apply_full_scan_penalty_to_virtual_tables off NULL user NULL off off
optimizer_hoist_uncorrelated_equality_subqueries on NULL user NULL on on
optimizer_merge_joins_enabled on NULL user NULL on on
optimizer_prove_implication_with_virtual_computed_columns on NULL user NULL on on
Expand Down Expand Up @@ -3253,6 +3255,7 @@ on_update_rehome_row_enabled NULL NULL NULL
opt_split_scan_limit NULL NULL NULL NULL NULL
optimizer NULL NULL NULL NULL NULL
optimizer_always_use_histograms NULL NULL NULL NULL NULL
optimizer_apply_full_scan_penalty_to_virtual_tables NULL NULL NULL NULL NULL
optimizer_hoist_uncorrelated_equality_subqueries NULL NULL NULL NULL NULL
optimizer_merge_joins_enabled NULL NULL NULL NULL NULL
optimizer_prove_implication_with_virtual_computed_columns NULL NULL NULL NULL NULL
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/logictest/testdata/logic_test/select
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ SELECT * FROM t_disallow_scans
statement error pq: index "b_idx" cannot be used for this query\nHINT: try overriding the `disallow_full_table_scans`
SELECT * FROM t_disallow_scans@b_idx

# Internal statements should still be allowed with this setting.
# Full scans of virtual tables are still allowed with this setting (#123783).
statement ok
SELECT * FROM pg_class

Expand Down
1 change: 1 addition & 0 deletions pkg/sql/logictest/testdata/logic_test/show_source
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ null_ordered_last off
on_update_rehome_row_enabled on
opt_split_scan_limit 2048
optimizer_always_use_histograms on
optimizer_apply_full_scan_penalty_to_virtual_tables off
optimizer_hoist_uncorrelated_equality_subqueries on
optimizer_merge_joins_enabled on
optimizer_prove_implication_with_virtual_computed_columns on
Expand Down
11 changes: 9 additions & 2 deletions pkg/sql/opt/exec/execbuilder/relational.go
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,8 @@ func (b *Builder) scanParams(
if b.evalCtx.SessionData().DisallowFullTableScans &&
(b.flags.IsSet(exec.PlanFlagContainsLargeFullTableScan) ||
b.flags.IsSet(exec.PlanFlagContainsLargeFullIndexScan)) {
// TODO(#123783): this code might need an adjustment for virtual
// tables.
err = errors.WithHint(err,
"try overriding the `disallow_full_table_scans` or increasing the `large_full_scan_rows` cluster/session settings",
)
Expand Down Expand Up @@ -753,8 +755,13 @@ func (b *Builder) buildScan(scan *memo.ScanExpr) (_ execPlan, outputCols colOrdM
}
b.IndexesUsed = util.CombineUnique(b.IndexesUsed, []string{fmt.Sprintf("%d@%d", tab.ID(), idx.ID())})

// Save if we planned a full table/index scan on the builder so that the
// planner can be made aware later. We only do this for non-virtual tables.
// Save if we planned a full (large) table/index scan on the builder so that
// the planner can be made aware later. We only do this for non-virtual
// tables.
// TODO(#27611): consider doing this for virtual tables too once we have
// stats for them and adjust the comments if so.
// TODO(#123783): not setting the plan flags allows plans with full scans of
// virtual tables to not be rejected when disallow_full_table_scans is set.
relProps := scan.Relational()
stats := relProps.Statistics()
if !tab.IsVirtualTable() && isUnfiltered {
Expand Down
Loading

0 comments on commit e0951b9

Please sign in to comment.