Connection Pooling Bug

[stoffman]

Hi,

I found, what I believe to be a bug in TConnectionPool.Acquire.

Code: Select all

try
            // Test for dead connections
            FConnections[I].Rollback; // PingServer did not work (tested with FB)
            FSlotsInUse[I] := True;
            Break;
          except
            // An exception can be raised when the dead connection is dropped
            try
              FConnections[I] := nil;
            except
            end;
            Inc(I);
          end;

The problem is that a connection that is set to AutoCommit := true will always throw an exception when you try to rollback. (TZAbstractConnection.Rollback -> CheckNonAutoCommitMode) . So even if the connection is still valid the Aquire will in-fact release it and established a new connection.
I think, in-order to fix it there are 2 options:
1. Fix ping server
2. Stop the throwing the exception

Any ideas? Maybe it is not a bug? Something I missed? More resolution options?

[marsupilami]

Hello Stoffman,

I didn't even know that somebody uses the pooled driver. Good to know

Anyway - the driver tries to find out if the connection is still working and the server is connected. The ping method isn't implemented for all supported servers. That possibly is why it was changed to using Rollback - which is an operation that is supported by most database systems.
Maybe we shoud check the AutoCommit state of the connection and either Rollback if it is not in autocommit mode or do a StartTransaction / Rollback if it is in autocommit mode? What do you think?

Best regards,

Jan

[aehimself]

A while ago I changed .Ping to throw EZUnsupportedException if it is not implemented / supported. What would be even better is to call .Ping, and in case EZUnsupportedException was raised, check for AutoCommit and call .RollBack. In case we are in autocommit we can try to execute a select null instead.

How would this sound?

[marsupilami]

In case we are in autocommit we can try to execute a select null instead.

The problem I see there is that the SQL for that operation looks different for each database:

• Firebird: select ... from RDB$DATABASE
• PostgreSQL, MS SQL Server, Sybase: select ...
• Oracle: select ... from dual

This is why I suggest to start a transaction and roll it back. These operations should be supported by almost all databases. And databases that don't support transactions (MS Jet Engine, ...) most probably don't get used with a connection pool - I hope...

[stoffman]

My understanding is that PingServer is a *concept* the actual implementation details do not really matter.

If a driver supports Ping using the driver. Great. Use it. Otherwise it should try to 'select null... ' with the its specific syntax or start/rollback transaction inside the PingServer method.

This solves the MS Jet Engine issue, and it provides a clean solution for anyone else that needs to use PingServer not in the context of the PoolConnection.

This as also the benefit that the code in the TConnectionPool.Acquire does not check for AutoCommit as it is complicates the code.

[marsupilami]

My understanding is that PingServer is a *concept* the actual implementation details do not really matter.

PingServer is a concept and how it is implemented depends on the driver.

If a driver supports Ping using the driver. Great. Use it. Otherwise it should try to 'select null... ' with the its specific syntax or start/rollback transaction inside the PingServer method.

It seems that currently drivers are allowed to raise an exception if they don't implement the PingServer method. It was discussed as a part of this thread. The pooled driver will have to be created with this design decision in mind. Feel free to implemet Ping for drivers that don't implemet it yet.

[stoffman]

My point, if I wasn't clear, is that the TConnectionPool.Acquire should call PingServer. It should not address the AutoCommit status of the connection directly.

[marsupilami]

My point, if I wasn't clear, is that the TConnectionPool.Acquire should call PingServer. It should not address the AutoCommit status of the connection directly.

I understand what you mean. The question is - what should the pooled driver do if it gets an EZUnsupportedException when trying to ping? Assume the connection is dead? Assume the connection works? Or try to use a different scheme for testing if the connection still works?

[aehimself]

The question is - what should the pooled driver do if it gets an EZUnsupportedException when trying to ping? Assume the connection is dead?

Definetly no. In that case the pool would never hand a single connection out if .Ping is not supported.

Or try to use a different scheme for testing if the connection still works?

I think this is going to be the way. I personally really like your idea of Start transaction - Rollback, as it works fine as an empty nested transaction and it also leaves .AutoCommit alone (.Rollback sets autocommit back to True if it was before).

The only thing we have to be careful about it to make sure no actual modification happens between this StartTransaction - Rollback in an other place. As one thread should have it's own connection and Application.ProcessMessages doesn't exist though this should be fine.

[MJFShark]

I'm not sure this is useful or not, but what I do to fake a ping to the server is to request a tablelist with a filter that won't exist. I'm not sure it's the way to go, but it's been working pretty well in all my test cases.

-Mark

Code: Select all

    if NativePingSupported then
    begin
      Result := ZConn.PingServer;
    end
    else
    begin
      // The idea here is to request a table list with just a name that *probably* doesn't exist.
      var MetaData: IZDatabaseMetadata;
      var ResultSet: IZResultSet;
      Metadata := ZConn.DbcConnection.GetMetadata;
      ResultSet := Metadata.GetTables('', '', 'ZPINGZ', []);
    end;

[aehimself]

I don't know too much about how RDBMS systems work but I guess even creating an empty dataset after collecting the (non-existent) data takes more resources than creating a snapshot and immediately releasing it.

What I know for sure is that it is faster.

[stoffman]

I think that we can all agree that if PingServer was implemented for all the drives it would have been the best solution, as such I propose that the *default implementation* of PingServer *will not be* raising EZUnsupportedException, but start/rollback transaction or 'select null... ' or whatever.

Then each driver can override the default implementation with more specific an efficient way to do the PingServer. And the code on TConnectionPool.Acquire is kept clean.

[aehimself]

I think that we can all agree that if PingServer was implemented for all the drives it would have been the best solution, as such I propose that the *default implementation* of PingServer *will not be* raising EZUnsupportedException, but start/rollback transaction or 'select null... ' or whatever.

I personally disagree. A .Ping operation is the bare-minimum way to verify if the remote RDBMS is still reachable. This should be done with the least necessary network / server side resource usage. "Imitating" this by transaction is suboptimal, especially because if the application is not written correctly this can lead to a potentional data loss as I described earlier.

I would say imitating a not supported .Ping is the job of the developer using Zeos as (s)he sees fit. But using the transaction trick is a very good alternative for null-queries IMO.

On the other hand, if .Ping is not implemented in Zeos but is supported by your RDBMS-of-choice and the library Zeos is using to access it - contributions are always welcome :)

[stoffman]

Using transaction is suboptimal for sure but as a default implementation it is acceptable for the following reasons:

1. IMHO having a suboptimal solution is OK, providing an uncomplete implementation is less so. TConnectionPool is not optimal in any way.. but it exists and it helped me a lot and I'll push changes to it later. When a future user will see a performance problem he or she can then implement a more robust solution to the ping and we all benefit.
2. As you said: Application.ProcessMessages should not be called during the transaction (it can be ensured with the default implementation). But if an application is not written correctly, and the connection is used by 2 threads, nothing will help no matter how it is implemented.
3. sqlite - is not a server, so you cannot really ping it. ever. so why not provide an acceptable solution? (I believe the same is true with Firebird. After reading the API reference for half an hour I couldn't find a ping or the like in the dll https://www.firebirdsql.org/pdfmanual/h ... ntlib.html)