Merge #113097

113097: opt: add setting for provided ordering fix r=DrewKimball a=DrewKimball 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 Co-authored-by: Drew Kimball <[email protected]>
cockroachdb · Oct 26, 2023 · 40bcea4 · 40bcea4
2 parents 2205334 + 9514555
commit 40bcea4
Show file tree

Hide file tree

Showing 13 changed files with 51 additions and 4 deletions.
diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go
@@ -3676,6 +3676,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
@@ -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
@@ -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
@@ -3053,6 +3054,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
@@ -3216,6 +3218,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -2981,6 +2981,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) {