Re: Patch pg_is_in_backup()

Hello,

Yesterday I was facing a little issue with a backup software (NetBackup)
that do not report error when a post backup script is run. The problem
is that this script execute pg_stop_backup() and if there's any failure
PostgreSQL keeps running in on-line backup mode. So the backup is not
completed and the next one too because the call to pg_start_backup()
will fail.

After some time searching for a Pg system administration function like
pg_is_in_recovery(), let's say pg_is_in_backup(), I couldn't find one.
The minor patch attached here adds this administrative function that can
be used with others backup control functions. This is a very little
patch outside documentation because the function is only a wrapper to
the internal BackupInProgress() function, just like pg_is_in_recovery()
is calling RecoveryInProgress().

I hope it could be included as it could help to save time for some
sysadmin who want to monitor on-line backup process and that do not have
SQL knowledge. Saying that it's always possible to check if the
backup_label file exist under PGDATA but this need to be checked on the
host with postgres priviledge. The other way is to create a plpgsql
function with security definer that will have the same effect than the
patch above except that it need some SQL knowledge. I've give it here
for web search, thanks to Guillaume and Marc.

CREATE OR REPLACE FUNCTION pg_is_in_backup ( )
RETURNS BOOLEAN AS $$
DECLARE is_exists BOOLEAN;
BEGIN
-- Set a secure search_path: trusted schemas, then 'pg_temp'.
PERFORM pg_catalog.set_config('search_path', 'pg_temp', true);
SELECT ((pg_stat_file('backup_label')).modification IS NOT NULL)
INTO is_exists;
RETURN is_exists;
EXCEPTION
WHEN undefined_file THEN
RETURN false;
END
$$ LANGUAGE plpgsql SECURITY DEFINER;

Regards,

--
Gilles Darold
http://dalibo.com - http://dalibo.org

On 30-01-2012 15:33, Gilles Darold wrote:
> Yesterday I was facing a little issue with a backup software (NetBackup)
> that do not report error when a post backup script is run. The problem
> is that this script execute pg_stop_backup() and if there's any failure
> PostgreSQL keeps running in on-line backup mode. So the backup is not
> completed and the next one too because the call to pg_start_backup()
> will fail.
>
I use something similar to your pl/PgSQL version and was about to propose
something along these lines to core. +1 to include it. Please, add your patch
to the next CF.

--
Euler Taveira de Oliveira - Timbira http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Le 30/01/2012 22:14, Euler Taveira de Oliveira a écrit :
> On 30-01-2012 15:33, Gilles Darold wrote:
>> Yesterday I was facing a little issue with a backup software (NetBackup)
>> that do not report error when a post backup script is run. The problem
>> is that this script execute pg_stop_backup() and if there's any failure
>> PostgreSQL keeps running in on-line backup mode. So the backup is not
>> completed and the next one too because the call to pg_start_backup()
>> will fail.
> I use something similar to your pl/PgSQL version and was about to propose
> something along these lines to core. +1 to include it. Please, add your patch
> to the next CF.

Ok, it has been added to next CF under the "System administration" new
topic I've just created. I'm not sure about that, so feel free to change
it or let me know.

Best regards,

--
Gilles Darold
http://dalibo.com - http://dalibo.org

On Mon, Jan 30, 2012 at 20:33, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
> After some time searching for a Pg system administration function like
> pg_is_in_recovery(), let's say pg_is_in_backup(), I couldn't find one.
> The minor patch attached here adds this administrative function that can
> be used with others backup control functions. This is a very little
> patch outside documentation because the function is only a wrapper to
> the internal BackupInProgress() function, just like pg_is_in_recovery()
> is calling RecoveryInProgress().

I think it would be more useful to have a function that returns a
timestamp when the backup started. That way, it's possible to write a
generic monitoring script that alerts the sysadmin only when a backup
has been running for too long, but is silent for well-behaved backups.

Regards,
Marti

2012/2/2 Marti Raudsepp <marti(at)juffo(dot)org>:
> On Mon, Jan 30, 2012 at 20:33, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> After some time searching for a Pg system administration function like
>> pg_is_in_recovery(), let's say pg_is_in_backup(), I couldn't find one.
>> The minor patch attached here adds this administrative function that can
>> be used with others backup control functions. This is a very little
>> patch outside documentation because the function is only a wrapper to
>> the internal BackupInProgress() function, just like pg_is_in_recovery()
>> is calling RecoveryInProgress().
>
> I think it would be more useful to have a function that returns a
> timestamp when the backup started. That way, it's possible to write a
> generic monitoring script that alerts the sysadmin only when a backup
> has been running for too long, but is silent for well-behaved backups.

+1

Pavel

>
> Regards,
> Marti
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Le 02/02/2012 12:23, Marti Raudsepp a écrit :
> On Mon, Jan 30, 2012 at 20:33, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> After some time searching for a Pg system administration function like
>> pg_is_in_recovery(), let's say pg_is_in_backup(), I couldn't find one.
>> The minor patch attached here adds this administrative function that can
>> be used with others backup control functions. This is a very little
>> patch outside documentation because the function is only a wrapper to
>> the internal BackupInProgress() function, just like pg_is_in_recovery()
>> is calling RecoveryInProgress().
> I think it would be more useful to have a function that returns a
> timestamp when the backup started. That way, it's possible to write a
> generic monitoring script that alerts the sysadmin only when a backup
> has been running for too long, but is silent for well-behaved backups.

For now the internal function BackupInProgress() only return a boolean.
I like the idea that pg_is_in_backup() just works as pg_is_in_recovery()
do. I mean it doesn't return the timestamp of the recovery starting time
but only true or false.

Maybe we can have an other function that will return all information
available in the backup_label file ?

Regards,

--
Gilles Darold
http://dalibo.com - http://dalibo.org

--On 2. Februar 2012 18:47:35 +0100 Gilles Darold <gilles(dot)darold(at)dalibo(dot)com>
wrote:

> For now the internal function BackupInProgress() only return a boolean.
> I like the idea that pg_is_in_backup() just works as pg_is_in_recovery()
> do. I mean it doesn't return the timestamp of the recovery starting time
> but only true or false.
>
> Maybe we can have an other function that will return all information
> available in the backup_label file ?

I've seen customers using pg_read_file() to do exactly this. E.g. they are
searching for the START TIME value, if backup_label is present, to report the
backup start.

--
Thanks

Bernd

On Thu, Feb 2, 2012 at 12:23, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
> On Mon, Jan 30, 2012 at 20:33, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> After some time searching for a Pg system administration function like
>> pg_is_in_recovery(), let's say pg_is_in_backup(), I couldn't find one.
>> The minor patch attached here adds this administrative function that can
>> be used with others backup control functions. This is a very little
>> patch outside documentation because the function is only a wrapper to
>> the internal BackupInProgress() function, just like pg_is_in_recovery()
>> is calling RecoveryInProgress().
>
> I think it would be more useful to have a function that returns a
> timestamp when the backup started. That way, it's possible to write a
> generic monitoring script that alerts the sysadmin only when a backup
> has been running for too long, but is silent for well-behaved backups.

If there is more than one concurrent backup running, which one do you
return? The first one or the latest one? Or perhaps you need an
interface thta can return them all...

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

On 02-02-2012 20:06, Magnus Hagander wrote:
> If there is more than one concurrent backup running, which one do you
> return? The first one or the latest one? Or perhaps you need an
> interface thta can return them all...
>
IMHO, pg_is_in_backup() should return true if one or more backup copies are
running. As about returning the backup timestamp, we could return an array
like array[['label_1', '2012-01-28 02:00:01 BRST'], ['label_2', '2012-01-28
03:40:34 BRST']] or NULL if none.

--
Euler Taveira de Oliveira - Timbira http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

On Fri, Feb 3, 2012 at 11:31 AM, Euler Taveira de Oliveira
<euler(at)timbira(dot)com> wrote:
> On 02-02-2012 20:06, Magnus Hagander wrote:
>> If there is more than one concurrent backup running, which one do you
>> return? The first one or the latest one? Or perhaps you need an
>> interface thta can return them all...
>>
> IMHO, pg_is_in_backup() should return true if one or more backup copies are
> running. As about returning the backup timestamp, we could return an array
> like array[['label_1', '2012-01-28 02:00:01 BRST'], ['label_2', '2012-01-28
> 03:40:34 BRST']] or NULL if none.

It seems to be more user-friendly to introduce a view like pg_stat_backup
rather than the function returning an array.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> It seems to be more user-friendly to introduce a view like pg_stat_backup
> rather than the function returning an array.

I like this idea. A use case i saw for monitoring backup_label's in the past,
was mainly to discover a forgotten exclusive pg_stop_backup() (e.g. due to
broken backup scripts). If the view would be able to distinguish both,
exclusive and non-exclusive backups, this would be great.

--
Thanks

Bernd

On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
> wrote:
>
>> It seems to be more user-friendly to introduce a view like pg_stat_backup
>> rather than the function returning an array.
>
>
> I like this idea. A use case i saw for monitoring backup_label's in the
> past, was mainly to discover a forgotten exclusive pg_stop_backup() (e.g.
> due to broken backup scripts). If the view would be able to distinguish
> both, exclusive and non-exclusive backups, this would be great.

Agreed. Monitoring an exclusive backup is very helpful. But I wonder
why we want to monitor non-exclusive backup. Is there any use case?
If we want to monitor non-exclusive backup, why not pg_dump backup?

If there is no use case, it seems sufficient to implement the function
which reports the information only about exclusive backup.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Feb 3, 2012 at 10:47, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>>
>>
>> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>> wrote:
>>
>>> It seems to be more user-friendly to introduce a view like pg_stat_backup
>>> rather than the function returning an array.
>>
>>
>> I like this idea. A use case i saw for monitoring backup_label's in the
>> past, was mainly to discover a forgotten exclusive pg_stop_backup() (e.g.
>> due to broken backup scripts). If the view would be able to distinguish
>> both, exclusive and non-exclusive backups, this would be great.
>
> Agreed. Monitoring an exclusive backup is very helpful. But I wonder
> why we want to monitor non-exclusive backup. Is there any use case?

Actually, we can already monitor much of the non-exclusive one through
pg_stat_replication. Including the info on when it was started (at
least in almost every case, that will be more or less the
backend_start time for that one)

> If we want to monitor non-exclusive backup, why not pg_dump backup?

.. which we can also monitor though pg_stat_activity by looking at
application_name (which can be faked of course, but still)

> If there is no use case, it seems sufficient to implement the function
> which reports the information only about exclusive backup.

Yeah, thinking more of it, i think I agree. But the function should
then probably be named in such a way that it's clear that we're
talking about exclusive backups, e.g. not pg_is_in_backup() but
instead pg_is_in_exclusive_backup() (renamed if we change it to return
the timestamp instead, of course, but you get the idea)

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

On Fri, Feb 3, 2012 at 6:52 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Feb 3, 2012 at 10:47, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>>>
>>>
>>> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>>> wrote:
>>>
>>>> It seems to be more user-friendly to introduce a view like pg_stat_backup
>>>> rather than the function returning an array.
>>>
>>>
>>> I like this idea. A use case i saw for monitoring backup_label's in the
>>> past, was mainly to discover a forgotten exclusive pg_stop_backup() (e.g.
>>> due to broken backup scripts). If the view would be able to distinguish
>>> both, exclusive and non-exclusive backups, this would be great.
>>
>> Agreed. Monitoring an exclusive backup is very helpful. But I wonder
>> why we want to monitor non-exclusive backup. Is there any use case?
>
> Actually, we can already monitor much of the non-exclusive one through
> pg_stat_replication. Including the info on when it was started (at
> least in almost every case, that will be more or less the
> backend_start time for that one)

Right.

>> If we want to monitor non-exclusive backup, why not pg_dump backup?
>
> .. which we can also monitor though pg_stat_activity by looking at
> application_name (which can be faked of course, but still)

Yep.

>> If there is no use case, it seems sufficient to implement the function
>> which reports the information only about exclusive backup.
>
> Yeah, thinking more of it, i think I agree. But the function should
> then probably be named in such a way that it's clear that we're
> talking about exclusive backups, e.g. not pg_is_in_backup() but
> instead pg_is_in_exclusive_backup() (renamed if we change it to return
> the timestamp instead, of course, but you get the idea)

Agreed.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Le 03/02/2012 10:52, Magnus Hagander a écrit :
> On Fri, Feb 3, 2012 at 10:47, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>>>
>>> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>>> wrote:
>>>
>>>> It seems to be more user-friendly to introduce a view like pg_stat_backup
>>>> rather than the function returning an array.
>>>
>>> I like this idea. A use case i saw for monitoring backup_label's in the
>>> past, was mainly to discover a forgotten exclusive pg_stop_backup() (e.g.
>>> due to broken backup scripts). If the view would be able to distinguish
>>> both, exclusive and non-exclusive backups, this would be great.
>> Agreed. Monitoring an exclusive backup is very helpful. But I wonder
>> why we want to monitor non-exclusive backup. Is there any use case?
> Actually, we can already monitor much of the non-exclusive one through
> pg_stat_replication. Including the info on when it was started (at
> least in almost every case, that will be more or less the
> backend_start time for that one)
>
>> If we want to monitor non-exclusive backup, why not pg_dump backup?
> .. which we can also monitor though pg_stat_activity by looking at
> application_name (which can be faked of course, but still)
>
>> If there is no use case, it seems sufficient to implement the function
>> which reports the information only about exclusive backup.
> Yeah, thinking more of it, i think I agree. But the function should
> then probably be named in such a way that it's clear that we're
> talking about exclusive backups, e.g. not pg_is_in_backup() but
> instead pg_is_in_exclusive_backup() (renamed if we change it to return
> the timestamp instead, of course, but you get the idea)

Agreed and sorry for the response delay. I've attached 2 patches here,
the first one is the same as before with just the renaming of the
function into pg_is_in_exclusive_backup(). Maybe this patch should be
abandoned in favor of the second one which introduce a new function
called pg_exclusive_backup_start_time() that return the backup_label
START TIME information as a timestamp with timezone.

Sample usage/result:

postgres=# select pg_start_backup('Online backup');
pg_start_backup
-----------------
0/2000020
(1 ligne)

postgres=# select pg_exclusive_backup_start_time();
pg_exclusive_backup_start_time
--------------------------------
2012-03-02 14:52:49+01
(1 ligne)

postgres=# select now() - pg_exclusive_backup_start_time() as
backup_started_since;
backup_started_since
------------------------
00:12:24.569226
(1 ligne)

postgres=# select pg_stop_backup();
pg_stop_backup
----------------
0/2000098
(1 ligne)

postgres=# select pg_exclusive_backup_start_time();
pg_exclusive_backup_start_time
--------------------------------

(1 ligne)

Regards,

--
Gilles Darold
http://dalibo.com - http://dalibo.org

Hi Gilles,

first and foremost, sorry for jumping in this thread so late. I read
all previous discussions and I'd be happy to help you with this patch.

> Agreed and sorry for the response delay. I've attached 2 patches here,
> the first one is the same as before with just the renaming of the
> function into pg_is_in_exclusive_backup().

My quick response: I really like the idea of having a function that
simply returns a boolean value. To be honest, I would have called it
pg_is_in_backup() as you originally did - after all, I do not execute
pg_start_exclusive_backup() or pg_stop_exclusive_backup(). It is more
intuitive and pairs with pg_is_in_recovery(). I have never found any
mention whatsoever in the documentation that talks about exclusive
backup, and I am afraid it would generate confusion.

However, I leave this up to the rest of the community's judgement
(here is my opinion).

I agree also that a function that returns the timestamp of the start
of the backup might be useful. My opinion with the 'exclusive' keyword
applies here too (I would simply name it pg_backup_start_time()).

Finally, I tried to apply the patch but it looks like we need a new
version that can apply with current HEAD. If you can do that, I am happy
to assist with the review.

Have a good weekend.

Thank you,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

Hi Gabriele,

Le 31/03/2012 14:25, Gabriele Bartolini a écrit :
> Hi Gilles,
>
> first and foremost, sorry for jumping in this thread so late. I
> read all previous discussions and I'd be happy to help you with this
> patch.
>
>> Agreed and sorry for the response delay. I've attached 2 patches
>> here, the first one is the same as before with just the renaming of
>> the function into pg_is_in_exclusive_backup().
>
> My quick response: I really like the idea of having a function that
> simply returns a boolean value. To be honest, I would have called it
> pg_is_in_backup() as you originally did - after all, I do not execute
> pg_start_exclusive_backup() or pg_stop_exclusive_backup(). It is more
> intuitive and pairs with pg_is_in_recovery(). I have never found any
> mention whatsoever in the documentation that talks about exclusive
> backup, and I am afraid it would generate confusion.

+1, this is also my point of view.

> However, I leave this up to the rest of the community's judgement
> (here is my opinion).
>
> I agree also that a function that returns the timestamp of the
> start of the backup might be useful. My opinion with the 'exclusive'
> keyword applies here too (I would simply name it pg_backup_start_time()).

Ok, I've reverted the name to pg_is_in_backup() and
pg_backup_start_time(), if at the end the functions names have to be
changed a simple replacement in the patch will be easy to do.

> Finally, I tried to apply the patch but it looks like we need a new
> version that can apply with current HEAD. If you can do that, I am
> happy to assist with the review.

I've attach 3 patches, the first one include both functions, the others
are patches per function. They are all from the current HEAD branch, if
they don't work please help me with the correct git/diff command to perform.

Thank you, regards,

--
Gilles Darold
http://dalibo.com - http://dalibo.org

Hi Gilles,

Sorry for the delay.

Il 03/04/12 14:21, Gilles Darold ha scritto:
> +1, this is also my point of view.

I have looked at the patch that contains both pg_is_in_backup() and
pg_backup_start_time().

From a functional point of view it looks fine to me. I was thinking
of adding the BackupInProgress() at the beginning of
pg_backup_start_time(), but the AllocateFile() function already make
sure the file exists.

I have performed some basic testing of both functions and tried to
inject invalid characters in the start time field of the backup_label
file and it is handled (with an exception) by the server. Cool.

I spotted though some formatting issues, in particular indentation
and multi-line comments. Some rows are longer than 80 chars.

Please resubmit with these cosmetic changes and it is fine with me.
Thank you.

Cheers,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

Sorry for the the double post but it seems that my previous reply
doesn't reach the pgsql-hacker list. So here is the new patches that
limit lines to 80 characters.

Regards,

Le 02/05/2012 19:53, Gabriele Bartolini a écrit :
> Hi Gilles,
>
> Sorry for the delay.
>
> Il 03/04/12 14:21, Gilles Darold ha scritto:
>> +1, this is also my point of view.
>
> I have looked at the patch that contains both pg_is_in_backup() and
> pg_backup_start_time().
>
> From a functional point of view it looks fine to me. I was thinking
> of adding the BackupInProgress() at the beginning of
> pg_backup_start_time(), but the AllocateFile() function already make
> sure the file exists.
>
> I have performed some basic testing of both functions and tried to
> inject invalid characters in the start time field of the backup_label
> file and it is handled (with an exception) by the server. Cool.
>
> I spotted though some formatting issues, in particular indentation
> and multi-line comments. Some rows are longer than 80 chars.
>
> Please resubmit with these cosmetic changes and it is fine with me.
> Thank you.
>
> Cheers,
> Gabriele
>

--
Gilles Darold
Administrateur de bases de données
http://dalibo.com - http://dalibo.org

Hi Gilles,

unfortunately Gabriele has been very busy recently and he asked me to
check again the status of this patch for this commit fest. In order to
speed up the application of the patch, I am sending an updated version
which correctly compiles with current head. Gabriele will then proceed
with the review.

Thank you,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco(dot)nenciarini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

Hello Gilles,

thank you very much for your patience (and thank you Marco for
supporting me). I apologise for the delay.

I have retested the updated patch and it works fine with me. It is
"ready for committer" for me.

Cheers,
Gabriele

Il 14/06/12 11:36, Marco Nenciarini ha scritto:
> Hi Gilles,
>
> unfortunately Gabriele has been very busy recently and he asked me to
> check again the status of this patch for this commit fest. In order to
> speed up the application of the patch, I am sending an updated version
> which correctly compiles with current head. Gabriele will then proceed
> with the review.
>
> Thank you,
> Marco
>
>
>
> This body part will be downloaded on demand.

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele(dot)bartolini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
<gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>    thank you very much for your patience (and thank you Marco for supporting
> me). I apologise for the delay.
>
>    I have retested the updated patch and it works fine with me. It is "ready
> for committer" for me.

Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
> <gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
> > thank you very much for your patience (and thank you Marco for
> supporting
> > me). I apologise for the delay.
> >
> > I have retested the updated patch and it works fine with me. It is
> "ready
> > for committer" for me.
>
> Committed.

A minor gripe:

+ /*
+ * Close the backup label file.
+ */
+ if (ferror(lfp) || FreeFile(lfp)) {
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m",
+ BACKUP_LABEL_FILE)));
+ }
+

If ferror(lfp) returns false, wouldn't we miss the FreeFile() and leak a
file pointer?

Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> wrote:
> On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
>> <gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>> >    thank you very much for your patience (and thank you Marco for
>> > supporting
>> > me). I apologise for the delay.
>> >
>> >    I have retested the updated patch and it works fine with me. It is
>> > "ready
>> > for committer" for me.
>>
>> Committed.
>
>
> A minor gripe:
>
> +     /*
> +     * Close the backup label file.
> +     */
> +     if (ferror(lfp) || FreeFile(lfp)) {
> +            ereport(ERROR,
> +                (errcode_for_file_access(),
> +                errmsg("could not read file \"%s\": %m",
> +                             BACKUP_LABEL_FILE)));
> +     }
> +
>
> If ferror(lfp) returns false, wouldn't we miss the FreeFile() and leak a
> file pointer?

Well, according to the comments for AllocateFile:

* fd.c will automatically close all files opened with AllocateFile at
* transaction commit or abort; this prevents FD leakage if a routine
* that calls AllocateFile is terminated prematurely by ereport(ERROR).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jun 15, 2012 at 2:29 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
> <gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>>    thank you very much for your patience (and thank you Marco for supporting
>> me). I apologise for the delay.
>>
>>    I have retested the updated patch and it works fine with me. It is "ready
>> for committer" for me.
>
> Committed.

I think the attached patch needs to be applied.

Regards,

--
Fujii Masao

On Thu, Jun 14, 2012 at 2:05 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Jun 15, 2012 at 2:29 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
>> <gabriele(dot)bartolini(at)2ndquadrant(dot)it> wrote:
>>>    thank you very much for your patience (and thank you Marco for supporting
>>> me). I apologise for the delay.
>>>
>>>    I have retested the updated patch and it works fine with me. It is "ready
>>> for committer" for me.
>>
>> Committed.
>
> I think the attached patch needs to be applied.

Thanks, committed.

(Somebody should really give you your own commit bit.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 14, 2012 at 2:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com>
> wrote:
> > On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >> Committed.
> >
> >
> > A minor gripe:
> >
> > + /*
> > + * Close the backup label file.
> > + */
> > + if (ferror(lfp) || FreeFile(lfp)) {
> > + ereport(ERROR,
> > + (errcode_for_file_access(),
> > + errmsg("could not read file \"%s\": %m",
> > + BACKUP_LABEL_FILE)));
> > + }
> > +
> >
> > If ferror(lfp) returns false, wouldn't we miss the FreeFile() and leak a
> > file pointer?
>
> Well, according to the comments for AllocateFile:
>
> * fd.c will automatically close all files opened with AllocateFile at
> * transaction commit or abort; this prevents FD leakage if a routine
> * that calls AllocateFile is terminated prematurely by ereport(ERROR).
>

I bet anyone else looking at this code, who is not in the know, will trip
over this again.

Another problem with that code block is that it will throw "could not read"
even though read has succeeded but FreeFile() failed.

I say we break it into two blocks, one to handle read error, and then close
the file separately. Also, either make sure FreeFile() is called in all
code paths, or not call FreeFile() at all and reference to the comment
above AllocateFile().

Patch attached.

Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

On Thu, Jun 14, 2012 at 3:10 PM, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> wrote:
>> Well, according to the comments for AllocateFile:
>>
>>  * fd.c will automatically close all files opened with AllocateFile at
>>  * transaction commit or abort; this prevents FD leakage if a routine
>>  * that calls AllocateFile is terminated prematurely by ereport(ERROR).
>
> I bet anyone else looking at this code, who is not in the know, will trip
> over this again.
>
> Another problem with that code block is that it will throw "could not read"
> even though read has succeeded but FreeFile() failed.
>
> I say we break it into two blocks, one to handle read error, and then close
> the file separately. Also, either make sure FreeFile() is called in all code
> paths, or not call FreeFile() at all and reference to the comment above
> AllocateFile().
>
> Patch attached.

I agree with breaking it into two blocks, but I don't agree that the
comments need to recapitulate how AllocateFile works. Also, you had
the same primary content for both comments, and you incorrectly
reversed the sense of the FreeFile() test.

Committed with those changes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 14, 2012 at 3:20 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Also, you had
> the same primary content for both comments, and you incorrectly
> reversed the sense of the FreeFile() test.
>

I am known for my fat fingers :)

> Committed with those changes.
>

Thanks!
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company