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

sql: sequences should be automatically added to a table for postgres compatibility #26730

Closed
BramGruneir opened this issue Jun 14, 2018 · 4 comments
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-hibernate Issues that pertain to Hibernate integration. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@BramGruneir
Copy link
Member

BramGruneir commented Jun 14, 2018

This came out of hibernate's test suite, the following tests fail:

  • org.hibernate.engine.spi.ExtraStateTest shouldMaintainExtraStateWhenUsingIdentityIdGenerationStrategy
  • org.hibernate.id.CreateDeleteTest createAndDeleteAnEntityInTheSameTransactionTest
  • org.hibernate.id.FlushIdGenTest testPersistBeforeTransaction

Due to a sequence not existing.

create table ChineseTakeawayRestaurant (
  id  bigserial not null,
  gobelinStars int4 not null,
  primary key (id)
)
insert into ChineseTakeawayRestaurant (gobelinStars) 
  values 3

select currval('ChineseTakeawayRestaurant_id_seq')

Postgres automatically creates a sequence whenever a serial or big serial type is used. We don't do this.

@BramGruneir BramGruneir added A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-hibernate Issues that pertain to Hibernate integration. labels Jun 14, 2018
@vilterp
Copy link
Contributor

vilterp commented Jun 14, 2018

We could change our serial type to use sequences, but that would make it slower, so would probably be a bad choice for most users.

Without doing that, it's unclear what would trigger autocreating the sequence — maybe if we see a CREATE TABLE with my_col INT DEFAULT nextval('my_seq') we'd autocreate my_seq, but that default expression isn't what Hibernate generates anyway.

@knz
Copy link
Contributor

knz commented Jun 22, 2018

I'd like to challenge the idea that "serial using sequences would be a bad choice". I think we're gaining more with proper pg compat on this one. We should then propose a crdb-specific feature for replacement if users want more performance.

@vilterp
Copy link
Contributor

vilterp commented Jun 22, 2018

The compat win would definitely be nice. Maybe we could try switching our TPCC benchmark roachtest to use sequences, and check the perf impact? I suspect it will slow things down a lot, but I'm not sure.

Also, a lot could be done to improve perf of sequences, such as pre-allocating values in memory for each node. This seems par for the course even on single-node databases (see Postgres CACHE option).

@knz
Copy link
Contributor

knz commented Jun 22, 2018

yes a sequence cache is what I had in mind. We'll do some brainstorming about that I'm sure. The overarching problem is to solve #26925.

@knz knz added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Jul 21, 2018
craig bot pushed a commit that referenced this issue Aug 16, 2018
28575: sql: introduce virtual sequences and use with SERIAL r=knz a=knz

Very good idea by @BramGruneir 

New features: virtual sequences (opt-in) and create-sequence-upon-SERIAL (opt-in). See individual commits for details and release notes.

For reference:

- I made everything opt-in here (i.e. default to previous CockroachDB behavior) to minimize risk.
- UX with Hibernate will thus not automatically be improved. We need to inform users (and upgrade our tutorials/docs) that this experimental feature is available and how to use it to unblock problems.
- It is possible that making the new default be `virtual_sequence` will both be backward-compatible with performance and unlock UX for Hibernate users. This has @awoods187 preference I think. However I propose to only flip that switch after we've seen a week or two pass with this patch merged.

Fixes #22607 (using the new setting set to `sql_sequence`).
Fixes #24062 (using the new setting set to `sql_sequence`).
Fixes #26730 (using the new setting set to either `virtual_sequence` or `sql_sequence`).



Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig craig bot closed this as completed in #28575 Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-hibernate Issues that pertain to Hibernate integration. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

3 participants