feat(dependency): Support pg version 7 #7888

alitaheri · 2017-07-05T09:04:29Z

Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Upgrade pg API usage to be forward compatible with version 7. Also gets rid of the deprecation warning.

Closes #7818

codecov · 2017-07-05T09:11:06Z

Codecov Report

Merging #7888 into master will increase coverage by 0.01%.
The diff coverage is 97.43%.

alitaheri · 2017-07-05T11:15:25Z

@felixfbecker No idea why the tests fail :/

felixfbecker · 2017-07-05T11:17:07Z

docs/usage.md

@@ -201,7 +201,7 @@ const sequelize = new Sequelize('sqlite:relativePath/dbname.db')

 ### PostgreSQL

-The library for PostgreSQL is`pg@~3.6.0` You'll just need to define the dialect:
+The library for PostgreSQL is`pg@~6.1.0` You'll just need to define the dialect:


7 doesnt match ~6.1.0. The whole range of supported versions should be documented.

Well it's not released yet. Might confuse some people O.o

I don't see anything wrong with documenting that both 6 and 7 are supported. Otherwise someone will have to update this documentation once 7 is out

Agreed. does pg@>=6.1.0 make more sense or pg@^6.1.0 || ^7.0.0

I would say the latter, there is a high chance we won't support 8.0 when it comes out

alitaheri · 2017-07-06T09:55:08Z

@felixfbecker I skipped ci, have a final look and lemme know if everything's alright.

felixfbecker · 2017-07-06T09:59:26Z

@alitaheri that doesn't work for PRs, you need the status check for the last commit ;)

alitaheri · 2017-07-06T10:01:37Z

Ops sorry 😅 😅

yocontra · 2017-07-06T15:39:28Z

Any idea what release this might make it out on? 4.x or backported?

felixfbecker · 2017-07-06T15:41:08Z

@contra as soon as it's merged into master, it will be released as 4.x

yocontra · 2017-07-06T15:48:28Z

@felixfbecker Cool, thanks for the speedy response.

felixfbecker · 2017-07-11T08:57:36Z

Sorry, there are merge conflicts. You probably just need to rebase your version and run eslint --fix over it, there were some updates to eslint

alitaheri · 2017-07-11T11:14:56Z

@felixfbecker It's done. please rerun the tests. thanks 😁

alitaheri · 2017-07-11T18:06:18Z

@felixfbecker Any chance we can merge this as is? 😁 😅

felixfbecker · 2017-07-11T18:07:12Z

Don't ask me, I approved - need more than 1 review

janmeier

Can we somehow check that the right version of pg is installed? Releasing this as a minor might break for some people on old versions of pg that don't support promises (it's an obvious break, but still a break)

alitaheri · 2017-07-12T07:29:13Z

@janmeier The version compatible ( v6.1.0 ) is almost a year old now. If people are upgrading sequelize to v4 I'm sure they've long upgraded pg to v6.1.0 😅

janmeier · 2017-07-12T07:36:17Z

Roger that :)

felixfbecker · 2017-07-13T16:31:52Z

lib/dialects/postgres/connection-manager.js


-            type.types.postgres.oids.push(row.oid);
-            type.types.postgres.array_oids.push(row.typarray);
+      return connection.query(query).then(result => {


I think this is nested .then() handler and could be moved one level higher

@felixfbecker Could you elaborate on that? Do you mean I should return query and continue the upper promise chain?

yeah.
bad:

return foo().then(() => { return bar().then(() => { return baz() }) })

good:

return foo() .then(() => { return bar() }) .then(() => { return baz() })

@felixfbecker I can't do what you asked. Because it's not a nested then it's tap and then. If I chain the promises then the client won't be returned. Since I hate non-standard promise APIs I'll remove the chaining code and solve it differently.

I hate them too, I don't even know what tap does 😄

felixfbecker · 2017-07-13T16:33:13Z

Hmm, but it's still a breaking change if we really did still support 3.x with Sequelize 4.0... Is there a way to make this change backwards-compatible, e.g. by not relying on the promise support?

alitaheri · 2017-07-15T04:31:09Z

@felixfbecker node-postgres also supports node-style callbacks. I can use promisify to make sure it works with older versions too. Would that be enough?

felixfbecker · 2017-07-15T08:07:49Z

I usually just use this oneliner:

new Promise((resolve, reject) => conn.query(sql, (err, rows) => err ? reject(err) : resolve(rows)))

alitaheri · 2017-07-15T11:06:20Z

@felixfbecker @janmeier Have a look.

felixfbecker

This is much better if this prevents the breaking change! Thanks!

felixfbecker · 2017-07-15T11:10:19Z

lib/dialects/postgres/connection-manager.js

-            } else if (row.typname === 'geography') {
-              type = dataTypes.postgres.GEOGRAPHY;
-            }
+      return new Promise((resolve, reject) => connection.query(query, (error, result) => error ? reject(error) : resolve([connection, result])));


hmm, resolving with a tuple to maintain the connection is a bit weird. I didn't see that connection is still needed in scope, I would say in that case the nested promise is okay

felixfbecker · 2017-07-15T11:10:47Z

docs/usage.md

@@ -201,7 +201,7 @@ const sequelize = new Sequelize('sqlite:relativePath/dbname.db')

 ### PostgreSQL

-The library for PostgreSQL is`pg@~3.6.0` You'll just need to define the dialect:
+The library for PostgreSQL is`pg@^6.1.0 || ^7.0.0` You'll just need to define the dialect:


Can we lower the constraint if we don't use the promise support?

Would 5.0.0 be ok?

I am not sure how outdated this doc is - maybe you could find out the minimal version that passes the tests? Does pg@3.6.0 even work?

@felixfbecker Is that really necessary?

alitaheri · 2017-07-15T11:32:29Z

@felixfbecker Done what you've asked.

brianc · 2017-07-18T15:10:41Z

Hey y'all - sorry I just saw this! The deprecation of query.on was to clean up some confusion around that API I've had users experience for a while and allow me to do fewer allocations and gymnastics internally in future versions. If it's causing an upgrade headache or you need to support older versions you can still use this API:

https://node-postgres.com/guides/upgrading#client-query-submittable-

+1 for using callbacks or promises though. The callback style should work for every old versions of node-postgres too.

If y'all ever run into questions or weird bugs you think are related to node-postgres itself please don't hesitate to open an issue or ping me on twitter. Thanks for all you do here!! ❤️ 🤗

alitaheri · 2017-07-20T07:41:22Z

@felixfbecker Any reason you haven't merged this yet? We need this change for production 😅 😅

alitaheri force-pushed the upgrade-pg branch from a7db356 to 33b1f30 Compare July 5, 2017 10:00

felixfbecker suggested changes Jul 5, 2017

View reviewed changes

felixfbecker added the dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). label Jul 5, 2017

felixfbecker approved these changes Jul 6, 2017

View reviewed changes

felixfbecker requested review from janmeier, sushantdhiman and eseliger July 6, 2017 10:00

felixfbecker added the pr:final review label Jul 6, 2017

alitaheri force-pushed the upgrade-pg branch 2 times, most recently from 28773ab to c4b0cbf Compare July 11, 2017 10:47

xcambar mentioned this pull request Jul 11, 2017

Support for pg 7.0 #7818

Closed

janmeier approved these changes Jul 12, 2017

View reviewed changes

janmeier reviewed Jul 12, 2017

View reviewed changes

felixfbecker reviewed Jul 13, 2017

View reviewed changes

feat(dependency): Support pg version 7

efa1543

alitaheri force-pushed the upgrade-pg branch from 8c56089 to efa1543 Compare July 15, 2017 11:04

felixfbecker reviewed Jul 15, 2017

View reviewed changes

feat(pg): Address review change requests

c8d915d

felixfbecker approved these changes Jul 15, 2017

View reviewed changes

eseliger approved these changes Jul 15, 2017

View reviewed changes

This was referenced Jul 18, 2017

Update pg to the latest version 🚀 #7972

Closed

Support for Postgres broken since pg 7.0.0 #7973

Closed

thiloo mentioned this pull request Jul 19, 2017

7.0.0 Breaks sequelize support brianc/node-postgres#1372

Closed

felixfbecker merged commit 4383922 into sequelize:master Jul 20, 2017

alitaheri deleted the upgrade-pg branch July 20, 2017 09:08

felixfbecker mentioned this pull request Jul 22, 2017

node-postgres version 7 incompatibilities #7998

Closed

renan-ti mentioned this pull request Jul 30, 2017

Exception on changeColumn with postgresql (or: query with multiple statements) #8034

Closed

daffl mentioned this pull request Aug 14, 2017

Use latest pg driver again feathersjs-ecosystem/generator-feathers#261

Merged

daffl mentioned this pull request Aug 31, 2017

Update pg to the latest version 🚀 feathersjs-ecosystem/feathers-sequelize#157

Closed

ammoradi mentioned this pull request Jan 27, 2018

Sequelize Incompatibility with pg@7.x avarona/generator-fullstack-webapp#1

Closed

jasperlin451 pushed a commit to convoyinc/sequelize that referenced this pull request Mar 26, 2020

feat(dependency): Support pg version 7 (sequelize#7888)

8b10afb

ghusse mentioned this pull request Dec 9, 2020

describeTable returns an invalid defaultValue on Postgresql for timezone('utc'::text, now()) #12749

Closed

7 tasks

feat(dependency): Support pg version 7 #7888

feat(dependency): Support pg version 7 #7888

Conversation

alitaheri commented Jul 5, 2017 • edited

Pull Request check-list

Description of change

codecov bot commented Jul 5, 2017 • edited

Codecov Report

alitaheri commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri Jul 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alitaheri commented Jul 6, 2017

felixfbecker commented Jul 6, 2017

alitaheri commented Jul 6, 2017

yocontra commented Jul 6, 2017

felixfbecker commented Jul 6, 2017

yocontra commented Jul 6, 2017

felixfbecker commented Jul 11, 2017

alitaheri commented Jul 11, 2017

alitaheri commented Jul 11, 2017

felixfbecker commented Jul 11, 2017

janmeier left a comment

Choose a reason for hiding this comment

alitaheri commented Jul 12, 2017

janmeier commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfbecker commented Jul 13, 2017

alitaheri commented Jul 15, 2017

felixfbecker commented Jul 15, 2017 • edited

alitaheri commented Jul 15, 2017

felixfbecker left a 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

alitaheri commented Jul 15, 2017

brianc commented Jul 18, 2017

alitaheri commented Jul 20, 2017

alitaheri commented Jul 5, 2017 •

edited

codecov bot commented Jul 5, 2017 •

edited

alitaheri Jul 5, 2017 •

edited

felixfbecker Jul 15, 2017 •

edited

felixfbecker commented Jul 15, 2017 •

edited