sql: add user-defined composite types by jordanlewis · Pull Request #90491 · cockroachdb/cockroach

jordanlewis · 2022-10-21T22:04:22Z

This commit adds user-defined composite types, which are created with
the CREATE TYPE t AS (field1 type1, field2 type2, ...) syntax.

See documentation here:
https://www.postgresql.org/docs/current/rowtypes.html

It permits this kind of thing:

CREATE TYPE t AS (a INT, b INT);
CREATE TABLE a (a t);
INSERT INTO a VALUES((1, 2))
SELECT (a).b FROM a

Release note (sql change): add user-defined composite column types

cockroach-teamcity · 2022-10-21T22:04:28Z

This change is

rafiss · 2022-10-21T23:10:46Z

super cool!

two idle comments before a full review:

let's make sure to add a new cluster version and version gate this.
do you know how exactly these differ from the implicit table record types in postgres? i was wondering if we could implement them in the exact same way we've already implemented implicit table types, with the addition of marking the table descriptor as unusable. maybe there would be an issue with privileges?

jordanlewis · 2022-10-24T13:36:56Z

let's make sure to add a new cluster version and version gate this.

Definitely.

do you know how exactly these differ from the implicit table record types in postgres?

There are a couple of differences, but they're pretty minor. It's telling that in Postgres, a UDT appears in pg_class. It does strike me that maybe we could do it by just creating a table that you can't insert to, and proxying all the alter statements into alter table statements, but I'm not sure it's a good idea - it seems like it might make existing code more complicated rather than adding new code that's separate.

I'm curious what @ajwerner thinks as the majority of the code here is in schema world but my instinct is that we should model it as a new type kind just like enum.

ajwerner · 2022-10-24T13:58:37Z

I'm curious what @ajwerner thinks as the majority of the code here is in schema world but my instinct is that we should model it as a new type kind just like enum.

That matches my expectation. I can imagine a future migration to add such types for tables too.

jordanlewis · 2022-10-25T14:12:54Z

I'm feeling relatively good about contents of this PR, if someone has a chance to review I'd appreciate it. It was nice to learn a little bit about the new schema changer - once you get the hang of it, it seems to do an impressive job of abstracting away the underlying complexity. Or, perhaps I just had blinders on :)

I'm not sure whether I did the dependency tracking right, and I'm sure it's missing a bunch of other stuff as well.

jordanlewis · 2022-10-25T14:13:19Z

Also the PR is fairly large, say the word and I'll try to chunk it up a bit.

jordanlewis · 2022-10-25T14:16:22Z

Actually, I don't think I have done anything about backup and restore in this patch, so it is probably not complete :)

chengxiong-ruan · 2022-10-25T18:57:19Z

Nice work. Maybe it's just me but I found myself had to do quite a bit of context switching while going through the PR briefly. I'd appreciate if you could at least split the PR into several commits : 1. front-end change (parser), 2. legacy schema changer support, 3. fix hydration logic, 4. declarative schema changer support (this worth a separate PR, I think).

jordanlewis · 2022-10-25T19:28:00Z

Thanks @chengxiong-ruan I will split it up. Question though, how do I avoid seeing failing tests on the intermediate commits? In particular maybe I need to find a way to tell the declarative schema changer to skip handling DROP TYPE in the case of composite types? I usually like each commit to pass all the tests but maybe that's not a realistic hope at this point.

chengxiong-ruan · 2022-10-25T19:47:47Z

Thanks @chengxiong-ruan I will split it up. Question though, how do I avoid seeing failing tests on the intermediate commits? In particular maybe I need to find a way to tell the declarative schema changer to skip handling DROP TYPE in the case of composite types? I usually like each commit to pass all the tests but maybe that's not a realistic hope at this point.

Ah, yeah, making CI passed on each commit is hard heh. I think in this case for the declarative schema changer, you could raise an unimplemented error like this so that it can fall back to the legacy schema changer.

jordanlewis · 2022-10-26T13:52:13Z

@chengxiong-ruan I have removed the declarative schema changer implementation from this PR and split into 2 commits, one for the parser and one for the rest. I wasn't sure what you meant by fixing the hydration so please let me know if this is helpful, or if you have additional split requests.

chengxiong-ruan

LGTM!

jordanlewis · 2022-11-16T17:29:22Z

@postamar PTAL, I have:

Banned composite types that reference other UDTs (sql: support user-defined types within user-defined composite types #91779)
Added the indicated missing tests
Added commented-out tests for the bug sql: old schema changer doesn't handle type back references properly in alter column #91972

rafiss

does this support arrays of composite types?

Release note: None

jordanlewis · 2022-12-06T06:01:56Z

does this support arrays of composite types?

Yes.

rafiss

can there be tests for arrays of composite types?

postamar · 2022-12-08T15:15:08Z

Do we want this to be a table ID instead? Zero meaning it's not an implicit record type, and vice-versa.

I don't see a use case for recording that table ID, so I guess not? :) We can easily add it if need be.

postamar · 2022-12-08T15:17:45Z

Is the schema changer support PR ready for review? I'm just being mindful of accruing technical debt so to be honest I'd rather this change were part of this PR, but won't insist on it. Hopefully removing this scerrors.NotImplementedErrorf error should be quite easy at this point.

Yes, it's ready for review. I moved it out of the PR at @chengxiong-ruan suggestion to keep things small. At this point I do not want to re-add it - can we get this one merged and then I'll work on the other one immediately?

That's fine by me, thanks! This need to change a zillion things everytime one touches anything in schema-land is sadly a fairly typical feature of our development experience.

postamar · 2022-12-08T15:29:56Z

nit: consider using catalog.DescriptorIDSet here.

Done, but for my edification, why? Doesn't IDClosure already dedupe the IDs here? I don't see what it's adding.

Sorry, I overlooked the fact that .GetIDClosure() returned a map and not a slice (or even better, a catalog.DescriptorIDSet). My bad!

postamar · 2022-12-08T15:32:07Z

Should we wrap the error in something which references the label? Seems like things could get confusing otherwise.

We don't seem to ever wrap the result error from ResolveType in other spots, even in places like CREATE TABLE, so I guess I don't see why this one is special - I'll leave it alone.

I didn't realize that. Fair enough. Let's stay consistent then.

postamar · 2022-12-08T15:36:26Z

Just curious: is this hydration logic exercised at all now that we can't reference user-defined types within composite types?

Yeah, it'll no-op. But, there's no harm in leaving it, right? We should always run hydration on types that are pulled from disk, everywhere - better to be consistent about that than have to remember to re-add it later when we resolve #91779.

Agreed in this case.

jordanlewis

Thanks for the review! PTAL.

jordanlewis · 2022-12-09T20:50:40Z

Yeah, it'll no-op. But, there's no harm in leaving it, right? We should always run hydration on types that are pulled from disk, everywhere - better to be consistent about that than have to remember to re-add it later when we resolve #91779.

jordanlewis · 2022-12-09T20:56:15Z

We don't seem to ever wrap the result error from ResolveType in other spots, even in places like CREATE TABLE, so I guess I don't see why this one is special - I'll leave it alone.

jordanlewis · 2022-12-09T21:06:44Z

It's not a correctness bug the way we define it - errors where there shouldn't be errors are not correctness bugs.

The reason this is broken is a type system issue that's unrelated to composite types. I filed #93349 for it but I don't agree that it's a showstopper - it's a classic example of CockroachDB's type system differences from Postgres, and is correctable by adding casts. I've improved the comments here.

Here's an example of the existing broken-ness:

demo@127.0.0.1:26257/defaultdb> values((1, 2)), ((1, null)); ERROR: VALUES types tuple{int, unknown} and tuple{int, int} cannot be matched SQLSTATE: 42804

jordanlewis · 2022-12-09T21:09:01Z

Yes, it's ready for review. I moved it out of the PR at @chengxiong-ruan suggestion to keep things small. At this point I do not want to re-add it - can we get this one merged and then I'll work on the other one immediately?

jordanlewis · 2022-12-09T21:11:41Z

I don't see a use case for recording that table ID, so I guess not? :) We can easily add it if need be.

This commit adds user-defined composite types, which are created with the `CREATE TYPE t AS (field1 type1, field2 type2, ...)` syntax. See documentation here: https://www.postgresql.org/docs/current/rowtypes.html It permits this kind of thing: ``` CREATE TYPE t AS (a INT, b INT); CREATE TABLE a (a t); INSERT INTO a VALUES((1, 2)) SELECT (a).b FROM a ``` Release note (sql change): add user-defined composite column types

postamar

LGTM. Thanks for doing all of this work!

@rafiss arrays of composite types get exercised in this PR in the new composite_types logic test suite, but did you have something else in mind?

jordanlewis · 2022-12-12T15:33:53Z

TFTR!

I added the test suite at @rafiss suggestion 😅

jordanlewis · 2022-12-12T16:41:37Z

Woohoo! Thanks for the reviews 😁

bors r+

craig · 2022-12-12T17:32:50Z

Build succeeded:

Bazel Essential CI (Cockroach)

dvarrazzo · 2022-12-12T19:42:17Z

Hello, just stumbled on thia. Psycopg 2 and 3's test suites have an amount of tests involving composite types. Did you try to un-skip them for the crdb version supporting this feature and give them a go? 🙂

jordanlewis · 2022-12-12T20:03:00Z

Hey @dvarrazzo, I didn't yet - if I understand our test infrastructure properly, the nightly tests will automatically exercise this tonight and show us which ones are now passing that should be marked as failed. I'll make sure I have the right idea with @rafiss though :-)

rafiss · 2022-12-12T20:08:32Z

We previously worked with @dvarrazzo to test CRDB in the psycopg CI. Example:

There's a helper in the test suite that skips certain tests that don't work on CRDB: https://github.com/psycopg/psycopg2/blob/e8d92b74fd4b219cb4725cb906c526f3f2dce31d/tests/testutils.py#L426

So we can try it out and make a commit to psycopg if more tests are passing (conditional on using CRDB >= 23.1)

jordanlewis force-pushed the udct branch 4 times, most recently from a19be20 to acf0bc6 Compare October 25, 2022 03:34

jordanlewis changed the title ~~[WIP] sql: add user-defined composite types~~ sql: add user-defined composite types Oct 25, 2022

jordanlewis marked this pull request as ready for review October 25, 2022 14:09

jordanlewis requested a review from a team as a code owner October 25, 2022 14:09

jordanlewis requested review from a team and benbardin and removed request for a team October 25, 2022 14:09

jordanlewis force-pushed the udct branch from acf0bc6 to 434e89e Compare October 26, 2022 13:50

jordanlewis mentioned this pull request Oct 26, 2022

schemachanger: implement composite types #90698

Merged

jordanlewis requested a review from chengxiong-ruan October 26, 2022 13:53

chengxiong-ruan approved these changes Oct 26, 2022

View reviewed changes

Comment thread pkg/sql/parser/sql.y Outdated

Comment thread pkg/sql/catalog/typedesc/type_desc.go Outdated

jordanlewis force-pushed the udct branch 3 times, most recently from 7385453 to 68fa7b4 Compare October 26, 2022 22:24

jordanlewis force-pushed the udct branch from 6e45113 to f70e150 Compare November 16, 2022 16:46

jordanlewis requested a review from postamar November 16, 2022 17:29

rafiss reviewed Dec 5, 2022

View reviewed changes

parser: add support for user-defined composite types

68aabbc

Release note: None

jordanlewis force-pushed the udct branch from f70e150 to 1a54d3d Compare December 6, 2022 06:01

rafiss requested changes Dec 6, 2022

View reviewed changes

postamar suggested changes Dec 8, 2022

View reviewed changes

jordanlewis force-pushed the udct branch from 1a54d3d to 503a7bb Compare December 9, 2022 21:13

jordanlewis commented Dec 9, 2022

View reviewed changes

jordanlewis force-pushed the udct branch from 503a7bb to f367d27 Compare December 9, 2022 22:17

jordanlewis force-pushed the udct branch from f367d27 to 86ae685 Compare December 9, 2022 22:18

postamar approved these changes Dec 12, 2022

View reviewed changes

rafiss approved these changes Dec 12, 2022

View reviewed changes

craig Bot merged commit aae18c7 into cockroachdb:master Dec 12, 2022

jordanlewis deleted the udct branch December 12, 2022 17:33

shermanCRL mentioned this pull request Dec 12, 2022

changefeedccl: support emitting binary-encoded row data #77849

Open

cockroach-teamcity mentioned this pull request Dec 13, 2022

PR #90491 - sql: add user-defined composite types cockroachdb/docs#15805

Closed

rmloveland mentioned this pull request Apr 14, 2023

Add docs for user-defined composite types cockroachdb/docs#16767

Merged

rafiss mentioned this pull request Sep 6, 2023

catalog: support table record types in table and type definitions #70099

Closed

rafiss mentioned this pull request Jul 17, 2024

workload/schemachange: create composite type in RSW #127185

Merged

Conversation

jordanlewis commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 21, 2022

Uh oh!

rafiss commented Oct 21, 2022

Uh oh!

jordanlewis commented Oct 24, 2022

Uh oh!

ajwerner commented Oct 24, 2022

Uh oh!

jordanlewis commented Oct 25, 2022

Uh oh!

jordanlewis commented Oct 25, 2022

Uh oh!

jordanlewis commented Oct 25, 2022

Uh oh!

chengxiong-ruan commented Oct 25, 2022

Uh oh!

jordanlewis commented Oct 25, 2022

Uh oh!

chengxiong-ruan commented Oct 25, 2022

Uh oh!

jordanlewis commented Oct 26, 2022

Uh oh!

chengxiong-ruan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jordanlewis commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis commented Dec 6, 2022

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jordanlewis commented Oct 21, 2022 •

edited

Loading

jordanlewis commented Nov 16, 2022 •

edited

Loading