I bumped into a little bug in BRIN, while hacking on something
unrelated. This causes a segfault, or an assertion failure if assertions
are enabled:
CREATE TABLE brintest (n numrange);
CREATE INDEX brinidx ON brintest USING brin (n);
INSERT INTO brintest VALUES ('empty');
INSERT INTO brintest VALUES (numrange(0, 2^1000::numeric));
INSERT INTO brintest VALUES ('(-1, 0)');
SELECT brin_desummarize_range('brinidx',0);
SELECT brin_summarize_range('brinidx',0) ;
gdb backtrace:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x0000560fef71f4b2 in GetMemoryChunkContext (pointer=0x7fed65a8fc30)
at ../../../../src/include/utils/memutils.h:129
129 AssertArg(MemoryContextIsValid(context));
(gdb) bt
#0 0x0000560fef71f4b2 in GetMemoryChunkContext (pointer=0x7fed65a8fc30)
at ../../../../src/include/utils/memutils.h:129
#1 0x0000560fef721290 in pfree (pointer=0x7fed65a8fc30) at mcxt.c:1033
#2 0x0000560fef0bd13f in brin_inclusion_add_value
(fcinfo=0x7ffc4cd36220) at brin_inclusion.c:239
#3 0x0000560fef6ee543 in FunctionCall4Coll (flinfo=0x560ff05d0cf0,
collation=0, arg1=94626457109880, arg2=94626457868896,
arg3=140657589550088, arg4=0) at fmgr.c:1214
#4 0x0000560fef0b39b7 in brinbuildCallback (index=0x7fed64f37460,
htup=0x560ff0685d68, values=0x7ffc4cd365d0, isnull=0x7ffc4cd365b0,
tupleIsAlive=true, brstate=0x560ff06859e0)
at brin.c:650
#5 0x0000560fef12e418 in heapam_index_build_range_scan
(heapRelation=0x7fed64f37248, indexRelation=0x7fed64f37460,
indexInfo=0x560ff0685b48, allow_sync=false, anyvisible=true,
progress=false, start_blockno=0, numblocks=1,
callback=0x560fef0b37c9 <brinbuildCallback>,
callback_state=0x560ff06859e0, scan=0x560ff0685d18) at heapam_handler.c:1663
#6 0x0000560fef0b2858 in table_index_build_range_scan
(table_rel=0x7fed64f37248, index_rel=0x7fed64f37460,
index_info=0x560ff0685b48, allow_sync=false, anyvisible=true,
progress=false,
start_blockno=0, numblocks=1, callback=0x560fef0b37c9
<brinbuildCallback>, callback_state=0x560ff06859e0, scan=0x0) at
../../../../src/include/access/tableam.h:1544
#7 0x0000560fef0b4dac in summarize_range (indexInfo=0x560ff0685b48,
state=0x560ff06859e0, heapRel=0x7fed64f37248, heapBlk=0, heapNumBlks=1)
at brin.c:1240
#8 0x0000560fef0b50d9 in brinsummarize (index=0x7fed64f37460,
heapRel=0x7fed64f37248, pageRange=0, include_partial=true,
numSummarized=0x7ffc4cd36928, numExisting=0x0) at brin.c:1375
#9 0x0000560fef0b43bf in brin_summarize_range (fcinfo=0x560ff06840d0)
at brin.c:933
#10 0x0000560fef339acc in ExecInterpExpr (state=0x560ff0683fe8,
econtext=0x560ff0683ce8, isnull=0x7ffc4cd36c17) at execExprInterp.c:650
Analysis:
This is a memory management issue in the brin_inclusion_add_value()
function:
/* Finally, merge the new value to the existing union. */
finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
Assert(finfo != NULL);
result = FunctionCall2Coll(finfo, colloid,
column->bv_values[INCLUSION_UNION], newval);
if (!attr->attbyval)
pfree(DatumGetPointer(column->bv_values[INCLUSION_UNION]));
column->bv_values[INCLUSION_UNION] = result;
This assumes that the merge function returns a newly-palloc'd value.
That's a shaky assumption; if one of the arguments is an empty range,
range_merge() returns the other argument, rather than a newly
constructed value. And surely we can't assume assume that for
user-defined opclasses.
When that happens, we store the 'newval' directly in
column->bv_values[INCLUSION_UNION], but it is allocated in a
shortly-lived memory context (or points directly to a buffer in the
buffer cache). On the next call, we try to pfree() the old value, but
the memory context that contained it was already reset.
It took a while to work out the script to reproduce it. The first value
passed to brin_inclusion_add_value() must be an empty range, so that
column->bv_values[INCLUSION_UNION] is set to an empty range. On the
second call, the new value must be non-empty, so that range_merge()
returns 'newval' unchanged. And it must be a very large value, because
if it uses a short varlen header, the PG_GETARG_RANGE_P() call in
range_merge() will make a copy of it.
brin_inclusion_union() has a similar issue, but I didn't write a script
to reproduce that. Fix attached.
- Heikki
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 86788024ef..cbe14cb3b1 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -235,8 +235,13 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
Assert(finfo != NULL);
result = FunctionCall2Coll(finfo, colloid,
column->bv_values[INCLUSION_UNION], newval);
- if (!attr->attbyval)
+ if (!attr->attbyval && result != column->bv_values[INCLUSION_UNION])
+ {
pfree(DatumGetPointer(column->bv_values[INCLUSION_UNION]));
+
+ if (result == newval)
+ result = datumCopy(result, attr->attbyval, attr->attlen);
+ }
column->bv_values[INCLUSION_UNION] = result;
PG_RETURN_BOOL(true);
@@ -574,8 +579,13 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
result = FunctionCall2Coll(finfo, colloid,
col_a->bv_values[INCLUSION_UNION],
col_b->bv_values[INCLUSION_UNION]);
- if (!attr->attbyval)
+ if (!attr->attbyval && result != col_a->bv_values[INCLUSION_UNION])
+ {
pfree(DatumGetPointer(col_a->bv_values[INCLUSION_UNION]));
+
+ if (result == col_b->bv_values[INCLUSION_UNION])
+ result = datumCopy(result, attr->attbyval, attr->attlen);
+ }
col_a->bv_values[INCLUSION_UNION] = result;
PG_RETURN_VOID();
