Thread: A couple logical decoding fixes/patches

A couple logical decoding fixes/patches

From
Andres Freund
Date:
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

Re: A couple logical decoding fixes/patches

From
Robert Haas
Date:
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



Re: A couple logical decoding fixes/patches

From
Andres Freund
Date:
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



Re: A couple logical decoding fixes/patches

From
Noah Misch
Date:
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



Re: A couple logical decoding fixes/patches

From
Andres Freund
Date:
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



Re: A couple logical decoding fixes/patches

From
Noah Misch
Date:
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