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

fix client_encoding setting to support pgbouncer #823

Merged
merged 1 commit into from
Mar 27, 2015

Conversation

rocksfrow
Copy link
Contributor

The current release doesn't allow connecting to pgbouncer when specifying the charset due to the lack of support for the 'options' parameter. The fix is to use the client_encoding option directly instead of passing it through the options parameter.

This exact same bug was fixed in node.js's PG driver the exact same way.

brianc/node-postgres#356

I've confirmed this fix works with pgbouncer.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1183

We use Jira to track the state of pull requests and the versions they got
included in.

@rocksfrow
Copy link
Contributor Author

FYI -- the error you'll get when trying to connect to pgbouncer using the current release:

Uncaught exception 'PDOException' with message 
'SQLSTATE[08006] [7] ERROR:  Unsupported startup parameter: options' 
in /home/xxx/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:43

Again the fix is simply to pass client_encoding option directly.

rocksfrow referenced this pull request Mar 27, 2015
add charset option for pdo_pgsql driver
deeky666 added a commit that referenced this pull request Mar 27, 2015
fix client_encoding setting to support pgbouncer
@deeky666 deeky666 merged commit 8e65cdc into doctrine:master Mar 27, 2015
@deeky666
Copy link
Member

Thanks @rocksfrow !

@deeky666
Copy link
Member

backported to 2.5 in e25ccae

@rocksfrow
Copy link
Contributor Author

@deeky666 no prob -- thanks for the quick merge! Would 2.5.2 be the first release that'll include this fix?

@billschaller
Copy link
Member

@rocksfrow maybe a test for these options would be a good idea

@rocksfrow
Copy link
Contributor Author

@zeroedin-bill there isn't already a postgres connection test?

@riccardonar
Copy link

@rocksfrow I'm using postgres 9.3.4 on slackware..

@billschaller
Copy link
Member

@rocksfrow yes, but it clearly is not covering completely...

@rocksfrow
Copy link
Contributor Author

@zeroedin-bill I'll check out the test and see if it's not using the parameters properly.

@rocksfrow
Copy link
Contributor Author

@riccardonar I don't see how you're getting an invalid option connecting to 9.3. Can you please provide more details? Are you connecting to a connectiong manager/pooler of any sort (pg-pool,pgbouncer) or just connecting directly to tcp 5432 -- or are you connecting via socket maybe?

@rocksfrow
Copy link
Contributor Author

I want to do a manual test and actually confirm client_encoding doesn't work on 8.x -- it might just not be in the doc. I'm definitely connecting to 9.3 with client_encoding -- but via TCP.

@riccardonar
Copy link

@rocksfrow you are right.. my postgres client version is 9.0.4

@rocksfrow
Copy link
Contributor Author

@riccardonar -- lol, okay I was going to say!

Alright so I am wrapping up a fix right now that will set the client encoding via the sql standard SET NAMES instead of via PDO dsn.

This will ensure the encoding is set properly in ALL versions. I just confirmed it works on both.

@rocksfrow
Copy link
Contributor Author

@riccardonar I just pushed up changes to my fork. Would you be a saint and confirm it works well for you with your environment? I've confirmed it's properly setting the charset set via a manual SHOW client_encoding after connection.

@deeky666 Here are my changes: rocksfrow@b8131d1

I'll create a pull request.

@billschaller
Copy link
Member

@rocksfrow can you add a comment alongside that change hat explains which
versions don't support the dsn option?
On Apr 2, 2015 11:23 AM, "Kyle Renfrow" [email protected] wrote:

@riccardonar https://github.com/riccardonar I just pushed up changes to
my fork. Would you be a saint and confirm it works well for you with your
environment? I've confirmed it's properly setting the charset set via a
manual SHOW client_encoding after connection.

@deeky666 https://github.com/deeky666 Here are my changes: rocksfrow@
b8131d1
rocksfrow@b8131d1

I'll create a pull request.


Reply to this email directly or view it on GitHub
#823 (comment).

@rocksfrow
Copy link
Contributor Author

Here is my pull request:

#828

@rocksfrow
Copy link
Contributor Author

@zeroedin-bill please see my pull request -- I just pushed up another commit adding details RE: client_encoding support. Is that sufficient?

@riccardonar would you mind forking my repo to test these changes on your environment? https://github.com/rocksfrow/dbal

@rocksfrow
Copy link
Contributor Author

@riccardonar I'd greatly appreciate you replying to my NEW pull request (not this one) -- and confirm the fix provided there resolves the issue you are running into. That way they can expedite the merge upstream.

The pull request: #828

@riccardonar
Copy link

@rocksfrow your repo works fine for me! Thanks

@rocksfrow
Copy link
Contributor Author

@riccardonar great! I am glad I was able to resolve that for you quickly --
sorry about that.

@deeky666 let me know if there is anything else you need to happen in order
to get this pull request merged asap.

On Thu, Apr 2, 2015 at 11:55 AM, riccardonar [email protected]
wrote:

@rocksfrow https://github.com/rocksfrow your repo works fine for me!
Thanks


Reply to this email directly or view it on GitHub
#823 (comment).

@rocksfrow
Copy link
Contributor Author

@riccardonar -- would you mind commenting on the new pull request - #825
confirming that the fix works? That way whoever reviews that pull request
will hopefully get it merged in quickly so we don't have to hack our
versions :)

On Thu, Apr 2, 2015 at 11:57 AM, Kyle Renfrow [email protected] wrote:

@riccardonar great! I am glad I was able to resolve that for you quickly
-- sorry about that.

@deeky666 let me know if there is anything else you need to happen in
order to get this pull request merged asap.

On Thu, Apr 2, 2015 at 11:55 AM, riccardonar [email protected]
wrote:

@rocksfrow https://github.com/rocksfrow your repo works fine for me!
Thanks


Reply to this email directly or view it on GitHub
#823 (comment).

@rocksfrow
Copy link
Contributor Author

Thanks guys! Happy Coding.

On Thu, Apr 2, 2015 at 11:58 AM, Kyle Renfrow [email protected] wrote:

@riccardonar -- would you mind commenting on the new pull request - #825
confirming that the fix works? That way whoever reviews that pull request
will hopefully get it merged in quickly so we don't have to hack our
versions :)

On Thu, Apr 2, 2015 at 11:57 AM, Kyle Renfrow [email protected] wrote:

@riccardonar great! I am glad I was able to resolve that for you quickly
-- sorry about that.

@deeky666 let me know if there is anything else you need to happen in
order to get this pull request merged asap.

On Thu, Apr 2, 2015 at 11:55 AM, riccardonar [email protected]
wrote:

@rocksfrow https://github.com/rocksfrow your repo works fine for me!
Thanks


Reply to this email directly or view it on GitHub
#823 (comment).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants