Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 10/08/2013 05:57, IKEDA Soji a écrit :
> Guillaume,
>
> sympa[9448] branches/sympa-cleanup/src/lib/Sympa/Datasource/SQL.pm: [dev] 
> drop uselessly complex connection pooling, there is no use case for it
> sympa[9478] branches/sympa-cleanup/src/lib/Sympa/Datasource/SQL.pm: [dev] 
> kill now useless do_query() and do_prepared_query() methods
>
> I suppose connection pooling, or mechanism to reconnect DB server,
> is needed.
That's two different issues.

The connection pooling mechanism was just a way to reduce connection 
duplication in the code, by comparing connections parameters during 
connect() call, and eventally reuse an already-existing DBI handle if 
needed. AFAIK, there is no concurrency in Sympa model (excepted maybe 
when using FastCGI, I'm unsure here), so a single database connection 
for the whole process should be enough. So, instead of having database 
connections created all over the place, and having code to magically 
coalesce them, it's much simpler to decide than a single database object 
for the whole process is enough, than it should be created only in the 
process executable, and be available everywhere else simply as 
$main::database.

Database connection, however, is needed, but only for long-lasting 
processes (sympa daemon, sympa web interface), not for every kind of 
code needing to interact with the database.

> In fact, database connection will be lost occasionally by various
> reasons (e.g. idle timeout, network trouble, ...), even if DB
> connection has been successfully established.
>
> Well, reconnect operation will block till connection comes back
> again.  I suppose it is preferable behavior: (1) Sympa does not use
> transaction (BEGIN...COMMIT/ROLLBACK).  To prevent inconsistencies
> or data loss as much as possible, programs should wait for resuming
> DB operations, instead aborting operations immediately.  (2) Retrying
> connection by increasing duration will reduce flood of system log
> caused by unavailable connection.
>
> Additionally, "Cannot connect to database %1, still trying..."
> listmaster notification mail message was very useful for me,
> however, this feature seems to be removed on sympa-cleanup branch.
I just removed the 'if any query fails, just try to reconnect and to 
execute it again' logic from low-level query execution methods, to 
separate concerns and keep final method KISS. Nothing prevent to add 
another wrapping methods, such as:

execute_query($query);
if (failure) {
    execute_query($query);
}

However, I don't think blindly retrying every kind of failed query is 
the best strategy here. At least a minimal error analysis would be 
needed here, to make some distinctions with trivial errors having 
nothing to do with connection failure.

Morevoer, it seems a better idea to check database availability once, 
before attempting a set of operations, rather than after every failing 
operation. For instance, the endless loop in sympa web interface:

while ($query = new_loop()) {
    # check if the database is still available
    if (!$base->ping()) {
        my $result = $base->connect();
        ...
    }
}

--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

Attachment: smime.p7s
Description: Signature cryptographique S/MIME