Worker Coordinator fails with PgBouncer due to unsupported statement_timeout #205
Comments
|
Hi @mfrister, thanks for the report -- we'd intended to do more testing with PgBouncer to make sure everything looked good there, but hadn't gotten around to it yet. One question before I start: what pooling mode are you using PgBouncer in? I didn't test it, but reading through the features docs in PgBouncer [1],
Blake would probably have a different answer on this, but I think realistically, no.
Yeah, we're not married to the |
|
I'm assuming that this is related, but I have not been able to get river to work with supabase they use a proprietary connection pooling system called supavisor. The error output from river is below when using session pooling. Happy to help with any testing here and or opening up a new issue. |
We had an issue opened a little while back (#171) wherein it turned out that River couldn't operate with pgx's simple protocol turned on, which appeared to be because the simple protocol doesn't know anything about a target Postgres tuple's makeup and therefore had to guess how to encode parameters based on input types, which was leading `[]byte` intended for `jsonb`s to be binary encoded, which isn't allowed. We closed that issue after I pointed out that PgBouncer now supports prepared statement pooling, and therefore simple protocol is no longer necessary. We recently got another comment [1] where someone had tried to use River with Supavisor on Supabase, which also does not supported prepared statements, leading to lots of errors. Normally, pgx's simple protocol could be used to address this, but as we saw in #171, River doesn't support that. Here, a proof of concept demonstrating how River could support simple protocol by modeling `jsonb` fields in sqlc as strings, which prevents pgx from encoding to binary with simple procotol, and we get to a point where all driver tests are passing. I'm not sure if we should action on this one or not. Having to fall back to `string` fields a bit gross, although I don't *think* carries significant performance penalty because it's basically the same underlying structure as far as Go is concerned. [1] #205 (comment)
|
Interesting. I've done a little investigating on this, and I'd love to get supavisor working, but it looks like it might be a bit of a yak shake. Supavisor does support prepared statements now [1], but only in transaction mode, and of course the client needs to start in session mode. Unfortunately, the only way to stop Pgx from using prepared statements is to have it downgrade to "simple protocol", and again unfortunately, we got a report in #171 that simple protocol doesn't work because sqlc tries to encode our jsonb input as binary, which Postgres does not allow. So I think we could potentially try to solve this in two ways:
The trouble with (1) is that even if we got that fixed, Supavisor also doesn't support listen/notify [2], so you'd probably just run into more problems. (2) seems plausible, but it's a bit of a job, and it may not be the most important thing we should be working on at the moment. |
|
Hi @brandur, thanks for your quick reply! Sorry it took me so long - I wasn’t working last week. I’m using PgBouncer in session mode. I looked a bit further into this and while PgBouncer supports For the startup message, PgBouncer unfortunately only supports a very limited number of parameters and Maybe setting a Thanks for discussing and being open to using contexts instead of How would you like to continue? |
|
@mfrister out of curiosity, does your setup give you any ability to access the raw (non-pgbouncer) url? One of the setups we’ve been considering for pgbouncer users is to allow specifying two pgx pools, or maybe one pool and an additional conn config: one for general use, and the latter solely for the notify listener. However if you are ultimately just using Supabase and your setup doesn’t support listen/notify at all, this is a moot point and we probably can’t support this kind of infrastructure as of now. There’s a fair bit of stuff dependent on the pub/sub setup and probably will be more in the future. And although we’ve talked about ways to “bring your own pubsub” it’s probably not on the short term priority list. |
FWIW, the |
|
Further context on my idea above: each River client that's working jobs needs a single dedicated Postgres connection that's used solely for listening for notifications. This is the only connection that actually needs to be able to support Postgres All of the other connections used within the client are for regular short-lived queries that can easily share a larger connection pool with the rest of your application and/or pass through pgbouncer in txn mode, but the listener's connection really needs either a direct DB connection or at least pgbouncer in session mode. |
|
@bgentry Our setup currently doesn't allow me to access Postgres directly, only via PgBouncer. I'll only get a limited number of connections via the session-based PgBouncer, so I'm already using the session-based PgBouncer for the worker coordinator only and I'm using a separate connection pool for the workers themselves that uses a transaction-based PgBouncer. So as far as I understand, using the coordinator with a separate connection pool might already work well enough, but maybe it's still opening more connections than I expect and having a single connection for LISTEN makes sense (haven't put it into production yet, only in a test environment with a single worker process). We're not using Supabase at all, so I'd expect us to be able to use NOTIFY/LISTEN. I've already tried the worker with the
🎉 Thanks a lot! This probably solves this issue for me. I'll try the unreleased master tomorrow and report back whether it works. |
|
I tested commit b2cb142 and it works with our PgBouncer/Postgres setup. Thanks again for the quick fix! I'll close the issue, given that the issue I originally reported is now fixed. If I notice issues regarding too many connections for the session-based PgBouncer, I'll open a separate one. Feel free to reopen this, if you want to further discuss Supabase support or something else in here, of course. |
We had an issue opened a little while back (#171) wherein it turned out that River couldn't operate with pgx's simple protocol turned on, which appeared to be because the simple protocol doesn't know anything about a target Postgres tuple's makeup and therefore had to guess how to encode parameters based on input types, which was leading `[]byte` intended for `jsonb`s to be binary encoded, which isn't allowed. We closed that issue after I pointed out that PgBouncer now supports prepared statement pooling, and therefore simple protocol is no longer necessary. We recently got another comment [1] where someone had tried to use River with Supavisor on Supabase, which also does not supported prepared statements, leading to lots of errors. Normally, pgx's simple protocol could be used to address this, but as we saw in #171, River doesn't support that. Here, a proof of concept demonstrating how River could support simple protocol by modeling `jsonb` fields in sqlc as strings, which prevents pgx from encoding to binary with simple procotol, and we get to a point where all driver tests are passing. I'm not sure if we should action on this one or not. Having to fall back to `string` fields a bit gross, although I don't *think* carries significant performance penalty because it's basically the same underlying structure as far as Go is concerned. [1] #205 (comment)
Thanks for the extensive documentation on using river with PgBouncer!
I’m still having a small issue with river and PgBouncer - when the worker tries to connect, it fails (wrapped for better readability):
PgBouncer doesn’t seem to support the
statement_timeoutparameter. Disabling the statement_timeout in the Notifier by removing the line makes the worker run. The comment above this line indicates that the timeout is necessary, though:Side Note: PgBouncer does seem to have an option to ignore certain startup parameters, but I’m a bit wary of doing so, given that ignoring the statement timeout might have hard-to-notice side effects for other applications. I’d rather have the applications fail while connecting and adapt them however necessary.
My questions:
I’m happy to submit a PR if you think a change in river makes sense.
The text was updated successfully, but these errors were encountered: