Re: Updated backup APIs for non-exclusive backups

On Fri, Apr 1, 2016 at 6:47 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sat, Mar 19, 2016 at 5:45 PM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
>
>> On Wed, Mar 2, 2016 at 6:49 PM, Marco Nenciarini <
>> marco(dot)nenciarini(at)2ndquadrant(dot)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.

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