Re: [PATCH] Fix crash in int8_avg_combine().

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Hadi Moshayedi <hadi(at)moshayedi(dot)net>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)citusdata(dot)com>, Ozgun Erdogan <ozgun(at)citusdata(dot)com>, Sumedh Pathak <sumedh(at)citusdata(dot)com>
Subject: Re: [PATCH] Fix crash in int8_avg_combine().
Date: 2017-11-26 03:55:00
Message-ID: 15003.1511668500@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hadi Moshayedi <hadi(at)moshayedi(dot)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(at)sss(dot)pgh(dot)pa(dot)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: https://postgr.es/m/20171110185747.31519.28038@wrigleys.postgresql.org

Does that solution not work for you?

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message 高增琦 2017-11-26 06:18:16 Re: no library dependency in Makefile?
Previous Message Andres Freund 2017-11-26 03:47:08 Re: [PATCH] Fix crash in int8_avg_combine().