Hi Robert,
On 3/4/17 1:58 AM, Robert Haas wrote:
> On Wed, Mar 1, 2017 at 9:07 AM, David Steele <[email protected]> wrote:
>> On 2/28/17 10:22 PM, Robert Haas wrote:
>>> On Tue, Feb 28, 2017 at 6:22 AM, David Steele <[email protected]> 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
[email protected]
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");
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");
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
