8000 Prevent int128 from requiring more than MAXALIGN alignment. · koderP/postgres@4a15f87 · GitHub
[go: up one dir, main page]
Skip to content

Commit 4a15f87

Browse files
tglsfdctglsfdc
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 6c35b3a commit 4a15f87

File tree

8 files changed

+108
-12
lines changed

8 files changed

+108
-12
lines changed

‎config/c-compiler.m4

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

131134

‎configure

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

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

1428014283
MAX_ALIGNOF=$ac_cv_alignof_long
1428114284
if test $MAX_ALIGNOF -lt $ac_cv_alignof_double ; then
@@ -14335,7 +14338,7 @@ _ACEOF
1433514338
fi
1433614339

1433714340

14338-
# Check for extensions offering the integer scalar type __int128.
14341+
# Some compilers offer a 128-bit integer scalar type.
1433914342
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for __int128" >&5
1434014343
$as_echo_n "checking for __int128... " >&6; }
1434114344
if ${pgac_cv__128bit_int+:} false; then :
@@ -14385,6 +14388,41 @@ if test x"$pgac_cv__128bit_int" = xyes ; then
1438514388

1438614389
$as_echo "#define PG_INT128_TYPE __int128" >>confdefs.h
1438714390

14391+
# The cast to long int works around a bug in the HP C Compiler,
14392+
# see AC_CHECK_SIZEOF for more information.
14393+
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking alignment of PG_INT128_TYPE" >&5
14394+
$as_echo_n "checking alignment of PG_INT128_TYPE... " >&6; }
14395+
if ${ac_cv_alignof_PG_INT128_TYPE+:} false; then :
14396+
$as_echo_n "(cached) " >&6
14397+
else
14398+
if ac_fn_c_compute_int "$LINENO" "(long int) offsetof (ac__type_alignof_, y)" "ac_cv_alignof_PG_INT128_TYPE" "$ac_includes_default
14399+
#ifndef offsetof
14400+
# define offsetof(type, member) ((char *) &((type *) 0)->member - (char *) 0)
14401+
#endif
14402+
typedef struct { char x; PG_INT128_TYPE y; } ac__type_alignof_;"; then :
14403+
14404+
else
14405+
if test "$ac_cv_type_PG_INT128_TYPE" = yes; then
14406+
{ { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
14407+
$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
14408+
as_fn_error 77 "cannot compute alignment of PG_INT128_TYPE
14409+
See \`config.log' for more details" "$LINENO" 5; }
14410+
else
14411+
ac_cv_alignof_PG_INT128_TYPE=0
14412+
fi
14413+
fi
14414+
14415+
fi
14416+
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_alignof_PG_INT128_TYPE" >&5
14417+
$as_echo "$ac_cv_alignof_PG_INT128_TYPE" >&6; }
14418+
14419+
14420+
14421+
cat >>confdefs.h <<_ACEOF
14422+
#define ALIGNOF_PG_INT128_TYPE $ac_cv_alignof_PG_INT128_TYPE
14423+
_ACEOF
14424+
14425+
1438814426
fi
1438914427

1439014428
# 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
@@ -1848,7 +1848,10 @@ AC_CHECK_ALIGNOF(double)
18481848

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

18531856
MAX_ALIGNOF=$ac_cv_alignof_long
18541857
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
18651868
AC_CHECK_TYPES([int8, uint8, int64, uint64], [], [],
18661869
[#include <stdio.h>])
18671870

1868-
# Check for extensions offering the integer scalar type __int128.
1871+
# Some compilers offer a 128-bit integer scalar type.
18691872
PGAC_TYPE_128BIT_INT
18701873

18711874
# 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
@@ -355,13 +355,29 @@ typedef unsigned long long int uint64;
355355

356356
/*
357357
* 128-bit signed and unsigned integers
358-
* There currently is only a limited support for the type. E.g. 128bit
359-
* literals and snprintf are not supported; but math is.
358+
* There currently is only limited support for such types.
359+
* E.g. 128bit literals and snprintf are not supported; but math is.
360+
* Also, because we exclude such types when choosing MAXIMUM_ALIGNOF,
361+
* it must be possible to coerce the compiler to allocate them on no
362+
* more than MAXALIGN boundaries.
360363
*/
361364
#if defined(PG_INT128_TYPE)
362-
#define HAVE_INT128
363-
typedef PG_INT128_TYPE int128;
364-
typedef unsigned PG_INT128_TYPE uint128;
365+
#if defined(pg_attribute_aligned) || ALIGNOF_PG_INT128_TYPE <= MAXIMUM_ALIGNOF
366+
#define HAVE_INT128 1
367+
368+
typedef PG_INT128_TYPE int128
369+
#if defined(pg_attribute_aligned)
370+
pg_attribute_aligned(MAXIMUM_ALIGNOF)
371+
#endif
372+
;
373+
374+
typedef unsigned PG_INT128_TYPE uint128
375+
#if defined(pg_attribute_aligned)
376+
pg_attribute_aligned(MAXIMUM_ALIGNOF)
377+
#endif
378+
;
379+
380+
#endif
365381
#endif
366382

367383
/*

‎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: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,30 @@ explain (costs off)
9999
-> Index Only Scan using tenk1_unique1 on tenk1
100100
(3 rows)
101101

102+
-- check parallelized int8 aggregate (bug #14897)
103+
explain (costs off)
104+
select avg(aa::int8) from a_star;
105+
QUERY PLAN
106+
-----------------------------------------------------
107+
Finalize Aggregate
108+
-> Gather
109+
Workers Planned: 1
110+
-> Partial Aggregate
111+
-> Append
112+
-> Parallel Seq Scan on a_star
113+
-> Parallel Seq Scan on b_star
114+
-> Parallel Seq Scan on c_star
115+
-> Parallel Seq Scan on d_star
116+
-> Parallel Seq Scan on e_star
117+
D70A -> Parallel Seq Scan on f_star
118+
(11 rows)
119+
120+
select avg(aa::int8) from a_star;
121+
avg
122+
---------------------
123+
13.6538461538461538
124+
(1 row)
125+
102126
-- test the sanity of parallel query after the active role is dropped.
103127
set force_parallel_mode=1;
104128
drop role if exists regress_parallel_worker;

‎src/test/regress/sql/select_parallel.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ explain (costs off)
3939
select sum(parallel_restricted(unique1)) from tenk1
4040
group by(parallel_restricted(unique1));
4141

42+
-- check parallelized int8 aggregate (bug #14897)
43+
explain (costs off)
44+
select avg(aa::int8) from a_star;
45+
46+
select avg(aa::int8) from a_star;
47+
4248
-- test the sanity of parallel query after the active role is dropped.
4349
set force_parallel_mode=1;
4450
drop role if exists regress_parallel_worker;

0 commit comments

Comments
 (0)
0