Closed ThomasLohmann closed 5 years ago
Thanks Thomas!
@ThomasLohmann I'm fixing the proximate problem (the one in the error message you observe) in #22314. Once that is merged, we'd welcome the results of any further testing -- in case of further errors please post them here in follow-up comments.
I just tested this out and with https://github.com/cockroachdb/cockroach/pull/22323 in place, the TypeORM demo seems to be working! There's probably more work to be done with this, but that's a good start.
.... and now we're stuck on https://github.com/cockroachdb/cockroach/issues/9851.
Hi there,
any news about that issue?
Kind regards, Thomas
cc @awoods187 perhaps you can chime in?
We are investigating pg compatibility errors like this for 2.1. More to come.
@awoods187 what are your release plans for 2.1?
@awoods187 the issue here is specifically the ability to change the data type of a column with ALTER.
@ThomasLohmann we are in the process of finalizing it now. Do you have any requests that you'd like for me to be aware of?
@awoods187 not for the moment. It would just be great if we find a solution for this problem as fast as possible.
CockroachDB 2.0 has just been released I think there is a lot of community interest in getting this added to TypeORM, especially now that JSON is supported.
@awoods187 and @knz any news on that issue?
Kind regards, Thomas
Hi Thomas! We have solved a small proximate issue introduced in the 2.0 release, fix will be available in 2.0.1 or 2.0.2 (#24475). The next main roadblock is the ability to change the type of an existing column with ALTER. We have initiated work on this feature (#24703); we are currently targeting the 2.1 release to publish this support, although you may have access to it in earlier alpha releases.
Sounds good!
Update: @nvanbenschoten and I looked into this. The requirement to use ALTER SET TYPE was a red herring. The reason why TypeORM wants ALTER SET TYPE is that it sees a discrepancy between the schema specified in the entity and the schema reported by information_schema
, so it wants to correct this discrepancy with ALTER COLUMN SET TYPE.
So adding support for ALTER SET TYPE would not be a good solution: a TypeORM app would then cause an expensive, wasteful column type change on every startup, whereas the correct solution is to make it satisfied that the current schema in-db is the one expected for the entities.
Will investigate further to see where the discrepancy crops up.
So, TypeORM support works if you don't use migrations?
I have tested this again with the upcoming CockroachDB 2.1.
There are still issues.
Starting with the quick start tutorial in the TypeORM docs, which defines a "User" entity with 4 fields.
1) Upon startup TypeORM inspects the existing schema no matter what, and that fails outright: it wants the columns udt_xxx
in information_schema.columns
which are not yet implemented. #31101
I have created a branch of CockroachDB to add that: https://github.com/knz/cockroach/tree/20181008-typeorm-tweaks
2) Then I went further. To create an entity with @PrimaryGeneratedColumn
it wants to create a sequence using the OWNED BY syntax, not yet supported by CockroachDB (#26382).
I changed my branch to recognize (but ignore) the OWNED BY.
Then the entity creation succeeded, and the test program completed once.
Then upon restart it will check whether the in-DB entity matches the definitions, and we get more failures.
The direct next symptom is TypeORM trying to drop the primary key constraint (which is not yet supported by cockroachdb). However the reason why it wants to do that is that it does not recognize the current type of the column in-DB as being the same as the one specified in the entity. Really the problem is as follows.
3) The first time around the primary column was created with the default type, which is postgres' INT (and was translated to SERIAL). TypeORM expects (correctly) that SERIAL/INT reflects in information_schema as INT4 like in postgres. This is not yet the case in CockroachDB (#26925).
So I manually changed the entity definition from @PrimaryGeneratedColumn
to @PrimaryGeneratedColumn({type:"int8"})
so that the actual type matches the expected type.
This makes progress but then the next error occurs: it tries to create the sequence for the entity PK again (new CREATE SEQUENCE) and that fails with an error "a sequence with that name already exists" (which it does indeed).
The reason why this occurs is that the first time around the sequence was meant to be "attached" to the column using OWNED BY. The second time around, TypeORM inspects "what are all the sequences attached to this column" and finds nothing (because CockroachDB ignored the OWNED BY column), so it thinks the sequence does not exist yet and tries to create it. Which fails because it does exist, is just not visible via introspection. Hence the next problem statement:
4) Sequence created with OWNED BY must be visible via pg_catalog/information_schema as "hanging off" the proper columns. I put a comment on the other issue about that: https://github.com/cockroachdb/cockroach/issues/26382#issuecomment-427964230
Here I gave up with the sequence stuff and I changed @PrimaryGeneratedColumn
to @PrimaryColumn
.
This makes the problems with the "id" column upon 2nd start disappear (Good!) but then we're on to the next problem:
error message: 'column "firstName" being dropped, try again later'
upon statement ALTER TABLE "user" ADD "firstName" character varying NOT NULL
ie TypeORM is trying to drop and re-add the column firstName.
This should not be necessary! Again TypeORM is confused about an apparent mismatch between the actual type and desired type.
I thought this was because my first prototype implementation of udt_name
was incorrect and was transforming "varchar" into "text" (TypeORM expects it to stay). However even after I changed my branch accordingly the error persisted.
Then I found out that TypeORM inspects the types not directly but via the ::REGTYPE
pseudo-cast.
This is also incorrect in that it transforms "varchar" into "text"!!
5) The ::REGTYPE cast does not suitably preserve the OID distinction between type aliases (this is a newly found issue! #31102)
The error is this piece of code:
colType, err := ctx.Planner.ParseType(s)
if err == nil {
datumType := coltypes.CastTargetToDatumType(colType)
return &DOid{semanticType: typ, DInt: DInt(datumType.Oid()), name: datumType.SQLName()}, nil
}
(CastTargetToDatumType is not information-preserving for string type aliases)
I tried to shuffle this code around to try and preserve this information (see my branch, but my patch is likely incorrect!!) however I did not get any success.
So instead I changed the column types for firstName/lastName in the entity definition to be "text" (instead of the inferred default "varchar"):
@Column("text")
firstName: string;
@Column("text")
lastName: string;
Then there is no error related to these two columns any more but then I get a similar problem for the remaining "age" column which is supposed to have type "int" (int4) but gets assigned type int8 by the database. This is the same as point 3 above (and issue #26925).
So instead I changed the column "age" to have type "numeric".
!!!!!!AND THEN THIS WORKS!!!!!!!
Yay.
So in summary:
CockroachDB must be extended to support udt_xxx
columns in information_schema.columns
as a prerequisite. Maybe the changes in https://github.com/knz/cockroach/tree/20181008-typeorm-tweaks are sufficient, maybe they're not. This needs to be investigated.
@PrimaryGeneratedColumn
does not work because of #26382 and #26382, but @PrimaryColumn
does, .
the automatic type selection by TypeORM does not survive introspection on the 2nd start. The types must be A) selected explicitly B) only the "main" type supported by CockroachDB will work (i.e. use "text" not "varchar", "numeric" not "decimal", "float8" not "float4" etc). These are newly found issues in CockroachDB which will need to be addressed for compatibility.
I also think that 99% of these problems would disappear by creating a suitable CockroachDB driver for TypeORM.
Here is the summary of past and recent findings:
udt_
columns in information_schema
https://github.com/cockroachdb/cockroach/issues/31101 - possible resolution:
data_type
is sufficient, otherwise crdb_sql_type
)udt_
columns in crdb, this may help with other 3rd party compat too::REGTYPE
cast which is improperly supported by CockroachDB https://github.com/cockroachdb/cockroach/issues/31102 - suggested resolution:
::REGTYPE
and instead query information_schema
directly ::regtype
in crdb::regtype
is broken, for example tell it to expect the type text
when the javascript type is string
(it would expect varchar
otherwise). See hint in my previous comment above.Hi @knz! I will describe our preliminary decisions for each item.
We used crdb_sql_type
instead of data_type
. This is due to the fact that CRDB internally converts STRING
data type to text
. TypeORM converts user specified data types into database internal types. If user defines column as @Column({ type: "string", length: 30 })
, TypeORM tries to convert it into text(30)
, but fails because text
data type does not have length
property. So we changed this behaviour and TypeORM rely on real CRDB types instead of aliases. E.g. text
data type converts to STRING
. Btw I discovered that column collation also stored in crdb_sql_type
- it makes it more diffucult to work with collation.
Since TypeORM’s @PrimaryGeneratedColumn
uses autoincrement (e.g. sequence) we are planning to keep this functionality using DEFAULT nextval('sequence')
. We are also introducing a new mode to this decorator - @PrimaryGeneratedColumn("rowid")
that is going to use your rowid generation strategy
Here we decided to stick with the following strategy:
user defines property with type number
@Column()
id: number;
column stored in int
data type and parsed to number when retrieved from database.
otherwise, user defines column with int
or any other numeric data type and string
property type
@Column({ type: "int" })
id: string;
column stored in specified data type in database and returned as a string
unique_rowid
columns also uses string
property type.
We made synchronization process without using ::REGTYPE
1. TypeORM tries to convert it into `text(30)`, but fails because `text` data type does not have `length` property.
can you explain this a bit more?
For example, create a table
CREATE TABLE "test" ("t1" text, "t2" string)
Then we will see how it looks in information_schema
SELECT "column_name", "data_type", "crdb_sql_type" from "information_schema"."columns" WHERE "table_name" = 'test'
column_name | data_type | crdb_sql_type |
---|---|---|
t1 | text | STRING |
t2 | text | STRING |
When user defines above table in TypeORM it looks like
@Column({ type: "text" })
t1: string;
@Column({ type: "string" })
t2: string;
At the beginning of the work, I used data_type
column to get column type for further synchronization. It means that TypeORM translates both column type into one text
type and create table
query looks like
CREATE TABLE "test" ("t1" text, "t2" text)
But when user specifies column length (for example 30 for both columns), TypeORM generates following query
CREATE TABLE "test" ("t1" text(30), "t2" text(30))
and fails with error ERROR: syntax error at or near "("
because CRDB does not support length definition on text
data type.
Based on this, now we use crdb_sql_type
to get the column type. In the specified example TypeORM translates both column types into string
type. And table creation looks like
CREATE TABLE "test" ("t1" string(30), "t2" string(30))
Thanks for explaining.
Can you also explain what makes it difficult with collated strings?
"name" string COLLATE "en_US"
stores as STRING COLLATE en_US
"name" string(30) COLLATE "en_US"
stores as STRING(30) COLLATE en_US
it brings little difficulty getting column collation, unlike postgres which stores collation in separate column named column_collation
Oh I see. Thanks for explaining.
I filed this separate issue for that: #34831
btw, I discovered that pg_tables
in addition to tables, also stores sequences
CREATE SEQUENCE "post_key_seq"
CREATE TABLE "post" ("key" INT DEFAULT nextval('"post_key_seq"') NOT NULL)
SELECT * FROM "pg_tables" WHERE "schemaname" = 'public'
Is it a bug or it is by design ?
This is not by design, no. I filed #34856 and fixing in #34857 and #34858. Feel free to pick either of those patches in your crdb branch.
@knz Hi everyone,
TypeORM released an offical example with CockroachDB a few hours ago as you can see here:
https://github.com/typeorm/cockroachdb-example
or here is the closed issue ("pleerock" is the founder of TypeORM):
https://github.com/typeorm/typeorm/issues/1040
Maybe you can add the example to your offical Docs as an alternative to Sequelize.js here:
https://www.cockroachlabs.com/docs/stable/build-a-nodejs-app-with-cockroachdb-sequelize.html
It would be really nice.
I think after more than one year, we can finally close this issue.
Kind regards, Thomas
@ThomasLohmann we're still iterating on QAing these changes. We'll come back to this when we have asserted the result is of sufficient maturity to be advertised. Thanks!
Hi @AlexMesser,
upon advice from Umed we downloaded a copy of https://github.com/typeorm/cockroachdb-example
We ran the following.
npm i
npm start
- this runs fine, the first timenpm start
again - this fails with:Error: { QueryFailedError: cannot drop primary key
at new QueryFailedError (/var/home/kena/src/cockroachdb-example/node_modules/typeorm/error/QueryFailedError.js:11:28)
at Query.callback (/var/home/kena/src/cockroachdb-example/node_modules/typeorm/driver/cockroachdb/CockroachQueryRunner.js:174:38)
at Query.handleError (/var/home/kena/src/cockroachdb-example/node_modules/pg/lib/query.js:142:17)
at Connection.connectedErrorMessageHandler (/var/home/kena/src/cockroachdb-example/node_modules/pg/lib/client.js:160:17)
at emitOne (events.js:116:13)
at Connection.emit (events.js:211:7)
at Socket.<anonymous> (/var/home/kena/src/cockroachdb-example/node_modules/pg/lib/connection.js:125:12)
at emitOne (events.js:116:13)
at Socket.emit (events.js:211:7)
at addChunk (_stream_readable.js:263:12)
message: 'cannot drop primary key',
name: 'QueryFailedError',
length: 44,
severity: 'ERROR',
code: 'XX000',
detail: undefined,
hint: undefined,
position: undefined,
internalPosition: undefined,
internalQuery: undefined,
where: undefined,
schema: undefined,
table: undefined,
column: undefined,
dataType: undefined,
constraint: undefined,
file: undefined,
line: undefined,
routine: undefined,
query: 'ALTER TABLE "category" DROP CONSTRAINT "PK_9c4e4a89e3674fc9f382d733f03"',
parameters: [] }
Can you clarify further what's happening here?
I have used the latest version in our main repository master
, build v19.1.0-beta.20190225-213-g71681f6003
.
Thank you
@knz can you please run against 2.1.5
? Since its the latest stable version we were developing against. It feels like master has some breaking changes causing you this error.
Good idea, that indeed yields a difference in behavior.
@bobvawter could you get the SQL exec traces on both crdb versions and point out where they diverge?
Hey @knz, did you have a chance to make a complete test of TypeORM with a stable 2.1.5
version?
Can you explain what "a complete test" is? At this point we're just looking at the example provided above: https://github.com/typeorm/cockroachdb-example
Do you have a more exhaustive test you'd like us to look at instead?
okay, let me rephrase it.
We have landed CockroachDB support in TypeORM. Everything works except something we have postponed for the future (email discussion). Example I provided is just a start point in the case if you want to start checking results. What's left to do from our side?
@pleerock you have pointed out a difference in behavior between CockroachDB 2.1 and 19.1. I confirm we also see a difference, and we are still determining where it comes from.
There are really two situations possible:
Unfortunately as of today I do not know which of the two points applies. Give us a little more time to investigate further.
I've confirmed that 2.1.5 works correctly with the most recent release of typeorm. The current beta of cockroach fails due to the addition of the domain_catalog
, domain_schema
, and domain_name
columns in the information_schema.columns
table, which we added since the 2.1 release.
I take it back. While that is a difference, it isn't the cause of this problem. So far in my testing it's non-deterministic, and sometimes errors with the above error.
Some progress: I think this is caused by our handling of the int
type. The above error is caused because on the second run, typeorm sees a column type of columns.id
as int8
but wants int
. Since the type is wrong it tries to drop and add the column with the correct type, but we don't support changing the PK. In cockroach int
is int8
, but we are going to make int
be int4
in the release after the upcoming release (see #32848). Still unsure why this was working with cockroach 2.1.
It's possible that the best solution for this is for typeorm to run set default_int_size = 4
at the beginning of each session.
In 2.1, int
is a wholly separate type from int4
or int8
. If you ask for int
, you'll see int
in the reflection and it happens to be an 8-byte value.
In 19.1, we completely removed the "naked" int
type by treating it, at parse time, as an alias either for int8
or int4
. This is how we handle the float
type, it's always resolved as float8
. We chose the aliasing approach so that it's unambiguous as to what size size int
you're using in cluster that have clients using different sizes. Otherwise, the introspection would have to switch the presentation based on the user's original intent and the client's default_int_size
setting. This would have created a combinatorial mess.
I guess the question is whether or not it's correct for a client to want to change a column from intX
to int
, given that the int
size has an implementation-dependent size, IIRC. If a client sets default_int_size=4
, then it would still see int4
for the column type. Is TypeORM looking for int
specifically, or is it switching on the declared vs. expected size? What does it do for the 'float' type since we always resolve that to float8
?
Ok now I understand fully. TypeORM issues a create table
with type int
. In cockroach 2.1 the int
acted like an int8
but would display itself as the type int
, which appeased typeorm. In 19.1 we always change int
to something. Since the default is int8
, typeorm complains upon second invocation. Some possible solutions:
set default_int_size = 4
at the start of each session started by typeorm.int8
or int4
and never int
during its type stuff (see https://github.com/typeorm/typeorm/blob/a12e72d417638ea8fba2ca672d5acfaaaaf691c9/src/driver/cockroachdb/CockroachDriver.ts#L421 for example).Cockroach is going to fix this, but not until the release in later half of this year, because we decided to allow people 1 release to deal with this if they wanted (see the PR I linked above for more).
We have applied suggested fix in the latest typeorm version (0.2.15
) and now everything seemed to work and tests are passing under latest master
version.
You can continue testing TypeORM's CockroachDB support with the latest cockroachdb's master now.
We have one issue however. For some reason CI randomly fails on a random test with following error:
restart transaction: TransactionRetryWithProtoRefreshError: TransactionRetryError: retry txn (RETRY_SERIALIZABLE): "sql txn" id=bc254db7 key=/Table/SystemConfigSpan/Start rw=true pri=0.08168281 stat=PENDING epo=0 ts=1552496631.167807787,1 orig=1552496631.114934486,0 max=1552496631.114934486,0 wto=false seq=80`
Sometimes tests successfully pass, sometimes they throw this error. Fail example on CI. Any hint on why we have this error is appreciated. We are using the latest docker image from CRDB-unstable hub.
Also side note. We have found your latest docker development image doesn't match the latest master and some changes are missing in there:
(not sure how often master is released for docker, but what I imagine they should go in parallel)
Hi all--you will need to setup a transaction retry loop as covered by these docs https://www.cockroachlabs.com/docs/v2.1/transactions.html#transaction-retries. Let us know if you have questions.
We applied transaction retries to the latest version (I released changes in typeorm@0.2.16-rc.1
). All tests are green now.
Amazing news! We will run a final set of internal tests tomorrow--thanks for all your hard work!
I also filed https://github.com/cockroachdb/cockroach/issues/35876 to make sure we don't accidentally change anything that would impact TypeORM.
Everything looks good on our end--we will try it out with some customers. I'm going to close this issue and will file new ones if they run into any problems!
Hi there,
does anyone have an idea how to get TypeORM running with CockroachDB? I tried it out, but it doesn’t work. It would be really great if we find a solution, because Sequelize is bad compared to TypeORM. Furthermore, TypeScript is now state of the art on the backend.
TypeORM Link: http://typeorm.io/#/
Here is what I have tried so far and my error:
My TypeORM config:
I followed just the instructions by CockroachDB from here:
https://www.cockroachlabs.com/docs/stable/build-a-nodejs-app-with-cockroachdb.html
And my folder structure is nothing more than the TypeORM starter project:
npm instruction: typeorm init --name TypeORM --database postgres --express
If I just switch the config to my postgres database everything works.