Thread: 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
Attachment
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, Suporte24x7 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.darold@dalibo.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@juffo.org>: > On Mon, Jan 30, 2012 at 20:33, Gilles Darold <gilles.darold@dalibo.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@postgresql.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.darold@dalibo.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.darold@dalibo.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@juffo.org> wrote: > On Mon, Jan 30, 2012 at 20:33, Gilles Darold <gilles.darold@dalibo.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, Suporte24x7 e Treinamento
On Fri, Feb 3, 2012 at 11:31 AM, Euler Taveira de Oliveira <euler@timbira.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.fujii@gmail.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@oopsware.de> wrote: > > > --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao.fujii@gmail.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.fujii@gmail.com> wrote: > On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings@oopsware.de> wrote: >> >> >> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao.fujii@gmail.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@hagander.net> wrote: > On Fri, Feb 3, 2012 at 10:47, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings@oopsware.de> wrote: >>> >>> >>> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao.fujii@gmail.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 :
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:
On Fri, Feb 3, 2012 at 10:47, Fujii Masao <masao.fujii@gmail.com> wrote:On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle <mailings@oopsware.de> wrote:--On 3. Februar 2012 13:21:11 +0900 Fujii Masao <masao.fujii@gmail.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');Regards,
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)
-- Gilles Darold http://dalibo.com - http://dalibo.org
Attachment
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.bartolini@2ndQuadrant.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
Attachment
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.bartolini@2ndQuadrant.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
Attachment
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.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachment
Hello Gilles,<br /><br /> thank you very much for your patience (and thank you Marco for supporting me). I apologisefor the delay.<br /><br /> I have retested the updated patch and it works fine with me. It is "ready for committer"for me.<br /><br /> Cheers,<br /> Gabriele<br /><br /><br /> Il 14/06/12 11:36, Marco Nenciarini ha scritto: <blockquotecite="mid:1339666579.3400.6.camel@greygoo.devise-it.lan" type="cite"><pre wrap="">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 </pre><br /><fieldset class="mimeAttachmentHeader"></fieldset><br /><pre wrap="">This body part will be downloaded on demand.</pre></blockquote><br/><br /><pre class="moz-signature" cols="72">-- Gabriele Bartolini - 2ndQuadrant ItaliaPostgreSQLTraining, Services and Support<a class="moz-txt-link-abbreviated" href="mailto:gabriele.bartolini@2ndQuadrant.it">gabriele.bartolini@2ndQuadrant.it</a>| <a class="moz-txt-link-abbreviated"href="http://www.2ndQuadrant.it">www.2ndQuadrant.it</a> </pre>
On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini <gabriele.bartolini@2ndquadrant.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@gmail.com> wrote:
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?
On Thu, Jun 14, 2012 at 6:06 AM, Gabriele BartoliniCommitted.
<gabriele.bartolini@2ndquadrant.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.
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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
>> <gabriele.bartolini@2ndquadrant.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
leakageif 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@gmail.com> wrote: > On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini > <gabriele.bartolini@2ndquadrant.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
Attachment
On Thu, Jun 14, 2012 at 2:05 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Fri, Jun 15, 2012 at 2:29 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini >> <gabriele.bartolini@2ndquadrant.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@gmail.com> wrote:
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,
-- Well, according to the comments for AllocateFile:On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas <robertmhaas@gmail.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?
* 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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Attachment
On Thu, Jun 14, 2012 at 3:10 PM, Gurjeet Singh <singh.gurjeet@gmail.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@gmail.com> wrote:
I am known for my fat fingers :)
Thanks!
-- 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
EnterpriseDB Corporation
The Enterprise PostgreSQL Company