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

Investigate alternatives to required PgBouncer configuration #2453

Closed
janpio opened this issue May 14, 2020 · 5 comments
Closed

Investigate alternatives to required PgBouncer configuration #2453

janpio opened this issue May 14, 2020 · 5 comments
Assignees
Labels
kind/improvement An improvement to existing feature and code. tech/engines Issue for tech Engines. topic: connection-pooler/pgbouncer topic: prepared statement does not/already exist/s Named prepared statement related error messages
Milestone

Comments

@janpio
Copy link
Member

janpio commented May 14, 2020

We recently became aware that DO's and Heroku's PgBouncer do not allow configuration as we advise, making it impossible for Prisma users to use them in front of their Postgres databases.

In an internal document we researched and documented the problem and its causes that make it necessary to have these 2 configuration options to use PgBouncer. Additionally I came up with 4 suggestion on how we could avoid having to use these configuration options, making it possible for our users to user PgBouncer of DO and Heroku.

After internal discussion we want to look into suggestions 2 and 3 deeper:

  1. Clean up manually before/after each transaction
    a) We could send DEALLOCATE ALL manually after each transaction (faking what the PgBouncer configuration would do automatically).
    b) We could send DEALLOCATE ALL manually before each transaction (making sure we have a clean slate before trying to prepare the query).

  2. Clean up manually if clash was detected
    We could catch the prepared statement "s2" already exists error when trying to prepare a query, manually send a DEALLOCATE ALL (or even DEALLOCATE s2) query, and then try to prepare the query again.

We are not super sure if these suggestion actually work (2a could be problematic if we can actually send the query before we lose our connection by committing a transaction, 3 could be problematic with losing the transaction (and thus connection) on the error).

Hence we need to investigate.

Comments:
If possible we prefer 3 over 2 as it affects less queries.
If 3 is not possible, 2a more mirrors the PgBouncer behavior, but 2b might be easier to achieve.

@janpio janpio added kind/improvement An improvement to existing feature and code. topic: connection-pooler/pgbouncer tech/engines Issue for tech Engines. labels May 14, 2020
@janpio
Copy link
Member Author

janpio commented May 14, 2020

An idea re suggestion 3:

This might possibly be able to be verified via pure queries against a normal Postgres server:

  1. prepare a query as s1
  2. start a transaction
  3. prepare a query as s1 to cause the clash
  4. run deallocate s1 or deallocate all
  5. start transaction (if lost on clash)
  6. prepare a query as s1
  7. execute query s1
  8. commit the transaction

If this works, we "just" need to find a way to express that in our code.
Let me know if this doesn't make sense or does not achieve the stated goal of testing suggestion 3.

@janpio janpio added this to the Beta 6 milestone May 14, 2020
@janpio janpio changed the title Investigate alternatives to PgBouncer configuration Investigate alternatives to required PgBouncer configuration May 14, 2020
@pimeys
Copy link
Contributor

pimeys commented May 14, 2020

3 is less queries, but then in case of an error, that failing query needs to:

  • start a new transaction
  • DEALLOCATE ALL
  • execute the statements once again

This means a non-deterministic spike on response times that's random. In the failing case, if the original transaction was three queries, the procedure runs the three queries + BEGIN + COMMIT + DEALLOCATE ALL, total of six extra queries. Would be reasonable to spread the load to all the queries to be more deterministic. This will also lead to a smaller amount of new code.

@pimeys
Copy link
Contributor

pimeys commented May 14, 2020

A quick test shows this is possible:

use postgres::{Client, NoTls};

fn main() {
    let mut client =
        Client::connect("host=localhost user=postgres password=prisma", NoTls).unwrap();

    let mut tx = client.transaction().unwrap();
    tx.execute("DEALLOCATE ALL", &[]).unwrap();

    for row in tx.query("SELECT 1 as integer", &[]).unwrap() {
        dbg!(row);
    }

    tx.commit().unwrap();
}

So the plan would be to introduce in Postgres connector in Quaint a new builder option .on_transaction("DEALLOCATE ALL"). Then when calling start_transaction on PostgreSQL, it would first call whatever SQL is stored to the on_transaction part and then give the transaction out to use. This would spread out the load to all the requests in a controlled manner and be quite straightforward to implement.

Then QE would turn this bit on if needed.

@janpio
Copy link
Member Author

janpio commented May 15, 2020

I verified suggestion 3 on a SQL level as well:

-- make sure session state is empty
deallocate all;
-- tx for context
select txid_current();
-- create s1 first time
prepare s1 as INSERT INTO test values ('foo');

-- start transaction as our code would
begin;
-- confirm new tx
select txid_current();
-- prepare again to trigger error we would get
prepare s1 as INSERT INTO test values ('bar');

-- here the transaction is aborted and you get 
-- "SQL Error [25P02]: ERROR: current transaction is aborted, commands ignored until end of transaction block" 
-- if you run any further queries

-- so we need to clean up
rollback;
begin;
-- confirm new transaction
select txid_current();
-- get rid of old s1 in session
deallocate s1;
-- prepare, execute and commit cleanly now
prepare s1 as INSERT INTO test values ('bar');
execute s1;
select txid_current();
commit;
select txid_current();

So if we catch error 25P02, we can rollback, start a new transaction, deallocate the prepared statement and then execute as we tried before.

The table test has the value bar inserted when we are done with this series of queries.

@janpio
Copy link
Member Author

janpio commented May 19, 2020

Investigation concluded: Suggestion 3 would probably work as well, but is a bit hard to put into the architecture we have. Suggestion 2 is a more blunt hammer but seems to work as a replacement for the custom configuration of PgBouncer.

@janpio janpio closed this as completed May 19, 2020
@janpio janpio added the topic: prepared statement does not/already exist/s Named prepared statement related error messages label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/improvement An improvement to existing feature and code. tech/engines Issue for tech Engines. topic: connection-pooler/pgbouncer topic: prepared statement does not/already exist/s Named prepared statement related error messages
Projects
None yet
Development

No branches or pull requests

3 participants