Add exclusive backup deprecation notes to documentation

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

Add exclusive backup deprecation notes to documentation

David Steele
Hackers,

It has been noted on multiple threads, such as [1], that it would be
good to have additional notes in the documentation to explain why
exclusive backups have been deprecated and why they should be avoided
when possible.

This patch attempts to document the limitations of the exclusive mode.

--
-David
[hidden email]

[1]
https://www.postgresql.org/message-id/flat/ac7339ca-3718-3c93-929f-99e725d1172c%40pgmasters.net

exclusive-backup-deprecation-doc-v01.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Laurenz Albe
David Steele wrote:
> This patch attempts to document the limitations of the exclusive mode.

Thanks!

> +   <para>
> +     The primary issue with the exclusive method is that the
> +     <filename>backup_label</filename> file is written into the data directory
> +     when <function>pg_start_backup</function> is called and remains until
> +     <function>pg_stop_backup</function> is called.  If
> +     <productname>PostgreSQL</productname> or the host terminates abnormally

There should be a comma at the end of this line.

> +     then <filename>backup_label</filename> will be left in the data directory
> +     and <productname>PostgreSQL</productname> will not start. A log message

You should say "*may* not start", because it will if the WAL segment is still there.

> +     recommends that <filename>backup_label</filename> be removed if not
> +     restoring from a backup.
> +   </para>

Yours,
Laurenz Albe


Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

David Steele
On 2/28/19 6:08 PM, Laurenz Albe wrote:

> David Steele wrote:
>> This patch attempts to document the limitations of the exclusive mode.
>
> Thanks!
>
>> +   <para>
>> +     The primary issue with the exclusive method is that the
>> +     <filename>backup_label</filename> file is written into the data directory
>> +     when <function>pg_start_backup</function> is called and remains until
>> +     <function>pg_stop_backup</function> is called.  If
>> +     <productname>PostgreSQL</productname> or the host terminates abnormally
>
> There should be a comma at the end of this line.
Fixed.

>> +     then <filename>backup_label</filename> will be left in the data directory
>> +     and <productname>PostgreSQL</productname> will not start. A log message
>
> You should say "*may* not start", because it will if the WAL segment is still there.

You are correct.  It's still pretty likely though so I went with "probably".

I added some extra language to the warning that gets emitted in the log.
  Users are more like to see that than the documentation.

I also addressed a comment from another thread by adding pg_basebackup
as .e.g. rather than or.

Thanks,
--
-David
[hidden email]

exclusive-backup-deprecation-doc-v02.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Laurenz Albe
David Steele wrote:
> I added some extra language to the warning that gets emitted in the log.
>   Users are more like to see that than the documentation.
>
> I also addressed a comment from another thread by adding pg_basebackup
> as .e.g. rather than or.

Looks good to me.

Yours,
Laurenz Albe


Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Martín Marqués-2
In reply to this post by David Steele
El vie., 1 de mar. de 2019 a la(s) 06:21, David Steele
([hidden email]) escribió:
>
>
> I also addressed a comment from another thread by adding pg_basebackup
> as .e.g. rather than or.

Thanks David,

This looks very good!

--
Martín Marqués                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Martín Marqués-2
In reply to this post by David Steele
El vie., 1 de mar. de 2019 a la(s) 06:21, David Steele
([hidden email]) escribió:
>
> I added some extra language to the warning that gets emitted in the log.
>   Users are more like to see that than the documentation.
>
> I also addressed a comment from another thread by adding pg_basebackup
> as .e.g. rather than or.

More awake, I gave this last patch a second read. Wording is good now.
No objections there at all.

I do think that paragraph 2 and 3 should be merged as it seems the
idea on the third is a continuation of what's in the second.

But even without that change, I believe this patch is good for commit.

Regards,

--
Martín Marqués                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Peter Eisentraut-6
In reply to this post by David Steele
Please follow the 1-space indentation in the documentation files.

I think the style of mentioning all the problems in a note before the
actual description is a bit backwards.

The layout of the section should be:

- This is what it does.

- Here are some comparisons with other methods.

- For this or that reason, it's deprecated.

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

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

David Steele
On 3/1/19 1:13 PM, Peter Eisentraut wrote:
> Please follow the 1-space indentation in the documentation files.

Whoops.  Will fix.

> I think the style of mentioning all the problems in a note before the
> actual description is a bit backwards.

In the case of an important note like this I think it should be right at
the top where people will see it.  Not everyone reads to the end.

--
-David
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Magnus Hagander-2
On Fri, Mar 1, 2019 at 2:07 PM David Steele <[hidden email]> wrote:
On 3/1/19 1:13 PM, Peter Eisentraut wrote:
> Please follow the 1-space indentation in the documentation files.

Whoops.  Will fix.

> I think the style of mentioning all the problems in a note before the
> actual description is a bit backwards.

In the case of an important note like this I think it should be right at
the top where people will see it.  Not everyone reads to the end

Maybe have the first note say "This method is deprecated bceause it has serious risks (see bellow)" and then list the actual risks at the end? 

--
Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Laurenz Albe
Magnus Hagander wrote:
> Maybe have the first note say "This method is deprecated bceause it has serious
> risks (see bellow)" and then list the actual risks at the end?

Good idea.  That may attract the attention of the dogs among the readers.

Yours,
Laurenz Albe


Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

David Steele
On 3/1/19 3:14 PM, Laurenz Albe wrote:
> Magnus Hagander wrote:
>> Maybe have the first note say "This method is deprecated bceause it has serious
>> risks (see bellow)" and then list the actual risks at the end?
>
> Good idea.  That may attract the attention of the dogs among the readers.

OK, here's a new version that splits the deprecation notes from the
discussion of risks.  I also fixed the indentation.

Thanks,
--
-David
[hidden email]

exclusive-backup-deprecation-doc-v03.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Michael Paquier-2
On Thu, Mar 07, 2019 at 11:33:20AM +0200, David Steele wrote:
> OK, here's a new version that splits the deprecation notes from the
> discussion of risks.  I also fixed the indentation.

The documentation part looks fine to me.  Just one nit regarding the
error hint.

> - errhint("If you are not restoring from a backup, try removing the file \"%s/backup_label\".", DataDir)));
> + errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" and add recovery options to \"%s/postgresql.auto.conf\".\n"

Here do we really want to recommend adding options to
postgresql.auto.conf?  This depends a lot on the solution integration
so I think that this hint could actually confuse some users because it
implies that they kind of *have* to do so, which is not correct.  I
would recommend to be a bit more generic and just use "and add
necessary recovery configuration".

> + "If you are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
> + "Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if restoring from a backup.",

Fine for these two ones.

> + DataDir, DataDir, DataDir, DataDir)));

:)
--
Michael

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

Re: Add exclusive backup deprecation notes to documentation

Magnus Hagander-2
On Thu, Mar 7, 2019 at 5:35 PM Michael Paquier <[hidden email]> wrote:
On Thu, Mar 07, 2019 at 11:33:20AM +0200, David Steele wrote:
> OK, here's a new version that splits the deprecation notes from the
> discussion of risks.  I also fixed the indentation.

The documentation part looks fine to me.  Just one nit regarding the
error hint.

> -     errhint("If you are not restoring from a backup, try removing the file \"%s/backup_label\".", DataDir)));
> +     errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" and add recovery options to \"%s/postgresql.auto.conf\".\n"

Here do we really want to recommend adding options to
postgresql.auto.conf?  This depends a lot on the solution integration
so I think that this hint could actually confuse some users because it
implies that they kind of *have* to do so, which is not correct.  I
would recommend to be a bit more generic and just use "and add
necessary recovery configuration".

Agreed, I think we should never tell people to "add recovery options to postgresql.auto.conf". Becuase they should never do that manually. If we want to suggest people use postgresql.auto.conf surely they should be using ALTER SYSTEM SET. Which of course doesn't work in this case, since postgrsql isn't running yet.

So yeah either that, or say "add to postgresql.conf" without the auto?

--
Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Peter Eisentraut-6
In reply to this post by David Steele
On 2019-03-07 10:33, David Steele wrote:
> On 3/1/19 3:14 PM, Laurenz Albe wrote:
>> Magnus Hagander wrote:
>>> Maybe have the first note say "This method is deprecated bceause it has serious
>>> risks (see bellow)" and then list the actual risks at the end?
>>
>> Good idea.  That may attract the attention of the dogs among the readers.
>
> OK, here's a new version that splits the deprecation notes from the
> discussion of risks.  I also fixed the indentation.

The documentation changes appear to continue the theme from the other
thread that the exclusive backup mode is terrible and everyone should
feel bad about it.  I don't think there is consensus about that.

I do welcome a more precise description of the handling of backup_label
and a better hint in the error message.  I think we haven't gotten to
the final shape there yet, especially for the latter.  I suggest to
focus on that.

The other changes repeat points already made in nearby documentation.

I think it would be helpful to frame the documentation in a way to
suggest that the nonexclusive mode is more for automation and more
sophisticated tools and the exclusive mode is more for manual or simple
scripted use.

If we do think that the exclusive mode will be removed in PG13, then I
don't think we need further documentation changes.  It already says it's
deprecated, and we don't need to justify that at length.  But again, I'm
not convinced that that will happen.

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

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Robert Haas
On Mon, Mar 18, 2019 at 8:33 AM Peter Eisentraut
<[hidden email]> wrote:
> The documentation changes appear to continue the theme from the other
> thread that the exclusive backup mode is terrible and everyone should
> feel bad about it.  I don't think there is consensus about that.

+1.

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

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Stephen Frost
In reply to this post by Peter Eisentraut-6
Greetings,

* Peter Eisentraut ([hidden email]) wrote:

> On 2019-03-07 10:33, David Steele wrote:
> > On 3/1/19 3:14 PM, Laurenz Albe wrote:
> >> Magnus Hagander wrote:
> >>> Maybe have the first note say "This method is deprecated bceause it has serious
> >>> risks (see bellow)" and then list the actual risks at the end?
> >>
> >> Good idea.  That may attract the attention of the dogs among the readers.
> >
> > OK, here's a new version that splits the deprecation notes from the
> > discussion of risks.  I also fixed the indentation.
>
> The documentation changes appear to continue the theme from the other
> thread that the exclusive backup mode is terrible and everyone should
> feel bad about it.  I don't think there is consensus about that.
I don't view it as up for much debate.  The exclusive backup mode is
quite bad.

> I do welcome a more precise description of the handling of backup_label
> and a better hint in the error message.  I think we haven't gotten to
> the final shape there yet, especially for the latter.  I suggest to
> focus on that.

There isn't a way to handle the backup_label in a sane way when it's
created by the server in the data directory, which is why the
non-exclusive mode explicitly doesn't do that.

> I think it would be helpful to frame the documentation in a way to
> suggest that the nonexclusive mode is more for automation and more
> sophisticated tools and the exclusive mode is more for manual or simple
> scripted use.

I don't agree with this at all, that's not the reason the two exist nor
were they ever developed with the intent that one is for the 'simple'
case and one is for the 'automated' case.  Trying to wedge them into
that framework strikes me as simply trying to sweep the serious issues
under the rug and I don't agree with that- if we are going to continue
to have this, we need to make it clear what the issues are.  Sadly, we
will still have users who don't actually read the docs that carefully
and get bit by the exclusive backup mode because they didn't appreciate
the issues, but we will continue to have that until we finally remove
the exclusive mode.

> If we do think that the exclusive mode will be removed in PG13, then I
> don't think we need further documentation changes.  It already says it's
> deprecated, and we don't need to justify that at length.  But again, I'm
> not convinced that that will happen.

There were at least a few comments made on this thread that it wasn't
made clear enough in the documentation that it's deprecated.  Saying
that we already deprecated it doesn't change that and doesn't do
anything to actually address those concerns.  Continuing to carry
forward these two modes makes further progress in this area difficult
and unlikely to happen, and that's disappointing.

Thanks!

Stephen

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

Re: Add exclusive backup deprecation notes to documentation

Robert Haas
On Mon, Mar 18, 2019 at 11:13 PM Stephen Frost <[hidden email]> wrote:
> I don't view it as up for much debate.

In other words, you're not willing to listen to what other people
think about this issue.

I can't say I haven't noticed.

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

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Stephen Frost
Greetings,

* Robert Haas ([hidden email]) wrote:
> On Mon, Mar 18, 2019 at 11:13 PM Stephen Frost <[hidden email]> wrote:
> > I don't view it as up for much debate.
>
> In other words, you're not willing to listen to what other people
> think about this issue.

I have listened, but unfortunately the discussion just revolves around
"oh, it isn't actually all that bad", which, no, isn't something that's
going to sway my opinion.

I suppose some might view that as being principled.

Thanks!

Stephen

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

Re: Add exclusive backup deprecation notes to documentation

Andres Freund
Hi,

On 2019-03-18 23:35:07 -0400, Stephen Frost wrote:

> * Robert Haas ([hidden email]) wrote:
> > On Mon, Mar 18, 2019 at 11:13 PM Stephen Frost <[hidden email]> wrote:
> > > I don't view it as up for much debate.
> >
> > In other words, you're not willing to listen to what other people
> > think about this issue.
>
> I have listened, but unfortunately the discussion just revolves around
> "oh, it isn't actually all that bad", which, no, isn't something that's
> going to sway my opinion.

I think you're right about the original issue. But at some point you
just gotta settle for not everyone agreeing with you (and me). Just
pushing forward hard won't achieve anything.  Nobody says you need to
agree with people saying "it's not all that bad", but that doesn't mean
you should just try to push forward at full speed.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: Add exclusive backup deprecation notes to documentation

Stephen Frost
Greetings,

* Andres Freund ([hidden email]) wrote:

> On 2019-03-18 23:35:07 -0400, Stephen Frost wrote:
> > * Robert Haas ([hidden email]) wrote:
> > > On Mon, Mar 18, 2019 at 11:13 PM Stephen Frost <[hidden email]> wrote:
> > > > I don't view it as up for much debate.
> > >
> > > In other words, you're not willing to listen to what other people
> > > think about this issue.
> >
> > I have listened, but unfortunately the discussion just revolves around
> > "oh, it isn't actually all that bad", which, no, isn't something that's
> > going to sway my opinion.
>
> I think you're right about the original issue. But at some point you
> just gotta settle for not everyone agreeing with you (and me). Just
> pushing forward hard won't achieve anything.  Nobody says you need to
> agree with people saying "it's not all that bad", but that doesn't mean
> you should just try to push forward at full speed.
This thread didn't exactly pop out of nowhere..  It's a result of the
prior discussion, with the goal of at least improving the situation by
documenting the issues and trying to make it clearer that the exclusive
mode is deprecated to hopefully provide a chance that we'll actually
remove it at some point in the future as being dangerous.

That's a pretty large step back from "let's rip it out for v12", and yet
people are now pushing back against even that, which is definitely
frustrating, and saying that I'm still pushing forward 'at full speed'
definitely comes across as not really considering that this started out
with "rip it out for v12" and has now regressed back to a documentation
patch.

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
12