TRACE_SORT defined by default

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

TRACE_SORT defined by default

Joe Conway
I just noticed that TRACE_SORT is defined by default (since 2005
apparently). It seems odd since it is the only debugging code enabled by
default.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Peter Geoghegan-4
On Wed, Apr 24, 2019 at 2:07 PM Joe Conway <[hidden email]> wrote:
> I just noticed that TRACE_SORT is defined by default (since 2005
> apparently). It seems odd since it is the only debugging code enabled by
> default.

I think that we should get rid of the #ifdef stuff, so that it isn't
possible to disable the trace_sort instrumentation my commenting out
the TRACE_SORT entry in pg_config_manual.h. I recall being opposed on
this point by Robert Haas. Possibly because he just didn't want to
deal with it at the time.

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Joe Conway
On 4/24/19 5:10 PM, Peter Geoghegan wrote:

> On Wed, Apr 24, 2019 at 2:07 PM Joe Conway <[hidden email]> wrote:
>> I just noticed that TRACE_SORT is defined by default (since 2005
>> apparently). It seems odd since it is the only debugging code enabled by
>> default.
>
> I think that we should get rid of the #ifdef stuff, so that it isn't
> possible to disable the trace_sort instrumentation my commenting out
> the TRACE_SORT entry in pg_config_manual.h. I recall being opposed on
> this point by Robert Haas. Possibly because he just didn't want to
> deal with it at the time.

Has anyone ever (or recently) measured the impact on performance to have
this enabled? Is it that generically useful for debugging of production
instances of Postgres that we really want it always enabled despite the
performance impact?

Maybe the answer to both is "yes", but if so I would agree that we ought
to remove the define and ifdef's and just bake it in.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Peter Geoghegan-4
On Wed, Apr 24, 2019 at 2:15 PM Joe Conway <[hidden email]> wrote:
> Has anyone ever (or recently) measured the impact on performance to have
> this enabled? Is it that generically useful for debugging of production
> instances of Postgres that we really want it always enabled despite the
> performance impact?

It is disabled by default, in the sense that the trace_sort GUC
defaults to off. I believe that the overhead of building in the
instrumentation without enabling it is indistinguishable from zero. In
any case the current status quo is that it's built by default. I have
used it in production, though not very often. It's easy to turn it on
and off.

> Maybe the answer to both is "yes", but if so I would agree that we ought
> to remove the define and ifdef's and just bake it in.

We're only talking about removing the option of including the
instrumentation in binaries when Postgres is built. I'm not aware that
anyone is doing that. It nobody was doing that, then nobody could be
affected by removing the #ifdef crud.

I suspect that the reason that this hasn't happened already is because
it leaves trace_sort/TRACE_SORT in the slightly awkward position of no
longer quite meeting the traditional definition of a "developer
option".

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Alvaro Herrera-9
On 2019-Apr-24, Peter Geoghegan wrote:

> I suspect that the reason that this hasn't happened already is because
> it leaves trace_sort/TRACE_SORT in the slightly awkward position of no
> longer quite meeting the traditional definition of a "developer
> option".

This is a really strange argument.  You're saying that somebody thought
about it: "Hmm, well, I can remove this preprocessor symbol but then
trace_sort would no longer resemble a developer option.  So I'm going to
leave the symbol alone".  I don't think that's what happened.  It seems
more likely to me that nobody has gone to the trouble of deciding that
the symbol is worth removing, let alone actually doing it.

If the instrumentation is good, and you seem to be saying that it is, I
think we should just remove the symbol and be done with it.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Peter Geoghegan-4
On Wed, Apr 24, 2019 at 2:29 PM Alvaro Herrera <[hidden email]> wrote:
> This is a really strange argument.  You're saying that somebody thought
> about it: "Hmm, well, I can remove this preprocessor symbol but then
> trace_sort would no longer resemble a developer option.  So I'm going to
> leave the symbol alone".  I don't think that's what happened.  It seems
> more likely to me that nobody has gone to the trouble of deciding that
> the symbol is worth removing, let alone actually doing it.

It doesn't seem very important now.

> If the instrumentation is good, and you seem to be saying that it is, I
> think we should just remove the symbol and be done with it.

Sounds like a plan. Do you want to take care of it, Joe?

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Tom Lane-2
In reply to this post by Peter Geoghegan-4
Peter Geoghegan <[hidden email]> writes:
> On Wed, Apr 24, 2019 at 2:15 PM Joe Conway <[hidden email]> wrote:
>> Has anyone ever (or recently) measured the impact on performance to have
>> this enabled? Is it that generically useful for debugging of production
>> instances of Postgres that we really want it always enabled despite the
>> performance impact?

> It is disabled by default, in the sense that the trace_sort GUC
> defaults to off. I believe that the overhead of building in the
> instrumentation without enabling it is indistinguishable from zero.

It would probably be useful to actually prove that rather than just
assuming it.  I do see some code under the symbol that is executed
even when !trace_sort, and in any case Andres keeps complaining that
even always-taken branches are expensive ...

> In
> any case the current status quo is that it's built by default. I have
> used it in production, though not very often. It's easy to turn it on
> and off.

Would any non-wizard really have a use for it?

It seems like we should either make this really a developer option
(and hence not enabled by default) or else move it into some other
category than DEVELOPER_OPTIONS.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Peter Geoghegan-4
On Wed, Apr 24, 2019 at 3:04 PM Tom Lane <[hidden email]> wrote:
> > It is disabled by default, in the sense that the trace_sort GUC
> > defaults to off. I believe that the overhead of building in the
> > instrumentation without enabling it is indistinguishable from zero.
>
> It would probably be useful to actually prove that rather than just
> assuming it.

The number of individual trace_sort LOG messages is proportionate to
the number of runs produced.

> I do see some code under the symbol that is executed
> even when !trace_sort, and in any case Andres keeps complaining that
> even always-taken branches are expensive ...

I think that you're referring to the stuff needed for the D-Trace
probes. It's a pity that there isn't better support for that, since
Linux has a lot for options around static userspace probes these days
(SystemTap is very much out of favor, and never was very popular).
There seems to be a recognition among the Linux people that the
distinction between users and backend experts is blurred. The kernel
support for things like eBPF and BCC is still patchy, but that will
change.

> Would any non-wizard really have a use for it?

That depends on what the cut-off point is for wizard. I recognize that
there is a need to draw the line somewhere. I suspect that a fair
number of people could intuit problems in a real-world scenario using
trace_sort, without having any grounding in the theory, and without
much knowledge of tuplesort.c specifically.

> It seems like we should either make this really a developer option
> (and hence not enabled by default) or else move it into some other
> category than DEVELOPER_OPTIONS.

The information that it makes available is approximately the same as
the information made available by the new
pg_stat_progress_create_index view, but with getrusage() stats.

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Jeff Janes
In reply to this post by Tom Lane-2
On Wed, Apr 24, 2019 at 6:04 PM Tom Lane <[hidden email]> wrote:
Peter Geoghegan <[hidden email]> writes:

> In
> any case the current status quo is that it's built by default. I have
> used it in production, though not very often. It's easy to turn it on
> and off.

Would any non-wizard really have a use for it?

I've had people use it to get some insight into the operation and memory usage of Aggregate nodes, since those nodes offer nothing useful via EXPLAIN ANALYZE.  It would be a shame to lose that ability on package-installed PostgreSQL unless we fix Aggregate node reporting first.

Cheers,

Jeff
Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Alvaro Herrera-9
On 2019-Apr-25, Jeff Janes wrote:

> I've had people use it to get some insight into the operation and memory
> usage of Aggregate nodes, since those nodes offer nothing useful via
> EXPLAIN ANALYZE.  It would be a shame to lose that ability on
> package-installed PostgreSQL unless we fix Aggregate node reporting first.

But the proposal is not to remove the _code_.  The proposal is just to
remove that "#ifdef" lines that would make it conditionally compilable,
*if* the symbol that they test weren't always enabled.  In other words,
turn it from "always compiled, but you can turn it off although nobody
does" into "always compiled".

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Peter Geoghegan-4
On Thu, Apr 25, 2019 at 1:52 PM Alvaro Herrera <[hidden email]> wrote:
> But the proposal is not to remove the _code_.  The proposal is just to
> remove that "#ifdef" lines that would make it conditionally compilable,
> *if* the symbol that they test weren't always enabled.  In other words,
> turn it from "always compiled, but you can turn it off although nobody
> does" into "always compiled".

Tom suggested that we might want to remove the code as an alternative.
We have two almost-opposite proposals here.

As I said, I think that the way that we think about or define
developer options frames this discussion.

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Tom Lane-2
In reply to this post by Alvaro Herrera-9
Alvaro Herrera <[hidden email]> writes:
> On 2019-Apr-25, Jeff Janes wrote:
>> I've had people use it to get some insight into the operation and memory
>> usage of Aggregate nodes, since those nodes offer nothing useful via
>> EXPLAIN ANALYZE.  It would be a shame to lose that ability on
>> package-installed PostgreSQL unless we fix Aggregate node reporting first.

> But the proposal is not to remove the _code_.  The proposal is just to
> remove that "#ifdef" lines that would make it conditionally compilable,
> *if* the symbol that they test weren't always enabled.  In other words,
> turn it from "always compiled, but you can turn it off although nobody
> does" into "always compiled".

Well, I was suggesting that we ought to consider the alternative of
making it *not* always compiled, and Jeff was pushing back on that.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Peter Geoghegan-4
On Thu, Apr 25, 2019 at 1:56 PM Tom Lane <[hidden email]> wrote:
> Well, I was suggesting that we ought to consider the alternative of
> making it *not* always compiled, and Jeff was pushing back on that.

Right. Sorry.

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: TRACE_SORT defined by default

Tomas Vondra-4
In reply to this post by Tom Lane-2
On Wed, Apr 24, 2019 at 06:04:41PM -0400, Tom Lane wrote:

>Peter Geoghegan <[hidden email]> writes:
>> On Wed, Apr 24, 2019 at 2:15 PM Joe Conway <[hidden email]> wrote:
>>> Has anyone ever (or recently) measured the impact on performance to have
>>> this enabled? Is it that generically useful for debugging of production
>>> instances of Postgres that we really want it always enabled despite the
>>> performance impact?
>
>> It is disabled by default, in the sense that the trace_sort GUC
>> defaults to off. I believe that the overhead of building in the
>> instrumentation without enabling it is indistinguishable from zero.
>
>It would probably be useful to actually prove that rather than just
>assuming it.  I do see some code under the symbol that is executed
>even when !trace_sort, and in any case Andres keeps complaining that
>even always-taken branches are expensive ...
>

Did I hear the magical word "benchmark" over here?

I suppose it'd be useful to have some actual numbers showing what
overhead this actually has, and whether disabling it would make any
difference. I can't run anything right away, but I could get us some
numbers in a couple of days, assuming there is some agreement on which
cases we need to test.


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services