PATCH: Make pg_stop_backup() archive wait optional
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
david@pgmasters.net
Attachments:
pgstopbackup-wait-v1.patchtext/plain; charset=UTF-8; name=pgstopbackup-wait-v1.patch; x-mac-creator=0; x-mac-type=0Download
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);
</row>
<row>
<entry>
- <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</>)</function></literal>
+ <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</> <optional>, <parameter>wait_for_archive</> <type>boolean</> </optional>)</function></literal>
</entry>
<entry><type>setof record</type></entry>
<entry>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)</entry>
@@ -18128,7 +18128,13 @@ postgres=# select pg_start_backup('label_goes_here');
<function>pg_start_backup</>. In a non-exclusive backup, the contents of
the <filename>backup_label</> and <filename>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 <function>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.
</para>
<para>
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)
bool nulls[3];
bool exclusive = PG_GETARG_BOOL(0);
+ bool waitforarchive = 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 OR REPLACE FUNCTION
json_populate_record(base anyelement, from_json json, use_json_as_text boolean DEFAULT false)
@@ -1084,7 +1090,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 ON FUNCTION pg_switch_wal() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 9e8c1c2..df47eea 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 201702231
+#define CATALOG_VERSION_NO 201702271
#endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index a4cc86d..68735c3 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_backup PGNSP 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_backup PGNSP 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_backup PGNSP 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_backup PGNSP PGUID 12 1 1 0 0 f f f f t t v r 2 0 2249 "16 16" "{16,16,3220,25,25}" "{i,i,o,o,o}" "{exclusive,wait_for_archive,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");
On Tue, Feb 28, 2017 at 9:25 AM, David Steele <david@pgmasters.net> 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
On 2/27/17 7:38 PM, Michael Paquier wrote:
On Tue, Feb 28, 2017 at 9:25 AM, David Steele <david@pgmasters.net> 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
david@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
On Tue, Feb 28, 2017 at 9:42 AM, David Steele <david@pgmasters.net> wrote:
On 2/27/17 7:38 PM, Michael Paquier wrote:
On Tue, Feb 28, 2017 at 9:25 AM, David Steele <david@pgmasters.net> 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
On 2/27/17 7:50 PM, Michael Paquier wrote:
On Tue, Feb 28, 2017 at 9:42 AM, David Steele <david@pgmasters.net> wrote:
On 2/27/17 7:38 PM, Michael Paquier wrote:
On Tue, Feb 28, 2017 at 9:25 AM, David Steele <david@pgmasters.net> 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
david@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
On Tue, Feb 28, 2017 at 6:22 AM, David Steele <david@pgmasters.net> 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
On 2/28/17 10:22 PM, Robert Haas wrote:
On Tue, Feb 28, 2017 at 6:22 AM, David Steele <david@pgmasters.net> 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
david@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
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
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
david@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
On Wed, Mar 1, 2017 at 9:07 AM, David Steele <david@pgmasters.net> wrote:
On 2/28/17 10:22 PM, Robert Haas wrote:
On Tue, Feb 28, 2017 at 6:22 AM, David Steele <david@pgmasters.net> 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
Hi Robert,
On 3/4/17 1:58 AM, Robert Haas wrote:
On Wed, Mar 1, 2017 at 9:07 AM, David Steele <david@pgmasters.net> wrote:
On 2/28/17 10:22 PM, Robert Haas wrote:
On Tue, Feb 28, 2017 at 6:22 AM, David Steele <david@pgmasters.net> 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
david@pgmasters.net
Attachments:
pgstopbackup-wait-01-flags-v2.patchtext/plain; charset=UTF-8; name=pgstopbackup-wait-01-flags-v2.patch; x-mac-creator=0; x-mac-type=0Download
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 @@
*/
/* yyyymmddN */
-#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_backup PGNSP 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_backup PGNSP 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_backup PGNSP 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_backup PGNSP 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");
pgstopbackup-wait-02-param-v2.patchtext/plain; charset=UTF-8; name=pgstopbackup-wait-02-param-v2.patch; x-mac-creator=0; x-mac-type=0Download
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);
</row>
<row>
<entry>
- <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</>)</function></literal>
+ <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</> <optional>, <parameter>wait_for_archive</> <type>boolean</> </optional>)</function></literal>
</entry>
<entry><type>setof record</type></entry>
<entry>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)</entry>
@@ -18182,7 +18182,13 @@ postgres=# select pg_start_backup('label_goes_here');
<function>pg_start_backup</>. In a non-exclusive backup, the contents of
the <filename>backup_label</> and <filename>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 <function>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.
</para>
<para>
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 27c0c56..77feb75 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)
bool nulls[3];
bool exclusive = PG_GETARG_BOOL(0);
+ bool wait_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 ba980de..791c1a2 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 EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public;
REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_switch_wal() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index b17a236..16448c0 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 201703041
+#define CATALOG_VERSION_NO 201703042
#endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index ec4aedb..fa4b5bf 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3143,7 +3143,7 @@ DATA(insert OID = 2172 ( pg_start_backup PGNSP PGUID 12 1 0 0 0 f f f f t f v r
DESCR("prepare for taking an online backup");
DATA(insert OID = 2173 ( pg_stop_backup PGNSP 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_backup PGNSP 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_ ));
+DATA(insert OID = 2739 ( pg_stop_backup PGNSP PGUID 12 1 1 0 0 f f f f t t v r 2 0 2249 "16 16" "{16,16,3220,25,25}" "{i,i,o,o,o}" "{exclusive,wait_for_archive,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");
On Sat, Mar 4, 2017 at 9:12 AM, David Steele <david@pgmasters.net> 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 backupI 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
Robert, all,
* Robert Haas (robertmhaas@gmail.com) wrote:
On Sat, Mar 4, 2017 at 9:12 AM, David Steele <david@pgmasters.net> 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 backupI 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
On Mon, Mar 6, 2017 at 12:53 PM, Stephen Frost <sfrost@snowman.net> 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
Hi Robert,
On 3/6/17 12:48 PM, Robert Haas wrote:
On Sat, Mar 4, 2017 at 9:12 AM, David Steele <david@pgmasters.net> 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 backupI 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
david@pgmasters.net
Attachments:
pgstopbackup-wait-v3.patchtext/plain; charset=UTF-8; name=pgstopbackup-wait-v3.patch; x-mac-creator=0; x-mac-type=0Download
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);
</row>
<row>
<entry>
- <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</>)</function></literal>
+ <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</> <optional>, <parameter>wait_for_archive</> <type>boolean</> </optional>)</function></literal>
</entry>
<entry><type>setof record</type></entry>
<entry>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)</entry>
@@ -18383,7 +18383,13 @@ postgres=# select pg_start_backup('label_goes_here');
<function>pg_start_backup</>. In a non-exclusive backup, the contents of
the <filename>backup_label</> and <filename>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 <function>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.
</para>
<para>
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)
bool nulls[3];
bool exclusive = PG_GETARG_BOOL(0);
+ bool wait_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 EXECUTE ON FUNCTION pg_stop_backup(boolean, boolean) FROM public;
REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_switch_wal() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 9f5e302..4581c44 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
*/
/* yyyymmddN */
-#define CATALOG_VERSION_NO 201703101
+#define CATALOG_VERSION_NO 201703131
#endif
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index ec4aedb..fa4b5bf 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3143,7 +3143,7 @@ DATA(insert OID = 2172 ( pg_start_backup PGNSP PGUID 12 1 0 0 0 f f f f t f v r
DESCR("prepare for taking an online backup");
DATA(insert OID = 2173 ( pg_stop_backup PGNSP 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_backup PGNSP 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_ ));
+DATA(insert OID = 2739 ( pg_stop_backup PGNSP PGUID 12 1 1 0 0 f f f f t t v r 2 0 2249 "16 16" "{16,16,3220,25,25}" "{i,i,o,o,o}" "{exclusive,wait_for_archive,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");
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@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/B0000F8,"START WAL LOCATION: 0/B000028 (file 00000001000000000000000B)+
CHECKPOINT LOCATION: 0/B000060 +
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/D000028 (file 00000001000000000000000D)+
CHECKPOINT LOCATION: 0/D000060 +
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
On Fri, Mar 17, 2017 at 1:47 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> 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
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
On Fri, Mar 17, 2017 at 1:47 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> 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
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@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
On 3/17/17 4:18 AM, Tsunakawa, Takayuki wrote:
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@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
david@pgmasters.net
Attachments:
pgstopbackup-wait-v4.patchtext/plain; charset=UTF-8; name=pgstopbackup-wait-v4.patch; x-mac-creator=0; x-mac-type=0Download
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);
</row>
<row>
<entry>
- <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</>)</function></literal>
+ <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</> <optional>, <parameter>wait_for_archive</> <type>boolean</> </optional>)</function></literal>
</entry>
<entry><type>setof record</type></entry>
<entry>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)</entry>
@@ -18439,7 +18439,13 @@ postgres=# select pg_start_backup('label_goes_here');
<function>pg_start_backup</>. In a non-exclusive backup, the contents of
the <filename>backup_label</> and <filename>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 <function>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.
</para>
<para>
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)
bool nulls[3];
bool exclusive = PG_GETARG_BOOL(0);
+ bool wait_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 ON FUNCTION pg_switch_wal() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 836d6ff..2263565 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3172,7 +3172,7 @@ DATA(insert OID = 2172 ( pg_start_backup PGNSP PGUID 12 1 0 0 0 f f f f t f v r
DESCR("prepare for taking an online backup");
DATA(insert OID = 2173 ( pg_stop_backup PGNSP 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_backup PGNSP 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_ ));
+DATA(insert OID = 2739 ( pg_stop_backup PGNSP PGUID 12 1 1 0 0 f f f f t t v r 2 0 2249 "16 16" "{16,16,3220,25,25}" "{i,i,o,o,o}" "{exclusive,wait_for_archive,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");
From: David Steele [mailto:david@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
On Tue, Mar 21, 2017 at 11:03 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
From: David Steele [mailto:david@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
On 3/21/17 2:34 PM, Fujii Masao wrote:
On Tue, Mar 21, 2017 at 11:03 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:From: David Steele [mailto:david@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
david@pgmasters.net
Attachments:
pgstopbackup-wait-v5.patchtext/plain; charset=UTF-8; name=pgstopbackup-wait-v5.patch; x-mac-creator=0; x-mac-type=0Download
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);
<para>
In the same connection as before, issue the command:
<programlisting>
-SELECT * FROM pg_stop_backup(false);
+SELECT * FROM pg_stop_backup(false [, true ]);
</programlisting>
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.
</para>
<para>
+ If the backup process monitors the WAL archiving process independently,
+ the second parameter (which defaults to true) can be set to false to
+ prevent <function>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.
+ </para>
+ <para>
The <function>pg_stop_backup</> will return one row with three
values. The second of these fields should be written to a file named
<filename>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);
</row>
<row>
<entry>
- <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</>)</function></literal>
+ <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</> <optional>, <parameter>wait_for_archive</> <type>boolean</> </optional>)</function></literal>
</entry>
<entry><type>setof record</type></entry>
<entry>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)</entry>
@@ -18439,7 +18439,13 @@ postgres=# select pg_start_backup('label_goes_here');
<function>pg_start_backup</>. In a non-exclusive backup, the contents of
the <filename>backup_label</> and <filename>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 <function>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.
</para>
<para>
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)
bool nulls[3];
bool exclusive = PG_GETARG_BOOL(0);
+ bool wait_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 ON FUNCTION pg_switch_wal() FROM public;
REVOKE EXECUTE ON FUNCTION pg_wal_replay_pause() FROM public;
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 836d6ff..2263565 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -3172,7 +3172,7 @@ DATA(insert OID = 2172 ( pg_start_backup PGNSP PGUID 12 1 0 0 0 f f f f t f v r
DESCR("prepare for taking an online backup");
DATA(insert OID = 2173 ( pg_stop_backup PGNSP 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_backup PGNSP 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_ ));
+DATA(insert OID = 2739 ( pg_stop_backup PGNSP PGUID 12 1 1 0 0 f f f f t t v r 2 0 2249 "16 16" "{16,16,3220,25,25}" "{i,i,o,o,o}" "{exclusive,wait_for_archive,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");
David, all,
* David Steele (david@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
On Thu, Mar 23, 2017 at 12:37 AM, Stephen Frost <sfrost@snowman.net> wrote:
David, all,
* David Steele (david@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 <function>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
Fujii,
* Fujii Masao (masao.fujii@gmail.com) wrote:
On Thu, Mar 23, 2017 at 12:37 AM, Stephen Frost <sfrost@snowman.net> wrote:
* David Steele (david@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 <function>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
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
* Peter Eisentraut (peter.eisentraut@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
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
david@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
* David Steele (david@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
On 4/4/17 11:42 AM, Stephen Frost wrote:
* David Steele (david@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
david@pgmasters.net