crosscluster/logical: hydrate table descriptors with types for event decoding #131130

msbutler · 2024-09-20T22:01:35Z

This hydrates the source side table descriptors with their referencing type
descriptors if they have any. This allows ldr ingestion to properly decode
tables with user defined types.

Note that LDR will still fast fail on replicating a table with user defined
types (unless the SKIP SCHEMA CHECK OPTION is used) because we still need to
teach LDR to:

validate that the source and destination enum's are physically identical
(contain the same map from int to enum values)
support UDT replication on sql path, which currently fails because the passed
in datum points to the source side udt instead of the destination udt.

As another step down the line, we should relax the requirement that UDTs must
physically equivalent. Instead, they only need to be logically equivalent
(i.e. have the same user facing enum values). Supporting this requires
remapping the ints from two logically equivalent enums.

Epic: none

Release note: none

blathers-crl · 2024-09-20T22:01:39Z

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2024-09-20T22:01:45Z

This change is

Epic: none Release note: none

msbutler · 2024-10-04T19:56:48Z

pkg/sql/importer/import_type_resolver.go

@@ -52,7 +52,7 @@ func makeImportTypeResolver(typeDescs []*descpb.TypeDescriptor) importTypeResolv
 // Note that if a table happens to have multiple types with the same name (but
 // different schemas), this implementation will return a "feature unsupported"
 // error.
-func (i importTypeResolver) ResolveType(
+func (i ImportTypeResolver) ResolveType(


I'm wondering if there's a better TypeResolver is should use?

This type resolver is insufficient for the sql write path. Ideally the type resolver used by LDR can take in foreign types with mismatched OIDs and allow the sql path to just work. Currently the sql path writes panic because we hydrate the datums ingested using the source side UDTs, which have a different OID than on the destination side. I can write up a separate issue to address this

msbutler · 2024-10-04T19:57:40Z

pkg/ccl/crosscluster/producer/replication_manager.go

+		}
+		typeIDs, _, err := td.GetAllReferencedTypeIDs(dbDesc,
+			func(id descpb.ID) (catalog.TypeDescriptor, error) {
+				if id > 0 {


what's the deal with this function passed to GetAllREferencedTypeIDs? as the panic suggests, it doesn't seem to get called.

msbutler · 2024-10-04T20:08:20Z

pkg/ccl/crosscluster/streamclient/partitioned_stream_client.go

@@ -341,10 +342,17 @@ func (p *partitionedStreamClient) PlanLogicalReplication(
 		descMap[int32(desc.ID)] = desc
 	}

+	// TODO(msbutler): unclear if we should persist the types as pointers in the protos.


ignore this comment. i like how i stored the types in stream.proto

fqazi

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @msbutler, and @rafiss)

pkg/ccl/crosscluster/logical/logical_replication_job.go line 363 at r3 (raw file):

	// TODO(msbutler): is this import type resolver kosher? Should put in a new package.
	importResolver := importer.MakeImportTypeResolver(plan.SourceTypes)

Probably want to in a separate PR move this out / rename this later.

pkg/ccl/crosscluster/producer/replication_manager.go line 163 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

what's the deal with this function passed to GetAllREferencedTypeIDs? as the panic suggests, it doesn't seem to get called.

This code is only focused on default / update expressions? Have your tests included that. i.e. Have a cast in your default expression.

pkg/ccl/crosscluster/producer/replication_manager.go line 170 at r2 (raw file):

				}
				foundTypeDescriptors[id] = struct{}{}
				return descriptors.ByIDWithoutLeased(r.txn.KV()).WithoutNonPublic().Get().Type(ctx, id)

Why do we avoid leasing here?

pkg/sql/importer/import_type_resolver.go line 55 at r1 (raw file):

Previously, msbutler (Michael Butler) wrote…

This type resolver is insufficient for the sql write path. Ideally the type resolver used by LDR can take in foreign types with mismatched OIDs and allow the sql path to just work. Currently the sql path writes panic because we hydrate the datums ingested using the source side UDTs, which have a different OID than on the destination side. I can write up a separate issue to address this

I think this is probably the better option. We can probably make this more generic implementation more generic. i.e. Extend it so that it can rewrite OIDs upon hydration as a feature, which only LDR needs.

Or implement a LDR specific version on top with that logic.

pkg/ccl/crosscluster/logical/logical_replication_job_test.go line 1974 at r3 (raw file):

	dbB.Exec(t, "CREATE TYPE my_enum AS ENUM ('one', 'two', 'three')")

	dbA.Exec(t, "CREATE TABLE data (pk INT PRIMARY KEY, val my_enum)")

To hit some of the code paths above we can have fun and add default expressions which use the type.

…ning Epic: none Release note: none

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, and @rafiss)

pkg/ccl/crosscluster/logical/logical_replication_job.go line 363 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Probably want to in a separate PR move this out / rename this later.

hrm, are you ok that I make the ImportTypeResolver public temporarily in this PR. Once we have the LDR specific type resolver, we can make it private again.

pkg/ccl/crosscluster/producer/replication_manager.go line 163 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

This code is only focused on default / update expressions? Have your tests included that. i.e. Have a cast in your default expression.

ah yes, i hit this panic after i added a default expression in my unit test. I feel like this GetAllReferencedTypeIDs function could be better documented to explain that the getType function only gets called when a column with the UDT in that table has a default expression.

pkg/ccl/crosscluster/producer/replication_manager.go line 170 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Why do we avoid leasing here?

This was copy pasta. We grab the descriptor with the lease now.

pkg/ccl/crosscluster/logical/logical_replication_job_test.go line 1974 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

To hit some of the code paths above we can have fun and add default expressions which use the type.

Done.

…decoding This hydrates the source side table descriptors with their referencing type descriptors if they have any. This allows ldr ingestion to properly decode tables with user defined types. Note that LDR will still fast fail on replicating a table with user defined types (unless the SKIP SCHEMA CHECK OPTION is used) because we still need to teach LDR to: - validate that the source and destination enum's are _physically_ identical (contain the same map from int to enum values) - support UDT replication on sql path, which currently fails because the passed in datum points to the source side udt instead of the destination udt. As another step down the line, we should relax the requirement that UDTs must physically equivalent. Instead, they only need to be _logically_ equivalent (i.e. have the same user facing enum values). Supporting this requires remapping the ints from two logically equivalent enums. Epic: none Release note: none

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @fqazi, and @rafiss)

pkg/sql/importer/import_type_resolver.go line 55 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I think this is probably the better option. We can probably make this more generic implementation more generic. i.e. Extend it so that it can rewrite OIDs upon hydration as a feature, which only LDR needs.

Or implement a LDR specific version on top with that logic.

wrote up this issue and assigned it to you #132164

fqazi

Nice work!

Reviewed 7 of 7 files at r4, 3 of 3 files at r5, 3 of 4 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt and @rafiss)

pkg/ccl/crosscluster/logical/logical_replication_job.go line 363 at r3 (raw file):

Previously, msbutler (Michael Butler) wrote…

hrm, are you ok that I make the ImportTypeResolver public temporarily in this PR. Once we have the LDR specific type resolver, we can make it private again.

Done.

Yeah, I'm good with making this public. And then come up with a clean interface later.

This will allow us to do type descriptor validation when we check for replicating table equivalency. Epic: none Release note: none

fqazi

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @msbutler, and @rafiss)

pkg/ccl/crosscluster/producer/replication_manager.go line 147 at r8 (raw file):

) ([]descpb.TypeDescriptor, map[descpb.ID]struct{}, error) {
	descriptors := txn.Descriptors()
	dbDesc, err := descriptors.MutableByID(txn.KV()).Database(ctx, td.GetParentID())

We can just get a leased one for the database descriptor, since I don't think these get modified.

pkg/ccl/crosscluster/producer/replication_manager.go line 165 at r8 (raw file):

		foundTypeDescriptors[typeID] = struct{}{}

		typeDesc, err := descriptors.MutableByID(txn.KV()).Type(ctx, typeID)

If we don't intend to write these back out and just want a modifiable copy, we can use a leased descriptor and the builder API. Just something to consider later, since it will avoid an unnecessary KV roundtrip to get the descriptor.

msbutler · 2024-10-08T14:12:16Z

TFTR!

bors=fqazi

msbutler · 2024-10-08T15:51:49Z

bors r+

rafiss · 2024-10-08T16:01:58Z

pkg/ccl/crosscluster/logical/logical_replication_job_test.go

+	dbB.Exec(t, "INSERT INTO data VALUES (2)")
+
+	var jobAID jobspb.JobID
+	dbA.QueryRow(t, "CREATE LOGICAL REPLICATION STREAM FROM TABLE data ON $1 INTO TABLE data with skip schema check", dbBURL.String()).Scan(&jobAID)


should we remove the check from here:

cockroach/pkg/sql/catalog/tabledesc/logical_replication_helpers.go

Lines 163 to 168 in 9f61a38

if dstCol.Type.UserDefined() {

return errors.Newf(

"destination table %s column %s has user-defined type %s",

dst.Name, dstCol.Name, dstCol.Type.SQLStringForError(),

)

}

that way we don't need the skip schema check

i think we should remove it once we fix this issue #132167

craig · 2024-10-08T16:38:23Z

Build succeeded:

msbutler mentioned this pull request Sep 20, 2024

crosscluster/logical: support replicating tables with columns that have a user defined type #131131

Closed

msbutler self-assigned this Sep 30, 2024

msbutler force-pushed the butler-udt-check branch 2 times, most recently from 8ba5e8b to 3c8a2b1 Compare September 30, 2024 20:50

importer: make import type resolver public

89a0f7b

Epic: none Release note: none

msbutler force-pushed the butler-udt-check branch 2 times, most recently from 313ec88 to afcc0fc Compare October 4, 2024 19:46

msbutler changed the title ~~Butler udt check~~ crosscluster/logical: hydrate table descriptors with types for event decoding Oct 4, 2024

msbutler commented Oct 4, 2024

View reviewed changes

msbutler marked this pull request as ready for review October 4, 2024 20:08

msbutler requested review from a team as code owners October 4, 2024 20:08

msbutler requested review from azhu-crl, mw5h, dt, rafiss and fqazi and removed request for a team, azhu-crl and mw5h October 4, 2024 20:08

fqazi requested changes Oct 7, 2024

View reviewed changes

crosscluster/producer: grab type descriptors during LDR physical plan…

5309287

…ning Epic: none Release note: none

msbutler force-pushed the butler-udt-check branch from afcc0fc to 56483c6 Compare October 8, 2024 12:34

msbutler commented Oct 8, 2024

View reviewed changes

msbutler force-pushed the butler-udt-check branch from 56483c6 to 849d013 Compare October 8, 2024 12:38

msbutler mentioned this pull request Oct 8, 2024

eval: create TypeResolver that can hydrate a Table with foreign type descriptors #132164

Closed

msbutler commented Oct 8, 2024

View reviewed changes

fqazi approved these changes Oct 8, 2024

View reviewed changes

crosscluster/logical: get type descriptors during planning

8d105a0

This will allow us to do type descriptor validation when we check for replicating table equivalency. Epic: none Release note: none

fqazi approved these changes Oct 8, 2024

View reviewed changes

msbutler mentioned this pull request Oct 8, 2024

crosscluster/logical: validate UDT equivalency in LDR planning #132167

Closed

rafiss reviewed Oct 8, 2024

View reviewed changes

craig bot merged commit d9d2abb into cockroachdb:master Oct 8, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crosscluster/logical: hydrate table descriptors with types for event decoding #131130

crosscluster/logical: hydrate table descriptors with types for event decoding #131130

msbutler commented Sep 20, 2024 •

edited

Loading

blathers-crl bot commented Sep 20, 2024

cockroach-teamcity commented Sep 20, 2024

msbutler Oct 4, 2024

msbutler Oct 7, 2024 •

edited

Loading

msbutler Oct 4, 2024

msbutler Oct 4, 2024

fqazi left a comment

msbutler left a comment

msbutler left a comment

fqazi left a comment

fqazi left a comment

msbutler commented Oct 8, 2024

msbutler commented Oct 8, 2024

rafiss Oct 8, 2024

msbutler Oct 8, 2024

craig bot commented Oct 8, 2024

	if dstCol.Type.UserDefined() {
	return errors.Newf(
	"destination table %s column %s has user-defined type %s",
	dst.Name, dstCol.Name, dstCol.Type.SQLStringForError(),
	)
	}

crosscluster/logical: hydrate table descriptors with types for event decoding #131130

crosscluster/logical: hydrate table descriptors with types for event decoding #131130

Conversation

msbutler commented Sep 20, 2024 • edited Loading

blathers-crl bot commented Sep 20, 2024

cockroach-teamcity commented Sep 20, 2024

msbutler Oct 4, 2024

Choose a reason for hiding this comment

msbutler Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

msbutler Oct 4, 2024

Choose a reason for hiding this comment

msbutler Oct 4, 2024

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

msbutler commented Oct 8, 2024

msbutler commented Oct 8, 2024

rafiss Oct 8, 2024

Choose a reason for hiding this comment

msbutler Oct 8, 2024

Choose a reason for hiding this comment

craig bot commented Oct 8, 2024

msbutler commented Sep 20, 2024 •

edited

Loading

msbutler Oct 7, 2024 •

edited

Loading