Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 24/08/2013 05:35, IKEDA Soji a écrit :
> On Thu, 22 Aug 2013 00:43:02 +0900
> IKEDA Soji <address@concealed> wrote:
> >                               I'll post correct code after some days.
> Attached below.  Some remarks:
>
> - PREPARE may be implemented on server-side (PostgreSQL, Oracle,
>    ...), hence connection loss breaks SQL session so that statement
>    handles cached on client-side no longer will be usable.  So we
>    shouldn't provide "cached" set of functions and "safe connection"
>    set separately:
Just because the cache may be eventually invalidated by a connection 
loss doesn't mean we shouldn't allow the calling code to specify that 
caching should be used (if possible), or should never be used (because 
useless).

>  We may provide exactly two sets:
>
>      $handle = $database->prepare_query_handle_safely($statement, ...);
>      $databse->execute_safely($handle, ...);
>
>      $handle = $database->prepare_query_handle($statement, ...);
>      $handle->execute(...);
>
> - We must RTFM.  Prototype won't work on methods; assigning local @_
>    will work.
That's more in line with what I originaly suggested, however this is 
still too much complex, and not versatile enough, for my taste.


> --- Soji
>
> sub get_query_handle {
>          my ($self, $query, %types) = @_;
>
>          my $handle = $self->{dbh}->prepare($query);
>          return undef unless $handle;
>
>          # Bind parameters with special types.
>          # Since this may be done only once when handle is prepared, dummy
>          # parameter value '0' will be used.
>          foreach my $i (sort keys %types) {
>                  next unless defined $types{$i};
>                  return undef
>                          unless $handle->bind_param($i, '0', $types{$i});
>          }
You're pre-assuming we may need to specifically assign explicit types to 
binded parameters, through usage of bind_param() before calling 
execute(). I don't have your knowledge of Sympa internal, but that's 
pure speculation for me. As long as we don't have an explicit need for 
this added complexity (and a corresponding test case), I'd rather avoid it.

>          # Since some DBDs don't implement "ParamTypes" and/or "Statement"
>          # handle attributes, save them into private attributes.
>          $handle->{private_Sympa_statement} = $query;
>          $handle->{private_Sympa_types} = \%types if %types;
You're interfering with a foreign object internal state, just to be able 
to automatically replay the query if eventually needed later.


>          return $handle;
> }
Corresponding simplified function:

sub get_query_handle {
    my ($self, $query) = @_;
    return $self->{dbh}->prepare($query);
}

Indeed, this won't cover the same exact usage scope, but is far much 
readable.

> sub prepare_query_handle_safely {
>          my ($self, $query, %types) = @_;
>
>          # Check connectivity.  When it failed, try to establish new session.
>          if ($self->{dbh}->ping) {
>                  # Use cached handle if any.
>                  return $self->{cache}->{$query} if $self->{cache}->{$query};
>          } else {
>                  # Discard dead caches then reconnect.
discarding cache should be handled in connect() instead.

>                  map { $_->finish } values %{$self->{cache} || {}};
calling finish() is useless, see DBI documentation here.

Morevoer, this kind of call will trigger a 'map usage in void context' 
warning. The 'correct' way is:
$_->finish foreach values %{$self->{cache} || {}};

>                  delete $self->{cache};
>                  $self->connect(keep_trying => 1);
>          }
>
>          # Prepare statement.  Failure means syntax error or binding error,
>          # because connectivity has already been checked.
>          return
>                  $self->{cache}->{$query} =
>                  $self->get_query_handle($query, %types);
> }
This is a wrapper over get_query_handle(), but the name doesn't match: 
get_query_handle_safely would be better in this regard.

Moreover, the difference isn't just in the additional safety, that's 
also in caching mechanism. Basically, you have the choice between either 
(caching AND automatic reconnection) OR (no caching AND no automatic 
reconnection). If you want security without caching (because you know 
the query won't ever be needed again), or if you want caching without 
security (because you created database connection 3 lines ago), that's 
impossible.

> sub execute_safely {
execute what ? That would rather be called 'execute_query_safely'.

>          # Don't break @_ so that $handle may be updated.
>          my ($self, $handle, @params) = @_;
>
>          # Execute.  When failure caused by lost connection, try to establish
>          # new session.
>          my $rv = $handle->execute(@params);
>          return $rv if defined $rv;           # success.
>          return undef if $self->{dbh}->ping;  # other than lost connection.
>
>          # Discard dead caches then reconnect.
>          map { $_->finish } values %{$self->{cache} || {}};
>          delete $self->{cache};
>          $self->connect(keep_trying => 1);
>
>          # Retry preparing and executing.
>          my $query = $handle->{private_Sympa_statement};
>          my %types = %{$handle->{private_Sympa_types} || {}};
>          $handle = $self->get_query_handle($query, %types);
>          return undef unless $handle;
>
>          $self->{cache}->{$query} = $handle; # update cache.
>          $_[1] = $handle; # update handle on caller side.
That's still useless. $handle variable on caller side contains the same 
memory location as the local $handle variable, they are references to 
the same object.


>          return $handle->execute(@params);
> }
The separation between prepare() and execute() stages in two different 
methods has been implemented in sympa_cleanup branch only, and only 
because I didn't realized we could cache the query string before binding 
parameters. Basically, it was a (probably bad) idea of mine. However, I 
don't see much interest in this separation, and the suggested code is 
even counter-productive here, because of object encapsulation violation. 
Given sympa sofar didn't need this separation, I'd rather revert to 
original 'all-in-once' method.

Minor implementation issues aside, we now have two different APIs proposals.

Soji's proposal (with some fantasy of mine for method names):
1) $database->get_handle($query, %types)
2) $database->get_handle_safely($query, %types)
3) $database->execute_query_safely($handle, @params)

The first two are alternatives, and the caller has to call either (1) or 
(2), then (3) (or call execute() directly on the query handle).

My own proposal:
1) $database->execute_query($query, @params)
2) $database->execute_query_safely($query, @params)
3) $database->execute_reusable_query($query, @params)
4) $database->execute_reusable_query_safely($query, @params)

These methods are alternatives, and the caller has to call one of them.
--
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