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

Clustered environments Javers will incorrectly version records. #877

Closed
duane-staehli opened this issue Sep 6, 2019 · 6 comments

Comments

@duane-staehli
Copy link

commented Sep 6, 2019

Issue:
User on node 1 inserts a record (version 1, initial snapshot is inserted).
User on node 2 updates the record (version 2, update is inserted).
User on node 3 updates the record (version 2, update is inserted). Note this is incorrect is should be version 3.

JV_SNAPSHOT table example.

snapshot_pk type version
5292412 UPDATE 2
5292604 UPDATE 2
5292701 INITIAL 1

What is should look like:
snapshot_pk | type | version |
--------------|------|-----------
5292412 | UPDATE | 3 |
5292604 | UPDATE | 2 |
5292701 | INITIAL | 1 |

Problem Code (I think):
https://github.com/javers/javers/blob/master/javers-persistence-sql/src/main/java/org/javers/repository/sql/finders/CdoSnapshotFinder.java#L104

The logic seems to be select the max pk and then find the version number from that.

I think here you are assuming the numbers are always representative of insert order. But there is many cases where this would not be true. In this case since the first node that did the insert was ahead in snapshot_pk, subsequent inserts are always ignored for selection as the latest version.

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

Seems that in clustered environments Javers should not use the Sequence Allocation trick, see https://github.com/javers/javers/blob/master/javers-persistence-sql/src/main/java/org/javers/repository/sql/session/Sequence.java#L12

Without the Sequence Allocation trick, primary keys ordering would be guaranteed by the database sequence.

Sequence nextval call should be inlined in the insert into jv_snapshot statement as it is not used anywhere as a foreign key.

bartoszwalacik added a commit that referenced this issue Sep 7, 2019
https://github.com//issues/877
inlined sequence nextval for jv_snapshot inserts
bartoszwalacik added a commit that referenced this issue Sep 7, 2019

@bartoszwalacik bartoszwalacik added the fixed label Sep 7, 2019

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

fixed in 5.7.1

@duane-staehli

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

Hi Bartosz,

Just some general questions about this fix.

It appears that you were multiplying the sequence number by a hundred as some sort of caching mechanism.

e.g.
The current value of our sequence in the DB for JV_SNAPSHOT_PK_SEQ is 52930.
But the values I'm seeing in JV_SNAPSHOT (for one record, this was updated across four nodes):
SNAPSHOT_PK's = 5292702, 5292417, 5292605, 5292416, 5292413, 5292412, 5292604, 5292701

Meaning I need to promote the JV_SNAPSHOT_PK_SEQ to SNAPSHOT_PK + 1?

Are there any other Sequences I need to promote? Your change appears to be to very generic code, so I assume you changed all the sequences to use this behaviour?

While I manage the JV tables/sequences (I've disabled schema management), I didn't see code in the commit that would automatically promote other sequences in the managed cased.
You may also want to put a release note to indicate that people who are managing the schema need to update their sequences if they don't let Javers manage the sequence.

Thanks,

Duane

@bartoszwalacik bartoszwalacik removed the fixed label Sep 10, 2019

@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Looks like the bigger problem is in this line in Sequence.java

currentValue = SEQUENCE_ALLOCATION_SIZE * currentSequenceValue ;

So when SEQUENCE_ALLOCATION trick is discontinued, db sequences should be multiplied by 100. I need to check it. For now, version 5.7.1 has to be withdrawn.

bartoszwalacik added a commit that referenced this issue Sep 10, 2019
https://github.com//issues/877
crud *100 fix to keep backward compatibility
@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

In the new fix, sequence.nextval is multipilied by 100. Sequences will not have to be incremented manually.

bartoszwalacik added a commit that referenced this issue Sep 11, 2019
@bartoszwalacik

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

fixed in 5.7.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.