Re: [BUGS] BUG #1707: statistics collector starts with stats_start_collector

The following bug has been logged online:

Bug reference: 1707
Logged by: Federico Di Gregorio
Email address: fog(at)initd(dot)org
PostgreSQL version: 7.4.8
Operating system: Debian GNU/Linux
Description: statistics collector starts with stats_start_collector =
false
Details:

If the following combinatio of parameters is used:

stats_start_collector = false
stats_row_level = true

the collector process is started even if the documentation says that "The
parameter stats_start_collector must be set to true for the statistics
collector to be launched at all."

"Federico Di Gregorio" <fog(at)initd(dot)org> writes:
> If the following combinatio of parameters is used:

> stats_start_collector = false
> stats_row_level = true

> the collector process is started even if the documentation says that "The
> parameter stats_start_collector must be set to true for the statistics
> collector to be launched at all."

I believe stats_start_collector is forced on if any of the subsidiary
parameters is set. Isn't that what you want?

regards, tom lane

Tom Lane wrote:
> "Federico Di Gregorio" <fog(at)initd(dot)org> writes:
> > If the following combinatio of parameters is used:
>
> > stats_start_collector = false
> > stats_row_level = true
>
> > the collector process is started even if the documentation says that "The
> > parameter stats_start_collector must be set to true for the statistics
> > collector to be launched at all."
>
> I believe stats_start_collector is forced on if any of the subsidiary
> parameters is set. Isn't that what you want?

If that is the logic, why do we have stats_start_collector at all?
Seems we either should honor it, or remove it.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> If that is the logic, why do we have stats_start_collector at all?

Ask Jan ;-)

I can see some advantage to it as a way of finding out whether the
collector was started or not ... but if that's the intent, we
should make it non user-writable.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > If that is the logic, why do we have stats_start_collector at all?
>
> Ask Jan ;-)
>
> I can see some advantage to it as a way of finding out whether the
> collector was started or not ... but if that's the intent, we
> should make it non user-writable.

I can see someone turning it off but leaving other options on because
they will want to turn them on later. I think our current behavior is
broken.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I can see someone turning it off but leaving other options on because
> they will want to turn them on later.

"later" when? It's not useful to try to turn on stats_start_collector
after postmaster start, because the UDP socket won't exist.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I can see someone turning it off but leaving other options on because
> > they will want to turn them on later.
>
> "later" when? It's not useful to try to turn on stats_start_collector
> after postmaster start, because the UDP socket won't exist.

Later such as in a later postmaster start, but personally I would just
remove the option completely.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Later such as in a later postmaster start, but personally I would just
> remove the option completely.

I don't have a problem with removing it as a writable option ... but
I'm thinking we should leave it as a read-only GUC parameter (like
the several others we have already). Otherwise we'll need to add some
other method of finding out whether the collector is running.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Later such as in a later postmaster start, but personally I would just
> > remove the option completely.
>
> I don't have a problem with removing it as a writable option ... but
> I'm thinking we should leave it as a read-only GUC parameter (like
> the several others we have already). Otherwise we'll need to add some
> other method of finding out whether the collector is running.

Why would a user care? They either are gathering such statistics, or
they are not. I think the only issue is that you can't turn on certain
options after postmaster start unless the stats collector is running ---
is that the issue?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> I don't have a problem with removing it as a writable option ... but
>> I'm thinking we should leave it as a read-only GUC parameter (like
>> the several others we have already). Otherwise we'll need to add some
>> other method of finding out whether the collector is running.

> Why would a user care?

luser> It's not collecting statistics!

us> Is the collector running?

luser> How should I know?

us> er ...

If we don't have a way to check this, we'll regret it soon enough...
now maybe a GUC setting isn't the optimal way, but I think we need
*some* way besides ps. ps doesn't work remotely and I think there's
no simple equivalent under Windows either.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> I don't have a problem with removing it as a writable option ... but
> >> I'm thinking we should leave it as a read-only GUC parameter (like
> >> the several others we have already). Otherwise we'll need to add some
> >> other method of finding out whether the collector is running.
>
> > Why would a user care?
>
> luser> It's not collecting statistics!
>
> us> Is the collector running?
>
> luser> How should I know?
>
> us> er ...
>
> If we don't have a way to check this, we'll regret it soon enough...
> now maybe a GUC setting isn't the optimal way, but I think we need
> *some* way besides ps. ps doesn't work remotely and I think there's
> no simple equivalent under Windows either.

Sure, but the GUC only reports that it thinks the stats collector
started, not whether it is running or not. I think 'ps' is a fine way
to tell.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>>
>>If we don't have a way to check this, we'll regret it soon enough...
>>now maybe a GUC setting isn't the optimal way, but I think we need
>>*some* way besides ps. ps doesn't work remotely and I think there's
>>no simple equivalent under Windows either.
>
>
> Sure, but the GUC only reports that it thinks the stats collector
> started, not whether it is running or not. I think 'ps' is a fine way
> to tell.

Um, no such beast under win32. You can only see some postgres processes,
but can't distinguish them. We'd need some functions that examine the
pids known to the postmaster (pg_get_collector_pid, pg_get_logger_pid, ...)

Regards,
Andreas

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Later such as in a later postmaster start, but personally I would just
> > remove the option completely.
>
> I don't have a problem with removing it as a writable option ... but
> I'm thinking we should leave it as a read-only GUC parameter (like
> the several others we have already). Otherwise we'll need to add some
> other method of finding out whether the collector is running.

I assume the collector should always be running, and if it isn't, it is
mentioned in the server logs. That seems sufficient.

The following patch removes the GUC stats_start_collector. The stats
process is now always started.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The following patch removes the GUC stats_start_collector. The stats
> process is now always started.

WAIT a second!!!

That was *not* the agenda; the collector is only supposed to start if
you turn on at least one of the collection options.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > The following patch removes the GUC stats_start_collector. The stats
> > process is now always started.
>
> WAIT a second!!!
>
> That was *not* the agenda; the collector is only supposed to start if
> you turn on at least one of the collection options.

Really? I thought the process always started and it just sits there if
nothing is fed to it, no? In fact, if it wasn't started from the
beginning, we couldn't enable query strings without restarting the
server.

The current default is true, meaning it always started.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> The current default is true, meaning it always started.

*By default* it starts. But you can turn it off, and there was no
discussion of removing that option.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > The current default is true, meaning it always started.
>
> *By default* it starts. But you can turn it off, and there was no
> discussion of removing that option.

I thought we agreed that there was little value in allowing someone to
prevent the collector from starting. It is just a process slot, right?

You were suggesting we make it read-only in the email I quoted
originally. I am confused.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Mon, Jun 27, 2005 at 08:51:46PM -0400, Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > The following patch removes the GUC stats_start_collector. The stats
> > > process is now always started.
> >
> > WAIT a second!!!
> >
> > That was *not* the agenda; the collector is only supposed to start if
> > you turn on at least one of the collection options.
>
> Really? I thought the process always started and it just sits there if
> nothing is fed to it, no?

No, the process is only started if you ask for it (which is true by
default).

> In fact, if it wasn't started from the beginning, we couldn't enable
> query strings without restarting the server.

That's true -- if you start with the collector disabled, you can't
enable query strings.

> The current default is true, meaning it always started.

Yes, but you can turn it off.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)

Alvaro Herrera wrote:
> On Mon, Jun 27, 2005 at 08:51:46PM -0400, Bruce Momjian wrote:
> > Tom Lane wrote:
> > > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > > The following patch removes the GUC stats_start_collector. The stats
> > > > process is now always started.
> > >
> > > WAIT a second!!!
> > >
> > > That was *not* the agenda; the collector is only supposed to start if
> > > you turn on at least one of the collection options.
> >
> > Really? I thought the process always started and it just sits there if
> > nothing is fed to it, no?
>
> No, the process is only started if you ask for it (which is true by
> default).
>
> > In fact, if it wasn't started from the beginning, we couldn't enable
> > query strings without restarting the server.
>
> That's true -- if you start with the collector disabled, you can't
> enable query strings.
>
> > The current default is true, meaning it always started.
>
> Yes, but you can turn it off.

OK, but if all the stats* variables are off, what overhead is there in
having the stats process run? I would think very little, and if we turn
it always on, we make configuration easier and remove a GUC variable.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Mon, Jun 27, 2005 at 09:13:20PM -0400, Bruce Momjian wrote:
> Alvaro Herrera wrote:

> > > In fact, if it wasn't started from the beginning, we couldn't enable
> > > query strings without restarting the server.
> >
> > That's true -- if you start with the collector disabled, you can't
> > enable query strings.
>
> OK, but if all the stats* variables are off, what overhead is there in
> having the stats process run? I would think very little, and if we turn
> it always on, we make configuration easier and remove a GUC variable.

What about arranging postmaster's main loop so that it starts the stats
collector process if one of the options is activated (assuming they were
all off at start)? Right now you have to restart the server, but I
don't see why it has to be like that.

That has even less overhead. We will have to keep the GUC var, but I
don't think that's too onerous, is it?

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"Granting software the freedom to evolve guarantees only different results,
not better ones." (Zygo Blaxell)

Alvaro Herrera <alvherre(at)surnet(dot)cl> writes:
> What about arranging postmaster's main loop so that it starts the stats
> collector process if one of the options is activated (assuming they were
> all off at start)? Right now you have to restart the server, but I
> don't see why it has to be like that.

Actually, this entire thread is based on a misconception: the "bug"
reporter was under the misimpression that the sum total of what the
collector does is described by the three suboptions, and so having
start_collector on without any of the suboptions on is a useless state.
But this is not so. The collector tracks backend start/end in any case,
and so you get a useful list of sessions in pg_stat_activity even
without enabling any of the more expensive reporting options.

So my vote at this point is that the code is not broken and we should
not change it. Possibly the documentation could stand a little
improvement.

regards, tom lane

Alvaro Herrera <alvherre(at)surnet(dot)cl> writes:
> What about arranging postmaster's main loop so that it starts the stats
> collector process if one of the options is activated (assuming they were
> all off at start)? Right now you have to restart the server, but I
> don't see why it has to be like that.

The reason for a restart is that what "stats_start_collector" is REALLY
all about is telling the postmaster whether to establish the loopback
UDP socket for stats messaging. You don't get to add that after the
fact because there'd be no way to propagate the open socket into
existing backends.

And no, I wouldn't look with favor on the idea of creating the socket
unconditionally. One of the reasons for wanting to be able to turn
off pgstats entirely is so that you aren't dead in the water if the
loopback UDP socket won't work for some reason (example: kernel packet
filtering that's not under your control).

regards, tom lane

Tom Lane wrote:
> Actually, this entire thread is based on a misconception: the "bug"
> reporter was under the misimpression that the sum total of what the
> collector does is described by the three suboptions, and so having
> start_collector on without any of the suboptions on is a useless state.
> But this is not so. The collector tracks backend start/end in any case,
> and so you get a useful list of sessions in pg_stat_activity even
> without enabling any of the more expensive reporting options.

Right.

> So my vote at this point is that the code is not broken and we should
> not change it. Possibly the documentation could stand a little
> improvement.

No question.

> Alvaro Herrera <alvherre(at)surnet(dot)cl> writes:
> > What about arranging postmaster's main loop so that it starts the stats
> > collector process if one of the options is activated (assuming they were
> > all off at start)? Right now you have to restart the server, but I
> > don't see why it has to be like that.
>
> The reason for a restart is that what "stats_start_collector" is REALLY
> all about is telling the postmaster whether to establish the loopback
> UDP socket for stats messaging. You don't get to add that after the
> fact because there'd be no way to propagate the open socket into
> existing backends.
>
> And no, I wouldn't look with favor on the idea of creating the socket
> unconditionally. One of the reasons for wanting to be able to turn
> off pgstats entirely is so that you aren't dead in the water if the
> loopback UDP socket won't work for some reason (example: kernel packet
> filtering that's not under your control).

My issue is that we usually don't have GUC variables that can be set
automatically. Why can't we have the stats collector try to start, and
just throw a server log message if it fails. I guess I don't see why
that is worse than a GUC. Why is the stats collector unique in its use
of of the UDP resource? If we can't start IPv6 we just throw a server
log message and contiue, right?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> My issue is that we usually don't have GUC variables that can be set
> automatically. Why can't we have the stats collector try to start, and
> just throw a server log message if it fails.

I still say the code is not broken and does not require fixing. If
someone doesn't want any stats collection service, they shouldn't have
to pay the overhead of having the collector there. That was the
original design intention for pgstats, and there isn't anyone except
you suggesting that we should force people to have the stats collector
enabled.

regards, tom lane