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

[10.x] Ensuring Primary Reference on Retry in createOrFirst() #48161

Merged
merged 3 commits into from
Aug 27, 2023

Conversation

mpyw
Copy link
Contributor

@mpyw mpyw commented Aug 24, 2023

Summary

This PR refines the createOrFirst() method to guarantee referencing the primary database when re-attempting record retrieval after a UniqueConstraintViolationException.

Details:

The existing createOrFirst() method, when facing a unique constraint violation, retries by fetching the matching record. However, in environments with replication, there's no assurance that the newly created record is immediately available on replicas due to possible replication lags.

To address this:

public function createOrFirst(array $attributes = [], array $values = [])
{
    try {
        return $this->getQuery()->withSavepointIfNeeded(fn () => $this->create(array_merge($attributes, $values)));
    } catch (UniqueConstraintViolationException $exception) {
        return $this->useWritePdo()->where($attributes)->first();
    }
}

With this change, in case of a retry, the method explicitly references the primary database using useWritePdo(), ensuring immediate and accurate record retrieval without being affected by replication delays.

@mpyw
Copy link
Contributor Author

mpyw commented Aug 24, 2023

We need to fix unit tests with Mockery. Okay

@mpyw mpyw marked this pull request as draft August 24, 2023 06:44
@mpyw mpyw marked this pull request as ready for review August 25, 2023 11:58
@taylorotwell
Copy link
Member

Why did you only update one of the createOrFirst methods?

@mpyw
Copy link
Contributor Author

mpyw commented Aug 25, 2023

Oops, sorry. I've just noticed that there are more createOrFirst methods. I'll check it out, thanks

@mpyw mpyw marked this pull request as draft August 25, 2023 15:18
@mpyw mpyw force-pushed the improve/use-write-pdo-for-retry branch from 1f0e298 to 70fde6f Compare August 25, 2023 15:35
@mpyw mpyw marked this pull request as ready for review August 25, 2023 15:43
@tonysm
Copy link
Contributor

tonysm commented Aug 25, 2023

@mpyw doesn't Eloquent automatically switches to the write connection after a write attempt (INSERT, UPDATE, or DELETE)? I thought about that in the original PR but I thought it was the case.

The docs say that the sticky option can be used to control that and the example for using read/write replicas has it set to true, so I assumed that was the "default" behavior:

The sticky option is an optional value that can be used to allow the immediate reading of records that have been written to the database during the current request cycle. If the sticky option is enabled and a "write" operation has been performed against the database during the current request cycle, any further "read" operations will use the "write" connection. This ensures that any data written during the request cycle can be immediately read back from the database during that same request. It is up to you to decide if this is the desired behavior for your application.

@mpyw
Copy link
Contributor Author

mpyw commented Aug 26, 2023

@tonysm I am aware of this feature, but I believe the read connection actually switches to the primary only after a successful write attempt.

/**
* Execute an SQL statement and return the boolean result.
*
* @param string $query
* @param array $bindings
* @return bool
*/
public function statement($query, $bindings = [])
{
return $this->run($query, $bindings, function ($query, $bindings) {
if ($this->pretending()) {
return true;
}
$statement = $this->getPdo()->prepare($query);
$this->bindValues($statement, $this->prepareBindings($bindings));
$this->recordsHaveBeenModified();
return $statement->execute();
});
}
/**
* Run an SQL statement and get the number of rows affected.
*
* @param string $query
* @param array $bindings
* @return int
*/
public function affectingStatement($query, $bindings = [])
{
return $this->run($query, $bindings, function ($query, $bindings) {
if ($this->pretending()) {
return 0;
}
// For update or delete statements, we want to get the number of rows affected
// by the statement and return that back to the developer. We'll first need
// to execute the statement and then we'll use PDO to fetch the affected.
$statement = $this->getPdo()->prepare($query);
$this->bindValues($statement, $this->prepareBindings($bindings));
$statement->execute();
$this->recordsHaveBeenModified(
($count = $statement->rowCount()) > 0
);
return $count;
});
}

According to this, statement pre-emptively calls recordsHaveBeenModified, whereas the actual affectingStatement used by create calls it post-execution. This indicates that recordsHaveBeenModified is not called when an exception is thrown in execute. Therefore, after catching an exception and correcting the primary reference, this mechanism does not work, and we need to explicitly apply useWritePdo in our code.

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

Successfully merging this pull request may close these issues.

3 participants