Re: Valgrind Memcheck support

On Wed, Aug 28, 2013 at 03:16:14PM +0200, Andres Freund wrote:
> On 2013-08-27 23:46:23 -0400, Noah Misch wrote:
> > On Tue, Aug 27, 2013 at 04:14:27PM +0200, Andres Freund wrote:
> > > On 2013-06-09 17:25:59 -0400, Noah Misch wrote:
> > > > ***************
> > > > *** 846,851 **** exec_simple_query(const char *query_string)
> > > > --- 847,856 ----
> > > >
> > > > TRACE_POSTGRESQL_QUERY_START(query_string);
> > > >
> > > > + #ifdef USE_VALGRIND
> > > > + VALGRIND_PRINTF("statement: %s\n", query_string);
> > > > + #endif
> > > > +
> > >
> > > Is there a special reason for adding more logging here? I find this
> > > makes the instrumentation much less useful since reports easily get
> > > burried in those traces. What's the advantage of doing this instead of
> > > log_statement=...? Especially as that location afaics won't help for the
> > > extended protocol?

> > That being said, could you tell me more about your workflow where the extra
> > messages cause a problem? Do you just typically diagnose each Valgrind error
> > without first isolating the pertinent SQL statement?
>
> There's basically two scenarios where I found it annoying:
> a) I manually reproduce some issue, potentially involving several psql
> shells. All those fire up autocompletion and such queries which bloat
> the log while I actually only want to analyze something specific.
> b) I don't actually look for anything triggered by an SQL statement
> itself, but am looking for issues in a walsender, background worker,
> whatever. I still need some foreground activity to trigger the behaviour
> I want to see, but once more, valgrind's logs hide the error.
>
> Also, I sometimes use valgrind's --db-command/--vgdb which gives you
> enough context from the backtrace itself since you can just get the
> statement from there.

Gotcha. My largest use case was running "make installcheck" in search of
longstanding bugs. The runs were unattended, and associating each Valgrind
error with its proximate command was a basic need.

> I vote for just removing that VALGRIND_PRINTF - it doesn't give you
> anything you cannot easily achieve otherwise...

I'd like to see a buildfarm member running "make installcheck" under Valgrind,
so I'd like the code to fit the needs thereof without patching beyond
pg_config_manual.h. Perhaps having the buildfarm member do "valgrind postgres
--log-statement=all 2>combined-logfile" is good enough.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com