Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CockroachDB session settings for migrations #12113

Closed
Tracked by #12385
tomhoule opened this issue Mar 2, 2022 · 1 comment · Fixed by prisma/prisma-engines#2889
Closed
Tracked by #12385

CockroachDB session settings for migrations #12113

tomhoule opened this issue Mar 2, 2022 · 1 comment · Fixed by prisma/prisma-engines#2889
Assignees
Labels
kind/feedback Issue for gathering feedback. team/schema Issue for team Schema. topic: cockroachdb topic: migrate
Milestone

Comments

@tomhoule
Copy link
Contributor

tomhoule commented Mar 2, 2022

In its current state, Migrate relies on the following session variables for diffing to function properly:

SET enable_experimental_alter_column_type_general = true;
SET default_int_size = 4;
SET serial_normalization = 'sql_sequence';

These are session variables that are set on a per-connection basis and not persisted on the server. The same options can be set to the same or different values with SET CLUSTER SETTING.

Let's review what they do:

  • enable_experimental_alter_column_type_general allows migrating the type of a column in an ALTER TABLE statement, with restrictions (depending on column types, and whether the column is indexed). It makes the experience better, and we count on this becoming the default behaviour, so this is fairly unproblematic.
  • default_int_size makes INTEGER map to INT4, i.e. a 32-bit integer instead of the default 64 bit. This is enabled to align with postgresql, on which our diffing logic is based. We could change that, it's just more convenient for our tests and our internal diffing logic.
  • serial_normalization = 'sql_sequence' ensures we can have SERIAL columns that are not BigInt, since that's implicit in the default value of rowid. Without this, the "traditional" Prisma id Int @default(autoincrement()) is not possible (it will always be a BigInt).

Open questions

  • Do we want to handle any combination of server settings? For example in case of default_int_size, we would need to adjust the diffing logic based on default_int_size. More problematic, for serial_normalization, we would either reject valid schemas during prisma schema validation, or allow schemas that will not work on all servers (it depends on the server setting, which we don't know until we have connected).
  • Assuming we do not want to support any combination of server settings, we will use session variables like we do now. Is it significantly better to stick to the cockroachdb defaults in the value of these variables?
  • Do we want to avoid ambiguous column types like INTEGER and always use the cockroach-specific INT2, INT4 and INT8 types for cockroachdb migrations?
tomhoule added a commit to prisma/prisma-engines that referenced this issue Mar 9, 2022
We use the cockroach-specific `INT4` to avoid integer sizing
ambiguities. For the complete solution, we will have to deal with the
different type of autoincrementing primary keys, because two out of the
there force INT8.

Addresses the default_int_size part of prisma/prisma#12113
tomhoule added a commit to prisma/prisma-engines that referenced this issue Mar 9, 2022
The gist of this commit is that we have Int and Bigint in the PSL. On all connectors, Int means 32 bit signed integer, and BigInt means 64 bit signed integer.

On Postgres, we map Int to INTEGER in a create table, and that's good and accurate.

On cockroachdb before this commit, we map Int to INTEGER, but the size of INTEGER depends on the default_int_size configuration (session or server) param, and by default it's 8. This commit goes around that by mapping int to INT4 to reflect Prisma expectations. BigInt is still mapped to BIGINTEGER, which is correct.

We use the cockroach-specific `INT4` to avoid integer sizing
ambiguities. For the complete solution, we will have to deal with the
different type of autoincrementing primary keys, because two out of the
there force INT8.

Addresses the default_int_size part of prisma/prisma#12113
Weakky pushed a commit to prisma/prisma-engines that referenced this issue Mar 11, 2022
The gist of this commit is that we have Int and Bigint in the PSL. On all connectors, Int means 32 bit signed integer, and BigInt means 64 bit signed integer.

On Postgres, we map Int to INTEGER in a create table, and that's good and accurate.

On cockroachdb before this commit, we map Int to INTEGER, but the size of INTEGER depends on the default_int_size configuration (session or server) param, and by default it's 8. This commit goes around that by mapping int to INT4 to reflect Prisma expectations. BigInt is still mapped to BIGINTEGER, which is correct.

We use the cockroach-specific `INT4` to avoid integer sizing
ambiguities. For the complete solution, we will have to deal with the
different type of autoincrementing primary keys, because two out of the
there force INT8.

Addresses the default_int_size part of prisma/prisma#12113
@Jolg42 Jolg42 added this to the 3.12.0 milestone Mar 16, 2022
@janpio janpio modified the milestones: 3.12.0, 3.13.0 Apr 11, 2022
tomhoule added a commit to prisma/prisma-engines that referenced this issue May 6, 2022
This PR implements the proposal on cockroachdb autoincrementing columns for
the introspection and migration engines, and adapts tests where
necessary in the query engine test suite.

This PR is a best effort implementation given the time constraints — it
makes a number of design and implementation compromises to ship the
changes.

**Overview of the changes**

On the cockroachdb connector:

- `@default(autoincrement())` now maps to the default mechanism in
  cockroachdb, `DEFAULT unique_rowid()`. Since that makes the column
  a 64 bit integer column automatically, this
  `@default(autoincrement())` is only supported on `BigInt` fields.`
- A new `@default(sequence())` maps to the usage of sequences in the
  database, which is not the recommended default on cockroachdb but
  closely matches how postgres autoincrement columns work. The
  `sequence()` function can take parameters defined in the PSL proposal.

**Tests**

The main difficulty is with the tests. Now that we map
`@default(autoincrement())` to `DEFAULT unique_rowid()` on cockroachdb
(implicitly through the use of SERIAL), autoincrementing integers in
cockroachdb prisma schemas cannot be `Int` anymore, only `BigInt`. This
makes most of the introspection and migration tests that are portable
between databases not portable to cockroachdb. In this PR, we pick a
subset we still want to test and adapt them to cockroachdb, and ignore
the rest. This is obviously less than ideal.

In the query engine tests, we adapt this similarly, with the additional
difficulty that values produced by unique_rowid() are not stable and do
not start at one. The tests that were not passing are special-cased for
cockroachdb and spread between schemas with `@default(sequence())`
(closely matches postgres @default(autoincrement())) and tests that use
the cockroach default `@default(unique_rowid())`.

An introspection and migrations test setup rewrite initiative is in the
plans.

**Limitations**

- In migrations, we do not diff the settings of existing sequences yet.

closes prisma/prisma#12239
closes prisma/prisma#12113
tomhoule added a commit to prisma/prisma-engines that referenced this issue May 6, 2022
This PR implements the proposal on cockroachdb autoincrementing columns for
the introspection and migration engines, and adapts tests where
necessary in the query engine test suite.

This PR is a best effort implementation given the time constraints — it
makes a number of design and implementation compromises to ship the
changes.

**Overview of the changes**

On the cockroachdb connector:

- `@default(autoincrement())` now maps to the default mechanism in
  cockroachdb, `DEFAULT unique_rowid()`. Since that makes the column
  a 64 bit integer column automatically, this
  `@default(autoincrement())` is only supported on `BigInt` fields.`
- A new `@default(sequence())` maps to the usage of sequences in the
  database, which is not the recommended default on cockroachdb but
  closely matches how postgres autoincrement columns work. The
  `sequence()` function can take parameters defined in the PSL proposal.

**Tests**

The main difficulty is with the tests. Now that we map
`@default(autoincrement())` to `DEFAULT unique_rowid()` on cockroachdb
(implicitly through the use of SERIAL), autoincrementing integers in
cockroachdb prisma schemas cannot be `Int` anymore, only `BigInt`. This
makes most of the introspection and migration tests that are portable
between databases not portable to cockroachdb. In this PR, we pick a
subset we still want to test and adapt them to cockroachdb, and ignore
the rest. This is obviously less than ideal.

In the query engine tests, we adapt this similarly, with the additional
difficulty that values produced by unique_rowid() are not stable and do
not start at one. The tests that were not passing are special-cased for
cockroachdb and spread between schemas with `@default(sequence())`
(closely matches postgres @default(autoincrement())) and tests that use
the cockroach default `@default(unique_rowid())`.

An introspection and migrations test setup rewrite initiative is in the
plans.

**Limitations**

- In migrations, we do not diff the settings of existing sequences yet.

closes prisma/prisma#12239
closes prisma/prisma#12113
closes prisma/prisma#12244
@janpio janpio modified the milestones: 3.13.0, 3.14.0 May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feedback Issue for gathering feedback. team/schema Issue for team Schema. topic: cockroachdb topic: migrate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@janpio @Jolg42 @floelhoeffel @tomhoule and others