Reorder subskiplsn in pg_subscription to avoid alignment issues.

The column 'subskiplsn' uses TYPALIGN_DOUBLE (which has 4 bytes alignment
on AIX) for storage. But the C Struct (Form_pg_subscription) has 8-byte
alignment for this field, so retrieving it from storage causes an
unaligned read.

To fix this, we rearranged the 'subskiplsn' column in the catalog so that
it naturally comes at an 8-byte boundary.

We have fixed a similar problem in commit f3b421da5f. This patch adds a
test to avoid a similar mistake in the future.

Reported-by: Noah Misch
Diagnosed-by: Noah Misch, Masahiko Sawada, Amit Kapila
Author: Masahiko Sawada
Reviewed-by: Noah Misch, Amit Kapila
Discussion: https://postgr.es/m/20220401074423.GC3682158@rfd.leadboat.com
	    https://postgr.es/m/CAD21AoDeScrsHhLyEPYqN3sydg6PxAPVBboK=30xJfUVihNZDA@mail.gmail.com

5 files changed, 109 insertions, 0 deletions

diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/expected/sanity_check.out
index 8370c1561cc..a2faefb4c02 100644
--- a/src/test/regress/expected/sanity_check.out
+++ b/src/test/regress/expected/sanity_check.out
@@ -25,3 +25,32 @@ SELECT relname, relkind
 ---------+---------
 (0 rows)
 
+--
+-- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on
+-- some of the C types that correspond to TYPALIGN_DOUBLE SQL types.  To ensure
+-- catalog C struct layout matches catalog tuple layout, arrange for the tuple
+-- offset of each fixed-width, attalign='d' catalog column to be divisible by 8
+-- unconditionally.  Keep such columns before the first NameData column of the
+-- catalog, since packagers can override NAMEDATALEN to an odd number.
+--
+WITH check_columns AS (
+ SELECT relname, attname,
+  array(
+   SELECT t.oid
+    FROM pg_type t JOIN pg_attribute pa ON t.oid = pa.atttypid
+    WHERE pa.attrelid = a.attrelid AND
+          pa.attnum > 0 AND pa.attnum <= a.attnum
+    ORDER BY pa.attnum) AS coltypes
+ FROM pg_attribute a JOIN pg_class c ON c.oid = attrelid
+  JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE attalign = 'd' AND relkind = 'r' AND
+  attnotnull AND attlen <> -1 AND n.nspname = 'pg_catalog'
+)
+SELECT relname, attname, coltypes, get_column_offset(coltypes)
+ FROM check_columns
+ WHERE get_column_offset(coltypes) % 8 != 0 OR
+       'name'::regtype::oid = ANY(coltypes);
+ relname | attname | coltypes | get_column_offset 
+---------+---------+----------+-------------------
+(0 rows)
+
diff --git a/src/test/regress/expected/test_setup.out b/src/test/regress/expected/test_setup.out
index a9d0de3deac..8b8ba7d778b 100644
--- a/src/test/regress/expected/test_setup.out
+++ b/src/test/regress/expected/test_setup.out
@@ -206,6 +206,10 @@ CREATE FUNCTION ttdummy ()
     RETURNS trigger
     AS :'regresslib'
     LANGUAGE C;
+CREATE FUNCTION get_column_offset (oid[])
+    RETURNS int
+    AS :'regresslib'
+    LANGUAGE C STRICT STABLE PARALLEL SAFE;
 -- Use hand-rolled hash functions and operator classes to get predictable
 -- result on different machines.  The hash function for int4 simply returns
 -- the sum of the values passed to it and the one for text returns the length
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 0802fb9136a..8b0c2d9d684 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -41,6 +41,7 @@
 #include "storage/spin.h"
 #include "utils/builtins.h"
 #include "utils/geo_decls.h"
+#include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
 #include "utils/typcache.h"
@@ -1216,3 +1217,47 @@ binary_coercible(PG_FUNCTION_ARGS)
 
 	PG_RETURN_BOOL(IsBinaryCoercible(srctype, targettype));
 }
+
+/*
+ * Return the column offset of the last data in the given array of
+ * data types.  The input data types must be fixed-length data types.
+ */
+PG_FUNCTION_INFO_V1(get_column_offset);
+Datum
+get_column_offset(PG_FUNCTION_ARGS)
+{
+	ArrayType	*ta = PG_GETARG_ARRAYTYPE_P(0);
+	Oid			*type_oids;
+	int			ntypes;
+	int			column_offset = 0;
+
+	if (ARR_HASNULL(ta) && array_contains_nulls(ta))
+		elog(ERROR, "argument must not contain nulls");
+
+	if (ARR_NDIM(ta) > 1)
+		elog(ERROR, "argument must be empty or one-dimensional array");
+
+	type_oids = (Oid *) ARR_DATA_PTR(ta);
+	ntypes = ArrayGetNItems(ARR_NDIM(ta), ARR_DIMS(ta));
+	for (int i = 0; i < ntypes; i++)
+	{
+		Oid typeoid = type_oids[i];
+		int16		typlen;
+		bool		typbyval;
+		char		typalign;
+
+		get_typlenbyvalalign(typeoid, &typlen, &typbyval, &typalign);
+
+		/* the data type must be fixed-length */
+		if (!(typbyval || (typlen > 0)))
+			elog(ERROR, "type %u is not fixed-length data type", typeoid);
+
+		column_offset = att_align_nominal(column_offset, typalign);
+
+		/* not include the last type size */
+		if (i != (ntypes - 1))
+			column_offset += typlen;
+	}
+
+	PG_RETURN_INT32(column_offset);
+}
diff --git a/src/test/regress/sql/sanity_check.sql b/src/test/regress/sql/sanity_check.sql
index 162e5324b5d..c70ff781faf 100644
--- a/src/test/regress/sql/sanity_check.sql
+++ b/src/test/regress/sql/sanity_check.sql
@@ -19,3 +19,29 @@ SELECT relname, relkind
   FROM pg_class
  WHERE relkind IN ('v', 'c', 'f', 'p', 'I')
        AND relfilenode <> 0;
+
+--
+-- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on
+-- some of the C types that correspond to TYPALIGN_DOUBLE SQL types.  To ensure
+-- catalog C struct layout matches catalog tuple layout, arrange for the tuple
+-- offset of each fixed-width, attalign='d' catalog column to be divisible by 8
+-- unconditionally.  Keep such columns before the first NameData column of the
+-- catalog, since packagers can override NAMEDATALEN to an odd number.
+--
+WITH check_columns AS (
+ SELECT relname, attname,
+  array(
+   SELECT t.oid
+    FROM pg_type t JOIN pg_attribute pa ON t.oid = pa.atttypid
+    WHERE pa.attrelid = a.attrelid AND
+          pa.attnum > 0 AND pa.attnum <= a.attnum
+    ORDER BY pa.attnum) AS coltypes
+ FROM pg_attribute a JOIN pg_class c ON c.oid = attrelid
+  JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE attalign = 'd' AND relkind = 'r' AND
+  attnotnull AND attlen <> -1 AND n.nspname = 'pg_catalog'
+)
+SELECT relname, attname, coltypes, get_column_offset(coltypes)
+ FROM check_columns
+ WHERE get_column_offset(coltypes) % 8 != 0 OR
+       'name'::regtype::oid = ANY(coltypes);
diff --git a/src/test/regress/sql/test_setup.sql b/src/test/regress/sql/test_setup.sql
index 1f3f2f17241..fbceb8cb464 100644
--- a/src/test/regress/sql/test_setup.sql
+++ b/src/test/regress/sql/test_setup.sql
@@ -253,6 +253,11 @@ CREATE FUNCTION ttdummy ()
     AS :'regresslib'
     LANGUAGE C;
 
+CREATE FUNCTION get_column_offset (oid[])
+    RETURNS int
+    AS :'regresslib'
+    LANGUAGE C STRICT STABLE PARALLEL SAFE;
+
 -- Use hand-rolled hash functions and operator classes to get predictable
 -- result on different machines.  The hash function for int4 simply returns
 -- the sum of the values passed to it and the one for text returns the length