node-postgres version 7 incompatibilities

[alitaheri]

Any version of sequelize is incompatible with version 7. The main reason is that pg7 returns multiple result objects instead of one with joined results from multiple statements. Therefore, any multiple statement query breaks the postgre's query runner.

I have tested sequelize with pg7 and I see a dozen failed tests. We should decide how we should handle multiple statements. I see these options:

1. Always return the last result as done in 4.4.0 breaks pg connection initialization #7986
2. Add a new query type ( MULTIPLE ) and return arrays of results when chosen, the last one otherwise

Before this is fixed, there is no choice but to use pg ^6.

Dialect: postgres
Database version: any
Sequelize version: any

[felixfbecker]

You didn't try the tests before #7888 was merged?

[alitaheri]

@felixfbecker The issue was timing 😅 I only submitted that PR to remove the deprecation warning. I wasn't expecting breaking changes other than the ones listed in their migration guide. I followed the guide thoroughly. After I migrated and submitted the PR version 7 was released. I didn't expect other breaking changes. It's partly my fault. But since I worked on that part of the code I can handle the fix. We should reach a consensus on my question first though.

[felixfbecker]

I think we should maintain whatever behaviour we had with v6

[alitaheri]

@felixfbecker take a look at brianc/node-postgres#508

It was corrupting the results before. They've fixed it so instead of joining all rows from all results into a single row ( which is wrong on so many levels ) it now returns a multi dimensional array. I can add a new query type: MULTIPLE.

The outcome would be this:

1. Every query run by the library will only care for the last result object.
2. All raw queries will only return the result from the last statement when the type is specified and is not MULTIPLE
3. When the type is not specified the metadata object will be returned as provided by pg
4. When MULTIPLE is selected it is guaranteed to return a 2 dimensional array ( even if the query doesn't have multiple select statements )

Do you think that makes sense?

[felixfbecker]

Yes. I am just worried about how to type a different return type depending on setting a nested option. Sounds a bit hard for IDEs to figure out. Then again, pg's way is even harder.

[alitaheri]

@felixfbecker Hmmmm very good question. Maybe just add a new method? something like multiple() that always resolves into a multidimensional array?

query and everything else will still only care for the last one. ( Or maybe the last one where command === 'SELECT'? uhhh )

[konrad-garus]

Since there are such serious issues with pg7, might I suggest you guys implement some means to prevent use of sequelize with pg7? I would even prefer a violent crash on app startup with a message saying this is not supported, over finding an error deep in logs and spending an hour debugging.

[alitaheri]

@konrad-garus I think that will do for now 👍 👍 Care to submit a PR? 😅

[konrad-garus]

What is a good way to detect this, though? The only one I found so far is looking for a function removed in pg7, like:

const pg7 = !pg.connect;
if (pg7) {
  throw new Error('pg in version 7 or newer is not supported yet, please downgrade to 6');
}

[felixfbecker]

Just read the package.json?

[konrad-garus]

I'm not aware of a Node API to do it, and I find scanning the file system pretty dirty and fragile. Does it even guarantee to give you correct results 100% of the time? But hey, hopefully it's a temporary hack.

What do you think about this:

let pg7;
try {
  const versionString = require(`${process.cwd()}/node_modules/pg/package.json`).version;
  pg7 = parseInt(versionString, 10) >= 7;
} catch (error) {
  // PG version detection didn't work, let's just give a warning and proceed...
  log.warn('Unable to determine pg version. Please note that pg in version 7 or newer is not supported yet');
}
if (pg7) {
  throw new Error('pg in version 7 or newer is not supported yet, please downgrade to 6');
}

[felixfbecker]

You can't assume the package is in ${cwd}/node_modules. Use require.resolve

parseInt works, but using semver would be nicer.

[konrad-garus]

cwd - that was my problem. I wasn't aware of require.resolve.

parseInt - I used that because I default to as few dependencies in libraries as possible. But apparently half the world (including sequelize) depends on semver anyway, so...

How about:

const pgPath = require.resolve('pg');
const pgVersionString = require(pgPath.substr(0, pgPath.lastIndexOf('node_modules')) + 'node_modules/pg/package.json').version;
if (semver.major(pgVersionString) >= 7) {
  blowUp();
}

[holmberd]

Please pin a note at the top of the readme stating that Sequelize is currently incompatible with pg - v7.0, you shouldn't have to read trough all the issues to find this information.

[corysimmons]

lol, wow. 😓

[doubliez]

I'm encountering this issue, running Sequelize with pg7. Multi-statement methods like findOrCreate or upsert are failing because of that, which is quite annoying.

I can of course downgrade to pg6, but would be nice to have this working with pg7.

Any progress on this issue would be greatly appreciated :)

[thebeachdev]

I'm currently using pg 6.4.1. I was wondering what the status of this issue is. It seems I can't use pg7 with the newest version of of sequelize.

[Connorelsea]

I am still encountering issues running pg7, specifically encountering issue #8078 which was closed :(

[gabegorelick]

Is there anything left to do now that pg7 is officially supported?

[sushantdhiman]

@gabegorelick just refactor work, not blocking

[papb]

What exactly needs to be done here? In other words, what kind of refactor are you looking for, @sushantdhiman ?

[sushantdhiman]

@papb I wanted to support real multiple statements where you can pass QueryType.MULTIPLE and possibly an index to get result of nth query or last one as default.

But currently we are using some hacks to support upsert and query generator code is not ready for this refactor, not sure if all dialect support what I planned.

As we already support pg@7 by merging results from all queries in a multiple statement query, this issue is no longer valid.