Re: Updated backup APIs for non-exclusive backups - Mailing list pgsql-hackers

From Magnus Hagander
Subject Re: Updated backup APIs for non-exclusive backups
Date
Msg-id CABUevExQBvGJJH54Nb0rzrt=AEwQj8qWWXNzNBanMxiN5u4yMQ@mail.gmail.com
Whole thread Raw
In response to Re: Updated backup APIs for non-exclusive backups  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: Updated backup APIs for non-exclusive backups  (Amit Kapila <amit.kapila16@gmail.com>)
List pgsql-hackers
On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Mar 19, 2016 at 5:45 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Wed, Mar 2, 2016 at 6:49 PM, Marco Nenciarini <marco.nenciarini@2ndquadrant.it> wrote:

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



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

+      </row>

+      <row>

+       <entry>

+        <literal><function>pg_stop_backup(<parameter>exclusive</> <type>boolean</>)</function></literal>

+        </entry>

+       <entry><type>setof record</type></entry>

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



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

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

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

<para>

    <function>pg_start_backup</> accepts an

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

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

    writes a backup label file (<filename>backup_label</>) and, if there

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

    file (<filename>tablespace_map</>) into the database cluster's data

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

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

    but it is provided in case it is useful.


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

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

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

 

CREATE OR REPLACE FUNCTION

-  pg_start_backup(label text, fast boolean DEFAULT false)

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

   RETURNS pg_lsn STRICT VOLATILE LANGUAGE internal AS 'pg_start_backup';



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

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

--

pgsql-hackers by date:

Previous
From: Etsuro Fujita
Date:
Subject: Re: Optimization for updating foreign tables in Postgres FDW
Next
From: Etsuro Fujita
Date:
Subject: Incorrect comment in contrib/postgres_fdw/deparse.c