[PATCH] Fix crash in int8_avg_combine().
While doing some tests on REL_10_STABLE, I was getting run-time exceptions
at int8_avg_combine() at the following line:
state1->sumX = state2->sumX;
After some debugging, I noticed that palloc()’s alignment is 8-bytes, while
this statement (which moves a __int128 from one memory location to another
memory location) expects 16-byte memory alignments. So when either state1
or state2 is not 16-byte aligned, this crashes.
When I disassemble the code, the above statement is translated to a pair of
movdqa and movaps assignments when compiled with -O2:
movdqa c(%rdx), %xmm0
movaps %xmm0, c(%rcx)
Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual,
Volume 2”, both of these instructions expect 16-byte aligned memory
locations, or a general-protection exception will be generated.
I wasn’t getting the exception always. For example, the same query that
crashed in REL_10_STABLE didn’t crash in master. But I think that was just
lucky allocation, and we will eventually see cases of this crash in all
versions that use __int128 assignment in int8_avg_combine().
I’ve attached a patch which sets the MAXIMUM_ALIGNOF correctly when
__int128’s are available, so fixes the crash. This patch is on top of
REL_10_STABLE, which is the branch I was getting the crash at. I can create
patches for other branches if we think this is the proper change.
The sequence for which I got the crash in REL_10_STABLE was the following
sequence of commands after a fresh initdb:
CREATE TABLE t(a BIGINT);
INSERT INTO t SELECT * FROM generate_series(1, 10000000);
SELECT sum(a) FROM t;
I’m not sure if this will crash for everyone, since you can be lucky and
have both states assigned 16-byte aligned locations. This was the case for
me in master branch. "SELECT version()" is "PostgreSQL 10.0 on
x86_64-pc-linux-gnu, compiled by gcc (GCC) 7.2.1 20170915 (Red Hat
7.2.1-2), 64-bit", if that is related.
I notice that there’s a comment in configure.in that says the penalty of
using 16-bit alignment might be too much for disk and memory space. If this
is the case, then we need to modify numeric.c to make sure that it never
use any instructions with 16-byte alignment requirements. I can do that if
that is the consensus.
Or maybe we can create an allocation function for 16-byte aligned
allocations.
Any thoughts?
-- Hadi
Attachments:
fix_alignment.REL10_0_STABLE.patchtext/x-patch; charset=US-ASCII; name=fix_alignment.REL10_0_STABLE.patchDownload+110-59
Hi Hadi,
On 2017-11-25 22:43:49 -0500, Hadi Moshayedi wrote:
While doing some tests on REL_10_STABLE, I was getting run-time exceptions
at int8_avg_combine() at the following line:state1->sumX = state2->sumX;
After some debugging, I noticed that palloc()’s alignment is 8-bytes, while
this statement (which moves a __int128 from one memory location to another
memory location) expects 16-byte memory alignments. So when either state1
or state2 is not 16-byte aligned, this crashes.When I disassemble the code, the above statement is translated to a pair of
movdqa and movaps assignments when compiled with -O2:movdqa c(%rdx), %xmm0
movaps %xmm0, c(%rcx)Looking at “Intel 64 and IA-32 Architectures Software Developer’s Manual,
Volume 2”, both of these instructions expect 16-byte aligned memory
locations, or a general-protection exception will be generated.
Nicely analyzed. [Un]fortunately the bug has already been found and
fixed:
https://git.postgresql.org/pg/commitdiff/619a8c47da7279c186bb57cc16b26ad011366b73
Will be included in the next set of back branch releases.
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h index 869c59dc85..2dc59e89cd 100644 --- a/src/include/utils/memutils.h +++ b/src/include/utils/memutils.h @@ -189,7 +189,7 @@ extern MemoryContext SlabContextCreate(MemoryContext parent, * Few callers should be interested in this, but tuplesort/tuplestore need * to know it. */ -#define ALLOCSET_SEPARATE_THRESHOLD 8192 +#define ALLOCSET_SEPARATE_THRESHOLD 16384
Huh, what's that about in this context?
Greetings,
Andres Freund
Hadi Moshayedi <hadi@moshayedi.net> writes:
While doing some tests on REL_10_STABLE, I was getting run-time exceptions
at int8_avg_combine() at the following line:
state1->sumX = state2->sumX;
After some debugging, I noticed that palloc()’s alignment is 8-bytes, while
this statement (which moves a __int128 from one memory location to another
memory location) expects 16-byte memory alignments. So when either state1
or state2 is not 16-byte aligned, this crashes.
I believe we already dealt with this:
Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: REL_10_STABLE [619a8c47d] 2017-11-14 17:49:49 -0500
Branch: REL9_6_STABLE [4a15f87d2] 2017-11-14 17:49:49 -0500
Branch: REL9_5_STABLE [d4e38489f] 2017-11-14 17:49:49 -0500
Prevent int128 from requiring more than MAXALIGN alignment.
Our initial work with int128 neglected alignment considerations, an
oversight that came back to bite us in bug #14897 from Vincent Lachenal.
It is unsurprising that int128 might have a 16-byte alignment requirement;
what's slightly more surprising is that even notoriously lax Intel chips
sometimes enforce that.
Raising MAXALIGN seems out of the question: the costs in wasted disk and
memory space would be significant, and there would also be an on-disk
compatibility break. Nor does it seem very practical to try to allow some
data structures to have more-than-MAXALIGN alignment requirement, as we'd
have to push knowledge of that throughout various code that copies data
structures around.
The only way out of the box is to make type int128 conform to the system's
alignment assumptions. Fortunately, gcc supports that via its
__attribute__(aligned()) pragma; and since we don't currently support
int128 on non-gcc-workalike compilers, we shouldn't be losing any platform
support this way.
Although we could have just done pg_attribute_aligned(MAXIMUM_ALIGNOF) and
called it a day, I did a little bit of extra work to make the code more
portable than that: it will also support int128 on compilers without
__attribute__(aligned()), if the native alignment of their 128-bit-int
type is no more than that of int64.
Add a regression test case that exercises the one known instance of the
problem, in parallel aggregation over a bigint column.
Back-patch of commit 751804998. The code known to be affected only exists
in 9.6 and later, but we do have some stuff using int128 in 9.5, so patch
back to 9.5.
Discussion: /messages/by-id/20171110185747.31519.28038@wrigleys.postgresql.org
Does that solution not work for you?
regards, tom lane
On Sat, Nov 25, 2017 at 10:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I believe we already dealt with this:
Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: REL_10_STABLE [619a8c47d] 2017-11-14 17:49:49 -0500
Branch: REL9_6_STABLE [4a15f87d2] 2017-11-14 17:49:49 -0500
Branch: REL9_5_STABLE [d4e38489f] 2017-11-14 17:49:49 -0500Prevent int128 from requiring more than MAXALIGN alignment.
Our initial work with int128 neglected alignment considerations, an
oversight that came back to bite us in bug #14897 from Vincent
Lachenal.
It is unsurprising that int128 might have a 16-byte alignment
requirement;
what's slightly more surprising is that even notoriously lax Intel
chips
sometimes enforce that.Raising MAXALIGN seems out of the question: the costs in wasted disk
and
memory space would be significant, and there would also be an on-disk
compatibility break. Nor does it seem very practical to try to allow
some
data structures to have more-than-MAXALIGN alignment requirement, as
we'd
have to push knowledge of that throughout various code that copies data
structures around.The only way out of the box is to make type int128 conform to the
system's
alignment assumptions. Fortunately, gcc supports that via its
__attribute__(aligned()) pragma; and since we don't currently support
int128 on non-gcc-workalike compilers, we shouldn't be losing any
platform
support this way.Although we could have just done pg_attribute_aligned(MAXIMUM_ALIGNOF)
and
called it a day, I did a little bit of extra work to make the code more
portable than that: it will also support int128 on compilers without
__attribute__(aligned()), if the native alignment of their 128-bit-int
type is no more than that of int64.Add a regression test case that exercises the one known instance of the
problem, in parallel aggregation over a bigint column.Back-patch of commit 751804998. The code known to be affected only
exists
in 9.6 and later, but we do have some stuff using int128 in 9.5, so
patch
back to 9.5.Discussion: /messages/by-id/20171110185747.31519.28038@
wrigleys.postgresql.orgDoes that solution not work for you?
It works for me. My REL_10_STABLE was out of date. A git pull fixed it.
On Sat, Nov 25, 2017 at 10:47 PM, Andres Freund <andres@citusdata.com>
wrote:
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.hindex 869c59dc85..2dc59e89cd 100644 --- a/src/include/utils/memutils.h +++ b/src/include/utils/memutils.h @@ -189,7 +189,7 @@ extern MemoryContext SlabContextCreate(MemoryContextparent,
* Few callers should be interested in this, but tuplesort/tuplestore
need
* to know it. */ -#define ALLOCSET_SEPARATE_THRESHOLD 8192 +#define ALLOCSET_SEPARATE_THRESHOLD 16384Huh, what's that about in this context?
There is following static assert:
StaticAssertStmt(ALLOC_CHUNK_LIMIT == ALLOCSET_SEPARATE_THRESHOLD, ...)
and ALLOCK_CHUNK_LIMIT is defined as:
#define ALLOC_CHUNK_LIMIT (1 << (ALLOCSET_NUM_FREELISTS-1+ALLOC_MINBITS))
and there is this comment:
"CAUTION: ALLOC_MINBITS must be large enough so that 1<<ALLOC_MINBITS is at
least MAXALIGN, ..."
So I increased ALLOC_MINBITS which resulted in doubling
ALLOCSET_SEPARATE_THRESHOLD.
Thanks,
-- Hadi