use common sql interfaces

[prolic]

See also here: https://github.com/prolic/sql
(We would need to move the repo to amphp organization)

If I missed something, let me know please.

[prolic]

see also amphp/mysql#74

[prolic]

@trowski the pool interface is here: https://github.com/amphp/sql/blob/master/src/Pool.php

this conflicts with your latest commit, can you revert it?

[prolic]

@trowski updated as well

[trowski]

This class is no longer needed.

[trowski]

Should extend Amp\Sql\Executor and not repeat the methods in that interface.

[trowski]

This interface still needs to extend Executor as well.

[trowski]

This method is already defined in Executor.

[prolic]

@trowski all done

[prolic]

@trowski there are still some tests failing, wanna look into it? you can submit a PR to my branch.

[DaveRandom]

@prolic I have fixed the failing tests in the fix/sql-interfaces-pr branch.

Rather than PR'ing this against your fork and creating a superfluous merge commit, you should be able to simply pull the changes from that branch into yours by doing something like:

# checkout the sql branch locally
git checkout sql

# pull the changes from the branch with the fixes
git pull git@github.com:amphp/postgres.git fix/sql-interfaces-pr

# push the changes up to your fork
git push origin sql

[prolic]

@DaveRandom thanks, it's done.

[DaveRandom]

As it currently stands, I don't think the interface layout is quite right.

The thing that sets off alarm bells is that neither of the concrete Handle classes implement Link, and thus their listen() methods are not covered by an interface. It's not as simple as changing the implements list, as they currently do not implement transaction().

I also have some issues with the interfaces defined upstream by the Sql package, I will open an issue there and link it here.

[trowski]

@DaveRandom Yep, looks like listen() should have been defined in Executor instead of Link. This means Transaction would need to add listen(). Should it? If so, should listening end when the transaction is committed/rolled-back? If not, we may need yet another interface.

Handle also does not need to define lastUsedAt(), as it is defined in Executor in amphp/sql.

[trowski]

@DaveRandom I made some changes based on your feedback in the squash branch. Have a look at the commits I made today and let me know what you think. /cc @prolic

[DaveRandom]

@trowski that seems like a reasonable change, yes. I'm a little surprised tbh, I was expecting LISTEN to be a link-level state, but docs indicate that doing it within a transaction will alter the semantics of the command to reflect the transaction, so it seems that both listen() and notify() should always be available together.

[trowski]

@DaveRandom It sounds like the listen command doesn't take affect until after the transaction is committed? I can do that too I guess… but I will have to keep the connection busy while the listener is active (i.e., the Transaction object will stick around in memory, not releasing the associated connection back to pool). That would probably be expected behavior based on the docs.

[DaveRandom]

@trowski pragmatically, it might be better to just not support listen/notify in transactions, at least for now, Using them in a transaction is functionally identical to changing the logic such that they are called on successful commit, which seems easy to accomplish in app code and avoids what seems like a lot of complexity in the library code to make this interact sanely with a pool.

Doing so initially would not preclude adding the functionality later in a backwards-compatible, if the interface structure is done right,

Just a thought, possibly not a great one :-P

[prolic]

I think this can be closed now? Will you update mysql as well, soon?

[kelunik]

@prolic I can close this now without merging, right?

[prolic]

Yes, it's already merged with rebased branch.