From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
To: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Ensuring hash tuples are properly maxaligned
Date: Tue, 02 Jan 2018 19:08:49 -0500
Message-ID: <14483.1514938129@sss.pgh.pa.us> (raw)
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
Attachments:ensure-proper-hash-tuple-alignment.patch (text/x-diff, 4.0K) downloaddiff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.cindex 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.hindex 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)
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, thomas.munro@enterprisedb.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Ensuring hash tuples are properly maxaligned
In-Reply-To: <14483.1514938129@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox