Seems to me like a better approach might be to do this in the optbuilder, but set b.trackViewDeps to false when building check constraints (or wherever you want to be sure not to track dependencies).

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

pkg/sql/opt_exec_factory.go, line 1712 at r1 (raw file):

						typeDepSet[id] = struct{}{}
					}
				}

Ideally opt_exec_factory.go would have as little of this type of logic as possible. Is there no way to move this back into the optbuilder?

pkg/sql/logictest/testdata/logic_test/views, line 968 at r1 (raw file):

statement ok
CREATE TABLE t (k typ AS ('a'::typ) STORED)

why did you change this?

pkg/sql/opt/optbuilder/builder.go, line 431 at r1 (raw file):

func (b *Builder) maybeTrackUserDefinedTypeDepsForViews(texpr tree.TypedExpr, inScope *scope) {
	if b.trackViewDeps {
		if texpr.ResolvedType().UserDefined() && inScope != nil && inScope.parent != nil {

Why do you need to check that these scopes are nil? This needs a comment. Also this seems fragile.

Would you mind adding a test for the issue you referenced in the PR description as well? Maybe in pkg/ccl/logictestccl/testdata/logic_test/multi_region, as the multiregion stuff is under CCL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

Seems to me like a better approach might be to do this in the optbuilder, but set b.trackViewDeps to false when building check constraints (or wherever you want to be sure not to track dependencies).

Yeah that seems much better, I've changed the implementation to do this. Thanks for the suggestion!

Would you mind adding a test for the issue you referenced in the PR description as well? Maybe in pkg/ccl/logictestccl/testdata/logic_test/multi_region, as the multiregion stuff is under CCL.

Yep, added a test. I'm not too familiar with multi region so I only added the simple case that was failing that you mentioned in the PR and verified Oliver's repro doesn't fail anymore, if there's anything else you'd like me to test, please let me know and I can add it to this PR.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

pkg/sql/opt_exec_factory.go, line 1712 at r1 (raw file):

Yep I've moved it into optbuilder/create_view.go, I'm not very familiar with the optbuilder stuff so please let me know if it should go elsewhere.

pkg/sql/logictest/testdata/logic_test/views, line 968 at r1 (raw file):

Sorry, leftovers from testing, reverted.

pkg/sql/opt/optbuilder/builder.go, line 431 at r1 (raw file):

Removed this after changing the implementation to use b.trackViewDeps.

Yep, added a test. I'm not too familiar with multi region so I only added the simple case that was failing that you mentioned in the PR and verified Oliver's repro doesn't fail anymore, if there's anything else you'd like me to test, please let me know and I can add it to this PR.

I think that should be good enough, thanks for fixing this!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

Looks much better!

Reviewed 12 of 12 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1516 at r2 (raw file):

Diagram: https://cockroachdb.github.io/distsqlplan/decode.html#eJykk19v2jwUxu_fT2GdXvRdZZq_BRqpkquNbVQMOqi0STNCaXzSegM7c5wtU8XVPtu-15SEtgMN1LILSHx8fjnPeexzB_nXOUTQ-3g5OO8Pyf-v-pOryfvBCzLpDXovr4g2As1MCkoyoxc6P64fq4BMkJxPmpfZd2lvdWFndcL97hERMk90oex6YpNFXo9H75oSObkY9YerImR0_3YsVaoJ44XrBrjKbGIf3vbGvQd55IwcnoYYXnfDTqub-mkrjJOTVjdIvVY79EQbA5F2_M4hUFBa4DBeYA7RJwhhSiEzOsE816YK3dUJfVFC5FKQKitsFZ5SSLRBiO7ASjtHiOAqvp7jGGOBxnGBgkAby3n92UYpy4xcxOYHUJhkscoj4nBgHBwOnJenIeclVn_X3Tecl93Uufj1k_My9QTnpSfUWbXoHHJo7ck5ByRWgnhE21s0QGFU2IgwjzKfshCmSwq6sI8d5ja-QYi8Jd3PBe_JLlS63T2d2Jd1Dp7sgL_VgcfGC1X3h2Kt6enyLx4NdUtnjr_uzkAupCXeVg3uc06hr76hsSgutFRonGC9VDNMrHnMqvmZSVECfcB6ZWYIaz9MGgs2rAp2uRU8R2mlcHVdwi0q76_LQOsvRUY-a6mIVhFhFTAaEtZZFzpGJdDUWgk7oYT51e-ItbcqDp-jeIx5plWOG-e87dSmFFDcYHNZcl2YBC-NTuoyzXJUc3VAYG6bXb9Z9FW9VY_gn7D3L7C_Ew7WYHcTDnbC4W443AmfbMDT5X-_AwAA___uBSO6

# Verify we can create a view off of a regional by row table.

Is this a regression test? would be good to mention the issue number if so.

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1529 at r2 (raw file):

statement ok
CREATE TABLE kv (k INT PRIMARY KEY, v INT) LOCALITY REGIONAL BY ROW

super nit: you can combine these setup statements like this:

statement ok
CREATE DATABASE db;
USE db;
ALTER DATABASE db PRIMARY REGION "us-east-1";
CREATE TABLE kv (k INT PRIMARY KEY, v INT) LOCALITY REGIONAL BY ROW

pkg/sql/opt_catalog.go, line 286 at r2 (raw file):

		return t.desc.PublicColumns()[ord].GetType(), nil
	case *optSequence:
		return t.desc.PublicColumns()[ord].GetType(), nil

I just fixed an issue where using PublicColumns() caused an index out of range error with the crdb_internal_mvcc_timestamp column (#63413). Is this going to have the same problem?

pkg/sql/opt/cat/catalog.go, line 162 at r2 (raw file):

	// GetColumnTypeOfDataSource returns the type of the given column of
	// the given table.

nit: table -> data source

pkg/sql/opt/cat/catalog.go, line 163 at r2 (raw file):

	// GetColumnTypeOfDataSource returns the type of the given column of
	// the given table.
	GetColumnTypeOfDataSource(o DataSource, ord int) (*types.T, error)

I think it would be better if this were a method on the DataSource interface, since it seems like you don't need a reference to the optCatalog for the implementation of this function. You could add ColumnCount() and ColumnType(i int) methods, and make sure that each of these data sources implement them. A more complete solution would have each data source return the entire column with Column(i int) similar to cat.Table, but that's probably overkill. Maybe just use ColumnType but add a TODO.

You should also make sure that there are tests with each type of data source.

pkg/sql/opt/optbuilder/create_view.go, line 60 at r3 (raw file):

	}

	// If the type of any column that this views references is user

nit: views -> view

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

// can only contain immutable operators.
func (b *Builder) addCheckConstraintsForTable(tabMeta *opt.TableMeta) {
	if b.trackViewDeps {

can you add a comment to explain why we don't want to track view deps here?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1516 at r2 (raw file):

Done.

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1529 at r2 (raw file):

Done.

pkg/sql/opt_catalog.go, line 286 at r2 (raw file):

Good point, I've changed it to use AllColumns() now.

pkg/sql/opt/cat/catalog.go, line 162 at r2 (raw file):

Done.

pkg/sql/opt/cat/catalog.go, line 163 at r2 (raw file):

That makes sense, I've moved ColumnType(i int) to be a method on the DataSource. One question I had was that in test_catalog.go, View only stores the column names rather than cat.Column like Table does, so I'm not sure how to implement ColumnType(i int) for views in test_catalog. It looks like View is constructed from a *tree.CreateView, which also only contains the column names. A similar problem exists for Sequence, although I don't think a sequence can have a UDT as a column type, so returning nil and just skipping it is probably fine.

pkg/sql/opt/optbuilder/create_view.go, line 60 at r3 (raw file):

Done.

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

Yes, done.

Reviewed 7 of 7 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1529 at r2 (raw file):

You need to add semicolons after each statement if you combine them like this.

pkg/sql/opt_catalog.go, line 532 at r4 (raw file):

}

// ColumnType is part of the cat.Table interface.

cat.Table -> cat.DataSource

pkg/sql/opt_catalog.go, line 577 at r4 (raw file):

func (os *optSequence) SequenceMarker() {}

// ColumnType is part of the cat.Table interface.

ditto

pkg/sql/opt_catalog.go, line 1129 at r4 (raw file):

}

// ColumnType is part of the cat.Table interface.

ditto

pkg/sql/opt_catalog.go, line 1999 at r4 (raw file):

}

// ColumnType is part of the cat.Table interface.

ditto

pkg/sql/opt/cat/catalog.go, line 163 at r2 (raw file):

Yea, just returning nil is fine for the test catalog.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 573 at r4 (raw file):

}

// GetColumnTypeOfDataSource is part of the cat.View interface.

fix name of function and interface

pkg/sql/opt/testutils/testcat/test_catalog.go, line 762 at r4 (raw file):

}

// GetColumnTypeOfDataSource is part of the cat.Table interface.

ditto

pkg/sql/opt/testutils/testcat/test_catalog.go, line 1242 at r4 (raw file):

}

func (ts *Sequence) ColumnType(ord int) (*types.T, error) {

add comment

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1529 at r2 (raw file):

Sorry, done.

pkg/sql/opt_catalog.go, line 532 at r4 (raw file):

Done.

pkg/sql/opt_catalog.go, line 577 at r4 (raw file):

Done.

pkg/sql/opt_catalog.go, line 1129 at r4 (raw file):

Done.

pkg/sql/opt_catalog.go, line 1999 at r4 (raw file):

Done.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 573 at r4 (raw file):

Eesh not sure why these didn't change when I refactored in GoLand, done.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 762 at r4 (raw file):

Done.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 1242 at r4 (raw file):

Done.

Reviewed 1 of 12 files at r2, 2 of 7 files at r4, 3 of 3 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

Have you tested similar cases with computed columns and partial indexes that reference UDT columns?

Reviewed 3 of 3 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

pkg/ccl/logictestccl/testdata/logic_test/multi_region, line 1529 at r2 (raw file):

No need to apologize!!

pkg/sql/logictest/testdata/logic_test/views, line 1036 at r5 (raw file):

# Now that v5 depends on t4.j which is of type typ4, v5 has a dependency to typ4.
statement error cannot drop type "typ4" because other objects \(\[db2.public.t4 db2.public.v5\]\) still depend on it
DROP type typ4

I still think you need some tests for the different kinds of data sources. Even though you may not be able to create columns with user-defined types in virtual tables or sequences, you should still have some tests that exercise the new ColumnType functions for each case. Simply creating a view that selects columns from all of these different data source types should be sufficient.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 573 at r4 (raw file):

Still need to update cat.View -> cat.DataSource

pkg/sql/opt/testutils/testcat/test_catalog.go, line 762 at r4 (raw file):

cat.Table -> cat.DataSource

pkg/sql/opt/testutils/testcat/test_catalog.go, line 1242 at r4 (raw file):

cat.Sequence -> cat.DataSource

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @rytaft)

pkg/sql/logictest/testdata/logic_test/views, line 1036 at r5 (raw file):

Done, I've added a few additional tests here that create views from sequences, virtual tables and other views. Please let me know if more needs to be added!

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

This is a good point, after more testing I believe this works if the column type itself is a UDT, but not if the expression just contains a UDT (for example 'a'::typ::string. I've changed the method on DataSource to not only check the type of the column for a UDT, but also walk the default/computed expressions to check for UDT usages, and return a set of all UDTs it finds.

Also added tests to check for these cases as well.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 573 at r4 (raw file):

Ahh sorry, done.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 762 at r4 (raw file):

Done.

pkg/sql/opt/testutils/testcat/test_catalog.go, line 1242 at r4 (raw file):

Done.

Reviewed 6 of 6 files at r6, 1 of 1 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @the-ericwang35)

pkg/sql/opt_catalog.go, line 2256 at r6 (raw file):

		}
	}
	if col.GetType() != nil && col.GetType().UserDefined() {

nit: you could do:

if typ := col.GetType(); typ != nil && typ.UserDefined() {
  visitor.OIDs[typ.Oid()] = struct{}{}
}

pkg/sql/logictest/testdata/logic_test/views, line 1036 at r5 (raw file):

Looks good - thanks!

pkg/sql/logictest/testdata/logic_test/views, line 1069 at r6 (raw file):

CREATE VIEW v6 AS (SELECT j FROM v4_dep)

# v6 depends on v4_dep.j, which depends on t4.j, which depends on typ4, so v6 also depends on typ5.

typ5 -> typ4 ?

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

Nice! I don't see the tests for computed columns, though -- did you forget to push those? Also, @mgartner made a good point about partial index predicates too -- I think you'll also want to consider that case and possibly set b.trackViewDeps to false when building partial index predicates.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @rytaft)

pkg/sql/opt_catalog.go, line 2256 at r6 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/views, line 1069 at r6 (raw file):

Ah yes, done.

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

There were some tests from before regarding UDTs in computed columns so I amended those (around line 968 in the views logic_test), if these aren't enough I can add some more.

Regarding partial indexes, yes you're right. I've set b.trackViewDeps to false while building them now and added some tests. Thanks so much for pointing out these cases!

Reviewed 3 of 6 files at r6, 1 of 1 files at r7, 3 of 3 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @rytaft, and @the-ericwang35)

pkg/sql/opt_catalog.go, line 535 at r8 (raw file):

// CollectTypes is part of the cat.DataSource interface.
func (ov *optView) CollectTypes(ord int) (map[descpb.ID]struct{}, error) {

Can the signature be (descpb.IDs, error) instead? The only place I see CollectTypes being used is in create_view.go where it iterates over all the types in the map, so I don't see the need for the overhead of a map.

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

Last question: I don't think it's allowed, but can you create a view from a mutation, .e.g. create view v as (update t set a = 1 returning a)? If so, there's a handful of other places where we build check constraint and partial index expressions which we'd have to turn off trackViewDeps for.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @rytaft)

pkg/sql/opt_catalog.go, line 535 at r8 (raw file):

Sure, changed it to return descpb.IDs instead.

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

Hmm so I gave this a try and wasn't able to get it to work.

demo@127.0.0.1:26257/movr> CREATE TABLE t (i INT);
CREATE TABLE

Time: 5ms total (execution 4ms / network 1ms)

demo@127.0.0.1:26257/movr> CREATE VIEW v AS (UPDATE t SET i = 1 WHERE i = 2 RETURNING i);
invalid syntax: statement ignored: at or near "update": syntax error
SQLSTATE: 42601
DETAIL: source SQL:
CREATE VIEW v AS (UPDATE t SET i = 1 WHERE i = 2 RETURNING i)
                  ^
HINT: try \h <SELECTCLAUSE>

Reviewed 2 of 3 files at r8, 4 of 4 files at r9.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @the-ericwang35)

pkg/sql/opt/optbuilder/select.go, line 592 at r2 (raw file):

It doesn't work (even with the right syntax):

demo@127.0.0.1:26257/movr> CREATE VIEW v AS (SELECT * FROM [UPDATE t SET i = 1 WHERE i = 2 RETURNING i]);
ERROR: UPDATE cannot be used inside a view definition
SQLSTATE: 42601

pkg/sql/opt/testutils/testcat/test_catalog.go, line 793 at r9 (raw file):

	}

	ids := make(descpb.IDs, len(visitor.OIDs))

if you're going to use append below, you need to change this to:

ids := make(descpb.IDs, 0, len(visitor.OIDs))

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, @rytaft, and @the-ericwang35)

pkg/sql/opt/testutils/testcat/test_catalog.go, line 793 at r9 (raw file):

Sorry, fixed.

Thanks for working through this and sorry for the back and forth! I think it turned out well.

Reviewed 1 of 1 files at r10.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @the-ericwang35)

Nice work! Looks like you'll have to rebase and fix a conflict.

Reviewed 3 of 4 files at r9, 1 of 1 files at r10.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @mgartner, and @the-ericwang35)