On 29.09.2013 23:32, Nicholas White wrote:
bms_add_member() is an accident waiting to happen
I've attached a patch that makes it use repalloc as suggested
You'll have to zero out the extended portion.
I tried to demonstrate that by setting RANDOMIZE_ALLOCATED_MEMORY, but
surprisingly regression tests still passed. I guess the regression suite
doesn't use wide enough bitmapsets to exercise that. But this causes an
assertion failure, with RANDOMIZE_ALLOCATED_MEMORY:
create table t (i int4);
select * from t as t1, t as t2, t as t3, t as t4, t as t5, t as t6, t as
t7, t as t8, t as t9, t as t10, t as t11, t as t12, t as t13, t as t14,
t as t15, t as t16, t as t17, t as t18, t as t19, t as t20, t as t21, t
as t22, t as t23, t as t24, t as t25, t as t26, t as t27, t as t28, t as
t29, t as t30, t as t31, t as t32, t as t33, t as t34, t as t35, t as
t36, t as t37, t as t38, t as t39, t as t40;
- is it OK to commit separately? I'll address the lead-lag patch
comments in the next couple of days. Thanks
Yep, thanks. I committed the attached.
After thinking about this some more, I realized that bms_add_member() is
still sensitive to CurrentMemoryContext, if the 'a' argument is NULL.
That's probably OK for the lag&lead patch - I didn't check - but if
we're going to start relying on the fact that bms_add_member() no longer
allocates a new bms in CurrentMemoryContext, that needs to be
documented. bitmapset.c currently doesn't say a word about memory contexts.
So what needs to be done next is to document how the functions in
bitmapset.c work wrt. memory contexts. Then double-check that the
behavior of all the other "recycling" bms functions is consistent. (At
least bms_add_members() needs a similar change).
- Heikki
>From ee01d848f39400c8524c66944ada6fde47894978 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 30 Sep 2013 16:37:00 +0300
Subject: [PATCH 1/1] In bms_add_member(), use repalloc() if the bms needs to
be enlarged.
Previously bms_add_member() would palloc a whole-new copy of the existing
set, copy the words, and pfree the old one. repalloc() is potentially much
faster, and more importantly, this is less surprising if CurrentMemoryContext
is not the same as the context the old set is in. bms_add_member() still
allocates a new bitmapset in CurrentMemoryContext if NULL is passed as
argument, but that is a lot less likely to induce bugs.
Nicholas White.
---
src/backend/nodes/bitmapset.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index b18b7a5..540db16 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -632,21 +632,20 @@ bms_add_member(Bitmapset *a, int x)
return bms_make_singleton(x);
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
+
+ /* enlarge the set if necessary */
if (wordnum >= a->nwords)
{
- /* Slow path: make a larger set and union the input set into it */
- Bitmapset *result;
- int nwords;
+ int oldnwords = a->nwords;
int i;
- result = bms_make_singleton(x);
- nwords = a->nwords;
- for (i = 0; i < nwords; i++)
- result->words[i] |= a->words[i];
- pfree(a);
- return result;
+ a = (Bitmapset *) repalloc(a, BITMAPSET_SIZE(wordnum + 1));
+ a->nwords = wordnum + 1;
+ /* zero out the enlarged portion */
+ for (i = oldnwords; i < a->nwords; i++)
+ a->words[i] = 0;
}
- /* Fast path: x fits in existing set */
+
a->words[wordnum] |= ((bitmapword) 1 << bitnum);
return a;
}
--
1.8.4.rc3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
