pgbouncers break postgres connection settings

[richvdh]

we carefully set up the transaction isolation level and bytea encoding for each connection to postgres, but of course pgbouncer can open new connections which don't get these settings, which can cause breakage such as incorrectly-encoded json being stored in the database

[richvdh]

We should either avoid depending on these settings, or, if we really can't help it, figure out how to replicate the settings in pgbouncer and document it.

(It would be really nice to avoid setting bytea_output at all. I think it only really matters when you are doing something silly like trying to store bytes in text columns, for which see #4475.)

[slipeer]

Problem can be quickly solved by adding to postgresql.conf:

bytea_output = 'escape'

[thegcat]

If the pgbouncer is used for synapses only this should not be a problem, should it?

[thegcat]

Case in point we have switched to synapse with a few workers, and this causes a lot of idle connections most of the time, which could probably be handled by pgbouncer?

[richvdh]

to update this slightly: #4475 is now fixed, so we no longer rely on the bytea_output setting. However we still set the transaction isolation level for each connection (to REPEATABLE_READ). The default is READ_COMMITTED.

I'm not really sure how much this matters in practice. @erikjohnston any thoughts on that?

[erikjohnston]

I don't know where and how we rely on REPEATABLE_READ, so without auditing all transactions its hard to know if its safe to use READ_COMMITTED (though if we can move to read committed I think that'd help with DB perf).

I'd have thought that there must be some way to get pgbouncer to respect isolation levels? Or maybe we can get psycopg2 to explicitly set the transcation level when it sends a BEGIN statement?

[richvdh]

I'd have thought that there must be some way to get pgbouncer to respect isolation levels?

I believe connect_query could be used for this: https://www.pgbouncer.org/config.html#section-databases.

That said, I very much feel like the right solution is for us to BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; for transactions that need it (which is very few of them, I believe) rather than set it on the connection level.

Maybe we could do this with an option to runInteraction which indicates whether we need to change the isolation level (defaulting to "yes"). Over time we can go through our transactions and turn the option off for transactions we are happy don't require this.

[richvdh]

That said, I very much feel like the right solution is for us to BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ; for transactions that need it (which is very few of them, I believe) rather than set it on the connection level.

opened #11567 to track this

[richvdh]

Given that connection-level settings for the isolation level are actually emulated within psycopg (https://github.com/psycopg/psycopg2/blob/8ef195f2ff187454cc709d7857235676bb4176ee/psycopg/pqpath.c#L372), I actually don't think it's correct that pgbouncers do break the isolation level setting.

I'm going to close this pending evidence.