#109 pool release fix

[zbarbuto]

Description

A combination of PR #146 which fixes #109 and a switch from explicitly calling connection.release to using the built-in done callback provided by pg to release connection (see here)

Related issues

• TypeError: Object [object Object] has no method 'release' #109

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[zbarbuto]

@jannyHou @superkhau This has been sitting here for a while and I haven't received any feedback. I noticed the test status is still waiting. #109 is still an issue for us if we don't use this forked branch.

[jannyHou]

@slnode test please

[jannyHou]

@zbarbuto Thank you for the pr, while I am currently working on some other high priority tasks, sorry for the delay of review. I triggered a test, will take a look ASAP.

[superkhau]

@zbarbuto Can you add a test to verify your results?

[raymondfeng]

LGTM

[superkhau]

@zbarbuto Can you rebase this PR also? I think the two failures are unrelated -- something to do with forceId -- @jannyHou please confirm before landing.

[jannyHou]

@superkhau good catch.
@zbarbuto Please rebase your code :) I believe the new checked in pr will get those two failures pass.

[zbarbuto]

Have rebased but can't seem to get the transaction tests to run on my local machine (Uncaught Error: getaddrinfo ENOTFOUND undefined undefined:5432). The README file says "Ask a core developer for instructions on how to set up test server credentials on your machine" - any clues?

[superkhau]

@zbarbuto Those instructions need updating, you should be able to override configs now for local testing via env vars. See https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/init.js#L21.

cc @jannyHou @crandmck We should update that README text

[zbarbuto]

@superkhau Ah yes, is working now, also need to use CI=true for some reason to allow those settings to be read. Adding a test case now.

[zbarbuto]

@superkhau @jannyHou Test added which fails on current master branch but passes with this PR 😀

[zbarbuto]

@superkhau Have made #214 to fix that README information about testing too.

[superkhau]

@rmg Hanging here too.

[jannyHou]

@slnode test please

[jannyHou]

@zbarbuto Now all test pass, please rebase the code and I think it will be ready to land 🚢

cc @superkhau

[zbarbuto]

Rebased. Thanks.

[zbarbuto]

Also would be good to get a release out of this sooner rather than later as it's a rather critical issue with transactions not being able to do more than a few at a time.

[zbarbuto]

@jannyHou @superkhau

[jannyHou]

@slnode test please

[jannyHou]

@zbarbuto After rebase the pr is all good now and we will make a new release after checking in it.

Sorry for the delay of trigging testing :( could you do a rebase again? I will keep an eye on this issue and run test immediately after your rebase. Thank you.

[zbarbuto]

@jannyHou Done

[jannyHou]

@slnode test please

[cadesalaberry]

Cannot wait for the release!

[jannyHou]

@cadesalaberry @zbarbuto released as 2.8.0. Please check. Thanks