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):
`... If another call is made to prepare_cached with the same $statement
and %attr parameter values, then the corresponding cached $sth will be
returned without contacting the database server. ...`
>
> $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().
>
> The particular case in the ticket uses an reference to an object with
> stringfy magic:
>
> $sth->bind_param(1, $object_that_stringifies);
>
> which is an extra wrinkle.
>
> All this seems like reasonable and natural behaviour to me.
> However, it also seems potentially inconvenient, as you've found.
>
> 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).
>
> 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 ;)
>
> 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.
