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