Add native support for Cockroach DB

[cpaczek]

Feature request

Please describe your feature request

I've been talking with @derrickmehaffy in the discord about this but thought I should make a feature request on GitHub in case anyone else has input.

https://www.cockroachlabs.com/

I propose that CockroachDB should be added as a supported database to Strapi v4. Currently knex 0.95.12 (v4 is on .0.95.6) officially supports CockroachDB as a dialect.

I think it should be added for the following reasons

• Great scaling on Kubernetes
• One command automatic backups to s3
• Multi-Master nodes
• Amazing diagnostics admin panel

We were able to get CockroachDB to work on v3 using Postgres config however we have yet to test v4 with Cockroach but I will be editing this issue with any updates.

[derrickmehaffy]

Hi @cpaczek this is something we are considering, it may take some time to evaluate as we want to handle v4 bugs first but since it's native to Knex.js now and largely based on PostgreSQL I don't forsee too much issue adding native support for cockroach.

The only concern I think we have is to make sure the auto schema migration works properly. Technically you should be able to use the postgres dialect and test that way.

[derrickmehaffy]

Was doing some initial testing, first boot actually worked fine, but you do have to specify an undocumented key that forces a PG version so Knex doesn't try and ask for it.

But after creating a new collection-type it's throwing an error because for some reason we are trying to drop a not null constraint on a primary key (why??)

Probably best if we look at upgrading knex and configuring a proper dialect. If anyone is willing to start a PR please feel free.

[ethan-gallant]

Another odd thing we noticed with V3 is that it would try to alter columns to their existing state. For example it would try to swap a column from VARCHAR(255) -> VARCHAR(255) which makes legitimately no sense.

[cpaczek]

Did some more testing. I forked the monorepo and added cockroachdb as a dialect using Yarn Link. You can see the changes in this repo https://github.com/Cpaczek/strapi/tree/cockroach-dialect/packages/core/database

Using this config:

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

However I am still getting the same error as @derrickmehaffy

I believe everything starts here: https://github.com/strapi/strapi/blob/master/packages/core/database/lib/schema/builder.js#L297
createColumn(tableBuilder, object).alter();

This is responsible for altering the strapi_core_store_settings table

It then calls: https://github.com/strapi/strapi/blob/master/packages/core/database/lib/schema/builder.js#L199

    if (notNullable === true) {
      console.log("Is not Nullable")
      col.notNullable()
    } else {
      console.log("Is Nullable")
      col.nullable();
    }

This adds the notNullable() before the .alter() as shown

What the notNullable() does is

  {
    sql: 'alter table "strapi_core_store_settings" alter column "id" type serial primary key using ("id"::serial primary key)',
    bindings: []
  },
  {
    sql: 'alter table "strapi_core_store_settings" alter column "id" set not null',
    bindings: []
  }

after sets "id" as a pk.

Then alter() happens which I believe is what is calling the error (https://github.com/strapi/strapi/blob/master/packages/core/database/lib/schema/builder.js#L297)

For some reason I think alter() trys to drop not null on the id column but because it was just set to a primary index its not allowed to? Honestly I am not sure. I starting learning Knex and CockroachDB 3 hours ago so someone with more experience which Knex, Cockroach, and Strapi's database would probably be a lot better suited to debug this.

P.S this issue looks interesting knex/knex#4401

[derrickmehaffy]

That issue you linked is exactly the problem, looks like they have a draft PR to work on a solution that isn't a breaking change, so we will need to wait for that and update our knex version to support it.

[derrickmehaffy]

Draft PR to Knex.js (looks like it waiting on tests): knex/knex#4730

[kibertoad]

Alternatively, you can use setNullable/dropNullable methods which do not have same problem and should work always.

[derrickmehaffy]

Alternatively, you can use setNullable/dropNullable methods which do not have same problem and should work always.

Problem is we are basically trying to build our ORM (kinda) as we dropped bookshelf. This issue doesn't happen on PostgreSQL, MySQL, MariaDB, or SQLite but CockroachDB throws an error due to the weird logic of alter.

The solution proposed in that PR should help here I think but I agree with others in the linked issue that it really shouldn't try to drop the not null.

[kibertoad]

@derrickmehaffy I agree, but it's unclear if PR's author will be finishing it. Maybe you would be open to pick up original PR and add tests to it?

[derrickmehaffy]

@derrickmehaffy I agree, but it's unclear if PR's author will be finishing it. Maybe you would be open to pick up original PR and add tests to it?

I can take a look, admittedly I'm not super familiar with Knex.js's codebase. But we (Strapi) have contributed before.

[intech]

@derrickmehaffy If you need help with the cockroach dialect, mention me in the issue or PR in knex repo, I will help.

[derrickmehaffy]

@derrickmehaffy If you need help with the cockroach dialect, mention me in the issue or PR in knex repo, I will help.

Aye I honestly have no idea how the CRDB dialect functions, it looks like it's extending the PG one but not everything 🤔

[intech]

@derrickmehaffy The cockroach developers do pg dialect support for full backward compatibility, but there are still many tasks in the roadmap and open issues with bugs.
In knex we are expanding the pg dialect to features in cockroach. This looks like adding new methods that will only be used with cockroach, they just won't work with other databases.

[derrickmehaffy]

Gotcha alright that makes sense 👍 I think I have an idea where to put the tests, just need to write them and figure out how the heck to test it myself within Strapi

[intech]

I have no experience with strapi and it is a little difficult for me now to determine what exactly the error occurred.
It will be ideal if someone can show the order of sql queries.
Because the alter column operation is exactly the same operation as the alter table for cockroach, there are a number of restrictions on its use, in addition to the fact that the cockroach does changes to table schemas asynchronously in the background and you can simply get into an unchanged state between queries.

In addition, I noticed that you create the primary key in the second step after creating the table, in the cockroach, by default, the rowid column will be created as the primary key, if you do not specify your own for the create table query.

[intech]

You can add once a temporary raw-query before all migration:
SET enable_experimental_alter_column_type_general = true;
and check again

[derrickmehaffy]

I have no experience with strapi and it is a little difficult for me now to determine what exactly the error occurred. It will be ideal if someone can show the order of sql queries. Because the alter column operation is exactly the same operation as the alter table for cockroach, there are a number of restrictions on its use, in addition to the fact that the cockroach does changes to table schemas asynchronously in the background and you can simply get into an unchanged state between queries.

In addition, I noticed that you create the primary key in the second step after creating the table, in the cockroach, by default, the rowid column will be created as the primary key, if you do not specify your own for the create table query.

The primary key id column should be created initially when the table is created:

strapi/packages/core/database/lib/schema/schema.js

Lines 105 to 111 in 828b143

	case 'increments': {
	return {
	type: 'increments',
	args: [{ primary: true }],
	notNullable: true,
	};
	}

Let me get debugging enabled and test again on cockroach and see if I can get a full sql log dump

[derrickmehaffy]

Here is the full SQL log from boot, to me creating a new content-type: https://paste.ee/p/2PuiC

[intech]

Don't you think the first 2 queries don't make sense?

[
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" drop default',
    bindings: []
  },
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" drop not null',
    bindings: []
  },
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" type serial primary key using ("id"::serial primary key)',
    bindings: []
  },
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" set not null',
    bindings: []
  }
]

[intech]

I decided to check the pg documentation and it can't be done there either:

The PRIMARY KEY constraint specifies that a column or columns of a table can contain only unique (non-duplicate), nonnull values. Only one primary key can be specified for a table, whether as a column constraint or a table constraint.

PRIMARY KEY enforces the same data constraints as a combination of UNIQUE and NOT NULL.

Maybe we're looking for a problem in the wrong place? :)

[derrickmehaffy]

Don't you think the first 2 queries don't make sense?

[
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" drop default',
    bindings: []
  },
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" drop not null',
    bindings: []
  },
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" type serial primary key using ("id"::serial primary key)',
    bindings: []
  },
  {
    sql: 'alter table "public"."strapi_core_store_settings" alter column "id" set not null',
    bindings: []
  }
]

Oh I 100% agree with you, I just can't tell if it's something in our code or Knex's but something is certainly odd. Because those queries make zero sense.

[cpaczek]

Started PR: #12346

[kibertoad]

@derrickmehaffy If you update to knex 1.0.2, you can use new alterType parameter to avoid unnecessary type conversion: knex/documentation@20adb5c

[WardenCommander]

thank you for this 🥇

[derrickmehaffy]

FYI as of v4.1.7 (v4.1.6) we have upgraded to Knex v1.0.x

[WardenCommander]

So soon it might be possible to use cockroachdb?

[derrickmehaffy]

Will depend on the community member currently working on the PR.

[cpaczek]

So soon it might be possible to use cockroachdb?

Yes Soon, we need cockroach db support in order to migrate to v4 so we need to get it working.

[cpaczek]

Currently debating on waiting until this issue is fixed on cockroach's side or figure out a work around with Strapi cockroachdb/cockroach#49329

See PR for more details

[derrickmehaffy]

As the PR has been currently abandoned I'll go ahead and migrate this over to our feedback database here: https://feedback.strapi.io/developer-experience/p/add-native-support-for-cockroach-db

For now I will close and lock this issue, if you would like to vote for support on this please see the feedback page.

[derrickmehaffy]

Reopening as @cpaczek has started work on the PR again.

[derrickmehaffy]

closing this in favor of https://feedback.strapi.io/developer-experience/p/add-native-support-for-cockroach-db