From 9514555730fa951a2c72c6ba8b2df6f099c8a720 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Wed, 25 Oct 2023 15:30:22 -0600 Subject: [PATCH] opt: add setting for provided ordering fix This patch adds a new session setting, `optimizer_use_provided_ordering_fix`, which toggles whether to use the `finalizeProvided` function introduced in required ordering. This setting is on by default, and will be used when backporting #100776. Informs #113072 Release note: None --- pkg/sql/exec_util.go | 4 ++++ .../testdata/logic_test/information_schema | 1 + .../logictest/testdata/logic_test/pg_catalog | 3 +++ .../logictest/testdata/logic_test/show_source | 1 + pkg/sql/opt/memo/memo.go | 3 +++ pkg/sql/opt/memo/memo_test.go | 6 ++++++ pkg/sql/opt/optgen/exprgen/expr_gen.go | 2 +- pkg/sql/opt/ordering/BUILD.bazel | 1 + pkg/sql/opt/ordering/ordering.go | 9 +++++++-- pkg/sql/opt/testutils/opttester/opt_tester.go | 1 + pkg/sql/opt/xform/optimizer.go | 2 +- .../sessiondatapb/local_only_session_data.proto | 5 +++++ pkg/sql/vars.go | 17 +++++++++++++++++ 13 files changed, 51 insertions(+), 4 deletions(-) diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 386da8516520..b7825614b6a1 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -3671,6 +3671,10 @@ func (m *sessionDataMutator) SetOptimizerUseLockOpForSerializable(val bool) { m.data.OptimizerUseLockOpForSerializable = val } +func (m *sessionDataMutator) SetOptimizerUseProvidedOrderingFix(val bool) { + m.data.OptimizerUseProvidedOrderingFix = val +} + // Utility functions related to scrubbing sensitive information on SQL Stats. // quantizeCounts ensures that the Count field in the diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index b20942a5f973..8d9bb9db08a1 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -5571,6 +5571,7 @@ optimizer_use_limit_ordering_for_streaming_group_by on optimizer_use_lock_op_for_serializable off optimizer_use_multicol_stats on optimizer_use_not_visible_indexes off +optimizer_use_provided_ordering_fix on override_multi_region_zone_config off parallelize_multi_key_lookup_joins_enabled off password_encryption scram-sha-256 diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index 0db766da7213..12e84cd69618 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -2889,6 +2889,7 @@ optimizer_use_limit_ordering_for_streaming_group_by on N optimizer_use_lock_op_for_serializable off NULL NULL NULL string optimizer_use_multicol_stats on NULL NULL NULL string optimizer_use_not_visible_indexes off NULL NULL NULL string +optimizer_use_provided_ordering_fix on NULL NULL NULL string override_multi_region_zone_config off NULL NULL NULL string parallelize_multi_key_lookup_joins_enabled off NULL NULL NULL string password_encryption scram-sha-256 NULL NULL NULL string @@ -3052,6 +3053,7 @@ optimizer_use_limit_ordering_for_streaming_group_by on N optimizer_use_lock_op_for_serializable off NULL user NULL off off optimizer_use_multicol_stats on NULL user NULL on on optimizer_use_not_visible_indexes off NULL user NULL off off +optimizer_use_provided_ordering_fix on NULL user NULL on on override_multi_region_zone_config off NULL user NULL off off parallelize_multi_key_lookup_joins_enabled off NULL user NULL false false password_encryption scram-sha-256 NULL user NULL scram-sha-256 scram-sha-256 @@ -3214,6 +3216,7 @@ optimizer_use_limit_ordering_for_streaming_group_by NULL NULL NULL optimizer_use_lock_op_for_serializable NULL NULL NULL NULL NULL optimizer_use_multicol_stats NULL NULL NULL NULL NULL optimizer_use_not_visible_indexes NULL NULL NULL NULL NULL +optimizer_use_provided_ordering_fix NULL NULL NULL NULL NULL override_multi_region_zone_config NULL NULL NULL NULL NULL parallelize_multi_key_lookup_joins_enabled NULL NULL NULL NULL NULL password_encryption NULL NULL NULL NULL NULL diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source index 71c772796b1e..b97bebfbd526 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_source +++ b/pkg/sql/logictest/testdata/logic_test/show_source @@ -127,6 +127,7 @@ optimizer_use_limit_ordering_for_streaming_group_by on optimizer_use_lock_op_for_serializable off optimizer_use_multicol_stats on optimizer_use_not_visible_indexes off +optimizer_use_provided_ordering_fix on override_multi_region_zone_config off parallelize_multi_key_lookup_joins_enabled off password_encryption scram-sha-256 diff --git a/pkg/sql/opt/memo/memo.go b/pkg/sql/opt/memo/memo.go index 35121b4fd169..5a2e25ad6ea2 100644 --- a/pkg/sql/opt/memo/memo.go +++ b/pkg/sql/opt/memo/memo.go @@ -173,6 +173,7 @@ type Memo struct { durableLockingForSerializable bool sharedLockingForSerializable bool useLockOpForSerializable bool + useProvidedOrderingFix bool // txnIsoLevel is the isolation level under which the plan was created. This // affects the planning of some locking operations, so it must be included in @@ -244,6 +245,7 @@ func (m *Memo) Init(ctx context.Context, evalCtx *eval.Context) { durableLockingForSerializable: evalCtx.SessionData().DurableLockingForSerializable, sharedLockingForSerializable: evalCtx.SessionData().SharedLockingForSerializable, useLockOpForSerializable: evalCtx.SessionData().OptimizerUseLockOpForSerializable, + useProvidedOrderingFix: evalCtx.SessionData().OptimizerUseProvidedOrderingFix, txnIsoLevel: evalCtx.TxnIsoLevel, } m.metadata.Init() @@ -389,6 +391,7 @@ func (m *Memo) IsStale( m.durableLockingForSerializable != evalCtx.SessionData().DurableLockingForSerializable || m.sharedLockingForSerializable != evalCtx.SessionData().SharedLockingForSerializable || m.useLockOpForSerializable != evalCtx.SessionData().OptimizerUseLockOpForSerializable || + m.useProvidedOrderingFix != evalCtx.SessionData().OptimizerUseProvidedOrderingFix || m.txnIsoLevel != evalCtx.TxnIsoLevel { return true, nil } diff --git a/pkg/sql/opt/memo/memo_test.go b/pkg/sql/opt/memo/memo_test.go index 4cdd4acc9d00..dd9bc844bfe8 100644 --- a/pkg/sql/opt/memo/memo_test.go +++ b/pkg/sql/opt/memo/memo_test.go @@ -418,6 +418,12 @@ func TestMemoIsStale(t *testing.T) { evalCtx.TxnIsoLevel = isolation.Serializable notStale() + // Stale optimizer_use_provided_ordering_fix. + evalCtx.SessionData().OptimizerUseProvidedOrderingFix = true + stale() + evalCtx.SessionData().OptimizerUseProvidedOrderingFix = false + notStale() + // User no longer has access to view. catalog.View(tree.NewTableNameWithSchema("t", catconstants.PublicSchemaName, "abcview")).Revoked = true _, err = o.Memo().IsStale(ctx, &evalCtx, catalog) diff --git a/pkg/sql/opt/optgen/exprgen/expr_gen.go b/pkg/sql/opt/optgen/exprgen/expr_gen.go index 23e7f8c99a5a..b409724e719e 100644 --- a/pkg/sql/opt/optgen/exprgen/expr_gen.go +++ b/pkg/sql/opt/optgen/exprgen/expr_gen.go @@ -411,7 +411,7 @@ func (eg *exprGen) populateBestProps( provided := &physical.Provided{} // BuildProvided relies on ProvidedPhysical() being set in the children, so // it must run after the recursive calls on the children. - provided.Ordering = ordering.BuildProvided(rel, &required.Ordering) + provided.Ordering = ordering.BuildProvided(eg.f.EvalContext(), rel, &required.Ordering) provided.Distribution = distribution.BuildProvided(ctx, eg.f.EvalContext(), rel, &required.Distribution) cost += eg.coster.ComputeCost(rel, required) diff --git a/pkg/sql/opt/ordering/BUILD.bazel b/pkg/sql/opt/ordering/BUILD.bazel index c6a727d29cc0..41372d2a9882 100644 --- a/pkg/sql/opt/ordering/BUILD.bazel +++ b/pkg/sql/opt/ordering/BUILD.bazel @@ -30,6 +30,7 @@ go_library( "//pkg/sql/opt/cat", "//pkg/sql/opt/memo", "//pkg/sql/opt/props", + "//pkg/sql/sem/eval", "//pkg/sql/sem/tree", "//pkg/util/buildutil", "@com_github_cockroachdb_errors//:errors", diff --git a/pkg/sql/opt/ordering/ordering.go b/pkg/sql/opt/ordering/ordering.go index a58d18a2409a..2c0f8519fe9f 100644 --- a/pkg/sql/opt/ordering/ordering.go +++ b/pkg/sql/opt/ordering/ordering.go @@ -14,6 +14,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/props" + "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" @@ -83,12 +84,16 @@ func BuildChildRequired( // // This function assumes that the provided orderings have already been set in // the children of the expression. -func BuildProvided(expr memo.RelExpr, required *props.OrderingChoice) opt.Ordering { +func BuildProvided( + evalCtx *eval.Context, expr memo.RelExpr, required *props.OrderingChoice, +) opt.Ordering { if required.Any() { return nil } provided := funcMap[expr.Op()].buildProvidedOrdering(expr, required) - provided = finalizeProvided(provided, required, expr.Relational().OutputCols) + if evalCtx.SessionData().OptimizerUseProvidedOrderingFix { + provided = finalizeProvided(provided, required, expr.Relational().OutputCols) + } if buildutil.CrdbTestBuild { checkProvided(expr, required, provided) diff --git a/pkg/sql/opt/testutils/opttester/opt_tester.go b/pkg/sql/opt/testutils/opttester/opt_tester.go index 9691c2d3f989..acd32195d88d 100644 --- a/pkg/sql/opt/testutils/opttester/opt_tester.go +++ b/pkg/sql/opt/testutils/opttester/opt_tester.go @@ -303,6 +303,7 @@ func New(catalog cat.Catalog, sql string) *OptTester { ot.evalCtx.SessionData().OptimizerHoistUncorrelatedEqualitySubqueries = true ot.evalCtx.SessionData().OptimizerUseImprovedComputedColumnFiltersDerivation = true ot.evalCtx.SessionData().OptimizerUseImprovedJoinElimination = true + ot.evalCtx.SessionData().OptimizerUseProvidedOrderingFix = true return ot } diff --git a/pkg/sql/opt/xform/optimizer.go b/pkg/sql/opt/xform/optimizer.go index 0cc4ea49db9f..4326fdf51ec8 100644 --- a/pkg/sql/opt/xform/optimizer.go +++ b/pkg/sql/opt/xform/optimizer.go @@ -814,7 +814,7 @@ func (o *Optimizer) setLowestCostTree(parent opt.Expr, parentProps *physical.Req var provided physical.Provided // BuildProvided relies on ProvidedPhysical() being set in the children, so // it must run after the recursive calls on the children. - provided.Ordering = ordering.BuildProvided(relParent, &parentProps.Ordering) + provided.Ordering = ordering.BuildProvided(o.evalCtx, relParent, &parentProps.Ordering) provided.Distribution = distribution.BuildProvided(o.ctx, o.evalCtx, relParent, &parentProps.Distribution) o.mem.SetBestProps(relParent, parentProps, &provided, relCost) } diff --git a/pkg/sql/sessiondatapb/local_only_session_data.proto b/pkg/sql/sessiondatapb/local_only_session_data.proto index cee2741e009e..945538511d58 100644 --- a/pkg/sql/sessiondatapb/local_only_session_data.proto +++ b/pkg/sql/sessiondatapb/local_only_session_data.proto @@ -458,6 +458,11 @@ message LocalOnlySessionData { // implements SELECT FOR UPDATE and SELECT FOR SHARE using the Lock operator, // regardless of this setting. bool optimizer_use_lock_op_for_serializable = 114; + // OptimizerUseProvidedOrderingFix, when true, causes the optimizer to + // reconcile provided orderings with required ordering choices. This prevents + // internal errors due to incomplete functional dependencies, and also + // fixes a bug that incorrectly truncated the provided ordering (see #113072). + bool optimizer_use_provided_ordering_fix = 115; /////////////////////////////////////////////////////////////////////////// // WARNING: consider whether a session parameter you're adding needs to // diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index 10df42358ecb..fa526dd0037f 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -2964,6 +2964,23 @@ var varGen = map[string]sessionVar{ }, GlobalDefault: globalFalse, }, + + // CockroachDB extension. + `optimizer_use_provided_ordering_fix`: { + GetStringVal: makePostgresBoolGetStringValFn(`optimizer_use_provided_ordering_fix`), + Set: func(_ context.Context, m sessionDataMutator, s string) error { + b, err := paramparse.ParseBoolVar("optimizer_use_provided_ordering_fix", s) + if err != nil { + return err + } + m.SetOptimizerUseProvidedOrderingFix(b) + return nil + }, + Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) { + return formatBoolAsPostgresSetting(evalCtx.SessionData().OptimizerUseProvidedOrderingFix), nil + }, + GlobalDefault: globalTrue, + }, } func ReplicationModeFromString(s string) (sessiondatapb.ReplicationMode, error) {