Re: Add exclusive backup deprecation notes to documentation

From: David Steele <david(at)pgmasters(dot)net>
To: Magnus Hagander <magnus(at)hagander(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Martín Marqués <martin(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Add exclusive backup deprecation notes to documentation
Date: 2019-03-20 12:29:35
Message-ID: d80dacb2-06c8-9ba9-3828-0e203f6cf74a@pgmasters.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 3/8/19 6:08 AM, Magnus Hagander wrote:
> On Thu, Mar 7, 2019 at 5:35 PM Michael Paquier <michael(at)paquier(dot)xyz
> <mailto:michael(at)paquier(dot)xyz>> 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?

I went with Michael's suggestion. Attached is a new patch.

I also think we should set a flag and throw the error below this if/else
block. This is a rather large message and maintaining two copies of it
is not ideal.

Please note that there have been objections to the patch later in this
thread by Peter and Robert. I'm not very interested in watering down
the documentation changes as Peter suggests, but I think at the very
least we should commit the added hints in the error message. For many
users this error will be their first point of contact with the
backup_label issue/behavior.

Regards,
--
-David
david(at)pgmasters(dot)net

Attachment Content-Type Size
exclusive-backup-deprecation-doc-v04.patch text/plain 4.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-03-20 12:54:51 Re: Offline enabling/disabling of data checksums
Previous Message Alexander Kuzmenkov 2019-03-20 12:20:35 Re: Removing unneeded self joins