Thread: [HACKERS] pg_control_recovery() return value when not in recovery
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) postgres[14388][1]=# SELECT pg_is_in_recovery(); ┌───────────────────┐ │ pg_is_in_recovery │ ├───────────────────┤ │ f │ └───────────────────┘ (1 row) Wouldn't it be more accurate to return NULLs here? 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 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
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
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