pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedCrash in BRIN summarization
6+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2020-01-20 4c87010981 fd436bba01 da7abcf0f4 ff0c567cbf e96f30d898 98f0d28377 Fix crash in BRIN inclusion op functions, due to missing datum copy.
* Crash in BRIN summarization
@ 2019-08-28 09:22 Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 3 replies; 6+ messages in thread
From: Heikki Linnakangas @ 2019-08-28 09:22 UTC (permalink / raw)
To: pgsql-hackers
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
Attachments:
fix-brin-inclusion-memory_handling-bug.patch (text/x-patch, 1.3K) download
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();
* Re: Crash in BRIN summarization
@ 2019-08-28 11:03 Emre Hasegeli <emre@hasegeli.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
2 siblings, 0 replies; 6+ messages in thread
From: Emre Hasegeli @ 2019-08-28 11:03 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers
Thank you for your fix.
> 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.
Your analysis looks right to me.
> brin_inclusion_union() has a similar issue, but I didn't write a script
> to reproduce that. Fix attached.
I am not sure about this part. If it's okay to use col_a->bv_values
without copying, it should also be okay to use col_b->bv_values, no?
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Crash in BRIN summarization
@ 2019-08-28 14:00 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
2 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2019-08-28 14:00 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> 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:
Good catch.
> Fix attached.
Hm, I don't particularly care for directly comparing Datum values
like that. We don't do that elsewhere (I think) and it feels like
a type violation to me. Since you've already tested for !attbyval,
I think it'd be legitimate to extract the pointer values and compare
those, eg
if (!attr->attbyval &&
DatumGetPointer(result) != DatumGetPointer(column->bv_values[INCLUSION_UNION]))
I realize that this makes no difference in terms of the machine
code (at least for most machines?) but it avoids breaking the
Datum abstraction.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Crash in BRIN summarization
@ 2020-01-17 21:35 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
2 siblings, 1 reply; 6+ messages in thread
From: Alvaro Herrera @ 2020-01-17 21:35 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers
On 2019-Aug-28, Heikki Linnakangas wrote:
> 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:
Heikki, I just noticed that you haven't pushed this bugfix. Would you
like me to? (If I don't hear from you, I'll probably get to it next
week.)
Thanks
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Crash in BRIN summarization
@ 2020-01-20 08:44 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Heikki Linnakangas @ 2020-01-20 08:44 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers; Emre Hasegeli <emre@hasegeli.com>
On 17/01/2020 23:35, Alvaro Herrera wrote:
> On 2019-Aug-28, Heikki Linnakangas wrote:
>
>> 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:
>
> Heikki, I just noticed that you haven't pushed this bugfix. Would you
> like me to? (If I don't hear from you, I'll probably get to it next
> week.)
Sorry, forgot all about it. Pushed now.
On 28/08/2019 14:03, Emre Hasegeli wrote:
>> brin_inclusion_union() has a similar issue, but I didn't write a script
>> to reproduce that. Fix attached.
>
> I am not sure about this part. If it's okay to use col_a->bv_values
> without copying, it should also be okay to use col_b->bv_values, no?
No, the 'a' and 'b' arguments are not symmetric. 'a' is in a long-lived
memory context, and the function modifies it in place, whereas 'b' is
short-lived and isn't touched by the function.
- Heikki
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Crash in BRIN summarization
@ 2020-01-20 12:14 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2020-01-20 12:14 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-hackers; Emre Hasegeli <emre@hasegeli.com>
On 2020-Jan-20, Heikki Linnakangas wrote:
> Sorry, forgot all about it. Pushed now.
Thank you!
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2020-01-20 12:14 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-08-28 09:22 Crash in BRIN summarization Heikki Linnakangas <hlinnaka@iki.fi>
2019-08-28 11:03 ` Emre Hasegeli <emre@hasegeli.com>
2019-08-28 14:00 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-01-17 21:35 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-01-20 08:44 ` Heikki Linnakangas <hlinnaka@iki.fi>
2020-01-20 12:14 ` Alvaro Herrera <alvherre@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox