Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 12/08/2013 12:02, IKEDA Soji a écrit :
> I told on daemons.  WWSympa, SympaSOAP and command line tools should
> rather fail immediately when connection has lost.
AFAIK, WWSympa, when running under Fast::CGI, is a persistent process, 
as a daemon.

> > > 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.
That's plainly overkill, as only daemons needs this kind of safety. 
Short-timed code, such as for instance database schema upgrade, doesn't.

The code I removed did exactly what I said: for each query execution 
step (prepare, execute), if there was any failure, a direct reconnection 
attempt was made, without attempting to analyse exact error cause. For 
this reason, do_query is 60 lines long, and quite painful to read.

If you really want the 'let's try to reconnect if any kind of query 
fails', here is a better model, using two distinct functions for two 
different contexts:

sub execute_query {
        my ($self, $query, @values) = @_;

        return $self->{dbh}->do($query, undef, @values);
}

sub execute_query_safely {
        my ($self, $query, @values) = @_;

        my $result = $self->execute_query($query, @values);
        if (!$result) {
            my $connected = $self->ping();
            if (!$connected) {
                $self->connect();
                $result = $self->execute_query($query, @values);
            }
        }

        return $result;
}

And even better, a smart execute_query_safely function, relying on 
exception string to identify error cause:

sub execute_query_safely {
        my ($self, $query, @values) = @_;

        my $result;
        eval {
            $result = $self->execute_query($query, @values);;
        };
        if ($EVAL_ERROR) {
            # something wrong happened
            if ($EVAL_ERROR =~ /some_pattern/) {
                # that's not a connection error, we can't do much here
            } else {
                # that may be a connection error
                if (!$connected) {
                    $self->connect();
                    $result = $self->execute_query($query, @values);
                }
            }
        }

        return $result;
}

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