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

Model::createOrFirst() in Postgres transaction may trigger SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block #48143

Closed
mpyw opened this issue Aug 23, 2023 · 2 comments · Fixed by #48144
Labels

Comments

@mpyw
Copy link
Contributor

mpyw commented Aug 23, 2023

Laravel Version

10.20.0

PHP Version

irrelevant

Database Driver & Version

PostgreSQL (Version: irrelevant)

Description

With the recent release of Laravel v10.20.0, due to #47973, there has been an overlap in functionality with my library: mpyw/laravel-retry-on-duplicate-key, which was originally designed to offer a feature now implemented by Laravel. Although this in itself is not an issue and I intend to update my library accordingly, I've identified a potential concern with how Laravel's new feature manages Postgres transactions.

Postgres has a distinctive behavior where, if an error occurs within a transaction, all subsequent queries fail unless the current transaction is rolled back or committed. However, this can be handled by setting a SAVEPOINT, which limits the rollback to that point. In my library, for Postgres and when the transaction level is greater than 0, I've incorporated a mechanism using DB::transaction() to create a SAVEPOINT. If Laravel's new feature doesn't adopt a similar approach to handle Postgres transactions, it could lead to errors: SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block.

class RetryOnDuplicateKey
{
    protected ConnectionInterface $connection;

    public function __construct(ConnectionInterface $connection)
    {
        $this->connection = $connection;
    }

    /**
     * Retries once on duplicate key errors.
     *
     * @param mixed ...$args
     * @return mixed
     */
    public function __invoke(callable $callback, ...$args)
    {
        try {
            return $this->withSavepoint(fn () => $callback(...$args));
        } catch (PDOException $e) {
            if ((new UniqueViolationDetector($this->connection))->violated($e)) {
                $this->forceReferringPrimaryConnection();
                return $this->withSavepoint(fn () => $callback(...$args));
            }
            throw $e;
        }
    }

    /**
     * Make sure to fetch the latest data on the next try.
     */
    protected function forceReferringPrimaryConnection(): void
    {
        $connection = $this->connection;

        if ($connection instanceof Connection) {
            $connection->recordsHaveBeenModified();
        }
    }

    /**
     * @phpstan-template T
     * @phpstan-param callable(): T $callback
     * @phpstan-return T
     * @return mixed
     * @noinspection PhpDocMissingThrowsInspection
     * @noinspection PhpUnhandledExceptionInspection
     */
    protected function withSavepoint(callable $callback)
    {
        return $this->needsSavepoint()
            ? $this->connection->transaction(fn () => $callback())
            : $callback();
    }

    protected function needsSavepoint(): bool
    {
        // In Postgres, savepoints allow recovery from errors.
        // This ensures retrying should work also in transactions.
        return $this->connection instanceof PostgresConnection
            && $this->connection->transactionLevel() > 0;
    }
}

Steps To Reproduce

User::create(['email' => 'example@example.com'], ['name' => 'example']);

DB::transaction(function () {
    User::createOrFirst(['email' => 'example@example.com'], ['name' => 'example']);
})
@mpyw
Copy link
Contributor Author

mpyw commented Aug 23, 2023

CC @tonysm

@tonysm
Copy link
Contributor

tonysm commented Aug 23, 2023

Ah, nice catch. Let me write some tests for that.

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

Successfully merging a pull request may close this issue.

3 participants