Developers of Sympa

[IKEDA Soji <address@concealed>]

Guillaume, sorry for very delayed response.

On Mon, 02 Sep 2013 15:51:40 +0200
Guillaume Rousse <address@concealed> wrote:

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

If connection loss invalidates all statement handles and handles were
never cached, connection must be confirmed anytime when queries are
executed.
So, we rather would cache all prepared handles, and would throw away
all of them at once, if connection loss is detected.

Of course this is not perfect solution.  Even if connection was
alive, it _can_ lose at the next moment.  And even if connection was
alive and a query was executed successfully, it _can_ lose during
result set is fetched.
However, this is slightly more efficient and makes codes simpler
than wrapping all execute() calls with error handling codes.

I'll attach corrected code at the end of this post.

<<snip>>

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

No, not all types must not be assigned explictly (note that %types
may not contain fields on all parameters).  However, AFAIK "blob"
and "double precision" types will cause problem without explicit
assignment.  Others look working without assignment.

"blob" values can be truncated by several DBDs, if they contain null
bytes; "double" values can be treated as interger values.

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

See L<DBI/"private_your_module_name_*">.  This is the official way not
to interfer internal state of DBI objects.

<<snip>>
> >                  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} || {}};

I mislead.  finish() is not usable here.  Caches must be simply
thrown away, as I had wrote in the beginning of this post.

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

I wrote about this point above.

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

$handle seems to be newly created by get_query_handle() (essentially
by prepare(), not prepare_cached()).


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

As I wrote, caching and sustainable connection are not separable
each other.

So if you propose the functions with preparing and executing at
once, there may be two --- normal and "safely" --- versions.

These correspond to do_query() and do_prepared_query() in the old
SDM package.


Regards,

--- 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});
        }
        # 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;

        return $handle;
}

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 all caches (they are dead) then reconnect.
                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);
}

sub execute_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 all caches (they are dead) then reconnect.
        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.
        return $handle->execute(@params);
}

$$