ESI-11623 perform locking fetch on Delete operation

[marcin-ptaszynski]

Transaction Commit Informer performs a fetch before deleting resource.
In order to have consistency and not race REAPEATABLE_READ view and
READ_COMMITED view when mixing locking and non-locking SELECTS in
SERIALIZABLE transactions.

Transaction Commit Informer performs a fetch before deleting resource.
In order to have consistency and not race consistent snapshot with
committed data. E.g:

TX1 is REPEATABLE READ
TX1 Makes some unrelated SELECTS (consistent snapshot established)
TX2 Inserts row X into table test and commits
TX1 SELECTS FOR UPDATE a couple of rows, including newly inserted X
TX1 Decides to DELETE X, but fails with Not Found
(X doesn't exist in consistent snapshot).

[nati]

Could you add an example for this? The result seems same for both serialization level for me.

[marcin-ptaszynski]

@nati , DELETE performs locking, because it's a DML Statement. And locks matching row, so performing locking operation 1 query earlier can't cause any any problems.

You're right. SERIALIZABLE Selects don't use consistent snapshot. The issue still occurred, though. Exact scenario is following:

1. TX1 Makes some unrelated SELECTS (consistent snapshot established)
2. TX2 Inserts row X into table test and commits
3. TX1 SELECTS FOR UPDATE a couple of rows, including newly inserted by TX2
4. TX1 Decides to DELETE X, but fails with Not Found Error during pre-SELECT pointed in code.

I've checked manually and 4 doesn't fail for SERIALIZABLE (LOCK IN SHARE MODE finds newly inserted rows), while it obviously fails for REPEATABLE READ. This points to a conclusion that in MySQL driver, Transaction Isolation Level is not properly set. This is strange, because after fix #458 we no longer do it incorrectly manually (which causes TX levels to be pretty much random), but instead use standard library to do it.

Documentation doesn't mention it, but it seems it silently fails to apply TxOptions if driver doesn't implement new interface (checked in runtime) and falls back to oldBegin() if context is root (this validation should be before this if). Vendored go-mysql library doesn't support new BeginTx, so TxOptions were never applied.

Fortunately, last week go-mysql enabled BeginTx support, so we can use it correctly after updating vendor package. Created separate PR #478 to bump it. Nope

In any case, I think this PR is still needed for cases where transaction is not Serializable (e.g. custom actions can create any type of transaction).

[nati]

it sounds like you are simulating the behavior of SERIALIZED on REPEATABLE READ by some coding. Why not just use serializable?

[marcin-ptaszynski]

@nati ,
Transaction isolation setting never worked in Gohan, so we need to fix it, but before that happens, we're using mysql default and I'd rather stick to REPEATABLE READ than SERIALIZABLE, which may introduce more deadlocks between read/write-locks.

Even when SERIALIZABLE is fixed for CUD actions, on rare occasions it's better to use REPEATABLE READ, when writing (e.g. we need to perform some aggregation and we have no need to lock all the related rows). This commit is only required for such rare cases.

Also, aside from other considerations, this PR is overall improvement (pre-fetch follows same locking rules as delete).

[nati]

I supposed that one was fixed on some PR.. let's fix it.

I'm not still seeing any difference between applying SERIALIZABLE for delete transaction.

[marcin-ptaszynski]

@nati ,
Unfortunately, we've wrongly assumed that delegating work to stdlib would solve the issue, since as documentation of stdlib BeginTx states:

// The provided TxOptions is optional and may be nil if defaults should be used.
// If a non-default isolation level is used that the driver doesn't support,
// an error will be returned. 

Which is not true as explained above.

@zimnx is adding BeginTx with Isolation Level to go-mysql driver currently, so we can have proper control after his PR is merged and we update vendored code.

[marcin-ptaszynski]

@nati , FYI go-sql-driver/mysql#619

[marcin-ptaszynski]

In any case, I still think we should merge this PR regardless. There is no guarantee that delete operation will be performed in Serializable transaction (e.g. custom actions).

[marcin-ptaszynski]

@nati , FYI, we've switched from SERIALIZABLE to READ_COMMITTED, as we've had too many problems with deadlocks caused by Read Locks.

[przemyslaw-dobrowolski-cl]

@marcin-ptaszynski please rebase

[nati]

Ok got it. LGTM

[przemyslaw-dobrowolski-cl]

@marcin-ptaszynski This PR seems to be very old and does not merge. Could you resolve the conflicts?
@morrisson @yudai Could you review this change?