Developers of Sympa

[IKEDA Soji <address@concealed>]

My revised code on yesterday doesn't work (I mistakenly removed a
line to restore statement).  I'll post correct code after some days.
Besides,

On Tue, 20 Aug 2013 14:55:07 +0200
Guillaume Rousse <address@concealed> wrote:

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

In sense.  As there are no 100% confidence, more precise checks
can be useful.

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

I worry this proposal might kill advantage of current code in
sympa-cleanup which can run prepare() and execute() separately.
That's why I tried to harmonize such separation and sustainable
connection.

<<snip>>

<<Sorry for reordering your comments below>>

[1]
> > 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

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

Don't they work?  Please give me some time to examine it.
Latter coding is important because caller-side will be able to use
refreshed handle.  Former is not so important for me.

[2]
> >          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.

I don't stick to sanitize queries, too.

[3]
> >          # 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.

I agree.  Checking dbh itself is useless.

[4]
> >                  $self->{dbh}->{CachedKids} = {} if $self->{dbh};
> I'd rather not mess with internal dbh state.

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

Caching feature of prepare_cached() is implemented by DBI module
using hash.  There are a few differences from cache mecanism prepared
by us.  So I don't stick to use of this method (along with CachedKeys
handle attribute).

[5]
> > sub execute($$@) {
> -1 for one-word-only function name: "execute" is way to vague, 
> especially when compared with "get_query_handle"

I might probably write why I did such in later days.
 

[6]
I had written on your proposal below at beginning of this post.

> 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


-- 
株式会社 コンバージョン  セキュリティ&OSSソリューション部   池田荘児
〒231-0004　神奈川県横浜市中区元浜町3-21-2 ヘリオス関内ビル7F
e-mail address@concealed  TEL 045-640-3550
http://www.conversion.co.jp/