database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-06-17 17:02:08 +03:00
Code Activity

Avoid using potentially-under-aligned page buffers.

Browse Source 
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
This commit is contained in:
Tom Lane
2018-09-01 15:27:12 -04:00
parent 5e8d670c31
commit 44cac93464
24 changed files with 141 additions and 168 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
src/backend/commands/tablecmds.c
View File

@ -7220,7 +7220,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
recursing | is_readd, /* allow_merge */
!recursing, /* is_local */
is_readd, /* is_internal */
NULL); /* queryString not available here */
NULL); /* queryString not available
* here */
/* we don't expect more than one constraint here */
Assert(list_length(newcons) <= 1);
@ -11276,21 +11277,14 @@ static void
copy_relation_data(SMgrRelation src, SMgrRelation dst,
ForkNumber forkNum, char relpersistence)
{
char *buf;
PGAlignedBlock buf;
Page page;
bool use_wal;
bool copying_initfork;
BlockNumber nblocks;
BlockNumber blkno;
/*
* palloc the buffer so that it's MAXALIGN'd. If it were just a local
* char[] array, the compiler might align it on any byte boundary, which
* can seriously hurt transfer speed to and from the kernel; not to
* mention possibly making log_newpage's accesses to the page header fail.
*/
buf = (char *) palloc(BLCKSZ);
page = (Page) buf;
page = (Page) buf.data;
/*
* The init fork for an unlogged relation in many respects has to be
@ -11314,7 +11308,7 @@ copy_relation_data(SMgrRelation src, SMgrRelation dst,
/* If we got a cancel signal during the copy of the data, quit */
CHECK_FOR_INTERRUPTS();
smgrread(src, forkNum, blkno, buf);
smgrread(src, forkNum, blkno, buf.data);
if (!PageIsVerified(page, blkno))
ereport(ERROR,
@ -11340,11 +11334,9 @@ copy_relation_data(SMgrRelation src, SMgrRelation dst,
* rel, because there's no need for smgr to schedule an fsync for this
* write; we'll do it ourselves below.
*/
smgrextend(dst, forkNum, blkno, buf, true);
smgrextend(dst, forkNum, blkno, buf.data, true);
}
pfree(buf);
/*
* If the rel is WAL-logged, must fsync before commit. We use heap_sync
* to ensure that the toast table gets fsync'd too. (For a temp or