[10.x] Fix Postgres cache store failed to put exist cache in transaction

[xdevor]

Problem

When using Postgres as the Cache database store,
an error occurs when performing put operation on an already existing cache key during a transaction.
The error is as follows: SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block...

Steps To Reproduce

• Step 1: after setup postgres and .env file put the following code to laravel routes/console.php

use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Facades\DB;

Artisan::command('test', function () {

    $store = Cache::store('database');

    DB::beginTransaction();

    $store->put('test-key', 'test-value');

    DB::commit();

});

• Step 2: run php artisan test twice times and the below error will show

Versions

• Postgres: v12 and v16
• PHP: 8.2
• Laravel: 10

[tpetry]

Wouldn't an upsert for all database drivers make the most sense? For existing keys they will have to execute two queries instead of just one with upserts.

[xdevor]

@tpetry 100% agree with your point. However, the reason I didn't implement it this way in this bug fix PR is because I want to minimize possible side effects. Also, I'm not entirely sure if there's a reason that led to the previous implementation not using upsert

[tpetry]

@plehatron Was there a specific reason for not using upsert?

[xdevor]

It seems there's no issue with using upsert for databases supported by Laravel. I've adjusted the statement.

[tpetry]

The add method has the same problem with PG when the insert fails because of a duplicate key. But honestly, I don't understand the code in the exception case. The logic seems to be different than described in the docblock.

[plehatron]

@plehatron Was there a specific reason for not using upsert?

Hey @tpetry, the original logic for DatabaseStore::put predates my work from #26726 where I made sure the cache component is fully compliant with the PSR-16 SimpleCache interface. Maybe @taylorotwell has more insight into why upsert was not used then 🤷‍♂️.

[xdevor]

The add method has the same problem with PG when the insert fails because of a duplicate key. But honestly, I don't understand the code in the exception case. The logic seems to be different than described in the docblock.

@tpetry seems like the add() method is designed to ensure that the cache value is not overwritten when the cache exists. However, it is challenging to prevent race conditions without lock (and lock has risk), that's why the current implementation like this.

Perhaps the only solution for now is to delete expired caches before each time add() call or periodically delete expired cache in laravel project.

[tpetry]

@tpetry seems like the add() method is designed to ensure that the cache value is not overwritten when the cache exists. However, it is challenging to prevent race conditions without lock (and lock has risk), that's why the current implementation like this.

But the logic in the catch-part is overwriting any value when the insert fails with an exception due to an existing value?

[xdevor]

But the logic in the catch-part is overwriting any value when the insert fails with an exception due to an existing value?

Because expired cache (where('expiration', '<=', $this->getTime()) ) is to be considered as not existing cache, and db will not remove the expired record automatically.

} catch (QueryException) {
        return $this->table()
            ->where('key', $key)
             ->where('expiration', '<=', $this->getTime()) // <--- only update expired cache, and expired cache is to be considered as not existing
            ->update([
                'value' => $value,
                'expiration' => $expiration,
            ]) >= 1;
}

[tpetry]

Oh you're right. didn't look could enough. Still the current transaction is aborted would also happen if the insert fails, an insertOrIgnore needs to be used here.

[xdevor]

Oh you're right. didn't look could enough. Still the current transaction is aborted would also happen if the insert fails, an insertOrIgnore needs to be used here.

@tpetry seems like possible solution ~ maybe implement like this to make sure it will not be silent while race condition occur, and call get() before insert because get() will delete expired cache automatically

// if connection is postgres and in transaction:
throw_if(
    is_null($this->table()->get($key))
    && $this->table()->insertOrIgnore(compact('key', 'value', 'expiration')) === 0,
    new \RuntimeException("Cache key '$key' has been set.")
)
return true;

UPDATE

or like this, because the current implementation of add() actually have overwrite problem while race condition occur in its error handle statement, also it keep silent while duplicate key exist, so we don't need to throw exception and should only do insert operation without any update in error handle

// for all database
return is_null($this->table()->get($key))
    && $this->table()->insertOrIgnore(compact('key', 'value', 'expiration')) > 0;

this approach fix two problem

• one is postgres error in transaction
• another one is overwritten while race condition occur in error handle statement

---

@tpetry you can open a PR for this 👍 , if you have no time or instrest I can do this