Thread: A couple logical decoding fixes/patches
Hi, Patch 01: Fix a couple of embarassing typos. Most of them harmless, but one isn't and can lead to crashing or decoding wrong data. Patch 02: Don't crash with an Assert() failure if wal_level=logical but max_replication_slots=0. 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. Details are in the commit messages of the individual patches. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Thu, May 8, 2014 at 12:29 PM, Andres Freund <andres@2ndquadrant.com> wrote: > Patch 01: Fix a couple of embarassing typos. Most of them harmless, but > one isn't and can lead to crashing or decoding wrong data. Committed. > Patch 02: Don't crash with an Assert() failure if wal_level=logical but > max_replication_slots=0. Committed. > 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 2014-05-09 10:49:09 -0400, Robert Haas wrote: > Committed. Thanks. > > 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? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
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. 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? Do you happen to have a self-contained procedure for causing the server to reach the code in question? -- Noah Misch EnterpriseDB http://www.enterprisedb.com
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_serializeMemcheck:Paramwrite(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
On Sat, May 10, 2014 at 04:56:51PM +0200, Andres Freund wrote:
> On 2014-05-10 00:59:59 -0400, Noah Misch wrote:
> > 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.
Yep. Committed that way.
> > 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.
Thanks.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com