Re: A couple logical decoding fixes/patches

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