feat: Add Cockroachdb support without transactions #16394

divye11 · 2023-08-17T17:36:05Z

Pull Request Checklist

Have you added new tests to prevent regressions?
If a documentation update is necessary, have you opened a PR to the documentation repository?
Did you update the typescript typings accordingly (if applicable)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Does the name of your PR follow our conventions?

Description Of Change

This PR is an ongoing effort to split feat: Add CockroachDb support #15569 into two PRs. This PR integrates cockroachdb to sequelize without supporting transactions.

Todos

Implement a util function to add ids to objects where we add ...(dialect === 'cockroachdb' && { id: 1 })

…ports

…t and overrid postgres helpers for cockroachDB

…ort for crdb

…file, addressed some review comments

…lint file

…hange in validtor-js package

WikiRik

I just have a few more files that I need to check out in the cockroachdb folder but it looks very good to me. In the end it's not too many tests that add an id, but adding a util function for it in the future might be nice. Not for this PR imo.

WikiRik · 2023-09-05T18:34:51Z

CONTRIBUTING.md

@@ -233,6 +236,7 @@ We use a simple conventional commits convention:
  - `db2`
  - `ibmi`
  - `snowflake`
+  - `cockroachdb`


This should be added to the scopes in .github/workflows/pr-title.yml as well

WikiRik · 2023-09-05T18:38:03Z

packages/core/test/unit/sql/delete.test.js

      it(util.inspect(options, { depth: 2 }), () => {
+
        return expectsql(


Suggested change

it(util.inspect(options, { depth: 2 }), () => {

return expectsql(

it(util.inspect(options, { depth: 2 }), () => {

return expectsql(

WikiRik · 2023-09-05T18:50:58Z

packages/core/test/unit/sql/insert.test.js

-const dialectName   = Support.getTestDialect();
+const current = Support.sequelize;
+const sql = current.dialect.queryGenerator;
+const dialectName = Support.getTestDialect();


Suggested change

const dialectName = Support.getTestDialect();

WikiRik · 2023-09-05T18:52:54Z

packages/core/test/unit/sql/literal.test.ts

@@ -71,7 +71,7 @@ describe('json', () => {



The two tests above should produce the same result for cockroachdb as well right? You can add those already

WikiRik · 2023-09-05T18:54:15Z

packages/core/test/unit/sql/select.test.js

+        default: `SELECT [user].*, [POSTS].[id] AS [POSTS.id], [POSTS].[title] AS [POSTS.title] FROM (SELECT [user].[id_user] AS [id], [user].[email], [user].[first_name] AS [firstName], [user].[last_name] AS [lastName] FROM [users] AS [user] ORDER BY [user].${TICK_LEFT}${TICK_LEFT}${TICK_LEFT}last_name${TICK_RIGHT}${TICK_RIGHT}${TICK_RIGHT} ASC${sql.addLimitAndOffset({
+          limit: 30,
+          offset: 10,
+          order: [['`user`.`last_name`', 'ASC']],
+        })
        }) AS [user] LEFT OUTER JOIN [post] AS [POSTS] ON [user].[id_user] = [POSTS].[user_id] ORDER BY [user].${TICK_LEFT}${TICK_LEFT}${TICK_LEFT}last_name${TICK_RIGHT}${TICK_RIGHT}${TICK_RIGHT} ASC;`,
-        ibmi: `${`SELECT "user".*, "POSTS"."id" AS "POSTS.id", "POSTS"."title" AS "POSTS.title" FROM (SELECT "user"."id_user" AS "id", "user"."email", "user"."first_name" AS "firstName", "user"."last_name" AS "lastName" FROM "users" AS "user" ORDER BY "user"."""last_name""" ASC`}${
-          sql.addLimitAndOffset({ limit: 30, offset: 10, order: [['`user`.`last_name`', 'ASC']] })
+        ibmi: `${`SELECT "user".*, "POSTS"."id" AS "POSTS.id", "POSTS"."title" AS "POSTS.title" FROM (SELECT "user"."id_user" AS "id", "user"."email", "user"."first_name" AS "firstName", "user"."last_name" AS "lastName" FROM "users" AS "user" ORDER BY "user"."""last_name""" ASC`}${sql.addLimitAndOffset({ limit: 30, offset: 10, order: [['`user`.`last_name`', 'ASC']] })
        }) AS "user" LEFT OUTER JOIN "post" AS "POSTS" ON "user"."id_user" = "POSTS"."user_id" ORDER BY "user"."""last_name""" ASC`,


Could you undo the changes to these lines?

WikiRik · 2023-09-05T20:01:07Z

packages/core/test/integration/model/create.test.js

@@ -849,7 +855,11 @@ describe(Support.getTestDialectTeaser('Model'), () => {
          await User.sync({ force: true });
          const user = await User.create({}, { returning: true });
          expect(user.get('id')).to.be.ok;
-          expect(user.get('id')).to.equal(1);
+
+          // This test expects the ID to be 1 but in CockroachDB uses UUIDv4 to generate IDs.


Is there a different expectation we can add instead?

We could manually add id = 1 for the record or fetch the record ID and compare if needed, but seemed redundant.
Let me know your thoughts @WikiRik

WikiRik · 2023-09-05T20:01:45Z

packages/core/test/integration/model/create.test.js

+
+        await userWithDefaults.sync({ force: true });
+        const user = await userWithDefaults.create({});
+        // uuid validation regex taken from http://stackoverflow.com/a/13653180/800016


You can use the isUUID validator for this

WikiRik · 2023-09-05T20:03:47Z

packages/core/test/integration/model/findOne.test.js

        expect(user2).to.not.be.null;
        await t.rollback();
      });

-      // Disabled in sqlite because it only supports one connection at a time
+      // Disabled in sqlite and cockroachdb because it only supports one connection at a time


Suggested change

// Disabled in sqlite and cockroachdb because it only supports one connection at a time

// Disabled in sqlite and partially for cockroachdb because it only supports one connection at a time

WikiRik · 2023-09-05T20:04:05Z

packages/core/test/integration/model/findOrBuild.test.js


+const dialectName = Support.getTestDialect();


Suggested change

const dialectName = Support.getTestDialect();

WikiRik · 2023-09-05T20:11:14Z

packages/core/test/integration/query-interface.test.js

@@ -570,8 +576,8 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => {
      this.User = this.sequelize.define('users', {


I'll review the tests below here after you've resolved the conflicts with the main branch

…b-support-without-transactions

rafiss · 2023-11-24T06:13:45Z

@WikiRik Hi! CockroachDB developer here - do you know if this PR is able to get reviewed?

…:divye11/sequelize-cockroachdb into cockroachdb-support-without-transactions

WikiRik · 2024-03-15T16:42:19Z

@divye11 are you able to resolve the merge conflicts so we can take another look at this?

ephys

Thank you for your work!

I have reviewed the source code part. I will be reviewing the tests next

ephys · 2024-04-03T07:38:05Z

dev/cockroachdb/latest/start.sh

+ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600;
+ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600;


Suggested change

ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600;

ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600;

ALTER RANGE default CONFIGURE ZONE USING \"gc.ttlseconds\" = 600;

ALTER DATABASE system CONFIGURE ZONE USING \"gc.ttlseconds\" = 600;

for https://github.com/koalaman/shellcheck/wiki/SC2140

ephys · 2024-04-03T07:38:22Z

dev/cockroachdb/oldest/start.sh

+ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600;
+ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600;


Suggested change

ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600;

ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600;

ALTER RANGE default CONFIGURE ZONE USING \"gc.ttlseconds\" = 600;

ALTER DATABASE system CONFIGURE ZONE USING \"gc.ttlseconds\" = 600;

ephys · 2024-04-03T07:41:44Z

packages/core/src/dialects/cockroachdb/data-types.ts

+
+export class INTEGER extends PostgresInteger {
+  sanitize(value: number): unknown {
+    if (value > Number.MAX_SAFE_INTEGER) {


If the value is > MAX_SAFE_INTEGER, it is already not reliable and should not be converted. This should throw an error

As that is already what BaseIntegerDataType does, there are no changes needed here, you can just re-export PostgresInteger as INTEGER

ephys · 2024-04-03T07:44:28Z

packages/core/src/dialects/cockroachdb/data-types.ts

+import type { AbstractDataType, AcceptableTypeOf, ArrayOptions, BindParamOptions, DataType } from '../abstract/data-types';
+import * as BaseTypes from '../abstract/data-types';
+import { attributeTypeToSql } from '../abstract/data-types-utils';
+import { ARRAY as PostgresArray, GEOGRAPHY as PostgresGeography, INTEGER as PostgresInteger } from '../postgres/data-types';


Now that postgres has been moved to its own package, you will need to add an exports config to postgres similar to what is done in core to access the internals (the _non-semver-use-at-your-own-risk_ export)

ephys · 2024-04-03T07:49:07Z

packages/core/src/dialects/cockroachdb/data-types.ts

+    } else if (this.options.type instanceof BaseTypes.HSTORE) {
+      ValidationErrorItem.throwDataTypeValidationError('cockroachdb does not support the HSTORE data type.\n See https://www.cockroachlabs.com/docs/stable/data-types/ for a list of supported data types.');
+    } else if (this.options.type instanceof BaseTypes.UUID) {
+      ValidationErrorItem.throwDataTypeValidationError('cockroachdb does not support the UUID data type.\n See https://www.cockroachlabs.com/docs/stable/data-types/ for a list of supported data types.');
+    } else if (this.options.type instanceof BaseTypes.CITEXT) {
+      ValidationErrorItem.throwDataTypeValidationError('cockroachdb does not support the CITEXT data type.\n See https://www.cockroachlabs.com/docs/stable/data-types/ for a list of supported data types.');
+    }


These error messages are redundant, as the types themselves will throw if they are not supported

The relevant support options to set to make them throw are:

supports.dataTypes.HSTORE

supports.dataTypes.CITEXT

Note that there is not one for UUID, as all dialects should support it (with a fallback to a binary field or text when not available). From the documentation you linked, it looks like cockroach does support UUID though

The error message about nested array is fine though, as it's an interaction between this type and its subtype

ephys · 2024-04-03T08:00:20Z

packages/core/src/dialects/cockroachdb/query-generator.ts

+    ]);
+  }
+
+  getUuidV1FunctionCall(): string {


getUuidV4FunctionCall is fine as it has a different implementation, but getUuidV1FunctionCall is the same implementation so this method can be removed

ephys · 2024-04-03T08:02:29Z

packages/core/src/dialects/cockroachdb/query-interface.ts

+    try {
+      await super.removeConstraint(tableName, constraintName, options);
+    } catch (error: any) {
+      if (error.message.includes('use DROP INDEX CASCADE instead')) {


I don't think the code should try to correct for this. It should alert the user, and the user should then use the cascade option themselves, that option is already supported in RemoveConstraintQueryOptions

ephys · 2024-04-03T08:03:49Z

packages/core/src/dialects/postgres/connection-manager.ts

@@ -57,7 +58,7 @@ export class PostgresConnectionManager extends AbstractConnectionManager<PgConne
  #oidMap = new Map<number, TypeOids>();
  #oidParserCache = new Map<number, TypeParser<any, any>>();

-  constructor(dialect: PostgresDialect, sequelize: Sequelize) {
+  constructor(dialect: PostgresDialect | CockroachDbDialect, sequelize: Sequelize) {


the postgres connection manager cannot have a direct reference to cockroachdb, it will create an package loop between @sequelize/postgres and @sequelize/cockroachdb.

After the merge of main in this PR, we can look into how this can be solved. I think the solution would be to have CockroachDbDialect extend PostgresDialect, but that may require moving some init code somehow

ephys · 2024-04-03T08:05:52Z

packages/core/src/model.js

@@ -857,7 +857,7 @@ ${associationOwner._getAssociationDebugList()}`);
    const missingIndexes = this.getIndexes()
      .filter(item1 => !existingIndexes.some(item2 => item1.name === item2.name))
      .sort((index1, index2) => {
-        if (this.sequelize.options.dialect === 'postgres') {
+        if (this.sequelize.options.dialect === 'postgres' || this.sequelize.options.dialect === 'cockroachdb') {


Following merge of main in this PR, keep in mind that this.sequelize.options.dialect should not be used anymore. Use this.sequelize.dialect.name instead

ephys · 2024-04-03T08:06:27Z

packages/core/src/dialects/cockroachdb/query.js

+import forOwn from 'lodash/forOwn';
+import isEqual from 'lodash/isEqual';
+
+const debug = logger.debugContext('sql:pg');


Suggested change

const debug = logger.debugContext('sql:pg');

const debug = logger.debugContext('sql:cockroachdb');

divye11 and others added 30 commits December 7, 2022 00:03

initial commit

458bf3a

updated docker compose to build a single cluster with proper exposed …

2fbdddd

…ports

Merge branch 'sequelize:main' into main

a6cee3a

Merge branch 'sequelize:main' into main

ad7f647

updated data types, connection manager, and query generator to inheri…

28c7c1a

…t and overrid postgres helpers for cockroachDB

Merge branch 'main' of github.com:divye11/sequelize-cockroachdb

7589ecd

Merge branch 'sequelize:main' into main

5719cb3

updated query, query generator, types, query interface and added supp…

9686cdb

…ort for crdb

Merge branch 'main' of https://github.com/sequelize/sequelize

203e3ca

Merge branch 'sequelize:main' into main

e05df1f

changed file structure

3187642

Merge branch 'sequelize:main' into main

8eee4a4

removed outdated files. removed .d.ts files, migrated to TS for each …

b0ca8d5

…file, addressed some review comments

Merge branch 'main' of github.com:divye11/sequelize-cockroachdb

5432da2

linter fix

a0a02e5

Merge branch 'sequelize:main' into main

d32dda7

linter fix

5caa5f4

added config variables

335ddfe

Merge branch 'sequelize:main' into main

aa92184

merged main from upstream

848beb7

Merge branch 'sequelize:main' into main

fb69929

improved overrides, imports, exports. added a linter rule in inner es…

4131a92

…lint file

removed linting fixes

5f1c3ce

merged main into divye11:main

0969c83

updated few test configs to add crdb ports, reverted a import/order c…

bb1c975

…hange in validtor-js package

Merge branch 'main' of github.com:sequelize/sequelize

63a72f0

Merge branch 'main' of github.com:sequelize/sequelize

379d476

reverted a few changes, removed array definition for crdb data type

415c5ea

linter fix

eaac9d6

build fix

4a24e83

divye11 marked this pull request as draft August 17, 2023 17:36

divye11 added 4 commits August 17, 2023 23:19

meta: changed comment

e663bc0

meta: changed comment

1ec03e4

meta: updated nx cache

e2beef3

added a comment for the unique parsing for int8 in cockroachdb

fcd8b9b

WikiRik reviewed Sep 5, 2023

View reviewed changes

divye11 and others added 5 commits September 17, 2023 20:26

fix: resolve wsay fixes for review comments

abf412b

meta: merged upstream mai

7db6d8b

fix: fixed conflicts

c26e6ab

Merge branch 'main' into cockroachdb-support-without-transactions

39dd7a7

Merge branch 'main' of github.com:sequelize/sequelize into cockroachd…

adee21f

…b-support-without-transactions

divye11 and others added 5 commits November 27, 2023 12:32

changed enum storage procedure

23d33e8

Merge branch 'cockroachdb-support-without-transactions' of github.com…

1f78847

…:divye11/sequelize-cockroachdb into cockroachdb-support-without-transactions

Merge branch 'main' into cockroachdb-support-without-transactions

f669df1

fix: merged upstream main and resolved conflicts

e022e64

Merge branch 'cockroachdb-support-without-transactions' of github.com…

2b1003f

…:divye11/sequelize-cockroachdb into cockroachdb-support-without-transactions

divye11 marked this pull request as ready for review January 30, 2024 11:11

fix: merged upstream main and resolved conflicts

f7a8507

WikiRik requested a review from ephys January 30, 2024 15:39

divye11 and others added 4 commits January 31, 2024 16:13

Merge branch 'main' into cockroachdb-support-without-transactions

5226fe8

fix: UT fix for uuid

6eb1b18

merged upstream main into origin main

5255738

Merge branch 'main' into cockroachdb-support-without-transactions

66dea65

divye11 requested a review from WikiRik February 2, 2024 07:45

Merge branch 'main' into cockroachdb-support-without-transactions

ef9a12b

divye11 requested a review from a team as a code owner March 18, 2024 23:04

ephys requested changes Apr 3, 2024

View reviewed changes

sequelize-bot bot added the conflicted This PR has merge conflicts and will not be present in the list of PRs to review label Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Cockroachdb support without transactions #16394

feat: Add Cockroachdb support without transactions #16394

divye11 commented Aug 17, 2023

WikiRik left a comment

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

divye11 Feb 2, 2024

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

WikiRik Sep 5, 2023

rafiss commented Nov 24, 2023

WikiRik commented Mar 15, 2024

ephys left a comment

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

ephys Apr 3, 2024

		it(util.inspect(options, { depth: 2 }), () => {

		return expectsql(

	// Disabled in sqlite and cockroachdb because it only supports one connection at a time
	// Disabled in sqlite and partially for cockroachdb because it only supports one connection at a time

		@@ -570,8 +576,8 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => {
		this.User = this.sequelize.define('users', {

		ALTER RANGE default CONFIGURE ZONE USING "gc.ttlseconds" = 600;
		ALTER DATABASE system CONFIGURE ZONE USING "gc.ttlseconds" = 600;

	const debug = logger.debugContext('sql:pg');
	const debug = logger.debugContext('sql:cockroachdb');

feat: Add Cockroachdb support without transactions #16394

Are you sure you want to change the base?

feat: Add Cockroachdb support without transactions #16394

Conversation

divye11 commented Aug 17, 2023

Pull Request Checklist

Description Of Change

Todos

WikiRik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss commented Nov 24, 2023

WikiRik commented Mar 15, 2024

ephys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment