On Sun, Mar 02, 2003 at 02:21:41PM +1100, Stas Bekman wrote:
> Tim Bunce wrote:
> [...]
>
> >Now, back to memory management...
> >
> >As I've mentioned recently, I figure the 'thing' that's placed into
> >the pool is a scalar string that holds a copy of the entire imp
> >data structure. Doing that, rather than passing an integer pointer
> >around, means we avoid issues over which interpreter owns and frees
> >that memory.
> >
> >That just leaves two other related issues...
> >a. How database API data gets cleaned up.
> >b. Who (which interpreter) owns and frees any SVs _within_ the imp data.
> >
> >The best way to handle (a) would be to create a new dbh using the
> >imp data and then let the dbh get destroyed 'normally' (not pass
> >the imp data back to the pool).
> >
> >For (b) we may be able to duck the issue to a certain extent because
> >all we're really trying to pass around is the pointers from the
> >database API. There should be no need to use any SV pointers.
> >On the other hand, any SVs need to get freed at some point. Only
> >the driver know where they are so the driver has to be involved.
> >
> >Seems like the $dbh->take_imp_data method will need driver involvement
> >to 'cleanse' the imp data by freeing everything not essential to
> >the connection - and hopefully that'll include all the SVs.
> >
> >If SVs are required, for some reason, can they be made shared, or
> >replaced with a shared copy? (My perl threads api knowledge is a
> >bit rusty.)
>
> There should be no perl datastructures involved at all in the pool storage.
Maybe not 'involved' but there's a strong chance the imp_dbh contains SV*'s.
If imp_dbh contains an SV* in the drivers portion and the imp_dbh
was allocated by thread A. Thread A deposits it into the pool
(copying the imp_dbh and the SV pointer) and then exits. Another thread
gets a copy of the imp_dbh from the pool, the DBI wraps a handle around it
and the driver reinitialises the drivers portion. In doing that it will
try to copy the contents of the SV*. That's now very dangerous as the
thread that allocated the original one has not exited.
That's the scenario I was refering to in those last three paragraphs
you quoted above.
> Finally the only fishy thing is the following: Since we memcpy the whole
> imp_dbh, including the com struct, the memory used by that struct (first n
> bytes) is invalid after the original dbh has been destroyed and should
> never be accessed. In fact we may want to null it all.
I'd expect the take_imp_data method to do that and clear the IMP_COM_SET flag.
> Only the memory past
> the com struct is valid. We could avoid that (and probably save some memory
> too) if the driver could provide us a copy of imp_dbh sans the com struct.
The memory saving would be tiny in the overall scheme of things.
> In retrospect I wanted to ask why DBI tells driver to define struct
> imp_xxh_st as:
>
> struct imp_dbh_st {
> dbih_dbc_t com; /* MUST be first element in structure */
> private element1;
> ...
> private elementN;
> }
>
> and not:
>
> typedef struct private_data imp_t;
> struct imp_dbh_st {
> dbih_dbc_t com; /* MUST be first element in structure */
> imp_t imp; /* MUST be the second element and the only one */
> }
>
> where imp_t could have any number of elements, while staying opaque to the
> DBI frontend.
Actually, it ought to have been:
struct imp_dbh_st {
dbih_dbc_t *com; /* POINTER to com MUST be first */
imp_t imp;
}
which would then make the drivers independant of the changes to the
size of the com structure.
It's on my list of things to do in the future. But I'd like to do
it as part of a general overhaul of the DBI/DBD interface and flag
the results a DBI 2.00. But I'm not sure when that'll happen.
Tim.
