From: | Andres Freund <andres(at)2ndquadrant(dot)com> |
---|---|
To: | Noah Misch <noah(at)leadboat(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: A couple logical decoding fixes/patches |
Date: | 2014-05-10 14:56:51 |
Message-ID: | 20140510145651.GA16507@awork2.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2014-05-10 00:59:59 -0400, Noah Misch wrote:
> On Fri, May 09, 2014 at 04:58:54PM +0200, Andres Freund wrote:
> > On 2014-05-09 10:49:09 -0400, Robert Haas wrote:
> > > > Patch 03: Add valgrind suppression for writing out padding bytes. That's
> > > > better than zeroing the data from the get go because unitialized
> > > > accesses are still detected.
> > >
> > > I have no understanding of valgrind suppressions. I can commit this
> > > blindly if nobody else wants to pick it up.
> >
> > I think the only committer with previous experience in that area is
> > Noah. Noah?
>
> I can pick up that patch.
Cool.
> Static functions having only one call site are especially vulnerable to
> inlining, so avoid naming them in the suppressions file. I do see
> ReorderBufferSerializeChange() inlined away at -O2 and higher. Is it fair to
> tie the suppression to ReorderBufferSerializeTXN() instead?
Hm. That's a good point. If you're talking about tying it to
ReorderBufferSerializeTXN() you mean to list it below the write, as part
of the callstack?
{
padding_reorderbuffer_serialize
Memcheck:Param
write(buf)
...
fun:ReorderBufferSerializeTXN
}
If so, yes, that should be fine. Since there's no other writes it
shouldn't make a difference.
> Do you happen to have a self-contained procedure for causing the server to
> reach the code in question?
(cd contrib/test_decoding && make -s installcheck-force)
against a server running with
valgrind \
--quiet --trace-children=yes --leak-check=no --track-origins=yes \
--read-var-info=yes run-pg-dev-master -c logging_collector=on \
--suppressions=/home/andres/src/postgresql/src/tools/valgrind.supp
<path/to/postgres> \
-c wal_level=logical -c max_replication_slots=3
Does the trick here. Valgrind warns in the first (ddl) test run.
the -force is needed because it needs a server running with -c
wal_level=logical -c max_replication_slots=3.
Note that you'll possibly get a spurious regression test failure because
autovacuum ran inbetween and it's transaction is visible in the test
output like:
BEGIN
+ COMMIT
+ BEGIN
...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Guillaume Lelarge | 2014-05-10 15:32:31 | Re: Weird behaviour with the new MOVE clause of ALTER TABLESPACE |
Previous Message | MauMau | 2014-05-10 14:10:55 | Re: [bug fix] pg_ctl always uses the same event source |