vapor / database-kit Goto Github PK

Title should say it all.

possible fix: append

.catchMap {
          error in
          try self.releasePooledConnection(conn, to: database)
          throw error
        }

the data is in a binary encoding so we need to convert it to something readable when logging queries is enabled

Serializer produce wrong sql statement when using .group(by:) and .sort(_:) together.

Steps to reproduce

SomeModel.query(on: req)
    .group(by: \.id)
    .sort(\Book.id)
    .all()

Expected behavior

SELECT * FROM `some_model`
GROUP BY `some_model`.`id`
ORDER BY `some_model`.`id`;

GROUP BY before ORDER BY.

Actual behavior

SELECT * FROM `some_model`
ORDER BY `some_model`.`id`
GROUP BY `some_model`.`id`;

ORDER BY before GROUP BY.

Possible Cause

In SQLSerializer+DataQuery.swift [Line: 61],

if !query.orderBys.isEmpty {
    statement.append(serialize(orderBys: query.orderBys))
}

was execute before

if !query.groupBys.isEmpty {
    statement.append(serialize(groupBys: query.groupBys))
}

Database connection config may need to be re-worked a bit.

let container = worker as! SubContainer // must be subcontainer, or we will create a cycle

For debugging I've enabled logging and I end up with something like this from my PostgreSQL query

SELECT * FROM "v_orders" WHERE ("v_orders"."month" = $1) ORDER BY "v_orders"."due" DESC ["TIMESTAMP (binary) \0\u{02}\u{11}(÷_@\0)"]

That's not very usable as I have no idea what got passed to the month parameter. Is there some way to enhance this so that it prints out the human readable value?

I see that right now, DatabaseConnectionPoolConfig.maximumConnections is hardcoded to 10 by default.

It would be nice if this number could dynamically scale at startup based on something like:

database maximum available connections / number of workers (cores?) 

My database knowledge is rusty, but they all support some kind of connection reporting, right?, ie for Postgres:

select max_conn,used,res_for_super,max_conn-used-res_for_super res_for_normal 
from 
  (select count(*) used from pg_stat_activity) t1,
  (select setting::int res_for_super from pg_settings where name=$$superuser_reserved_connections$$) t2,
  (select setting::int max_conn from pg_settings where name=$$max_connections$$) t3

Produces:

Connection pool could be expanded with additional storage or "sharing" options to allow for better control over the numbers of connections an application uses.

• Thread: Each connection pool shares its connections with all other pools on the same thread.
• Process: Each connection pool shares its connections with all other pools in the same process. This is similar to thread-sharing but requires locks for synchronizing access.
• Instance: Each connection pool has its own, private connections.

Allow the connection pool to prune connections that have closed due to error or timeout.

Assume we have two requests, request1 and request2.
Each request writes something to database via Fluent API, and server receives these requests at the same time.

First, request handler calls Model.query(on: DatabaseConnectable). Each request is passed as DatabaseConnectable.

Then this function calls Request.databaseConnection(to database: DatabaseIdentifier<D>?).
And finally it reaches Container.requestCachedConnection(to database: DatabaseIdentifier<Database>).

In this function, request1 tries to get cached connection and fails, then gets from connectionPool.
This connection is registered in the cache object.

And, in the same function, request2 also tries to get connection from the cache.
This time connection is found in the cache, but it is the same instance request1 got from pool.

Two requests run DB operations on single connection simultaneously, and, in my case, the program crashes here.

It seems these two requests possess same cache instance. But, in contrast, it seems Request is designed to have own cache, since Request.deinit releases this.

Questions.

1. Is it designed to share connecton cache among requests? or is it a bug?
2. How can we fix this?

I'm using this connection pool and fire up a bunch of threads to insert data into postgresql:
NIO-ELT-#0 (5): EXC_BAD_ACCESS (code=2, address=0x10cdea810)
https://github.com/vapor/database-kit/blob/master/Sources/DatabaseKit/Connection/DatabaseConnectionPool.swift#L62

Thread 3: Fatal error: Can't form Range with upperBound < lowerBound
https://github.com/vapor/database-kit/blob/master/Sources/DatabaseKit/Connection/DatabaseConnectionPool.swift#L53

Am I using the connection pool wrong? How can I avoid these errors?

Compile Swift Module 'DatabaseKit' (30 sources)
/Users/pcmoritz/XXX/.build/checkouts/database-kit.git--5641525802723651692/Sources/DatabaseKit/Connection/Container+NewConnection.swift:23:28: error: missing argument for parameter #1 in call
                conn.close()
                           ^
                           <#_#>
/Users/pcmoritz/XXX/.build/checkouts/database-kit.git--5641525802723651692/Sources/DatabaseKit/Database/DatabaseConfig.swift:72:30: error: argument type 'D.Connection' does not conform to expected type 'Worker' (aka 'EventLoopGroup')
            return .done(on: conn)
                             ^~~~
                                  as! Worker
/Users/pcmoritz/XXX/.build/checkouts/database-kit.git--5641525802723651692/Sources/DatabaseKit/Utilities/Deprecated.swift:39:16: error: value of type 'D.Connection' has no member 'close'
        return conn.close()
               ^~~~ ~~~~~
error: terminated(1): /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2018-07-12-a.xctoolchain/usr/bin/swift-build-tool -f /Users/pcmoritz/XXX/.build/debug.yaml main output:

If anybody has an idea for a workaround, please let me know!

In my application, it's desirable that even the extremely debug-level logs that are database queries when database logging is enabled be redirected to an appropriate log output on a per-request basis - or more exactly, that it be possible to associate a given set of logged queries with a particular request. Currently, the only way this is possible is to "know" that MySQLConnection and SQLiteConnection have public logger properties that one can reset inside request.withPooledConnection(to:) {} closures. Needless to say, this is not very generic. Nor does it lend itself well to the dependency injection that the separation of DatabaseLogger and DatabaseLogHandler implies was a goal of this particular subsystem (to say nothing of the DI-heavy design of Vapor as a whole). I would suggest one of:

1. Make public var logger: DatabaseLogger? { get } a protocol requirement of DatabaseConnection where Self.Database: LogSupporting, which at least solves the problem of overriding the logger per-connection for any given database (except that DatabaseConnection doesn't have a Self.Database, so hm)
2. Have DatabaseLogger leverage Services to get its DatabaseLogHandler (which is what I already expected it to be doing and was surprised to find out otherwise!), which would if properly designed allow for using Request's privateContainer to override the logger per-request
3. Allow an optional DatabaseLogHandler to be passed to with[Pooled]Connection(to:) when Database: LogSupporting, which would override the logger for that connection only - this strikes me as the most threadsafe (and probably also easiest to implement) option, since it can leverage LogSupporting's enableLogging(_:on:) with the connection.

I ran into a deadlock situation on a single CPU virtual machine when using TokenAuthentication. The deadlock appeared in a controller that handles queries to my user table, more precisely when resolving a parameter like so: return try req.parameters.next(User.Public.self).

I tried to break it down and the problem seems to be that a connection to the DB is requested twice. First request appears in BearerAuthenticatable::authenticate when the query for the token is set up; second (nested) request is performed when looking up the parameter.

The nested request can't be fulfilled because there is no free connection left in the pool. So the thread falls asleep and waits for a connection to become available - boom: now we are waiting for ourselve ....

Don't know how to fix it but to me it appears a bit suspicious that requestConnection returns a Future, meaning sometimes in the Future we'll establish a connection, but already marks it as not being available ...

Interestingly, the deadlock only occurs when resolving Parameters. If I perform other queries on the users's table (e.g. use User.query(on:req) ...), no deadlock occurs. So it could be that the real problem is not caused by DatabaseKit but lies in the implementation of Model::make(for parameter: String, using container: Container).

Any ideas?

Was this supposed to be part of the public API?