Developers of Sympa

[IKEDA Soji <address@concealed>]

Hi,
Here some additional comments,

On Mon, 12 Aug 2013 19:02:19 +0900
IKEDA Soji <address@concealed> wrote:

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

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

I suspect there might be a misinterpretation on code of
SDM::do_prepared_query().  It retrys connection/preparation not
always _but_ when execution of query handle fails.

Since connection failure happen on lower layer, there are no ways to
recognize it immediately on higher layer.  Only way for Perl
programmers to detect connection failure is to access server in
practice.

OTOH if cached query handle fails to execute, it must _not_ be due
to syntax error, because SQL statement had been passed prepare()
method.  Server-side or connection problems are possible.

So, it may be reasonable to retry connection when execute() method
failed.  I rewrote code based on this idea (as a result, it has
very similar logic to SDM code) attached in bottom of this post.

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

Revised code:

- 'wont_block' attribute may allow/disallow attempt to reconnect.
- Feature to bind values with special types (BLOB etc.) is omitted,
  but adding it is easy.

sub get_query_handle($$%) {
        my ($self, $query, %types) = @_;

        $query =~ s/^\s+//;
        $query =~ s/\s+$//;
        $query =~ s/\s*[\r\n]\s*/ /g;

        # Check connectivity.  Failure means lost connection.
        unless ($self->{dbh} and $self->{dbh}->ping) {

                # If blocking is allowed, discard statement cache and try
                # establishing new session.
                if ($self->{wont_block}) {
                        return undef;
                }
                $self->{dbh}->{CachedKids} = {} if $self->{dbh};
                $self->connect(keep_trying => 1);
        }

        # Prepare statement.  Failure means syntax error, because connectivity
        # has already been checked.
        return $self->{dbh}->prepare_cached($query);
}

sub execute($$@) {
        my ($self, $handle, @params) = @_;

        # Execute.  Failure is considered as lost connection, because syntax
        # has already been checked.
        my $rv = $handle->execute(@params);
        if (defined $rv) {
                return $rv;
        }

        # If blocking is allowed, discard statement cache and try establishing
        # new session.
        if ($self->{wont_block}) {
                return undef;
        }
        $self->{dbh}->{CachedKids} = {};
        $self->connect(keep_trying => 1);

        # Retry preparing and executing.
        unless ($handle = $self->{dbh}->prepare_cached($query)) {
                return undef;
        }
        $_[1] = $handle; # update argument on caller side.
        return $handle->execute(@params);
}



-- 
IKEDA Soji <address@concealed>