From 8735978e7aebfbc499843630131c18d1f7346c79 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 26 Nov 2017 15:17:24 -0500 Subject: [PATCH] Pad XLogReaderState's main_data buffer more aggressively. Originally, we palloc'd this buffer just barely big enough to hold the largest xlog record seen so far. It turns out that that can result in valgrind complaints, because some compilers will emit code that assumes it can safely fetch padding bytes at the end of a struct, and those padding bytes were unallocated so far as aset.c was concerned. We can fix that by MAXALIGN'ing the palloc request size, ensuring that it is big enough to include any possible padding that might've been omitted from the on-disk record. An additional objection to the original coding is that it could result in many repeated palloc cycles, in the worst case where we see a series of gradually larger xlog records. We can ameliorate that cheaply by imposing a minimum buffer size that's large enough for most xlog records. BLCKSZ/2 was chosen after a bit of discussion. In passing, remove an obsolete comment in struct xl_heap_new_cid that the combocid field is free due to alignment considerations. Perhaps that was true at some point, but it's not now. Back-patch to 9.5 where this code came in. Discussion: https://postgr.es/m/E1eHa4J-0006hI-Q8@gemulon.postgresql.org --- src/backend/access/transam/xlogreader.c | 17 ++++++++++++++++- src/include/access/heapam_xlog.h | 8 +------- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index aeaafedf0b..fa9432eb67 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -1279,7 +1279,22 @@ DecodeXLogRecord(XLogReaderState *state, XLogRecord *record, char **errormsg) { if (state->main_data) pfree(state->main_data); - state->main_data_bufsz = state->main_data_len; + + /* + * main_data_bufsz must be MAXALIGN'ed. In many xlog record + * types, we omit trailing struct padding on-disk to save a few + * bytes; but compilers may generate accesses to the xlog struct + * that assume that padding bytes are present. If the palloc + * request is not large enough to include such padding bytes then + * we'll get valgrind complaints due to otherwise-harmless fetches + * of the padding bytes. + * + * In addition, force the initial request to be reasonably large + * so that we don't waste time with lots of trips through this + * stanza. BLCKSZ / 2 seems like a good compromise choice. + */ + state->main_data_bufsz = MAXALIGN(Max(state->main_data_len, + BLCKSZ / 2)); state->main_data = palloc(state->main_data_bufsz); } memcpy(state->main_data, ptr, state->main_data_len); diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h index 81a6a395c4..5e4dee60c7 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -339,13 +339,7 @@ typedef struct xl_heap_new_cid TransactionId top_xid; CommandId cmin; CommandId cmax; - - /* - * don't really need the combocid since we have the actual values right in - * this struct, but the padding makes it free and its useful for - * debugging. - */ - CommandId combocid; + CommandId combocid; /* just for debugging */ /* * Store the relfilenode/ctid pair to facilitate lookups.