rehashed charset implementation to support old versions of postgresql #828

rocksfrow · 2015-04-02T15:29:31Z

My previous client_encoding fix turned out not to work on older postgres versions -- BUT, the options param isn't supported by pgbouncer at all. SO, because there isn't consistent behavior with setting the character set via DSN - this change utilizes SET NAMES. I used SET NAMES since that is SQL standard and supported by PG.

I confirmed SET NAMES is supported all the way back to the older doc PG has, 7.1: http://www.postgresql.org/docs/7.1/static/multibyte.html

doctrinebot · 2015-04-02T15:29:43Z

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-1189

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

…sted

billschaller · 2015-04-02T16:02:39Z

Looks like travis is happy, @riccardonar is happy, my tests run, all seems OK. I think we need to add a functional test for pgsql connections though - Esp. given that we have people connecting via bouncers and whatnot.

rehashed charset option implementation to support old and new versions of postgresql, and pgbouncer

riccardonar · 2015-04-02T16:20:55Z

Yes, for me it works!

rocksfrow · 2015-04-02T16:23:15Z

@zeroedin-bill when it comes to postgres, it's extremely common to use poolers. PG has a lot more overhead when creating connections than other DBs so they're almost required for any high-volume environment.

RE: travis checks

I noticed you only have travis checks setup for 9.1+ anyway. The previous fix would have passed travis checks too :)

RE: functional test

Can you elaborate on the critiera and/or point me to some references? I wouldn't mind contributing a test. I just wrote one basically when I made these recent changes -- in that, it opens a connection used the configuration and then uses SHOW client_encoding and determines the previous SET NAMES usage works. Is that basically what you mean?

billschaller · 2015-04-02T16:27:35Z

@rocksfrow I'm aware - I use postgres :)

I'm writing a quick functional test for the charset stuff.

deeky666 · 2015-04-02T16:38:39Z

lib/Doctrine/DBAL/Driver/PDOPgSql/Driver.php

+             * - passing client_encoding via the 'options' param breaks pgbouncer support
+             */
+            if (isset($params['charset'])) {
+              $pdo->query('SET NAMES \''.$params['charset'].'\'');


I am not sure whether this is an acceptable solution. Having one extra query on each connect is affecting performance I suppose... I remember we had SET NAMES back in old Doctrine 1 days and I believe it was avoided in 2.x for a good reason.
@Ocramius @beberlei thoughts?

@deeky666 I definitely agree this isn't the cleanest fix -- BUT, with that said, I also disagree with it going back how it was -- because you're then making the class unusable with pgbouncer (or other poolers) when it worked fine before.

I would think the only other option would be to simply rely on AUTO, and not set the client_encoding at all.

I totally get your point. It's just a question of portability vs performance. Gosh why can't PostgreSQL be consistent on this -.-

I know right! So frustrating!

Well FYI -- this wasn't even being set before at all (like you said I guess it was removed). The reason this came up is because somebody added it -- so when I upgraded it totally broke on PG.

So maybe the BEST solution would be to simply totally revert these changes AND the original change:

rocksfrow@36fa410

Basically, that means DBAL doesn't set the charset at all and leaves it up to the server "auto" mode.

TBH, I was a little surprised it was being set explicity anyway as I never have in the past (instead I use AUTO (default) and it uses the systems LOCALE).

Thoughts?

Haha -- I just noticed you're actually the one who made that change @deeky666 heh

My phpinfo
and server postgres 9.3.4 is on another server

With
$dsn .= 'client_encoding=' . $params['charset'] . ' ';
i confirm that don't work.

@rocksfrow no offense! I just realized that this discussion is getting rather heavy for a rather trivial issue. I was just asking for additional opinions about the SET NAMES fix. I can live with that and obviously there is no better solution yet. So let's just keep it. Thanks for all your investigation on this!

@riccardonar yeah so check it out -- I have a LOWER version (8.4.20) -- and client_encoding works fine for me.

This just confirms what I was trying to tell you and @deeky666 -- this is not reliable data to make a decision off of.

@deeky666 confirmed he was just guessing anyway -- so we're leaving the current implementation (my fork). You can use my fork meanwhile until they release the next minor if you want.

Backported to 2.5 branch.

@deeky666 the convo got crazy because people jumping in giving inaccurate data haha.

I agree this is the best solution for now -- maybe eventually we'll have something we can rely on (like PGSQL_LIBPQ_VERSION being accurate).

I think the issue is that PGSQL_LIBPQ_VERSION does not update -- even after you upgrade (I read that somewhere).

Anyway -- thank you guys for an awesome class! I am glad to contribute.

We should be good to move forward now at least.

Have a good day.

rocksfrow · 2015-04-02T16:43:07Z

@deeky666 what we COULD do is use my previous fix for PG >= 9.1, and for backwards compatibility -- use SET NAMES. These versions that don't support client_encoding aren't supported by postgres anyway. I noticed you guys have "platforms" to add support for new stuff in 9.1 and 9.2 -- is that something we could take advantage of?

Curious to hear your thoughts.

deeky666 · 2015-04-02T16:46:07Z

@rocksfrow yep we could do that if we knew the version of the underlying server before actually connecting. This is totally driver specific, nothing that needs to be in the platform...

rocksfrow · 2015-04-02T20:29:01Z

NOTE: if you don't want the overhead of SET NAMES and are OK with AUTO (your LOCALE is the same as your DB) -- just leave charset out of your config.

rocksfrow · 2015-04-02T20:41:30Z

@zeroedin-bill I just saw you're from Maryland. Baltimore here!

rocksfrow · 2015-04-02T20:41:44Z

MD represent! haha

billschaller · 2015-04-02T20:55:08Z

@rocksfrow how bout dem O's

billschaller · 2015-04-03T15:03:47Z

@deeky666 should we backport this?

rocksfrow · 2015-04-03T15:06:03Z

The previous fix this is enhancing was backported so this one should be
otherwise that previous fix is going to remain flawed I'd think.

Note, this email was sent from my cell phone.
On Apr 3, 2015 11:03 AM, "Bill Schaller" notifications@github.com wrote:

@deeky666 https://github.com/deeky666 should we backport this?

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

rocksfrow · 2015-04-03T15:07:10Z

(The previous fix of mine that deeky already backported wasn't working w
older versions of PG)

Note, this email was sent from my cell phone.
On Apr 3, 2015 11:05 AM, "Kyle Renfrow" frow05@gmail.com wrote:

The previous fix this is enhancing was backported so this one should be
otherwise that previous fix is going to remain flawed I'd think.

Note, this email was sent from my cell phone.
On Apr 3, 2015 11:03 AM, "Bill Schaller" notifications@github.com wrote:

@deeky666 https://github.com/deeky666 should we backport this?

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

rocksfrow · 2015-04-03T15:08:27Z

Just out of curiosity - what are you guys doing when you backport? When you
do that does it mean you're making it available in previous 2.5.x releases?
I'm just am curious for my own dev processes.

Note, this email was sent from my cell phone.
On Apr 3, 2015 11:07 AM, "Kyle Renfrow" frow05@gmail.com wrote:

(The previous fix of mine that deeky already backported wasn't working w
older versions of PG)

Note, this email was sent from my cell phone.
On Apr 3, 2015 11:05 AM, "Kyle Renfrow" frow05@gmail.com wrote:

The previous fix this is enhancing was backported so this one should be
otherwise that previous fix is going to remain flawed I'd think.

Note, this email was sent from my cell phone.
On Apr 3, 2015 11:03 AM, "Bill Schaller" notifications@github.com
wrote:

@deeky666 https://github.com/deeky666 should we backport this?

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

billschaller · 2015-04-03T15:15:22Z

@rocksfrow deeky did backport it to 2.5 - i missed that. Take a look at the 2.5 branch.

rocksfrow · 2015-04-03T15:19:02Z

Yeah OK -- I thought he already did. So backporting it just means it's
going to be available in the NEXT 2.5.x release.

On Fri, Apr 3, 2015 at 11:15 AM, Bill Schaller notifications@github.com
wrote:

@rocksfrow https://github.com/rocksfrow deeky did backport it to 2.5 -
i missed that. Take a look at the 2.5 branch.

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

rocksfrow · 2015-04-03T15:20:00Z

Is that true? IE -- I can expect to see this fix in 2.5.2 I suppose?

On Fri, Apr 3, 2015 at 11:19 AM, Kyle Renfrow frow05@gmail.com wrote:

Yeah OK -- I thought he already did. So backporting it just means it's
going to be available in the NEXT 2.5.x release.

On Fri, Apr 3, 2015 at 11:15 AM, Bill Schaller notifications@github.com
wrote:

@rocksfrow https://github.com/rocksfrow deeky did backport it to 2.5 -
i missed that. Take a look at the 2.5 branch.

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

rocksfrow · 2015-04-03T15:23:00Z

I just recently started using DBAL because it wasn't working with pgbouncer
so I just am using my own branch for now -- but I want to make sure I am
using the official branch again once it hits the next release.

Thanks for any clarification you may give. I am not sure about DBAL's
release schedule/practice.

On Fri, Apr 3, 2015 at 11:19 AM, Kyle Renfrow frow05@gmail.com wrote:

Is that true? IE -- I can expect to see this fix in 2.5.2 I suppose?

On Fri, Apr 3, 2015 at 11:19 AM, Kyle Renfrow frow05@gmail.com wrote:

Yeah OK -- I thought he already did. So backporting it just means it's
going to be available in the NEXT 2.5.x release.

On Fri, Apr 3, 2015 at 11:15 AM, Bill Schaller notifications@github.com
wrote:

@rocksfrow https://github.com/rocksfrow deeky did backport it to 2.5

i missed that. Take a look at the 2.5 branch.

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

billschaller · 2015-04-03T15:30:11Z

@rocksfrow if you're targeting the 2.5 branch in your composer.json, you will get the update - if you're targeting a specific point version, you'll have to wait for 2.5.2 and then target that point version...

rocksfrow · 2015-04-03T15:37:24Z

@zeroedin-bill I am using packagist, so I suppose the "2.5.x-dev" is what you're talking about. These are production servers so I'm definitely not pointing a dev branch I am pointing to 2.5.1. I will keep an eye out for the 2.5.2 release -- and meanwhile I'll just point to my own branch.

Thanks!

rocksfrow · 2015-04-03T15:38:48Z

@zeroedin-bill does the dbal have any standard release schedule -- or is it pretty random? Are there any processes that could lead to a predictable release date or should I not bother and just check in every now and then?

rixbeck · 2015-04-22T15:56:32Z

Hey @rocksfrow,

I'm also hitting this problem with pgbouncer and if you don't want to patch DBAL or use your branch - that is not always possible - there's a workaround here.
In pgbouncer.ini set

ignore_startup_parameters=application_name,options

Then after connection made you may call SET NAMES query directly or you can build up a postConnect event listener that will be fired by DBAL.

...in case also anyone else affected in thread here #823...

rocksfrow · 2015-04-22T16:34:42Z

@rixbeck that's a cool trick but I would imagine DBAL managers would agree -- we don't want to require config changes to your database stack to work with DBAL -- it should work out of the box.

I am simply using the 2.5-x branch which includes my merged in fix -- I am not sure if you noticed but we ended up merging in a second/better fix which is doing exactly that -- setting the charset via SET NAMES after connection.

I have been running 2.5-x branch in production within any issues -- just waiting on them to cut 2.5.2 so I can start using that (to avoid any bugs introduced) in the dev branch.

rixbeck · 2015-04-22T16:50:11Z

Yeah, agree. Although it is really just a workaround until fix comes out with stable.
We are working on the Bolt CMS project here and DBAL is just a depending component under with stable required branches so currently I'm glad with this 'cause I can use pgbouncer in my projects without patching. :)

… out - see doctrine/dbal#828

rocksfrow mentioned this pull request Apr 2, 2015

fix client_encoding setting to support pgbouncer #823

Merged

rocksfrow added 3 commits April 2, 2015 15:31

rehashed charset implementation to support old versions of postgresql

5a4c0f2

added additional comments explaining client_encoding support as reque…

828325d

…sted

fixed typo in my comment

cef58bd

billschaller pushed a commit that referenced this pull request Apr 2, 2015

Merge pull request #828 from rocksfrow/master

9aadacd

rehashed charset option implementation to support old and new versions of postgresql, and pgbouncer

billschaller merged commit 9aadacd into doctrine:master Apr 2, 2015

deeky666 reviewed Apr 2, 2015
View reviewed changes

billschaller mentioned this pull request Apr 2, 2015

Add PostgreSQL connection test with the new charset parameter #829

Merged

rixbeck mentioned this pull request Apr 22, 2015

PGSQL encoding settings in post connect event until doctrine/2.5.2 is out bolt/bolt#3429

Merged

bobdenotter pushed a commit to bolt/bolt that referenced this pull request Apr 23, 2015

PGSQL encoding settings in post connect event until doctrine/2.5.2 is…

2eea769

… out - see doctrine/dbal#828

This was referenced Dec 6, 2015

DBAL-1189: [GH-828] rehashed charset implementation to support old versions of postgresql #1132

Closed

DBAL-1190 #829 Add PostgreSQL connection test with the new charset parameter #1134

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2022

rehashed charset implementation to support old versions of postgresql #828

rehashed charset implementation to support old versions of postgresql #828

Conversation

rocksfrow commented Apr 2, 2015

doctrinebot commented Apr 2, 2015

billschaller commented Apr 2, 2015

riccardonar commented Apr 2, 2015

rocksfrow commented Apr 2, 2015

billschaller commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocksfrow commented Apr 2, 2015

deeky666 commented Apr 2, 2015

rocksfrow commented Apr 2, 2015

rocksfrow commented Apr 2, 2015

rocksfrow commented Apr 2, 2015

billschaller commented Apr 2, 2015

billschaller commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

billschaller commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

billschaller commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

rocksfrow commented Apr 3, 2015

rixbeck commented Apr 22, 2015

rocksfrow commented Apr 22, 2015

rixbeck commented Apr 22, 2015