diff --git a/pkg/sql/logictest/testdata/logic_test/prepare b/pkg/sql/logictest/testdata/logic_test/prepare index 02ffabccb3bd..6afdde5350e1 100644 --- a/pkg/sql/logictest/testdata/logic_test/prepare +++ b/pkg/sql/logictest/testdata/logic_test/prepare @@ -739,7 +739,8 @@ EXECUTE change_view statement ok ALTER VIEW otherview RENAME TO otherview2 -query error pq: relation "otherview" does not exist +# HP and CBO return slightly different errors, so accept both. +query error pq: relation "(otherdb.public.)?otherview" does not exist EXECUTE change_view statement ok diff --git a/pkg/sql/opt/memo/memo.go b/pkg/sql/opt/memo/memo.go index ea975be6ba73..7b1b53865d7f 100644 --- a/pkg/sql/opt/memo/memo.go +++ b/pkg/sql/opt/memo/memo.go @@ -247,29 +247,36 @@ func (m *Memo) HasPlaceholders() bool { // 5. Data source privileges: current user may no longer have access to one or // more data sources. // -func (m *Memo) IsStale(ctx context.Context, evalCtx *tree.EvalContext, catalog cat.Catalog) bool { +// This function cannot swallow errors and return only a boolean, as it may +// perform KV operations on behalf of the transaction associated with the +// provided catalog, and those errors are required to be propagated. +func (m *Memo) IsStale( + ctx context.Context, evalCtx *tree.EvalContext, catalog cat.Catalog, +) (bool, error) { // Memo is stale if the current database has changed. if m.dbName != evalCtx.SessionData.Database { - return true + return true, nil } // Memo is stale if the search path has changed. if !m.searchPath.Equals(&evalCtx.SessionData.SearchPath) { - return true + return true, nil } // Memo is stale if the location has changed. if m.locName != evalCtx.GetLocation().String() { - return true + return true, nil } // Memo is stale if the fingerprint of any data source in the memo's metadata // has changed, or if the current user no longer has sufficient privilege to // access the data source. - if !m.Metadata().CheckDependencies(ctx, catalog) { - return true + if depsUpToDate, err := m.Metadata().CheckDependencies(ctx, catalog); err != nil { + return false, err + } else if !depsUpToDate { + return true, nil } - return false + return false, nil } // InternPhysicalProps adds the given physical props to the memo if they haven't diff --git a/pkg/sql/opt/memo/memo_test.go b/pkg/sql/opt/memo/memo_test.go index 6dedaeb34421..7fc1a271cb28 100644 --- a/pkg/sql/opt/memo/memo_test.go +++ b/pkg/sql/opt/memo/memo_test.go @@ -22,12 +22,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/optbuilder" - "github.com/cockroachdb/cockroach/pkg/sql/opt/testutils" + opttestutils "github.com/cockroachdb/cockroach/pkg/sql/opt/testutils" "github.com/cockroachdb/cockroach/pkg/sql/opt/testutils/testcat" "github.com/cockroachdb/cockroach/pkg/sql/opt/xform" "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + testutils "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/datadriven" ) @@ -121,31 +122,41 @@ func TestMemoIsStale(t *testing.T) { if err != nil { t.Fatal(err) } - if o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if isStale { t.Errorf("memo should not be stale") } // Stale current database. evalCtx.SessionData.Database = "newdb" - if !o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if !isStale { t.Errorf("expected stale current database") } evalCtx.SessionData.Database = "t" // Stale search path. evalCtx.SessionData.SearchPath = sessiondata.SearchPath{} - if !o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if !isStale { t.Errorf("expected stale search path") } evalCtx.SessionData.SearchPath = sessiondata.MakeSearchPath([]string{"path1", "path2"}) - if o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if isStale { t.Errorf("memo should not be stale") } evalCtx.SessionData.SearchPath = sessiondata.MakeSearchPath(searchPath) // Stale location. evalCtx.SessionData.DataConversion.Location = time.FixedZone("PST", -8*60*60) - if !o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if !isStale { t.Errorf("expected stale location") } evalCtx.SessionData.DataConversion.Location = time.UTC @@ -159,7 +170,9 @@ func TestMemoIsStale(t *testing.T) { if err != nil { t.Fatal(err) } - if !o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if !isStale { t.Errorf("expected stale schema") } catalog = testcat.New() @@ -168,13 +181,16 @@ func TestMemoIsStale(t *testing.T) { // User no longer has access to view. catalog.View(tree.NewTableName("t", "abcview")).Revoked = true - if !o.Memo().IsStale(ctx, &evalCtx, catalog) { - t.Errorf("expected user not to have SELECT privilege on view") + _, err = o.Memo().IsStale(ctx, &evalCtx, catalog) + if exp := "user does not have privilege"; !testutils.IsError(err, exp) { + t.Fatalf("expected %q error, but got %+v", exp, err) } catalog.View(tree.NewTableName("t", "abcview")).Revoked = false // Ensure that memo is not stale after restoring to original state. - if o.Memo().IsStale(ctx, &evalCtx, catalog) { + if isStale, err := o.Memo().IsStale(ctx, &evalCtx, catalog); err != nil { + t.Fatal(err) + } else if isStale { t.Errorf("memo should not be stale") } } @@ -190,7 +206,7 @@ func runDataDrivenTest(t *testing.T, path string, fmtFlags memo.ExprFmtFlags) { datadriven.Walk(t, path, func(t *testing.T, path string) { catalog := testcat.New() datadriven.RunTest(t, path, func(d *datadriven.TestData) string { - tester := testutils.NewOptTester(catalog, d.Input) + tester := opttestutils.NewOptTester(catalog, d.Input) tester.Flags.ExprFormat = fmtFlags return tester.RunCommand(t, d) }) diff --git a/pkg/sql/opt/metadata.go b/pkg/sql/opt/metadata.go index 9f4cb9a9e0d7..cf2d6fa91593 100644 --- a/pkg/sql/opt/metadata.go +++ b/pkg/sql/opt/metadata.go @@ -119,17 +119,21 @@ func (md *Metadata) AddDependency(ds cat.DataSource, priv privilege.Kind) { // in order to check that the fully qualified data source names still resolve to // the same version of the same data source, and that the user still has // sufficient privileges to access the data source. -func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog) bool { +// +// This function cannot swallow errors and return only a boolean, as it may +// perform KV operations on behalf of the transaction associated with the +// provided catalog, and those errors are required to be propagated. +func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog) (bool, error) { for dep, privs := range md.deps { ds, err := catalog.ResolveDataSource(ctx, dep.Name()) if err != nil { - return false + return false, err } if dep.ID() != ds.ID() { - return false + return false, nil } if dep.Version() != ds.Version() { - return false + return false, nil } for privs != 0 { @@ -140,7 +144,7 @@ func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog) priv := privilege.Kind(bits.TrailingZeros32(uint32(privs))) if priv != 0 { if err = catalog.CheckPrivilege(ctx, ds, priv); err != nil { - return false + return false, err } } @@ -148,7 +152,7 @@ func (md *Metadata) CheckDependencies(ctx context.Context, catalog cat.Catalog) privs &= ^(1 << priv) } } - return true + return true, nil } // AddTable indexes a new reference to a table within the query. Separate diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go index e21f5e6b0b49..4515e0441a06 100644 --- a/pkg/sql/plan.go +++ b/pkg/sql/plan.go @@ -439,7 +439,9 @@ func (p *planner) makeOptimizerPlan(ctx context.Context, stmt Statement) (planFl // If the prepared memo has been invalidated by schema or other changes, // re-prepare it. - if stmt.Prepared.Memo.IsStale(ctx, p.EvalContext(), &catalog) { + if isStale, err := stmt.Prepared.Memo.IsStale(ctx, p.EvalContext(), &catalog); err != nil { + return 0, err + } else if isStale { stmt.Prepared.Memo, err = p.prepareMemo(ctx, &catalog, stmt) if err != nil { return 0, err @@ -451,7 +453,9 @@ func (p *planner) makeOptimizerPlan(ctx context.Context, stmt Statement) (planFl // Consult the query cache. cachedData, ok := p.execCfg.QueryCache.Find(stmt.SQL) if ok { - if cachedData.Memo.IsStale(ctx, p.EvalContext(), &catalog) { + if isStale, err := cachedData.Memo.IsStale(ctx, p.EvalContext(), &catalog); err != nil { + return 0, err + } else if isStale { cachedData.Memo, err = p.prepareMemo(ctx, &catalog, stmt) if err != nil { return 0, err