setReadOnly does not work with pgbouncer and transaction pooling mode

[eshkinkot]

https://github.com/pgjdbc/pgjdbc/blob/REL42.1.1/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java#L698

Currently setReadOnly change mode for all session, not for transaction. It breaks work through pgbouncer in transaction pooling mode.

Can this be changed? For example like in psycopg2 python driver, see:
https://github.com/psycopg/psycopg2/blob/2_7_1/psycopg/pqpath.c#L504

psycopg2 can set read only as part of BEGIN clause, like:
BEGIN READ ONLY;
this work perfectly over pgbouncer in transaction pooling mode too.

[davecramer]

haven't looked at too many details, but it seems reasonable in autocommit = false mode this would be possible, yes.

[jamestenglish]

Is there any update on this? I am trying to use pg bouncer in transaction pooling mode and running into this issue as well

[davecramer]

Pull requests are welcome

[dan-aksenov]

Got same problem here. Any chance we get fix?

[davecramer]

So the harder problem here is creating the test case. Can someone provide the specification of what they want this to do, and a configuration for pgbouncer which exhibits this ?

[jamestenglish]

@eshkinkot @dan-aksenov

It appears setting this to 1 will resolve this particular issue, the problem is I don't know if it in turn induces any other issues

server_reset_query_always
Whether server_reset_query should be run in all pooling modes. When this setting is off (default), the server_reset_query will be run only in pools that are in sessions-pooling mode. Connections in transaction-pooling mode should not have any need for reset query.

It is workaround for broken setups that run apps that use session features over transaction-pooled pgbouncer. Is changes non-deterministic breakage to deterministic breakage - client always lose their state after each transaction.

@davecramer I'd love to try to make a PR but I am not even sure where to start.
The specifics of what I would like accomplished is for the driver to not set anything on the session:
https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
Lines: 699, 838, 1501, 1504

Pg Bouncer in transaction mode pools the connections and expects that there are no session level things set.

So let's take just the READ ONLY issue, line 699, for instance. A client connects, that gets invoked on the session, work is done. Then Pg Bouncer puts that connection that can only perform READ ONLY work into its pool another connection takes it. If that connection tries to perform READ WRITE work, it bombs out.

Like @eshkinkot mentions would I would like to see is that TRANSACTION READ ONLY not be applied to the session but instead to the particular transaction that needs it.

So instead of:

<client connects>
SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY;
BEGIN;
< Work here >;
COMMIT;

It would instead be

<client connects>
BEGIN TRANSACTION READ ONLY;
< Work here >;
COMMIT;

So everything that is currently stored on the session gets pushed down to the actual transaction that needs it so that connection can be re-used with a clean session state.

[dan-aksenov]

I was thinking same here. Already set up:
server_reset_query_always=1
No errors for about four hours. Still testing.
Glad to hear that other people came up with same fix.

[vlsi]

I'd love to try to make a PR but I am not even sure where to start.

@jamestenglish , I think the key point is to add .travis.yml configuration that reproduces the issue.
For instance:

1. Add a job (like https://github.com/pgjdbc/pgjdbc/blob/master/.travis.yml#L197-L203) with PGBOUNCER=TRANSACTION environment variable. It could make sense to just add the setting to an existing job
2. Add a line to .travis.yml that would install and configure pgbouncer (like https://github.com/pgjdbc/pgjdbc/blob/master/.travis.yml#L30 )
3. Run the tests through pgbouncer (e.g. adding something like -Dport=${pgbouncerport})
   Then we'll see how it breaks.

Does it make sense?

[jamestenglish]

@vlsi That is a very helpful starting point, I'll see what I can do about making an at least somewhat repeatable failure state.

[colinmorelli]

Just adding, since it wasn't mentioned here before, that the same should probably be done for transaction isolation level. It's also currently set for the session (but can be set as part of the begin transaction)

[mkurz]

Hi all,

I have a question related to this topic here and since you are experts in this field you probably can help me.
Like said above, when running pgbouncer in transaction pooling mode it expects there are no session level things set.
So something like this is bad:

<client connects>
SET SESSION my.var = 'foo';
-- here pgbouncer may switch the connection
BEGIN;
< Work here >;
SHOW my.var; --likely doesn't display "foo" since we may use a different connection
COMMIT;

my.var is likely set on a different connection than the transaction itself, so we leak information into the wrong connection.
However, would following be ok?

<client connects>
BEGIN;
SET SESSION my.var = 'foo';
< Work here >;
SHOW my.var; --always displays "foo" since we are on the same connection? sure?
RESET my.var;
COMMIT;

Given that the SET SESSION... happens inside the transaction it should be ok or am I wrong?
Meaning my.var would only be set for the connection that is used to run the transaction.
In case I rollback the transaction also the my.var would be rollbacked as well as far as I know.

I am asking this since I want to use session variables for row-level security policies and if that would work this way with pgbouncer in transaction pooling mode.

[mkurz]

Further:
Probably in my case it would be even better to use SET LOCAL, instead of SET SESSION:

<client connects>
BEGIN;
SET LOCAL my.var = 'foo';
< Work here >;
SHOW my.var; --always displays "foo" since we are on the same connection? sure?
--RESET my.var; -- should not be necessary since SET LOCAL only sets a var for a specific session
COMMIT;

[mkurz]

This post on the postgres mailing list clarified a lot of things for me and problably is also helpful for this issue here:

> AFAIK PgBouncer in transaction mode is not compatible with any
> SET statements.

It is compatible with any SET statements which don't leave a
persistent state beyond the end of the transaction.  That includes
any SET LOCAL as well as a few SET options which only affect the
current transaction, like transaction_isolation,
transaction_read_only, and transaction_deferrable (note that these
three have a default_* setting which persists beyond transaction
boundaries -- those defaults are *not* safe to use).

Also have a look a the example in the post.

[azraino]

Hi Guys,
Do we have any update on this topic ? any released version ?
Thank you very much

[davecramer]

From what I can tell #1252 committed a fix for this and it was released. See

pgjdbc/pgjdbc/src/main/java/org/postgresql/PGProperty.java

Line 425 in f06c15e

"Controls the behavior when a connection is set to be read only, one of 'ignore', 'transaction', or 'always' "

Does this not work or do what you require ?

[azraino]

Hi Dave,
Thank you very much for answer.
I guess this issue is now fixed from 42.2.10 right ?

How can I ignore the read-only transaction with the new feature implemented? I see the PGProperty class but don't know how to set them on my spring boot application.

Thank you again.

[davecramer]

@azraino I think DataSources can take any property. There are setters and getters for the properties in the datasource

[azraino]

@davecramer Can you please confirm that the documentation on https://jdbc.postgresql.org/documentation/head/connect.html is up to date ?
Because I can't see the new parameter for the driver readOnlyMode

[davecramer]

readOnly = boolean

Put the connection in read-only mode

[azraino]

But is should be readOnlyMode and not readOnly

`READ_ONLY_MODE(

"readOnlyMode",
"transaction",
"Controls the behavior when a connection is set to be read only, one of 'ignore', 'transaction', or 'always' "
+ "When 'ignore', setting readOnly has no effect. "
+ "When 'transaction' setting readOnly to 'true' will cause transactions to BEGIN READ ONLY if autocommit is 'false'. "
+ "When 'always' setting readOnly to 'true' will set the session to READ ONLY if autoCommit is 'true' "
+ "and the transaction to BEGIN READ ONLY if autocommit is 'false'.",
false,
new String[] {"ignore", "transaction", "always"}),
`

[davecramer]

ah, good point. You are correct. Care to send in a PR ?

[azraino]

@davecramer please find the PR for documentation fix.