Errors of statements at PqHandle

deadbeat88 commented 5 years ago

Using the pq extension sometimes get the following errors:

at PqHandle::statementExecute: Named statement not found when executing
at PqHandle::prepare: prepared statement \"amp_c1e68597d8be2371adc09335382ad9f18ab407dd\" already exists

These errors occur only with a high number of requests per second, more than 150. The following results were obtained (total queries 5000-6000, using pool with max_coonnections = 90, timeout = 30 sec)

failed=19, rps=150-200
failed=0, rps=250-300
failed=7, rps=250-300
failed=11, rps=350-400
failed=25, rps=350-400
failed=40, rps=400-450

Also at PqHandle::statementExecute after success assert i had pq\Connection state like this: "status": 0, "transactionStatus": 0, "errorMessage": "ERROR: prepared statement \"amp_c1e68597d8be2371adc09335382ad9f18ab407dd\" does not exist"

trowski commented 5 years ago

Can you please try with the latest master and see if the problem persists?

deadbeat88 commented 5 years ago

These are solved with the latest update, but the pool never releases connections now)

trowski commented 5 years ago

I missed that I created a reference to the statement object in PqHandle, so I reverted to a similar approach as before, but I realized that a refCount was being incremented prior where it should not have been. Can you please try again with the latest master?

deadbeat88 commented 5 years ago

I tried again and got the similar errors: prepared statement "amp_xxx" does not exist or prepared statement "amp_xxx" already exists

trowski commented 5 years ago

Does the same thing happen if you use the pgsql extension?

deadbeat88 commented 5 years ago

I use docker postgres:9.6 and own image based on php:7.3-cli-alpine.

with event (\Amp\Loop\EventDriver) and pq extensions I get errors above.
with event and pgsql extensions I get log "incomplete startup packet" from postgres and docker container crashes with error "Segmentation fault"
with NativeDriver and pgsql or pq I get the same errors as with event extension

trowski commented 5 years ago

Is it possible you were preparing similar queries with different placeholders? For example, SELECT * FROM users WHERE id=:id and SELECT * FROM users WHERE id=?. I noticed this would result in the same prepared statement name on the connection. I fixed this in master, if you would again give that a try.

deadbeat88 commented 5 years ago

No, all queries are preparing without named placeholders. There are the same errors after latest fix( Also I noticed some weird behavior during test cases:

the maximum number of connections is reached extremely quickly and pool releases only part of connections even after few minutes. I checked extra/unnecessary references in my application several times, often use "unset" function + periodic forced launch of the garbage collector (Loop::repeat() every 5 minutes).
it seems that сondition (isset($this->statements[$name])) at PqHandle::prepare is never satisfied, let alone increasing of "refCount". It is strange, because about half of queries are have the same values of $sql and $modifiedSql variables, and there are errors like "prepared statement "amp_xxx" already exists"

trowski commented 5 years ago

I spent some more time looking at this today. I use this library in production without issue, so I'm surprised your seeing errors and I'm not. I'm still unable to reproduce the errors you're seeing.

Addressing your points:

The maximum number of connections is quickly reached because the pool caches statement pools, which themselves hold connections anticipating reuse. This may not have been a wise design decision, see below.
When a connection from the pool is used to prepare a statement it is marked as used and will not be used again to prepare a statement (or do any other action) until the statement object is destroyed. Thus two statements will not be prepared on a connection when the connection is part of a pool. isset($this->statement[$name]) will never be true within PqHandle::prepare() for a pooled connection.

I pushed a new branch, remove-statement-pool-caching, that does not retain statement pools within the connection pool, which will free resources much more quickly, but will require more prepares if the same statement is being used over and over again. You can retain a statement object manually for some time if you anticipate using it again. The statement pool will still free connections associated with it in time, preparing a new statement if it is used. Could you please try out this branch and see how your application behaves with it? Thanks!

deadbeat88 commented 5 years ago

I ran few tests without statement pools within the connection pool at new branch remove-statement-pool-caching and there are no any issues)

mmasiukevich commented 5 years ago

Perhaps this information will help a little:

The problem manifested itself mainly (99% of cases) on UPDATE requests. According to the structure, there was a BYTEA field, and it was filled with a rather large value.

https://github.com/php-service-bus/sagas/blob/v3.2/src/Store/Sql/SQLSagaStore.php#L132

However, I can not reproduce the problem as I did not try. There is clearly a problem somewhere, but at what stage it is not clear.

mmasiukevich commented 5 years ago

With the latest changes, the error is not reproduced. Maybe it makes sense to release?

trowski commented 5 years ago

@mmasiukevich Tagged a new release with the changes! Thanks for the input and please let me know if you have any further issues.

deadbeat88 commented 5 years ago

@trowski Nice, thanks)

amphp / postgres

Errors of statements at PqHandle #19