On Mon, Feb 5, 2018 at 1:45 PM, Peter Geoghegan <p...@bowt.ie> wrote:
>> So, I guess another option might be to call VALGRIND_MAKE_MEM_DEFINED
>> on the buffer. "We know what we're doing, trust us!"
>>
>> In some ways, that seems better than inserting a suppression, because
>> it only affects the memory in the buffer.
>
> I think that that would also work, and would be simpler, but also
> slightly inferior to using the proposed suppression. If there is
> garbage in logtape.c buffers, we still generally don't want to do
> anything important on the basis of those values. We make one exception
> with the suppression, which is a pretty typical kind of exception to
> make -- don't worry if we write() poisoned bytes, since those are
> bound to be alignment related.
>
> OTOH, as I've said we are generally bound to write some kind of
> logtape.c garbage, which will almost certainly not be of the
> uninitialized memory variety. So, while I feel that the suppression is
> better, the advantage is likely microscopic.
Attached patch does it to the tail of the buffer, as Tom suggested on
the -committers thread.
Note that there is one other place in logtape.c that can write a
partial block like this: LogicalTapeRewindForRead(). I haven't
bothered to do anything there, since it cannot possibly be affected by
this issue for the same reason that serial sorts cannot be -- it's
code that is only used by a tuplesort that really needs to spill to
disk, and merge multiple runs (or for tapes that have already been
frozen, that are expected to never reallocate logtape.c buffers).
--
Peter Geoghegan
From a67d4ae602247067a367265acfe83d35a264f40c Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Tue, 6 Feb 2018 09:55:25 -0800
Subject: [PATCH] Mark logtape.c buffer's tail as defined to Valgrind.
LogicalTapeFreeze() may write out its first block when it is dirty but
not full, and then immediately read the first block back in from its
BufFile as a BLCKSZ-width block. This can only occur in rare cases
where next to no tuples were written out, which is only possible with
parallel external tuplesorts. While this is pointless, it's also
harmless. However, this can cause Valgrind to complain about a write()
of uninitialized bytes, so mark the tail of the buffer as defined to
Valgrind.
Note that LogicalTapeFreeze() has always tended to write out some amount
of garbage bytes. All that changed with parallel tuplesort is that the
garbage is sometimes uninitialized memory rather than memory containing
stale data from the tapeset.
Per buildfarm members lousyjack and skink.
Peter Geoghegan, based on a suggestion from Robert Haas and Tom Lane.
---
src/backend/utils/sort/logtape.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index 6b7c10b..1a02aba 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -86,6 +86,7 @@
#include "storage/buffile.h"
#include "utils/builtins.h"
#include "utils/logtape.h"
+#include "utils/memdebug.h"
#include "utils/memutils.h"
/*
@@ -874,6 +875,13 @@ LogicalTapeFreeze(LogicalTapeSet *lts, int tapenum, TapeShare *share)
*/
if (lt->dirty)
{
+ /*
+ * Since garbage bytes at the tail of the buffer may remain
+ * uninitialized in the case of worker tuplesorts with very little
+ * input, mark the tail as defined
+ */
+ VALGRIND_MAKE_MEM_DEFINED(lt->buffer + lt->nbytes,
+ lt->buffer_size - lt->nbytes);
TapeBlockSetNBytes(lt->buffer, lt->nbytes);
ltsWriteBlock(lts, lt->curBlockNumber, (void *) lt->buffer);
lt->writing = false;
--
2.7.4
