search

ory / hydra

The most scalable and customizable OpenID Certified™ OpenID Connect and OAuth Provider on the market. Become an OpenID Connect and OAuth2 Provider over night. Broad support for related RFCs. Written in Go, cloud native, headless, API-first. Available as a service on Ory Network and for self-hosters.
https://www.ory.sh/?utm_source=github&utm_medium=banner&utm_campaign=hydra
Apache License 2.0
15.47k stars 1.48k forks source link

Feature request: CockroachDB support #990

Closed glerchundi closed 5 years ago

glerchundi commented 6 years ago

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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Which column is causing the trouble?

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

Multiply commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Yup!

lopezator commented 5 years ago

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

glerchundi commented 5 years ago

Great job @lopezator!

aeneasr commented 5 years ago

Resolved #1348

glerchundi commented 5 years ago

Woohoo! 🎉