Skip to content

Commit 44cac93

Browse files
tglsfdctglsfdc
committed
Avoid using potentially-under-aligned page buffers.
There's a project policy against using plain "char buf[BLCKSZ]" local or static variables as page buffers; preferred style is to palloc or malloc each buffer to ensure it is MAXALIGN'd. However, that policy's been ignored in an increasing number of places. We've apparently got away with it so far, probably because (a) relatively few people use platforms on which misalignment causes core dumps and/or (b) the variables chance to be sufficiently aligned anyway. But this is not something to rely on. Moreover, even if we don't get a core dump, we might be paying a lot of cycles for misaligned accesses. To fix, invent new union types PGAlignedBlock and PGAlignedXLogBlock that the compiler must allocate with sufficient alignment, and use those in place of plain char arrays. I used these types even for variables where there's no risk of a misaligned access, since ensuring proper alignment should make kernel data transfers faster. I also changed some places where we had been palloc'ing short-lived buffers, for coding style uniformity and to save palloc/pfree overhead. Since this seems to be a live portability hazard (despite the lack of field reports), back-patch to all supported versions. Patch by me; thanks to Michael Paquier for review. Discussion: https://postgr.es/m/1535618100.1286.3.camel@credativ.de
1 parent 5e8d670 commit 44cac93

File tree

24 files changed

+141
-168
lines changed

24 files changed

+141
-168
lines changed

‎contrib/bloom/blinsert.c

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ typedef struct
3636
int64 indtuples; /* total number of tuples indexed */
3737
MemoryContext tmpCtx; /* temporary memory context reset after each
3838
* tuple */
39-
char data[BLCKSZ]; /* cached page */
39+
PGAlignedBlock data; /* cached page */
4040
int count; /* number of tuples in cached page */
4141
} BloomBuildState;
4242

@@ -52,7 +52,7 @@ flushCachedPage(Relation index, BloomBuildState *buildstate)
5252

5353
state = GenericXLogStart(index);
5454
page = GenericXLogRegisterBuffer(state, buffer, GENERIC_XLOG_FULL_IMAGE);
55-
memcpy(page, buildstate->data, BLCKSZ);
55+
memcpy(page, buildstate->data.data, BLCKSZ);
5656
GenericXLogFinish(state);
5757
UnlockReleaseBuffer(buffer);
5858
}
@@ -63,8 +63,8 @@ flushCachedPage(Relation index, BloomBuildState *buildstate)
6363
static void
6464
initCachedPage(BloomBuildState *buildstate)
6565
{
66-
memset(buildstate->data, 0, BLCKSZ);
67-
BloomInitPage(buildstate->data, 0);
66+
memset(buildstate->data.data, 0, BLCKSZ);
67+
BloomInitPage(buildstate->data.data, 0);
6868
buildstate->count = 0;
6969
}
7070

@@ -84,7 +84,7 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
8484
itup = BloomFormTuple(&buildstate->blstate, &htup->t_self, values, isnull);
8585

8686
/* Try to add next item to cached page */
87-
if (BloomPageAddItem(&buildstate->blstate, buildstate->data, itup))
87+
if (BloomPageAddItem(&buildstate->blstate, buildstate->data.data, itup))
8888
{
8989
/* Next item was added successfully */
9090
buildstate->count++;
@@ -98,7 +98,7 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
9898

9999
initCachedPage(buildstate);
100100

101-
if (!BloomPageAddItem(&buildstate->blstate, buildstate->data, itup))
101+
if (!BloomPageAddItem(&buildstate->blstate, buildstate->data.data, itup))
102102
{
103103
/* We shouldn't be here since we're inserting to the empty page */
104104
elog(ERROR, "could not add new bloom tuple to empty page");

‎contrib/pg_prewarm/pg_prewarm.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ typedef enum
3636
PREWARM_BUFFER
3737
} PrewarmType;
3838

39-
static char blockbuffer[BLCKSZ];
39+
static PGAlignedBlock blockbuffer;
4040

4141
/*
4242
* pg_prewarm(regclass, mode text, fork text,
@@ -178,7 +178,7 @@ pg_prewarm(PG_FUNCTION_ARGS)
178178
for (block = first_block; block <= last_block; ++block)
179179
{
180180
CHECK_FOR_INTERRUPTS();
181-
smgrread(rel->rd_smgr, forkNumber, block, blockbuffer);
181+
smgrread(rel->rd_smgr, forkNumber, block, blockbuffer.data);
182182
++blocks_done;
183183
}
184184
}

‎contrib/pg_standby/pg_standby.c

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -401,24 +401,21 @@ SetWALSegSize(void)
401401
{
402402
bool ret_val = false;
403403
int fd;
404-
405-
/* malloc this buffer to ensure sufficient alignment: */
406-
char *buf = (char *) pg_malloc(XLOG_BLCKSZ);
404+
PGAlignedXLogBlock buf;
407405

408406
Assert(WalSegSz == -1);
409407

410408
if ((fd = open(WALFilePath, O_RDWR, 0)) < 0)
411409
{
412410
fprintf(stderr, "%s: could not open WAL file \"%s\": %s\n",
413411
progname, WALFilePath, strerror(errno));
414-
pg_free(buf);
415412
return false;
416413
}
417414

418415
errno = 0;
419-
if (read(fd, buf, XLOG_BLCKSZ) == XLOG_BLCKSZ)
416+
if (read(fd, buf.data, XLOG_BLCKSZ) == XLOG_BLCKSZ)
420417
{
421-
XLogLongPageHeader longhdr = (XLogLongPageHeader) buf;
418+
XLogLongPageHeader longhdr = (XLogLongPageHeader) buf.data;
422419

423420
WalSegSz = longhdr->xlp_seg_size;
424421

@@ -455,7 +452,6 @@ SetWALSegSize(void)
455452
fflush(stderr);
456453

457454
close(fd);
458-
pg_free(buf);
459455
return ret_val;
460456
}
461457

‎src/backend/access/gin/ginentrypage.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -616,7 +616,7 @@ entrySplitPage(GinBtree btree, Buffer origbuf,
616616
Page lpage = PageGetTempPageCopy(BufferGetPage(origbuf));
617617
Page rpage = PageGetTempPageCopy(BufferGetPage(origbuf));
618618
Size pageSize = PageGetPageSize(lpage);
619-
char tupstore[2 * BLCKSZ];
619+
PGAlignedBlock tupstore[2]; /* could need 2 pages' worth of tuples */
620620

621621
entryPreparePage(btree, lpage, off, insertData, updateblkno);
622622

@@ -625,7 +625,7 @@ entrySplitPage(GinBtree btree, Buffer origbuf,
625625
* one after another in a temporary workspace.
626626
*/
627627
maxoff = PageGetMaxOffsetNumber(lpage);
628-
ptr = tupstore;
628+
ptr = tupstore[0].data;
629629
for (i = FirstOffsetNumber; i <= maxoff; i++)
630630
{
631631
if (i == off)
@@ -658,7 +658,7 @@ entrySplitPage(GinBtree btree, Buffer origbuf,
658658
GinInitPage(rpage, GinPageGetOpaque(lpage)->flags, pageSize);
659659
GinInitPage(lpage, GinPageGetOpaque(rpage)->flags, pageSize);
660660

661-
ptr = tupstore;
661+
ptr = tupstore[0].data;
662662
maxoff++;
663663
lsize = 0;
664664

‎src/backend/access/gin/ginfast.c

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,18 +64,15 @@ writeListPage(Relation index, Buffer buffer,
6464
size = 0;
6565
OffsetNumber l,
6666
off;
67-
char *workspace;
67+
PGAlignedBlock workspace;
6868
char *ptr;
6969

70-
/* workspace could be a local array; we use palloc for alignment */
71-
workspace = palloc(BLCKSZ);
72-
7370
START_CRIT_SECTION();
7471

7572
GinInitBuffer(buffer, GIN_LIST);
7673

7774
off = FirstOffsetNumber;
78-
ptr = workspace;
75+
ptr = workspace.data;
7976

8077
for (i = 0; i < ntuples; i++)
8178
{
@@ -127,7 +124,7 @@ writeListPage(Relation index, Buffer buffer,
127124
XLogRegisterData((char *) &data, sizeof(ginxlogInsertListPage));
128125

129126
XLogRegisterBuffer(0, buffer, REGBUF_WILL_INIT);
130-
XLogRegisterBufData(0, workspace, size);
127+
XLogRegisterBufData(0, workspace.data, size);
131128

132129
recptr = XLogInsert(RM_GIN_ID, XLOG_GIN_INSERT_LISTPAGE);
133130
PageSetLSN(page, recptr);
@@ -140,8 +137,6 @@ writeListPage(Relation index, Buffer buffer,
140137

141138
END_CRIT_SECTION();
142139

143-
pfree(workspace);
144-
145140
return freesize;
146141
}
147142

‎src/backend/access/hash/hashpage.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,7 +1000,7 @@ static bool
10001000
_hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
10011001
{
10021002
BlockNumber lastblock;
1003-
char zerobuf[BLCKSZ];
1003+
PGAlignedBlock zerobuf;
10041004
Page page;
10051005
HashPageOpaque ovflopaque;
10061006

@@ -1013,7 +1013,7 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
10131013
if (lastblock < firstblock || lastblock == InvalidBlockNumber)
10141014
return false;
10151015

1016-
page = (Page) zerobuf;
1016+
page = (Page) zerobuf.data;
10171017

10181018
/*
10191019
* Initialize the page. Just zeroing the page won't work; see
@@ -1034,11 +1034,11 @@ _hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
10341034
log_newpage(&rel->rd_node,
10351035
MAIN_FORKNUM,
10361036
lastblock,
1037-
zerobuf,
1037+
zerobuf.data,
10381038
true);
10391039

10401040
RelationOpenSmgr(rel);
1041-
smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf, false);
1041+
smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf.data, false);
10421042

10431043
return true;
10441044
}

‎src/backend/access/heap/heapam.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2709,7 +2709,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
27092709
HeapTuple *heaptuples;
27102710
int i;
27112711
int ndone;
2712-
char *scratch = NULL;
2712+
PGAlignedBlock scratch;
27132713
Page page;
27142714
bool needwal;
27152715
Size saveFreeSpace;
@@ -2726,14 +2726,6 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
27262726
heaptuples[i] = heap_prepare_insert(relation, tuples[i],
27272727
xid, cid, options);
27282728

2729-
/*
2730-
* Allocate some memory to use for constructing the WAL record. Using
2731-
* palloc() within a critical section is not safe, so we allocate this
2732-
* beforehand.
2733-
*/
2734-
if (needwal)
2735-
scratch = palloc(BLCKSZ);
2736-
27372729
/*
27382730
* We're about to do the actual inserts -- but check for conflict first,
27392731
* to minimize the possibility of having to roll back work we've just
@@ -2826,7 +2818,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
28262818
uint8 info = XLOG_HEAP2_MULTI_INSERT;
28272819
char *tupledata;
28282820
int totaldatalen;
2829-
char *scratchptr = scratch;
2821+
char *scratchptr = scratch.data;
28302822
bool init;
28312823
int bufflags = 0;
28322824

@@ -2885,7 +2877,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
28852877
scratchptr += datalen;
28862878
}
28872879
totaldatalen = scratchptr - tupledata;
2888-
Assert((scratchptr - scratch) < BLCKSZ);
2880+
Assert((scratchptr - scratch.data) < BLCKSZ);
28892881

28902882
if (need_tuple_data)
28912883
xlrec->flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
@@ -2912,7 +2904,7 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
29122904
bufflags |= REGBUF_KEEP_DATA;
29132905

29142906
XLogBeginInsert();
2915-
XLogRegisterData((char *) xlrec, tupledata - scratch);
2907+
XLogRegisterData((char *) xlrec, tupledata - scratch.data);
29162908
XLogRegisterBuffer(0, buffer, REGBUF_STANDARD | bufflags);
29172909

29182910
XLogRegisterBufData(0, tupledata, totaldatalen);

‎src/backend/access/heap/visibilitymap.c

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -645,10 +645,9 @@ static void
645645
vm_extend(Relation rel, BlockNumber vm_nblocks)
646646
{
647647
BlockNumber vm_nblocks_now;
648-
Page pg;
648+
PGAlignedBlock pg;
649649

650-
pg = (Page) palloc(BLCKSZ);
651-
PageInit(pg, BLCKSZ, 0);
650+
PageInit((Page) pg.data, BLCKSZ, 0);
652651

653652
/*
654653
* We use the relation extension lock to lock out other backends trying to
@@ -679,10 +678,10 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
679678
/* Now extend the file */
680679
while (vm_nblocks_now < vm_nblocks)
681680
{
682-
PageSetChecksumInplace(pg, vm_nblocks_now);
681+
PageSetChecksumInplace((Page) pg.data, vm_nblocks_now);
683682

684683
smgrextend(rel->rd_smgr, VISIBILITYMAP_FORKNUM, vm_nblocks_now,
685-
(char *) pg, false);
684+
pg.data, false);
686685
vm_nblocks_now++;
687686
}
688687

@@ -699,6 +698,4 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
699698
rel->rd_smgr->smgr_vm_nblocks = vm_nblocks_now;
700699

701700
UnlockRelationForExtension(rel, ExclusiveLock);
702-
703-
pfree(pg);
704701
}

‎src/backend/access/transam/generic_xlog.c

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -61,14 +61,11 @@ typedef struct
6161
/* State of generic xlog record construction */
6262
struct GenericXLogState
6363
{
64-
/*
65-
* page's images. Should be first in this struct to have MAXALIGN'ed
66-
* images addresses, because some code working with pages directly aligns
67-
* addresses, not offsets from beginning of page
68-
*/
69-
char images[MAX_GENERIC_XLOG_PAGES * BLCKSZ];
64+
/* Info about each page, see above */
7065
PageData pages[MAX_GENERIC_XLOG_PAGES];
7166
bool isLogged;
67+
/* Page images (properly aligned) */
68+
PGAlignedBlock images[MAX_GENERIC_XLOG_PAGES];
7269
};
7370

7471
static void writeFragment(PageData *pageData, OffsetNumber offset,
@@ -251,12 +248,12 @@ computeDelta(PageData *pageData, Page curpage, Page targetpage)
251248
#ifdef WAL_DEBUG
252249
if (XLOG_DEBUG)
253250
{
254-
char tmp[BLCKSZ];
251+
PGAlignedBlock tmp;
255252

256-
memcpy(tmp, curpage, BLCKSZ);
257-
applyPageRedo(tmp, pageData->delta, pageData->deltaLen);
258-
if (memcmp(tmp, targetpage, targetLower) != 0 ||
259-
memcmp(tmp + targetUpper, targetpage + targetUpper,
253+
memcpy(tmp.data, curpage, BLCKSZ);
254+
applyPageRedo(tmp.data, pageData->delta, pageData->deltaLen);
255+
if (memcmp(tmp.data, targetpage, targetLower) != 0 ||
256+
memcmp(tmp.data + targetUpper, targetpage + targetUpper,
260257
BLCKSZ - targetUpper) != 0)
261258
elog(ERROR, "result of generic xlog apply does not match");
262259
}
@@ -277,7 +274,7 @@ GenericXLogStart(Relation relation)
277274

278275
for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
279276
{
280-
state->pages[i].image = state->images + BLCKSZ * i;
277+
state->pages[i].image = state->images[i].data;
281278
state->pages[i].buffer = InvalidBuffer;
282279
}
283280

0 commit comments

Comments
 (0)