Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 619a8c4

Browse files
committed
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 7518049. 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
1 parent a891050 commit 619a8c4

File tree

8 files changed

+102
-12
lines changed

8 files changed

+102
-12
lines changed

config/c-compiler.m4

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,11 @@ undefine([Ac_cachevar])dnl
9696
# PGAC_TYPE_128BIT_INT
9797
# ---------------------
9898
# Check if __int128 is a working 128 bit integer type, and if so
99-
# define PG_INT128_TYPE to that typename. This currently only detects
100-
# a GCC/clang extension, but support for different environments may be
101-
# added in the future.
99+
# define PG_INT128_TYPE to that typename, and define ALIGNOF_PG_INT128_TYPE
100+
# as its alignment requirement.
101+
#
102+
# This currently only detects a GCC/clang extension, but support for other
103+
# environments may be added in the future.
102104
#
103105
# For the moment we only test for support for 128bit math; support for
104106
# 128bit literals and snprintf is not required.
@@ -128,6 +130,7 @@ return 1;
128130
[pgac_cv__128bit_int=no])])
129131
if test x"$pgac_cv__128bit_int" = xyes ; then
130132
AC_DEFINE(PG_INT128_TYPE, __int128, [Define to the name of a signed 128-bit integer type.])
133+
AC_CHECK_ALIGNOF(PG_INT128_TYPE)
131134
fi])# PGAC_TYPE_128BIT_INT
132135

133136

configure

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14806,7 +14806,10 @@ _ACEOF
1480614806

1480714807
# Compute maximum alignment of any basic type.
1480814808
# We assume long's alignment is at least as strong as char, short, or int;
14809-
# but we must check long long (if it exists) and double.
14809+
# but we must check long long (if it is being used for int64) and double.
14810+
# Note that we intentionally do not consider any types wider than 64 bits,
14811+
# as allowing MAXIMUM_ALIGNOF to exceed 8 would be too much of a penalty
14812+
# for disk and memory space.
1481014813

1481114814
MAX_ALIGNOF=$ac_cv_alignof_long
1481214815
if test $MAX_ALIGNOF -lt $ac_cv_alignof_double ; then
@@ -14866,7 +14869,7 @@ _ACEOF
1486614869
fi
1486714870

1486814871

14869-
# Check for extensions offering the integer scalar type __int128.
14872+
# Some compilers offer a 128-bit integer scalar type.
1487014873
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for __int128" >&5
1487114874
$as_echo_n "checking for __int128... " >&6; }
1487214875
if ${pgac_cv__128bit_int+:} false; then :
@@ -14916,6 +14919,41 @@ if test x"$pgac_cv__128bit_int" = xyes ; then
1491614919

1491714920
$as_echo "#define PG_INT128_TYPE __int128" >>confdefs.h
1491814921

14922+
# The cast to long int works around a bug in the HP C Compiler,
14923+
# see AC_CHECK_SIZEOF for more information.
14924+
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking alignment of PG_INT128_TYPE" >&5
14925+
$as_echo_n "checking alignment of PG_INT128_TYPE... " >&6; }
14926+
if ${ac_cv_alignof_PG_INT128_TYPE+:} false; then :
14927+
$as_echo_n "(cached) " >&6
14928+
else
14929+
if ac_fn_c_compute_int "$LINENO" "(long int) offsetof (ac__type_alignof_, y)" "ac_cv_alignof_PG_INT128_TYPE" "$ac_includes_default
14930+
#ifndef offsetof
14931+
# define offsetof(type, member) ((char *) &((type *) 0)->member - (char *) 0)
14932+
#endif
14933+
typedef struct { char x; PG_INT128_TYPE y; } ac__type_alignof_;"; then :
14934+
14935+
else
14936+
if test "$ac_cv_type_PG_INT128_TYPE" = yes; then
14937+
{ { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
14938+
$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
14939+
as_fn_error 77 "cannot compute alignment of PG_INT128_TYPE
14940+
See \`config.log' for more details" "$LINENO" 5; }
14941+
else
14942+
ac_cv_alignof_PG_INT128_TYPE=0
14943+
fi
14944+
fi
14945+
14946+
fi
14947+
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_alignof_PG_INT128_TYPE" >&5
14948+
$as_echo "$ac_cv_alignof_PG_INT128_TYPE" >&6; }
14949+
14950+
14951+
14952+
cat >>confdefs.h <<_ACEOF
14953+
#define ALIGNOF_PG_INT128_TYPE $ac_cv_alignof_PG_INT128_TYPE
14954+
_ACEOF
14955+
14956+
1491914957
fi
1492014958

1492114959
# Check for various atomic operations now that we have checked how to declare

configure.in

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1832,7 +1832,10 @@ AC_CHECK_ALIGNOF(double)
18321832

18331833
# Compute maximum alignment of any basic type.
18341834
# We assume long's alignment is at least as strong as char, short, or int;
1835-
# but we must check long long (if it exists) and double.
1835+
# but we must check long long (if it is being used for int64) and double.
1836+
# Note that we intentionally do not consider any types wider than 64 bits,
1837+
# as allowing MAXIMUM_ALIGNOF to exceed 8 would be too much of a penalty
1838+
# for disk and memory space.
18361839

18371840
MAX_ALIGNOF=$ac_cv_alignof_long
18381841
if test $MAX_ALIGNOF -lt $ac_cv_alignof_double ; then
@@ -1849,7 +1852,7 @@ AC_DEFINE_UNQUOTED(MAXIMUM_ALIGNOF, $MAX_ALIGNOF, [Define as the maximum alignme
18491852
AC_CHECK_TYPES([int8, uint8, int64, uint64], [], [],
18501853
[#include <stdio.h>])
18511854

1852-
# Check for extensions offering the integer scalar type __int128.
1855+
# Some compilers offer a 128-bit integer scalar type.
18531856
PGAC_TYPE_128BIT_INT
18541857

18551858
# Check for various atomic operations now that we have checked how to declare

src/include/c.h

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -370,13 +370,29 @@ typedef unsigned long long int uint64;
370370

371371
/*
372372
* 128-bit signed and unsigned integers
373-
* There currently is only a limited support for the type. E.g. 128bit
374-
* literals and snprintf are not supported; but math is.
373+
* There currently is only limited support for such types.
374+
* E.g. 128bit literals and snprintf are not supported; but math is.
375+
* Also, because we exclude such types when choosing MAXIMUM_ALIGNOF,
376+
* it must be possible to coerce the compiler to allocate them on no
377+
* more than MAXALIGN boundaries.
375378
*/
376379
#if defined(PG_INT128_TYPE)
377-
#define HAVE_INT128
378-
typedef PG_INT128_TYPE int128;
379-
typedef unsigned PG_INT128_TYPE uint128;
380+
#if defined(pg_attribute_aligned) || ALIGNOF_PG_INT128_TYPE <= MAXIMUM_ALIGNOF
381+
#define HAVE_INT128 1
382+
383+
typedef PG_INT128_TYPE int128
384+
#if defined(pg_attribute_aligned)
385+
pg_attribute_aligned(MAXIMUM_ALIGNOF)
386+
#endif
387+
;
388+
389+
typedef unsigned PG_INT128_TYPE uint128
390+
#if defined(pg_attribute_aligned)
391+
pg_attribute_aligned(MAXIMUM_ALIGNOF)
392+
#endif
393+
;
394+
395+
#endif
380396
#endif
381397

382398
/*

src/include/pg_config.h.in

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@
2727
/* The normal alignment of `long long int', in bytes. */
2828
#undef ALIGNOF_LONG_LONG_INT
2929

30+
/* The normal alignment of `PG_INT128_TYPE', in bytes. */
31+
#undef ALIGNOF_PG_INT128_TYPE
32+
3033
/* The normal alignment of `short', in bytes. */
3134
#undef ALIGNOF_SHORT
3235

src/include/pg_config.h.win32

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@
3434
/* The alignment requirement of a `long long int'. */
3535
#define ALIGNOF_LONG_LONG_INT 8
3636

37+
/* The normal alignment of `PG_INT128_TYPE', in bytes. */
38+
#undef ALIGNOF_PG_INT128_TYPE
39+
3740
/* The alignment requirement of a `short'. */
3841
#define ALIGNOF_SHORT 2
3942

src/test/regress/expected/select_parallel.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,24 @@ select string4 from tenk1 order by string4 limit 5;
444444

445445
reset max_parallel_workers;
446446
reset enable_hashagg;
447+
-- check parallelized int8 aggregate (bug #14897)
448+
explain (costs off)
449+
select avg(unique1::int8) from tenk1;
450+
QUERY PLAN
451+
-------------------------------------------------------------------------
452+
Finalize Aggregate
453+
-> Gather
454+
Workers Planned: 4
455+
-> Partial Aggregate
456+
-> Parallel Index Only Scan using tenk1_unique1 on tenk1
457+
(5 rows)
458+
459+
select avg(unique1::int8) from tenk1;
460+
avg
461+
-----------------------
462+
4999.5000000000000000
463+
(1 row)
464+
447465
-- test the sanity of parallel query after the active role is dropped.
448466
drop role if exists regress_parallel_worker;
449467
NOTICE: role "regress_parallel_worker" does not exist, skipping

src/test/regress/sql/select_parallel.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,12 @@ select string4 from tenk1 order by string4 limit 5;
168168
reset max_parallel_workers;
169169
reset enable_hashagg;
170170

171+
-- check parallelized int8 aggregate (bug #14897)
172+
explain (costs off)
173+
select avg(unique1::int8) from tenk1;
174+
175+
select avg(unique1::int8) from tenk1;
176+
171177
-- test the sanity of parallel query after the active role is dropped.
172178
drop role if exists regress_parallel_worker;
173179
create role regress_parallel_worker;

0 commit comments

Comments
 (0)