fix: added GROUP_STARTUP_PARAMETERS boolean property to determine whether or not to group startup parameters in a transaction or not fixes Issue 2423 pgbouncer cannot deal with transactions in statement pooling mode

[davecramer]

fixes Issue #2423

[jesperpedersen]

I think it should be the other way around - e.g. true.

Executing statements assume auto-commit per statement unless an explicit transaction is started.

If "statement pooling" doesn't respect that - a transaction can be started on one connection and each of the "body" statement executed on another (or more) plus finally the commit on a third connection - then users need to explicit set this property to false.

Connection pools looks at the Z message in order to identify the transaction state of a connection - grouping statements inside a transaction from a single client is critical to both initialize and execute work-loads.

[davecramer]

Hmmm the idea is that your PR is a "new" behaviour and would have to be requested.

Good question for discussion though

[jesperpedersen]

I understand that.

However, "statement pooling" assumes auto-commit ALWAYS and SELECT ONLY as ANY transaction semantics is disabled. This is one of the reasons why @pgagroal won't support this mode, as people need to really understand the underlying constraints. Once you code to those constraints transaction pooling will give you the same performance and architecture.

[davecramer]

hmmm... interesting as ANY transaction semantics is disabled. Every statement in postgres is a transaction. So are you saying you can't do an insert in a single statement ?

[jesperpedersen]

From a user level.

Having BEGIN + COMMIT/ROLLBACK around your functionality is very different than having it without.

Sure , in JDBC you have to do

c.setAutoCommit(false);
...
c.commit();

and so on, but in practice a transaction manager will handle that for you transparently.

This PR is only to group the startup statements, not to change the overall driver behaviour - which would be dangerous.

[davecramer]

So you are referring to user level transactions, not "actual transactions"

[jesperpedersen]

If this PR is merged with false as the default you should probably bump the minor version to 42.4, as it is a behaviour change from 42.3.

[hannibal218bc]

Thank you @davecramer for the quick PR.
Is there something I could contribute to move forward with this?

(I'd be happy with any of the defaults. I agree that statement-level pooling is an advanced configuration and whoever employs that should be able to RTFM and set the appropriate parameter. On the other hand, I'd find it surprising if you need to RTFM and set an appropriate parameter to use pgbouncer with pgjdbc.)

Furthermore, I have still not understood what the original PR is actually supposed to deliver. In a transaction pool, the effects of SET ... will be reverted anyways at the end of the transaction, and I did not find any evidence of pgbouncer saving a client's SET ... commands to be replayed when re-connecting to the next backend on a subsequent connection. Yes, it will replay the startup parameters, but SET ... is no such parameter as far as I can tell? So I'm thinking the backend will run 'DISCARD ALL;' immediately after the new commit, so there won't be any effect at all?
YMMV by reset_query or other poolers ...

Thanks again for all of your ongoing efforts in this project!

[davecramer]

@hannibal218bc we've been a little distracted by the latest security advisory.

@jesperpedersen care to expound on the use case. My understanding is that it ensures that the startup parameters are all applied to the same connection ?

[hannibal218bc]

@davecramer may I again ask if there is anything we can help to get this issue resolved? The issue is still present in the latest 42.3.4 release. Thanks!

[vlsi]

I think the PR is good to go, except I would add a relevant notice to the changelog like «revert to … behaviour, do not group startup parameters in a transaction, …»

[davecramer]

Alternatively we bump to 42.4.0 ?

[vlsi]

Sure. Let's merge it, bump the version and proceed with the release.