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 pg_bouncer when using libpq (#270) #356

Merged
merged 1 commit into from
May 23, 2013

Conversation

Hebo
Copy link
Contributor

@Hebo Hebo commented May 21, 2013

Fix for #270

The same change needs to be made when using the native driver as with the JS driver.

@strk
Copy link
Contributor

strk commented May 23, 2013

This one seems good to me, please merge.

@brianc
Copy link
Owner

brianc commented May 23, 2013

👍

brianc added a commit that referenced this pull request May 23, 2013
Fix client_encoding setting to support pg_bouncer when using libpq (#270)
@brianc brianc merged commit b7f81f8 into brianc:master May 23, 2013
@brianc
Copy link
Owner

brianc commented May 23, 2013

@Hebo thanks so much.

@iToto
Copy link

iToto commented Jun 3, 2013

Hey guys, just as a FYI, this change broke my app that's currently hosted on Heroku. Funny thing is the issue only arose when I deployed to Heroku. Locally it works fine...

Here's a dump of the log:

2013-06-03T03:22:54.836472+00:00 app[web.1]: Error: invalid connection option "client_encoding"
2013-06-03T03:22:54.836472+00:00 app[web.1]:
2013-06-03T03:22:54.836472+00:00 app[web.1]:     at Connection.<anonymous> (/app/node_modules/pg/lib/native/index.js:188:17)
2013-06-03T03:22:54.836472+00:00 app[web.1]:     at Connection.EventEmitter.emit (events.js:95:17)
2013-06-03T03:22:54.836472+00:00 app[web.1]:     at /app/node_modules/pg/lib/native/index.js:49:19
2013-06-03T03:22:54.836472+00:00 app[web.1]:     at /app/node_modules/pg/lib/connection-parameters.js:68:12
2013-06-03T03:22:54.836472+00:00 app[web.1]:     at asyncCallback (dns.js:68:16)
2013-06-03T03:22:54.836472+00:00 app[web.1]:     at Object.onanswer [as oncomplete] (dns.js:121:9)
2013-06-03T03:22:56.386049+00:00 heroku[web.1]: Process exited with status 8
2013-06-03T03:22:56.405519+00:00 heroku[web.1]: State changed from starting to crashed

@strk
Copy link
Contributor

strk commented Jun 3, 2013

I guess a quick fix would be to make encoding setting optional,
but recommend that installer should make sure it is utf8 somehow.

@brianc
Copy link
Owner

brianc commented Jun 3, 2013

Yeah I was thinking of only passing it into postgres if you hard set it explicitly on your connection parameters. Funny how small changes like this break the hell out of so many things. So it goes.

@brianc
Copy link
Owner

brianc commented Jun 3, 2013

@iToto possible to open an issue with steps to reproduce?

@Hebo
Copy link
Contributor Author

Hebo commented Jun 3, 2013

Does the javascript driver have this same issue? The change should bring the native connection options up to parity with the JS ones.

I've seen this error when using libpq v8.4, as the client_encoding option seems to have been added around 9.1.

@Hebo Hebo deleted the pgbouncer_encoding_libpq_fix branch June 3, 2013 18:40
@badave
Copy link
Contributor

badave commented Jun 6, 2013

This was crashing our app on heroku as well. Downversioning to 1.0.x has temporarily fixed the issue on Heroku, but some of our devs are having an issue with the 1.0.x version locally.

@brianc
Copy link
Owner

brianc commented Jun 7, 2013

@badave your app is crashing in heroku now? Which version of node-postgres are you using? Using native driver or javascript?

@badave
Copy link
Contributor

badave commented Jun 7, 2013

Hey Brian,  the minor patch I submitted yesterday fixed this problem.  Making client encoding optional allowed us to take out specfying it to postgres altogether, which is what had broken versions 1.1-1.2 for us.  Currently our heroku boxes are happy with the latest version.

Sent from Mailbox for iPhone

On Fri, Jun 7, 2013 at 9:48 AM, Brian C notifications@github.com wrote:

@badave your app is crashing in heroku now? Which version of node-postgres are you using? Using native driver or javascript?

Reply to this email directly or view it on GitHub:
#356 (comment)

@brianc
Copy link
Owner

brianc commented Jun 7, 2013

okay sweet.

@iToto
Copy link

iToto commented Jul 2, 2013

sorry @brianc, didn't see this till today. However, I can confirm that my app is stable on heroku on version 2.0.0.

Keep up the awesome work! 👍

@brianc
Copy link
Owner

brianc commented Jul 2, 2013

no problemo! Keep in mind the type parsing changes in v2.0. I'll put
together a backwards compatiblity plugin module on npm soon if you're not
interested in bigint and numeric/decimal coming out as text. This
actually sucks kind of because now COUNT() results are strings (the
function return type is bigint) even though you will likely never, ever,
ever have more than a few billion rows in your table.

On Mon, Jul 1, 2013 at 9:13 PM, iToto notifications@github.com wrote:

sorry @brianc https://github.com/brianc, didn't see this till today.
However, I can confirm that my app is stable on heroku on version 2.0.0.

Keep up the awesome work! [image: 👍]


Reply to this email directly or view it on GitHubhttps://github.com//pull/356#issuecomment-20322612
.

@iToto
Copy link

iToto commented Jul 2, 2013

I don't think I'll be effected by that change. Thanks though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants