Feature request: CockroachDB support

[glerchundi]

I'm evaluating Hydra v1.0.0-beta.8 and its compatibility with CockroachDB and I can confirm that except for two issues, things are working properly.

These two issues are:

• ALTER COLUMN ... SET NOT NULL is not supported by CockroachDB which is required by this migration. They've opened an issue for tracking this feature request (sql: support ALTER TABLE ALTER COLUMN SET NOT NULL cockroachdb/cockroach#28751) and for your interest this probably was triggered by the issue I raised some days ago sql: compatibility with ORY Hydra cockroachdb/cockroach#28415, PR: sql: support ALTER COLUMN SET NOT NULL cockroachdb/cockroach#37554.
• Updates without a WHERE statement are not supported although there is a work-around by calling SET sql_safe_updates = false explicitly in the current connection or transaction. Another one could be to modify current migration to do an UPDATE .. WHERE (SELECT *) or something like that. Nothing to do here, for details take a look here: sql: CockroachDB does not support updating session vars via UPDATE to pg_settings cockroachdb/cockroach#28826 (comment).

WDYT, are you open to include CockroachDB support?

Update:

It seems like CockroachDB people is working on adding support for ALTER TABLE ... ALTER COLUM ... SET NOT NULL, take a look at cockroachdb/cockroach#37554.

[aeneasr]

I really want to avoid having another migration table like here. I also suspect that the two issues you mentioned wont be the only ones. I'm not sure how well CockroachDB works with SQL and specifically more complex join and lock statements. As most cloud providers support manages postgres and mysql at very low cost (but not cockroach), adding cockroach is not a priority for this project.

[glerchundi]

I really want to avoid having another migration table like here.

The idea is to have the same migration table as the postgres one, I'm aligned with you here.

I also suspect that the two issues you mentioned wont be the only ones.

CockroachDB is working hard making everything pg-compat so if something doesn't work it should be fixed in the upstream and not here.

I'm not sure how well CockroachDB works with SQL and specifically more complex join and lock statements.

Can you point me to concrete SQL sentences, the more complex one you can think of?

As most cloud providers support manages postgres and mysql at very low cost (but not cockroach)

Yeah, but CockroachDB is open-source (at least most of its features) and with a growing user base. Great PostgreSQL compatibility, automatic sharding (world-wide capable), NoSQL capabilities, Geo-Partitioning and cloud-native philosophy (no need for an operator to run inside a k8s cluster, for example) makes this a great database to start with.

adding cockroach is not a priority for this project.

Would you be open to do some reviews of further PRs at least?

Thanks for your time.

[aeneasr]

Can you point me to concrete SQL sentences, the more complex one you can think of?

Of course, this is a particularly nasty one.

CockroachDB is working hard making everything pg-compat so if something doesn't work it should be fixed in the upstream and not here.

With pg-flavoured SQL-compatibility it should be no work to get it working with this project. How far away are they from achieving that?

Would you be open to do some reviews of further PRs at least?

Review yes but no guarantee of merging. This is not about "oh I don't like nothing that isn't MySQL or PostgreSQL" or "I dismiss the upsides of the project". Instead the issue is that including this in master implies that we:

1. Write and test proper migrations for feature versions for another database
2. Are able to support people who use CDB and have issues with our database layer due to whatever (e.g. unsupported syntax)
3. Actually know how to maintain and use CDB because we officially support it
4. Work around issues in CDB which clutter the proper/easier to read SQL but might stay there because we are not following the project and don't know when specific issues will be addressed.

None of these four things are something we can currently afford wrt to resources. Depending on the PR it may be possible that we have an "implicit" compatibility with CDB, but if CDB specific issues are addressed in a new migration "table"/instruction or existing migrations change (would bring serious trouble) this can't be merged.

I am however happy to review a PR regarding this, maybe it's much less stuff that changes than I think it is.

[glerchundi]

Of course, this is a particularly nasty one.

It can be more or less performant due to joins but I think that should work. Inner joins with a pattern matching which is also supported.

With pg-flavoured SQL-compatibility it should be no work to get it working with this project. How far away are they from achieving that?

They are fixing the issues one by one but my feeling is that they are not too far from having a huge list of statement compatibilities. Take a look to a list of database management tools here.

Review yes but no guarantee of merging. This is not about "oh I don't like nothing that isn't MySQL or PostgreSQL" or "I dismiss the upsides of the project". Instead the issue is that including this in master implies that we:

Write and test proper migrations for feature versions for another database
Are able to support people who use CDB and have issues with our database layer due to whatever (e.g. unsupported syntax)
Actually know how to maintain and use CDB because we officially support it
Work around issues in CDB which clutter the proper/easier to read SQL but might stay there because we are not following the project and don't know when specific issues will be addressed.
None of these four things are something we can currently afford wrt to resources. Depending on the PR it may be possible that we have an "implicit" compatibility with CDB, but if CDB specific issues are addressed in a new migration "table"/instruction or existing migrations change (would bring serious trouble) this can't be merged.

IMHO merging doesn't mean official support, as you said what about just implicit compatibility and test it out explicitly adding a note saying that "[...] although it seems CockroachDB to be working properly no official support is given"?

I am however happy to review a PR regarding this, maybe it's much less stuff that changes than I think it is.

Cool, no much time ATM but will do as soon as I can, lets discuss on the PR(s). Making proposals without promises are the most comforting jobs after all ;)

Thanks again,

[aeneasr]

IMHO merging doesn't mean official support, as you said what about just implicit compatibility and test it out explicitly adding a note saying that "[...] although it seems CockroachDB to be working properly no official support is given"?

That's not how this project works :) If we're including stuff in the codebase, it's supported. With implicit support I mean that there will be no extra migrations, integration, nor e2e tests that check if this actually works! This means that future upgrades might not work with CDB or break the database (very unlikely but possible). If we're making substantial code changes it means that it needs to be tested, and with tests we must maintain it. I hope you understand this reasoning here.

But again, if it's possible to do this without any funkiness, let's try it!

[Multiply]

For the migration part, would it hurt too badly to add a new column with the right settings, move data from old to new column, remove old column and rename new column to old's name?

[aeneasr]

That really depends, there are production instances with millions of tokens - depending on which table this touches that could cause some serious db downtime. Which one is it?

[Multiply]

So far it only seems to be hydra_client.
Another problem I stumbled into is changing primary keys, which doesn't seem to be supported by Cockroach either. (unless I'm doing something entirely wrong)

So my current take on the issue, would be to create new tables and migrate data over, which seems silly.

Essentially what I had to do, to make Hydra work with Cockroach, was to run the migrations on a normal Postgres installation, and then export the structure. Not ideal, but good enough for my use-case for now.

[aeneasr]

Which column is causing the trouble?

The latest patch introduces real primary keys (uint, serial) to all tables iirc.

[Multiply]

Migrations causing trouble:

1. All columns with the ALTER COLUMN ... SET NOT NULL as mentioned in the ticket.
2. All migrations dropping primary keys. Example

[aeneasr]

Ah ok, I see. I am more inclined now than I was back in August to have CockroachDB officially supported. The new migration layout would potentially allow to run different migrations for each database, so theoretically, we could add just one new migration file for cockroach db.

[Multiply]

Another problem I had while trying to figure out which migrations failed, is the fact that it's not telling me which line/query in the migration that failed, so I had to narrow it down myself.

Example right now:

Applying `jwk` SQL migrations...
Applied 0 `jwk` SQL migrations.
Applying `client` SQL migrations...
Applied 0 `client` SQL migrations.
Applying `consent` SQL migrations...
An error occurred while running the migrations: could not apply consent SQL migrations: Could not migrate sql schema, applied 0 migrations: pq: no data source matches prefix: hydra_oauth2_consent_request handling 7

I can easily find the file, but since it's a migration on the 'large' side, it's a bit tough to easily spot the failing one. I basically just run each query one by one, and fix it manually.

[lopezator]

This given:

Ah ok, I see. I am more inclined now than I was back in August to have CockroachDB officially supported. The new migration layout would potentially allow to run different migrations for each database, so theoretically, we could add just one new migration file for cockroach db.

Would you be open to receive/review a PR adding support for CockroachDB? At least to discuss it.

I could propose some changes to accomplish this.

[aeneasr]

Yup!

[lopezator]

I've been advancing on this, so I'll send a draft PR shortly, just to start the discussion :)

[glerchundi]

Great job @lopezator!

[aeneasr]

Resolved #1348

[glerchundi]

Woohoo! 🎉