From 4a15f87d22773ba208c441142ac53ddeb090d1b8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 14 Nov 2017 17:49:49 -0500 Subject: [PATCH] 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 --- config/c-compiler.m4 | 9 ++-- configure | 42 ++++++++++++++++++- configure.in | 7 +++- src/include/c.h | 26 +++++++++--- src/include/pg_config.h.in | 3 ++ src/include/pg_config.h.win32 | 3 ++ src/test/regress/expected/select_parallel.out | 24 +++++++++++ src/test/regress/sql/select_parallel.sql | 6 +++ 8 files changed, 108 insertions(+), 12 deletions(-) diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index a7f6773ae13..9d131ce028a 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -94,9 +94,11 @@ undefine([Ac_cachevar])dnl # PGAC_TYPE_128BIT_INT # --------------------- # Check if __int128 is a working 128 bit integer type, and if so -# define PG_INT128_TYPE to that typename. This currently only detects -# a GCC/clang extension, but support for different environments may be -# added in the future. +# define PG_INT128_TYPE to that typename, and define ALIGNOF_PG_INT128_TYPE +# as its alignment requirement. +# +# This currently only detects a GCC/clang extension, but support for other +# environments may be added in the future. # # For the moment we only test for support for 128bit math; support for # 128bit literals and snprintf is not required. @@ -126,6 +128,7 @@ return 1; [pgac_cv__128bit_int=no])]) if test x"$pgac_cv__128bit_int" = xyes ; then AC_DEFINE(PG_INT128_TYPE, __int128, [Define to the name of a signed 128-bit integer type.]) + AC_CHECK_ALIGNOF(PG_INT128_TYPE) fi])# PGAC_TYPE_128BIT_INT diff --git a/configure b/configure index f10dee827f6..0062ee263a4 100755 --- a/configure +++ b/configure @@ -14275,7 +14275,10 @@ _ACEOF # Compute maximum alignment of any basic type. # We assume long's alignment is at least as strong as char, short, or int; -# but we must check long long (if it exists) and double. +# but we must check long long (if it is being used for int64) and double. +# Note that we intentionally do not consider any types wider than 64 bits, +# as allowing MAXIMUM_ALIGNOF to exceed 8 would be too much of a penalty +# for disk and memory space. MAX_ALIGNOF=$ac_cv_alignof_long if test $MAX_ALIGNOF -lt $ac_cv_alignof_double ; then @@ -14335,7 +14338,7 @@ _ACEOF fi -# Check for extensions offering the integer scalar type __int128. +# Some compilers offer a 128-bit integer scalar type. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for __int128" >&5 $as_echo_n "checking for __int128... " >&6; } if ${pgac_cv__128bit_int+:} false; then : @@ -14385,6 +14388,41 @@ if test x"$pgac_cv__128bit_int" = xyes ; then $as_echo "#define PG_INT128_TYPE __int128" >>confdefs.h + # The cast to long int works around a bug in the HP C Compiler, +# see AC_CHECK_SIZEOF for more information. +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking alignment of PG_INT128_TYPE" >&5 +$as_echo_n "checking alignment of PG_INT128_TYPE... " >&6; } +if ${ac_cv_alignof_PG_INT128_TYPE+:} false; then : + $as_echo_n "(cached) " >&6 +else + if ac_fn_c_compute_int "$LINENO" "(long int) offsetof (ac__type_alignof_, y)" "ac_cv_alignof_PG_INT128_TYPE" "$ac_includes_default +#ifndef offsetof +# define offsetof(type, member) ((char *) &((type *) 0)->member - (char *) 0) +#endif +typedef struct { char x; PG_INT128_TYPE y; } ac__type_alignof_;"; then : + +else + if test "$ac_cv_type_PG_INT128_TYPE" = yes; then + { { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5 +$as_echo "$as_me: error: in \`$ac_pwd':" >&2;} +as_fn_error 77 "cannot compute alignment of PG_INT128_TYPE +See \`config.log' for more details" "$LINENO" 5; } + else + ac_cv_alignof_PG_INT128_TYPE=0 + fi +fi + +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_alignof_PG_INT128_TYPE" >&5 +$as_echo "$ac_cv_alignof_PG_INT128_TYPE" >&6; } + + + +cat >>confdefs.h <<_ACEOF +#define ALIGNOF_PG_INT128_TYPE $ac_cv_alignof_PG_INT128_TYPE +_ACEOF + + fi # Check for various atomic operations now that we have checked how to declare diff --git a/configure.in b/configure.in index 1ba41896f39..0b97a532a64 100644 --- a/configure.in +++ b/configure.in @@ -1848,7 +1848,10 @@ AC_CHECK_ALIGNOF(double) # Compute maximum alignment of any basic type. # We assume long's alignment is at least as strong as char, short, or int; -# but we must check long long (if it exists) and double. +# but we must check long long (if it is being used for int64) and double. +# Note that we intentionally do not consider any types wider than 64 bits, +# as allowing MAXIMUM_ALIGNOF to exceed 8 would be too much of a penalty +# for disk and memory space. MAX_ALIGNOF=$ac_cv_alignof_long if test $MAX_ALIGNOF -lt $ac_cv_alignof_double ; then @@ -1865,7 +1868,7 @@ AC_DEFINE_UNQUOTED(MAXIMUM_ALIGNOF, $MAX_ALIGNOF, [Define as the maximum alignme AC_CHECK_TYPES([int8, uint8, int64, uint64], [], [], [#include ]) -# Check for extensions offering the integer scalar type __int128. +# Some compilers offer a 128-bit integer scalar type. PGAC_TYPE_128BIT_INT # Check for various atomic operations now that we have checked how to declare diff --git a/src/include/c.h b/src/include/c.h index bf4043dddf7..3a569c80602 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -355,13 +355,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 1e0583f7b15..f7ac5658cc4 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 0c2403f1bf0..d7dcb85af4f 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 164c4bd61c9..72dd4204ace 100644 --- a/src/test/regress/expected/select_parallel.out +++ b/src/test/regress/expected/select_parallel.out @@ -99,6 +99,30 @@ explain (costs off) -> Index Only Scan using tenk1_unique1 on tenk1 (3 rows) +-- check parallelized int8 aggregate (bug #14897) +explain (costs off) +select avg(aa::int8) from a_star; + QUERY PLAN +----------------------------------------------------- + Finalize Aggregate + -> Gather + Workers Planned: 1 + -> Partial Aggregate + -> Append + -> Parallel Seq Scan on a_star + -> Parallel Seq Scan on b_star + -> Parallel Seq Scan on c_star + -> Parallel Seq Scan on d_star + -> Parallel Seq Scan on e_star + -> Parallel Seq Scan on f_star +(11 rows) + +select avg(aa::int8) from a_star; + avg +--------------------- + 13.6538461538461538 +(1 row) + -- test the sanity of parallel query after the active role is dropped. set force_parallel_mode=1; drop role if exists regress_parallel_worker; diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql index e310c096811..af0cc558104 100644 --- a/src/test/regress/sql/select_parallel.sql +++ b/src/test/regress/sql/select_parallel.sql @@ -39,6 +39,12 @@ explain (costs off) select sum(parallel_restricted(unique1)) from tenk1 group by(parallel_restricted(unique1)); +-- check parallelized int8 aggregate (bug #14897) +explain (costs off) +select avg(aa::int8) from a_star; + +select avg(aa::int8) from a_star; + -- test the sanity of parallel query after the active role is dropped. set force_parallel_mode=1; drop role if exists regress_parallel_worker;