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 pg v7 regressions #8029

Closed
wants to merge 3 commits into from

Conversation

alitaheri
Copy link
Contributor

This hacks around #7998.

I wrote a function which squashes the results returned by pg. except for when raw query doesn't have a type. In that case it returns whatever returned by pg (I don't think we should change this behavior).

This is a temporary fix until we come up with a solution that satisfies every use-case. Might require architectural changes. Since other dialects support multiple statements too.

Until then this fixes #7998

@alitaheri
Copy link
Contributor Author

@felixfbecker The fields will come from the first result that contains them. The rows are the concatenation of all rows from all results. The rowCount is the accumulation of all non NaN rowCounts. And everything else command, parsers, etc. are from the last one. Anything out of place?

Also, when the query is raw with no type, the metadata might be an array ( possible breaking change ). But I'm not sure what others would expect of this. We explicitly notify the users that the results and metadata are dialect specific. so This is expected I guess. Right?

@oozimok
Copy link
Contributor

oozimok commented Jul 29, 2017

Faced with this problem, tried to understand and find a solution. Later came across this merge request.

sequelize/lib/dialects/postgres/query.js
screenshot

Array in queryResult:
screen shot 2017-07-29 at 18 59 05

My version of fixes:

.then(queryResults => {
  debug(`executed(${this.client.uuid || 'default'}) : ${this.sql}`);

  if (benchmark) {
    this.sequelize.log('Executed (' + (this.client.uuid || 'default') + '): ' + this.sql, Date.now() - queryBegin, this.options);
  }

  return queryResults;
})
.then(queryResults => {
  const queryResult = Array.isArray(queryResults) ? queryResults.pop() : queryResults;
  const rows = queryResult.rows;
  const rowCount = queryResult.rowCount;

node v7.7.3, npm v5.3.0, sequelize v4.4.2, pg v7.0.2

P.S. At the moment rolled back to the 6th version of pg. The flight is normal.

@alitaheri
Copy link
Contributor Author

@4e1ovek That won't always work. Some queries might have their main stuff in the second statement.

@codecov
Copy link

codecov bot commented Jul 31, 2017

Codecov Report

Merging #8029 into master will increase coverage by <.01%.
The diff coverage is 100%.

@alitaheri alitaheri mentioned this pull request Aug 8, 2017
5 tasks
@corysimmons
Copy link

I don't mean to be one of those entitled devs, but can contribs/maintainers take this one a bit more seriously and get a fix, or at least a documented workaround, merged?

Broken enums is a huge problem.

@alitaheri
Copy link
Contributor Author

@corysimmons downgrade to pg 6 for now. it's not just the enums that are broken. Any multi-statement query will fail.

@corysimmons
Copy link

@alitaheri So pg @ 6.4.2. Do I need to add pg-native? Do I need to downgrade pg-hstore to something?

@alitaheri
Copy link
Contributor Author

alitaheri commented Sep 4, 2017

@corysimmons You don't need pg-native. Best not use it anyway, some features aren't implemented there. And pg-hstore is a simple serializer/deserializer with no dependencies. Just install pg@6.4.2 and your issue will be fixed for now.

P.S. pg and pg-native are installed here only for testing purposes.

@corysimmons
Copy link

Thanks Ali. Maybe we can update the README with a warning about pg7 while a fix is in the works?

@gabegorelick
Copy link
Contributor

Is there anything that needs to be done to get this into a mergable state (besides rebasing package.json)?

@joaovitoras
Copy link

Do you intend to merge this pull request?

@sushantdhiman sushantdhiman mentioned this pull request Jan 31, 2018
5 tasks
@alitaheri alitaheri deleted the fix-pg-v7-regressions branch February 3, 2018 11:42
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.

node-postgres version 7 incompatibilities
5 participants