Ensuring hash tuples are properly maxaligned

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

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.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 = MAXALIGN(size);

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

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

      /*
!      * If the data[] field of HashMemoryChunkData has a non-maxaligned offset,
!      * we'll return non-maxaligned allocations, which is bad.
!      */
!     StaticAssertStmt(HASH_CHUNK_HEADER_SIZE == MAXALIGN(HASH_CHUNK_HEADER_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 = (HashMemoryChunk) MemoryContextAlloc(hashtable->batchCxt,
!                                                         HASH_CHUNK_HEADER_SIZE + size);
          newChunk->maxlen = size;
!         newChunk->used = size;
!         newChunk->ntuples = 1;

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

-         newChunk->used += size;
-         newChunk->ntuples += 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 = (HashMemoryChunk) MemoryContextAlloc(hashtable->batchCxt,
!                                                         offsetof(HashMemoryChunkData, data) + HASH_CHUNK_SIZE);

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

          newChunk->maxlen = HASH_CHUNK_SIZE;
          newChunk->used = size;
diff --git a/src/include/executor/hashjoin.h b/src/include/executor/hashjoin.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 padding
+      * 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 static
+      * 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) != 0
+     char        padding[MAXIMUM_ALIGNOF - HASHMEMORYCHUNKDATA_RAWSIZE % MAXIMUM_ALIGNOF];
+ #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)

  /*