Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 20/08/2013 13:46, IKEDA Soji a écrit :
> 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.
prepare() doesn't have final values, whereas execute() does. Sure, 
connection problems may happen, but I don't think you can have 100% 
confidence it is the cause here.

> 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.
It is still not reasonable to mix concerns (executing a query, and 
ensuring a query is executed correctly) in the same piece of code. As I 
said earlier, you don't need such kind of safety in every context. Hence 
my proposal of using two different methods, the second wrapping the 
first one:
- $database->execute_query()
- $database->execute_query_safely()

[..]
> 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($$%) {
-1 for function prototypes: we don't use them anywhere else.

Morevoer, they are ignored for method calls:
http://stackoverflow.com/questions/297034/why-are-perl-5s-function-prototypes-bad

>          my ($self, $query, %types) = @_;
>
>          $query =~ s/^\s+//;
>          $query =~ s/\s+$//;
>          $query =~ s/\s*[\r\n]\s*/ /g;
I'm not big fan of automatically tidying every incoming queries. Given 
we control the calling code, I'd rather ensure our queries are correctly 
formated everywhere, for readability sake at least. But that's a minor 
issue, I can live with it.

>          # Check connectivity.  Failure means lost connection.
>          unless ($self->{dbh} and $self->{dbh}->ping) {
There is no reason to check if $self->{dbh} is defined here, and not later.

Morevoer, this kind of check will allow the following erroneous code to run:
my $database = Sympa::Database->new(%params);
# no call to $database->connect() !
$database->get_query($query);

Such kind of logic error should not be possible by design.

>                  # If blocking is allowed, discard statement cache and try
>                  # establishing new session.
>                  if ($self->{wont_block}) {
>                          return undef;
>                  }
>                  $self->{dbh}->{CachedKids} = {} if $self->{dbh};
I'd rather not mess with internal dbh state.

>                  $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($$@) {
-1 for one-word-only function name: "execute" is way to vague, 
especially when compared with "get_query_handle"

>          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} = {};
There is even less reason to mess with internal dbh state here.

>          $self->connect(keep_trying => 1);
>
>          # Retry preparing and executing.
>          unless ($handle = $self->{dbh}->prepare_cached($query)) {
>                  return undef;
>          }
-1 for mixing actions and conditions. This is far more readable as:
my $handle = $self->{dbh}->prepare_cached($query);
unless ($handle) { ... }

>          $_[1] = $handle; # update argument on caller side.
$handle is a reference, this is useless.

>          return $handle->execute(@params);
> }

Also, you're switching from managing our own cache from using 
DBI-managed cache. I'd rather not, for sake of better control, unless 
there is an explicit reason for it.

Here is a counter-proposal, based on your previous remarks, with four 
alternative methods:

# takes a query string and its list of arguments
# prepare and execute it, then return the corresponding handle
sub execute_query {
    my ($self, $query, @params) = @_;

    # tidify query

    my $handle = $self->{dbh}->prepare($query);
    return if !$handle;

    my $result = $handle->execute(@params);
    return if !$result;

    return $handle;
}

# takes a query string and its list of arguments
# prepare, cache and execute it, then return the corresponding handle
sub execute_reusable_query {
    my ($self, $query, @params) = @_;

    # tidify query

    my $handle =
        $self->{cache}->{$query} ||=
        $self->{dbh}->prepare($query);
    return if !$handle;

    my $result = $handle->execute(@params);
    return if !$result;

    return $handle;
}

# takes a query string and its list of arguments
# call execute_query(), and if it fails,
# try to reconnect before trying again once
sub execute_query_safely {
    my ($self, $query, @params) = @_;

    my $handle = $self->{dbh}->execute_query($query, @params);
    return $handle if $handle;

    # something wrong happened, check connection
    my $is_connected = $self->{dbh}->ping();

    # this is not a connection issue, nothing to do
    return if $is_connected;

    # connection has been lost, try to reconnect
    $self->connect(keep_trying => 1);

    # retry execution once
    return $self->{dbh}->execute_query($query, @params);
}

# takes a query string and its list of arguments
# call execute_reusable_query(), and if it fails,
# try to reconnect before trying again once
sub execute_reusable_query_safely {
    my ($self, $query, @params) = @_;

    my $handle = $self->{dbh}->execute_reusable_query($query, @params);
    return $handle if $handle;

    # something wrong happened, check connection
    my $is_connected = $self->{dbh}->ping();

    # this is not a connection issue, nothing to do
    return if $is_connected;

    # connection has been lost, try to reconnect
    $self->connect(keep_trying => 1);

    # retry execution once
    return $self->{dbh}->execute_reusable_query($query, @params);
}

Here, we have clear distinctions between:
- caching and non-caching methods
- blocking and non-blocking methods

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