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)
/*
