Received: from magus.postgresql.org (magus.postgresql.org [87.238.57.229]) by mail.postgresql.org (Postfix) with ESMTP id 8806F1810A00 for ; Fri, 2 Mar 2012 11:05:39 -0400 (AST) Received: from mimolette.dalibo.net ([212.85.154.222]) by magus.postgresql.org with esmtp (Exim 4.72) (envelope-from ) id 1S3U35-0004eJ-Au for pgsql-hackers@postgresql.org; Fri, 02 Mar 2012 15:05:38 +0000 Received: from [192.168.1.104] (AGrenoble-551-1-110-141.w86-197.abo.wanadoo.fr [86.197.249.141]) by mimolette.dalibo.net (Postfix) with ESMTPSA id 1746028C55A; Fri, 2 Mar 2012 16:05:21 +0100 (CET) Message-ID: <4F50E1B0.90808@dalibo.com> Date: Fri, 02 Mar 2012 16:05:20 +0100 From: Gilles Darold User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.27) Gecko/20120216 Thunderbird/3.1.19 MIME-Version: 1.0 To: pgsql-hackers@postgresql.org CC: Magnus Hagander , Fujii Masao , Bernd Helmle , Euler Taveira de Oliveira , Marti Raudsepp Subject: Re: Patch pg_is_in_backup() References: <4F26E291.9020605@dalibo.com> <4F2B470F.5070107@timbira.com> <691525170451D3F76CEF7ED1@apophis.local> In-Reply-To: Content-Type: multipart/mixed; boundary="------------010206080206060307010704" X-Pg-Spam-Score: -1.9 (-) X-Archive-Number: 201203/64 X-Sequence-Number: 203946 This is a multi-part message in MIME format. --------------010206080206060307010704 Content-Type: multipart/alternative; boundary="------------010403060708040506060008" --------------010403060708040506060008 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Le 03/02/2012 10:52, Magnus Hagander a =E9crit : > On Fri, Feb 3, 2012 at 10:47, Fujii Masao wrote= : >> On Fri, Feb 3, 2012 at 6:10 PM, Bernd Helmle wr= ote: >>> >>> --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_b= ackup >>>> rather than the function returning an array. >>> >>> I like this idea. A use case i saw for monitoring backup_label's in t= he >>> 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 distingui= sh >>> 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=3D# select pg_start_backup('Online backup'); pg_start_backup ----------------- 0/2000020 (1 ligne) postgres=3D# select pg_exclusive_backup_start_time(); pg_exclusive_backup_start_time -------------------------------- 2012-03-02 14:52:49+01 (1 ligne) postgres=3D# select now() - pg_exclusive_backup_start_time() as backup_started_since; backup_started_since ------------------------ 00:12:24.569226 (1 ligne) postgres=3D# select pg_stop_backup(); pg_stop_backup ---------------- 0/2000098 (1 ligne) postgres=3D# select pg_exclusive_backup_start_time(); pg_exclusive_backup_start_time -------------------------------- (1 ligne) Regards, --=20 Gilles Darold http://dalibo.com - http://dalibo.org --------------010403060708040506060008 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Le 03/02/2012 10:52, Magnus Hagander a écrit : 
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');
 pg_start_backup
-----------------
 0/2000020
(1 ligne)

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

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

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

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

(1 ligne)

Regards,
-- 
Gilles Darold
http://dalibo.com - http://dalibo.org
--------------010403060708040506060008-- --------------010206080206060307010704 Content-Type: text/x-patch; name="postgresql-is_in_exclusive_backup-patch.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="postgresql-is_in_exclusive_backup-patch.diff" 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/catalog/pg_proc.h 2012-01-26 23:01:32.340613398 +0100 +++ postgresql-is_in_exclusive_backup-patch//src/include/catalog/pg_proc.h 2012-03-01 10:48:48.068473056 +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_exclusive_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 0 f f f t f v 1 0 25 "25" _null_ _null_ _null_ _null_ pg_create_restore_point _null_ _null_ _null_ )); --------------010206080206060307010704 Content-Type: text/x-patch; name="postgresql-exclusive_backup_start_time-patch.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="postgresql-exclusive_backup_start_time-patch.diff" diff -ru postgresql/doc/src/sgml/func.sgml postgresql-exclusive_backup_start_time-patch/doc/src/sgml/func.sgml --- postgresql/doc/src/sgml/func.sgml 2012-01-26 23:01:31.956613398 +0100 +++ postgresql-exclusive_backup_start_time-patch/doc/src/sgml/func.sgml 2012-03-02 10:56:56.986487006 +0100 @@ -14371,6 +14371,9 @@ pg_current_xlog_location + pg_exclusive_backup_start_time + + pg_start_backup @@ -14424,6 +14427,14 @@ + pg_exclusive_backup_start_time() + + timestamp with time zone + Get start time of an on-line exclusive backup in progress. + + + + pg_start_backup(label text , fast boolean ) text diff -ru postgresql/src/backend/access/transam/xlogfuncs.c postgresql-exclusive_backup_start_time-patch/src/backend/access/transam/xlogfuncs.c --- postgresql/src/backend/access/transam/xlogfuncs.c 2012-03-02 15:21:42.278479256 +0100 +++ postgresql-exclusive_backup_start_time-patch/src/backend/access/transam/xlogfuncs.c 2012-03-02 15:17:50.390479369 +0100 @@ -28,6 +28,7 @@ #include "utils/builtins.h" #include "utils/guc.h" #include "utils/timestamp.h" +#include "storage/fd.h" /* * pg_start_backup: set up for taking an on-line backup dump @@ -465,3 +466,71 @@ { PG_RETURN_BOOL(RecoveryInProgress()); } + + +/* + * Returns start time of an online exclusive backup. + * + * When there's no exclusive backup in progress, the function + * returns NULL. + */ +Datum +pg_exclusive_backup_start_time(PG_FUNCTION_ARGS) +{ + TimestampTz xtime; + FILE *lfp; + char fline[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/src/include/access/xlog_internal.h postgresql-exclusive_backup_start_time-patch/src/include/access/xlog_internal.h --- postgresql/src/include/access/xlog_internal.h 2012-01-26 23:01:32.332613398 +0100 +++ postgresql-exclusive_backup_start_time-patch/src/include/access/xlog_internal.h 2012-03-02 11:04:48.378486777 +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_exclusive_backup_start_time(PG_FUNCTION_ARGS); #endif /* XLOG_INTERNAL_H */ diff -ru postgresql/src/include/catalog/pg_proc.h postgresql-exclusive_backup_start_time-patch/src/include/catalog/pg_proc.h --- postgresql/src/include/catalog/pg_proc.h 2012-01-26 23:01:32.340613398 +0100 +++ postgresql-exclusive_backup_start_time-patch/src/include/catalog/pg_proc.h 2012-03-02 11:02:45.346486836 +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_exclusive_backup_start_time PGNSP PGUID 12 1 0 0 0 f f f t f s 0 0 1184 "" _null_ _null_ _null_ _null_ pg_exclusive_backup_start_time _null_ _null_ _null_ )); +DESCR("start time of an online 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 0 f f f t f v 1 0 25 "25" _null_ _null_ _null_ _null_ pg_create_restore_point _null_ _null_ _null_ )); --------------010206080206060307010704--