You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Steven Fosdick <st...@gmail.com> on 2020/03/14 01:18:08 UTC
DBD, rows and memory pools
I have just been writing a small example program using APR-util's DBD
with Oracle as the database and have come across an interesting
puzzle. My first attempt used just one pool and didn't attempt to
clear it but then, when I tried to introduce sensible lifetimes, I
ended up with a program including this fragment:
apr_pool_t *spool;
st = apr_pool_create(&spool, pool);
if (st == APR_SUCCESS) {
apr_pool_t *rpool;
st = apr_pool_create(&rpool, spool);
if (st == APR_SUCCESS) {
apr_dbd_results_t *res;
st = apr_dbd_select(drv, spool, dbh, &res, "SELECT * FROM mytab", 0);
if (st == 0) {
int cols = apr_dbd_num_cols(drv, res);
apr_dbd_row_t *row;
while (!apr_dbd_get_row(drv, rpool, res, &row, -1)) {
for (int c = 0; c < cols; c++) {
const char *name = apr_dbd_get_name(drv, res, c);
const char *value = apr_dbd_get_entry(drv, row, c);
printf("%s=%s\n", name, value);
}
apr_pool_clear(rpool);
putchar('\n');
}
}
apr_pool_clear(spool);
}
}
apr_dbd_close(drv, dbh);
This core dumps. For fetching the first row, by luck, the variable
'row' was NULL so apr_dbd_get_row allocated it's internal row
structure from the pool rpool, initialised it and set row to point to
it. By the time the second row is being fetched and rpool has been
cleared apr_dbd_get_row finds the pointer to the apr_dbd_row_t it
allocated last time and tries to re-use it which works for the first
column but, after the call to apr_dbd_get_entry, this is then
overwritten and processing the 2nd column causes the core dump.
I can obviously resolve this by initialising row to NULL each time:
apr_pool_t *rpool;
st = apr_pool_create(&rpool, spool);
if (st == APR_SUCCESS) {
apr_dbd_results_t *res;
st = apr_dbd_select(drv, spool, dbh, &res, "SELECT * FROM hn_lookup_rt", 0);
if (st == 0) {
int cols = apr_dbd_num_cols(drv, res);
apr_dbd_row_t *row = NULL;
while (!apr_dbd_get_row(drv, rpool, res, &row, -1)) {
for (int c = 0; c < cols; c++) {
const char *name = apr_dbd_get_name(drv, res, c);
const char *value = apr_dbd_get_entry(drv, row, c);
printf("%s=%s\n", name, value);
}
putchar('\n');
apr_pool_clear(rpool);
row = NULL;
}
}
apr_pool_clear(spool);
}
but clearly using the pass by reference of 'row' and using it as a
in/out parameter and thus re-using the previously allocated
apr_dbd_row_t is intended as an optimisation to avoid re-doing that
allocation and initialisation for each row. It does however seem to
me to have the wrong lifetime. If I was to change this example so it
was:
apr_pool_t *spool;
st = apr_pool_create(&spool, pool);
if (st == APR_SUCCESS) {
apr_dbd_results_t *res;
st = apr_dbd_select(drv, spool, dbh, &res, "SELECT * FROM hn_lookup_rt", 0);
if (st == 0) {
int cols = apr_dbd_num_cols(drv, res);
apr_dbd_row_t *row = NULL;
while (!apr_dbd_get_row(drv, spool, res, &row, -1)) {
for (int c = 0; c < cols; c++) {
const char *name = apr_dbd_get_name(drv, res, c);
const char *value = apr_dbd_get_entry(drv, row, c);
printf("%s=%s\n", name, value);
}
putchar('\n');
}
}
apr_pool_clear(spool);
}
that would allow the allocated apr_dbd_row_t to be cached between rows
but this now has a different problem because apr_dbd_get_entry is
allocating the returned strings from a memory pool that now lasts for
the whole of the iteration over the result set. That's fine for a
small table but if iterating over millions of rows this would consume
a lot of memory.
It seems to me that if there is some data to be retained between
fetching one row and the next it should really be allocated from the
memory pool given to apr_dbd_select, not the one given to
apr_dbd_get_row. What do you think?
Re: DBD, rows and memory pools
Posted by Simon Walter <si...@gikaku.com>.
On 3/19/20 10:25 AM, Simon Walter wrote:
...
> Maybe it's gone after a year
> or so of updates. I am not sure anything has changed though. Although,
> as you explained about apr_dbd_get_row, maybe the apr_dbd_pselect was
> written in a similar way.
Steven, I can report that I cannot replicate that problem anymore and
have removed my workaround. I still have an old version of debian
installed where the problem occurs. Though it does not exist since Stretch.
Best regards,
Simon
Re: DBD, rows and memory pools
Posted by Simon Walter <si...@gikaku.com>.
On 3/15/20 6:31 AM, Nick Kew wrote:
...
>> Is there guidance or a standard for driver writers
>> elsewhere?
>
> No. Apart from our own needs, we just have occasional feedback
> from users like yourself.
>
> Just thinking aloud here, I wonder if an apr_dbd_row_alloc API
> would meet your needs? I haven't thought through how it
> would be implemented, and it also raises the question of whether
> allocator APIs for other apr_dbd structs might be in order.
>
I am very interested in this outcome as I have run into a similar
(perhaps it is the same) issue. I have chosen to live with my work
around until my code is more complete. I have a unit test called
test_storage_successive_read where I debugged this issue but did not dig
into it much, as I was not prepared to try and determine if there was a
bug and where it might lie at that time.
Here are the comments from my code:
// the above statement has no results. if we use the Statement var
again
// we will get no results when calling apr_dbd_get_row()
...
//there seems to be a bug when creating the statement and using it
//right away
//if we set it to NULL before hand, however, it is fine.
//if we do not set Statement = NULL; all subsequent statements will
have no
//results.
I then simply:
Statement = NULL;
before using the variable again.
I am calling apr_dbd_pselect with various statements one after the other
in order to find why in my program loop there was only the first result.
Setting the apr_dbd_prepared_t * to NULL seemed to fix the issue. It
seemed quite odd to me. When I saw Steven's email, I remembered about my
own similar issue. So I thought I should comment and offer to provide
some test code and help debug the issue.
I am using libaprutil1:amd64 1.5.4-3 on Debian if that makes a difference.
Best regards,
Simon
Re: DBD, rows and memory pools
Posted by Nick Kew <ni...@apache.org>.
On Sat, 14 Mar 2020 19:58:11 +0000
Steven Fosdick <st...@gmail.com> wrote:
> On Sat, 14 Mar 2020 at 02:34, Nick Kew <ni...@apache.org> wrote:
> > Isn't your most obvious fix simply to allocate your row
> > from your choice of pool before the first get_row?
>
> That wouldn't work. Here's the start of the dbd_oracle_get_row
> function in the apr_dbd_oracle driver:
OK, fairy nuff. Probably my fault. apr_dbd started life
as complementary to HTTPD's mod_dbd, and your millions-of-rows
usage wasn't really a consideration.
> Is there guidance or a standard for driver writers
> elsewhere?
No. Apart from our own needs, we just have occasional feedback
from users like yourself.
Just thinking aloud here, I wonder if an apr_dbd_row_alloc API
would meet your needs? I haven't thought through how it
would be implemented, and it also raises the question of whether
allocator APIs for other apr_dbd structs might be in order.
--
Nick Kew
Re: DBD, rows and memory pools
Posted by Steven Fosdick <st...@gmail.com>.
On Sat, 14 Mar 2020 at 02:34, Nick Kew <ni...@apache.org> wrote:
> Isn't your most obvious fix simply to allocate your row
> from your choice of pool before the first get_row?
That wouldn't work. Here's the start of the dbd_oracle_get_row
function in the apr_dbd_oracle driver:
static int dbd_oracle_get_row(apr_pool_t *pool, apr_dbd_results_t *res,
apr_dbd_row_t **rowp, int rownum)
{
apr_dbd_row_t *row = *rowp;
apr_dbd_t *sql = res->handle;
int_errorcode;
if (row == NULL) {
row = apr_palloc(pool, sizeof(apr_dbd_row_t));
*rowp = row;
row->res = res;
/* Oracle starts counting at 1 according to the docs */
row->n = res->seek ? rownum : 1;
row->pool = pool;
}
else {
if (res->seek) {
row->n = rownum;
}
else {
++row->n;
}
}
so as well as allocating a apr_dbd_row_t if one does not exist it is
also setting up some of the members. If I allocated it myself from a
different pool it would not be initialised. Compare, though, with the
equivalent section of the dbd_mysql_get_row function from the
apr_dbd_mysql driver:
if (ret == 0) {
if (!*row) {
*row = apr_palloc(pool, sizeof(apr_dbd_row_t));
}
(*row)->row = r;
(*row)->res = res;
(*row)->len = mysql_fetch_lengths(res->res);
}
else {
apr_pool_cleanup_run(res->pool, res->res, free_result);
}
return ret;
so with that in mind I thought I'd compare what apr_dbd_get_entry does
in each of those two drivers. In the Oracle driver the a sufficient
fragment is:
default:
buf = apr_pstrndup(row->pool, val->buf.sval, val->len);
break;
}
return (const char*) buf;
}
so the Oracle driver is returning a copy of the value, allocated from
a pool remember from the call to apr_dbd_get_row. The equivalent from
the MySQL driver:
if (row->res->statement) {
bind = &row->res->bind[n];
if (mysql_stmt_fetch_column(row->res->statement, bind, n, 0) != 0) {
return NULL;
}
if (*bind->is_null) {
return NULL;
}
else {
return bind->buffer;
}
}
else {
return row->row[n];
}
return NULL;
so this appears to be returning a pointer into something linked to the
apr_dbd_row_t which is presumably re-used for each row fetched as long
as the apr_dbd_row_t remains allocated. In fact this seems much
easier for the library user to control - want long lifetime values
from apr_dbd_get_entry then reset the apr_dbd_row_t pointer to NULL
each iteration and it will achieve that. Want to keep memory
consumption down then leave the pointer alone and the memory will be
re-used.
So maybe the real issue is that the two drivers have interpreted what
is required differently. There is nothing in the Doxygen
documentation for apr_dbd_get_entry about the lifetime of the value
returned. Is there guidance or a standard for driver writers
elsewhere?
Re: DBD, rows and memory pools
Posted by Nick Kew <ni...@apache.org>.
> On 14 Mar 2020, at 02:23, Nick Kew <ni...@apache.org> wrote:
>
> [chop]
Damn, shouldn't be posting while down with a lurgy.
Isn't your most obvious fix simply to allocate your row
from your choice of pool before the first get_row?
Or am I still missing something?
--
Nick Kew
Re: DBD, rows and memory pools
Posted by Nick Kew <ni...@apache.org>.
> On 14 Mar 2020, at 01:18, Steven Fosdick <st...@gmail.com> wrote:
> while (!apr_dbd_get_row(drv, rpool, res, &row, -1)) {
> for (int c = 0; c < cols; c++) {
> const char *name = apr_dbd_get_name(drv, res, c);
> const char *value = apr_dbd_get_entry(drv, row, c);
> printf("%s=%s\n", name, value);
> }
> apr_pool_clear(rpool);
> putchar('\n');
> }
Clearing the pool within a loop using it? Yes, that would segfault!
> I can obviously resolve this by initialising row to NULL each time:
Indeed. Or if you're dealing with millions of rows, you might consider
a hybrid approach: clear and reset to NULL every $count rows?
> It seems to me that if there is some data to be retained between
> fetching one row and the next it should really be allocated from the
> memory pool given to apr_dbd_select, not the one given to
> apr_dbd_get_row. What do you think?
Indeed, I wonder if you're the first to try that particular pool usage?
It's your choice of pools. Are you suggesting apr_dbd_get_row should
take two pool arguments, or that apr_dbd_results_t should remember
its pool and allocate rows out of it? Mightn't that itself become a nasty
gotcha for programmers, and risk blowing up existing programs?
--
Nick Kew