Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-04-04 Thread David Steele 
On 4/4/17 11:42 AM, Stephen Frost wrote:
> * David Steele (da...@pgmasters.net) wrote:
>> On 3/22/17 4:42 PM, Peter Eisentraut wrote:
>>> On 3/22/17 15:14, Stephen Frost wrote:
> -SELECT * FROM pg_stop_backup(false);
> +SELECT * FROM pg_stop_backup(false [, true ]);
>
> I think that it's better to get rid of "[" and "]" from the above because
> IMO this should be the command example that users actually can run.
 Using the '[' and ']' are how all of the optional arguments are
 specified in the documentation, see things like current_setting() in our
 existing documentation:
>>>
>>> In the synopsis, but not in concrete examples.
>>
>> Wasn't quite sure what the protocol was in this case, and figured it
>> was easier to subtract than to add.
> 
> Forgot to close this out sooner, apologies.
> 
> This has been committed now.

Thank you, Stephen!

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-04-04 Thread Stephen Frost 
* David Steele (da...@pgmasters.net) wrote:
> On 3/22/17 4:42 PM, Peter Eisentraut wrote:
> >On 3/22/17 15:14, Stephen Frost wrote:
> >>>-SELECT * FROM pg_stop_backup(false);
> >>>+SELECT * FROM pg_stop_backup(false [, true ]);
> >>>
> >>>I think that it's better to get rid of "[" and "]" from the above because
> >>>IMO this should be the command example that users actually can run.
> >>Using the '[' and ']' are how all of the optional arguments are
> >>specified in the documentation, see things like current_setting() in our
> >>existing documentation:
> >
> >In the synopsis, but not in concrete examples.
> 
> Wasn't quite sure what the protocol was in this case, and figured it
> was easier to subtract than to add.

Forgot to close this out sooner, apologies.

This has been committed now.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-22 Thread David Steele 

On 3/22/17 4:42 PM, Peter Eisentraut wrote:

On 3/22/17 15:14, Stephen Frost wrote:

-SELECT * FROM pg_stop_backup(false);
+SELECT * FROM pg_stop_backup(false [, true ]);

I think that it's better to get rid of "[" and "]" from the above because
IMO this should be the command example that users actually can run.

Using the '[' and ']' are how all of the optional arguments are
specified in the documentation, see things like current_setting() in our
existing documentation:


In the synopsis, but not in concrete examples.


Wasn't quite sure what the protocol was in this case, and figured it was 
easier to subtract than to add.


--
-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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-22 Thread Stephen Frost 
* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 3/22/17 15:14, Stephen Frost wrote:
> >> -SELECT * FROM pg_stop_backup(false);
> >> +SELECT * FROM pg_stop_backup(false [, true ]);
> >>
> >> I think that it's better to get rid of "[" and "]" from the above because
> >> IMO this should be the command example that users actually can run.
> > Using the '[' and ']' are how all of the optional arguments are
> > specified in the documentation, see things like current_setting() in our
> > existing documentation:
> 
> In the synopsis, but not in concrete examples.

Oh, right, yes.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-22 Thread Peter Eisentraut 
On 3/22/17 15:14, Stephen Frost wrote:
>> -SELECT * FROM pg_stop_backup(false);
>> +SELECT * FROM pg_stop_backup(false [, true ]);
>>
>> I think that it's better to get rid of "[" and "]" from the above because
>> IMO this should be the command example that users actually can run.
> Using the '[' and ']' are how all of the optional arguments are
> specified in the documentation, see things like current_setting() in our
> existing documentation:

In the synopsis, but not in concrete examples.

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


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

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-22 Thread Stephen Frost 
Fujii,

* Fujii Masao (masao.fu...@gmail.com) wrote:
> On Thu, Mar 23, 2017 at 12:37 AM, Stephen Frost  wrote:
> > * David Steele (da...@pgmasters.net) wrote:
> >> On 3/21/17 2:34 PM, Fujii Masao wrote:
> >> >The patch basically looks good to me, but one comment is;
> >> >backup.sgml (at least the description for "Making a non-exclusive
> >> >low level backup) seems to need to be updated.
> >>
> >> Agreed.  Added in the attached patch and rebased on 8027556.
> 
> Thanks for updating the patch!
> 
> -SELECT * FROM pg_stop_backup(false);
> +SELECT * FROM pg_stop_backup(false [, true ]);
> 
> I think that it's better to get rid of "[" and "]" from the above because
> IMO this should be the command example that users actually can run.

Using the '[' and ']' are how all of the optional arguments are
specified in the documentation, see things like current_setting() in our
existing documentation:

https://www.postgresql.org/docs/9.6/static/functions-admin.html

> + If the backup process monitors the WAL archiving process independently,
> + the second parameter (which defaults to true) can be set to false to
> + prevent pg_stop_backup from blocking until all WAL is
> + archived.  Instead, the function will return as soon as the stop backup
> + record is written to the WAL.  This option must be used with caution:
> + if WAL archiving is not monitored correctly then the result might be a
> + useless backup.
> 
> You added this descriptions into the step #4 in the non-exclusive
> backup procedure.. But since the step #5 already explains how
> pg_stop_backup has to do with WAL archiving, I think that it's better
> to update (or add something like the above descriptions into)
> the step #5. Thought?

That seems pretty reasonable to me.

> + If the backup process monitors the WAL archiving process independently,
> 
> Can we explain "monitor the WAL archiving process" part a bit more
> explicitly? For example, "monitor and ensure that all WAL segment files
> required for the backup are successfully archived".

Sure, makes sense.  I'll add some language along those lines.

> > I've started looking at this.  Seems pretty straight-forward and will
> > try to get it committed later today.
> 
> Thanks!

My apologies if you had intended to look at committing this, I just
noticed that it hadn't been 'claimed' yet in the CF app and did so to
move forward with it.  I didn't mean to step on anyone's 'toes'.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-22 Thread Fujii Masao 
On Thu, Mar 23, 2017 at 12:37 AM, Stephen Frost  wrote:
> David, all,
>
> * David Steele (da...@pgmasters.net) wrote:
>> On 3/21/17 2:34 PM, Fujii Masao wrote:
>> >The patch basically looks good to me, but one comment is;
>> >backup.sgml (at least the description for "Making a non-exclusive
>> >low level backup) seems to need to be updated.
>>
>> Agreed.  Added in the attached patch and rebased on 8027556.

Thanks for updating the patch!

-SELECT * FROM pg_stop_backup(false);
+SELECT * FROM pg_stop_backup(false [, true ]);

I think that it's better to get rid of "[" and "]" from the above because
IMO this should be the command example that users actually can run.

+ If the backup process monitors the WAL archiving process independently,
+ the second parameter (which defaults to true) can be set to false to
+ prevent pg_stop_backup from blocking until all WAL is
+ archived.  Instead, the function will return as soon as the stop backup
+ record is written to the WAL.  This option must be used with caution:
+ if WAL archiving is not monitored correctly then the result might be a
+ useless backup.

You added this descriptions into the step #4 in the non-exclusive
backup procedure.. But since the step #5 already explains how
pg_stop_backup has to do with WAL archiving, I think that it's better
to update (or add something like the above descriptions into)
the step #5. Thought?

+ If the backup process monitors the WAL archiving process independently,

Can we explain "monitor the WAL archiving process" part a bit more
explicitly? For example, "monitor and ensure that all WAL segment files
required for the backup are successfully archived".

> I've started looking at this.  Seems pretty straight-forward and will
> try to get it committed later today.

Thanks!

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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-22 Thread Stephen Frost 
David, all,

* David Steele (da...@pgmasters.net) wrote:
> On 3/21/17 2:34 PM, Fujii Masao wrote:
> >The patch basically looks good to me, but one comment is;
> >backup.sgml (at least the description for "Making a non-exclusive
> >low level backup) seems to need to be updated.
> 
> Agreed.  Added in the attached patch and rebased on 8027556.

I've started looking at this.  Seems pretty straight-forward and will
try to get it committed later today.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-21 Thread David Steele 

On 3/21/17 2:34 PM, Fujii Masao wrote:

On Tue, Mar 21, 2017 at 11:03 AM, Tsunakawa, Takayuki
 wrote:

From: David Steele [mailto:da...@pgmasters.net]

Well, that's embarrassing.  When I recreated the function to add defaults
I messed up the AS clause and did not pay attention to the results of the
regression tests, apparently.

Attached is a new version rebased on 88e66d1.  Catalog version bump has
also been omitted.


I was late to reply because yesterday was a national holiday in Japan.  I 
marked this as ready for committer.  The patch applied cleanly and worked as 
expected.


The patch basically looks good to me, but one comment is;
backup.sgml (at least the description for "Making a non-exclusive
low level backup) seems to need to be updated.


Agreed.  Added in the attached patch and rebased on 8027556.

Thanks!
--
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 2d67521..c04eb46 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -887,7 +887,7 @@ SELECT pg_start_backup('label', false, false);
 
  In the same connection as before, issue the command:
 
-SELECT * FROM pg_stop_backup(false);
+SELECT * FROM pg_stop_backup(false [, true ]);
 
  This terminates the backup mode and performs an automatic switch to
  the next WAL segment.  The reason for the switch is to arrange for
@@ -895,6 +895,15 @@ SELECT * FROM pg_stop_backup(false);
  ready to archive.
 
 
+ If the backup process monitors the WAL archiving process independently,
+ the second parameter (which defaults to true) can be set to false to
+ prevent pg_stop_backup from blocking until all WAL is
+ archived.  Instead, the function will return as soon as the stop backup
+ record is written to the WAL.  This option must be used with caution:
+ if WAL archiving is not monitored correctly then the result might be a
+ useless 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
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9408a25..4dc30ca 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18355,7 +18355,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_stop_backup(exclusive 
boolean)
+pg_stop_backup(exclusive 
boolean , wait_for_archive boolean 
)
 
setof record
Finish performing exclusive or non-exclusive on-line backup 
(restricted to superusers by default, but other users can be granted EXECUTE to 
run the function)
@@ -18439,7 +18439,13 @@ postgres=# select pg_start_backup('label_goes_here');
 pg_start_backup. In a non-exclusive backup, the contents of
 the backup_label and tablespace_map are returned
 in the result of the function, and should be written to files in the
-backup (and not in the data directory).
+backup (and not in the data directory).  There is an optional second
+parameter of type boolean.  If false, the pg_stop_backup
+will return immediately after the backup is completed without waiting for
+WAL to be archived.  This behavior is only useful for backup
+software which independently monitors WAL archiving. Otherwise, WAL
+required to make the backup consistent might be missing and make the backup
+useless.

 

diff --git a/src/backend/access/transam/xlogfuncs.c 
b/src/backend/access/transam/xlogfuncs.c
index 96aa15e..1ef9f2b 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -190,6 +190,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
boolnulls[3];
 
boolexclusive = PG_GETARG_BOOL(0);
+   boolwait_for_archive = PG_GETARG_BOOL(1);
XLogRecPtr  stoppoint;
 
/* check to see if caller supports us returning a tuplestore */
@@ -232,7 +233,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the exclusive backup, and since we're in an exclusive 
backup
 * return NULL for both backup_label and tablespace_map.
 */
-   stoppoint = do_pg_stop_backup(NULL, true, NULL);
+   stoppoint = do_pg_stop_backup(NULL, wait_for_archive, NULL);
exclusive_backup_running = false;
 
nulls[1] = true;
@@ -250,7 +251,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the non-exclusive backup. Return a copy of the backup 
label
 * and tablespace map so they can be written to disk by the 
caller.
 */
-   stoppoint = do_pg_stop_backup(label_file->data, true, NULL);
+   stoppoint = do_pg_stop_backup(label_file->data, 
wait_for_archive, NULL);
nonexclusive_backup_running = false;

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-21 Thread Fujii Masao 
On Tue, Mar 21, 2017 at 11:03 AM, Tsunakawa, Takayuki
 wrote:
> From: David Steele [mailto:da...@pgmasters.net]
>> Well, that's embarrassing.  When I recreated the function to add defaults
>> I messed up the AS clause and did not pay attention to the results of the
>> regression tests, apparently.
>>
>> Attached is a new version rebased on 88e66d1.  Catalog version bump has
>> also been omitted.
>
> I was late to reply because yesterday was a national holiday in Japan.  I 
> marked this as ready for committer.  The patch applied cleanly and worked as 
> expected.

The patch basically looks good to me, but one comment is;
backup.sgml (at least the description for "Making a non-exclusive
low level backup) seems to need to be updated.

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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-20 Thread Tsunakawa, Takayuki 
From: David Steele [mailto:da...@pgmasters.net]
> Well, that's embarrassing.  When I recreated the function to add defaults
> I messed up the AS clause and did not pay attention to the results of the
> regression tests, apparently.
> 
> Attached is a new version rebased on 88e66d1.  Catalog version bump has
> also been omitted.

I was late to reply because yesterday was a national holiday in Japan.  I 
marked this as ready for committer.  The patch applied cleanly and worked as 
expected.

Regards
Takayuki Tsunakawa


-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-17 Thread David Steele 
On 3/17/17 4:18 AM, Tsunakawa, Takayuki wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
>> Takayuki
>> I made this ready for committer.  The patch applied except for catversion.h,
>> the patch content looks good, and the target test passed as follows:
> 
> Sorry, I reverted this to waiting for author, because make check failed.  
> src/test/regress/expected/opr_sanity.out seems to need revision because 
> pg_stop_backup() was added.

Well, that's embarrassing.  When I recreated the function to add
defaults I messed up the AS clause and did not pay attention to the
results of the regression tests, apparently.

Attached is a new version rebased on 88e66d1.  Catalog version bump has
also been omitted.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9408a25..4dc30ca 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18355,7 +18355,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_stop_backup(exclusive 
boolean)
+pg_stop_backup(exclusive 
boolean , wait_for_archive boolean 
)
 
setof record
Finish performing exclusive or non-exclusive on-line backup 
(restricted to superusers by default, but other users can be granted EXECUTE to 
run the function)
@@ -18439,7 +18439,13 @@ postgres=# select pg_start_backup('label_goes_here');
 pg_start_backup. In a non-exclusive backup, the contents of
 the backup_label and tablespace_map are returned
 in the result of the function, and should be written to files in the
-backup (and not in the data directory).
+backup (and not in the data directory).  There is an optional second
+parameter of type boolean.  If false, the pg_stop_backup
+will return immediately after the backup is completed without waiting for
+WAL to be archived.  This behavior is only useful for backup
+software which independently monitors WAL archiving. Otherwise, WAL
+required to make the backup consistent might be missing and make the backup
+useless.

 

diff --git a/src/backend/access/transam/xlogfuncs.c 
b/src/backend/access/transam/xlogfuncs.c
index 96aa15e..1ef9f2b 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -190,6 +190,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
boolnulls[3];
 
boolexclusive = PG_GETARG_BOOL(0);
+   boolwait_for_archive = PG_GETARG_BOOL(1);
XLogRecPtr  stoppoint;
 
/* check to see if caller supports us returning a tuplestore */
@@ -232,7 +233,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the exclusive backup, and since we're in an exclusive 
backup
 * return NULL for both backup_label and tablespace_map.
 */
-   stoppoint = do_pg_stop_backup(NULL, true, NULL);
+   stoppoint = do_pg_stop_backup(NULL, wait_for_archive, NULL);
exclusive_backup_running = false;
 
nulls[1] = true;
@@ -250,7 +251,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the non-exclusive backup. Return a copy of the backup 
label
 * and tablespace map so they can be written to disk by the 
caller.
 */
-   stoppoint = do_pg_stop_backup(label_file->data, true, NULL);
+   stoppoint = do_pg_stop_backup(label_file->data, 
wait_for_archive, NULL);
nonexclusive_backup_running = false;
cancel_before_shmem_exit(nonexclusive_base_backup_cleanup, 
(Datum) 0);
 
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index b6552da..c2b0bed 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -988,6 +988,12 @@ CREATE OR REPLACE FUNCTION
   RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup'
   PARALLEL RESTRICTED;
 
+CREATE OR REPLACE FUNCTION pg_stop_backup (
+exclusive boolean, wait_for_archive boolean DEFAULT true,
+OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
+  RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2'
+  PARALLEL RESTRICTED;
+
 -- legacy definition for compatibility with 9.3
 CREATE OR REPLACE FUNCTION
   json_populate_record(base anyelement, from_json json, use_json_as_text 
boolean DEFAULT false)
@@ -1088,7 +1094,7 @@ AS 'jsonb_insert';
 -- available to superuser / cluster owner, if they choose.
 REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
-REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean) FROM public;
+REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public;
 REVOKE EXECUTE

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-17 Thread Tsunakawa, Takayuki 
From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> I made this ready for committer.  The patch applied except for catversion.h,
> the patch content looks good, and the target test passed as follows:

Sorry, I reverted this to waiting for author, because make check failed.  
src/test/regress/expected/opr_sanity.out seems to need revision because 
pg_stop_backup() was added.

Regards
Takayuki Tsunakawa



-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-16 Thread Tsunakawa, Takayuki 
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> On Fri, Mar 17, 2017 at 1:47 PM, Tsunakawa, Takayuki
>  wrote:
> > BTW, does the developer of each feature have to modify the catalog version
> in catversion.h?  It's a bit annoying to see the patch application failure
> on catversion.h.
> 
> Committers take care of this part.

I understood the committer modifies the catalog version based on the patch 
content, so the patch submitter doesn't have to modify it.  I'm relieved.

> > Isn't it enough to modify the catalog version only when
> alpha/beta/RC/final versions are released?
> 
> That's useful at least for developers to bump it even during a development
> cycle as you can notice with a hard failure at startup if a data folder
> you have created is compatible with the new binaries or not.

Oh, you're absolutely right.

Regards
Takayuki Tsunakawa


-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-16 Thread Michael Paquier 
On Fri, Mar 17, 2017 at 1:47 PM, Tsunakawa, Takayuki
 wrote:
> BTW, does the developer of each feature have to modify the catalog version in 
> catversion.h?  It's a bit annoying to see the patch application failure on 
> catversion.h.

Committers take care of this part.

> Isn't it enough to modify the catalog version only when alpha/beta/RC/final 
> versions are released?

That's useful at least for developers to bump it even during a
development cycle as you can notice with a hard failure at startup if
a data folder you have created is compatible with the new binaries or
not.
-- 
Michael


-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-16 Thread Tsunakawa, Takayuki 
From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of David Steele
> The attached patch udpates the docs per your suggestion and has been rebased
> on master at d69fae2.

I made this ready for committer.  The patch applied except for catversion.h, 
the patch content looks good, and the target test passed as follows:

I set archive_command to 'sleep 10'.  pg_stop_backup() with archive wait took 
about 10 seconds, emitting NOTICE messages.

postgres=# select pg_stop_backup(false, true);
NOTICE:  pg_stop_backup cleanup done, waiting for required WAL segments to be 
archived

NOTICE:  pg_stop_backup complete, all required WAL segments have been archived
  pg_stop_backup
---
 (0/BF8,"START WAL LOCATION: 0/B28 (file 0001000B)+
 CHECKPOINT LOCATION: 0/B60   +
 BACKUP METHOD: streamed  +
 BACKUP FROM: master  +
 START TIME: 2017-03-17 13:26:47 JST  +
 LABEL: a +
 ","")
(1 row)


pg_stop_backup() without archive wait returned immediately without displaying 
any NOTICE messages.


postgres=# select pg_stop_backup(false, false);
  pg_stop_backup
---
 (0/D000130,"START WAL LOCATION: 0/D28 (file 0001000D)+
 CHECKPOINT LOCATION: 0/D60   +
 BACKUP METHOD: streamed  +
 BACKUP FROM: master  +
 START TIME: 2017-03-17 13:29:46 JST  +
 LABEL: a +
 ","")
(1 row)


BTW, does the developer of each feature have to modify the catalog version in 
catversion.h?  It's a bit annoying to see the patch application failure on 
catversion.h.  Isn't it enough to modify the catalog version only when 
alpha/beta/RC/final versions are released?


Regards
Takayuki Tsunakawa


-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-13 Thread David Steele 
Hi Robert,

On 3/6/17 12:48 PM, Robert Haas wrote:
> On Sat, Mar 4, 2017 at 9:12 AM, David Steele  wrote:
>> Yes, that makes sense.  Attached are two patches as requested:
>>
>> 01 - Just marks pg_stop_backup() variants as parallel restricted
>> 02 - Add the wait_for_archive param to pg_stop_backup().
>>
>> These apply cleanly on 272adf4.
> 
> Committed 01.  

Thanks!

> Nobody's offered an opinion about 02 yet, so I'm not
> going to commit that, but one minor nitpick:
> 
> +WAL to be archived.  This behavior is only useful for backup
> +software which independently monitors WAL archiving, otherwise WAL
> +required to make the backup consistent might be missing and make the 
> backup
> 
> I think this should really say "...which independently monitors WAL
> archiving.  Otherwise, WAL..."

The attached patch udpates the docs per your suggestion and has been
rebased on master at d69fae2.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 583b3b2..ddf3298 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18299,7 +18299,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_stop_backup(exclusive 
boolean)
+pg_stop_backup(exclusive 
boolean , wait_for_archive boolean 
)
 
setof record
Finish performing exclusive or non-exclusive on-line backup 
(restricted to superusers by default, but other users can be granted EXECUTE to 
run the function)
@@ -18383,7 +18383,13 @@ postgres=# select pg_start_backup('label_goes_here');
 pg_start_backup. In a non-exclusive backup, the contents of
 the backup_label and tablespace_map are returned
 in the result of the function, and should be written to files in the
-backup (and not in the data directory).
+backup (and not in the data directory).  There is an optional second
+parameter of type boolean.  If false, the pg_stop_backup
+will return immediately after the backup is completed without waiting for
+WAL to be archived.  This behavior is only useful for backup
+software which independently monitors WAL archiving. Otherwise, WAL
+required to make the backup consistent might be missing and make the backup
+useless.

 

diff --git a/src/backend/access/transam/xlogfuncs.c 
b/src/backend/access/transam/xlogfuncs.c
index 96aa15e..1ef9f2b 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -190,6 +190,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
boolnulls[3];
 
boolexclusive = PG_GETARG_BOOL(0);
+   boolwait_for_archive = PG_GETARG_BOOL(1);
XLogRecPtr  stoppoint;
 
/* check to see if caller supports us returning a tuplestore */
@@ -232,7 +233,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the exclusive backup, and since we're in an exclusive 
backup
 * return NULL for both backup_label and tablespace_map.
 */
-   stoppoint = do_pg_stop_backup(NULL, true, NULL);
+   stoppoint = do_pg_stop_backup(NULL, wait_for_archive, NULL);
exclusive_backup_running = false;
 
nulls[1] = true;
@@ -250,7 +251,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the non-exclusive backup. Return a copy of the backup 
label
 * and tablespace map so they can be written to disk by the 
caller.
 */
-   stoppoint = do_pg_stop_backup(label_file->data, true, NULL);
+   stoppoint = do_pg_stop_backup(label_file->data, 
wait_for_archive, NULL);
nonexclusive_backup_running = false;
cancel_before_shmem_exit(nonexclusive_base_backup_cleanup, 
(Datum) 0);
 
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 0bce209..27ddaf0 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -988,6 +988,12 @@ CREATE OR REPLACE FUNCTION
   RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup'
   PARALLEL RESTRICTED;
 
+CREATE OR REPLACE FUNCTION pg_stop_backup (
+exclusive boolean, wait_for_archive boolean DEFAULT true,
+OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
+  RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup'
+  PARALLEL RESTRICTED;
+
 -- legacy definition for compatibility with 9.3
 CREATE OR REPLACE FUNCTION
   json_populate_record(base anyelement, from_json json, use_json_as_text 
boolean DEFAULT false)
@@ -1088,7 +1094,7 @@ AS 'jsonb_insert';
 -- available to superuser / cluster owner, if they choose.
 REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean, boolean) FROM public;
 REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public;
-REVOKE EXECUTE ON FUNCTION pg_stop_backup(boolean) FROM public;
+REVOKE

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-06 Thread Robert Haas 
On Mon, Mar 6, 2017 at 12:53 PM, Stephen Frost  wrote:
> Regarding 02, I certainly see that as valuable for the reasons which
> David outlined in his initial email.  I can certainly take point on
> getting it committed, but I wouldn't complain if someone else does
> either.

Sold, to the snowman in the first row.

-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-06 Thread Stephen Frost 
Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sat, Mar 4, 2017 at 9:12 AM, David Steele  wrote:
> > Yes, that makes sense.  Attached are two patches as requested:
> >
> > 01 - Just marks pg_stop_backup() variants as parallel restricted
> > 02 - Add the wait_for_archive param to pg_stop_backup().
> >
> > These apply cleanly on 272adf4.
> 
> Committed 01.  Nobody's offered an opinion about 02 yet, so I'm not
> going to commit that, but one minor nitpick:
> 
> +WAL to be archived.  This behavior is only useful for backup
> +software which independently monitors WAL archiving, otherwise WAL
> +required to make the backup consistent might be missing and make the 
> backup
> 
> I think this should really say "...which independently monitors WAL
> archiving.  Otherwise, WAL..."

Regarding 02, I certainly see that as valuable for the reasons which
David outlined in his initial email.  I can certainly take point on
getting it committed, but I wouldn't complain if someone else does
either.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-06 Thread Robert Haas 
On Sat, Mar 4, 2017 at 9:12 AM, David Steele  wrote:
> Yes, that makes sense.  Attached are two patches as requested:
>
> 01 - Just marks pg_stop_backup() variants as parallel restricted
> 02 - Add the wait_for_archive param to pg_stop_backup().
>
> These apply cleanly on 272adf4.

Committed 01.  Nobody's offered an opinion about 02 yet, so I'm not
going to commit that, but one minor nitpick:

+WAL to be archived.  This behavior is only useful for backup
+software which independently monitors WAL archiving, otherwise WAL
+required to make the backup consistent might be missing and make the backup

I think this should really say "...which independently monitors WAL
archiving.  Otherwise, WAL..."

-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-04 Thread David Steele 
Hi Robert,

On 3/4/17 1:58 AM, Robert Haas wrote:
> On Wed, Mar 1, 2017 at 9:07 AM, David Steele  wrote:
>> On 2/28/17 10:22 PM, Robert Haas wrote:
>>> On Tue, Feb 28, 2017 at 6:22 AM, David Steele  wrote:
>> I'm not sure that's the case.  It seems like it should lock just as
>> multiple backends would now.  One process would succeed and the others
>> would error.  Maybe I'm missing something?
>
> Hm, any errors happening in the workers would be reported to the
> leader, meaning that even if one worker succeeded to run
> pg_start_backup() it would be reported as an error at the end to the
> client, no? By marking the exclusive function restricted we get sure
> that it is just the leader that fails or succeeds.

 Good point, and it strengthens the argument beyond, "it just seems right."
>>>
>>> I think the argument should be based on whether or not the function
>>> depends on backend-private state that will not be synchronized.
>>> That's the definition of what makes something parallel-restricted or
>>> not.
>>
>> Absolutely.  Yesterday was a long day so I may have (perhaps) become a
>> bit flippant.
>>
>>> It looks like pg_start_backup() and pg_stop_backup() depend on the
>>> backend-private global variable nonexclusive_backup_running, so they
>>> should be parallel-restricted.
>>
>> Agreed.
> 
> How about a separately-committable patch that just does that, and then
> a main patch that applies on top of it?

Yes, that makes sense.  Attached are two patches as requested:

01 - Just marks pg_stop_backup() variants as parallel restricted
02 - Add the wait_for_archive param to pg_stop_backup().

These apply cleanly on 272adf4.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 438378d..b17a236 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /* mmddN */
-#define CATALOG_VERSION_NO 201703032
+#define CATALOG_VERSION_NO 201703041
 
 #endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 0c8b5c6..ec4aedb 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3141,9 +3141,9 @@ DATA(insert OID = 2096 ( pg_terminate_backend 
PGNSP PGUID 12 1 0 0 0 f f f f t
 DESCR("terminate a server process");
 DATA(insert OID = 2172 ( pg_start_backup   PGNSP PGUID 12 1 0 0 0 
f f f f t f v r 3 0 3220 "25 16 16" _null_ _null_ _null_ _null_ _null_ 
pg_start_backup _null_ _null_ _null_ ));
 DESCR("prepare for taking an online backup");
-DATA(insert OID = 2173 ( pg_stop_backupPGNSP PGUID 12 
1 0 0 0 f f f f t f v s 0 0 3220 "" _null_ _null_ _null_ _null_ _null_ 
pg_stop_backup _null_ _null_ _null_ ));
+DATA(insert OID = 2173 ( pg_stop_backupPGNSP PGUID 12 
1 0 0 0 f f f f t f v r 0 0 3220 "" _null_ _null_ _null_ _null_ _null_ 
pg_stop_backup _null_ _null_ _null_ ));
 DESCR("finish taking an online backup");
-DATA(insert OID = 2739 ( pg_stop_backupPGNSP PGUID 12 
1 1 0 0 f f f f t t v s 1 0 2249 "16" "{16,3220,25,25}" "{i,o,o,o}" 
"{exclusive,lsn,labelfile,spcmapfile}" _null_ _null_ pg_stop_backup_v2 _null_ 
_null_ _null_ ));
+DATA(insert OID = 2739 ( pg_stop_backupPGNSP PGUID 12 
1 1 0 0 f f f f t t v r 1 0 2249 "16" "{16,3220,25,25}" "{i,o,o,o}" 
"{exclusive,lsn,labelfile,spcmapfile}" _null_ _null_ pg_stop_backup_v2 _null_ 
_null_ _null_ ));
 DESCR("finish taking an online backup");
 DATA(insert OID = 3813 ( pg_is_in_backup   PGNSP PGUID 12 1 0 0 0 
f f f f t f v s 0 0 16 "" _null_ _null_ _null_ _null_ _null_ pg_is_in_backup 
_null_ _null_ _null_ ));
 DESCR("true if server is in online backup");
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9e084ad..ba6f030 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18098,7 +18098,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_stop_backup(exclusive 
boolean)
+pg_stop_backup(exclusive 
boolean , wait_for_archive boolean 
)
 
setof record
Finish performing exclusive or non-exclusive on-line backup 
(restricted to superusers by default, but other users can be granted EXECUTE to 
run the function)
@@ -18182,7 +18182,13 @@ postgres=# select pg_start_backup('label_goes_here');
 pg_start_backup. In a non-exclusive backup, the contents of
 the backup_label and tablespace_map are returned
 in the result of the function, and should be written to files in the
-backup (and not in the data directory).
+backup (and not in the data directory).  There is an optional second
+parameter of type boolean.  If false, the pg_stop_backup
+will return immediately after the backup is completed

Re: [HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-03-03 Thread Robert Haas 
On Wed, Mar 1, 2017 at 9:07 AM, David Steele  wrote:
> On 2/28/17 10:22 PM, Robert Haas wrote:
>> On Tue, Feb 28, 2017 at 6:22 AM, David Steele  wrote:
> I'm not sure that's the case.  It seems like it should lock just as
> multiple backends would now.  One process would succeed and the others
> would error.  Maybe I'm missing something?

 Hm, any errors happening in the workers would be reported to the
 leader, meaning that even if one worker succeeded to run
 pg_start_backup() it would be reported as an error at the end to the
 client, no? By marking the exclusive function restricted we get sure
 that it is just the leader that fails or succeeds.
>>>
>>> Good point, and it strengthens the argument beyond, "it just seems right."
>>
>> I think the argument should be based on whether or not the function
>> depends on backend-private state that will not be synchronized.
>> That's the definition of what makes something parallel-restricted or
>> not.
>
> Absolutely.  Yesterday was a long day so I may have (perhaps) become a
> bit flippant.
>
>> It looks like pg_start_backup() and pg_stop_backup() depend on the
>> backend-private global variable nonexclusive_backup_running, so they
>> should be parallel-restricted.
>
> Agreed.

How about a separately-committable patch that just does that, and then
a main patch that applies on top of it?

-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-01 Thread David Steele 
On 3/1/17 5:11 PM, Jim Nasby wrote:
> On 2/27/17 6:25 PM, David Steele wrote:
>> The purpose of this patch is to make waiting for archive optional, with
>> the default being the current behavior, i.e., to wait for all WAL to be
>> archived.  This functionality is already used internally by
>> pg_basebackup, so the only real change is to expose it through the
>> pg_stop_backup() function.
> 
> Do the docs mention anywhere how to monitor WAL archiving to know if
> you've got all the necessary WAL? Perhaps a function to do that would be
> worth adding.

The docs in the patch mention that this option should only be used by
backup solutions that know how to monitor archiving:

+This behavior is only useful for backup
+software which independently monitors WAL archiving, otherwise WAL
+required to make the backup consistent might be missing and make
the backup
+useless.

There is already a view, pg_stat_archiver, that allows a program to see
what has been archived.  I'm not sure we'd be adding much by putting a
function around that.

I would be OK with adding a link to pg_stat_archiver in the
pg_stop_backup() documentation to if that seems appropriate.

-- 
-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] PATCH: Make pg_stop_backup() archive wait optional

2017-03-01 Thread Jim Nasby 

On 2/27/17 6:25 PM, David Steele wrote:

The purpose of this patch is to make waiting for archive optional, with
the default being the current behavior, i.e., to wait for all WAL to be
archived.  This functionality is already used internally by
pg_basebackup, so the only real change is to expose it through the
pg_stop_backup() function.


Do the docs mention anywhere how to monitor WAL archiving to know if 
you've got all the necessary WAL? Perhaps a function to do that would be 
worth adding.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] PATCH: Make pg_stop_backup() archive wait optional

2017-02-28 Thread David Steele 
On 2/28/17 10:22 PM, Robert Haas wrote:
> On Tue, Feb 28, 2017 at 6:22 AM, David Steele  wrote:
 I'm not sure that's the case.  It seems like it should lock just as
 multiple backends would now.  One process would succeed and the others
 would error.  Maybe I'm missing something?
>>>
>>> Hm, any errors happening in the workers would be reported to the
>>> leader, meaning that even if one worker succeeded to run
>>> pg_start_backup() it would be reported as an error at the end to the
>>> client, no? By marking the exclusive function restricted we get sure
>>> that it is just the leader that fails or succeeds.
>>
>> Good point, and it strengthens the argument beyond, "it just seems right."
> 
> I think the argument should be based on whether or not the function
> depends on backend-private state that will not be synchronized.
> That's the definition of what makes something parallel-restricted or
> not.

Absolutely.  Yesterday was a long day so I may have (perhaps) become a
bit flippant.

> It looks like pg_start_backup() and pg_stop_backup() depend on the
> backend-private global variable nonexclusive_backup_running, so they
> should be parallel-restricted.

Agreed.

-- 
-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] PATCH: Make pg_stop_backup() archive wait optional

2017-02-28 Thread Robert Haas 
On Tue, Feb 28, 2017 at 6:22 AM, David Steele  wrote:
>>> I'm not sure that's the case.  It seems like it should lock just as
>>> multiple backends would now.  One process would succeed and the others
>>> would error.  Maybe I'm missing something?
>>
>> Hm, any errors happening in the workers would be reported to the
>> leader, meaning that even if one worker succeeded to run
>> pg_start_backup() it would be reported as an error at the end to the
>> client, no? By marking the exclusive function restricted we get sure
>> that it is just the leader that fails or succeeds.
>
> Good point, and it strengthens the argument beyond, "it just seems right."

I think the argument should be based on whether or not the function
depends on backend-private state that will not be synchronized.
That's the definition of what makes something parallel-restricted or
not.

It looks like pg_start_backup() and pg_stop_backup() depend on the
backend-private global variable nonexclusive_backup_running, so they
should be parallel-restricted.

-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-02-27 Thread David Steele 
On 2/27/17 7:50 PM, Michael Paquier wrote:
> On Tue, Feb 28, 2017 at 9:42 AM, David Steele  wrote:
>> On 2/27/17 7:38 PM, Michael Paquier wrote:
>>> On Tue, Feb 28, 2017 at 9:25 AM, David Steele  wrote:
 I also marked the pg_stop_* functions as parallel restricted, the same
 as pg_start_backup().  Previously they were parallel safe which I don't
 believe is accurate for the non-exclusive version at the very least,
 since it is tied to a particular backend.
>>>
>>> Yeah, those should really be parallel restricted. For the exclusive
>>> version, having the function run in parallel would also lead to errors
>>> per the presence/lack of backup_label file.
>>
>> I'm not sure that's the case.  It seems like it should lock just as
>> multiple backends would now.  One process would succeed and the others
>> would error.  Maybe I'm missing something?
> 
> Hm, any errors happening in the workers would be reported to the
> leader, meaning that even if one worker succeeded to run
> pg_start_backup() it would be reported as an error at the end to the
> client, no? By marking the exclusive function restricted we get sure
> that it is just the leader that fails or succeeds.

Good point, and it strengthens the argument beyond, "it just seems right."

-- 
-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] PATCH: Make pg_stop_backup() archive wait optional

2017-02-27 Thread Michael Paquier 
On Tue, Feb 28, 2017 at 9:42 AM, David Steele  wrote:
> On 2/27/17 7:38 PM, Michael Paquier wrote:
>> On Tue, Feb 28, 2017 at 9:25 AM, David Steele  wrote:
>>> I also marked the pg_stop_* functions as parallel restricted, the same
>>> as pg_start_backup().  Previously they were parallel safe which I don't
>>> believe is accurate for the non-exclusive version at the very least,
>>> since it is tied to a particular backend.
>>
>> Yeah, those should really be parallel restricted. For the exclusive
>> version, having the function run in parallel would also lead to errors
>> per the presence/lack of backup_label file.
>
> I'm not sure that's the case.  It seems like it should lock just as
> multiple backends would now.  One process would succeed and the others
> would error.  Maybe I'm missing something?

Hm, any errors happening in the workers would be reported to the
leader, meaning that even if one worker succeeded to run
pg_start_backup() it would be reported as an error at the end to the
client, no? By marking the exclusive function restricted we get sure
that it is just the leader that fails or succeeds.
-- 
Michael


-- 
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] PATCH: Make pg_stop_backup() archive wait optional

2017-02-27 Thread David Steele 
On 2/27/17 7:38 PM, Michael Paquier wrote:
> On Tue, Feb 28, 2017 at 9:25 AM, David Steele  wrote:
>> I also marked the pg_stop_* functions as parallel restricted, the same
>> as pg_start_backup().  Previously they were parallel safe which I don't
>> believe is accurate for the non-exclusive version at the very least,
>> since it is tied to a particular backend.
> 
> Yeah, those should really be parallel restricted. For the exclusive
> version, having the function run in parallel would also lead to errors
> per the presence/lack of backup_label file.

I'm not sure that's the case.  It seems like it should lock just as
multiple backends would now.  One process would succeed and the others
would error.  Maybe I'm missing something?

Either way, I don't think the behavior makes any sense.  Parallel safe
seems more sensible.

-- 
-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] PATCH: Make pg_stop_backup() archive wait optional

2017-02-27 Thread Michael Paquier 
On Tue, Feb 28, 2017 at 9:25 AM, David Steele  wrote:
> I also marked the pg_stop_* functions as parallel restricted, the same
> as pg_start_backup().  Previously they were parallel safe which I don't
> believe is accurate for the non-exclusive version at the very least,
> since it is tied to a particular backend.

Yeah, those should really be parallel restricted. For the exclusive
version, having the function run in parallel would also lead to errors
per the presence/lack of backup_label file.
-- 
Michael


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

[HACKERS] PATCH: Make pg_stop_backup() archive wait optional

2017-02-27 Thread David Steele 
Currently pg_stop_backup() will wait for all WAL to be archived before
returning.  If there is a problem with the archive command or a large
backlog it may not return for a very long time (if ever).  Backup
software is faced with the choice of cancelling the query and hoping the
stop backup record was written or waiting indefinitely.

The purpose of this patch is to make waiting for archive optional, with
the default being the current behavior, i.e., to wait for all WAL to be
archived.  This functionality is already used internally by
pg_basebackup, so the only real change is to expose it through the
pg_stop_backup() function.

I wasn't sure where, if anywhere, to put tests (the underlying
functionality is tested in the pg_basebackup TAP tests).  test/recovery
and bin/pg_basebackup did not seem like appropriate places and the
pg_regress tests are not capable enough.  It seems like a new
test/backup suite would be a good idea but I wanted to get some feedback
before proceeding in that direction.

I also marked the pg_stop_* functions as parallel restricted, the same
as pg_start_backup().  Previously they were parallel safe which I don't
believe is accurate for the non-exclusive version at the very least,
since it is tied to a particular backend.

The patch applies cleanly on 30df93f but will be broken (and easily
fixed) by any cat version bump in later commits.

-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9c53e42..680a0dc 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -18044,7 +18044,7 @@ SELECT set_config('log_statement_stats', 'off', false);
   
   

-pg_stop_backup(exclusive 
boolean)
+pg_stop_backup(exclusive 
boolean , wait_for_archive boolean 
)
 
setof record
Finish performing exclusive or non-exclusive on-line backup 
(restricted to superusers by default, but other users can be granted EXECUTE to 
run the function)
@@ -18128,7 +18128,13 @@ postgres=# select pg_start_backup('label_goes_here');
 pg_start_backup. In a non-exclusive backup, the contents of
 the backup_label and tablespace_map are returned
 in the result of the function, and should be written to files in the
-backup (and not in the data directory).
+backup (and not in the data directory).  There is an optional second
+parameter of type boolean.  If false, the pg_stop_backup
+will return immediately after the backup is completed without waiting for
+WAL to be archived.  This behavior is only useful for backup
+software which independently monitors WAL archiving, otherwise WAL
+required to make the backup consistent might be missing and make the backup
+useless.

 

diff --git a/src/backend/access/transam/xlogfuncs.c 
b/src/backend/access/transam/xlogfuncs.c
index 27c0c56..d8fdacf 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -190,6 +190,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
boolnulls[3];
 
boolexclusive = PG_GETARG_BOOL(0);
+   boolwaitforarchive = PG_GETARG_BOOL(1);
XLogRecPtr  stoppoint;
 
/* check to see if caller supports us returning a tuplestore */
@@ -232,7 +233,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the exclusive backup, and since we're in an exclusive 
backup
 * return NULL for both backup_label and tablespace_map.
 */
-   stoppoint = do_pg_stop_backup(NULL, true, NULL);
+   stoppoint = do_pg_stop_backup(NULL, waitforarchive, NULL);
exclusive_backup_running = false;
 
nulls[1] = true;
@@ -250,7 +251,7 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS)
 * Stop the non-exclusive backup. Return a copy of the backup 
label
 * and tablespace map so they can be written to disk by the 
caller.
 */
-   stoppoint = do_pg_stop_backup(label_file->data, true, NULL);
+   stoppoint = do_pg_stop_backup(label_file->data, waitforarchive, 
NULL);
nonexclusive_backup_running = false;
cancel_before_shmem_exit(nonexclusive_base_backup_cleanup, 
(Datum) 0);
 
diff --git a/src/backend/catalog/system_views.sql 
b/src/backend/catalog/system_views.sql
index 38be9cf..c2ca2b8 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -984,6 +984,12 @@ CREATE OR REPLACE FUNCTION
   RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup'
   PARALLEL RESTRICTED;
 
+CREATE OR REPLACE FUNCTION pg_stop_backup (
+exclusive boolean, wait_for_archive boolean DEFAULT true,
+OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text)
+  RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup'
+  PARALLEL RESTRICTED;
+
 -- legacy definition for compatibility with 9.3
 CREATE