Developers of Sympa

[IKEDA Soji <address@concealed>]

On Mon, 12 Aug 2013 10:04:06 +0200
Guillaume Rousse <address@concealed> wrote:

> 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.

I agree.  Sympa currently doesn't need connection pooling in the
original meaning.

> 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.

I told on daemons.  WWSympa, SympaSOAP and command line tools should
rather fail immediately when connection has lost.


> > 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();
>          ...
>      }
> }

From view of pessimist, connection can lose everytime; consequently,
lost connection shall be restored _everytime_ _before_ execution of
each query.
The code you removed seems to be written under that idea.  In my
paraphrase:

sub execute_SQL_statement {
    my $self = shift;
    my $statement = shift;
    my @bind_vals = @_;

    my $sth;

    unless ($self->{'dbh'} and $self->{'dbh'}->ping) {
        # Connection lost.  Let's block until connection comes back.
        $self->connect('keep_trying' => 1);
    }
    unless ($sth = $self->{'dbh'}->prepare($statement)) {
        return undef;
    }
    for ($i = 0; $i < scalar @bind_vals, $i++) {
        $sth->bind_param($i + 1, $bind_vals[$i]);
    }
    unless ($sth->execute) {
        return undef;
    }
    return $sth;
}

I feel this is more robust.


-- 
株式会社 コンバージョン  セキュリティ&OSSソリューション部   池田荘児
〒231-0004　神奈川県横浜市中区元浜町3-21-2 ヘリオス関内ビル7F
e-mail address@concealed  TEL 045-640-3550
http://www.conversion.co.jp/