Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 3:20 PM, Robert Haas 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
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 3:10 PM, Gurjeet Singh 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 2:02 PM, Robert Haas wrote: > On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh > wrote: > > On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas > 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 FreeFile.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 2:05 PM, Fujii Masao wrote: > On Fri, Jun 15, 2012 at 2:29 AM, Robert Haas wrote: >> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Fri, Jun 15, 2012 at 2:29 AM, Robert Haas wrote: > On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini > 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 doc_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh wrote: > On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas wrote: >> >> On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas wrote: > On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini > 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
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
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.bartol...@2ndquadrant.it | www.2ndQuadrant.it
Re: [HACKERS] Patch pg_is_in_backup()
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.nenciar...@2ndquadrant.it | www.2ndQuadrant.it pg_backup_start_time-and-pg_is_in_backup-v4.1.patch.bz2 Description: application/bzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
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 diff -ru postgresql-head/doc/src/sgml/func.sgml postgresql/doc/src/sgml/func.sgml --- postgresql-head/doc/src/sgml/func.sgml 2012-04-03 10:44:43.979702643 +0200 +++ postgresql/doc/src/sgml/func.sgml 2012-04-03 11:21:15.075701574 +0200 @@ -14467,6 +14467,9 @@ pg_stop_backup +pg_backup_start_time + + pg_switch_xlog @@ -14532,6 +14535,13 @@ +pg_backup_start_time() + + timestamp with time zone + Get start time of an online exclusive backup in progress. + + + pg_switch_xlog() text diff -ru postgresql-head/src/backend/access/transam/xlogfuncs.c postgresql/src/backend/access/transam/xlogfuncs.c --- postgresql-head/src/backend/access/transam/xlogfuncs.c 2012-04-03 10:44:44.227702645 +0200 +++ postgresql/src/backend/access/transam/xlogfuncs.c 2012-04-03 11:21:15.075701574 +0200 @@ -29,6 +29,7 @@ #include "utils/numeric.h" #include "utils/guc.h" #include "utils/timestamp.h" +#include "storage/fd.h" static void validate_xlog_location(char *str); @@ -563,3 +564,67 @@ PG_RETURN_NUMERIC(result); } + +/* + * Returns start time of an online exclusive backup. + * + * When there's no exclusive backup in progress, the function + * returns NULL. + */ +Datum +pg_backup_start_time(PG_FUNCTION_ARGS) +{ + TimestampTzxtime; + FILE*lfp; + charfline[MAXPGPATH]; + char backup_start_time[30]; + + /* + * See if label file is present + */ + lfp = AllocateFile(BACKUP_LABEL_FILE, "r"); + if (lfp == NULL) + { + if (errno != ENOENT) + ereport(ERROR, +(errcode_for_file_access(), +errmsg("could not read file \"%s\": %m", + BACKUP_LABEL_FILE))); + PG_RETURN_NULL(); + } + + /* + * Parse the file to find the the START TIME line. + */ + backup_start_time[0] = '\0'; + while (fgets(fline, sizeof(fline), lfp) != NULL) + { + if (sscanf(fline, "START TIME: %25[^\n]\n", + backup_start_time) == 1) + break; + } + + /* + * 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 (strlen(backup_start_time) == 0) { + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("invalid data in file \"%s\"", + BACKUP_LABEL_FILE))); + } + + /* + * Convert the time string read from file to TimestampTz form. + */ + xtime = DatumGetTimestampTz( + DirectFunctionCall3(timestamptz_in, + CStringGetDatum(backup_start_time), + ObjectIdGetDatum(InvalidOid),Int32GetDatum(-1)) + ); + + PG_RETURN_TIMESTAMPTZ(xtime); +} + diff -ru postgresql-head/src/include/access/xlog_internal.h postgresql/src/include/access/xlog_internal.h --- postgresql-head/src/include/access/xlog_internal.h 2012-04-03 10:44:45.051702642 +0200 +++ postgresql/src/include/access/xlog_internal.h 2012-04-03 11:21:15.075701574 +0200 @@ -282,5 +282,6 @@ extern Datum pg_xlog_replay_resume(PG_FUNCTION_ARGS); extern Datum pg_is_xlog_replay_paused(PG_FUNCTION_ARGS); extern Datum pg_xlog_location_diff(PG_FUNCTION_ARGS); +extern Datum pg_backup_start_time(PG_FUNCTION_ARGS); #endif /* XLOG_INTERNAL_H */ diff -ru postgresql-head/src/include/catalog/pg_proc.h postgresql/src/include/catalog/pg_proc.h --- postgresql-head/src/include/catalog/pg_proc.h 2012-04-03 10:44:45.051702642 +0200 +++ postgresql/src/include/catalog/pg_proc.h 2012-04-03 11:21:15.075701574 +0200 @@ -2924,6 +2924,8 @@ 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 0 0 25 "" _null_ _null_ _null_ _null_ pg_stop_backu
Re: [HACKERS] Patch pg_is_in_backup()
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.bartol...@2ndquadrant.it | www.2ndQuadrant.it -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
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 diff -ru postgresql-head/doc/src/sgml/func.sgml postgresql/doc/src/sgml/func.sgml --- postgresql-head/doc/src/sgml/func.sgml 2012-04-03 10:44:43.979702643 +0200 +++ postgresql/doc/src/sgml/func.sgml 2012-04-03 12:00:13.099700433 +0200 @@ -14467,6 +14467,12 @@ pg_stop_backup +pg_is_in_backup + + +pg_backup_start_time + + pg_switch_xlog @@ -14532,6 +14538,20 @@ +pg_is_in_backup() + + bool + True if an on-line exclusive backup is still in progress. + + + +pg_backup_start_time() + + timestamp with time zone + Get start time of an on-line exclusive backup in progress. + + + pg_switch_xlog() text diff -ru postgresql-head/src/backend/access/transam/xlogfuncs.c postgresql/src/backend/access/transam/xlogfuncs.c --- postgresql-head/src/backend/access/transam/xlogfuncs.c 2012-04-03 10:44:44.227702645 +0200 +++ postgresql/src/backend/access/transam/xlogfuncs.c 2012-04-03 12:02:51.043700358 +0200 @@ -29,7 +29,7 @@ #include "utils/numeric.h" #include "utils/guc.h" #include "utils/timestamp.h" - +#include "storage/fd.h" static void validate_xlog_location(char *str); @@ -563,3 +563,75 @@ PG_RETURN_NUMERIC(result); } + +/* + * Returns bool with current on-line backup mode, a global state. + */ +Datum +pg_is_in_backup(PG_FUNCTION_ARGS) +{ + PG_RETURN_BOOL(BackupInProgress()); +} + +/* + * Returns start time of an online exclusive backup. + * + * When there's no exclusive backup in progress, the function + * returns NULL. + */ +Datum +pg_backup_start_time(PG_FUNCTION_ARGS) +{ + TimestampTzxtime; + FILE*lfp; + charfline[MAXPGPATH]; + char backup_start_time[30]; + + /* + * See if label file is present + */ + lfp = AllocateFile(BACKUP_LABEL_FILE, "r"); + if (lfp == NULL) + { + if (errno != ENOENT) + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not read file \"%s\": %m", BACKUP_LABEL_FILE))); + PG_RETURN_NULL(); + } + + /* + * Parse the file to find the the START TIME line. + */ + backup_start_time[0] = '\0'; + while (fgets(fline, sizeof(fline), lfp) != NULL) + { + if (sscanf(fline, "START TIME: %25[^\n]\n", backup_start_time) == 1) + break; + } + + /* + * 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 (strlen(backup_start_time) == 0) { + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("invalid data in file \"%s\"", BACKUP_LABEL_FILE))); + } + + /* + * Convert the time string read from file to TimestampTz form. + */ + xtime = DatumGetTimestampTz( + DirectFunctionCall3(timestamptz_in, CStringGetDatum(backup_start_time), + ObjectIdGetDatum(InvalidOid),Int32GetDatum(-1)) + ); + + PG_RE
Re: [HACKERS] Patch pg_is_in_backup()
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.bartol...@2ndquadrant.it | www.2ndQuadrant.it -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
Le 03/02/2012 10:52, Magnus Hagander a écrit : > On Fri, Feb 3, 2012 at 10:47, Fujii Masao wrote: >> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle wrote: >>> >>> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao >>> 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/220 (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/298 (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 diff -ru postgresql/doc/src/sgml/func.sgml postgresql-is_in_exclusive_backup-patch//doc/src/sgml/func.sgml --- postgresql/doc/src/sgml/func.sgml 2012-01-26 23:01:31.956613398 +0100 +++ postgresql-is_in_exclusive_backup-patch//doc/src/sgml/func.sgml 2012-03-01 10:58:20.556472776 +0100 @@ -14371,6 +14371,9 @@ pg_current_xlog_location +pg_is_in_exclusive_backup + + pg_start_backup @@ -14424,6 +14427,14 @@ +pg_is_in_exclusive_backup() + + bool + True if an on-line exclusive backup is still in progress. + + + + pg_start_backup(label text , fast boolean ) text diff -ru postgresql/src/backend/access/transam/xlogfuncs.c postgresql-is_in_exclusive_backup-patch//src/backend/access/transam/xlogfuncs.c --- postgresql/src/backend/access/transam/xlogfuncs.c 2012-01-26 23:01:32.032613398 +0100 +++ postgresql-is_in_exclusive_backup-patch//src/backend/access/transam/xlogfuncs.c 2012-03-01 10:48:48.056473056 +0100 @@ -465,3 +465,12 @@ { PG_RETURN_BOOL(RecoveryInProgress()); } + +/* + * Returns bool with current on-line backup mode, a global state. + */ +Datum +pg_is_in_exclusive_backup(PG_FUNCTION_ARGS) +{ + PG_RETURN_BOOL(BackupInProgress()); +} diff -ru postgresql/src/include/access/xlog_internal.h postgresql-is_in_exclusive_backup-patch//src/include/access/xlog_internal.h --- postgresql/src/include/access/xlog_internal.h 2012-01-26 23:01:32.332613398 +0100 +++ postgresql-is_in_exclusive_backup-patch//src/include/access/xlog_internal.h 2012-03-01 10:48:48.076473056 +0100 @@ -281,5 +281,6 @@ extern Datum pg_xlog_replay_pause(PG_FUNCTION_ARGS); extern Datum pg_xlog_replay_resume(PG_FUNCTION_ARGS); extern Datum pg_is_xlog_replay_paused(PG_FUNCTION_ARGS); +extern Datum pg_is_in_exclusive_backup(PG_FUNCTION_ARGS); #endif /* XLOG_INTERNAL_H */ diff -ru postgresql/src/include/catalog/pg_proc.h postgresql-is_in_exclusive_backup-patch//src/include/catalog/pg_proc.h --- postgresql/src/include/
Re: [HACKERS] Patch pg_is_in_backup()
On Fri, Feb 3, 2012 at 6:52 PM, Magnus Hagander wrote: > On Fri, Feb 3, 2012 at 10:47, Fujii Masao wrote: >> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle wrote: >>> >>> >>> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao >>> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Fri, Feb 3, 2012 at 10:47, Fujii Masao wrote: > On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle wrote: >> >> >> --On 3. Februar 2012 13:21:11 +0900 Fujii Masao >> 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle wrote: > > > --On 3. Februar 2012 13:21:11 +0900 Fujii Masao > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
--On 3. Februar 2012 13:21:11 +0900 Fujii Masao 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Fri, Feb 3, 2012 at 11:31 AM, Euler Taveira de Oliveira 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Feb 2, 2012 at 12:23, Marti Raudsepp wrote: > On Mon, Jan 30, 2012 at 20:33, Gilles Darold 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
--On 2. Februar 2012 18:47:35 +0100 Gilles Darold 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
Le 02/02/2012 12:23, Marti Raudsepp a écrit : > On Mon, Jan 30, 2012 at 20:33, Gilles Darold 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
2012/2/2 Marti Raudsepp : > On Mon, Jan 30, 2012 at 20:33, Gilles Darold 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
On Mon, Jan 30, 2012 at 20:33, Gilles Darold 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch pg_is_in_backup()
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers