From 083d9ced14e4af4b48e7607bd6cc7567a68ec899 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 1 Sep 2018 15:27:13 -0400 Subject: 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 --- src/backend/access/transam/xlog.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) (limited to 'src/backend/access/transam/xlog.c') diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 621dcb451fd..5e3b6caecf5 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -3173,8 +3173,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock) { 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; @@ -3228,16 +3227,12 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock) * 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; @@ -3328,7 +3323,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno) { char path[MAXPGPATH]; char tmppath[MAXPGPATH]; - char buffer[XLOG_BLCKSZ]; + PGAlignedXLogBlock buffer; int srcfd; int fd; int nbytes; @@ -3364,7 +3359,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno) 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, @@ -3375,7 +3370,7 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno) (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; @@ -9200,7 +9195,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std) */ if (XLogCheckBuffer(rdata, false, &lsn, &bkpb)) { - char copied_buffer[BLCKSZ]; + PGAlignedBlock copied_buffer; char *origdata = (char *) BufferGetBlock(buffer); /* @@ -9212,8 +9207,8 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std) * 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); @@ -9228,7 +9223,7 @@ XLogSaveBufferForHint(Buffer buffer, bool buffer_std) /* * 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; -- cgit v1.2.3