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()
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()
On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini gabriele.bartol...@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 -- 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 robertmh...@gmail.com wrote:
On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
gabriele.bartol...@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?
Regards,
--
Gurjeet Singh
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Re: [HACKERS] Patch pg_is_in_backup()
On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh singh.gurj...@gmail.com wrote:
On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas robertmh...@gmail.com wrote:
On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini
gabriele.bartol...@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 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 Fri, Jun 15, 2012 at 2:29 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini gabriele.bartol...@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 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 2:05 PM, Fujii Masao masao.fu...@gmail.com wrote: On Fri, Jun 15, 2012 at 2:29 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 14, 2012 at 6:06 AM, Gabriele Bartolini gabriele.bartol...@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 -- 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 robertmh...@gmail.com wrote:
On Thu, Jun 14, 2012 at 1:48 PM, Gurjeet Singh singh.gurj...@gmail.com
wrote:
On Thu, Jun 14, 2012 at 1:29 PM, Robert Haas robertmh...@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?
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 3:10 PM, Gurjeet Singh singh.gurj...@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 -- 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 3:20 PM, Robert Haas robertmh...@gmail.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
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 @@
primarypg_stop_backup/primary
/indexterm
indexterm
+primarypg_backup_start_time/primary
+ /indexterm
+ indexterm
primarypg_switch_xlog/primary
/indexterm
indexterm
@@ -14532,6 +14535,13 @@
/row
row
entry
+literalfunctionpg_backup_start_time()/function/literal
+/entry
+ entrytypetimestamp with time zone/type/entry
+ entryGet start time of an online exclusive backup in progress./entry
+ /row
+ row
+ entry
literalfunctionpg_switch_xlog()/function/literal
/entry
entrytypetext/type/entry
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
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 @@
primarypg_stop_backup/primary
/indexterm
indexterm
+primarypg_is_in_backup/primary
+ /indexterm
+ indexterm
+primarypg_backup_start_time/primary
+ /indexterm
+ indexterm
primarypg_switch_xlog/primary
/indexterm
indexterm
@@ -14532,6 +14538,20 @@
/row
row
entry
+literalfunctionpg_is_in_backup()/function/literal
+/entry
+ entrytypebool/type/entry
+ entryTrue if an on-line exclusive backup is still in progress./entry
+ /row
+ row
+ entry
+literalfunctionpg_backup_start_time()/function/literal
+/entry
+ entrytypetimestamp with time zone/type/entry
+ entryGet start time of an on-line exclusive backup in progress./entry
+ /row
+ row
+ entry
literalfunctionpg_switch_xlog()/function/literal
/entry
entrytypetext/type/entry
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),
+
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 masao.fu...@gmail.com wrote:
On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle maili...@oopsware.de wrote:
--On 3. Februar 2012 13:21:11 +0900 Fujii Masao masao.fu...@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');
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 @@
primarypg_current_xlog_location/primary
/indexterm
indexterm
+primarypg_is_in_exclusive_backup/primary
+ /indexterm
+ indexterm
primarypg_start_backup/primary
/indexterm
indexterm
@@ -14424,6 +14427,14 @@
/row
row
entry
+literalfunctionpg_is_in_exclusive_backup()/function/literal
+/entry
+ entrytypebool/type/entry
+ entryTrue if an on-line exclusive backup is still in progress.
+ /entry
+ /row
+ row
+ entry
literalfunctionpg_start_backup(parameterlabel/ typetext/ optional, parameterfast/ typeboolean/ /optional)/function/literal
/entry
entrytypetext/type/entry
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
Re: [HACKERS] Patch pg_is_in_backup()
--On 3. Februar 2012 13:21:11 +0900 Fujii Masao masao.fu...@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 -- 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 maili...@oopsware.de wrote: --On 3. Februar 2012 13:21:11 +0900 Fujii Masao masao.fu...@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 -- 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 masao.fu...@gmail.com wrote: On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle maili...@oopsware.de wrote: --On 3. Februar 2012 13:21:11 +0900 Fujii Masao masao.fu...@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/ -- 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:52 PM, Magnus Hagander mag...@hagander.net wrote: On Fri, Feb 3, 2012 at 10:47, Fujii Masao masao.fu...@gmail.com wrote: On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle maili...@oopsware.de wrote: --On 3. Februar 2012 13:21:11 +0900 Fujii Masao masao.fu...@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 -- 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 gilles.dar...@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 -- 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 ma...@juffo.org: On Mon, Jan 30, 2012 at 20:33, Gilles Darold gilles.dar...@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 -- 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 gilles.dar...@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 -- 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 gilles.dar...@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 -- 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 ma...@juffo.org wrote: On Mon, Jan 30, 2012 at 20:33, Gilles Darold gilles.dar...@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/ -- 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 Fri, Feb 3, 2012 at 11:31 AM, Euler Taveira de Oliveira eu...@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 -- 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
[HACKERS] 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
diff -ru postgresql/doc/src/sgml/func.sgml postgresql-is_in_backup-patch/doc/src/sgml/func.sgml
--- postgresql/doc/src/sgml/func.sgml 2012-01-26 23:01:31.956613398 +0100
+++ postgresql-is_in_backup-patch/doc/src/sgml/func.sgml 2012-01-26 23:14:29.468613019 +0100
@@ -14371,6 +14371,9 @@
primarypg_current_xlog_location/primary
/indexterm
indexterm
+primarypg_is_in_backup/primary
+ /indexterm
+ indexterm
primarypg_start_backup/primary
/indexterm
indexterm
@@ -14424,6 +14427,14 @@
/row
row
entry
+literalfunctionpg_is_in_backup()/function/literal
+/entry
+ entrytypebool/type/entry
+ entryTrue if an on-line backup is still in progress.
+ /entry
+ /row
+ row
+ entry
literalfunctionpg_start_backup(parameterlabel/ typetext/ optional, parameterfast/ typeboolean/ /optional)/function/literal
/entry
entrytypetext/type/entry
diff -ru postgresql/src/backend/access/transam/xlogfuncs.c postgresql-is_in_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_backup-patch/src/backend/access/transam/xlogfuncs.c 2012-01-26 23:16:04.100612972 +0100
@@ -465,3 +465,12 @@
{
PG_RETURN_BOOL(RecoveryInProgress());
}
+
+/*
+ * Returns bool with current on-line backup mode, a global state.
+ */
+Datum
+pg_is_in_backup(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_BOOL(BackupInProgress());
+}
diff -ru postgresql/src/include/access/xlog_internal.h postgresql-is_in_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_backup-patch/src/include/access/xlog_internal.h 2012-01-26 23:22:51.492612774 +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_backup(PG_FUNCTION_ARGS);
#endif /* XLOG_INTERNAL_H */
diff -ru postgresql/src/include/catalog/pg_proc.h postgresql-is_in_backup-patch/src/include/catalog/pg_proc.h
--- postgresql/src/include/catalog/pg_proc.h 2012-01-26 23:01:32.340613398 +0100
+++ postgresql-is_in_backup-patch/src/include/catalog/pg_proc.h 2012-01-27 09:26:09.918736657 +0100
@@ -2899,6 +2899,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 t f v 0 0 25 _null_ _null_ _null_ _null_ pg_stop_backup _null_ _null_ _null_ ));
DESCR(finish taking an online backup);
+DATA(insert OID = 3882 ( pg_is_in_backup PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 16 _null_ _null_ _null_ _null_ pg_is_in_backup _null_ _null_ _null_ ));
+DESCR(true if server is in on-line backup);
DATA(insert OID = 2848 ( pg_switch_xlog PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 25 _null_ _null_ _null_ _null_ pg_switch_xlog _null_ _null_ _null_ ));
DESCR(switch to new xlog file);
DATA(insert OID = 3098 ( pg_create_restore_point PGNSP PGUID 12 1 0 0
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
