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

Issues with PG Named Prepared Statements #1028

Closed
jamatthews opened this Issue Jul 16, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@jamatthews

jamatthews commented Jul 16, 2017

Hello! Really appreciate the work you've been doing so far with Diesel, especially enabling easier untyped SQL statements and typed multi table joins in the recent releases.

The first issue I have is that using named prepared statements causes PG to cache generic query plans. Often this results in horrific performance issues if you have a non-uniform data distribution, if the cached plan is expecting 5 rows and using a loop join, but one row in your table actually joins to 5.7 million rows in the other table. Cached prepared statements also rule out using constraint exclusion on partitioned tables and scan all partitions. I'm working around this by adding sql::<Bool>("TRUE = TRUE") to queries which would otherwise be cached. Allegedly this has been fixed in more recent versions of PG, but I'm still experiencing the issue in 9.6.

The second problem is the change in #686 from using the single statement PQexecParams to PQprepare and PQexecPrepared breaks the ability to use an external connection pool like pgbouncer in "transaction pooling" mode. I think I can work around this issue for now by wrapping each call in an explicit transaction.

Would you consider reverting the change away from using PQexecPrepared? I'm happy to open a PR to optionally disable named prepared statements if you would accept it?

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 16, 2017

There are a lot of deeper issues here which warrant discussion, but the short answer is that yes I would accept a pull request which provides an option to globally disable prepared statement caching.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 17, 2017

So just to address a few of the specific points here:

The first issue I have is that using named prepared statements causes PG to cache generic query plans.

This actually isn't true at all. Generally query planning occurs when the bind parameters are initially passed in. Postgres may decide to cache a generic plan, but it will only do so for queries which are frequently executed, and when the generic plan is determined to be roughly as efficient as the plan created with specific values. I'd be surprised if generic plans were being cached in the cases that you've described. Then again, you also sound like you've specifically isolated the issue so ¯\_(ツ)_/¯

The second problem is the change in #686 from using the single statement PQexecParams to PQprepare and PQexecPrepared breaks the ability to use an external connection pool like pgbouncer in "transaction pooling" mode.

Can you elaborate on this for me? The only difference between using PQexecParams and what we're doing now is that one additional Sync message is sent between the Parse and Bind/Execute messages. The only way that could cause an issue is if the connection is returned to the pool in between those two round trips, which should not occur even in statement pooling mode. It certainly should not occur in transaction pooling mode, since the connection is held for the entire transaction.

Unrelated to the problem you're describing there, I am generally interested in seeing if we can generate deterministic statement names so that they actually can be shared when using pg bouncer (this would require graceful re-preparing behavior since we can no longer guarantee that a statement being in our cache means that it's been prepared on the real connection)

@sgrif

This comment has been minimized.

Member

sgrif commented Aug 5, 2017

Since there isn't any activity on this issue, and there are no actionable items for us to take at this time, I'm going to close this issue.

@sgrif sgrif closed this Aug 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment