summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 20cd888)
raw | patch | inline | side by side (parent: 20cd888)
| author | Tom Lane <tgl@sss.pgh.pa.us> | |
| Sat, 1 Sep 2018 19:27:13 +0000 (15:27 -0400) | ||
| committer | Tom Lane <tgl@sss.pgh.pa.us> | |
| Sat, 1 Sep 2018 19:27:13 +0000 (15:27 -0400) |
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
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
14 files changed:
index 32c724e5ce2fded164be54deefaee289b16c5197..f719059f0a7b501ea5328dee4b661ad800ebbc83 100644 (file)
PREWARM_BUFFER
} PrewarmType;
-static char blockbuffer[BLCKSZ];
+static PGAlignedBlock blockbuffer;
/*
* pg_prewarm(regclass, mode text, fork text,
for (block = first_block; block <= last_block; ++block)
{
CHECK_FOR_INTERRUPTS();
- smgrread(rel->rd_smgr, forkNumber, block, blockbuffer);
+ smgrread(rel->rd_smgr, forkNumber, block, blockbuffer.data);
++blocks_done;
}
}
index bdf1f2e58890273794742f3fa0df1e690b2aee14..f013b9fdeb25569a50995a868014f5f532d7302d 100644 (file)
/* these must be static so they can be returned to caller */
static ginxlogSplitEntry data;
- static char tupstore[2 * BLCKSZ];
+ static PGAlignedBlock tupstore[2];
entryPreparePage(btree, lpage, off, insertData, updateblkno);
* one after another in a temporary workspace.
*/
maxoff = PageGetMaxOffsetNumber(lpage);
- ptr = tupstore;
+ ptr = tupstore[0].data;
for (i = FirstOffsetNumber; i <= maxoff; i++)
{
if (i == off)
ptr += size;
totalsize += size + sizeof(ItemIdData);
}
- tupstoresize = ptr - tupstore;
+ tupstoresize = ptr - tupstore[0].data;
/*
* Initialize the left and right pages, and copy all the tuples back to
GinInitPage(rpage, GinPageGetOpaque(lpage)->flags, pageSize);
GinInitPage(lpage, GinPageGetOpaque(rpage)->flags, pageSize);
- ptr = tupstore;
+ ptr = tupstore[0].data;
maxoff++;
lsize = 0;
rdata[0].next = &rdata[1];
rdata[1].buffer = InvalidBuffer;
- rdata[1].data = tupstore;
+ rdata[1].data = tupstore[0].data;
rdata[1].len = tupstoresize;
rdata[1].next = NULL;
index 7888b896ab3b246e0d0f96282e72624e6b9bccf1..a54d7c4f3632fda154fc72c92e040953d34bafea 100644 (file)
size = 0;
OffsetNumber l,
off;
- char *workspace;
+ PGAlignedBlock workspace;
char *ptr;
- /* workspace could be a local array; we use palloc for alignment */
- workspace = palloc(BLCKSZ);
-
START_CRIT_SECTION();
GinInitBuffer(buffer, GIN_LIST);
off = FirstOffsetNumber;
- ptr = workspace;
+ ptr = workspace.data;
for (i = 0; i < ntuples; i++)
{
rdata[0].next = rdata + 1;
rdata[1].buffer = InvalidBuffer;
- rdata[1].data = workspace;
+ rdata[1].data = workspace.data;
rdata[1].len = size;
rdata[1].next = NULL;
END_CRIT_SECTION();
- pfree(workspace);
-
return freesize;
}
index 9ab2b06cc9fb75714a59040308a4cb677280d250..38e719ecfac1ec9ff4f1c479f5f3dcabd6ee5673 100644 (file)
_hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
{
BlockNumber lastblock;
- char zerobuf[BLCKSZ];
+ PGAlignedBlock zerobuf;
lastblock = firstblock + nblocks - 1;
if (lastblock < firstblock || lastblock == InvalidBlockNumber)
return false;
- MemSet(zerobuf, 0, sizeof(zerobuf));
+ MemSet(zerobuf.data, 0, sizeof(zerobuf));
RelationOpenSmgr(rel);
- smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf, false);
+ smgrextend(rel->rd_smgr, MAIN_FORKNUM, lastblock, zerobuf.data, false);
return true;
}
index 99165b1e487515224097c3d1b62eb29d0dd695c0..ed8dbae0d24badd163ae1156dc54b236fe51f25f 100644 (file)
HeapTuple *heaptuples;
int i;
int ndone;
- char *scratch = NULL;
+ PGAlignedBlock scratch;
Page page;
bool needwal;
Size saveFreeSpace;
heaptuples[i] = heap_prepare_insert(relation, tuples[i],
xid, cid, options);
- /*
- * Allocate some memory to use for constructing the WAL record. Using
- * palloc() within a critical section is not safe, so we allocate this
- * beforehand.
- */
- if (needwal)
- scratch = palloc(BLCKSZ);
-
/*
* We're about to do the actual inserts -- but check for conflict first,
* to minimize the possibility of having to roll back work we've just
uint8 info = XLOG_HEAP2_MULTI_INSERT;
char *tupledata;
int totaldatalen;
- char *scratchptr = scratch;
+ char *scratchptr = scratch.data;
bool init;
/*
log_heap_new_cid(relation, heaptup);
}
totaldatalen = scratchptr - tupledata;
- Assert((scratchptr - scratch) < BLCKSZ);
+ Assert((scratchptr - scratch.data) < BLCKSZ);
rdata[0].data = (char *) xlrec;
- rdata[0].len = tupledata - scratch;
+ rdata[0].len = tupledata - scratch.data;
rdata[0].buffer = InvalidBuffer;
rdata[0].next = &rdata[1];
index 6301a773aafbe018e63adaf8aafdfdc224ba29cd..35b76967571bca6c9773747dd898d024eaa356af 100644 (file)
vm_extend(Relation rel, BlockNumber vm_nblocks)
{
BlockNumber vm_nblocks_now;
- Page pg;
+ PGAlignedBlock pg;
- pg = (Page) palloc(BLCKSZ);
- PageInit(pg, BLCKSZ, 0);
+ PageInit((Page) pg.data, BLCKSZ, 0);
/*
* We use the relation extension lock to lock out other backends trying to
/* Now extend the file */
while (vm_nblocks_now < vm_nblocks)
{
- PageSetChecksumInplace(pg, vm_nblocks_now);
+ PageSetChecksumInplace((Page) pg.data, vm_nblocks_now);
smgrextend(rel->rd_smgr, VISIBILITYMAP_FORKNUM, vm_nblocks_now,
- (char *) pg, false);
+ pg.data, false);
vm_nblocks_now++;
}
rel->rd_smgr->smgr_vm_nblocks = vm_nblocks_now;
UnlockRelationForExtension(rel, ExclusiveLock);
-
- pfree(pg);
}
index 621dcb451fd6ac88e3dadc0beaa8575a074fb516..5e3b6caecf5559ad83305bf4162caa4d7c61cdeb 100644 (file)
{
char path[MAXPGPATH];
char tmppath[MAXPGPATH];
- char zbuffer_raw[XLOG_BLCKSZ + MAXIMUM_ALIGNOF];
- char *zbuffer;
+ PGAlignedXLogBlock zbuffer;
XLogSegNo installed_segno;
int max_advance;
int fd;
* fsync below) that all the indirect blocks are down on disk. Therefore,
* fdatasync(2) or O_DSYNC will be sufficient to sync future writes to the
* log file.
- *
- * Note: ensure the buffer is reasonably well-aligned; this may save a few
- * cycles transferring data to the kernel.
*/
- zbuffer = (char *) MAXALIGN(zbuffer_raw);
- memset(zbuffer, 0, XLOG_BLCKSZ);
+ memset(zbuffer.data, 0, XLOG_BLCKSZ);
for (nbytes = 0; nbytes < XLogSegSize; nbytes += XLOG_BLCKSZ)
{
errno = 0;
- if ((int) write(fd, zbuffer, XLOG_BLCKSZ) != (int) XLOG_BLCKSZ)
+ if ((int) write(fd, zbuffer.data, XLOG_BLCKSZ) != (int) XLOG_BLCKSZ)
{
int save_errno = errno;
{
char path[MAXPGPATH];
char tmppath[MAXPGPATH];
- char buffer[XLOG_BLCKSZ];
+ PGAlignedXLogBlock buffer;
int srcfd;
int fd;
int nbytes;
for (nbytes = 0; nbytes < XLogSegSize; nbytes += sizeof(buffer))
{
errno = 0;
- if ((int) read(srcfd, buffer, sizeof(buffer)) != (int) sizeof(buffer))
+ if ((int) read(srcfd, buffer.data, sizeof(buffer)) != (int) sizeof(buffer))
{
if (errno != 0)
ereport(ERROR,
(errmsg("not enough data in file \"%s\"", path)));
}
errno = 0;
- if ((int) write(fd, buffer, sizeof(buffer)) != (int) sizeof(buffer))
+ if ((int) write(fd, buffer.data, sizeof(buffer)) != (int) sizeof(buffer))
{
int save_errno = errno;
*/
if (XLogCheckBuffer(rdata, false, &lsn, &bkpb))
{
- char copied_buffer[BLCKSZ];
+ PGAlignedBlock copied_buffer;
char *origdata = (char *) BufferGetBlock(buffer);
/*
* and hole_offset to 0; so the following code is safe for either
* case.
*/
- memcpy(copied_buffer, origdata, bkpb.hole_offset);
- memcpy(copied_buffer + bkpb.hole_offset,
+ memcpy(copied_buffer.data, origdata, bkpb.hole_offset);
+ memcpy(copied_buffer.data + bkpb.hole_offset,
origdata + bkpb.hole_offset + bkpb.hole_length,
BLCKSZ - bkpb.hole_offset - bkpb.hole_length);
/*
* Save copy of the buffer.
*/
- rdata[1].data = copied_buffer;
+ rdata[1].data = copied_buffer.data;
rdata[1].len = BLCKSZ - bkpb.hole_length;
rdata[1].buffer = InvalidBuffer;
rdata[1].next = NULL;
index f2b6a3fb6aa577054b0c4df7c0cb651c63f5440d..584995e6f5c4bd24452335610399bf266fa84608 100644 (file)
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
/* 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,
* 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
index 7737c4ddbefecf1edd55bab6632b722bf0aae228..4c7d55488ed0517c9ab4e22a0ee85d881f73848d 100644 (file)
bytesleft = histfilelen;
while (bytesleft > 0)
{
- char rbuf[BLCKSZ];
+ PGAlignedBlock rbuf;
int nread;
- nread = read(fd, rbuf, sizeof(rbuf));
+ nread = read(fd, rbuf.data, sizeof(rbuf));
if (nread <= 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not read file \"%s\": %m",
path)));
- pq_sendbytes(&buf, rbuf, nread);
+ pq_sendbytes(&buf, rbuf.data, nread);
bytesleft -= nread;
}
CloseTransientFile(fd);
index 0f007c821224b7efa96a5f51736b4b4277495b8e..bc0f59f783c7ad3440f891f672e71b273a2a174b 100644 (file)
off_t curOffset; /* offset part of current pos */
int pos; /* next read/write position in buffer */
int nbytes; /* total # of valid bytes in buffer */
- char buffer[BLCKSZ];
+ PGAlignedBlock buffer;
};
static BufFile *makeBufFile(File firstfile);
/*
* Read whatever we can get, up to a full bufferload.
*/
- file->nbytes = FileRead(thisfile, file->buffer, sizeof(file->buffer));
+ file->nbytes = FileRead(thisfile, file->buffer.data, sizeof(file->buffer));
if (file->nbytes < 0)
file->nbytes = 0;
file->offsets[file->curFile] += file->nbytes;
return; /* seek failed, give up */
file->offsets[file->curFile] = file->curOffset;
}
- bytestowrite = FileWrite(thisfile, file->buffer + wpos, bytestowrite);
+ bytestowrite = FileWrite(thisfile, file->buffer.data + wpos, bytestowrite);
if (bytestowrite <= 0)
return; /* failed to write */
file->offsets[file->curFile] += bytestowrite;
nthistime = size;
Assert(nthistime > 0);
- memcpy(ptr, file->buffer + file->pos, nthistime);
+ memcpy(ptr, file->buffer.data + file->pos, nthistime);
file->pos += nthistime;
ptr = (void *) ((char *) ptr + nthistime);
nthistime = size;
Assert(nthistime > 0);
- memcpy(file->buffer + file->pos, ptr, nthistime);
+ memcpy(file->buffer.data + file->pos, ptr, nthistime);
file->dirty = true;
file->pos += nthistime;
index 7eb9b4b09e88fc0218c3896af4302389cb0a481b..1fa7679a4a016f08439b0747390868e97e3bb8ab 100644 (file)
fsm_extend(Relation rel, BlockNumber fsm_nblocks)
{
BlockNumber fsm_nblocks_now;
- Page pg;
+ PGAlignedBlock pg;
- pg = (Page) palloc(BLCKSZ);
- PageInit(pg, BLCKSZ, 0);
+ PageInit((Page) pg.data, BLCKSZ, 0);
/*
* We use the relation extension lock to lock out other backends trying to
while (fsm_nblocks_now < fsm_nblocks)
{
- PageSetChecksumInplace(pg, fsm_nblocks_now);
+ PageSetChecksumInplace((Page) pg.data, fsm_nblocks_now);
smgrextend(rel->rd_smgr, FSM_FORKNUM, fsm_nblocks_now,
- (char *) pg, false);
+ pg.data, false);
fsm_nblocks_now++;
}
rel->rd_smgr->smgr_fsm_nblocks = fsm_nblocks_now;
UnlockRelationForExtension(rel, ExclusiveLock);
-
- pfree(pg);
}
/*
index 5e1f3d40496ac4795290f3c546e1b33637efc9d7..2fbd25c0fd079a19c893d428f28359d7d7b6dd3d 100644 (file)
int f;
char fn[MAXPGPATH];
struct stat statbuf;
- char *zerobuf;
+ PGAlignedXLogBlock zerobuf;
int bytes;
XLogSegNo segno;
}
/* New, empty, file. So pad it to 16Mb with zeroes */
- zerobuf = pg_malloc0(XLOG_BLCKSZ);
+ memset(zerobuf.data, 0, XLOG_BLCKSZ);
for (bytes = 0; bytes < XLogSegSize; bytes += XLOG_BLCKSZ)
{
errno = 0;
- if (write(f, zerobuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ if (write(f, zerobuf.data, XLOG_BLCKSZ) != XLOG_BLCKSZ)
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
fprintf(stderr,
_("%s: could not pad transaction log file \"%s\": %s\n"),
progname, fn, strerror(errno));
- free(zerobuf);
close(f);
unlink(fn);
return false;
}
}
- free(zerobuf);
if (lseek(f, SEEK_SET, 0) != 0)
{
index 78ef243ad997102df54d19c1e19f85fbd4297cac..d037325c1d301a87b1a8baa64646bce4baa0ba20 100644 (file)
static void
WriteEmptyXLOG(void)
{
- char *buffer;
+ PGAlignedXLogBlock buffer;
XLogPageHeader page;
XLogLongPageHeader longpage;
XLogRecord *record;
int fd;
int nbytes;
- /* Use malloc() to ensure buffer is MAXALIGNED */
- buffer = (char *) pg_malloc(XLOG_BLCKSZ);
- page = (XLogPageHeader) buffer;
- memset(buffer, 0, XLOG_BLCKSZ);
+ memset(buffer.data, 0, XLOG_BLCKSZ);
/* Set up the XLOG page header */
+ page = (XLogPageHeader) buffer.data;
page->xlp_magic = XLOG_PAGE_MAGIC;
page->xlp_info = XLP_LONG_HEADER;
page->xlp_tli = ControlFile.checkPointCopy.ThisTimeLineID;
}
errno = 0;
- if (write(fd, buffer, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ if (write(fd, buffer.data, XLOG_BLCKSZ) != XLOG_BLCKSZ)
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
}
/* Fill the rest of the file with zeroes */
- memset(buffer, 0, XLOG_BLCKSZ);
+ memset(buffer.data, 0, XLOG_BLCKSZ);
for (nbytes = XLOG_BLCKSZ; nbytes < XLogSegSize; nbytes += XLOG_BLCKSZ)
{
errno = 0;
- if (write(fd, buffer, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ if (write(fd, buffer.data, XLOG_BLCKSZ) != XLOG_BLCKSZ)
{
if (errno == 0)
errno = ENOSPC;
diff --git a/src/include/c.h b/src/include/c.h
index e6a1f4b32915377223b41f6b29812fdc3b09fcc3..6836b0a0f56828c2d867b97f259ddf6780f5e6f1 100644 (file)
--- a/src/include/c.h
+++ b/src/include/c.h
* ----------------------------------------------------------------
*/
+/*
+ * Use this, not "char buf[BLCKSZ]", to declare a field or local variable
+ * holding a page buffer, if that page might be accessed as a page and not
+ * just a string of bytes. Otherwise the variable might be under-aligned,
+ * causing problems on alignment-picky hardware. (In some places, we use
+ * this to declare buffers even though we only pass them to read() and
+ * write(), because copying to/from aligned buffers is usually faster than
+ * using unaligned buffers.) We include both "double" and "int64" in the
+ * union to ensure that the compiler knows the value must be MAXALIGN'ed
+ * (cf. configure's computation of MAXIMUM_ALIGNOF).
+ */
+typedef union PGAlignedBlock
+{
+ char data[BLCKSZ];
+ double force_align_d;
+ int64 force_align_i64;
+} PGAlignedBlock;
+
+/* Same, but for an XLOG_BLCKSZ-sized buffer */
+typedef union PGAlignedXLogBlock
+{
+ char data[XLOG_BLCKSZ];
+ double force_align_d;
+ int64 force_align_i64;
+} PGAlignedXLogBlock;
+
/* msb for char */
#define HIGHBIT (0x80)
#define IS_HIGHBIT_SET(ch) ((unsigned char)(ch) & HIGHBIT)