From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>,
        Thomas Munro <thomas.munro@enterprisedb.com>
cc: pgsql-hackers@lists.postgresql.org
Subject: Ensuring hash tuples are properly maxaligned
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <14439.1514938059.0@sss.pgh.pa.us>
Date: Tue, 02 Jan 2018 19:08:49 -0500
Message-ID: <14483.1514938129@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <14439.1514938059.1@sss.pgh.pa.us>

I've been poking around in the PHJ code trying to identify the reason
why there are still so many buildfarm failures.  I've not nailed it
down yet, but one thing I did notice is that there's an entirely
undocumented assumption that offsetof(HashMemoryChunkData, data) is
maxalign'ed.  If it isn't, the allocation code will give back non-
maxaligned pointers, which will appear to work as long as you only
test on alignment-lax processors.

Now, the existing definition of the struct seems safe on all
architectures we support, but it would not take much to break it.
I think we ought to do what we did recently in the memory-context
code: insert an explicit padding calculation and a static assertion
that it's right.  Hence, the attached proposed patch (in which
I also failed to resist the temptation to make the two code paths
in dense_alloc() look more alike).  Any objections?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="ensure-proper-hash-tuple-alignment.patch";
	charset="us-ascii"
Content-ID: <14439.1514938059.2@sss.pgh.pa.us>
Content-Description: ensure-proper-hash-tuple-alignment.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHa=
sh.c
index 4e1a280..6aad152 100644
*** a/src/backend/executor/nodeHash.c
--- b/src/backend/executor/nodeHash.c
*************** dense_alloc(HashJoinTable hashtable, Siz
*** 2651,2667 ****
  	size =3D MAXALIGN(size);
  =

  	/*
! 	 * If tuple size is larger than of 1/4 of chunk size, allocate a separa=
te
! 	 * chunk.
  	 */
  	if (size > HASH_CHUNK_THRESHOLD)
  	{
  		/* allocate new chunk and put it at the beginning of the list */
  		newChunk =3D (HashMemoryChunk) MemoryContextAlloc(hashtable->batchCxt,
! 														offsetof(HashMemoryChunkData, data) + size);
  		newChunk->maxlen =3D size;
! 		newChunk->used =3D 0;
! 		newChunk->ntuples =3D 0;
  =

  		/*
  		 * Add this chunk to the list after the first existing chunk, so that
--- 2651,2673 ----
  	size =3D MAXALIGN(size);
  =

  	/*
! 	 * If the data[] field of HashMemoryChunkData has a non-maxaligned offs=
et,
! 	 * we'll return non-maxaligned allocations, which is bad.
! 	 */
! 	StaticAssertStmt(HASH_CHUNK_HEADER_SIZE =3D=3D MAXALIGN(HASH_CHUNK_HEAD=
ER_SIZE),
! 					 "sizeof(HashMemoryChunkData) is not maxaligned");
! =

! 	/*
! 	 * If tuple size is larger than threshold, allocate a separate chunk.
  	 */
  	if (size > HASH_CHUNK_THRESHOLD)
  	{
  		/* allocate new chunk and put it at the beginning of the list */
  		newChunk =3D (HashMemoryChunk) MemoryContextAlloc(hashtable->batchCxt,
! 														HASH_CHUNK_HEADER_SIZE + size);
  		newChunk->maxlen =3D size;
! 		newChunk->used =3D size;
! 		newChunk->ntuples =3D 1;
  =

  		/*
  		 * Add this chunk to the list after the first existing chunk, so that
*************** dense_alloc(HashJoinTable hashtable, Siz
*** 2678,2686 ****
  			hashtable->chunks =3D newChunk;
  		}
  =

- 		newChunk->used +=3D size;
- 		newChunk->ntuples +=3D 1;
- =

  		return newChunk->data;
  	}
  =

--- 2684,2689 ----
*************** dense_alloc(HashJoinTable hashtable, Siz
*** 2693,2699 ****
  	{
  		/* allocate new chunk and put it at the beginning of the list */
  		newChunk =3D (HashMemoryChunk) MemoryContextAlloc(hashtable->batchCxt,
! 														offsetof(HashMemoryChunkData, data) + HASH_CHUNK_SIZE);
  =

  		newChunk->maxlen =3D HASH_CHUNK_SIZE;
  		newChunk->used =3D size;
--- 2696,2702 ----
  	{
  		/* allocate new chunk and put it at the beginning of the list */
  		newChunk =3D (HashMemoryChunk) MemoryContextAlloc(hashtable->batchCxt,
! 														HASH_CHUNK_HEADER_SIZE + HASH_CHUNK_SIZE);
  =

  		newChunk->maxlen =3D HASH_CHUNK_SIZE;
  		newChunk->used =3D size;
diff --git a/src/include/executor/hashjoin.h b/src/include/executor/hashjo=
in.h
index d8c82d4..cb91f79 100644
*** a/src/include/executor/hashjoin.h
--- b/src/include/executor/hashjoin.h
*************** typedef struct HashMemoryChunkData
*** 127,139 ****
  		dsa_pointer shared;
  	}			next;
  =

  	char		data[FLEXIBLE_ARRAY_MEMBER];	/* buffer allocated at the end */
  }			HashMemoryChunkData;
  =

  typedef struct HashMemoryChunkData *HashMemoryChunk;
  =

  #define HASH_CHUNK_SIZE			(32 * 1024L)
! #define HASH_CHUNK_HEADER_SIZE (offsetof(HashMemoryChunkData, data))
  #define HASH_CHUNK_THRESHOLD	(HASH_CHUNK_SIZE / 4)
  =

  /*
--- 127,152 ----
  		dsa_pointer shared;
  	}			next;
  =

+ 	/*
+ 	 * It's required that data[] start at a maxaligned offset.  This paddin=
g
+ 	 * calculation is correct as long as int is not wider than size_t and
+ 	 * dsa_pointer is not wider than a regular pointer, but there's a stati=
c
+ 	 * assertion to check things in nodeHash.c.
+ 	 */
+ #define HASHMEMORYCHUNKDATA_RAWSIZE (SIZEOF_SIZE_T * 3 + SIZEOF_VOID_P)
+ =

+ 	/* ensure proper alignment by adding padding if needed */
+ #if (HASHMEMORYCHUNKDATA_RAWSIZE % MAXIMUM_ALIGNOF) !=3D 0
+ 	char		padding[MAXIMUM_ALIGNOF - HASHMEMORYCHUNKDATA_RAWSIZE % MAXIMUM_A=
LIGNOF];
+ #endif
+ =

  	char		data[FLEXIBLE_ARRAY_MEMBER];	/* buffer allocated at the end */
  }			HashMemoryChunkData;
  =

  typedef struct HashMemoryChunkData *HashMemoryChunk;
  =

  #define HASH_CHUNK_SIZE			(32 * 1024L)
! #define HASH_CHUNK_HEADER_SIZE	offsetof(HashMemoryChunkData, data)
  #define HASH_CHUNK_THRESHOLD	(HASH_CHUNK_SIZE / 4)
  =

  /*

------- =_aaaaaaaaaa0--