diff options
author | Tom Lane <tgl@sss.pgh.pa.us> | 2017-11-14 15:03:55 -0500 |
---|---|---|
committer | Tom Lane <tgl@sss.pgh.pa.us> | 2017-11-14 15:03:55 -0500 |
commit | 7518049980be1d90264addab003476ae105f70d4 (patch) | |
tree | 78f553ca09439d572a6c897dee4dd3bae7b4eea3 /src | |
parent | 91aec93e6089a5ba49cce0aca3bf7f7022d62ea4 (diff) | |
download | postgresql-7518049980be1d90264addab003476ae105f70d4.tar.gz postgresql-7518049980be1d90264addab003476ae105f70d4.zip |
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.
This will need to be back-patched, along with the preparatory commit
91aec93e6. But let's see what the buildfarm makes of it first.
Discussion: https://postgr.es/m/20171110185747.31519.28038@wrigleys.postgresql.org
Diffstat (limited to 'src')
-rw-r--r-- | src/include/c.h | 26 | ||||
-rw-r--r-- | src/include/pg_config.h.in | 3 | ||||
-rw-r--r-- | src/include/pg_config.h.win32 | 3 | ||||
-rw-r--r-- | src/test/regress/expected/select_parallel.out | 18 | ||||
-rw-r--r-- | src/test/regress/sql/select_parallel.sql | 6 |
5 files changed, 51 insertions, 5 deletions
diff --git a/src/include/c.h b/src/include/c.h index 331e6f8f93b..18809c93720 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -376,13 +376,29 @@ typedef unsigned long long int uint64; /* * 128-bit signed and unsigned integers - * There currently is only a limited support for the type. E.g. 128bit - * literals and snprintf are not supported; but math is. + * There currently is only limited support for such types. + * E.g. 128bit literals and snprintf are not supported; but math is. + * Also, because we exclude such types when choosing MAXIMUM_ALIGNOF, + * it must be possible to coerce the compiler to allocate them on no + * more than MAXALIGN boundaries. */ #if defined(PG_INT128_TYPE) -#define HAVE_INT128 -typedef PG_INT128_TYPE int128; -typedef unsigned PG_INT128_TYPE uint128; +#if defined(pg_attribute_aligned) || ALIGNOF_PG_INT128_TYPE <= MAXIMUM_ALIGNOF +#define HAVE_INT128 1 + +typedef PG_INT128_TYPE int128 +#if defined(pg_attribute_aligned) +pg_attribute_aligned(MAXIMUM_ALIGNOF) +#endif +; + +typedef unsigned PG_INT128_TYPE uint128 +#if defined(pg_attribute_aligned) +pg_attribute_aligned(MAXIMUM_ALIGNOF) +#endif +; + +#endif #endif /* diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index cfdcc5ac62f..84d59f12b23 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -27,6 +27,9 @@ /* The normal alignment of `long long int', in bytes. */ #undef ALIGNOF_LONG_LONG_INT +/* The normal alignment of `PG_INT128_TYPE', in bytes. */ +#undef ALIGNOF_PG_INT128_TYPE + /* The normal alignment of `short', in bytes. */ #undef ALIGNOF_SHORT diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32 index ab9b941e89d..e192d98c5a6 100644 --- a/src/include/pg_config.h.win32 +++ b/src/include/pg_config.h.win32 @@ -34,6 +34,9 @@ /* The alignment requirement of a `long long int'. */ #define ALIGNOF_LONG_LONG_INT 8 +/* The normal alignment of `PG_INT128_TYPE', in bytes. */ +#undef ALIGNOF_PG_INT128_TYPE + /* The alignment requirement of a `short'. */ #define ALIGNOF_SHORT 2 diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out index 6f04769e3ea..63ed6a33c15 100644 --- a/src/test/regress/expected/select_parallel.out +++ b/src/test/regress/expected/select_parallel.out @@ -444,6 +444,24 @@ select * from reset enable_material; reset enable_hashagg; +-- check parallelized int8 aggregate (bug #14897) +explain (costs off) +select avg(unique1::int8) from tenk1; + QUERY PLAN +------------------------------------------------------------------------- + Finalize Aggregate + -> Gather + Workers Planned: 4 + -> Partial Aggregate + -> Parallel Index Only Scan using tenk1_unique1 on tenk1 +(5 rows) + +select avg(unique1::int8) from tenk1; + avg +----------------------- + 4999.5000000000000000 +(1 row) + -- gather merge test with a LIMIT explain (costs off) select fivethous from tenk1 order by fivethous limit 4; diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql index 9c1b87abdfc..1bd2821083d 100644 --- a/src/test/regress/sql/select_parallel.sql +++ b/src/test/regress/sql/select_parallel.sql @@ -175,6 +175,12 @@ reset enable_material; reset enable_hashagg; +-- check parallelized int8 aggregate (bug #14897) +explain (costs off) +select avg(unique1::int8) from tenk1; + +select avg(unique1::int8) from tenk1; + -- gather merge test with a LIMIT explain (costs off) select fivethous from tenk1 order by fivethous limit 4; |