CockroachDB Support

[ansarizafar]

Please include support for CockroachDB https://www.cockroachlabs.com/

[absolux]

@ansarizafar
CockroachDB can be used with the pg driver look at

[elhigu]

Support is implemented in #1993 (comes in release knex 0.13)

[nicck]

hm... I would say passing version of connected db is implemented, not CockroachDB support.

[elhigu]

@nicck is there some CockroachDB queries which are not compatible with postgresql? If so I'll reopen this as a feature request.

[nicck]

It's the opposite, PostgreSQL queries not supported by CockroachDB. I mentioned some of them in cockroachdb/cockroach#15299 (comment). CockroachDB is not PostgreSQL, protocol is compatible but CockroachDB support only subset of SQL queries which are valid in PostgreSQL.

[elhigu]

@nicck since cockroach db queries is subset of postgres queries, you should be able to use cockroach db just fine with postgres dialect. Just don't use those features that are not supported by it.

[nicck]

I can avoid using uuid and json types, but some sql queries, like select * for update and ... table_schema = current_schema are parts of knex as I understand and it's not possible to avoid them if you use pg dialect.

[elhigu]

@nicck ah right... those queries done by migration system requires some fixing. At least that select for update could be avoided by some flag which allows migration locking to throw an error instead of blocking (like it did work while ago).

So currently migration system is unusable with CockroachDB. Do you know if there are any other problems?

[thomasdavis]

I was trailing Cockroach DB yesterday and also got stuck on the migrations. Commenting as a plus one and so I can watch the thread.

[elhigu]

Meanwhile that 0.13 doesn't support it you might want to try if 0.12 does. IIRC migrations work a bit different in that version.

[kmaid]

Just tried the migration system and it won't work even with just raw sql. I set my knex-migrations version from 1.3 to 1.2.0 and 1.0.0 but no luck. I don't think I can roll back knex as cockroach requires the version configuration option

[mhaagens]

Anyone found a solution to the migrations issue yet?
Still getting the error

Knex:warning - Can't take lock to run migrations: select * from "knex_migrations_lock" for update - syntax error at or near "for"
Knex:warning - If you are sure migrations are not running you can release the lock manually by deleting all the rows from migrations lock table: knex_migrations_lock

[mailaneel]

Temporary fix we are using for migrations to work in cockroachdb

knexfile.js

const Compiler = require('knex/lib/dialects/postgres/query/compiler')
const types = require('pg').types

Compiler.prototype.forUpdate = function forUpdate () {
  console.warn('table lock is not supported by cockroachdb/postgres dialect');
  return '';
};

/**
 * Required because postgres returns int as string and 
 * knex checks for isLocked are evaluating to true all the time
 */
types.setTypeParser(20, function(val) {
  return parseInt(val)
})

module.exports = {

  development: {
    client: 'postgresql',
    version: '7.2',
    debug: true,
    connection: {
      database: 'db_name',
      port: '26257',
      user: 'root',
      password: ''
    },
    pool: {
      min: 2,
      max: 10
    },
    migrations: {
      disableTransactions: true,
      tableName: 'knex_migrations'
    }
  }
};

[kibertoad]

Would anyone volunteer to create a failing test that would assert not triggering the undesired operation from within migrations? I consider giving this issue a shot if there is any interest in it.

[Bessonov]

@mailaneel thank you for your snippet. It worked for me.

The one thing, it can lead to errors, because the driver returns string for 64-bit integers, which are not supported in this way.

Looking forward for native support of BigInt: brianc/node-pg-types#78 . Vote for it, if you want it too :o)

[Bessonov]

Related issue: cockroachdb/cockroach#6583

[Bessonov]

I've discovered another incompatibility. CockroachDB doesn't support savepoints. But the savepoints are a part of Transaction Retries Protocol. This leads to the issue, that the name of savepoint must be cockroach_restart and not generated one. CockroachDB can be configured to accept all names, but then you lost confidence to catch programmer errors, especially with nested transactions. Could savepoint-method be extended to support name parameter?

Is there any way to retry multi-query transaction with knex (without run all the code again)?

[Bessonov]

May be I did something wrong, but it turns out, that there is a more work for supporting the TRP. tx.rollback() leads that the transaction becomes completed, so there is no further actions possible. Actually, instead of sub-transaction I run raw queries:

SAVEPOINT cockroach_restart;
-- if any 40001 error, then
ROLLBACK TO SAVEPOINT cockroach_restart;
-- run full code again

[kibertoad]

@Bessonov You could try extending it, if it can be implemented in a non-breaking way, we might accept the PR.

[awoods187]

Hey all, PM at Cockroach Labs here. We are adding support for savepoints in our upcoming 20.1 release. It will be in our next beta (https://www.cockroachlabs.com/docs/releases/#testing-releases). Let us know if you run into any trouble using it!

[kibertoad]

@elhigu @lorefnon @briandamaged @maximelkin Considering popularity of CockroachDB, I'm thinking of adding additional test run configuration to see how many of our tests are failing when PG driver is used to access CockroachDB instance from docker image (could be all of them or most of them). Might be that we do need a separate dialect (extending PG) to handle certain cases; in that case it would probably make sense to create it as a separate repo in knex project and just use as a devDependency to run tests. Thoughts?

@awoods187 If your company would consider lending any dev effort towards better support of CockroachDB in knex, that would be greatly appreciated.

[awoods187]

We generally recommend making dialects by forking the PG dialect for these types of tools. We'd be happy to answer any questions and support you along the way. We can also set up a shared slack if that'd be helpful to you.

[Bessonov]

@awoods187 thanks for the update. As I can see the docs are not updated yet. How it impacts transaction retry protocol?

@kibertoad we run knex + CRDB for a while and doesn't experienced any major problems with pg version 9.6.

ATM the only wish we have is to get rid of our custom transaction-retry-protocol implementation. There also some issues with ALTER TABLE for primary keys, but it works with specificType('id', 'UUID PRIMARY KEY').

[awoods187]

We are currently in the process of documenting savepoints cockroachdb/docs#5953

[jahudka]

Hi, thanks everybody for your work! I'm mostly writing to add a "plus one" to show interest in this feature and to get notifications about further updates, but I also have a question which seems relevant enough to ask here: will this driver support specifying multiple hosts in the connection URL, similarly to what libpq allows?

TL;DR - In libpq-based clients one can specify multiple host:port pairs in the connection URL separated by a comma, so e.g. postgresql://user@host1:5432,host2:5432,host3:5432/dbname. As I understand it, libpq should then try connecting to the hosts in the order they're specified and use the first one that succeeds. I'm not sure my logic is undeniable here, but I think that having an app always connect to the same hardcoded Cockroach node kind of defeats the purpose of having a distributed database, and having to rely on DNS to resolve a hostname to a running node adds both complexity and an unnecessary dependency.

The pg driver doesn't seem to support this yet, but pg-native does (since it uses libpq bindings under the hood), and pg can wrap pg-native and re-expose it with (mostly) the same API, so in theory, if this driver is built on top of pg, supporting this feature could be as easy as require('pg').native instead of require('pg'). This would kill a couple of features that pg-native can't support, like query streaming, but it might not be an unacceptable tradeoff for some use cases.

[kibertoad]

@jahudka There is mostly complete support for pg-native: #4327

[jahudka]

@kibertoad yeah I know, but if I understand correctly, the pgnative driver, when finished, will be compatible with Postgres, but not with Cockroach (in the sense that it won't solve the problems that @intech's Cockroach driver discussed here solves), right? So I'll then have either the option to use the Cockroach driver, but only connect to a single configured node, or to use pgnative, but maybe some stuff like migrations won't work.. if that's the case then that might prevent me from using pgnative completely, because what I'd really like to use is MikroORM, which is built on top of Knex, and so I might not be able to decide to avoid some Knex features that break when pgnative is used with CockroachDb..

EDIT: I think I might be thinking about this the wrong way - instead of you guys making both the Postgres and the Cockroach drivers compatible with both pg and pg-native it might be better to nudge the developer of pg to implement the multiple hosts feature.. There's an issue for that as well somewhere iirc, I'll try asking there.

[intech]

@jahudka don't worry about streaming, cockroach doesn't support it.

You are right, pg does not have the ability to pass a list of nodes, and I agree that this should be added to the pg package. The problem is in the architecture of different databases using the same postgres dialect, that not all of them work in master-master mode and failover will be different.
We need to take as an example the configuration used in redis with priorities and master/slave roles.

[vy-ton]

@intech If you have time over the next few months to work on the CockroachDB support, I would like to discuss a sponsorship opportunity.

[intech]

@intech If you have time over the next few months to work on the CockroachDB support, I would like to discuss a sponsorship opportunity.

Wow, this is unexpected! I have interests to be closer to Cockroach Labs, let's continue communicating by email (we have already communicated with Andy, Rafi and Jesse earlier).

[kibertoad]

@intech I've started working on CockroachDB as a Knex dialect. Are there any insights or changes you had to do so far, that you could share?

[intech]

@kibertoad After refactoring Knex itself is not yet. Make a cockroachdb branch with base dialect from postgres and we will work on it together.

[intech]

Saw already in https://github.com/knex/knex/tree/feat/cockroachdb!

[rustyconover]

Any luck yet with migrations and cockroachdb?

[kibertoad]

Working on it!

[kibertoad]

@intech Rudimentary CockroachDB support landed in master already. Not all tests pass yet, but core functionality should work already. Any chance you could take a look, and maybe you have any code for the driver itself on top of what we currently have that you could share?

[kibertoad]

This is coming in 0.95.12, most likely late October!

[Bessonov]

@kibertoad great news! Is there any description what the new dialect do or support of crdb mean? I'm asking because we don't experience any problems with postgres dialect beside mentioned workaround.

[kibertoad]

@Bessonov It mostly means having knex's test suite pass on CockroachDB, which meant fixing things over pg dialect here and there. You can take a look at https://github.com/knex/knex/tree/master/lib/dialects/cockroachdb or try using 0.95.12-rc1 to help test it for the public release :)
(one fix that you will not see there is that knex no longer tries to obtain lock inside the transaction when performing migrations, because CockroachDB doesn't allow doing that; and we remove lock manually on fail anyway).
Your workarounds sound interesting, by the way. Would you consider contributing them to knex mainline? I assumed primary key alteration is an inherent limitation of CockroachDB, but if there is a feasible solution for it, would be happy to include it.

[Bessonov]

@kibertoad thanks! I see, changes are minimal. We would try it on the next update batch, but I'm sure it doesn't happen this month :(

[intech]

@kibertoad greetings! I'm back and glad to see updates here.
I cannot promise full immersion in this task. I will gradually add the methods that my team needs to work with CockroachDB on a real project.

[kibertoad]

@intech Appreciate it!

[rafiss]

I believe this can be closed now, correct?

[intech]

I believe this can be closed now, correct?

Yes, support can be tracked by the CockroachDB label and wiki page

[jmsunseri]

I still need to add disableTransactions: true, in my knexfile in order to be able to run migrate:latest or i get an error like this.

COMMIT; - restart transaction: TransactionRetryWithProtoRefreshError: TransactionRetryError: retry txn (RETRY_SERIALIZABLE - failed preemptive refresh): "sql txn" meta={id=da05773f key=/Tenant/1929/NamespaceTable/30/1/87/29/"projects"/4/1 pri=0.01368187 epo=0 ts=1642501152.147079958,1 min=1642501146.536358536,0 seq=160} lock=true stat=PENDING rts=1642501146.536358536,0 wto=false gul=1642501147.036358536,0
error: COMMIT; - restart transaction: TransactionRetryWithProtoRefreshError: TransactionRetryError: retry txn (RETRY_SERIALIZABLE - failed preemptive refresh): "sql txn" meta={id=da05773f key=/Tenant/1929/NamespaceTable/30/1/87/29/"projects"/4/1 pri=0.01368187 epo=0 ts=1642501152.147079958,1 min=1642501146.536358536,0 seq=160} lock=true stat=PENDING rts=1642501146.536358536,0 wto=false gul=1642501147.036358536,0
    at Parser.parseErrorMessage (/Users/jmsunseri/dev/code/flash-card-epub-builder/node_modules/pg-protocol/dist/parser.js:287:98)
    at Parser.handlePacket (/Users/jmsunseri/dev/code/flash-card-epub-builder/node_modules/pg-protocol/dist/parser.js:126:29)
    at Parser.parse (/Users/jmsunseri/dev/code/flash-card-epub-builder/node_modules/pg-protocol/dist/parser.js:39:38)
    at TLSSocket.<anonymous> (/Users/jmsunseri/dev/code/flash-card-epub-builder/node_modules/pg-protocol/dist/index.js:11:42)
    at TLSSocket.emit (events.js:400:28)
    at addChunk (internal/streams/readable.js:293:12)
    at readableAddChunk (internal/streams/readable.js:267:9)
    at TLSSocket.Readable.push (internal/streams/readable.js:206:10)
    at TLSWrap.onStreamRead (internal/stream_base_commons.js:188:23)

migrate:up works fine.

[kibertoad]

@jmsunseri Which knex version?

[jmsunseri]

"knex": "^0.95.15" Sincerely, Justin Sunseri

…

On Tue, Jan 18, 2022 at 7:05 PM Igor Savin ***@***.***> wrote: @jmsunseri <https://github.com/jmsunseri> Which knex version? — Reply to this email directly, view it on GitHub <#2002 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEPF44EWBFQQSCY6QDPNDUWVCOTANCNFSM4DGZMWTQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[dedisuryadi]

Same here, migration running fine on docker with dev mode on, but got error when migrating to remote machine, disableTransactions: true saved me, thank you @jmsunseri

migration failed with error: select * from "knex_migrations_lock" - restart transaction: TransactionRetryWithProtoRefreshError: ReadWithinUncertaintyIntervalError: read at time 1669183964.437105736,0 encountered previous write with future timestamp 1669183964.613185698,0 within uncertainty interval `t <= (local=1669183964.756284634,0, global=1669183964.937105736,0)`; observed timestamps: [{1 1669183964.437105736,0} {2 1669183964.756284634,0}]: "sql txn" meta={id=dae4d74f pri=0.00170548 epo=0 ts=1669183964.437105736,0 min=1669183964.437105736,0 seq=0} lock=false stat=PENDING rts=1669183964.437105736,0 wto=false gul=1669183964.937105736,0
select * from "knex_migrations_lock" - restart transaction: TransactionRetryWithProtoRefreshError: ReadWithinUncertaintyIntervalError: read at time 1669183964.437105736,0 encountered previous write with future timestamp 1669183964.613185698,0 within uncertainty interval `t <= (local=1669183964.756284634,0, global=1669183964.937105736,0)`; observed timestamps: [{1 1669183964.437105736,0} {2 1669183964.756284634,0}]: "sql txn" meta={id=dae4d74f pri=0.00170548 epo=0 ts=1669183964.437105736,0 min=1669183964.437105736,0 seq=0} lock=false stat=PENDING rts=1669183964.437105736,0 wto=false gul=1669183964.937105736,0
error: select * from "knex_migrations_lock" - restart transaction: TransactionRetryWithProtoRefreshError: ReadWithinUncertaintyIntervalError: read at time 1669183964.437105736,0 encountered previous write with future timestamp 1669183964.613185698,0 within uncertainty interval `t <= (local=1669183964.756284634,0, global=1669183964.937105736,0)`; observed timestamps: [{1 1669183964.437105736,0} {2 1669183964.756284634,0}]: "sql txn" meta={id=dae4d74f pri=0.00170548 epo=0 ts=1669183964.437105736,0 min=1669183964.437105736,0 seq=0} lock=false stat=PENDING rts=1669183964.437105736,0 wto=false gul=1669183964.937105736,0
    at Parser.parseErrorMessage (/Users/REDACTED/node_modules/pg-protocol/dist/parser.js:287:98)
    at Parser.handlePacket (/Users/REDACTED/node_modules/pg-protocol/dist/parser.js:126:29)
    at Parser.parse (/Users/REDACTED/node_modules/pg-protocol/dist/parser.js:39:38)
    at Socket.<anonymous> (/Users/REDACTED/node_modules/pg-protocol/dist/index.js:11:42)
    at Socket.emit (node:events:390:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at Socket.Readable.push (node:internal/streams/readable:228:10)
    at TCP.onStreamRead (node:internal/stream_base_commons:199:23)