pg_control_recovery() return value when not in recovery

On 18 September 2017 at 05:50, Andres Freund <andres@anarazel.de> wrote:

Hi,

Just noticed that we're returning the underlying values for
pg_control_recovery() without any checks:
postgres[14388][1]=# SELECT * FROM pg_control_recovery();
┌──────────────────────┬───────────────────────────┬──────────────────┬────────────────┬───────────────────────────────┐
│ min_recovery_end_lsn │ min_recovery_end_timeline │ backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │
├──────────────────────┼───────────────────────────┼──────────────────┼────────────────┼───────────────────────────────┤
│ 0/0 │ 0 │ 0/0 │ 0/0 │ f │
└──────────────────────┴───────────────────────────┴──────────────────┴────────────────┴───────────────────────────────┘
(1 row)

Yes, that would have made sense for these to be NULL

postgres[14388][1]=# SELECT pg_is_in_recovery();
┌───────────────────┐
│ pg_is_in_recovery │
├───────────────────┤
│ f │
└───────────────────┘
(1 row)

But not this, since it is a boolean and the answer is known.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:

On 18 September 2017 at 05:50, Andres Freund <andres@anarazel.de> wrote:

Hi,

Just noticed that we're returning the underlying values for
pg_control_recovery() without any checks:
postgres[14388][1]=# SELECT * FROM pg_control_recovery();
┌──────────────────────┬───────────────────────────┬──────────────────┬────────────────┬───────────────────────────────┐
│ min_recovery_end_lsn │ min_recovery_end_timeline │ backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │
├──────────────────────┼───────────────────────────┼──────────────────┼────────────────┼───────────────────────────────┤
│ 0/0 │ 0 │ 0/0 │ 0/0 │ f │
└──────────────────────┴───────────────────────────┴──────────────────┴────────────────┴───────────────────────────────┘
(1 row)

Yes, that would have made sense for these to be NULL

Yea, that's what I think was well. Joe, IIRC that's your code, do you
agree as well?

postgres[14388][1]=# SELECT pg_is_in_recovery();
┌───────────────────┐
│ pg_is_in_recovery │
├───────────────────┤
│ f │
└───────────────────┘
(1 row)

But not this, since it is a boolean and the answer is known.

Oh, that was just for reference, to show that the cluster isn't in
recovery...

- Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 3:29 PM, Andres Freund <andres@anarazel.de> wrote:

On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:

On 18 September 2017 at 05:50, Andres Freund <andres@anarazel.de> wrote:

Hi,

Just noticed that we're returning the underlying values for
pg_control_recovery() without any checks:
postgres[14388][1]=# SELECT * FROM pg_control_recovery();
┌──────────────────────┬───────────────────────────┬──────────────────┬────────────────┬───────────────────────────────┐
│ min_recovery_end_lsn │ min_recovery_end_timeline │ backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │
├──────────────────────┼───────────────────────────┼──────────────────┼────────────────┼───────────────────────────────┤
│ 0/0 │ 0 │ 0/0 │ 0/0 │ f │
└──────────────────────┴───────────────────────────┴──────────────────┴────────────────┴───────────────────────────────┘
(1 row)

Yes, that would have made sense for these to be NULL

Yea, that's what I think was well. Joe, IIRC that's your code, do you
agree as well?

+1 for NULLness here. That was a point not covered during the review
of the feature.
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sorry for the top post. Sounds reasonable to me. Cannot look closely until Tuesday or so.

Joe

On September 17, 2017 11:29:32 PM PDT, Andres Freund <andres@anarazel.de> wrote:

On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:

On 18 September 2017 at 05:50, Andres Freund <andres@anarazel.de>

wrote:

Hi,

Just noticed that we're returning the underlying values for
pg_control_recovery() without any checks:
postgres[14388][1]=# SELECT * FROM pg_control_recovery();

┌──────────────────────┬───────────────────────────┬──────────────────┬────────────────┬───────────────────────────────┐

│ min_recovery_end_lsn │ min_recovery_end_timeline │

backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │

├──────────────────────┼───────────────────────────┼──────────────────┼────────────────┼───────────────────────────────┤

│ 0/0 │ 0 │ 0/0

│ 0/0 │ f │

└──────────────────────┴───────────────────────────┴──────────────────┴────────────────┴───────────────────────────────┘

(1 row)

Yes, that would have made sense for these to be NULL

Yea, that's what I think was well. Joe, IIRC that's your code, do you
agree as well?

postgres[14388][1]=# SELECT pg_is_in_recovery();
┌───────────────────┐
│ pg_is_in_recovery │
├───────────────────┤
│ f │
└───────────────────┘
(1 row)

But not this, since it is a boolean and the answer is known.

Oh, that was just for reference, to show that the cluster isn't in
recovery...

- Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On 09/17/2017 11:29 PM, Andres Freund wrote:

On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:

On 18 September 2017 at 05:50, Andres Freund <andres@anarazel.de> wrote:

Hi,

Just noticed that we're returning the underlying values for
pg_control_recovery() without any checks:
postgres[14388][1]=# SELECT * FROM pg_control_recovery();
┌──────────────────────┬───────────────────────────┬──────────────────┬────────────────┬───────────────────────────────┐
│ min_recovery_end_lsn │ min_recovery_end_timeline │ backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │
├──────────────────────┼───────────────────────────┼──────────────────┼────────────────┼───────────────────────────────┤
│ 0/0 │ 0 │ 0/0 │ 0/0 │ f │
└──────────────────────┴───────────────────────────┴──────────────────┴────────────────┴───────────────────────────────┘
(1 row)

Yes, that would have made sense for these to be NULL

Yea, that's what I think was well. Joe, IIRC that's your code, do you
agree as well?

Sorry for the slow response, but thinking back on this now, the idea of
these functions, in my mind at least, was to provide as close to the
same output as possible to what pg_controldata outputs. So:

# pg_controldata
...
Minimum recovery ending location: 0/0
Min recovery ending loc's timeline: 0
Backup start location: 0/0
Backup end location: 0/0
End-of-backup record required: no
...

So if we make a change here, do we also change pg_controldata?

Joe

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

On 2017-10-13 16:31:37 -0700, Joe Conway wrote:

On 09/17/2017 11:29 PM, Andres Freund wrote:

On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:

On 18 September 2017 at 05:50, Andres Freund <andres@anarazel.de> wrote:

Hi,

Just noticed that we're returning the underlying values for
pg_control_recovery() without any checks:
postgres[14388][1]=# SELECT * FROM pg_control_recovery();
┌──────────────────────┬───────────────────────────┬──────────────────┬────────────────┬───────────────────────────────┐
│ min_recovery_end_lsn │ min_recovery_end_timeline │ backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │
├──────────────────────┼───────────────────────────┼──────────────────┼────────────────┼───────────────────────────────┤
│ 0/0 │ 0 │ 0/0 │ 0/0 │ f │
└──────────────────────┴───────────────────────────┴──────────────────┴────────────────┴───────────────────────────────┘
(1 row)

Yes, that would have made sense for these to be NULL

Yea, that's what I think was well. Joe, IIRC that's your code, do you
agree as well?

Sorry for the slow response, but thinking back on this now, the idea of
these functions, in my mind at least, was to provide as close to the
same output as possible to what pg_controldata outputs. So:

# pg_controldata
...
Minimum recovery ending location: 0/0
Min recovery ending loc's timeline: 0
Backup start location: 0/0
Backup end location: 0/0
End-of-backup record required: no
...

So if we make a change here, do we also change pg_controldata?

I'm unconvince that they need to be kept that close. For one, text
output doesn't have the concept of NULLs. Secondly, pg_controldata
output also the cluster state at the same time.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Oct 14, 2017 at 8:31 AM, Joe Conway <mail@joeconway.com> wrote:

Sorry for the slow response, but thinking back on this now, the idea of
these functions, in my mind at least, was to provide as close to the
same output as possible to what pg_controldata outputs. So:

# pg_controldata
...
Minimum recovery ending location: 0/0
Min recovery ending loc's timeline: 0
Backup start location: 0/0
Backup end location: 0/0
End-of-backup record required: no
...

So if we make a change here, do we also change pg_controldata?

For a lot of folks on this list, it is clear that things like
InvalidXLogRecPtr map to 0/0, but what of end-users? Couldn't we
consider marking those fields as "undefined" for example. "invalid"
would mean that the state of the cluster is incorrect, so I am not
sure if that is most adapted.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 13, 2017 at 7:31 PM, Joe Conway <mail@joeconway.com> wrote:

Sorry for the slow response, but thinking back on this now, the idea of
these functions, in my mind at least, was to provide as close to the
same output as possible to what pg_controldata outputs.

I think that's a good goal.

So if we make a change here, do we also change pg_controldata?

I think it would make more sense to leave both as they are and
consider writing more documentation.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Oct 13, 2017 at 7:31 PM, Joe Conway <mail@joeconway.com> wrote:

Sorry for the slow response, but thinking back on this now, the idea of
these functions, in my mind at least, was to provide as close to the
same output as possible to what pg_controldata outputs.

I think that's a good goal.

So if we make a change here, do we also change pg_controldata?

I think it would make more sense to leave both as they are and
consider writing more documentation.

+1. Changing already-shipped behavior seems more disruptive than
this is worth.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers