Обсуждение: Function to track shmem reinit time

Attached patch introduces a new function pg_shmem_init_time(),
which returns the time shared memory was last (re)initialized.
It is created for use by monitoring tools to track backend crashes.

Currently, if the 'restart_after_crash' option is on, postgres will just restart.
And the only way to know that it happened is to regularly parse logfile
or monitor it, catching restart messages. This approach is really inconvenient for
users, who have gigabytes of logs.

This new function can be periodiacally called by a monitoring agent, and,
if shmem_init_time doesn't match pg_postmaster_start_time,
we know that server crashed-restarted, and also know the exact time, when.

Also, working on this patch, I noticed a bit of dead code
and some discordant comments in postmaster.c.
I see no reason to leave it as is.
So there is a small remove_dead_shmem_reinit_code_v0.patch.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

It can be used to accurately calculate server uptime, since you can`t rely on pg_postmaster_start_time() in this.

Attached patch introduces a new function pg_shmem_init_time(),
which returns the time shared memory was last (re)initialized.
It is created for use by monitoring tools to track backend crashes.

Currently, if the 'restart_after_crash' option is on, postgres will just restart.
And the only way to know that it happened is to regularly parse logfile
or monitor it, catching restart messages. This approach is really inconvenient for
users, who have gigabytes of logs.

This new function can be periodiacally called by a monitoring agent, and,
if shmem_init_time doesn't match pg_postmaster_start_time,
we know that server crashed-restarted, and also know the exact time, when.

Also, working on this patch, I noticed a bit of dead code
and some discordant comments in postmaster.c.
I see no reason to leave it as is.
So there is a small remove_dead_shmem_reinit_code_v0.patch.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

-- 
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2/28/18 07:11, Anastasia Lubennikova wrote:
> Currently, if the 'restart_after_crash' option is on, postgres will just
> restart.
> And the only way to know that it happened is to regularly parse logfile
> or monitor it, catching restart messages. This approach is really
> inconvenient for
> users, who have gigabytes of logs.

I find this premise a bit dubious.  Why have a log file if it's too big
to find anything in it?  Server crashes aren't the only thing people are
interested in.  So we'll need a function for "last $anything".

> This new function can be periodiacally called by a monitoring agent, and,
> if /shmem_init_time/ doesn't match /pg_postmaster_start_time,/
> we know that server crashed-restarted, and also know the exact time, when.

If we want to do something like this, I'd rather have a name that does
not expose implementation details.  If we restructure shared memory
management in the future, the name won't apply anymore.  Call it "last
crash" or "last reinit" or something like that.

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

On Sat, Mar 03, 2018 at 01:00:52PM -0500, Peter Eisentraut wrote:
> On 2/28/18 07:11, Anastasia Lubennikova wrote:
> > Currently, if the 'restart_after_crash' option is on, postgres will just
> > restart.
> > And the only way to know that it happened is to regularly parse logfile
> > or monitor it, catching restart messages. This approach is really
> > inconvenient for
> > users, who have gigabytes of logs.
> 
> I find this premise a bit dubious.  Why have a log file if it's too big
> to find anything in it?  Server crashes aren't the only thing people are
> interested in.  So we'll need a function for "last $anything".

I think one can tell if it's crashed recently by comparing start time of parent
postmaster and its main children (I'm going to go put this in place for myself
now).

ts=# SELECT backend_type, backend_start, pg_postmaster_start_time(), backend_start-pg_postmaster_start_time() FROM
pg_stat_activityORDER BY backend_start LIMIT 1;
 
    backend_type     |         backend_start         |   pg_postmaster_start_time    |    ?column?     
---------------------+-------------------------------+-------------------------------+-----------------
 autovacuum launcher | 2018-03-02 00:21:11.604468-03 | 2018-03-02 00:12:46.757642-03 | 00:08:24.846826

pryzbyj@pryzbyj:~$ ps -O lstart -upostgres # on a separate server with 9.4 which doen't have backend_type, add in v10
  PID                  STARTED S TTY          TIME COMMAND
12982 Mon Feb 19 06:03:13 2018 S ?        00:00:33 /usr/lib/postgresql/9.4/bin/postgres -D /var/lib/postgresql/9.4/main
-cconfig_file=/etc/postgresql/9.4/main/postgresql.conf
 
12997 Mon Feb 19 06:03:14 2018 S ?        00:00:00 postgres: checkpointer process
                                                      
 
12998 Mon Feb 19 06:03:14 2018 S ?        00:00:08 postgres: writer process
                                                      
 
12999 Mon Feb 19 06:03:14 2018 S ?        00:00:08 postgres: wal writer process
                                                      
 
13000 Mon Feb 19 06:03:14 2018 S ?        00:00:19 postgres: autovacuum launcher process
                                                      
 
13001 Mon Feb 19 06:03:14 2018 S ?        00:00:45 postgres: stats collector process
                                                      
 

pryzbyj@pryzbyj:~$ sudo kill -SEGV 12997
pryzbyj@pryzbyj:~$ ps -O lstart -upostgres
  PID                  STARTED S TTY          TIME COMMAND
 8644 Sat Mar  3 12:05:34 2018 S ?        00:00:00 postgres: checkpointer process
                                                      
 
 8645 Sat Mar  3 12:05:34 2018 S ?        00:00:00 postgres: writer process
                                                      
 
 8646 Sat Mar  3 12:05:34 2018 S ?        00:00:00 postgres: wal writer process
                                                      
 
 8647 Sat Mar  3 12:05:34 2018 S ?        00:00:00 postgres: autovacuum launcher process
                                                      
 
 8648 Sat Mar  3 12:05:34 2018 S ?        00:00:00 postgres: stats collector process
                                                      
 
12982 Mon Feb 19 06:03:13 2018 S ?        00:00:33 /usr/lib/postgresql/9.4/bin/postgres -D /var/lib/postgresql/9.4/main
-cconfig_file=/etc/postgresql/9.4/main/postgresql.conf
 

Justin

On 02/28/2018 02:39 PM, Grigory Smolkin wrote:
> It can be used to accurately calculate server uptime, since you can`t
> rely on pg_postmaster_start_time() in this.
> 

Can you please explain why pg_postmaster_start_time can't be used for
this purpose? It seems like a pretty good match, considering it's meant
to show server start time.


regards

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

On 02/28/2018 01:11 PM, Anastasia Lubennikova wrote:
> Attached patch introduces a new function pg_shmem_init_time(),
> which returns the time shared memory was last (re)initialized.
> It is created for use by monitoring tools to track backend crashes.
> 
> Currently, if the 'restart_after_crash' option is on, postgres will
> just restart. And the only way to know that it happened is to
> regularly parse logfile or monitor it, catching restart messages.
> This approach is really inconvenient for users, who have gigabytes of
> logs.
> 
> This new function can be periodiacally called by a monitoring agent,
> and, if /shmem_init_time/ doesn't match /pg_postmaster_start_time,/ 
> we know that server crashed-restarted, and also know the exact time,
> when.
> 

I don't think it really solves the problem, though. For example if the
whole VM reboots (which can be a matter of seconds), this check will say
"shmem_init_time == pg_postmaster_start_time" and you've not detected
anything.

IMHO pg_postmaster_start_time is the right way to monitor uptime, and
the right way to detect spurious restarts is to remember the last value
you've seen and compare it to the current one.

regards

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

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> On 02/28/2018 02:39 PM, Grigory Smolkin wrote:
>> It can be used to accurately calculate server uptime, since you can`t
>> rely on pg_postmaster_start_time() in this.

> Can you please explain why pg_postmaster_start_time can't be used for
> this purpose? It seems like a pretty good match, considering it's meant
> to show server start time.

It evidently depends on how you want to define "server uptime".  If you
get backend crashes often enough, you might feel a need to define it
as "time since last crash".  Although I would think that if that's
happening regularly in a production environment, you have a problem
you need to fix, not just measure.

My own thought about that is that if you are trying to measure
backend crashes, just knowing the time of the latest one is little help.
You want to know how often they're happening.  So this gets back to the
question of why the postmaster log isn't a useful source of that
information.  I think that if we're to do anything in this area,
improving the usefulness of the log would be more important than
providing the proposed function.

            regards, tom lane

On 02/28/2018 01:11 PM, Anastasia Lubennikova wrote:
> 
> This new function can be periodiacally called by a monitoring agent, 
> and, if /shmem_init_time/ doesn't match /pg_postmaster_start_time,/ 
> we know that server crashed-restarted, and also know the exact time, 
> when.
> 

Actually, after looking at the code a bit, I think that test would not
really work anyway, because those two timestamps come from two separate
GetCurrentTimestamp calls, so you get this:

    test=# select pg_shmem_init_time(), pg_postmaster_start_time();
          pg_shmem_init_time       |   pg_postmaster_start_time
    -------------------------------+-------------------------------
     2018-03-04 17:10:59.017058+01 | 2018-03-04 17:10:59.022071+01
    (1 row)

    test=# select pg_shmem_init_time() = pg_postmaster_start_time();
     ?column?
    ----------
     f
    (1 row)

So there would have to be some sort of "fuzz" parameter when comparing
the values, etc.

Furthermore, the patch is yet another victim of fd1a421fe - fixing the
pg_proc entries is trivial, but a new version is needed.

I'd also like to see an example/explanation how this improves this
situation compared to only having pg_postmaster_start_time.

So I'm setting this as waiting on author for now.

regards

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

On 03/03/2018 09:00 PM, Peter Eisentraut wrote:
> I find this premise a bit dubious.  Why have a log file if it's too big
> to find anything in it?  Server crashes aren't the only thing people are
> interested in.  So we'll need a function for "last $anything".

Thank you for your interest in this topic.
Well, on heavy loaded machine log file will be big, because you usually 
want to log every query for later analysis, and, because postgres is 
dumping everything in a single file, searching for some specific error 
will be slow. Log file is certainly needed for digging the details of a 
crash, but pg_shmem_init_time is more about online monitoring.

On 03/03/2018 09:43 PM, Justin Pryzby wrote:

> I think one can tell if it's crashed recently by comparing start time of parent
> postmaster and its main children (I'm going to go put this in place for myself
> now).
>
> ts=# SELECT backend_type, backend_start, pg_postmaster_start_time(), backend_start-pg_postmaster_start_time() FROM
pg_stat_activityORDER BY backend_start LIMIT 1;
 
>      backend_type     |         backend_start         |   pg_postmaster_start_time    |    ?column?
> ---------------------+-------------------------------+-------------------------------+-----------------
>   autovacuum launcher | 2018-03-02 00:21:11.604468-03 | 2018-03-02 00:12:46.757642-03 | 00:08:24.846826

Thank you, though we must take into account the fact that autovacuum may 
not be enabled so it`s better to use checkpointer or bgwriter. It`s 
working solution, yes, but it looks more like workaround and requires 
from user an understanding of postgres internals.


On 03/04/2018 06:56 PM, Tomas Vondra wrote:

> Can you please explain why pg_postmaster_start_time can't be used for
> this purpose? It seems like a pretty good match, considering it's meant
> to show server start time.

Because a backend crash do not reset pg_postmaster_start_time.

On 03/04/2018 07:02 PM, Tomas Vondra wrote:
> On 02/28/2018 01:11 PM, Anastasia Lubennikova wrote:
>> Attached patch introduces a new function pg_shmem_init_time(),
>> which returns the time shared memory was last (re)initialized.
>> It is created for use by monitoring tools to track backend crashes.
>>
>> Currently, if the 'restart_after_crash' option is on, postgres will
>> just restart. And the only way to know that it happened is to
>> regularly parse logfile or monitor it, catching restart messages.
>> This approach is really inconvenient for users, who have gigabytes of
>> logs.
>>
>> This new function can be periodiacally called by a monitoring agent,
>> and, if /shmem_init_time/ doesn't match /pg_postmaster_start_time,/
>> we know that server crashed-restarted, and also know the exact time,
>> when.
>>
> I don't think it really solves the problem, though. For example if the
> whole VM reboots (which can be a matter of seconds), this check will say
> "shmem_init_time == pg_postmaster_start_time" and you've not detected
> anything.
>
> IMHO pg_postmaster_start_time is the right way to monitor uptime, and
> the right way to detect spurious restarts is to remember the last value
> you've seen and compare it to the current one.

Yes, for the described case with VM restart pg_postmaster_start_time 
works fine.
pg_postmaster_start_time is essential for almost every case and 
pg_shmem_init_time only expand his usefulness, not diminish.

On 03/04/2018 07:09 PM, Tom Lane wrote:
> It evidently depends on how you want to define "server uptime".  If you
> get backend crashes often enough, you might feel a need to define it
> as "time since last crash".  Although I would think that if that's
> happening regularly in a production environment, you have a problem
> you need to fix, not just measure.

Absolutely. And for that you need to know ASAP that backend crash 
happened in the first place.

On 03/04/2018 07:09 PM, Tom Lane wrote:
> My own thought about that is that if you are trying to measure
> backend crashes, just knowing the time of the latest one is little help.
> You want to know how often they're happening.  So this gets back to the
> question of why the postmaster log isn't a useful source of that
> information.

For that purpose pg_shmem_init_time also can be used.
For example we can build a chart based on values from "select (now() - 
pg_shmem_init_time());" taken, for example, every 10 seconds.
Values around 0 =< x =<10 will signal about memory reinitialization 
which is usually a byproduct of a backend crash.

On 03/04/2018 07:09 PM, Tom Lane wrote:
>   I think that if we're to do anything in this area,
> improving the usefulness of the log would be more important than
> providing the proposed function.

The separation of maintenance messages and query messaged into different 
log files is sorely needed.
This way server errors can be identified fast and in convenient way.
But as I mentioned earlier, pg_shmem_init_time() is about online monitoring.


On 03/04/2018 07:02 PM, Tomas Vondra wrote:
> Actually, after looking at the code a bit, I think that test would not
> really work anyway, because those two timestamps come from two separate
> GetCurrentTimestamp calls, so you get this:
>
>     test=# select pg_shmem_init_time(), pg_postmaster_start_time();
>           pg_shmem_init_time       |   pg_postmaster_start_time
> -------------------------------+-------------------------------
>      2018-03-04 17:10:59.017058+01 | 2018-03-04 17:10:59.022071+01
>     (1 row)
>
>     test=# select pg_shmem_init_time() = pg_postmaster_start_time();
>      ?column?
>     ----------
>      f
>     (1 row)
>
> So there would have to be some sort of "fuzz" parameter when comparing
> the values, etc.

The difference measured in milliseconds is expected. After all, shared 
memory is initialized after postmaster start.




-- 
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 3/4/18 11:17 AM, Tomas Vondra wrote:
> 
> Furthermore, the patch is yet another victim of fd1a421fe - fixing the
> pg_proc entries is trivial, but a new version is needed.
> 
> I'd also like to see an example/explanation how this improves this
> situation compared to only having pg_postmaster_start_time.
> 
> So I'm setting this as waiting on author for now.

I'm not sure why this was set back to Needs Review since it neither
applies cleanly nor builds.

I'm setting this entry to Waiting on Author, but based on the discussion
I think it should be Returned with Feedback.

Regards,
-- 
-David
david@pgmasters.net

On 03/28/2018 08:55 PM, David Steele wrote:
> On 3/4/18 11:17 AM, Tomas Vondra wrote:
>>
>> Furthermore, the patch is yet another victim of fd1a421fe - fixing the
>> pg_proc entries is trivial, but a new version is needed.
>>
>> I'd also like to see an example/explanation how this improves this
>> situation compared to only having pg_postmaster_start_time.
>>
>> So I'm setting this as waiting on author for now.
> 
> I'm not sure why this was set back to Needs Review since it neither
> applies cleanly nor builds.
> 

It likely applied/built fine when it was set to NR, but got broken by
some recent commit.

> I'm setting this entry to Waiting on Author, but based on the discussion
> I think it should be Returned with Feedback.
> 

Fine with me.

regards

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

On 3/29/18 9:40 AM, Tomas Vondra wrote:
> On 03/28/2018 08:55 PM, David Steele wrote:
> 
>> I'm setting this entry to Waiting on Author, but based on the discussion
>> I think it should be Returned with Feedback.
>>
> 
> Fine with me.

This entry has been marked Returned with Feedback.

Regards,
-- 
-David
david@pgmasters.net

On Tue, Apr 10, 2018 at 9:07 AM, David Steele <david@pgmasters.net> wrote:
> On 3/29/18 9:40 AM, Tomas Vondra wrote:
>> On 03/28/2018 08:55 PM, David Steele wrote:
>>> I'm setting this entry to Waiting on Author, but based on the discussion
>>> I think it should be Returned with Feedback.
>>
>> Fine with me.
>
> This entry has been marked Returned with Feedback.

I guess I'm in the minority here, but I don't see why it isn't useful
to have something like this. It's a lot easier for a monitoring
process to poll for this kind of thing than it is to monitor the logs.
Not that log monitoring isn't a good idea, but this is pretty
lightweight.

OTOH, it also seems like this could live as an out-of-core extension,
so I guess if nobody else likes the idea Anastasia could always do it
that way.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 10, 2018 at 8:58 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Apr 10, 2018 at 9:07 AM, David Steele <david@pgmasters.net> wrote:
> > On 3/29/18 9:40 AM, Tomas Vondra wrote:
> >> On 03/28/2018 08:55 PM, David Steele wrote:
> >>> I'm setting this entry to Waiting on Author, but based on the discussion
> >>> I think it should be Returned with Feedback.
> >>
> >> Fine with me.
> >
> > This entry has been marked Returned with Feedback.
>
> I guess I'm in the minority here, but I don't see why it isn't useful
> to have something like this. It's a lot easier for a monitoring
> process to poll for this kind of thing than it is to monitor the logs.
> Not that log monitoring isn't a good idea, but this is pretty
> lightweight.

+1,

I think the issue this patch is addressing is that PostgreSQL now
doesn't have true uptime function.  We now recommend using
pg_postmaster_start_time() for uptime calculation, but that isn't it.
When backed crashes, shmen is reinitialized and PostgreSQL is
restarted then uptime should be reset to zero, but
pg_postmaster_start_time() is not about that.

The major argument against this patch I got from the thread is that we
shouldn't introduce new functions exposing information, which could be
already fetched from logs.  However, I think following this logic we
should remove pg_postmaster_start_time() too.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sun, Dec 23, 2018 at 11:14 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Tue, Apr 10, 2018 at 8:58 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Apr 10, 2018 at 9:07 AM, David Steele <david@pgmasters.net> wrote:
> > > On 3/29/18 9:40 AM, Tomas Vondra wrote:
> > >> On 03/28/2018 08:55 PM, David Steele wrote:
> > >>> I'm setting this entry to Waiting on Author, but based on the discussion
> > >>> I think it should be Returned with Feedback.
> > >>
> > >> Fine with me.
> > >
> > > This entry has been marked Returned with Feedback.
> >
> > I guess I'm in the minority here, but I don't see why it isn't useful
> > to have something like this. It's a lot easier for a monitoring
> > process to poll for this kind of thing than it is to monitor the logs.
> > Not that log monitoring isn't a good idea, but this is pretty
> > lightweight.
>
> +1,
>
> I think the issue this patch is addressing is that PostgreSQL now
> doesn't have true uptime function.  We now recommend using
> pg_postmaster_start_time() for uptime calculation, but that isn't it.
> When backed crashes, shmen is reinitialized and PostgreSQL is
> restarted then uptime should be reset to zero, but
> pg_postmaster_start_time() is not about that.
>
> The major argument against this patch I got from the thread is that we
> shouldn't introduce new functions exposing information, which could be
> already fetched from logs.  However, I think following this logic we
> should remove pg_postmaster_start_time() too.

I've revised the patchset.

0001-remove_dead_shmem_reinit_code-v1.patch
 * -n option is removed from getopt() argument, initdb help and documentation
 * some comments are slightly revised

0002-shmem_init_time_v1.patch
 * detailed function description is added to the documentation

From my point of view criticism of this patch was addressed by
argument, that pg_shmem_init_time() allows to calculate the server
uptime [1].  This is very basic information, which is reasonable to
get without log files parsing.  It's more than year since [1] is
unanswered.  So, I'm going to push this if no objections.

Links
1. https://www.postgresql.org/message-id/CAPpHfds3_CgNgK4nfcOBo2BfPMFS7yrDXDCPKJSqdGrpqoLwoQ%40mail.gmail.com

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> From my point of view criticism of this patch was addressed by
> argument, that pg_shmem_init_time() allows to calculate the server
> uptime [1].  This is very basic information, which is reasonable to
> get without log files parsing.  It's more than year since [1] is
> unanswered.  So, I'm going to push this if no objections.

I'm still going to object to it, on the grounds that

(1) it's exposing an implementation detail that clients should not be
concerned with, and that we might change in future.  The name isn't
even well chosen --- if the argument is that it's useful to monitor
server uptime, why isn't the name "pg_server_uptime"?

(2) if your server is crashing often enough that postmaster start
time isn't an adequate substitute, you have way worse problems than
whether you can measure it.  I'd rather see us put effort into
fixing whatever the underlying problem is.

            regards, tom lane

On Sat, Feb 22, 2020 at 8:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > From my point of view criticism of this patch was addressed by
> > argument, that pg_shmem_init_time() allows to calculate the server
> > uptime [1].  This is very basic information, which is reasonable to
> > get without log files parsing.  It's more than year since [1] is
> > unanswered.  So, I'm going to push this if no objections.
>
> I'm still going to object to it, on the grounds that

OK!

> (1) it's exposing an implementation detail that clients should not be
> concerned with, and that we might change in future.  The name isn't
> even well chosen --- if the argument is that it's useful to monitor
> server uptime, why isn't the name "pg_server_uptime"?

Choosing a more user-friendly name sounds like good idea for me.
pg_server_uptime() sounds like it should return an interval.  I'd like
this function to calculate a diff between timestamps.  I would rather
delegate it to user side.  What about pg_server_up_since()?

> (2) if your server is crashing often enough that postmaster start
> time isn't an adequate substitute, you have way worse problems than
> whether you can measure it.

Well, it's enough server to crash once per postmaster run to make this
measure absolutely inadequate.  We have different reasons for server
crash, not all of them are exact bugs.  OOM killer can cause a crash,
and it doesn't seem feasible we can exclude this reason completely.

> I'd rather see us put effort into
> fixing whatever the underlying problem is.

This is monitoring problem vs fixing problem tradeoff.  We had similar
for lwlock wait monitoring.  Ideally we should make our code never
stuck on lwlock, but that's not feasible.  So, we got lwlock wait
monitoring for problem diagnosis.  I think now we're discussing
similar issue.  Ideally, postgres should never crash.  But that's too
hard to achieve.  But we can easily get better monitoring on server
crashes and that's useful.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Feb 22, 2020 at 10:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm still going to object to it, on the grounds that
>
> (1) it's exposing an implementation detail that clients should not be
> concerned with, and that we might change in future.  The name isn't
> even well chosen --- if the argument is that it's useful to monitor
> server uptime, why isn't the name "pg_server_uptime"?
>
> (2) if your server is crashing often enough that postmaster start
> time isn't an adequate substitute, you have way worse problems than
> whether you can measure it.  I'd rather see us put effort into
> fixing whatever the underlying problem is.

I don't accept the first argument, because I think the chances that
we'll change our basic approach to this problem are indistinguishable
from zero.  A few years back, you criticized some idea of mine as
tantamount to rm -rf src/backend/optimizer, which you were not ready
to endorse. That proposal was surely vastly less ambitious than some
proposal which would remove the idea of shared memory reinitialization
after an unsafe backend exit.

I don't accept the second argument either, because it's a false
dichotomy. Adding this function won't reduce the amount of energy that
gets spent fixing crashes. There are always more crashes.

I realize that several other people voted against this proposal so I
don't think it's OK to commit a patch in this area unless some more
positive votes turn up, but I'm still +1 on the idea. Granted, we
don't want an infinite amount of clutter in the system catalogs, but I
have a hard time believing that this proposed function would get any
less use than the hyperbolic trig functions.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2/24/20 10:57 PM, Robert Haas wrote:
> On Sat, Feb 22, 2020 at 10:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm still going to object to it, on the grounds that
>>
>> (1) it's exposing an implementation detail that clients should not be
>> concerned with, and that we might change in future.  The name isn't
>> even well chosen --- if the argument is that it's useful to monitor
>> server uptime, why isn't the name "pg_server_uptime"?
>>
>> (2) if your server is crashing often enough that postmaster start
>> time isn't an adequate substitute, you have way worse problems than
>> whether you can measure it.  I'd rather see us put effort into
>> fixing whatever the underlying problem is.
> 
> I don't accept the first argument, because I think the chances that
> we'll change our basic approach to this problem are indistinguishable
> from zero.  A few years back, you criticized some idea of mine as
> tantamount to rm -rf src/backend/optimizer, which you were not ready
> to endorse. That proposal was surely vastly less ambitious than some
> proposal which would remove the idea of shared memory reinitialization
> after an unsafe backend exit.
> 
> I don't accept the second argument either, because it's a false
> dichotomy. Adding this function won't reduce the amount of energy that
> gets spent fixing crashes. There are always more crashes.
> 
> I realize that several other people voted against this proposal so I
> don't think it's OK to commit a patch in this area unless some more
> positive votes turn up, but I'm still +1 on the idea. Granted, we
> don't want an infinite amount of clutter in the system catalogs, but I
> have a hard time believing that this proposed function would get any
> less use than the hyperbolic trig functions.

Marking this Returned with Feedback again since we are still at an impasse.

Regards,
-- 
-David
david@pgmasters.net

FWIW, I don't object for adding the feature like this (in other words
+1), since I see the advantages Alex mentioned is valid.  (Even though
most of our customers notices server restart by client
disconnections..)

At Wed, 8 Apr 2020 11:49:11 -0400, David Steele <david@pgmasters.net> wrote in 
> On 2/24/20 10:57 PM, Robert Haas wrote:
> > On Sat, Feb 22, 2020 at 10:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I'm still going to object to it, on the grounds that
> >>
> >> (1) it's exposing an implementation detail that clients should not be
> >> concerned with, and that we might change in future.  The name isn't
> >> even well chosen --- if the argument is that it's useful to monitor
> >> server uptime, why isn't the name "pg_server_uptime"?
> >>
> >> (2) if your server is crashing often enough that postmaster start
> >> time isn't an adequate substitute, you have way worse problems than
> >> whether you can measure it.  I'd rather see us put effort into
> >> fixing whatever the underlying problem is.
> > I don't accept the first argument, because I think the chances that
> > we'll change our basic approach to this problem are indistinguishable
> > from zero.  A few years back, you criticized some idea of mine as
> > tantamount to rm -rf src/backend/optimizer, which you were not ready
> > to endorse. That proposal was surely vastly less ambitious than some
> > proposal which would remove the idea of shared memory reinitialization
> > after an unsafe backend exit.

I'm not sure the name is good, mainly for the reason of
user-friendliness. Alexander's proposal "pg_server_up_since()"
returning absolute time makes sense to me.

> > I don't accept the second argument either, because it's a false
> > dichotomy. Adding this function won't reduce the amount of energy that
> > gets spent fixing crashes. There are always more crashes.
> > I realize that several other people voted against this proposal so I
> > don't think it's OK to commit a patch in this area unless some more
> > positive votes turn up, but I'm still +1 on the idea. Granted, we
> > don't want an infinite amount of clutter in the system catalogs, but I
> > have a hard time believing that this proposed function would get any
> > less use than the hyperbolic trig functions.

I'm on Robert and Alexander side about this.  I don't think
introducing this necessarily means we are obliged to accept all
requests for "last $anything" for other events, like introducing the
hyperbolic functions doesn't mean to accept all new functions alike,
maybe.

> Marking this Returned with Feedback again since we are still at an
> impasse.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center