Added support for Notifications that use DispatchSource

[mlilback]

Using a DispatchSource is less wasteful of CPU power as per PostgreSQL documentation.

A parametrized init for Connection.Notification is needed so an application using Notifications does not need to import CPostgreSQL.

Fixed a compiler warning about an unused variable.

[codecov[bot]]

Codecov Report

Merging #59 into master will decrease coverage by 2.04%. The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   82.33%   80.29%   -2.05%     
==========================================
  Files          10       10              
  Lines        1325     1142     -183     
  Branches      107        0     -107     
==========================================
- Hits         1091      917     -174     
+ Misses        229      225       -4     
+ Partials        5        0       -5

Impacted Files	Coverage Δ
Sources/PostgreSQL/Error.swift	28.81% <100%> (-5.4%)	:arrow_down:
Sources/PostgreSQL/Connection.swift	91.76% <76.31%> (-4.94%)	:arrow_down:
Sources/PostgreSQL/Result.swift	74.25% <0%> (-9.86%)	:arrow_down:
Sources/PostgreSQL/Bind/FieldType.swift	25.24% <0%> (-5.01%)	:arrow_down:
Sources/PostgreSQL/Bind/Bind.swift	77.24% <0%> (-2.17%)	:arrow_down:
Sources/PostgreSQL/Database.swift	100% <0%> (ø)	:arrow_up:
Sources/PostgreSQL/ConnectionInfo.swift	100% <0%> (ø)	:arrow_up:
Sources/PostgreSQL/Bind/Bind+Node.swift	94.79% <0%> (+0.14%)	:arrow_up:
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d57aa99...b127f46. Read the comment docs.

[vzsg]

This is really nice, thank you! Personally I wouldn't mind eliminating the old solution for notifications altogether, in favor of using Dispatch.

What do you think, @sroebert?

[Joannis]

@tanner0101 I think this would remove the need for a pure swift driver.

[sroebert]

Nice work, this is much better than the existing function with sleeping. This should simply replace the old one. I added some review comments.

Also have a look at the tests, as the changes are only covered 66%

[mlilback]

To test the error handling code at 206-207 the test needs to close the connection. Then it crashes in the deinit because that tries to close it, too.

This will always crash because cConnection can't be set to nil. Any ideas on how to fix this so it doesn't crash? I see two options:

1. make public private(set) var cConnection: CConnection and set to nil in close and check for nil before calling PQfinish.

2. Add a Bool flag to know if it was closed.

[sroebert]

I actually think the close function is not correct in this case. Close should check if it is connected and only close in that case, otherwise it should simply do nothing. So the validateConnection should not be there in my opinion.

And probably the same goes for the reset function.

[mlilback]

PQfinish is documented as freeing the memory and "must not be used again". That means there is no way to query if the connection is open without a crash. So it comes back to my previous question. Make cConnection nullable or add a flag for the connection state?

Generally I'd want to make it optional, but that is an API breaking change.

[sroebert]

Ok, yeah that is a tricky one. I would also go for making it optional. I know it is a breaking change, but at the same time, I doubt there will be many (if any) libraries out there, making direct use of this property.

What about making this property deprecated, changing it to a var that points to a new optional property, throwing a fatal error if it is nil.

[mlilback]

reset() and close() were marked as throws because they called validateConnection(). The connection status isn't checked after the call to PQreset(). Therefore, an error is thrown if the connection is invalid (isn't that the reason reset() is called?) but not if the reset fails.

I think reset() should only throw an error if PQStatus() is CONNECTION_BAD after the call to PQreset(). Throwing an error if already closed will fail current tests. Thoughts?

Along the same line, I think close should not through an error if already closed. It is throwing an error if isConnection is false. That would mean it would not throw errors anymore, so should close() no longer be marked as throws, or keep it for compatibility?

[sroebert]

Yes, that is exactly what I meant. I like the idea of throwing an error is reset does not properly reset.

About compatibility, I find it a bit hard, as it is a breaking change, but again, I doubt anyone is using this directly currently. And I don't like the idea of keeping it a throwing function when it does not. So in my opinion we should change it to not throwing.

[sroebert]

Any idea what the problem with travis is? I see some message about brew and vapor.

[mlilback]

Here are a couple of possible fixes: https://github.com/PowerShell/PowerShell/issues/5062

Either add a homebrew environment variable, or move the location of brew update. Seems like an issue with a recent brew upgrade.

[mlilback]

can someone merge this if it all good?

[natebird]

@mlilback - Can you update the .travis.yml file to move the brew update line above other brew commands? That should solve the failing tests. Thanks!