fix(mimetype): Fix aborted transaction on PostgreSQL when storing mimetype

[lhsazevedo]

• Resolves [Bug]: OC\DB\Exceptions\DbalException: An exception occurred while executing a query: SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block #40064.

Summary

In OC\Files\Type\Loader::store() we're executing a SELECT query after an expected failed mimetype INSERT in the same transaction. MySQL implicitly rollbacks¹² the transaction after the unique constraint violation error, but PostgreSQL does not³ and expects a explicit rollback. That's why we getting the ERROR: current transaction is aborted, commands ignored until end of transaction block error.

A possible fix is to rollback before the SELECT statement. We also need to restart the transaction as atomic() will try to commit the results after calling our callback and PostgreSQL will complain if there isn't one active (MySQL won't).

$mimetypeId = $this->atomic(function () use ($mimetype) {
	try {
		$insert = $this->dbConnection->getQueryBuilder();
		$insert->insert('mimetypes')
		// (...)
		return $insert->getLastInsertId();
	} catch (DbalException $e) {   
		if ($e->getReason() !== DBException::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
			throw $e;
		}

		// If we got here, then a unique constraint
		// violation happened. and we need to rollback. 
		$this->dbConnection->rollBack();
		// Restart the transaction for atomic() to commit.
		$this->dbConnection->beginTransaction();

		$qb = $this->dbConnection->getQueryBuilder();
		$qb->select('id')
		// (...)
	}
}, $this->dbConnection);

While this works, we're now executing a single statement in each transaction and this is what the db does by default (see autocommit ⁴⁵), so we might as well not use the transaction at all and this is what I did. @tcitworld I haven't found a good reason to keep this transaction, but I still don't know the codebase in depth so would you mind checking if my proposal is correct?

Lastly, a valid concern would be the case where another connection erases the mimetype between the INSERT attempt and the SELECT query. Unfortunately transactions alone won't be enough (on MySQL at least, see isolation levels ⁶⁷). This should be extremely rare, so the exception thrown at the end of the method looks like a good enough solution for me.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

Footnotes

1. https://dev.mysql.com/doc/refman/8.0/en/innodb-error-handling.html ↩

2. https://www.burnison.ca/notes/fun-mysql-fact-of-the-day-implicit-rollbacks ↩

3. https://www.postgresql.org/docs/current/tutorial-transactions.html ↩

4. https://dev.mysql.com/doc/refman/8.0/en/innodb-autocommit-commit-rollback.html ↩

5. https://www.postgresql.org/docs/7.1/sql-begin.html#R1-SQL-BEGIN-1 ↩

6. https://dev.mysql.com/doc/refman/8.0/en/innodb-transaction-isolation-levels.html ↩

7. https://www.postgresql.org/docs/current/transaction-iso.html ↩

[lhsazevedo]

One small thing that I would like to propose as a follow up is to extract the insert/select to another method to avoid the possibility of having an undefined $mimetypeId outside the try/catch block.

Show code

protected function store($mimetype) {
	$mimetypeId = $this->storeOrRetrieve($mimetype);

	$this->mimetypes[$mimetypeId] = $mimetype;
	$this->mimetypeIds[$mimetype] = $mimetypeId;
	return $mimetypeId;
}

protected function storeOrRetrieve($mimetype) {
	try {
		$insert = $this->dbConnection->getQueryBuilder();
		$insert->insert('mimetypes')
			->values([
				'mimetype' => $insert->createNamedParameter($mimetype)
			])
			->executeStatement();
		return $insert->getLastInsertId();
	} catch (DbalException $e) {
		if ($e->getReason() !== DBException::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
			throw $e;
		}

		$qb = $this->dbConnection->getQueryBuilder();
		$qb->select('id')
			->from('mimetypes')
			->where($qb->expr()->eq('mimetype', $qb->createNamedParameter($mimetype)));
		$result = $qb->executeQuery();
		$id = $result->fetchOne();
		$result->closeCursor();

		if ($id === false) {
			throw new \Exception("Database threw an unique constraint on inserting a new mimetype, but couldn't return the ID for this very mimetype");
		}

		return (int) $id;
	}
}

[tcitworld]

The motivation for #35744 was to fix a read after write issue with async replicas, as the getLastInsertId (running on the r/o replica) didn't return anything for the INSERT (made on r/w primary). Having a transaction made sure things were being run on the same node.

So while the handling of the unique constraint violation can probably live outside the transaction, it seems important to me that both the INSERT and getLastInsertId are in the same transaction. So wrapping only the contents of the try block in an atomic might be enough? The insert would be rolled back and the exception still thrown for the catch block to catch it?

EDIT: Also if we were to wrap the whole thing with two levels of transactions (very probably a bad idea), we might need #39589 first.

[lhsazevedo]

So wrapping only the contents of the try block in an atomic might be enough?

Sounds good to me, it fixes the issue and avoid the read-after-write. Thank you, I'll have replication in mind for the next PRs.

The insert would be rolled back and the exception still thrown for the catch block to catch it?

Exactly, atomic() re-throws the exception after rolling it back:

server/lib/public/AppFramework/Db/TTransactional.php

Lines 66 to 69 in 050c6d5

	} catch (Throwable $e) {
	$db->rollBack();
	throw $e;
	}

[lhsazevedo]

/backport to stable27

[szaimen]

CI failure unrelated