Add Cockroach Dialect #12346

cpaczek · 2022-01-29T00:41:37Z

To test this PR here are some commands to get you up and running with a CRDB docker container.

docker volume create roach-single

docker run --name=roach-single -p 26257:26257 -p 8080:8080 -v "roach-single:/cockroach/cockroach-data" cockroachdb/cockroach:latest start-single-node --insecure

Get an SQL shell

docker exec -it roach-single ./cockroach sql --url="postgresql://root@127.0.0.1:26257/defaultdb?sslmode=disable"

Strapi DB Config

  connection: {
    client: 'cockroachdb',
    connection: {
      host: env('DATABASE_HOST', '127.0.0.1'),
      port: env.int('DATABASE_PORT', 26257),
      database: env('DATABASE_NAME', 'defaultdb'),
      user: env('DATABASE_USERNAME', 'root'),
      password: env('DATABASE_PASSWORD', ''),
      ssl: env.bool('DATABASE_SSL', false)
    },
    debug: false, //optional
  },

fixes #11738

codecov · 2022-01-29T00:49:24Z

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (b9e92de) 60.68% compared to head (fcfc414) 60.67%.
Report is 5072 commits behind head on main.

Files	Patch %	Lines
packages/core/database/lib/schema/builder.js	0.00%	2 Missing and 2 partials ⚠️
packages/core/database/lib/schema/diff.js	0.00%	1 Missing and 2 partials ⚠️
packages/core/database/lib/dialects/index.js	0.00%	2 Missing ⚠️
packages/core/database/lib/query/helpers/search.js	0.00%	1 Missing ⚠️
packages/core/database/lib/query/helpers/where.js	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12346      +/-   ##
==========================================
- Coverage   60.68%   60.67%   -0.01%     
==========================================
  Files        1495     1495              
  Lines       36878    36884       +6     
  Branches     7359     7363       +4     
==========================================
  Hits        22380    22380              
- Misses      12417    12421       +4     
- Partials     2081     2083       +2

Flag	Coverage Δ
back	`51.29% <0.00%> (-0.05%)`	⬇️
front	`66.31% <ø> (+<0.01%)`	⬆️
unit_back	`51.29% <0.00%> (-0.05%)`	⬇️
unit_front	`66.31% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kibertoad · 2022-02-01T22:11:23Z

You can use alterType parameter in newer version of knex instead: knex/documentation@20adb5c

cpaczek · 2022-02-01T22:13:14Z

You can use alterType parameter in newer version of knex instead: knex/documentation@20adb5c

Ya was waiting for newest npm publish, which happened 7 mins ago

Still will have to migrate to knex 1.0 for the other dbs

cpaczek · 2022-02-03T04:14:46Z

(This PR is still full of logs but you can use this branch and use yarn link to test it on a Strapi instance)

You can use alterType parameter in newer version of knex instead: knex/documentation@20adb5c

I just migrated to knex 1.0.2 and implemented the alterNullable and that fixed the drop null on id column. (https://github.com/Cpaczek/strapi/blob/8a3b09759c76c80a996f99b76b3f7cdb0ff0debe/packages/core/database/lib/schema/builder.js#L307)

However I am running into a new issue. On knex .95.x our issue outside of the drop nullable was the int -> int on column types now its the following.

I also "implemented" alterType by setting it to false on all alter()

        let alterNullable = false;
        if (updatedColumn.name === 'id') {
          alterNullable = true;
          continue;
        }

        createColumn(tableBuilder, object).alter({ alterNullable, alterType: true });

not sure if I have to selectively change if alterType is true of false depending on the change. this is just a very rough test. Still doing research.

With alterType = false

[2022-02-02 19:50:51.706] error: alter table "admin_permissions_role_links" drop constraint "primary" - constraint "primary" of relation "admin_permissions_role_links" does not exist
error: alter table "admin_permissions_role_links" drop constraint "primary" - constraint "primary" of relation "admin_permissions_role_links" does not exist

With alterType = true

[2022-02-02 19:55:11.954] error: alter table "admin_permissions" alter column "created_by_id" type integer using ("created_by_id"::integer) - unimplemented: ALTER COLUMN TYPE for a column that has a constraint is currently not supported
error: alter table "admin_permissions" alter column "created_by_id" type integer using ("created_by_id"::integer) - unimplemented: ALTER COLUMN TYPE for a column that has a constraint is currently not supported

With it set to true looks like its trying to use a cdb experimental feature: Experimental alter column in use, see issue: cockroachdb/cockroach#49329

What I think may be the root of the problem is this:

The errors above happen when I am trying to add a new collection. You can see that it wants to add the table just fine however pretty much every table has updated columns which I am not sure why yet.

Why would adding a new collection modify the so many different other table's columns. Pretty sure there is something really wrong with the diff function within Strapi (https://github.com/Cpaczek/strapi/blob/8a3b09759c76c80a996f99b76b3f7cdb0ff0debe/packages/core/database/lib/schema/diff.js#L325)

All of these tables get updated just because I added a new collection:

https://pastebin.com/PLpj4nZf

Makes no sense why the schema of these tables need to be updated.

@derrickmehaffy Any idea why Strapi wants to change so many table's schemas?

There also may be issues unrelated to cockroach db becuase of updating to knex 1.0.

Breaking changes
Dropped support for Node 10; <- This won't effect anything
Replaced unsupported sqlite3 driver with @vscode/sqlite3; <- Will have to fix sqlite driver but that is a future problem
Changed data structure from RETURNING operation to be consistent with SELECT; <- Not sure if this effect the Strapi codebase
Changed Migrator to return list of migrations as objects consistently. <- Not sure about this either

ethan-gallant · 2022-02-03T13:33:19Z

I have been following along with this as well.

The behaviour seems like whenever another table is added which has a foreign key, the table owning the foreign key is updated for no reason.

For example, if I have table A which references a column on table B. When table A is created, an update is issued to table B (which doesn't change anything at all).

derrickmehaffy · 2022-02-03T17:55:41Z

@derrickmehaffy Any idea why Strapi wants to change so many table's schemas?

I don't have any ideas, @alexandrebodin would need to weigh in here as he wrote basically all of this himself
(Alex is out of the office this week)

The behaviour seems like whenever another table is added which has a foreign key, the table owning the foreign key is updated for no reason.

This would be my guess though as we added FQ now to basically everything (for good reason) but I don't know why doing so would mean doing updates if nothing changed.

I would assume our diff code could be improved but I haven't dug that deep into it myself as I've been slammed with multiple other topics lately.

cpaczek · 2022-02-13T06:26:34Z

Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.

derrickmehaffy · 2022-02-14T19:50:50Z

Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.

We didn't have any specific plans but I don't think we have any specific arguments not to either.

More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't.

So long as we don't introduce any breaking changes.

cpaczek · 2022-02-14T22:37:46Z

Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.

We didn't have any specific plans but I don't think we have any specific arguments not to either.

More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't.

So long as we don't introduce any breaking changes.

Afaik the only thing that would have to be changed from the user's perspective is the SQLite driver and that is rarely using in prod anyway, and that is probably for the better as its not longer supported.

There are a lot of cool new features in the 1.0 release as well https://github.com/knex/knex/releases

derrickmehaffy · 2022-02-15T15:24:32Z

Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.

We didn't have any specific plans but I don't think we have any specific arguments not to either.
More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't.
So long as we don't introduce any breaking changes.

Afaik the only thing that would have to be changed from the user's perspective is the SQLite driver and that is rarely using in prod anyway, and that is probably for the better as its not longer supported.

There are a lot of cool new features in the 1.0 release as well https://github.com/knex/knex/releases

Did they finally drop mapbox's SQLite package

cpaczek · 2022-02-15T16:42:11Z

Are there any existing plans to migrate to knex v1? I am planning on doing this in this PR but I was wondering if that was already in the works.

We didn't have any specific plans but I don't think we have any specific arguments not to either.
More so wasn't a concern because we didn't need anything in the update yet, but if adding cockroach needs it, I don't see why we can't.
So long as we don't introduce any breaking changes.

Afaik the only thing that would have to be changed from the user's perspective is the SQLite driver and that is rarely using in prod anyway, and that is probably for the better as its not longer supported.

There are a lot of cool new features in the 1.0 release as well https://github.com/knex/knex/releases

Did they finally drop mapbox's SQLite package

Yes, I think they support 2 driver's now, vscode's and something else

Edit:

derrickmehaffy · 2022-02-15T20:20:17Z

So we will need to change the default SQLite driver we install when someone chooses quickstart.

kibertoad · 2022-02-16T09:27:01Z

VSCode sqlite one is a drop-in replacement for mapbox, though.

derrickmehaffy · 2022-02-16T18:48:28Z

VSCode sqlite one is a drop-in replacement for mapbox, though.

We still have a method on project generation that adds the specific database package to the main package.json, usually sqlite3, MySQL, or pg.

I'll find the generator logic and point it out after my lunch.

derrickmehaffy · 2022-02-16T20:37:11Z

VSCode sqlite one is a drop-in replacement for mapbox, though.

We still have a method on project generation that adds the specific database package to the main package.json, usually sqlite3, MySQL, or pg.

I'll find the generator logic and point it out after my lunch.

Here we go, this is where we set the db-client + version to set in the new project's package.json:

strapi/packages/generators/app/lib/utils/db-client-dependencies.js

Line 6 in 81934f5

sqlite: { sqlite3: '5.0.2' },

derrickmehaffy · 2022-02-16T21:01:32Z

I've got a draft PR open to work on upgrading knex: #12557

roelbeerens · 2022-02-17T17:53:51Z

I'm excited about this 😊

strapi-bot · 2022-02-24T20:58:51Z

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/mongodb-compatibility-delayed-on-v4/4549/135

cpaczek · 2023-03-04T03:38:24Z

Hey @alexandrebodin I think this PR is pretty much ready for review, just added the integration tests and they are ~~mostly passing (see below for the 1 failing check)~~ passing 🎉🎉🎉

Edit:
Trying to fix some weird behavior relating to knex/knex#5506 and cockroachdb/cockroach#98034 This branch is working for CockroachDB but there are some issues when using promise.all() so working on that. In the meantime let me know if there are any other changes you need.

To test this PR here are some commands to get you up and running with a CRDB docker container.

docker volume create roach-single

docker run --name=roach-single -p 26257:26257 -p 8080:8080 -v "roach-single:/cockroach/cockroach-data" cockroachdb/cockroach:latest start-single-node --insecure

Get an SQL shell

docker exec -it roach-single ./cockroach sql --url="postgresql://root@127.0.0.1:26257/defaultdb?sslmode=disable"

Strapi DB Config

  connection: {
    client: 'cockroachdb',
    connection: {
      host: env('DATABASE_HOST', '127.0.0.1'),
      port: env.int('DATABASE_PORT', 26257),
      database: env('DATABASE_NAME', 'defaultdb'),
      user: env('DATABASE_USERNAME', 'root'),
      password: env('DATABASE_PASSWORD', ''),
      ssl: env.bool('DATABASE_SSL', false)
    },
    debug: false, //optional
  },

Current Status of this PR:

Currently everything is working and I have ~~only 1 pending bug~~ no pending bugs 🎉.

There are a few things to note:

Altering Column Types

Cockroach db doesn't support altering column type. i.e going from int -> string. My current implementation will just log a message if the user is trying to alter the type and skip that alter. We have tried to create a temporary column -> move data -> delete old column -> rename temp column to original column name However due to the way knex tableBuilder works it will always try to delete columns before creating it. To solve this we would have to do this migration outside of the main migration transaction which would be unsafe hence why I just log a warning. There are 2 workarounds for this issue, first being to delete the column from your schema, let Strapi delete the column and then let Strapi recreate it within a new migration however this will result in loss of data. The other way is CRDB supports alter column type experimentally however it doesn't support it within a transaction so you would have to sql shell in, enable the flag and alter the column.

Experimental Status

Talking with @derrickmehaffy it seems like Strapi will support this experimentally first, so I have added very big red letters in the startup log saying that its experimental and big red notice in my docs PR saying that its experimental. I was talking with derrick and we were wondering if we should require an env var like STRAPI_EXPERIMENTAL to be set to true before CRDB can be used and/or during npx create-strapi-app we should require the --experimental flag in order to show the CRDB db option. Please advise :)

Race Conditions

I also have run into some race conditions with how you are assigning orders for relations you can read about that more here #15558 the issue with CRDB specifically is that it enforces the unique constraint on the link tables for relation order whereas the other databases seem to be a bit looser when it comes to this constraint. This won't be a problem for most Strapi apps but if you have any horizontal scaling it could cause an issue, or if you are doing stuff like promise.all() like what I fixed in the user and permissions plugin. See the issue for more details.

Huge thanks to @Boegie19 without him this PR would of never been completed.

As always I am open to any changes that the Strapi team needs and I'm always available to hop on a call if you want to discuss. Hoping to get this merged soon, There is already at least 1 person using the code on the branch to add cockroach support to their project.

packages/core/database/lib/entity-manager/index.js

selfagency · 2023-09-22T19:28:44Z

Hi @cpaczek, I was just wondering why this stalled out and if it's ever getting merged

Boegie19 · 2023-09-23T09:55:01Z

@selfagency The answer is most likely no this will never happen. unless strapi does the required changes to make it work.
What is the issue.
Cockroach uses Serializable Isolation Level
while all databases strapi support use Read Committed Isolation Level and Cockroach can't be changed to this mode.

This causes issues with lots of race conditions in strapi that Read Committed Isolation Level would not care about but Serializable Isolation Level will cancel the saving in the DB if any of the race conditions happen.

After this where to be fixed we still have.
cockroachdb/cockroach#91223
that needs to be fixed for a full integration with strapi

cpaczek · 2024-01-30T05:47:28Z

Cockroach db has added read committed isolation levels as of Jan 17 2024 in public preview meaning this will allow support for Cockroach to be used with Strapi.

https://www.cockroachlabs.com/docs/releases/v23.2

cpaczek · 2024-01-30T05:48:44Z

@derrickmehaffy ill be updating this pr to be up to date with main, can you reopen issue #11738

derrickmehaffy · 2024-01-30T12:19:15Z

@derrickmehaffy ill be updating this pr to be up to date with main, can you reopen issue #11738

You should target develop I believe now

cpaczek added 3 commits January 6, 2022 20:58

cockroach dialect

670487d

Merge remote-tracking branch 'strapi/master' into cockroach-dialect

cd2d2d7

Update knex and begin to add cockroach dialect

ed9ec22

cpaczek mentioned this pull request Jan 29, 2022

Add native support for Cockroach DB #11738

Closed

update knex to 1.0.1

316f9e5

cpaczek added 4 commits February 2, 2022 19:19

Merge remote-tracking branch 'strapi/master' into cockroach-dialect

220bc14

Update to Knex 1.0.2 and implement alterType

fe7a1ad

quickfix

8a3b097

more logging

c517eef

derrickmehaffy added source: core:database Source is core/database package pr: feature This PR adds a new feature labels Mar 15, 2022

Merge remote-tracking branch 'strapi/master' into cockroach-dialect

71a81a1

cpaczek added 12 commits February 26, 2023 23:06

merge main into cockroach

05932dc

update lock

cc43b90

fix lint and disable automatic json type parsing

92b32b6

use main repo lock

2147386

Merge remote-tracking branch 'origin/main' into cockroach-dialect

b35e143

fix index column diff

9b50deb

add tests

44e5954

update image for ee tests

ac155cc

fix healthcheck

96247d8

remove init script

ad36079

change db to defaultdb

5e07dc1

update actions

72a2140

cpaczek added 3 commits March 3, 2023 19:57

fix race condition

f865b65

convert promise to for of loop

d51f658

convert to for of

66c3d73

cpaczek mentioned this pull request Mar 4, 2023

Simultaneous inserts are not all being saved to database. cockroachdb/cockroach#98034

Open

cpaczek added 2 commits March 5, 2023 15:18

fix relations

ef8eb79

remove transaction for select query

1c1d8c0

cpaczek commented Mar 5, 2023

View reviewed changes

packages/core/database/lib/entity-manager/index.js Outdated Show resolved Hide resolved

cpaczek mentioned this pull request Mar 6, 2023

Knex is not throwing a 40001 when using cockroach db. knex/knex#5506

Open

cpaczek added 2 commits March 5, 2023 17:02

add transacting back

a1556b3

set isolation mode to read commited

fcfc414

Boegie19 closed this Sep 24, 2023

cpaczek reopened this Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cockroach Dialect #12346

Add Cockroach Dialect #12346

cpaczek commented Jan 29, 2022 •

edited

codecov bot commented Jan 29, 2022 •

edited

kibertoad commented Feb 1, 2022 •

edited

cpaczek commented Feb 1, 2022 •

edited

cpaczek commented Feb 3, 2022 •

edited

ethan-gallant commented Feb 3, 2022

derrickmehaffy commented Feb 3, 2022 •

edited

cpaczek commented Feb 13, 2022

derrickmehaffy commented Feb 14, 2022

cpaczek commented Feb 14, 2022

derrickmehaffy commented Feb 15, 2022

cpaczek commented Feb 15, 2022 •

edited

derrickmehaffy commented Feb 15, 2022

kibertoad commented Feb 16, 2022

derrickmehaffy commented Feb 16, 2022

derrickmehaffy commented Feb 16, 2022

derrickmehaffy commented Feb 16, 2022

roelbeerens commented Feb 17, 2022

strapi-bot commented Feb 24, 2022

cpaczek commented Mar 4, 2023 •

edited

selfagency commented Sep 22, 2023

Boegie19 commented Sep 23, 2023 •

edited

cpaczek commented Jan 30, 2024

cpaczek commented Jan 30, 2024

derrickmehaffy commented Jan 30, 2024

Add Cockroach Dialect #12346

Are you sure you want to change the base?

Add Cockroach Dialect #12346

Conversation

cpaczek commented Jan 29, 2022 • edited

codecov bot commented Jan 29, 2022 • edited

Codecov Report

kibertoad commented Feb 1, 2022 • edited

cpaczek commented Feb 1, 2022 • edited

cpaczek commented Feb 3, 2022 • edited

With alterType = false

With alterType = true

What I think may be the root of the problem is this:

All of these tables get updated just because I added a new collection:

There also may be issues unrelated to cockroach db becuase of updating to knex 1.0.

ethan-gallant commented Feb 3, 2022

derrickmehaffy commented Feb 3, 2022 • edited

cpaczek commented Feb 13, 2022

derrickmehaffy commented Feb 14, 2022

cpaczek commented Feb 14, 2022

derrickmehaffy commented Feb 15, 2022

cpaczek commented Feb 15, 2022 • edited

derrickmehaffy commented Feb 15, 2022

kibertoad commented Feb 16, 2022

derrickmehaffy commented Feb 16, 2022

derrickmehaffy commented Feb 16, 2022

derrickmehaffy commented Feb 16, 2022

roelbeerens commented Feb 17, 2022

strapi-bot commented Feb 24, 2022

cpaczek commented Mar 4, 2023 • edited

Current Status of this PR:

There are a few things to note:

Altering Column Types

Experimental Status

Race Conditions

selfagency commented Sep 22, 2023

Boegie19 commented Sep 23, 2023 • edited

cpaczek commented Jan 30, 2024

cpaczek commented Jan 30, 2024

derrickmehaffy commented Jan 30, 2024

cpaczek commented Jan 29, 2022 •

edited

codecov bot commented Jan 29, 2022 •

edited

kibertoad commented Feb 1, 2022 •

edited

cpaczek commented Feb 1, 2022 •

edited

cpaczek commented Feb 3, 2022 •

edited

derrickmehaffy commented Feb 3, 2022 •

edited

cpaczek commented Feb 15, 2022 •

edited

cpaczek commented Mar 4, 2023 •

edited

Boegie19 commented Sep 23, 2023 •

edited