Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Kevin Grittner]

On Mon, Apr 25, 2016 at 9:11 AM, Fujii Masao  wrote:

> PostgreSQL resource agent for Pacemaker removes backup_label in
> the case of failover to prevent the standby server from failing
> to recover from the crash.

Yikes!  I hope that the providers of Pacemaker document that they
are using a technique which can silently corrupt the cluster.

http://tbeitr.blogspot.com/2015/07/deleting-backuplabel-on-restore-will.html

> This is not so neat solution, but they could live with the
> problem for a long time, so far.

It can work sometimes.  It can also produce corruption which will
silently return incorrect results from queries for a long time
before some other symptom of the corruption surfaces.

--
Kevin Grittner
EDB: 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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 4/24/16 11:49 AM, Stephen Frost wrote:
> * Magnus Hagander (mag...@hagander.net) wrote:
>> On Sun, Apr 24, 2016 at 5:37 AM, Bruce Momjian  wrote:
>>
>>> On Fri, Apr 22, 2016 at 11:53:46AM -0400, Robert Haas wrote:
>>>
 Frankly, I think that's right.  It is one thing to say that the new
 method is preferred - +1 for that.  But the old method is going to
 continue to be used by many people for a long time, and in some cases
 will be superior.  That's not something we can deprecate, unless I'm
 misunderstanding the situation.
>>>
>>> I agree with Robert.  One the one hand we are saying pg_stop_backup()
>>> doesn't work well in psql because you get those two file contents output
>>> that you have to write, and on the other hand we are saying we are going
>>> to deprecate the method that does work well in psql?  I must be missing
>>> something too, as that makes no sense.
>>
>> I don't agree. I don't see how "making a backup using psql" is more
>> important than "making a backup without potentially dangerous sideeffects".
>> But if others don't agree, could one of you at least provide an example of
>> how you'd like the docs to read in a way that doesn't deprecate the unsafe
>> way but still informs the user properly?
> 
> I'm with Magnus on this, primairly because I've come to understand just
> how dangerous the old backup method is.  That method *should* be
> deprecated and discouraged.  A backup method which means your database
> doesn't restart properly if the system crashes during the backup is
> *bad*.

+1

> Perhaps we can look at improving psql to make it easier to use it for
> the new backup method but, honestly, all these hackish scripts to do
> backups aren't doing a lot of things that a real backup solution needs
> to do.  Improving psql for this is a bit like new paint on the titanic.

Personally I think we do the users a disservice by implying that backup
is as simple as calling two functions and copying the files.  Oh, and
don't forget to include WAL!

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Mon, Apr 25, 2016 at 4:11 PM, Fujii Masao  wrote:

> On Thu, Apr 21, 2016 at 2:32 AM, Magnus Hagander 
> wrote:
> > On Wed, Apr 20, 2016 at 1:12 AM, Fujii Masao 
> wrote:
> >>
> >> On Sun, Apr 17, 2016 at 1:22 AM, Magnus Hagander 
> >> wrote:
> >> > On Wed, Apr 13, 2016 at 4:07 AM, Noah Misch 
> wrote:
> >> >>
> >> >> On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
> >> >> > On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch 
> >> >> > wrote:
> >> >> > > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
> > Yes, it's a bit annoying. But it's something you can handle. Unlike the
> > problems that exist with an exclusive backup which you *cannot* handle
> from
> > the backup script/commands.
>
> I know many systems which are currently running well in production and
> handling that problem of an exclusive backup. For example, they use
> Pacemaker/Corosync to shared-disk HA configuration. PostgreSQL resource
> agent for Pacemaker removes backup_label in the case of failover to
> prevent the standby server from failing to recover from the crash.
> This is not so neat solution, but they could live with the problem for
> a long time, so far.
>
> I'm NOT against migrating their backup scripts so that new method is used,
> at the end. On the other hand, I think that we don't need to warn and rush
> them to do that at least until new method will be polished.
>

We have not put a timeframe on the deprecation. Given that, they can expect
to have multiple releases. And surely they don't use psql in the first
place, but an interface that gives them more control already, don't they?



> >> Another pain in a non-exclusive backup is to have to execute both
> >> pg_start_backup() and pg_stop_backup() on the same connection.
> >
> >
> > Pretty sure that's the same one you just listed?
>
> Yes, the sources of those pains are the same.
>
> I wonder if it's better to store the backup state information in shared
> memory, catalog, flat file or something instead of local memory
> so that we can execute pg_stop_backup() in different session from that
> pg_start_backup() is called. Maybe I'm missing something basic, but
> why do those functions need to be called in the same session at all?
>

So that we can automatically terminate backup mode if the client disappears.


> Yes, if you insist on using psql - which is not a good tool for this -
> then
> > yes. But you could for example make it a psql script that does something
> > along
> > SELECT pg_start_backup();
> > \! rsync ...
> > SELECT pg_stop_backup()
> >
> > Or as said above, drive it through a pipe instead. Or use an appropriate
> > client library.
>
> So, what's the appropriate client library for new backup method?
> It's better to document that.
>


Take your pick. *Any* client library will work fine. libpq, psycopg2, JDBC,
DBD::Pg, npgsql, doesn't matter.

psql is not a client *library*, it's a client.



>> How should we write those two values to different files when
> >> we execute pg_stop_backup() via psql? Whole output of
> >> pg_stop_backup() should be written to a transient file and
> >> it should be filtered and written to different two files by
> >> using some Linux commands? This also seems to make the backup
> >> script more complicated.
> >
> >
> > Yes, you would have to do that. And yes, psql is not a good tool for
> this.
> > So the solution is to not force yourself to use a tool that doesn't work
> > well for this problem.
>
> So it's better to document a tool which can work well or how to use psql
> for new backup method (i.e., how to create backup_label and tablespace_map
> files from the output of psql, of course it's also better to write
> the example).
>

We didn't have examples before, did we?

These APIs are directed at *tool developers*. Surely they can be trusted to
be able to write a file.

*End users* should not be using those APIs at all. They should be using
pg_basebackup, or one of the other tools. That's the "bigger documentation
rewrite" that was referenced in the thread, but nobody could commit to
getting done before beta.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Fujii Masao]

On Thu, Apr 21, 2016 at 2:32 AM, Magnus Hagander  wrote:
> On Wed, Apr 20, 2016 at 1:12 AM, Fujii Masao  wrote:
>>
>> On Sun, Apr 17, 2016 at 1:22 AM, Magnus Hagander 
>> wrote:
>> > On Wed, Apr 13, 2016 at 4:07 AM, Noah Misch  wrote:
>> >>
>> >> On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
>> >> > On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch 
>> >> > wrote:
>> >> > > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
>> >> > > > Well, if we *don't* do the rewrite before we release it, then we
>> >> > > > have to
>> >> > > > instead put information about the new version of the functions
>> >> > > > into
>> >> > > > the
>> >> > > old
>> >> > > > structure I think.
>> >> > > >
>> >> > > > So I think it's an open issue.
>> >> > >
>> >> > > Works for me...
>> >> > >
>> >> > > [This is a generic notification.]
>> >> > >
>> >> > > The above-described topic is currently a PostgreSQL 9.6 open item.
>> >> > > Magnus,
>> >> > > since you committed the patch believed to have created it, you own
>> >> > > this
>> >> > > open
>> >> > > item.  If that responsibility lies elsewhere, please let us know
>> >> > > whose
>> >> > > responsibility it is to fix this.  Since new open items may be
>> >> > > discovered
>> >> > > at
>> >> > > any time and I want to plan to have them all fixed well in advance
>> >> > > of
>> >> > > the
>> >> > > ship
>> >> > > date, I will appreciate your efforts toward speedy resolution.
>> >> > > Please
>> >> > > present, within 72 hours, a plan to fix the defect within seven
>> >> > > days
>> >> > > of
>> >> > > this
>> >> > > message.  Thanks.
>> >> > >
>> >> >
>> >> > I won't have time to do the bigger rewrite/reordeirng by then, but I
>> >> > can
>> >> > certainly commit to having the smaller updates done to cover the new
>> >> > functionality in less than a week. If nothing else, that'll be
>> >> > something
>> >> > for me to do on the flight over to pgconf.us.
>> >>
>> >> Thanks for that plan; it sounds good.
>> >
>> >
>> > Here's a suggested patch.
>> >
>> > There is some duplication between the non-exclusive and exclusive backup
>> > sections, but I wanted to make sure that each set of instructions can
>> > just
>> > be followed top-to-bottom.
>> >
>> > I've also removed some tips that aren't really necessary as part of the
>> > step-by-step instructions in order to keep things from exploding in
>> > size.
>> >
>> > Finally, I've changed references to "backup dump" to just be "backup",
>> > because it's confusing to call them something with dumps in when it's
>> > not
>> > pg_dump. Enough that I got partially confused myself while editing...
>> >
>> > Comments?
>>
>> +Low level base backups can be made in a non-exclusive or an exclusive
>> +way. The non-exclusive method is recommended and the exclusive one
>> will
>> +at some point be deprecated and removed.
>>
>> I don't object to add a non-exclusive mode of low level backup,
>> but I disagree to mark an exclusive backup as deprecated at least
>> until we can alleviate some pains that a non-exclusive mode causes.
>
>
> Note that we have not marked them as deprecated. We're just giving warnings
> that they will be deprecated.
>
>>
>>
>> One example of the pain, in a non-exclusive backup, we need to keep
>> the IDLE connection which was used to execute pg_start_backup(),
>> until the end of backup. Of course a backup can take a very
>> long time. In this case the IDLE connection also needs to remain
>> for such a long time. If it's accidentally terminated (e.g., because
>> of IDLE connection), the backup fails and needs to be taken again
>> from the beginning.
>
>
>
> Yes, it's a bit annoying. But it's something you can handle. Unlike the
> problems that exist with an exclusive backup which you *cannot* handle from
> the backup script/commands.

I know many systems which are currently running well in production and
handling that problem of an exclusive backup. For example, they use
Pacemaker/Corosync to shared-disk HA configuration. PostgreSQL resource
agent for Pacemaker removes backup_label in the case of failover to
prevent the standby server from failing to recover from the crash.
This is not so neat solution, but they could live with the problem for
a long time, so far.

I'm NOT against migrating their backup scripts so that new method is used,
at the end. On the other hand, I think that we don't need to warn and rush
them to do that at least until new method will be polished.

>> Another pain in a non-exclusive backup is to have to execute both
>> pg_start_backup() and pg_stop_backup() on the same connection.
>
>
> Pretty sure that's the same one you just listed?

Yes, the sources of those pains are the same.

I wonder if it's better to store the backup state information in shared
memory, catalog, flat file or something instead of local memory
so that we can execute pg_stop_backup() in different session from that
pg_start_backup() is called. Maybe I'm missing something basic, but
why do tho

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Craig Ringer]

On 24 April 2016 at 23:49, Stephen Frost  wrote:


>
> Fixing that means using something more complicated than the old method
> and that's a bit of a pain in psql, but that doesn't mean we should tell
> people that the old method is an acceptable approach.
>

+1

Frankly, I don't find doing backups with psql this way interesting. I'm a
bit astonished that anyone's doing it, really. Most of the time I'm doing
anything more complicated than pg_basebackup I've got more flexible tools
at hand than psql too.

\gset should be a workaround for anyone who really wants to do this in psql
for some reason.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Stephen Frost]

* Magnus Hagander (mag...@hagander.net) wrote:
> On Sun, Apr 24, 2016 at 5:37 AM, Bruce Momjian  wrote:
> 
> > On Fri, Apr 22, 2016 at 11:53:46AM -0400, Robert Haas wrote:
> > > On Wed, Apr 20, 2016 at 1:32 PM, Magnus Hagander 
> > wrote:
> > > > Note that we have not marked them as deprecated. We're just giving
> > warnings
> > > > that they will be deprecated.
> > >
> > > But I think that is being said here is that maybe they won't be
> > > deprecated, at least not any time soon.  And therefore maybe we
> > > shouldn't say so.
> > >
> > > Frankly, I think that's right.  It is one thing to say that the new
> > > method is preferred - +1 for that.  But the old method is going to
> > > continue to be used by many people for a long time, and in some cases
> > > will be superior.  That's not something we can deprecate, unless I'm
> > > misunderstanding the situation.
> >
> > I agree with Robert.  One the one hand we are saying pg_stop_backup()
> > doesn't work well in psql because you get those two file contents output
> > that you have to write, and on the other hand we are saying we are going
> > to deprecate the method that does work well in psql?  I must be missing
> > something too, as that makes no sense.
> 
> I don't agree. I don't see how "making a backup using psql" is more
> important than "making a backup without potentially dangerous sideeffects".
> But if others don't agree, could one of you at least provide an example of
> how you'd like the docs to read in a way that doesn't deprecate the unsafe
> way but still informs the user properly?

I'm with Magnus on this, primairly because I've come to understand just
how dangerous the old backup method is.  That method *should* be
deprecated and discouraged.  A backup method which means your database
doesn't restart properly if the system crashes during the backup is
*bad*.

Fixing that means using something more complicated than the old method
and that's a bit of a pain in psql, but that doesn't mean we should tell
people that the old method is an acceptable approach.

Perhaps we can look at improving psql to make it easier to use it for
the new backup method but, honestly, all these hackish scripts to do
backups aren't doing a lot of things that a real backup solution needs
to do.  Improving psql for this is a bit like new paint on the titanic.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Sun, Apr 24, 2016 at 5:37 AM, Bruce Momjian  wrote:

> On Fri, Apr 22, 2016 at 11:53:46AM -0400, Robert Haas wrote:
> > On Wed, Apr 20, 2016 at 1:32 PM, Magnus Hagander 
> wrote:
> > > Note that we have not marked them as deprecated. We're just giving
> warnings
> > > that they will be deprecated.
> >
> > But I think that is being said here is that maybe they won't be
> > deprecated, at least not any time soon.  And therefore maybe we
> > shouldn't say so.
> >
> > Frankly, I think that's right.  It is one thing to say that the new
> > method is preferred - +1 for that.  But the old method is going to
> > continue to be used by many people for a long time, and in some cases
> > will be superior.  That's not something we can deprecate, unless I'm
> > misunderstanding the situation.
>
> I agree with Robert.  One the one hand we are saying pg_stop_backup()
> doesn't work well in psql because you get those two file contents output
> that you have to write, and on the other hand we are saying we are going
> to deprecate the method that does work well in psql?  I must be missing
> something too, as that makes no sense.
>

I don't agree. I don't see how "making a backup using psql" is more
important than "making a backup without potentially dangerous sideeffects".
But if others don't agree, could one of you at least provide an example of
how you'd like the docs to read in a way that doesn't deprecate the unsafe
way but still informs the user properly?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Bruce Momjian]

On Fri, Apr 22, 2016 at 11:53:46AM -0400, Robert Haas wrote:
> On Wed, Apr 20, 2016 at 1:32 PM, Magnus Hagander  wrote:
> > Note that we have not marked them as deprecated. We're just giving warnings
> > that they will be deprecated.
> 
> But I think that is being said here is that maybe they won't be
> deprecated, at least not any time soon.  And therefore maybe we
> shouldn't say so.
> 
> Frankly, I think that's right.  It is one thing to say that the new
> method is preferred - +1 for that.  But the old method is going to
> continue to be used by many people for a long time, and in some cases
> will be superior.  That's not something we can deprecate, unless I'm
> misunderstanding the situation.

I agree with Robert.  One the one hand we are saying pg_stop_backup()
doesn't work well in psql because you get those two file contents output
that you have to write, and on the other hand we are saying we are going
to deprecate the method that does work well in psql?  I must be missing
something too, as that makes no sense.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Robert Haas]

On Wed, Apr 20, 2016 at 1:32 PM, Magnus Hagander  wrote:
> Note that we have not marked them as deprecated. We're just giving warnings
> that they will be deprecated.

But I think that is being said here is that maybe they won't be
deprecated, at least not any time soon.  And therefore maybe we
shouldn't say so.

Frankly, I think that's right.  It is one thing to say that the new
method is preferred - +1 for that.  But the old method is going to
continue to be used by many people for a long time, and in some cases
will be superior.  That's not something we can deprecate, unless I'm
misunderstanding the situation.

-- 
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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Apr 20, 2016 at 1:12 AM, Fujii Masao  wrote:

> On Sun, Apr 17, 2016 at 1:22 AM, Magnus Hagander 
> wrote:
> > On Wed, Apr 13, 2016 at 4:07 AM, Noah Misch  wrote:
> >>
> >> On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
> >> > On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch 
> wrote:
> >> > > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
> >> > > > Well, if we *don't* do the rewrite before we release it, then we
> >> > > > have to
> >> > > > instead put information about the new version of the functions
> into
> >> > > > the
> >> > > old
> >> > > > structure I think.
> >> > > >
> >> > > > So I think it's an open issue.
> >> > >
> >> > > Works for me...
> >> > >
> >> > > [This is a generic notification.]
> >> > >
> >> > > The above-described topic is currently a PostgreSQL 9.6 open item.
> >> > > Magnus,
> >> > > since you committed the patch believed to have created it, you own
> >> > > this
> >> > > open
> >> > > item.  If that responsibility lies elsewhere, please let us know
> whose
> >> > > responsibility it is to fix this.  Since new open items may be
> >> > > discovered
> >> > > at
> >> > > any time and I want to plan to have them all fixed well in advance
> of
> >> > > the
> >> > > ship
> >> > > date, I will appreciate your efforts toward speedy resolution.
> Please
> >> > > present, within 72 hours, a plan to fix the defect within seven days
> >> > > of
> >> > > this
> >> > > message.  Thanks.
> >> > >
> >> >
> >> > I won't have time to do the bigger rewrite/reordeirng by then, but I
> can
> >> > certainly commit to having the smaller updates done to cover the new
> >> > functionality in less than a week. If nothing else, that'll be
> something
> >> > for me to do on the flight over to pgconf.us.
> >>
> >> Thanks for that plan; it sounds good.
> >
> >
> > Here's a suggested patch.
> >
> > There is some duplication between the non-exclusive and exclusive backup
> > sections, but I wanted to make sure that each set of instructions can
> just
> > be followed top-to-bottom.
> >
> > I've also removed some tips that aren't really necessary as part of the
> > step-by-step instructions in order to keep things from exploding in size.
> >
> > Finally, I've changed references to "backup dump" to just be "backup",
> > because it's confusing to call them something with dumps in when it's not
> > pg_dump. Enough that I got partially confused myself while editing...
> >
> > Comments?
>
> +Low level base backups can be made in a non-exclusive or an exclusive
> +way. The non-exclusive method is recommended and the exclusive one
> will
> +at some point be deprecated and removed.
>
> I don't object to add a non-exclusive mode of low level backup,
> but I disagree to mark an exclusive backup as deprecated at least
> until we can alleviate some pains that a non-exclusive mode causes.
>

Note that we have not marked them as deprecated. We're just giving warnings
that they will be deprecated.


>
> One example of the pain, in a non-exclusive backup, we need to keep
> the IDLE connection which was used to execute pg_start_backup(),
> until the end of backup. Of course a backup can take a very
> long time. In this case the IDLE connection also needs to remain
> for such a long time. If it's accidentally terminated (e.g., because
> of IDLE connection), the backup fails and needs to be taken again
> from the beginning.
>


Yes, it's a bit annoying. But it's something you can handle. Unlike the
problems that exist with an exclusive backup which you *cannot* handle from
the backup script/commands.



>
> Another pain in a non-exclusive backup is to have to execute both
> pg_start_backup() and pg_stop_backup() on the same connection.
>

Pretty sure that's the same one you just listed?



> Please imagine the case where psql is used to execute those two
> backup functions (I believe that there are many users who do this).
> For example,
>
> psql -c "SELECT pg_start_backup()"
> rsync, cp, tar, storage backup, or something
> psql -c "SELECT pg_stop_backup()"
>
> A non-exclusive backup breaks the above very simple steps because
> two backup functions are executed on different connections.

So, how should we modify the steps for a non-exclusive backup?
>

You should at least not use psql in that way. You need to open psql in a
pipe and drive it through that. Or use a more appropriate client.


Basically we need to pause psql after pg_start_backup(), signal it
> to resume after the copy of database cluster is taken, and make
> it execute pg_stop_backup(). I'm afraid that the backup script
> will be complicated because of this pain of non-exclusive backup.
>

Yes, if you insist on using psql - which is not a good tool for this - then
yes. But you could for example make it a psql script that does something
along
SELECT pg_start_backup();
\! rsync ...
SELECT pg_stop_backup()

Or as said above, drive it through a pipe instead. Or use an appropriate
client library.

The bott

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Fujii Masao]

On Sun, Apr 17, 2016 at 1:22 AM, Magnus Hagander  wrote:
> On Wed, Apr 13, 2016 at 4:07 AM, Noah Misch  wrote:
>>
>> On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
>> > On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch  wrote:
>> > > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
>> > > > Well, if we *don't* do the rewrite before we release it, then we
>> > > > have to
>> > > > instead put information about the new version of the functions into
>> > > > the
>> > > old
>> > > > structure I think.
>> > > >
>> > > > So I think it's an open issue.
>> > >
>> > > Works for me...
>> > >
>> > > [This is a generic notification.]
>> > >
>> > > The above-described topic is currently a PostgreSQL 9.6 open item.
>> > > Magnus,
>> > > since you committed the patch believed to have created it, you own
>> > > this
>> > > open
>> > > item.  If that responsibility lies elsewhere, please let us know whose
>> > > responsibility it is to fix this.  Since new open items may be
>> > > discovered
>> > > at
>> > > any time and I want to plan to have them all fixed well in advance of
>> > > the
>> > > ship
>> > > date, I will appreciate your efforts toward speedy resolution.  Please
>> > > present, within 72 hours, a plan to fix the defect within seven days
>> > > of
>> > > this
>> > > message.  Thanks.
>> > >
>> >
>> > I won't have time to do the bigger rewrite/reordeirng by then, but I can
>> > certainly commit to having the smaller updates done to cover the new
>> > functionality in less than a week. If nothing else, that'll be something
>> > for me to do on the flight over to pgconf.us.
>>
>> Thanks for that plan; it sounds good.
>
>
> Here's a suggested patch.
>
> There is some duplication between the non-exclusive and exclusive backup
> sections, but I wanted to make sure that each set of instructions can just
> be followed top-to-bottom.
>
> I've also removed some tips that aren't really necessary as part of the
> step-by-step instructions in order to keep things from exploding in size.
>
> Finally, I've changed references to "backup dump" to just be "backup",
> because it's confusing to call them something with dumps in when it's not
> pg_dump. Enough that I got partially confused myself while editing...
>
> Comments?

+Low level base backups can be made in a non-exclusive or an exclusive
+way. The non-exclusive method is recommended and the exclusive one will
+at some point be deprecated and removed.

I don't object to add a non-exclusive mode of low level backup,
but I disagree to mark an exclusive backup as deprecated at least
until we can alleviate some pains that a non-exclusive mode causes.

One example of the pain, in a non-exclusive backup, we need to keep
the IDLE connection which was used to execute pg_start_backup(),
until the end of backup. Of course a backup can take a very
long time. In this case the IDLE connection also needs to remain
for such a long time. If it's accidentally terminated (e.g., because
of IDLE connection), the backup fails and needs to be taken again
from the beginning.

Another pain in a non-exclusive backup is to have to execute both
pg_start_backup() and pg_stop_backup() on the same connection.
Please imagine the case where psql is used to execute those two
backup functions (I believe that there are many users who do this).
For example,

psql -c "SELECT pg_start_backup()"
rsync, cp, tar, storage backup, or something
psql -c "SELECT pg_stop_backup()"

A non-exclusive backup breaks the above very simple steps because
two backup functions are executed on different connections.
So, how should we modify the steps for a non-exclusive backup?
Basically we need to pause psql after pg_start_backup(), signal it
to resume after the copy of database cluster is taken, and make
it execute pg_stop_backup(). I'm afraid that the backup script
will be complicated because of this pain of non-exclusive backup.

+ The pg_stop_backup will return one row with three
+ values. The second of these fields should be written to a file named
+ backup_label in the root directory of the backup. The
+ third field should be written to a file named
+ tablespace_map unless the field is empty.

How should we write those two values to different files when
we execute pg_stop_backup() via psql? Whole output of
pg_stop_backup() should be written to a transient file and
it should be filtered and written to different two files by
using some Linux commands? This also seems to make the backup
script more complicated.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Noah Misch]

On Sat, Apr 16, 2016 at 06:22:47PM +0200, Magnus Hagander wrote:
> > On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
> > > I won't have time to do the bigger rewrite/reordeirng by then, but I can
> > > certainly commit to having the smaller updates done to cover the new
> > > functionality in less than a week.

> There is some duplication between the non-exclusive and exclusive backup
> sections, but I wanted to make sure that each set of instructions can just
> be followed top-to-bottom.
> 
> I've also removed some tips that aren't really necessary as part of the
> step-by-step instructions in order to keep things from exploding in size.
> 
> Finally, I've changed references to "backup dump" to just be "backup",
> because it's confusing to call them something with dumps in when it's not
> pg_dump. Enough that I got partially confused myself while editing...
> 
> Comments?

I scanned this briefly, and it looks reasonable.  I recommend committing it
forthwith.

> *** a/doc/src/sgml/backup.sgml
> --- b/doc/src/sgml/backup.sgml
> ***
> *** 818,823  test ! -f /mnt/server/archivedir/000100A90065 
> && cp pg_xlog/
> --- 818,838 
>   simple. It is very important that these steps are executed in
>   sequence, and that the success of a step is verified before
>   proceeding to the next step.
> +
> +
> + Low level base backups can be made in a non-exclusive or an exclusive
> + way. The non-exclusive method is recommended and the exclusive one will
> + at some point be deprecated and removed.

"I will deprecate X at some point" has the same effect as "I deprecate X now."
If you have no doubt you want to deprecate it, I advise plainer phrasing like,
"The exclusive method is deprecated and will eventually be removed."  That is
to say, just deprecate it right now.  If you have doubts, omit deprecation.


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Apr 13, 2016 at 4:07 AM, Noah Misch  wrote:

> On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
> > On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch  wrote:
> > > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
> > > > Well, if we *don't* do the rewrite before we release it, then we
> have to
> > > > instead put information about the new version of the functions into
> the
> > > old
> > > > structure I think.
> > > >
> > > > So I think it's an open issue.
> > >
> > > Works for me...
> > >
> > > [This is a generic notification.]
> > >
> > > The above-described topic is currently a PostgreSQL 9.6 open item.
> Magnus,
> > > since you committed the patch believed to have created it, you own this
> > > open
> > > item.  If that responsibility lies elsewhere, please let us know whose
> > > responsibility it is to fix this.  Since new open items may be
> discovered
> > > at
> > > any time and I want to plan to have them all fixed well in advance of
> the
> > > ship
> > > date, I will appreciate your efforts toward speedy resolution.  Please
> > > present, within 72 hours, a plan to fix the defect within seven days of
> > > this
> > > message.  Thanks.
> > >
> >
> > I won't have time to do the bigger rewrite/reordeirng by then, but I can
> > certainly commit to having the smaller updates done to cover the new
> > functionality in less than a week. If nothing else, that'll be something
> > for me to do on the flight over to pgconf.us.
>
> Thanks for that plan; it sounds good.
>

Here's a suggested patch.

There is some duplication between the non-exclusive and exclusive backup
sections, but I wanted to make sure that each set of instructions can just
be followed top-to-bottom.

I've also removed some tips that aren't really necessary as part of the
step-by-step instructions in order to keep things from exploding in size.

Finally, I've changed references to "backup dump" to just be "backup",
because it's confusing to call them something with dumps in when it's not
pg_dump. Enough that I got partially confused myself while editing...

Comments?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
*** a/doc/src/sgml/backup.sgml
--- b/doc/src/sgml/backup.sgml
***
*** 818,823  test ! -f /mnt/server/archivedir/000100A90065 && cp pg_xlog/
--- 818,838 
  simple. It is very important that these steps are executed in
  sequence, and that the success of a step is verified before
  proceeding to the next step.
+
+
+ Low level base backups can be made in a non-exclusive or an exclusive
+ way. The non-exclusive method is recommended and the exclusive one will
+ at some point be deprecated and removed.
+
+
+ Making a non-exclusive low level backup
+ 
+  A non-exclusive low level backup is one that allows other
+  concurrent backups to be running (both those started using
+  the same backup API and those started using
+  .
+ 
+ 

 
  
***
*** 826,857  test ! -f /mnt/server/archivedir/000100A90065 && cp pg_xlog/
 
 
  
!  Connect to the database as a user with rights to run pg_start_backup
!  (superuser, or a user who has been granted EXECUTE on the function)
!  and issue the command:
  
  SELECT pg_start_backup('label');
  
   where label is any string you want to use to uniquely
!  identify this backup operation.  (One good practice is to use the
!  full path where you intend to put the backup dump file.)
   pg_start_backup creates a backup label file,
   called backup_label, in the cluster directory with
!  information about your backup, including the start time and label
!  string.  The function also creates a tablespace map file,
   called tablespace_map, in the cluster directory with
!  information about tablespace symbolic links in pg_tblspc/
!  if one or more such link is present.  Both files are critical to the
   integrity of the backup, should you need to restore from it.
  
  
  
-  It does not matter which database within the cluster you connect to to
-  issue this command.  You can ignore the result returned by the function;
-  but if it reports an error, deal with that before proceeding.
- 
- 
- 
   By default, pg_start_backup can take a long time to finish.
   This is because it performs a checkpoint, and the I/O
   required for the checkpoint will be spread out over a significant
--- 841,970 
 
 
  
!  Connect to the server (it does not matter which database) as a user with
!  rights to run pg_start_backup (superuser, or a user who has been granted
!  EXECUTE on the function) and issue the command:
! 
! SELECT pg_start_backup('label', false, false);
! 
!  where label is any string you want to use to uniquely
!  identify this backup operation. The connection
! 

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Noah Misch]

On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
> On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch  wrote:
> > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
> > > Well, if we *don't* do the rewrite before we release it, then we have to
> > > instead put information about the new version of the functions into the
> > old
> > > structure I think.
> > >
> > > So I think it's an open issue.
> >
> > Works for me...
> >
> > [This is a generic notification.]
> >
> > The above-described topic is currently a PostgreSQL 9.6 open item.  Magnus,
> > since you committed the patch believed to have created it, you own this
> > open
> > item.  If that responsibility lies elsewhere, please let us know whose
> > responsibility it is to fix this.  Since new open items may be discovered
> > at
> > any time and I want to plan to have them all fixed well in advance of the
> > ship
> > date, I will appreciate your efforts toward speedy resolution.  Please
> > present, within 72 hours, a plan to fix the defect within seven days of
> > this
> > message.  Thanks.
> >
> 
> I won't have time to do the bigger rewrite/reordeirng by then, but I can
> certainly commit to having the smaller updates done to cover the new
> functionality in less than a week. If nothing else, that'll be something
> for me to do on the flight over to pgconf.us.

Thanks for that plan; it sounds good.


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch  wrote:

> On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
> > On Thu, Apr 7, 2016 at 5:15 AM, Noah Misch  wrote:
> > > Unless you especially want to self-impose the same tight resolution
> > > schedule
> > > that 9.6 regressions experience, let's move this to the "Non-bugs"
> section.
> > > Which do you prefer?  I don't think the opportunity for more
> documentation
> > > in
> > > light of 7117685 constitutes a regression, and I don't want "Open
> Issues"
> > > to
> > > double as a parking lot for slow-moving non-regressions.
> > >
> >
> > Well, if we *don't* do the rewrite before we release it, then we have to
> > instead put information about the new version of the functions into the
> old
> > structure I think.
> >
> > So I think it's an open issue.
>
> Works for me...
>
> [This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item.  Magnus,
> since you committed the patch believed to have created it, you own this
> open
> item.  If that responsibility lies elsewhere, please let us know whose
> responsibility it is to fix this.  Since new open items may be discovered
> at
> any time and I want to plan to have them all fixed well in advance of the
> ship
> date, I will appreciate your efforts toward speedy resolution.  Please
> present, within 72 hours, a plan to fix the defect within seven days of
> this
> message.  Thanks.
>

I won't have time to do the bigger rewrite/reordeirng by then, but I can
certainly commit to having the smaller updates done to cover the new
functionality in less than a week. If nothing else, that'll be something
for me to do on the flight over to pgconf.us.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Noah Misch]

On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
> On Thu, Apr 7, 2016 at 5:15 AM, Noah Misch  wrote:
> > Unless you especially want to self-impose the same tight resolution
> > schedule
> > that 9.6 regressions experience, let's move this to the "Non-bugs" section.
> > Which do you prefer?  I don't think the opportunity for more documentation
> > in
> > light of 7117685 constitutes a regression, and I don't want "Open Issues"
> > to
> > double as a parking lot for slow-moving non-regressions.
> >
> 
> Well, if we *don't* do the rewrite before we release it, then we have to
> instead put information about the new version of the functions into the old
> structure I think.
> 
> So I think it's an open issue.

Works for me...

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Magnus,
since you committed the patch believed to have created it, you own this open
item.  If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this.  Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution.  Please
present, within 72 hours, a plan to fix the defect within seven days of this
message.  Thanks.

> But maybe we should have a separate section
> on the open items list for documentation issues? I tihnk we've had that
> some times before.

Maybe.  If the list were starting to get crowded with doc-only items, that
would certainly have benefits.


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Robert Haas]

On Mon, Apr 11, 2016 at 5:22 AM, Magnus Hagander  wrote:
> Well, if we *don't* do the rewrite before we release it, then we have to
> instead put information about the new version of the functions into the old
> structure I think.

I think that you should have done that in the patch as committed.
Maybe you could take an hour and go do that now, and then you can do
the rewrite when you get to it.

Tracking open issues and getting them resolved is a lot of work, so it
kind of frustrates me that you committed this patch knowing that it
was going to create one when, with only slightly more work, you could
have avoided that.  Perhaps that is rigid and intolerant of me, but if
I had done that for even 25% of the patches I committed this
CommitFest, the open-issues list would be a bloodbath right now.

I apologize if this sounds harsh.  I don't mean to be a jerk.

-- 
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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Thu, Apr 7, 2016 at 5:15 AM, Noah Misch  wrote:

> On Wed, Apr 06, 2016 at 09:17:22AM +0200, Magnus Hagander wrote:
> > On Wed, Apr 6, 2016 at 6:42 AM, Noah Misch  wrote:
> > > On Tue, Apr 05, 2016 at 08:15:16PM +0200, Magnus Hagander wrote:
> > > > I've pushed this version, and also added the item from the Brussels
> > > > developer meeting to actually rewrite the main backup docs to the
> open
> > > > items so they are definitely not forgotten for 9.6.
> > >
> > > Here's that PostgreSQL 9.6 open item:
> > >
> > > * Update of backup documentation (assigned to Bruce at [
> > > https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting
> > > Brussels Developer Meeting], but others are surely allowed to work on
> it as
> > > well)
> > > ** Made required by 7117685461af50f50c03f43e6a622284c8d54694 since the
> > > current documentation is now incorrect
> > >
> > > Unless Bruce endorsed this development strategy, I think it unfair for
> > > commit
> > > 7117685 to impose a deadline on his backup.sgml project.  Did commit
> > > 7117685
> > >
> >
> > I agree that's a horrible wording. What it meant to imply was a deadline
> > for *me* to help take that off Bruce's shoulders before then while also
> > opening the doors for others to work on it should they want. Re-reading
> it
> > now I can see that's not at all what it says. I'll reword (or rather,
> > remove most of that, since it's been discussed separately and doesn't
> > actually need to go on the open items list which is a list and not a
> > discussion)
>
> Ahh.  Your original wording did admit your interpretation; I just didn't
> think
> of that interpretation.
>
> > > The chapter already does describe pg_basebackup before describing
> > > pg_start_backup; what else did the plan entail?
>
> > Recommending people to primarily use tried and tested ways of doing
> backups
> > (including a reference to a list, probably on the wiki, of known external
> > tools that "do things the right way", such as backrest or barman) rather
> > than focusing on examples that aren't necessarily even correct, and
> > certainly not complete.
> >
> > Mentioning the actual problems of exclusive base backups. (And obviously
> > talk about how using pg_start_backup/pg_stop_backup now makes it possible
> > to do "low level" backups without the risks of exclusive backups -- which
> > is the "incomplete" part from my note.
> >
> > More clearly outlining backup vs dump, and possibly (I can't actually
> > remember if we decided the second step) put base backups before pg_dump
> > since the topic is backups.
>
> Unless you especially want to self-impose the same tight resolution
> schedule
> that 9.6 regressions experience, let's move this to the "Non-bugs" section.
> Which do you prefer?  I don't think the opportunity for more documentation
> in
> light of 7117685 constitutes a regression, and I don't want "Open Issues"
> to
> double as a parking lot for slow-moving non-regressions.
>

Well, if we *don't* do the rewrite before we release it, then we have to
instead put information about the new version of the functions into the old
structure I think.

So I think it's an open issue. But maybe we should have a separate section
on the open items list for documentation issues? I tihnk we've had that
some times before.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Noah Misch]

On Wed, Apr 06, 2016 at 09:17:22AM +0200, Magnus Hagander wrote:
> On Wed, Apr 6, 2016 at 6:42 AM, Noah Misch  wrote:
> > On Tue, Apr 05, 2016 at 08:15:16PM +0200, Magnus Hagander wrote:
> > > I've pushed this version, and also added the item from the Brussels
> > > developer meeting to actually rewrite the main backup docs to the open
> > > items so they are definitely not forgotten for 9.6.
> >
> > Here's that PostgreSQL 9.6 open item:
> >
> > * Update of backup documentation (assigned to Bruce at [
> > https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting
> > Brussels Developer Meeting], but others are surely allowed to work on it as
> > well)
> > ** Made required by 7117685461af50f50c03f43e6a622284c8d54694 since the
> > current documentation is now incorrect
> >
> > Unless Bruce endorsed this development strategy, I think it unfair for
> > commit
> > 7117685 to impose a deadline on his backup.sgml project.  Did commit
> > 7117685
> >
> 
> I agree that's a horrible wording. What it meant to imply was a deadline
> for *me* to help take that off Bruce's shoulders before then while also
> opening the doors for others to work on it should they want. Re-reading it
> now I can see that's not at all what it says. I'll reword (or rather,
> remove most of that, since it's been discussed separately and doesn't
> actually need to go on the open items list which is a list and not a
> discussion)

Ahh.  Your original wording did admit your interpretation; I just didn't think
of that interpretation.

> > The chapter already does describe pg_basebackup before describing
> > pg_start_backup; what else did the plan entail?

> Recommending people to primarily use tried and tested ways of doing backups
> (including a reference to a list, probably on the wiki, of known external
> tools that "do things the right way", such as backrest or barman) rather
> than focusing on examples that aren't necessarily even correct, and
> certainly not complete.
> 
> Mentioning the actual problems of exclusive base backups. (And obviously
> talk about how using pg_start_backup/pg_stop_backup now makes it possible
> to do "low level" backups without the risks of exclusive backups -- which
> is the "incomplete" part from my note.
> 
> More clearly outlining backup vs dump, and possibly (I can't actually
> remember if we decided the second step) put base backups before pg_dump
> since the topic is backups.

Unless you especially want to self-impose the same tight resolution schedule
that 9.6 regressions experience, let's move this to the "Non-bugs" section.
Which do you prefer?  I don't think the opportunity for more documentation in
light of 7117685 constitutes a regression, and I don't want "Open Issues" to
double as a parking lot for slow-moving non-regressions.


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Apr 6, 2016 at 6:42 AM, Noah Misch  wrote:

> On Tue, Apr 05, 2016 at 08:15:16PM +0200, Magnus Hagander wrote:
> > I've pushed this version, and also added the item from the Brussels
> > developer meeting to actually rewrite the main backup docs to the open
> > items so they are definitely not forgotten for 9.6.
>
> Here's that PostgreSQL 9.6 open item:
>
> * Update of backup documentation (assigned to Bruce at [
> https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting
> Brussels Developer Meeting], but others are surely allowed to work on it as
> well)
> ** Made required by 7117685461af50f50c03f43e6a622284c8d54694 since the
> current documentation is now incorrect
>
> Unless Bruce endorsed this development strategy, I think it unfair for
> commit
> 7117685 to impose a deadline on his backup.sgml project.  Did commit
> 7117685
>

I agree that's a horrible wording. What it meant to imply was a deadline
for *me* to help take that off Bruce's shoulders before then while also
opening the doors for others to work on it should they want. Re-reading it
now I can see that's not at all what it says. I'll reword (or rather,
remove most of that, since it's been discussed separately and doesn't
actually need to go on the open items list which is a list and not a
discussion)

(And yes, I chatted with Bruce about doing that weeks ago, so he knows well
about it - just that's not what the entry says).



> indeed make the documentation "incorrect?"  Coverage in the Backup and
> Restore
> chapter would be a good thing, but I don't think that gap alone makes
> 7117685,
> with its reference-only documentation, incorrect.  Did 7117685 impair
> documentation in any other respect?


Probably incomplete is a better word there as well.



> Incidentally, I'm not clear on the extent of Bruce's plans to change backup
> documentation.  Relevant meeting note fragment:
>
>   Magnus: We need a new robust API fornon-exclusive backups
>   Simon: Keep but deprecate the existing API.
>   Need to find a better way to ensure users have the required xlog in
> backups
>   Craig: Our docs are in the wrong order. pg_basebackup should be first,
> ahead of manual methods.
>   Action point: Re-arrange backup docs page (Bruce)
>
> The chapter already does describe pg_basebackup before describing
> pg_start_backup; what else did the plan entail?
>
>
Recommending people to primarily use tried and tested ways of doing backups
(including a reference to a list, probably on the wiki, of known external
tools that "do things the right way", such as backrest or barman) rather
than focusing on examples that aren't necessarily even correct, and
certainly not complete.

Mentioning the actual problems of exclusive base backups. (And obviously
talk about how using pg_start_backup/pg_stop_backup now makes it possible
to do "low level" backups without the risks of exclusive backups -- which
is the "incomplete" part from my note.

More clearly outlining backup vs dump, and possibly (I can't actually
remember if we decided the second step) put base backups before pg_dump
since the topic is backups.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Craig Ringer]

On 6 April 2016 at 12:42, Noah Misch  wrote:


>
> The chapter already does describe pg_basebackup before describing
> pg_start_backup; what else did the plan entail?
>

http://www.postgresql.org/docs/current/static/backup.html


24.1. SQL Dump
24.1.1. Restoring the Dump
24.1.2. Using pg_dumpall
24.1.3. Handling Large Databases
24.2. File System Level Backup
24.3. Continuous Archiving and Point-in-Time Recovery (PITR)
24.3.1. Setting Up WAL Archiving
24.3.2. Making a Base Backup
24.3.3. Making a Base Backup Using the Low Level API
24.3.4. Recovering Using a Continuous Archive Backup
24.3.5. Timelines
24.3.6. Tips and Examples
24.3.7. Caveats

"File System Level Backup" should be last, and should link to pg_basebackup
as the strongly recommended method.

I'm not too keen on how pg_basebackup is only covered under continuous
archiving and PITR, but OTOH that's really how it's mostly used  and I
think most people who're doing physical backups are doing continuous
archiving. Or should be.

I'd also be inclined to add a hint in the sql dumps section to the effect
that while there's no incremental/differential pg_dump support, you can do
incrementals with continuous archiving.

In "Making a base backup" discussion of pg_basebackup should probably
mention that 'pg_basebackup -X stream' is required if you want a
self-contained base backup.

I think that'd address most of the confusion I've seen on occasion.

I'd love to spend several solid days reorganizing and updating those docs
some day, but I think that for now moving some chunks around will still
help.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Noah Misch]

On Tue, Apr 05, 2016 at 08:15:16PM +0200, Magnus Hagander wrote:
> I've pushed this version, and also added the item from the Brussels
> developer meeting to actually rewrite the main backup docs to the open
> items so they are definitely not forgotten for 9.6.

Here's that PostgreSQL 9.6 open item:

* Update of backup documentation (assigned to Bruce at 
[https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2016_Developer_Meeting Brussels 
Developer Meeting], but others are surely allowed to work on it as well)
** Made required by 7117685461af50f50c03f43e6a622284c8d54694 since the current 
documentation is now incorrect

Unless Bruce endorsed this development strategy, I think it unfair for commit
7117685 to impose a deadline on his backup.sgml project.  Did commit 7117685
indeed make the documentation "incorrect?"  Coverage in the Backup and Restore
chapter would be a good thing, but I don't think that gap alone makes 7117685,
with its reference-only documentation, incorrect.  Did 7117685 impair
documentation in any other respect?


Incidentally, I'm not clear on the extent of Bruce's plans to change backup
documentation.  Relevant meeting note fragment:

  Magnus: We need a new robust API fornon-exclusive backups
  Simon: Keep but deprecate the existing API.
  Need to find a better way to ensure users have the required xlog in backups
  Craig: Our docs are in the wrong order. pg_basebackup should be first, ahead 
of manual methods.
  Action point: Re-arrange backup docs page (Bruce)

The chapter already does describe pg_basebackup before describing
pg_start_backup; what else did the plan entail?

Thanks,
nm


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Tue, Apr 5, 2016 at 5:57 PM, Amit Kapila  wrote:

> On Tue, Apr 5, 2016 at 5:35 PM, Magnus Hagander 
> wrote:
>
>>
>>
>> On Mon, Apr 4, 2016 at 3:15 PM, Amit Kapila 
>> wrote:
>>
>>> On Mon, Apr 4, 2016 at 4:31 PM, Magnus Hagander 
>>> wrote:
>>>
 On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila 
 wrote:


> Also, I think below part of documentation for pg_start_backup() needs
> to be modified:
>
> 
>
> pg_start_backup accepts an
>
> arbitrary user-defined label for the backup.  (Typically this
> would be
>
> the name under which the backup dump file will be stored.)  The
> function
>
> writes a backup label file (backup_label) and, if
> there
>
> are any links in the pg_tblspc/ directory, a
> tablespace map
>
> file (tablespace_map) into the database cluster's
> data
>
> directory, performs a checkpoint, and then returns the backup's
> starting
>
> transaction log location as text.  The user can ignore this result
> value,
>
> but it is provided in case it is useful.
>

 That one definitely needs to be fixed, as it's part of the reference.
 Good spot.



> Similarly, there is a description for pg_stop_backup which needs to be
> modified.
>

 That's the one you're referring to in your first commend above, is it
 not? Or is there one more that you mean?


>>> I am referring to below part of docs in func.sgml
>>>
>>> 
>>>
>>> pg_stop_backup removes the label file and, if it
>>> exists,
>>>
>>> the tablespace_map file created by
>>>
>>> pg_start_backup, and creates a backup history file in
>>>
>>> the transaction log archive area.  The history file includes the
>>> label given to
>>>
>>> pg_start_backup, the starting and ending transaction
>>> log locations for
>>>
>>> the backup, and the starting and ending times of the backup.  The
>>> return
>>>
>>> value is the backup's ending transaction log location (which again
>>>
>>> can be ignored).  After recording the ending location, the current
>>>
>>> transaction log insertion
>>>
>>> point is automatically advanced to the next transaction log file, so
>>> that the
>>>
>>> ending transaction log file can be archived immediately to complete
>>> the backup.
>>>
>>>  
>>>
>>>
>> Ah, gotcha. Clearly I wasn't paying attention properly.
>>
>> PFA a better one (I think), also with the rename and added comments that
>> David was asking for.
>>
>>
> This version looks good.  Apart from review, I have done some tests
> (including regression test), everything seems to be fine.
>
>
I've pushed this version, and also added the item from the Brussels
developer meeting to actually rewrite the main backup docs to the open
items so they are definitely not forgotten for 9.6.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Amit Kapila]

On Tue, Apr 5, 2016 at 5:35 PM, Magnus Hagander  wrote:

>
>
> On Mon, Apr 4, 2016 at 3:15 PM, Amit Kapila 
> wrote:
>
>> On Mon, Apr 4, 2016 at 4:31 PM, Magnus Hagander 
>> wrote:
>>
>>> On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila 
>>> wrote:
>>>
>>>
 Also, I think below part of documentation for pg_start_backup() needs
 to be modified:

 

 pg_start_backup accepts an

 arbitrary user-defined label for the backup.  (Typically this would
 be

 the name under which the backup dump file will be stored.)  The
 function

 writes a backup label file (backup_label) and, if
 there

 are any links in the pg_tblspc/ directory, a
 tablespace map

 file (tablespace_map) into the database cluster's data

 directory, performs a checkpoint, and then returns the backup's
 starting

 transaction log location as text.  The user can ignore this result
 value,

 but it is provided in case it is useful.

>>>
>>> That one definitely needs to be fixed, as it's part of the reference.
>>> Good spot.
>>>
>>>
>>>
 Similarly, there is a description for pg_stop_backup which needs to be
 modified.

>>>
>>> That's the one you're referring to in your first commend above, is it
>>> not? Or is there one more that you mean?
>>>
>>>
>> I am referring to below part of docs in func.sgml
>>
>> 
>>
>> pg_stop_backup removes the label file and, if it exists,
>>
>> the tablespace_map file created by
>>
>> pg_start_backup, and creates a backup history file in
>>
>> the transaction log archive area.  The history file includes the
>> label given to
>>
>> pg_start_backup, the starting and ending transaction
>> log locations for
>>
>> the backup, and the starting and ending times of the backup.  The
>> return
>>
>> value is the backup's ending transaction log location (which again
>>
>> can be ignored).  After recording the ending location, the current
>>
>> transaction log insertion
>>
>> point is automatically advanced to the next transaction log file, so
>> that the
>>
>> ending transaction log file can be archived immediately to complete
>> the backup.
>>
>>  
>>
>>
> Ah, gotcha. Clearly I wasn't paying attention properly.
>
> PFA a better one (I think), also with the rename and added comments that
> David was asking for.
>
>
This version looks good.  Apart from review, I have done some tests
(including regression test), everything seems to be fine.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 4/5/16 8:05 AM, Magnus Hagander wrote:


PFA a better one (I think), also with the rename and added comments that
David was asking for.

Barring objections, I will apply this version.


This version looks good to me.

--
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Mon, Apr 4, 2016 at 3:15 PM, Amit Kapila  wrote:

> On Mon, Apr 4, 2016 at 4:31 PM, Magnus Hagander 
> wrote:
>
>> On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila 
>> wrote:
>>
>>
>>> Also, I think below part of documentation for pg_start_backup() needs to
>>> be modified:
>>>
>>> 
>>>
>>> pg_start_backup accepts an
>>>
>>> arbitrary user-defined label for the backup.  (Typically this would
>>> be
>>>
>>> the name under which the backup dump file will be stored.)  The
>>> function
>>>
>>> writes a backup label file (backup_label) and, if there
>>>
>>> are any links in the pg_tblspc/ directory, a
>>> tablespace map
>>>
>>> file (tablespace_map) into the database cluster's data
>>>
>>> directory, performs a checkpoint, and then returns the backup's
>>> starting
>>>
>>> transaction log location as text.  The user can ignore this result
>>> value,
>>>
>>> but it is provided in case it is useful.
>>>
>>
>> That one definitely needs to be fixed, as it's part of the reference.
>> Good spot.
>>
>>
>>
>>> Similarly, there is a description for pg_stop_backup which needs to be
>>> modified.
>>>
>>
>> That's the one you're referring to in your first commend above, is it
>> not? Or is there one more that you mean?
>>
>>
> I am referring to below part of docs in func.sgml
>
> 
>
> pg_stop_backup removes the label file and, if it exists,
>
> the tablespace_map file created by
>
> pg_start_backup, and creates a backup history file in
>
> the transaction log archive area.  The history file includes the label
> given to
>
> pg_start_backup, the starting and ending transaction log
> locations for
>
> the backup, and the starting and ending times of the backup.  The
> return
>
> value is the backup's ending transaction log location (which again
>
> can be ignored).  After recording the ending location, the current
>
> transaction log insertion
>
> point is automatically advanced to the next transaction log file, so
> that the
>
> ending transaction log file can be archived immediately to complete
> the backup.
>
>  
>
>
Ah, gotcha. Clearly I wasn't paying attention properly.

PFA a better one (I think), also with the rename and added comments that
David was asking for.

Barring objections, I will apply this version.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1bc9fbc..c6017e6 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17478,7 +17478,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_start_backup(label text , fast boolean )
+pg_start_backup(label text , fast boolean , exclusive boolean )
 
pg_lsn
Prepare for performing on-line backup (restricted to superusers or replication roles)
@@ -17488,7 +17488,14 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_stop_backup()
 
pg_lsn
-   Finish performing on-line backup (restricted to superusers or replication roles)
+   Finish performing exclusive on-line backup (restricted to superusers or replication roles)
+  
+  
+   
+pg_stop_backup(exclusive boolean)
+
+   setof record
+   Finish performing exclusive or non-exclusive on-line backup (restricted to superusers or replication roles)
   
   

@@ -17537,15 +17544,19 @@ SELECT set_config('log_statement_stats', 'off', false);

 

-pg_start_backup accepts an
-arbitrary user-defined label for the backup.  (Typically this would be
-the name under which the backup dump file will be stored.)  The function
-writes a backup label file (backup_label) and, if there
-are any links in the pg_tblspc/ directory, a tablespace map
-file (tablespace_map) into the database cluster's data
-directory, performs a checkpoint, and then returns the backup's starting
-transaction log location as text.  The user can ignore this result value,
-but it is provided in case it is useful.
+pg_start_backup accepts an arbitrary user-defined label for
+the backup.  (Typically this would be the name under which the backup dump
+file will be stored.) When used in exclusive mode, the function writes a
+backup label file (backup_label) and, if there are any links
+in the pg_tblspc/ directory, a tablespace map file
+(tablespace_map) into the database cluster's data directory,
+performs a checkpoint, and then returns the backup's starting transaction
+log location as text.  The user can ignore this result value, but it is
+provided in case it is useful. When used in non-exclusive mode, the
+contents of these files are instead returned by the
+pg_stop_backup function, and should be written to the backup
+by the caller.
+
 
 postgres=# select pg_start_backup('label_goes_here');
  pg_start_backup
@@ -17

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Amit Kapila]

On Mon, Apr 4, 2016 at 4:31 PM, Magnus Hagander  wrote:

> On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila 
> wrote:
>
>
>> Also, I think below part of documentation for pg_start_backup() needs to
>> be modified:
>>
>> 
>>
>> pg_start_backup accepts an
>>
>> arbitrary user-defined label for the backup.  (Typically this would be
>>
>> the name under which the backup dump file will be stored.)  The
>> function
>>
>> writes a backup label file (backup_label) and, if there
>>
>> are any links in the pg_tblspc/ directory, a tablespace
>> map
>>
>> file (tablespace_map) into the database cluster's data
>>
>> directory, performs a checkpoint, and then returns the backup's
>> starting
>>
>> transaction log location as text.  The user can ignore this result
>> value,
>>
>> but it is provided in case it is useful.
>>
>
> That one definitely needs to be fixed, as it's part of the reference. Good
> spot.
>
>
>
>> Similarly, there is a description for pg_stop_backup which needs to be
>> modified.
>>
>
> That's the one you're referring to in your first commend above, is it not?
> Or is there one more that you mean?
>
>
I am referring to below part of docs in func.sgml



pg_stop_backup removes the label file and, if it exists,

the tablespace_map file created by

pg_start_backup, and creates a backup history file in

the transaction log archive area.  The history file includes the label
given to

pg_start_backup, the starting and ending transaction log
locations for

the backup, and the starting and ending times of the backup.  The return

value is the backup's ending transaction log location (which again

can be ignored).  After recording the ending location, the current

transaction log insertion

point is automatically advanced to the next transaction log file, so
that the

ending transaction log file can be archived immediately to complete the
backup.

 



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 3/19/16 8:15 AM, Magnus Hagander wrote:


I've attached an updated patch, which is rebased on current master and
includes the oid fix.


I've now had a chance to go through this in detail and test thoroughly. 
 We had a lot of discussion about copying pg_control last and that led 
me to believe that there might be some sort of race condition with 
multiple backups running concurrently.


I tried every trick I could think of and everything worked as expected 
so as far as I can see this requirement only applies to backups taken 
from a standby which agrees with the comments in do_pg_stop_backup().


Note that I only tested backups against a master database since that's 
what is modified by this patch.  I'm still not entirely comfortable with 
how backups against a standby are done and it that case still think it 
would be best to write the stop wal location into backup_label, but as 
you said before that would be a completely separate patch.


So in the end I'm fine with the code as it stands with the exception of 
the pg_stop_backup2() naming.  I'd prefer a more verbose name here but 
I'm unable to come up with anything very good.  How about 
pg_stop_backup_v2()?  This function could also use a few more comments, 
at least at the top of the exclusive and non-exclusive blocks.


Except for those minor details I believe this is "ready for committer" 
unless anybody has an objection.


--
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila  wrote:

> On Sat, Mar 19, 2016 at 5:45 PM, Magnus Hagander 
> wrote:
>
>> On Wed, Mar 2, 2016 at 6:49 PM, Marco Nenciarini <
>> marco.nenciar...@2ndquadrant.it> wrote:
>>
>>>
>>> I've attached an updated patch, which is rebased on current master and
>> includes the oid fix.
>>
>>
>
> +   Finish performing exclusive on-line backup (restricted to
> superusers or replication roles)
>
> +  
>
> +  
>
> +   
>
> +pg_stop_backup(exclusive
> boolean)
>
> +
>
> +   setof record
>
> +   Finish performing exclusive or non-exclusive on-line backup
> (restricted to superusers or replication roles)
>
>
> Isn't it better to indicate that user needs to form a backup_label and
> tablespace_map file from the output of this API and those needs to be
> dropped into data directory?
>

I think that documentation should go in the "usage" part of the
documentation, and not the reference to the function itself.

This is the documentation that is not written yet of course. I was planinng
to wait for Bruce to finish his restructuring of the backup documentation
in general, but latest news on that was that it's several months away, so I
am going to go ahead and write it anyway, without waiting for that (or
possibly do the restructure at hte same time). That's where the process of
how to use these functions belong.


> Also, I think below part of documentation for pg_start_backup() needs to
> be modified:
>
> 
>
> pg_start_backup accepts an
>
> arbitrary user-defined label for the backup.  (Typically this would be
>
> the name under which the backup dump file will be stored.)  The
> function
>
> writes a backup label file (backup_label) and, if there
>
> are any links in the pg_tblspc/ directory, a tablespace
> map
>
> file (tablespace_map) into the database cluster's data
>
> directory, performs a checkpoint, and then returns the backup's
> starting
>
> transaction log location as text.  The user can ignore this result
> value,
>
> but it is provided in case it is useful.
>

That one definitely needs to be fixed, as it's part of the reference. Good
spot.



> Similarly, there is a description for pg_stop_backup which needs to be
> modified.
>

That's the one you're referring to in your first commend above, is it not?
Or is there one more that you mean?



> CREATE OR REPLACE FUNCTION
>
> -  pg_start_backup(label text, fast boolean DEFAULT false)
>
> +  pg_start_backup(label text, fast boolean DEFAULT false, exclusive
> boolean DEFAULT true)
>
>RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup';
>
>
> One thing, that might be slightly inconvenient for user is if he or she
> wants to use this API for non-exclusive backups then, they need to pass the
> value of second parameter as well which doesn't seem to be a big issue.
>

Well, they have to pass it *somehow*. The other option would be to have a
different function, which I think doesn't help at all. And we do *not* want
the behaviour of existing scripts to implicitly change, so we can't have
the default be non-exclusive.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Amit Kapila]

On Sat, Mar 19, 2016 at 5:45 PM, Magnus Hagander 
wrote:

> On Wed, Mar 2, 2016 at 6:49 PM, Marco Nenciarini <
> marco.nenciar...@2ndquadrant.it> wrote:
>
>>
>> I've attached an updated patch, which is rebased on current master and
> includes the oid fix.
>
>

+   Finish performing exclusive on-line backup (restricted to
superusers or replication roles)

+  

+  

+   

+pg_stop_backup(exclusive
boolean)

+

+   setof record

+   Finish performing exclusive or non-exclusive on-line backup
(restricted to superusers or replication roles)


Isn't it better to indicate that user needs to form a backup_label and
tablespace_map file from the output of this API and those needs to be
dropped into data directory?

Also, I think below part of documentation for pg_start_backup() needs to be
modified:



pg_start_backup accepts an

arbitrary user-defined label for the backup.  (Typically this would be

the name under which the backup dump file will be stored.)  The function

writes a backup label file (backup_label) and, if there

are any links in the pg_tblspc/ directory, a tablespace map

file (tablespace_map) into the database cluster's data

directory, performs a checkpoint, and then returns the backup's starting

transaction log location as text.  The user can ignore this result
value,

but it is provided in case it is useful.


Similarly, there is a description for pg_stop_backup which needs to be
modified.


CREATE OR REPLACE FUNCTION

-  pg_start_backup(label text, fast boolean DEFAULT false)

+  pg_start_backup(label text, fast boolean DEFAULT false, exclusive
boolean DEFAULT true)

   RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup';


One thing, that might be slightly inconvenient for user is if he or she
wants to use this API for non-exclusive backups then, they need to pass the
value of second parameter as well which doesn't seem to be a big issue.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Amit Kapila]

On Thu, Mar 31, 2016 at 5:49 PM, Magnus Hagander 
wrote:

> On Thu, Mar 31, 2016 at 4:00 AM, David Steele  wrote:
> So maybe we should at least start that way. And just document that
> clearly, and then use the patch that we have right now?
>
> Except we add so it still returns the stoppoint() as well (which is
> returned from the current pg_stop_backup, but not in the new one).
>
> We can always extend the function with more columns later if we need -
> it's changing the ones we have that's a big problem there :)
>
>
+1 for doing that way for now.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Thu, Mar 31, 2016 at 2:19 PM, Magnus Hagander 
wrote:

> On Thu, Mar 31, 2016 at 4:00 AM, David Steele  wrote:
>
>> On 3/30/16 4:18 AM, Magnus Hagander wrote:
>> >
>> > On Wed, Mar 30, 2016 at 4:10 AM, David Steele > > > wrote:
>> >
>> > This certainly looks like it would work but it raises the barrier
>> for
>> > implementing backups by quite a lot.  It's fine for backrest or
>> barman
>> > but it won't be pleasant for anyone who has home-grown scripts.
>> >
>> >
>> > How much does it really raise the bar, though?
>> >
>> > It would go from "copy all files and make damn sure you copy pg_control
>> > last, and rename it to pg_control.backup" to "take this binary blob you
>> > got from the server and write it to pg_control.backup"?
>> >
>> > Also, the target of these APIs is specifically the backup tools and not
>> > homewritten scripts.
>>
>> Then what would home-grown scripts use, the deprecated API that we know
>> has issues?
>>
>
> They should use either pg_basebackup/pg_receivexlog, or they should use a
> tool like backrest or barman.
>
>
>
>> > A simple shellscript will have trouble enough using
>> > it in the first place since it requires a persistent connection to the
>> > database.
>>
>> All that's required is to spawn a psql process.  I'm no shell expert but
>> that's simple enough.
>>
>
> Oh, it's certainly doable, but you also need to come back and talk to it
> at a later time (to call stop backup), and do something useful with a
> multiline output. None of that is particularly easy. Certainly doable, but
> it becomes the wrong tool for the job.
>
>
>
>> > But those scripts are likely broken anyway.
>>
>> Yes, probably.  Backup and especially archiving correctly are harder
>> than most people realize.
>>
>
> Exactly. Which is why we should discourage people from doing that, and
> instead point them to the tools that do the job right. This is the whole
> reason we're making this change in the first place.
>
>
>
>> > The main reason for Heikki to suggest this one over the other basic one
>> > is that it brings protection against the "backup script/program crashed
>> > halfway through but the user still tried to restore from that". They
>> will
>> > outright fail because there is no pg_control.backup in that case.
>>
>> But if we are going to make this complicated I'm not sure it's a big
>> deal just to require pg_control to be copied last.  pgBackRest already
>> does that and Barman probably does, too.
>>
>
> It does (I did doublecheck that at some point).
>
>
>
>> I don't see "don't copy pg_control" and "copy pg_control last" as all
>> that different in terms of complexity.
>>
>> pgBackRest also *restores* pg_control last which I think is pretty
>> important and would not be addressed by this solution.
>>
>
> So maybe we should at least start that way. And just document that
> clearly, and then use the patch that we have right now?
>
> Except we add so it still returns the stoppoint() as well (which is
> returned from the current pg_stop_backup, but not in the new one).
>

Eh, nevermind. We do already return the stoppoint. I was looking at the
wrong version of the patch. So basically that's current version of patch,
but with proper docs of course.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Thu, Mar 31, 2016 at 4:00 AM, David Steele  wrote:

> On 3/30/16 4:18 AM, Magnus Hagander wrote:
> >
> > On Wed, Mar 30, 2016 at 4:10 AM, David Steele  > > wrote:
> >
> > This certainly looks like it would work but it raises the barrier for
> > implementing backups by quite a lot.  It's fine for backrest or
> barman
> > but it won't be pleasant for anyone who has home-grown scripts.
> >
> >
> > How much does it really raise the bar, though?
> >
> > It would go from "copy all files and make damn sure you copy pg_control
> > last, and rename it to pg_control.backup" to "take this binary blob you
> > got from the server and write it to pg_control.backup"?
> >
> > Also, the target of these APIs is specifically the backup tools and not
> > homewritten scripts.
>
> Then what would home-grown scripts use, the deprecated API that we know
> has issues?
>

They should use either pg_basebackup/pg_receivexlog, or they should use a
tool like backrest or barman.



> > A simple shellscript will have trouble enough using
> > it in the first place since it requires a persistent connection to the
> > database.
>
> All that's required is to spawn a psql process.  I'm no shell expert but
> that's simple enough.
>

Oh, it's certainly doable, but you also need to come back and talk to it at
a later time (to call stop backup), and do something useful with a
multiline output. None of that is particularly easy. Certainly doable, but
it becomes the wrong tool for the job.



> > But those scripts are likely broken anyway.
>
> Yes, probably.  Backup and especially archiving correctly are harder
> than most people realize.
>

Exactly. Which is why we should discourage people from doing that, and
instead point them to the tools that do the job right. This is the whole
reason we're making this change in the first place.



> > The main reason for Heikki to suggest this one over the other basic one
> > is that it brings protection against the "backup script/program crashed
> > halfway through but the user still tried to restore from that". They will
> > outright fail because there is no pg_control.backup in that case.
>
> But if we are going to make this complicated I'm not sure it's a big
> deal just to require pg_control to be copied last.  pgBackRest already
> does that and Barman probably does, too.
>

It does (I did doublecheck that at some point).



> I don't see "don't copy pg_control" and "copy pg_control last" as all
> that different in terms of complexity.
>
> pgBackRest also *restores* pg_control last which I think is pretty
> important and would not be addressed by this solution.
>

So maybe we should at least start that way. And just document that clearly,
and then use the patch that we have right now?

Except we add so it still returns the stoppoint() as well (which is
returned from the current pg_stop_backup, but not in the new one).

We can always extend the function with more columns later if we need - it's
changing the ones we have that's a big problem there :)

Then we start this way and we can keep improving if necessary. But the
advantage of sticking to this for now is we don't have to touch the
recovery side at all. And we're pretty late in the cycle ATM.



> > If we
> > don't care about that, then we can go back to just saying "copy
> > pg_control last and we're done". But you yourself complained about that
> > requirement because it's too easy to get wrong (though you advocated
> > using backup_label to transfer the data over -- but that has the
> > potential for getting more complicated if we now or at any point in the
> > future want more than one field to transfer, for example).
>
> Perhaps.  I'm still not convinced that getting some information from
> backup_label and other information from pg_control is a good solution.
> I would rather write the recovery point into the backup_label and use
> that instead of the value in pg_control.  Text files are much easier to
> parse and push around accurately (and test).
>

What about all the *other* functionality that's then in pg_control?

We could do as Heikki at one point suggested in our chat as well which is
basically write all of pg_control out to the backup_label file, and
reconstruct it from that. But that makes that file a lot more complicated...

//Magnus

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 3/30/16 4:18 AM, Magnus Hagander wrote:
> 
> On Wed, Mar 30, 2016 at 4:10 AM, David Steele  > wrote:
> 
> This certainly looks like it would work but it raises the barrier for
> implementing backups by quite a lot.  It's fine for backrest or barman
> but it won't be pleasant for anyone who has home-grown scripts.
> 
> 
> How much does it really raise the bar, though?
> 
> It would go from "copy all files and make damn sure you copy pg_control
> last, and rename it to pg_control.backup" to "take this binary blob you
> got from the server and write it to pg_control.backup"?
> 
> Also, the target of these APIs is specifically the backup tools and not
> homewritten scripts.

Then what would home-grown scripts use, the deprecated API that we know
has issues?

> A simple shellscript will have trouble enough using
> it in the first place since it requires a persistent connection to the
> database. 

All that's required is to spawn a psql process.  I'm no shell expert but
that's simple enough.

> But those scripts are likely broken anyway.

Yes, probably.  Backup and especially archiving correctly are harder
than most people realize.

> <...>
> 
> The main reason for Heikki to suggest this one over the other basic one
> is that it brings protection against the "backup script/program crashed
> halfway through but the user still tried to restore from that". They will
> outright fail because there is no pg_control.backup in that case.

But if we are going to make this complicated I'm not sure it's a big
deal just to require pg_control to be copied last.  pgBackRest already
does that and Barman probably does, too.

I don't see "don't copy pg_control" and "copy pg_control last" as all
that different in terms of complexity.

pgBackRest also *restores* pg_control last which I think is pretty
important and would not be addressed by this solution.

> If we
> don't care about that, then we can go back to just saying "copy
> pg_control last and we're done". But you yourself complained about that
> requirement because it's too easy to get wrong (though you advocated
> using backup_label to transfer the data over -- but that has the
> potential for getting more complicated if we now or at any point in the
> future want more than one field to transfer, for example).

Perhaps.  I'm still not convinced that getting some information from
backup_label and other information from pg_control is a good solution.
I would rather write the recovery point into the backup_label and use
that instead of the value in pg_control.  Text files are much easier to
parse and push around accurately (and test).

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Mar 30, 2016 at 10:09 AM, Amit Kapila 
wrote:

> On Tue, Mar 29, 2016 at 11:39 PM, Magnus Hagander 
> wrote:
>
>> On Tue, Mar 29, 2016 at 6:40 PM, Magnus Hagander 
>> wrote:
>>
>>> So - I can definitely see the argument for returning the stop wal
>>> *location*. But I'm still not sure what the definition of the time would
>>> be? We can't return it before we know what it means...
>>>
>>
>>
>> I had a chat with Heikki, and here's another suggestion:
>>
>> 1. We don't touch the current exclusive backups at all, as previously
>> discussed, other than deprecating their use. For backwards compat.
>>
>> 2. For new backups, we return the contents of pg_control as a bytea from
>> pg_stop_backup(). We tell backup programs they are supposed to write this
>> out as pg_control.backup, *not* as pg_control.
>>
>> 3a. On recovery, if it's an exlcusive backup, we do as we did before.
>>
>> 3b. on recovery, in non-exclusive backups (determined from backup_label),
>> we check that pg_control.backup exists *and* that pg_control does *not*
>> exist.
>>
>
> Currently pg_control has been read before backup_label file, so as per
> this proposal do you want to change that?  If yes, I think that will make
> this patch more invasive with respect to handling of failure modes.  Also
> as David points out, I also feel that it will raise the bar for usage of
> this API.
>

Yes, we'd have to change that. I don't think it's going to be much more
invasive than reading part of it from pg_control and part of it from
backup_label, as suggested by David. It would be a bit more complicated
than what we have today - but it would move complication from user scripts
(that are likely to get it wrong) to a central place in the backend (where
we can be more certain that it's at least less wrong).


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Mar 30, 2016 at 4:10 AM, David Steele  wrote:

> On 3/29/16 2:09 PM, Magnus Hagander wrote:
>
> > I had a chat with Heikki, and here's another suggestion:
> >
> > 1. We don't touch the current exclusive backups at all, as previously
> > discussed, other than deprecating their use. For backwards compat.
> >
> > 2. For new backups, we return the contents of pg_control as a bytea from
> > pg_stop_backup(). We tell backup programs they are supposed to write
> > this out as pg_control.backup, *not* as pg_control.
> >
> > 3a. On recovery, if it's an exclusive backup, we do as we did before.
> >
> > 3b. on recovery, in non-exclusive backups (determined from
> > backup_label), we check that pg_control.backup exists *and* that
> > pg_control does *not* exist. That guards us reasonably against backup
> > programs that do the wrong thing, and we know we get the correct version
> > of pg_control.
> >
> > 4. (we can still add the stop location to the backup_label file in case
> > backup programs find it useful, but we don't use it in recovery)
> >
> > Thoughts about this approach?
>
> This certainly looks like it would work but it raises the barrier for
> implementing backups by quite a lot.  It's fine for backrest or barman
> but it won't be pleasant for anyone who has home-grown scripts.
>
>
How much does it really raise the bar, though?

It would go from "copy all files and make damn sure you copy pg_control
last, and rename it to pg_control.backup" to "take this binary blob you got
from the server and write it to pg_control.backup"?

Also, the target of these APIs is specifically the backup tools and not
homewritten scripts. A simple shellscript will have trouble enough using it
in the first place since it requires a persistent connection to the
database. But those scripts are likely broken anyway.

You can of course keep the current requirements which is just "copy
pg_control last", but if we do that we have zero way of checking that that
happened, and you may end up with subtly broken restores if the backup
software gets it wrong. (Of course it can get the rename/writeout thing
wrong as well, but that's going to be a lot more obvious if you're doing it
wrong).

The main reason for Heikki to suggest this one over the other basic one is
that it brings protection against the "backup script/program crashed
halfway through but the user still tried to restore from that".They will
outright fail becuase there is no pg_control.backup in that case. If we
don't care about that, then we can go back to just saying "copy pg_control
last and we're done". But you yourself complained about that requirement
because it's too easy to get wrong (though you advocated using backup_label
to transfer the data over -- but that has the potential for getting more
complicated if we now or at any point in the future want more than one
field to transfer, for example).


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Amit Kapila]

On Tue, Mar 29, 2016 at 11:39 PM, Magnus Hagander 
wrote:

> On Tue, Mar 29, 2016 at 6:40 PM, Magnus Hagander 
> wrote:
>
>> So - I can definitely see the argument for returning the stop wal
>> *location*. But I'm still not sure what the definition of the time would
>> be? We can't return it before we know what it means...
>>
>
>
> I had a chat with Heikki, and here's another suggestion:
>
> 1. We don't touch the current exclusive backups at all, as previously
> discussed, other than deprecating their use. For backwards compat.
>
> 2. For new backups, we return the contents of pg_control as a bytea from
> pg_stop_backup(). We tell backup programs they are supposed to write this
> out as pg_control.backup, *not* as pg_control.
>
> 3a. On recovery, if it's an exlcusive backup, we do as we did before.
>
> 3b. on recovery, in non-exclusive backups (determined from backup_label),
> we check that pg_control.backup exists *and* that pg_control does *not*
> exist.
>

Currently pg_control has been read before backup_label file, so as per this
proposal do you want to change that?  If yes, I think that will make this
patch more invasive with respect to handling of failure modes.  Also as
David points out, I also feel that it will raise the bar for usage of this
API.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 3/29/16 2:09 PM, Magnus Hagander wrote:

> I had a chat with Heikki, and here's another suggestion:
> 
> 1. We don't touch the current exclusive backups at all, as previously
> discussed, other than deprecating their use. For backwards compat.
> 
> 2. For new backups, we return the contents of pg_control as a bytea from
> pg_stop_backup(). We tell backup programs they are supposed to write
> this out as pg_control.backup, *not* as pg_control.
> 
> 3a. On recovery, if it's an exclusive backup, we do as we did before.
> 
> 3b. on recovery, in non-exclusive backups (determined from
> backup_label), we check that pg_control.backup exists *and* that
> pg_control does *not* exist. That guards us reasonably against backup
> programs that do the wrong thing, and we know we get the correct version
> of pg_control.
> 
> 4. (we can still add the stop location to the backup_label file in case
> backup programs find it useful, but we don't use it in recovery)
> 
> Thoughts about this approach?

This certainly looks like it would work but it raises the barrier for
implementing backups by quite a lot.  It's fine for backrest or barman
but it won't be pleasant for anyone who has home-grown scripts.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Tue, Mar 29, 2016 at 6:40 PM, Magnus Hagander 
wrote:

>
>
> On Tue, Mar 29, 2016 at 4:36 PM, David Steele  wrote:
>
>> On 3/22/16 12:31 PM, Magnus Hagander wrote:
>>
>> On Tue, Mar 22, 2016 at 5:27 PM, David Steele >> > wrote:
>>>
>> >
>>
>>> > Adding the stop time column should be a simple addition and I
>>> don't see
>>> > a problem with that. I think I misunderstood your original request
>>> on
>>> > that. Because you are just talking about returning a timestamptz
>>> with
>>> > the "right now" value for when you called pg_stop_backup()? Or to
>>> be
>>> > specific, just before pg_Stop_backup *finished*. Or do you mean
>>> when
>>> > pg_stop_backup() started?
>>>
>>> What would be ideal is the minimum time that could be used for
>>> PITR.  In
>>> an exclusive backup that's the time the end-of-backup record is
>>> written
>>> to WAL.  In a non-exlusive backup I'm not quite sure how that works.
>>>
>>
>> I guess I was hoping that you would know.  I fine with just getting the
>> current timestamp as is currently done in do_pg_stop_backup().  It's not
>> perfect but it will be pretty close.
>>
>> I thought some more about putting STOP_WAL_LOCATION into the backup label
>> and I think this is an important step.  Without that the recovery process
>> needs to use pg_control to determine when the database has reach
>> consistency and that will only work if pg_control was copied last.
>>
>> In summary, I think the patch should be updated to include stop_time as a
>> column and add STOP_WAL_LOCATION and STOP_TIME to backup_label.  The
>> recovery process should read STOP_WAL_LOCATION from backup_label and use
>> that to decide when the database has become consistent.
>>
>
> Is that the only reason we need pg_control to be copied last? I thought
> there were other reasons for that.
>
> I was chatting with Stephen about this earlier, and one thing we
> considered was that maybe we should return pg_control as a bytea field from
> pg_stop_backup() thereby strongly hinting to tools that they should write
> it out from there rather than copy it from the data directory (we can't
> stop them from copying it from the data directory like we do for
> backup_label of course, but if we return the contents and document that's
> how it should be done, it's pretty clear).
>
> But if we can actually remove the whole requirement on the order of the
> copy of pg_control by doing that it certainly seems worthwhile.
>
>
> So - I can definitely see the argument for returning the stop wal
> *location*. But I'm still not sure what the definition of the time would
> be? We can't return it before we know what it means...
>


I had a chat with Heikki, and here's another suggestion:

1. We don't touch the current exclusive backups at all, as previously
discussed, other than deprecating their use. For backwards compat.

2. For new backups, we return the contents of pg_control as a bytea from
pg_stop_backup(). We tell backup programs they are supposed to write this
out as pg_control.backup, *not* as pg_control.

3a. On recovery, if it's an exlcusive backup, we do as we did before.

3b. on recovery, in non-exclusive backups (determined from backup_label),
we check that pg_control.backup exists *and* that pg_control does *not*
exist. That guards us reasonably against backup programs that do the wrong
thing, and we know we get the correct version of pg_control.

4. (we can still add the stop location to the backup_label file in case
backup programs find it useful, but we don't use it in recovery)

Thoughts about this approach?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Tue, Mar 29, 2016 at 4:36 PM, David Steele  wrote:

> On 3/22/16 12:31 PM, Magnus Hagander wrote:
>
> On Tue, Mar 22, 2016 at 5:27 PM, David Steele > > wrote:
>>
> >
>
>> > Adding the stop time column should be a simple addition and I don't
>> see
>> > a problem with that. I think I misunderstood your original request
>> on
>> > that. Because you are just talking about returning a timestamptz
>> with
>> > the "right now" value for when you called pg_stop_backup()? Or to be
>> > specific, just before pg_Stop_backup *finished*. Or do you mean when
>> > pg_stop_backup() started?
>>
>> What would be ideal is the minimum time that could be used for PITR.
>> In
>> an exclusive backup that's the time the end-of-backup record is
>> written
>> to WAL.  In a non-exlusive backup I'm not quite sure how that works.
>>
>
> I guess I was hoping that you would know.  I fine with just getting the
> current timestamp as is currently done in do_pg_stop_backup().  It's not
> perfect but it will be pretty close.
>
> I thought some more about putting STOP_WAL_LOCATION into the backup label
> and I think this is an important step.  Without that the recovery process
> needs to use pg_control to determine when the database has reach
> consistency and that will only work if pg_control was copied last.
>
> In summary, I think the patch should be updated to include stop_time as a
> column and add STOP_WAL_LOCATION and STOP_TIME to backup_label.  The
> recovery process should read STOP_WAL_LOCATION from backup_label and use
> that to decide when the database has become consistent.
>

Is that the only reason we need pg_control to be copied last? I thought
there were other reasons for that.

I was chatting with Stephen about this earlier, and one thing we considered
was that maybe we should return pg_control as a bytea field from
pg_stop_backup() thereby strongly hinting to tools that they should write
it out from there rather than copy it from the data directory (we can't
stop them from copying it from the data directory like we do for
backup_label of course, but if we return the contents and document that's
how it should be done, it's pretty clear).

But if we can actually remove the whole requirement on the order of the
copy of pg_control by doing that it certainly seems worthwhile.


So - I can definitely see the argument for returning the stop wal
*location*. But I'm still not sure what the definition of the time would
be? We can't return it before we know what it means...


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 3/22/16 12:31 PM, Magnus Hagander wrote:


On Tue, Mar 22, 2016 at 5:27 PM, David Steele mailto:da...@pgmasters.net>> wrote:

>

> Adding the stop time column should be a simple addition and I don't see
> a problem with that. I think I misunderstood your original request on
> that. Because you are just talking about returning a timestamptz with
> the "right now" value for when you called pg_stop_backup()? Or to be
> specific, just before pg_Stop_backup *finished*. Or do you mean when
> pg_stop_backup() started?

What would be ideal is the minimum time that could be used for PITR.  In
an exclusive backup that's the time the end-of-backup record is written
to WAL.  In a non-exlusive backup I'm not quite sure how that works.


I guess I was hoping that you would know.  I fine with just getting the 
current timestamp as is currently done in do_pg_stop_backup().  It's not 
perfect but it will be pretty close.


I thought some more about putting STOP_WAL_LOCATION into the backup 
label and I think this is an important step.  Without that the recovery 
process needs to use pg_control to determine when the database has reach 
consistency and that will only work if pg_control was copied last.


In summary, I think the patch should be updated to include stop_time as 
a column and add STOP_WAL_LOCATION and STOP_TIME to backup_label.  The 
recovery process should read STOP_WAL_LOCATION from backup_label and use 
that to decide when the database has become consistent.


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Craig Ringer]

On 23 March 2016 at 00:14, Magnus Hagander  wrote:


> Doing it in the backup label file is obviously a different target, where
> we might need to consider backwards compatibility, Should we?
>

As part of the failover slots a few folks at 2ndQ looked into whether tools
would get upset by the addition of new label file entries. Everything we
found used regexps to find what they wanted and didn't care in the
slightest about new lines. So I wouldn't worry too much.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Tue, Mar 22, 2016 at 5:27 PM, David Steele  wrote:

> On 3/22/16 12:14 PM, Magnus Hagander wrote:
> > On Tue, Mar 22, 2016 at 5:08 PM, David Steele  > > wrote:
> >
> > On 3/19/16 8:15 AM, Magnus Hagander wrote:
> >
> > > I've attached an updated patch, which is rebased on current master
> and
> > > includes the oid fix.
> >
> > Before doing a thorough review of this patch there are a few points I
> > would like to consider:
> >
> > * I think it's really important to provide the stop time in some
> fashion
> > when using this new technique.  I would prefer a new column to be
> > returned from pg_stop_backup() but I could live with STOP TIME being
> > recorded in the label file.  STOP TIME should probably be included in
> > the label file anyway.
> >
> > Adding the stop time column should be a simple addition and I don't see
> > a problem with that. I think I misunderstood your original request on
> > that. Because you are just talking about returning a timestamptz with
> > the "right now" value for when you called pg_stop_backup()? Or to be
> > specific, just before pg_Stop_backup *finished*. Or do you mean when
> > pg_stop_backup() started?
>
> What would be ideal is the minimum time that could be used for PITR.  In
> an exclusive backup that's the time the end-of-backup record is written
> to WAL.  In a non-exlusive backup I'm not quite sure how that works.
>

Having an actual definition of that is kind of required before adding it :P


> > Doing it in the backup label file is obviously a different target, where
> > we might need to consider backwards compatibility, Should we?
>
> Physical backups can only be restored in the same version so I'm not
> sure why it would be a problem?  Do you mean for programs outside of
> Postgres that are parsing this file?
>

Yes, I meant programs outside postgres.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 3/22/16 12:14 PM, Magnus Hagander wrote:
> On Tue, Mar 22, 2016 at 5:08 PM, David Steele  > wrote:
> 
> On 3/19/16 8:15 AM, Magnus Hagander wrote:
> 
> > I've attached an updated patch, which is rebased on current master and
> > includes the oid fix.
> 
> Before doing a thorough review of this patch there are a few points I
> would like to consider:
> 
> * I think it's really important to provide the stop time in some fashion
> when using this new technique.  I would prefer a new column to be
> returned from pg_stop_backup() but I could live with STOP TIME being
> recorded in the label file.  STOP TIME should probably be included in
> the label file anyway.
> 
> Adding the stop time column should be a simple addition and I don't see
> a problem with that. I think I misunderstood your original request on
> that. Because you are just talking about returning a timestamptz with
> the "right now" value for when you called pg_stop_backup()? Or to be
> specific, just before pg_Stop_backup *finished*. Or do you mean when
> pg_stop_backup() started?

What would be ideal is the minimum time that could be used for PITR.  In
an exclusive backup that's the time the end-of-backup record is written
to WAL.  In a non-exlusive backup I'm not quite sure how that works.

> Doing it in the backup label file is obviously a different target, where
> we might need to consider backwards compatibility, Should we?

Physical backups can only be restored in the same version so I'm not
sure why it would be a problem?  Do you mean for programs outside of
Postgres that are parsing this file?

> * It seems like STOP WAL LOCATION should now also be recorded in the
> label file.  Preferably this would used by recovery to determine when
> the database has reach consistency but that could be a future patch.
> I'm not very happy with the current method of using pg_control to get
> this information as it assumes that pg_control is copied last (at least
> based on the code comments).
> 
> That seems entirely out of scope for this patch, though. Doesn't mean it
> shouldn't be done, but that's a separate thing.

Fair enough.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Tue, Mar 22, 2016 at 5:08 PM, David Steele  wrote:

>
> On 3/19/16 8:15 AM, Magnus Hagander wrote:
>
> > I've attached an updated patch, which is rebased on current master and
> > includes the oid fix.
>
> Before doing a thorough review of this patch there are a few points I
> would like to consider:
>
> * I think it's really important to provide the stop time in some fashion
> when using this new technique.  I would prefer a new column to be
> returned from pg_stop_backup() but I could live with STOP TIME being
> recorded in the label file.  STOP TIME should probably be included in
> the label file anyway.
>

Adding the stop time column should be a simple addition and I don't see a
problem with that. I think I misunderstood your original request on that.
Because you are just talking about returning a timestamptz with the "right
now" value for when you called pg_stop_backup()? Or to be specific, just
before pg_Stop_backup *finished*. Or do you mean when pg_stop_backup()
started?

Doing it in the backup label file is obviously a different target, where we
might need to consider backwards compatibility, Should we?



> * It seems like STOP WAL LOCATION should now also be recorded in the
> label file.  Preferably this would used by recovery to determine when
> the database has reach consistency but that could be a future patch.
> I'm not very happy with the current method of using pg_control to get
> this information as it assumes that pg_control is copied last (at least
> based on the code comments).
>

That seems entirely out of scope for this patch, though. Doesn't mean it
shouldn't be done, but that's a separate thing.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

Hi Magnus,

On 3/19/16 8:15 AM, Magnus Hagander wrote:

> I've attached an updated patch, which is rebased on current master and
> includes the oid fix.

Before doing a thorough review of this patch there are a few points I
would like to consider:

* I think it's really important to provide the stop time in some fashion
when using this new technique.  I would prefer a new column to be
returned from pg_stop_backup() but I could live with STOP TIME being
recorded in the label file.  STOP TIME should probably be included in
the label file anyway.

* It seems like STOP WAL LOCATION should now also be recorded in the
label file.  Preferably this would used by recovery to determine when
the database has reach consistency but that could be a future patch.
I'm not very happy with the current method of using pg_control to get
this information as it assumes that pg_control is copied last (at least
based on the code comments).

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Mar 2, 2016 at 6:49 PM, Marco Nenciarini <
marco.nenciar...@2ndquadrant.it> wrote:

> Hi Magnus,
>

Hi!

First, again my apologies for completely missing that you had posted this
review!



> I've finally found some time to take a look to the patch.
>
> It applies with some fuzziness on master, but the result looks correct.
> Unfortunately the OID of the new pg_stop_backup function conflicts with
> "pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).
>

Fixed, thanks!



> After changing it the patch does not compile:
>

It compiles fine for me, and with no warnings.



>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
> -Wformat-security -fno-strict-aliasing -fwrapv
> -Wno-unused-command-line-argument -g -O0 -g -fno-omit-frame-pointer
> -I../../../../src/include
>
> -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/libxml2
> -I/usr/local/include -I/usr/local/opt/openssl/include -c -o xlog.o
> xlog.c -MMD -MP -MF .deps/xlog.Po
> xlog.c:1:19: error: use of undeclared identifier 'tblspc_mapfbuf';
>

Eh. There is no presence of "tblspc_mapfbuf" after the patch. I think it
looks like the "applies with fuzziness" actually wasn't correct, and you
ended up with bad code with a mix of the old and the new code in it.

I've attached an updated patch, which is rebased on current master and
includes the oid fix.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index ae93e69..cb1ea71 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17479,7 +17479,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_start_backup(label text , fast boolean )
+pg_start_backup(label text , fast boolean , exclusive boolean )
 
pg_lsn
Prepare for performing on-line backup (restricted to superusers or replication roles)
@@ -17489,7 +17489,14 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_stop_backup()
 
pg_lsn
-   Finish performing on-line backup (restricted to superusers or replication roles)
+   Finish performing exclusive on-line backup (restricted to superusers or replication roles)
+  
+  
+   
+pg_stop_backup(exclusive boolean)
+
+   setof record
+   Finish performing exclusive or non-exclusive on-line backup (restricted to superusers or replication roles)
   
   

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b119a47..8634cae 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9754,8 +9754,8 @@ XLogFileNameP(TimeLineID tli, XLogSegNo segno)
  */
 XLogRecPtr
 do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
-   char **labelfile, DIR *tblspcdir, List **tablespaces,
-   char **tblspcmapfile, bool infotbssize,
+   StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
+   StringInfo tblspcmapfile, bool infotbssize,
    bool needtblspcmapfile)
 {
 	bool		exclusive = (labelfile == NULL);
@@ -9769,8 +9769,6 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 	XLogSegNo	_logSegNo;
 	struct stat stat_buf;
 	FILE	   *fp;
-	StringInfoData labelfbuf;
-	StringInfoData tblspc_mapfbuf;
 
 	backup_started_in_recovery = RecoveryInProgress();
 
@@ -9967,7 +9965,8 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 		/*
 		 * Construct tablespace_map file
 		 */
-		initStringInfo(&tblspc_mapfbuf);
+		if (exclusive)
+			tblspcmapfile = makeStringInfo();
 
 		datadirpathlen = strlen(DataDir);
 
@@ -10040,7 +10039,7 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 			if (tablespaces)
 *tablespaces = lappend(*tablespaces, ti);
 
-			appendStringInfo(&tblspc_mapfbuf, "%s %s\n", ti->oid, ti->path);
+			appendStringInfo(tblspcmapfile, "%s %s\n", ti->oid, ti->path);
 
 			pfree(buflinkpath.data);
 #else
@@ -10059,23 +10058,24 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
 		/*
 		 * Construct backup label file
 		 */
-		initStringInfo(&labelfbuf);
+		if (exclusive)
+			labelfile = makeStringInfo();
 
 		/* Use the log timezone here, not the session timezone */
 		stamp_time = (pg_time_t) time(NULL);
 		pg_strftime(strfbuf, sizeof(strfbuf),
 	"%Y-%m-%d %H:%M:%S %Z",
 	pg_localtime(&stamp_time, log_timezone));
-		appendStringInfo(&labelfbuf, "START WAL LOCATION: %X/%X (file %s)\n",
+		appendStringInfo(labelfile, "START WAL LOCATION: %X/%X (file %s)\n",
 			 (uint32) (startpoint >> 32), (uint32) startpoint, xlogfilename);
-		appendStringInfo(&labelfbuf, "CHECKPOINT LOCATION: %X/%X\n",
+		appendStringInfo(labelfile, "CHECKPOINT LOCAT

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

Hi Magnus,

On 3/2/16 12:49 PM, Marco Nenciarini wrote:

> I've finally found some time to take a look to the patch.
> 
> It applies with some fuzziness on master, but the result looks correct.
> Unfortunately the OID of the new pg_stop_backup function conflicts with
> "pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).
>
> After changing it the patch does not compile:

It's been two weeks since Marco found these issues in the patch.  Please
provide an updated patch soon or I will mark this "returned with feedback".

Thanks,
-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Mar 16, 2016 at 5:06 PM, David Steele  wrote:

> Hi Magnus,
>
> On 3/2/16 12:49 PM, Marco Nenciarini wrote:
>
> > I've finally found some time to take a look to the patch.
> >
> > It applies with some fuzziness on master, but the result looks correct.
> > Unfortunately the OID of the new pg_stop_backup function conflicts with
> > "pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).
> >
> > After changing it the patch does not compile:
>
> It's been two weeks since Marco found these issues in the patch.  Please
> provide an updated patch soon or I will mark this "returned with feedback".
>
>
Hi!

Thanks for the note - I had missed Marco's response completely!

I'll take a look at it once Nordic PGDay is done!


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Marco Nenciarini]

Hi Magnus,

I've finally found some time to take a look to the patch.

It applies with some fuzziness on master, but the result looks correct.
Unfortunately the OID of the new pg_stop_backup function conflicts with
"pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).

After changing it the patch does not compile:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -g -O0 -g -fno-omit-frame-pointer
-I../../../../src/include
-I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/libxml2
-I/usr/local/include -I/usr/local/opt/openssl/include -c -o xlog.o
xlog.c -MMD -MP -MF .deps/xlog.Po
xlog.c:1:19: error: use of undeclared identifier 'tblspc_mapfbuf';
did you mean 'tblspcmapfile'?
initStringInfo(&tblspc_mapfbuf);
^~
tblspcmapfile
xlog.c:9790:19: note: 'tblspcmapfile' declared here
StringInfo tblspcmapfile, bool infotbssize,
^
xlog.c:10073:22: error: use of undeclared identifier 'tblspc_mapfbuf';
did you mean 'tblspcmapfile'?
appendStringInfo(&tblspc_mapfbuf, "%s %s\n", ti->oid, ti->path);
^~
tblspcmapfile
xlog.c:9790:19: note: 'tblspcmapfile' declared here
StringInfo tblspcmapfile, bool infotbssize,
^
xlog.c:10092:19: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
initStringInfo(&labelfbuf);
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10099:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "START WAL LOCATION: %X/%X (file %s)\n",
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10101:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "CHECKPOINT LOCATION: %X/%X\n",
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10103:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "BACKUP METHOD: %s\n",
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10105:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "BACKUP FROM: %s\n",
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10107:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "START TIME: %s\n", strfbuf);
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10108:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "LABEL: %s\n", backupidstr);
^
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10142:15: error: use of undeclared identifier 'labelfbuf'
if (fwrite(labelfbuf.data, labelfbuf.len, 1, fp) != 1 ||
^
xlog.c:10142:31: error: use of undeclared identifier 'labelfbuf'
if (fwrite(labelfbuf.data, labelfbuf.len, 1, fp) != 1 ||
^
xlog.c:10151:10: error: use of undeclared identifier 'labelfbuf'
pfree(labelfbuf.data);
^
xlog.c:10154:8: error: use of undeclared identifier 'tblspc_mapfbuf'
if (tblspc_mapfbuf.len > 0)
^
xlog.c:10178:16: error: use of undeclared identifier 'tblspc_mapfbuf'
if (fwrite(tblspc_mapfbuf.data, tblspc_mapfbuf.len, 1, fp) != 1 ||
^
xlog.c:10178:37: error: use of undeclared identifier 'tblspc_mapfbuf'
if (fwrite(tblspc_mapfbuf.data, tblspc_mapfbuf.len, 1, fp) != 1 ||
^
xlog.c:10189:10: error: use of undeclared identifier 'tblspc_mapfbuf'
pfree(tblspc_mapfbuf.data);
^
xlog.c:10193:17: error: use of undeclared identifier 'labelfbuf'
*labelfile = labelfbuf.data;
^
xlog.c:10194:8: error: use of undeclared identifier 'tblspc_mapfbuf'
if (tblspc_mapfbuf.len > 0)
^
xlog.c:10195:22: error: use of undeclared identifier 'tblspc_mapfbuf'
*tblspcmapfile = tblspc_mapfbuf.data;
^
19 errors generated.
make[4]: *** [xlog.o] Error 1
make[3]: *** [transam-recursive] Error 2
make[2]: *** [access-recursive] Error 2
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2

I've searched in past commits for any recent change that involves the
affected lines, but I have not found any.
Maybe some changes are missing?

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Marco Nenciarini]

Hi Magnus,

thanks for working on this topic.
What it does is very similar to what Barman's pgespresso extension does and I'd 
like to see it implemented soon in the core.

I've added myself as reviewer for the patch on commitfest site.

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 2/10/16 11:12 AM, Andres Freund wrote:
> On 2016-02-10 11:06:01 -0500, David Steele wrote:
>> That makes sense.  The backup_label "as is" could be output at the
>> beginning but if we want to add the minimum recovery point it would need
>> to be output at the end.
>>
>> It seems like tablespace_map could still be output at the beginning, though.
> 
> I don't really see enough benefit to go that way. What are you thinking
> of using the information for ("This would give the backup software more
> information to work with from the start.")?

Currently backup software that handles tablespaces needs to read the
pg_tblspc directory to build an oid/path mapping in order to know which
tablespace directories to copy.  Since Postgres is already doing this
when it creates tablespace_map it seems like it's redundant for the
backup software to do it again.

Since tablespace_map is created in do_pg_start_backup() and I don't see
how it could be updated after that, I think it is logical to output it
from pg_start_backup().  I don't feel strongly about it, though.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Andres Freund]

On 2016-02-10 11:06:01 -0500, David Steele wrote:
> That makes sense.  The backup_label "as is" could be output at the
> beginning but if we want to add the minimum recovery point it would need
> to be output at the end.
> 
> It seems like tablespace_map could still be output at the beginning, though.

I don't really see enough benefit to go that way. What are you thinking
of using the information for ("This would give the backup software more
information to work with from the start.")?

Andres


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 2/10/16 10:50 AM, Magnus Hagander wrote:
> On Wed, Feb 10, 2016 at 4:38 PM, David Steele 
> This information is handy for automating selection of a backup when
> doing time-based PITR (or so the user can manually select).  For
> exclusive backups it is possible to parse the .backup file to get this
> information but non-exclusive backups do not output the .backup file.
> 
> 
> The non-exclusive backups *do* output the backup_label file, it just
> shows up as a text field instead of as a separate file. You're supposed
> to write that data to a file in the backup program.

I meant the .backup file (e.g. 0001008C001A.0028.backup)
that is archived along with the WAL for an exlcusive backup.  I believe
this is currently the only place to get the stop time (without reading
the WAL segments).

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 2/10/16 11:01 AM, Andres Freund wrote:
> On 2016-02-10 16:50:26 +0100, Magnus Hagander wrote:
>>> I would be happy to see the time-stamp returned from the
>>> pg_start_backup() function as well.  It's a bigger change, but once
>>> pg_start_backup() returns multiple columns it will be easier to add more
>>> columns in the future.
>>>
>>> In fact, it might be better if backup_label and tablespace_map were
>>> output by pg_start_backup().  This would give the backup software more
>>> information to work with from the start.
>>
>> I was trying to figure out why that's a bad idea, but I can't really say
>> why :)
>>
>> For pg_basebackup backups, I think the reason we put it at the end is
>> simply that we don't want to write it into the backup directory until the
>> rest of the backup is complete, since it's not going to be useful before
>> then. But that requirement can certainly be put on the backup client
>> instead of the server. With the newer API it's going to have to keep those
>> things around anyway.
>>
>> That would then change the function syntax for pg_start_backup() but
>> instead make pg_stop_backup() now behave the same as it did before.
>>
>> Would anybody else like to comment on if that's a good idea or if there are
>> any holes in it? :)
> 
> I don't think that's a good idea. It makes it impossible to add
> information to labels about the minimum recovery point and
> such. Currently there's some rather fragile logic to discover that, but
> I'd really like to get rid of that at some point.

That makes sense.  The backup_label "as is" could be output at the
beginning but if we want to add the minimum recovery point it would need
to be output at the end.

It seems like tablespace_map could still be output at the beginning, though.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Andres Freund]

On 2016-02-10 16:50:26 +0100, Magnus Hagander wrote:
> > I would be happy to see the time-stamp returned from the
> > pg_start_backup() function as well.  It's a bigger change, but once
> > pg_start_backup() returns multiple columns it will be easier to add more
> > columns in the future.
> >
> > In fact, it might be better if backup_label and tablespace_map were
> > output by pg_start_backup().  This would give the backup software more
> > information to work with from the start.
> >
> 
> I was trying to figure out why that's a bad idea, but I can't really say
> why :)
> 
> For pg_basebackup backups, I think the reason we put it at the end is
> simply that we don't want to write it into the backup directory until the
> rest of the backup is complete, since it's not going to be useful before
> then. But that requirement can certainly be put on the backup client
> instead of the server. With the newer API it's going to have to keep those
> things around anyway.
> 
> That would then change the function syntax for pg_start_backup() but
> instead make pg_stop_backup() now behave the same as it did before.
> 
> Would anybody else like to comment on if that's a good idea or if there are
> any holes in it? :)

I don't think that's a good idea. It makes it impossible to add
information to labels about the minimum recovery point and
such. Currently there's some rather fragile logic to discover that, but
I'd really like to get rid of that at some point.

Regards,

Andres


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Feb 10, 2016 at 4:38 PM, David Steele  wrote:

> On 2/10/16 7:46 AM, Magnus Hagander wrote:
> > Per discussion at the developer meeting in Brussels, here's a patch that
> > makes some updates to the backup APIs, to support non-exclusive backups
> > without using pg_basebackup. <...>
>
> This sounds like a great idea and I have signed up to review.
>
> > * A new version of pg_stop_backup is created, taking an argument
> > specifying if it's exclusive or not. The current version of
> > pg_stop_backup() continues to work for exclusive backups, with no
> > changes to behavior. The new pg_stop_backup will return a record of
> > three columns instead of just the value -- the LSN (pglsn), the backup
> > label file (text) and the tablespace map file (text). If used to cancel
> > an exclusive backup, backup label file and tablespace map will be NULL.
> > At this point it's up to the backup software to write down the files in
> > the location of the backup.
>
> It would be nice if this new pg_stop_backup() function also output the
> exact time when the backup stopped.  Depending on how long
> pg_stop_backup() waits for WAL to be archived a time-stamp recorded
> after pg_stop_backup() returns can be pretty far off.
>
> This information is handy for automating selection of a backup when
> doing time-based PITR (or so the user can manually select).  For
> exclusive backups it is possible to parse the .backup file to get this
> information but non-exclusive backups do not output the .backup file.
>

The non-exclusive backups *do* output the backup_label file, it just shows
up as a text field instead of as a separate file. You're supposed to write
that data to a file in the backup program.



> I would be happy to see the time-stamp returned from the
> pg_start_backup() function as well.  It's a bigger change, but once
> pg_start_backup() returns multiple columns it will be easier to add more
> columns in the future.
>
> In fact, it might be better if backup_label and tablespace_map were
> output by pg_start_backup().  This would give the backup software more
> information to work with from the start.
>

I was trying to figure out why that's a bad idea, but I can't really say
why :)

For pg_basebackup backups, I think the reason we put it at the end is
simply that we don't want to write it into the backup directory until the
rest of the backup is complete, since it's not going to be useful before
then. But that requirement can certainly be put on the backup client
instead of the server. With the newer API it's going to have to keep those
things around anyway.

That would then change the function syntax for pg_start_backup() but
instead make pg_stop_backup() now behave the same as it did before.

Would anybody else like to comment on if that's a good idea or if there are
any holes in it? :)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 2/10/16 7:46 AM, Magnus Hagander wrote:
> Per discussion at the developer meeting in Brussels, here's a patch that
> makes some updates to the backup APIs, to support non-exclusive backups
> without using pg_basebackup. <...>

This sounds like a great idea and I have signed up to review.

> * A new version of pg_stop_backup is created, taking an argument
> specifying if it's exclusive or not. The current version of
> pg_stop_backup() continues to work for exclusive backups, with no
> changes to behavior. The new pg_stop_backup will return a record of
> three columns instead of just the value -- the LSN (pglsn), the backup
> label file (text) and the tablespace map file (text). If used to cancel
> an exclusive backup, backup label file and tablespace map will be NULL.
> At this point it's up to the backup software to write down the files in
> the location of the backup.

It would be nice if this new pg_stop_backup() function also output the
exact time when the backup stopped.  Depending on how long
pg_stop_backup() waits for WAL to be archived a time-stamp recorded
after pg_stop_backup() returns can be pretty far off.

This information is handy for automating selection of a backup when
doing time-based PITR (or so the user can manually select).  For
exclusive backups it is possible to parse the .backup file to get this
information but non-exclusive backups do not output the .backup file.

I would be happy to see the time-stamp returned from the
pg_start_backup() function as well.  It's a bigger change, but once
pg_start_backup() returns multiple columns it will be easier to add more
columns in the future.

In fact, it might be better if backup_label and tablespace_map were
output by pg_start_backup().  This would give the backup software more
information to work with from the start.

Thanks!
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Stephen Frost]

* David Steele (da...@pgmasters.net) wrote:
> On 2/10/16 9:44 AM, Stephen Frost wrote:
> > Hrmmm.  If that's the case then perhaps you're right.  I liked the
> > general idea of not having to maintain a TCP connection during the
> > entire backup (TCP connections can be annoyingly finicky in certain
> > environments...), but I'm not sure it's worth a lot of additional
> > complexity.
> 
> Well, pgBackRest holds a connection to PostgreSQL through the entire
> backup and will abort the backup if it is severed.  The connection is
> always held locally, though, even if the master backup process is on a
> different server.  I haven't gotten any reports of aborted backups due
> to the connection failing.

Yeah, I know, I had been thinking it might be nice to not do that at
some point in the future, but thinking on it further, we've already got
a "pick up where you left off" capability with pgbackrest, so it's
really not a huge deal if the backup fails and has to be restarted, and
this does remove the need (or at least deprecate) to use the "stop an
already-running backup if you find one" option that pgbackrest has.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[David Steele]

On 2/10/16 9:44 AM, Stephen Frost wrote:
> * Magnus Hagander (mag...@hagander.net) wrote:
>> On Wed, Feb 10, 2016 at 2:46 PM, Andres Freund  wrote:
>>> On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:
 * If the client disconnects with a non-exclusive backup running, the
>>> backup
 is automatically aborted. This is the same thing that pg_basebackup does.
 To use these non-exclusive backups the backup software will have to
 maintain a persistent connection to the database -- something that should
 not be a problem for any of the modern ones out there (but would've been
 slightly trickier back in the days when we suggested shellscripts)
>>>
>>> I think we might want to make this one optional, but I'm not going to
>>> fight super hard for it.
>>
>> Not sure what you're referring to here. Do you mean being able to make a
>> non-exclusive backup while not maintaining a connection? That's going to
>> make things a *lot* more complicated, as we'd have to invent something like
>> "backup slots" similar to what we're doing with replication slots. I doubt
>> it's worth all that work and complexity.

Agreed.

> Hrmmm.  If that's the case then perhaps you're right.  I liked the
> general idea of not having to maintain a TCP connection during the
> entire backup (TCP connections can be annoyingly finicky in certain
> environments...), but I'm not sure it's worth a lot of additional
> complexity.

Well, pgBackRest holds a connection to PostgreSQL through the entire
backup and will abort the backup if it is severed.  The connection is
always held locally, though, even if the master backup process is on a
different server.  I haven't gotten any reports of aborted backups due
to the connection failing.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Stephen Frost]

* Magnus Hagander (mag...@hagander.net) wrote:
> On Wed, Feb 10, 2016 at 2:46 PM, Andres Freund  wrote:
> > On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:
> > > Per discussionat the developer meeting in Brussels, here's a patch that
> > > makes some updates to the backup APIs, to support non-exclusive backups
> > > without using pg_basebackup.
> >
> > Thanks for following through with this!
> >
> > > * If the client disconnects with a non-exclusive backup running, the
> > backup
> > > is automatically aborted. This is the same thing that pg_basebackup does.
> > > To use these non-exclusive backups the backup software will have to
> > > maintain a persistent connection to the database -- something that should
> > > not be a problem for any of the modern ones out there (but would've been
> > > slightly trickier back in the days when we suggested shellscripts)
> >
> > I think we might want to make this one optional, but I'm not going to
> > fight super hard for it.
> 
> Not sure what you're referring to here. Do you mean being able to make a
> non-exclusive backup while not maintaining a connection? That's going to
> make things a *lot* more complicated, as we'd have to invent something like
> "backup slots" similar to what we're doing with replication slots. I doubt
> it's worth all that work and complexity.

Hrmmm.  If that's the case then perhaps you're right.  I liked the
general idea of not having to maintain a TCP connection during the
entire backup (TCP connections can be annoyingly finicky in certain
environments...), but I'm not sure it's worth a lot of additional
complexity.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:
> > * If the client disconnects with a non-exclusive backup running, the backup
> > is automatically aborted. This is the same thing that pg_basebackup does.
> > To use these non-exclusive backups the backup software will have to
> > maintain a persistent connection to the database -- something that should
> > not be a problem for any of the modern ones out there (but would've been
> > slightly trickier back in the days when we suggested shellscripts)
> 
> I think we might want to make this one optional, but I'm not going to
> fight super hard for it.

I was thinking along the same lines.

> > * A new version of pg_stop_backup is created, taking an argument specifying
> > if it's exclusive or not. The current version of pg_stop_backup() continues
> > to work for exclusive backups, with no changes to behavior. The new
> > pg_stop_backup will return a record of three columns instead of just the
> > value -- the LSN (pglsn), the backup label file (text) and the tablespace
> > map file (text).
> 
> I wonder if we shouldn't change the API a bit more aggressively. Right
> now we return the label and the map - but what if there's more files at
> some later point? One way would be to make it a SETOF function returning
> 'filename', 'content' or such.  Makes it less clear what to do with the
> lsn admittedly.

If we make the 'client disconnect means abort' optional then we'd also
need to modify the API of stop backup to specify which backup to stop,
I'd think.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Wed, Feb 10, 2016 at 2:46 PM, Andres Freund  wrote:

> Hi,
>
> On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:
> > Per discussionat the developer meeting in Brussels, here's a patch that
> > makes some updates to the backup APIs, to support non-exclusive backups
> > without using pg_basebackup.
>
> Thanks for following through with this!
>
> > * If the client disconnects with a non-exclusive backup running, the
> backup
> > is automatically aborted. This is the same thing that pg_basebackup does.
> > To use these non-exclusive backups the backup software will have to
> > maintain a persistent connection to the database -- something that should
> > not be a problem for any of the modern ones out there (but would've been
> > slightly trickier back in the days when we suggested shellscripts)
>
> I think we might want to make this one optional, but I'm not going to
> fight super hard for it.
>

Not sure what you're referring to here. Do you mean being able to make a
non-exclusive backup while not maintaining a connection? That's going to
make things a *lot* more complicated, as we'd have to invent something like
"backup slots" similar to what we're doing with replication slots. I doubt
it's worth all that work and complexity.


>
> > * A new version of pg_stop_backup is created, taking an argument
> specifying
> > if it's exclusive or not. The current version of pg_stop_backup()
> continues
> > to work for exclusive backups, with no changes to behavior. The new
> > pg_stop_backup will return a record of three columns instead of just the
> > value -- the LSN (pglsn), the backup label file (text) and the tablespace
> > map file (text).
>
> I wonder if we shouldn't change the API a bit more aggressively. Right
> now we return the label and the map - but what if there's more files at
> some later point? One way would be to make it a SETOF function returning
> 'filename', 'content' or such.  Makes it less clear what to do with the
> lsn admittedly.
>

*Adding* more columns to the API shouldn't be a problem in the future. If
there's another file, we can return a fourth column. A backup program is
going to have to know about those things anyway and shouldn't just blindly
write those files to the backup, IMO.


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Andres Freund]

Hi,

On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:
> Per discussionat the developer meeting in Brussels, here's a patch that
> makes some updates to the backup APIs, to support non-exclusive backups
> without using pg_basebackup.

Thanks for following through with this!

> * If the client disconnects with a non-exclusive backup running, the backup
> is automatically aborted. This is the same thing that pg_basebackup does.
> To use these non-exclusive backups the backup software will have to
> maintain a persistent connection to the database -- something that should
> not be a problem for any of the modern ones out there (but would've been
> slightly trickier back in the days when we suggested shellscripts)

I think we might want to make this one optional, but I'm not going to
fight super hard for it.

> * A new version of pg_stop_backup is created, taking an argument specifying
> if it's exclusive or not. The current version of pg_stop_backup() continues
> to work for exclusive backups, with no changes to behavior. The new
> pg_stop_backup will return a record of three columns instead of just the
> value -- the LSN (pglsn), the backup label file (text) and the tablespace
> map file (text).

I wonder if we shouldn't change the API a bit more aggressively. Right
now we return the label and the map - but what if there's more files at
some later point? One way would be to make it a SETOF function returning
'filename', 'content' or such.  Makes it less clear what to do with the
lsn admittedly.

Regards,

Andres


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Robert Haas]

On Wed, Feb 10, 2016 at 7:46 AM, Magnus Hagander  wrote:
> Per discussionat the developer meeting in Brussels, here's a patch that
> makes some updates to the backup APIs, to support non-exclusive backups
> without using pg_basebackup. The idea is to fix at least three main issues
> that are there today -- that you cannot run concurrent backups, that the
> backup_label file is created in the data directory which makes it impossible
> to distinguish between a cluster restored from backup and one that crashed
> while a backup was running, and a cluster can get "stuck" in backup mode if
> the backup script/software crashes.
>
> To make this work, this patch:
>
> * Introduces a new argument for pg_start_backup(), for "exclusive". It
> defaults to true, which means that just calling pg_start_backup() like
> before will behave exactly like before. If it's called with exclusive='f', a
> non-exclusive backup is started, and the backup label is collected in memory
> instead of in the backup_label file.
>
> * If the client disconnects with a non-exclusive backup running, the backup
> is automatically aborted. This is the same thing that pg_basebackup does. To
> use these non-exclusive backups the backup software will have to maintain a
> persistent connection to the database -- something that should not be a
> problem for any of the modern ones out there (but would've been slightly
> trickier back in the days when we suggested shellscripts)
>
> * A new version of pg_stop_backup is created, taking an argument specifying
> if it's exclusive or not. The current version of pg_stop_backup() continues
> to work for exclusive backups, with no changes to behavior. The new
> pg_stop_backup will return a record of three columns instead of just the
> value -- the LSN (pglsn), the backup label file (text) and the tablespace
> map file (text). If used to cancel an exclusive backup, backup label file
> and tablespace map will be NULL. At this point it's up to the backup
> software to write down the files in the location of the backup.
>
>
> Attached patch currently doesn't include full documentation, since Bruce was
> going to restructure that section of the docs (also based on the
> devmeeting). I will write up the docs once that is done (I assume it will be
> soon enough, or I'll go do it regardless), but I wanted to get some review
> in on the code while waiting.

Wow, this is a great idea.

-- 
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

[HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

Per discussionat the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup. The idea is to fix at least three main issues
that are there today -- that you cannot run concurrent backups, that the
backup_label file is created in the data directory which makes it
impossible to distinguish between a cluster restored from backup and one
that crashed while a backup was running, and a cluster can get "stuck" in
backup mode if the backup script/software crashes.

To make this work, this patch:

* Introduces a new argument for pg_start_backup(), for "exclusive". It
defaults to true, which means that just calling pg_start_backup() like
before will behave exactly like before. If it's called with exclusive='f',
a non-exclusive backup is started, and the backup label is collected in
memory instead of in the backup_label file.

* If the client disconnects with a non-exclusive backup running, the backup
is automatically aborted. This is the same thing that pg_basebackup does.
To use these non-exclusive backups the backup software will have to
maintain a persistent connection to the database -- something that should
not be a problem for any of the modern ones out there (but would've been
slightly trickier back in the days when we suggested shellscripts)

* A new version of pg_stop_backup is created, taking an argument specifying
if it's exclusive or not. The current version of pg_stop_backup() continues
to work for exclusive backups, with no changes to behavior. The new
pg_stop_backup will return a record of three columns instead of just the
value -- the LSN (pglsn), the backup label file (text) and the tablespace
map file (text). If used to cancel an exclusive backup, backup label file
and tablespace map will be NULL. At this point it's up to the backup
software to write down the files in the location of the backup.


Attached patch currently doesn't include full documentation, since Bruce
was going to restructure that section of the docs (also based on the
devmeeting). I will write up the docs once that is done (I assume it will
be soon enough, or I'll go do it regardless), but I wanted to get some
review in on the code while waiting.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 16981,16987  SELECT set_config('log_statement_stats', 'off', false);


 
! pg_start_backup(label text , fast boolean )
  
 pg_lsn
 Prepare for performing on-line backup (restricted to superusers or replication roles)
--- 16981,16987 


 
! pg_start_backup(label text , fast boolean , exclusive boolean )
  
 pg_lsn
 Prepare for performing on-line backup (restricted to superusers or replication roles)
***
*** 16991,16997  SELECT set_config('log_statement_stats', 'off', false);
  pg_stop_backup()
  
 pg_lsn
!Finish performing on-line backup (restricted to superusers or replication roles)


 
--- 16991,17004 
  pg_stop_backup()
  
 pg_lsn
!Finish performing exclusive on-line backup (restricted to superusers or replication roles)
!   
!   
!
! pg_stop_backup(exclusive boolean)
! 
!setof record
!Finish performing exclusive or non-exclusive on-line backup (restricted to superusers or replication roles)


 
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***
*** 9797,9804  XLogFileNameP(TimeLineID tli, XLogSegNo segno)
   */
  XLogRecPtr
  do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
!    char **labelfile, DIR *tblspcdir, List **tablespaces,
!    char **tblspcmapfile, bool infotbssize,
     bool needtblspcmapfile)
  {
  	bool		exclusive = (labelfile == NULL);
--- 9797,9804 
   */
  XLogRecPtr
  do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
!    StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
!    StringInfo tblspcmapfile, bool infotbssize,
     bool needtblspcmapfile)
  {
  	bool		exclusive = (labelfile == NULL);
***
*** 9812,9819  do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
  	XLogSegNo	_logSegNo;
  	struct stat stat_buf;
  	FILE	   *fp;
- 	StringInfoData labelfbuf;
- 	StringInfoData tblspc_mapfbuf;
  
  	backup_started_in_recovery = RecoveryInProgress();
  
--- 9812,9817 
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***
*** 28,41 
--- 28,64 
  #include "replication/walreceiver.h"
  #include "storage/smgr.h"
  #include "utils/builtins.h"
+ #include "utils/memutils.h"
  #inclu