On Fri, Feb 2, 2018 at 4:31 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Fri, Feb 2, 2018 at 1:58 PM, Andres Freund <and...@anarazel.de> wrote:
>> Not saying you're wrong, but you should include a comment on why this is
>> a benign warning. Presumably it's some padding memory somewhere, but
>> it's not obvious from the above bleat.
>
> Sure. This looks slightly more complicated than first anticipated, but
> I'll keep everyone posted.
I couldn't make up my mind if it was best to prevent the uninitialized
write(), or to instead just add a suppression. I eventually decided
upon the suppression -- see attached patch. My proposed commit message
has a full explanation of the Valgrind issue, which I won't repeat
here. Go read it before reading the rest of this e-mail.
It might seem like my suppression is overly broad, or not broad
enough, since it essentially targets LogicalTapeFreeze(). I don't
think it is, though, because this can occur in two places within
LogicalTapeFreeze() -- it can occur in the place we actually saw the
issue on lousyjack, from the ltsReadBlock() call within
LogicalTapeFreeze(), as well as a second place -- when
BufFileExportShared() is called. I found that you have to tweak code
to prevent it happening in the first place before you'll see it happen
in the second place. I see no point in actually playing whack-a-mole
for a totally benign issue like this, though, which made me finally
decide upon the suppression approach.
Bear in mind that a third way of fixing this would be to allocate
logtape.c buffers using palloc0() rather than palloc() (though I don't
like that idea at all). For serial external sorts, the logtape.c
buffers are guaranteed to have been written to/initialized at least
once as part of spilling a sort to disk. Parallel external sorts don't
quite guarantee that, which is why we run into this Valgrind issue.
--
Peter Geoghegan
From e51b9d411a859072e6a3f8b2f77e32e0f47d409a Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Fri, 2 Feb 2018 17:47:13 -0800
Subject: [PATCH] Add logtape.c Valgrind suppression.
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 issue causes Valgrind to complain about a write() of
uninitialized bytes, so a suppression is needed. This is because
BufFile block reading will flush out its whole dirty stdio-style temp
buffer, writing bytes that include values originating from
poisoned/uninitialized logtape.c buffer bytes.
Author: Peter Geoghegan
---
src/tools/valgrind.supp | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/tools/valgrind.supp b/src/tools/valgrind.supp
index af03051..22039c0 100644
--- a/src/tools/valgrind.supp
+++ b/src/tools/valgrind.supp
@@ -112,6 +112,16 @@
fun:BootStrapXLOG
}
+# LogicalTapeFreeze() needs this for calls from parallel worker with few tuples
+{
+ padding_buffile_dump_buffer
+ Memcheck:Param
+ write(buf)
+
+ ...
+ fun:LogicalTapeFreeze
+}
+
{
bootstrap_write_relmap_overlap
Memcheck:Overlap
--
2.7.4
