Re: Cached statements hold strong references to bind values

On Sat, Jan 26, 2013 at 07:21:56PM +1100, Peter Rabbitson wrote:
> On Fri, Jan 25, 2013 at 10:59:46PM +0000, Tim Bunce wrote:
> > On Fri, Jan 25, 2013 at 09:40:07PM +1100, Peter Rabbitson wrote:
> > > On Fri, Jan 25, 2013 at 10:17:11AM +0000, Tim Bunce wrote:
> > > > Re https://rt.cpan.org/Public/Bug/Display.html?id=82942
> > > > 
> > > > The key question: is this a bug or a feature?
> > > 
> > > Can you walk me through your thought process on what makes this a feature?
> > > Not trying to be difficult, I am just having trouble seeing the use-case ;)
> > 
> > A cache statement is cached because the user asked for it to be cached.
> 
> Perhaps there is a mismatch of understanding here. For me the only 
> benefit of using a cached statement is to avoid a prepare. Executing the 
> *same* statement with the *same* values *without* rebind does not seem 
> to be possible in the first place (according to the docs at least):

I don't follow you here Peter. This should work fine:

    $sth = $dbh->prepare_cached($sql);
    $sth->bind_param(1, $foo);
    $sth->execute();
    $sth->execute();
    $sth->execute();
    $sth->execute();

> >     $sth = $dbh->prepare_cached($sql);
> > 
> > I think it's reasonable to assume that binding a reference value to a
> > placeholder of a cached statement handle will cause the ref count of the
> > value to increase.
> > 
> >     $sth->bind_param_inout(1, \@foo);
> 
> Yes, otherwise it may get lost before the consequent execute().

> > So, the question then is what options do we have for "fixing" it?
> > 
> > The most obvious thing to do would be to weaken the refs in
> > values %{ $sth->{ParamValues} }. That could be done in the
> > application, so a DBI change isn't strictly needed.
> 
> That won't work as per my above remark. Now that I think of it your 
> cached statements are not copies, they are extra refs. Which means that 
> you can't really weaken it in the cached ones only.
> 
> > If it's agreed that the DBI should weaken the refs then
> > we'd need to discuss when. As soon as they're bound?
> > I can imagine cases where values are bound in a lexical scope
> > that's exited before the $sth->execute() call, so that's not safe.
> > 
> > After the execute()? No, execute might be called again.
> 
> If this is possible - docs/tests should reflect that (I am still fuzzy 
> on what the semantics are, hence no patches to offer).

Does the example above clarify it?

> > After finish()? Maybe. The docs for finish is already say "Calling finish ...
> > may also make some statement handle attributes (such as NAME and TYPE)
> > unavailable if they have not already been accessed (and thus cached)."
> > It seems reasonable to extend that to say that any references in
> > ParamValues will be weaked.
> 
> After finish() won't work, because the app (DBIC in this case) doesn't 
> know to call finish() before garbage collection, which never happens 
> because of the circular ref. Augh!

> > Bound values are meant to persist across finish(), so there are still risks.
> 
> Also undocumented ;)

Bound values are spec'd to persist in general. No need to mention finish().

> > I'm not keen on adding another attribute just to control this edge case.
> 
> No, we shouldn't. After running through your thought process (thank you) 
> it is clear this can not be fixed with conventional approaches. I need 
> to wrap up Variable::Entangled that we specced out with Aristotle at 
> last YAPC::EU but then neither of us got to completion.
> 
> In the meantime stalling the bugreport with a link to this discussion 
> sounds sane.

Will do.

Tim.