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

[WIP] EZP-28681: Legacy transaction handling issues #1380

Closed
wants to merge 4 commits into from

Conversation

2 participants
@glye
Copy link
Member

glye commented Jul 30, 2018

Fixes https://jira.ez.no/browse/EZP-28681
Work in progress

glye added some commits Jul 30, 2018

EZP-28681: Legacy transaction handling issues
2. Attributes deleted during draft discard
EZP-28681: Legacy transaction handling issues
3. Status change during draft discard

@glye glye changed the title Ezp28681 legacy transaction handling [WIP] EZP-28681: Legacy transaction handling issues Jul 30, 2018

@gggeek

This comment has been minimized.

Copy link
Contributor

gggeek commented Jul 30, 2018

See comments in ezsystems/ezautosave#27 ...

$this->_commit( $fname );
if ( $this->_commit( $fname ) === true )

This comment has been minimized.

Copy link
@glye

glye Jul 30, 2018

Author Member

This is problematic, I know. Aside from the issue of adding a return value to _commit, what happens if the commits are recursive and null is returned? The files won't be deleted, but if the outer commit eventually goes through, we would want them to be. OTOH if we did delete them in the null case, and the outer commit was rolled back, we'd have the opposite problem.

@glye glye force-pushed the ezp28681_legacy_transaction_handling branch 2 times, most recently from ad71eda to 860dd08 Aug 2, 2018

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Aug 2, 2018

Update for 4. Current version changed during publish
Lock the affected rows in share mode. Means we must query ezcontentobject even when $versionNum is specified. This Mysql specific and must be moved to the eZDB abstraction layer. Just testing it here now.

@glye glye force-pushed the ezp28681_legacy_transaction_handling branch 2 times, most recently from 6e4a244 to 8939f5d Aug 2, 2018

@gggeek

This comment has been minimized.

Copy link
Contributor

gggeek commented Aug 2, 2018

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Aug 2, 2018

@gggeek Yes...? Nice to know, but I'm afaik not creating any rows in the commit for point 4, so don't see a problem with counters. I am writing to ezcontentobject_version, but didn't see any reason to lock it for reading. If someone else is trying to set the status of the same version at the same time, it shouldn't matter if they read the old or the new row data - the action doesn't depend on the status. But chances are I've missed something :)

@gggeek

This comment has been minimized.

Copy link
Contributor

gggeek commented Aug 2, 2018

The thing is, if you do 'select for update' you will not prevent others to do plain selects. But others transactions having the same select for update' will queue, which is what we want.
Reading that SO page makes me thing that using 'lock in share mode' will not prevent another update transaction to start its operation

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Aug 3, 2018

The docs say that "lock in share mode" will cause attempts by others at exclusive lock (update) will have to wait. Also that "for update" will block others from "reading the data in certain transaction isolation levels". https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html

So I went with share mode in both cases since I saw no need to block reads. But since the second lock is for a table that we will actually update, it is maybe safest to lock "for update" in that case.

@gggeek

This comment has been minimized.

Copy link
Contributor

gggeek commented Aug 3, 2018

Correct. Generally speaking, you want to lock as few tables as possible, to avoid the risk of ending up with the 'dining philosophers' situation, where threads deadlock each other.
The 'select for update' that you do should be:

  • on the main table holding the data you want to update (in this case most likely version and/or object)
  • used to ascertain that the state of the data is good to start the transaction (ie. no checking of version status in a previous select, as that would not be atomic)

ps: seems that mysql at least supports select-for-update on a join... https://stackoverflow.com/questions/38544770/mysql-can-i-use-one-select-for-update-to-protect-multiple-tables-lock

glye added some commits Jul 30, 2018

EZP-28681: Legacy transaction handling issues
4. Current version changed during publish
EZP-28681: Legacy transaction handling issues
5. Files deleted during DFS cache purge

@glye glye force-pushed the ezp28681_legacy_transaction_handling branch from 8939f5d to ab9bdae Aug 3, 2018

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Aug 3, 2018

I locked "for update" in the second query now since I found a hard reason to do so: A read lock doesn't stop other sessions from also getting a read lock. That other lock would block my update.

Now to find a way to do this properly within the eZDB framework... :hurtrealbad:

@glye

This comment has been minimized.

Copy link
Member Author

glye commented Nov 12, 2018

Closing. I'll deal with these in separate PRs instead:

  1. ezsystems/ezautosave#27
  2. #1397
  3. #1398
  4. #1399
  5. #1400

@glye glye closed this Nov 12, 2018

@glye glye deleted the ezp28681_legacy_transaction_handling branch Nov 12, 2018

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