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

Fix ConcurrencyCheck + DatabaseGeneratedOption.Computed #23

Closed

Conversation

tohagan
Copy link

@tohagan tohagan commented May 1, 2018

Optimistic locking via a database generated field has been regularly reported as an issue and awaiting a fix for over 2.5 years now. It's required whenever you need to perform optimistic locking with an external non-EF application.

The bug reports below all use a Timestamp (DateTime) field which I think can fail due to rounding when converting C# DateTime => MySQL timestamp or datetime. So in the unit tests I've instead used a BIGINT RowVersion column with using a BEFORE UPDATE trigger to increment the field.

    [ConcurrencyCheck, DatabaseGenerated(DatabaseGeneratedOption.Computed)]
    [Column(TypeName = "bigint")]
    public virtual long RowVersion { get; set; }

Example:

CREATE TRIGGER `trg_mytable_before_update` 
      BEFORE UPDATE ON `mytable`
      FOR EACH ROW SET NEW.RowVersion = OLD.RowVersion + 1;

NOTE: I've replace row_count() > 0 with row_count() = 1 as this also checks that the Id is unique.

I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

@mysql-oca-bot
Copy link

Hi, thank you for submitting this pull request. In order to consider your code we need you to sign the Oracle Contribution Agreement (OCA). Please review the details and follow the instructions at http://www.oracle.com/technetwork/community/oca-486395.html
Please make sure to include your MySQL bug system user (email) in the returned form.
Thanks

@tohagan
Copy link
Author

tohagan commented May 1, 2018

A minor fix to this PR ... In the unit test ConcurrencyCheckWithDbGeneratedColumn() ...
You might want to remove DEFAULT CHARSET=binary clause from CREATE TABLE MovieReleases2 since the Name field is type VARHCAR (not VAR BINARY). I kept this the same for the older unit test that I've renamed to ConcurrencyCheckWithNonDbGeneratedColumn.

@tohagan
Copy link
Author

tohagan commented May 2, 2018

Sent OCA.

@tohagan
Copy link
Author

tohagan commented May 3, 2018

TIMESTAMP Solution?

I'm also investigating how to performing optimistic locking with a TIMESTAMP field.

Firstly, you need to use a more fine grained timestamp value.

So for example if you use the following, your timestamp value will be truncated to the nearest second (not very safe for optimistic locking).

UpdatedAt TIMESTAMP NOT NULL DEFAULT CURRENT_TIME ON UPDATE CURRENT_TIME

Instead you should use following to record microsecond precision.

UpdatedAt TIMESTAMP(6) NOT NULL DEFAULT NOW(6) ON UPDATE NOW(6)

Secondly, I'm observing a bug that I'm reproducing within the environment of the MySQL .NET Connector unit test suite combined with the PR patch I've just submitted. EF6 now generates the correct optimistic locking SQL to perform an UPDATE followed by the SELECT (now fixed) that returns the updated TIMESTAMP field. However the MySQL connector returns a zero TIMESTAMP (0000-00-00 00:00:00.000000) even though executing the exact same UPDATE and SELECT in MySQL Workbench it returns a valid non-zero TIMESTAMP value. I've observed the packets read via the connection socket return the string '0000-00-00 00:00:00.000000' so its probably related to the MySQL session configuration in some way.

Hints welcome!

I'm currently testing this with MySQL v5.6.26 (Windows). It might be fixed in a more recent MySQL server version.

@tohagan
Copy link
Author

tohagan commented May 14, 2018

I've spent some time this past week investigating why TIMESTAMP fields are not working for optimistic locking within the test suite environment of this project. The behaviour I described above appears to be related to the isolation level of the transaction since we're seeing what appears to be an old value of the TIMESTAMP returned. I've attempted to mimic the RowVersion solution that we know works by replacing

 UpdatedAt TIMESTAMP(6) NOT NULL DEFAULT NOW(6) ON UPDATE NOW(6)

with a trigger

  CREATE TRIGGER `trg_mytable_before_update` 
  BEFORE UPDATE ON `mytable` FOR EACH ROW SET NEW.UpdatedAt = NOW(6);

However I still observe the same result (SELECT UpdatedAt returns a ZERO datetime after the trigger should have run). I've run this inside a transaction set to IsolationLevel.ReadUncommitted but surprisingly this still yields the the same behaviour. Tracing confirms that the session isolation level is being set though tracing is displaying the output from SHOW_VARIABLES as

 LoadServerProperties: tx_isolation=REPEATABLE-READ

So I suspect something in this test environment is setting the wrong isolation level. The default isolation level for EF6 with SQL Server is ReadUncommitted.

Due my project commitments, I won't be submitting a further PR for the TIMESTAMP issue at this time.

@mysql-oca-bot
Copy link

Hi, thank you for your contribution. Please confirm this code is submitted under the terms of the OCA (Oracle's Contribution Agreement) you have previously signed by cutting and pasting the following text as a comment:
"I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it."
Thanks

@tohagan
Copy link
Author

tohagan commented May 26, 2018

I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

@mysql-oca-bot
Copy link

Hi, thank you for your contribution. Your code has been assigned to an internal queue. Please follow
bug http://bugs.mysql.com/bug.php?id=91064 for updates.
Thanks

@tohagan
Copy link
Author

tohagan commented Aug 20, 2018

Will this fix become part of 6.10.x ?

@tohagan
Copy link
Author

tohagan commented May 28, 2019

Sadly this critical fix never made it into the next release and so it was lost!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants