Thread: temporary statistics option at initdb time
Hi, After the Magnus patch [1], that make it possible store statistics files at another (RAM-based) disk, I was thinking that would be useful to add an option at initdb time to do the symlink as we already do with xlog. Maybe it could be documented in monitoring.sgml too. Comments? [1] http://archives.postgresql.org/pgsql-hackers/2008-08/msg00176.php -- Euler Taveira de Oliveira http://www.timbira.com/ Index: doc/src/sgml/ref/initdb.sgml =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/doc/src/sgml/ref/initdb.sgml,v retrieving revision 1.43 diff -c -r1.43 initdb.sgml *** doc/src/sgml/ref/initdb.sgml 26 Mar 2007 17:23:36 -0000 1.43 --- doc/src/sgml/ref/initdb.sgml 10 Aug 2008 01:04:06 -0000 *************** *** 175,180 **** --- 175,191 ---- </para> </listitem> </varlistentry> + + <varlistentry> + <term><option>-S <replaceable class="parameter">directory</replaceable></option></term> + <term><option>--statdir=<replaceable class="parameter">directory</replaceable></option></term> + <listitem> + <para> + This option specifies the directory where the temporary statistics + should be stored. + </para> + </listitem> + </varlistentry> <varlistentry> <term><option>-X <replaceable class="parameter">directory</replaceable></option></term> Index: src/bin/initdb/initdb.c =================================================================== RCS file: /a/pgsql/dev/anoncvs/pgsql/src/bin/initdb/initdb.c,v retrieving revision 1.159 diff -c -r1.159 initdb.c *** src/bin/initdb/initdb.c 5 Aug 2008 12:09:30 -0000 1.159 --- src/bin/initdb/initdb.c 8 Aug 2008 18:34:57 -0000 *************** *** 91,96 **** --- 91,97 ---- static bool noclean = false; static bool show_setting = false; static char *xlog_dir = ""; + static char *tmp_stat_dir = ""; /* internal vars */ *************** *** 111,116 **** --- 112,119 ---- static bool found_existing_pgdata = false; static bool made_new_xlogdir = false; static bool found_existing_xlogdir = false; + static bool made_new_tmpstatdir = false; + static bool found_existing_tmpstatdir = false; static char infoversion[100]; static bool caught_signal = false; static bool output_failed = false; *************** *** 628,633 **** --- 631,654 ---- fprintf(stderr, _("%s: failed to remove contents of transaction log directory\n"), progname); } + + if (made_new_tmpstatdir) + { + fprintf(stderr, _("%s: removing temporary statistics directory \"%s\"\n"), + progname, tmp_stat_dir); + if (!rmtree(tmp_stat_dir, true)) + fprintf(stderr, _("%s: failed to remove temporary statistics directory\n"), + progname); + } + else if (found_existing_tmpstatdir) + { + fprintf(stderr, + _("%s: removing contents of temporary statistics directory \"%s\"\n"), + progname, tmp_stat_dir); + if (!rmtree(tmp_stat_dir, false)) + fprintf(stderr, _("%s: failed to remove contents of temporary statistics directory\n"), + progname); + } /* otherwise died during startup, do nothing! */ } else *************** *** 641,646 **** --- 662,672 ---- fprintf(stderr, _("%s: transaction log directory \"%s\" not removed at user's request\n"), progname, xlog_dir); + + if (made_new_tmpstatdir || found_existing_tmpstatdir) + fprintf(stderr, + _("%s: temporary statistics directory \"%s\" not removed at user's request\n"), + progname, tmp_stat_dir); } exit(1); *************** *** 2387,2392 **** --- 2413,2419 ---- printf(_(" --no-locale equivalent to --locale=C\n")); printf(_(" -T, --text-search-config=CFG\n" " default text search configuration\n")); + printf(_(" -S, --statdir=STATDIR location for the temporary statistics directory\n")); printf(_(" -X, --xlogdir=XLOGDIR location for the transaction log directory\n")); printf(_(" -A, --auth=METHOD default authentication method for local connections\n")); printf(_(" -U, --username=NAME database superuser name\n")); *************** *** 2433,2438 **** --- 2460,2466 ---- {"show", no_argument, NULL, 's'}, {"noclean", no_argument, NULL, 'n'}, {"xlogdir", required_argument, NULL, 'X'}, + {"statdir", required_argument, NULL, 'S'}, {NULL, 0, NULL, 0} }; *************** *** 2484,2490 **** /* process command-line options */ ! while ((c = getopt_long(argc, argv, "dD:E:L:nU:WA:sT:X:", long_options, &option_index)) != -1) { switch (c) { --- 2512,2518 ---- /* process command-line options */ ! while ((c = getopt_long(argc, argv, "dD:E:L:nU:WA:sS:T:X:", long_options, &option_index)) != -1) { switch (c) { *************** *** 2544,2549 **** --- 2572,2580 ---- case 's': show_setting = true; break; + case 'S': + tmp_stat_dir = xstrdup(optarg); + break; case 'T': default_text_search_config = xstrdup(optarg); break; *************** *** 3095,3100 **** --- 3126,3217 ---- #endif } + /* Create temporary statistics symlink, if required */ + if (strcmp(tmp_stat_dir, "") != 0) + { + char *linkloc; + + /* clean up temporary statistics directory name, check it's absolute */ + canonicalize_path(tmp_stat_dir); + if (!is_absolute_path(tmp_stat_dir)) + { + fprintf(stderr, _("%s: temporary statistics directory location must be an absolute path\n"), progname); + exit_nicely(); + } + + /* check if the specified tmp_stat directory is empty */ + switch (check_data_dir(tmp_stat_dir)) + { + case 0: + /* tmp_stat directory not there, must create it */ + printf(_("creating directory %s ... "), + tmp_stat_dir); + fflush(stdout); + + if (mkdir_p(tmp_stat_dir, 0700) != 0) + { + fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"), + progname, tmp_stat_dir, strerror(errno)); + exit_nicely(); + } + else + check_ok(); + + made_new_tmpstatdir = true; + break; + case 1: + /* Present but empty, fix permissions and use it */ + printf(_("fixing permissions on existing directory %s ... "), + tmp_stat_dir); + fflush(stdout); + + if (chmod(tmp_stat_dir, 0700) != 0) + { + fprintf(stderr, _("%s: could not change permissions of directory \"%s\": %s\n"), + progname, tmp_stat_dir, strerror(errno)); + exit_nicely(); + } + else + check_ok(); + + found_existing_tmpstatdir = true; + break; + case 2: + /* Present and not empty */ + fprintf(stderr, + _("%s: directory \"%s\" exists but is not empty\n"), + progname, tmp_stat_dir); + fprintf(stderr, + _("If you want to store the temporary statistics there, either\n" + "remove or empty the directory \"%s\".\n"), + tmp_stat_dir); + exit_nicely(); + + default: + /* Trouble accessing directory */ + fprintf(stderr, _("%s: could not access directory \"%s\": %s\n"), + progname, tmp_stat_dir, strerror(errno)); + exit_nicely(); + } + + /* form name of the place where the symlink must go */ + linkloc = (char *) pg_malloc(strlen(pg_data) + 12 + 1); + sprintf(linkloc, "%s/pg_stat_tmp", pg_data); + + #ifdef HAVE_SYMLINK + if (symlink(tmp_stat_dir, linkloc) != 0) + { + fprintf(stderr, _("%s: could not create symbolic link \"%s\": %s\n"), + progname, linkloc, strerror(errno)); + exit_nicely(); + } + #else + fprintf(stderr, _("%s: symlinks are not supported on this platform")); + exit_nicely(); + #endif + } + + /* Create required subdirectories */ printf(_("creating subdirectories ... ")); fflush(stdout);
On Saturday 09 August 2008 21:31:28 Euler Taveira de Oliveira wrote: > Hi, > > After the Magnus patch [1], that make it possible store statistics files > at another (RAM-based) disk, I was thinking that would be useful to add > an option at initdb time to do the symlink as we already do with xlog. > Maybe it could be documented in monitoring.sgml too. Comments? > > [1] http://archives.postgresql.org/pgsql-hackers/2008-08/msg00176.php I find Magnus's approach to be admin un-friendly, and your patch a re-enforcement of that feeling. Forcing people to fall back on the OS tools (which may not be terribly good on systems like win32) when we could provide a consistent way for postgres to handle this itself seems backwards. I believe this would be much simpler for DBA's if we simply give them a GUC for stat file location, and allow them to set the location that way. Ideally this could be done as PGC_SIGHUP, and a change to the location would move the file "on-the-fly" as they say. (There might be practical limitation to making that work, but it would certainly be simpler for admins, imho) -- Robert Treat Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
Robert Treat wrote: > On Saturday 09 August 2008 21:31:28 Euler Taveira de Oliveira wrote: >> Hi, >> >> After the Magnus patch [1], that make it possible store statistics files >> at another (RAM-based) disk, I was thinking that would be useful to add >> an option at initdb time to do the symlink as we already do with xlog. >> Maybe it could be documented in monitoring.sgml too. Comments? >> >> [1] http://archives.postgresql.org/pgsql-hackers/2008-08/msg00176.php > > I find Magnus's approach to be admin un-friendly, and your patch a > re-enforcement of that feeling. Forcing people to fall back on the OS tools > (which may not be terribly good on systems like win32) when we could provide > a consistent way for postgres to handle this itself seems backwards. I > believe this would be much simpler for DBA's if we simply give them a GUC for > stat file location, and allow them to set the location that way. Ideally this > could be done as PGC_SIGHUP, and a change to the location would move the > file "on-the-fly" as they say. (There might be practical limitation to making > that work, but it would certainly be simpler for admins, imho) Well, the general enthusiasm for this feature at all wasn't very high, so I implemented in the least invasive way. It would certainly be possible to do it as a GUC, and not all that much more work. I don't think it'd be that hard to handle the SIGHUP case - just have the stats collector start writing it in the new location the next time it writes it out, and backends will start reading from there. There's a short window where the backends would get no data, but I think that's quite acceptable.. //Magnus
On Aug 12, 2008, at 12:27 PM, Magnus Hagander wrote: > I don't think it'd be that hard to handle the SIGHUP case - just have > the stats collector start writing it in the new location the next time > it writes it out, and backends will start reading from there. > There's a > short window where the backends would get no data, but I think that's > quite acceptable.. I disagree. While we don't guarantee stats are absolutely up-to-date, or atomic I don't think that gives license for them to just magically not exist sometimes. Would it really be that hard to have the system copy the file out before telling all the other backends of the change? BTW, it would be nice if it would actually remove the old file, but that sounds like a lot more work... -- Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org Give your computer some brain candy! www.distributed.net Team #1828
Robert Treat escreveu: > On Saturday 09 August 2008 21:31:28 Euler Taveira de Oliveira wrote: >> Hi, >> >> After the Magnus patch [1], that make it possible store statistics files >> at another (RAM-based) disk, I was thinking that would be useful to add >> an option at initdb time to do the symlink as we already do with xlog. >> Maybe it could be documented in monitoring.sgml too. Comments? >> >> [1] http://archives.postgresql.org/pgsql-hackers/2008-08/msg00176.php > > I find Magnus's approach to be admin un-friendly, and your patch a > re-enforcement of that feeling. Forcing people to fall back on the OS tools > (which may not be terribly good on systems like win32) when we could provide > a consistent way for postgres to handle this itself seems backwards. I > believe this would be much simpler for DBA's if we simply give them a GUC for > stat file location, and allow them to set the location that way. Ideally this > could be done as PGC_SIGHUP, and a change to the location would move the > file "on-the-fly" as they say. (There might be practical limitation to making > that work, but it would certainly be simpler for admins, imho) > How many times will you change the pg_stat_tmp directory? IMHO it is something that we don't frequently change. I agree that GUC is a DBA-friendly-option but we have to deal with (i) stop stats collector (ii) remove the old contents? (iii) start it at another location. I'm not saying that is _so_ difficult to do but I could live with pg_stat_tmp symlink option. -- Euler Taveira de Oliveira http://www.timbira.com/
Decibel! <decibel@decibel.org> writes:
> I disagree. While we don't guarantee stats are absolutely up-to-date,
> or atomic I don't think that gives license for them to just magically
> not exist sometimes.
> Would it really be that hard to have the system copy the file out
> before telling all the other backends of the change?
Well, there is no (zero, zilch, nada) use-case for changing this setting
on the fly. Why not make it a "frozen at postmaster start" GUC? Seems
like that gets all the functionality needed and most of the ease of use.
regards, tom lane
Tom Lane wrote: > Decibel! <decibel@decibel.org> writes: >> I disagree. While we don't guarantee stats are absolutely up-to-date, >> or atomic I don't think that gives license for them to just magically >> not exist sometimes. > >> Would it really be that hard to have the system copy the file out >> before telling all the other backends of the change? > > Well, there is no (zero, zilch, nada) use-case for changing this setting > on the fly. Why not make it a "frozen at postmaster start" GUC? Seems > like that gets all the functionality needed and most of the ease of use. Oh, there is a use-case. If you run your system and then only afterwards realize the I/O from the stats file is high enough to be an issue, and want to change it. That said, I'm not sure the use-case is anywhere near common enough to put a lot of code into it. But I can certainly look at making it a startup GUC. As you say, that'll solve *most* of the cases. //Magnus
Magnus Hagander wrote:
> Tom Lane wrote:
>> Decibel! <decibel@decibel.org> writes:
>>> I disagree. While we don't guarantee stats are absolutely up-to-date,
>>> or atomic I don't think that gives license for them to just magically
>>> not exist sometimes.
>>> Would it really be that hard to have the system copy the file out
>>> before telling all the other backends of the change?
>> Well, there is no (zero, zilch, nada) use-case for changing this setting
>> on the fly. Why not make it a "frozen at postmaster start" GUC? Seems
>> like that gets all the functionality needed and most of the ease of use.
>
> Oh, there is a use-case. If you run your system and then only afterwards
> realize the I/O from the stats file is high enough to be an issue, and
> want to change it.
>
> That said, I'm not sure the use-case is anywhere near common enough to
> put a lot of code into it.
>
> But I can certainly look at making it a startup GUC. As you say, that'll
> solve *most* of the cases.
Here's a patch that implements the simple case making it a
PGC_POSTMASTER variable. Is this good enough for people? ;-)
//Magnus
Index: src/backend/postmaster/pgstat.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/pgstat.c,v
retrieving revision 1.178
diff -c -r1.178 pgstat.c
*** src/backend/postmaster/pgstat.c 5 Aug 2008 12:09:30 -0000 1.178
--- src/backend/postmaster/pgstat.c 13 Aug 2008 13:32:02 -0000
***************
*** 70,77 ****
*/
#define PGSTAT_STAT_PERMANENT_FILENAME "global/pgstat.stat"
#define PGSTAT_STAT_PERMANENT_TMPFILE "global/pgstat.tmp"
- #define PGSTAT_STAT_FILENAME "pg_stat_tmp/pgstat.stat"
- #define PGSTAT_STAT_TMPFILE "pg_stat_tmp/pgstat.tmp"
/* ----------
* Timer definitions.
--- 70,75 ----
***************
*** 106,111 ****
--- 104,116 ----
int pgstat_track_functions = TRACK_FUNC_OFF;
int pgstat_track_activity_query_size = 1024;
+ /* ----------
+ * Built from GUC parameter
+ * ----------
+ */
+ char *pgstat_stat_filename = NULL;
+ char *pgstat_stat_tmpname = NULL;
+
/*
* BgWriter global statistics counters (unused in other processes).
* Stored directly in a stats message structure so it can be sent
***************
*** 511,517 ****
void
pgstat_reset_all(void)
{
! unlink(PGSTAT_STAT_FILENAME);
unlink(PGSTAT_STAT_PERMANENT_FILENAME);
}
--- 516,522 ----
void
pgstat_reset_all(void)
{
! unlink(pgstat_stat_filename);
unlink(PGSTAT_STAT_PERMANENT_FILENAME);
}
***************
*** 2911,2918 ****
PgStat_StatFuncEntry *funcentry;
FILE *fpout;
int32 format_id;
! const char *tmpfile = permanent?PGSTAT_STAT_PERMANENT_TMPFILE:PGSTAT_STAT_TMPFILE;
! const char *statfile = permanent?PGSTAT_STAT_PERMANENT_FILENAME:PGSTAT_STAT_FILENAME;
/*
* Open the statistics temp file to write out the current values.
--- 2916,2923 ----
PgStat_StatFuncEntry *funcentry;
FILE *fpout;
int32 format_id;
! const char *tmpfile = permanent?PGSTAT_STAT_PERMANENT_TMPFILE:pgstat_stat_tmpname;
! const char *statfile = permanent?PGSTAT_STAT_PERMANENT_FILENAME:pgstat_stat_filename;
/*
* Open the statistics temp file to write out the current values.
***************
*** 3012,3018 ****
}
if (permanent)
! unlink(PGSTAT_STAT_FILENAME);
}
--- 3017,3023 ----
}
if (permanent)
! unlink(pgstat_stat_filename);
}
***************
*** 3039,3045 ****
FILE *fpin;
int32 format_id;
bool found;
! const char *statfile = permanent?PGSTAT_STAT_PERMANENT_FILENAME:PGSTAT_STAT_FILENAME;
/*
* The tables will live in pgStatLocalContext.
--- 3044,3050 ----
FILE *fpin;
int32 format_id;
bool found;
! const char *statfile = permanent?PGSTAT_STAT_PERMANENT_FILENAME:pgstat_stat_filename;
/*
* The tables will live in pgStatLocalContext.
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.465
diff -c -r1.465 guc.c
*** src/backend/utils/misc/guc.c 23 Jul 2008 17:29:53 -0000 1.465
--- src/backend/utils/misc/guc.c 13 Aug 2008 13:32:03 -0000
***************
*** 164,169 ****
--- 164,170 ----
static const char *show_tcp_keepalives_count(void);
static bool assign_autovacuum_max_workers(int newval, bool doit, GucSource source);
static bool assign_maxconnections(int newval, bool doit, GucSource source);
+ static const char *assign_pgstat_temp_directory(const char *newval, bool doit, GucSource source);
static char *config_enum_get_options(struct config_enum *record,
const char *prefix, const char *suffix);
***************
*** 343,348 ****
--- 344,351 ----
char *IdentFileName;
char *external_pid_file;
+ char *pgstat_temp_directory;
+
int tcp_keepalives_idle;
int tcp_keepalives_interval;
int tcp_keepalives_count;
***************
*** 2467,2472 ****
--- 2470,2485 ----
},
{
+ {"stats_temp_directory", PGC_POSTMASTER, STATS_COLLECTOR,
+ gettext_noop("Writes temporary statistics files to the specified directory."),
+ NULL,
+ GUC_SUPERUSER_ONLY
+ },
+ &pgstat_temp_directory,
+ "pg_stat_tmp", assign_pgstat_temp_directory, NULL
+ },
+
+ {
{"default_text_search_config", PGC_USERSET, CLIENT_CONN_LOCALE,
gettext_noop("Sets default text search configuration."),
NULL
***************
*** 7370,7373 ****
--- 7383,7406 ----
return true;
}
+ static const char *
+ assign_pgstat_temp_directory(const char *newval, bool doit, GucSource source)
+ {
+ if (doit)
+ {
+ if (pgstat_stat_tmpname)
+ free(pgstat_stat_tmpname);
+ if (pgstat_stat_filename)
+ free(pgstat_stat_filename);
+
+ pgstat_stat_tmpname = guc_malloc(FATAL, strlen(newval) + 12); /* /pgstat.tmp */
+ pgstat_stat_filename = guc_malloc(FATAL, strlen(newval) + 13); /* /pgstat.stat */
+
+ sprintf(pgstat_stat_tmpname, "%s/pgstat.tmp", newval);
+ sprintf(pgstat_stat_filename, "%s/pgstat.stat", newval);
+ }
+
+ return newval;
+ }
+
#include "guc-file.c"
Index: src/backend/utils/misc/postgresql.conf.sample
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/postgresql.conf.sample,v
retrieving revision 1.243
diff -c -r1.243 postgresql.conf.sample
*** src/backend/utils/misc/postgresql.conf.sample 30 Jun 2008 10:58:47 -0000 1.243
--- src/backend/utils/misc/postgresql.conf.sample 13 Aug 2008 13:32:03 -0000
***************
*** 366,371 ****
--- 366,372 ----
#track_functions = none # none, pl, all
#track_activity_query_size = 1024
#update_process_title = on
+ #stats_temp_directory = 'pg_stat_tmp'
# - Statistics Monitoring -
Index: src/include/pgstat.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/pgstat.h,v
retrieving revision 1.77
diff -c -r1.77 pgstat.h
*** src/include/pgstat.h 30 Jun 2008 10:58:47 -0000 1.77
--- src/include/pgstat.h 13 Aug 2008 13:32:03 -0000
***************
*** 576,581 ****
--- 576,583 ----
extern bool pgstat_track_counts;
extern int pgstat_track_functions;
extern int pgstat_track_activity_query_size;
+ extern char *pgstat_stat_tmpname;
+ extern char *pgstat_stat_filename;
/*
* BgWriter statistics counters are updated directly by bgwriter and bufmgr
On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote: > Tom Lane wrote: >> Decibel! <decibel@decibel.org> writes: >>> I disagree. While we don't guarantee stats are absolutely up-to- >>> date, >>> or atomic I don't think that gives license for them to just >>> magically >>> not exist sometimes. >> >>> Would it really be that hard to have the system copy the file out >>> before telling all the other backends of the change? >> >> Well, there is no (zero, zilch, nada) use-case for changing this >> setting >> on the fly. Why not make it a "frozen at postmaster start" GUC? >> Seems >> like that gets all the functionality needed and most of the ease >> of use. > > Oh, there is a use-case. If you run your system and then only > afterwards > realize the I/O from the stats file is high enough to be an issue, and > want to change it. > > That said, I'm not sure the use-case is anywhere near common enough to > put a lot of code into it. Something to keep in mind as PG is used to build larger systems 'further up the enterprise'... for us to bounce a database at work costs us a LOT in lost revenue. I don't want to go into specifics, but it's more than enough to buy a very nice car. :) That's why I asked how hard it'd be to do this on the fly. -- Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org Give your computer some brain candy! www.distributed.net Team #1828
Decibel! wrote: > On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote: >> Tom Lane wrote: >>> Decibel! <decibel@decibel.org> writes: >>>> I disagree. While we don't guarantee stats are absolutely up-to-date, >>>> or atomic I don't think that gives license for them to just magically >>>> not exist sometimes. >>> >>>> Would it really be that hard to have the system copy the file out >>>> before telling all the other backends of the change? >>> >>> Well, there is no (zero, zilch, nada) use-case for changing this setting >>> on the fly. Why not make it a "frozen at postmaster start" GUC? Seems >>> like that gets all the functionality needed and most of the ease of use. >> >> Oh, there is a use-case. If you run your system and then only afterwards >> realize the I/O from the stats file is high enough to be an issue, and >> want to change it. >> >> That said, I'm not sure the use-case is anywhere near common enough to >> put a lot of code into it. > > > Something to keep in mind as PG is used to build larger systems 'further > up the enterprise'... for us to bounce a database at work costs us a LOT > in lost revenue. I don't want to go into specifics, but it's more than > enough to buy a very nice car. :) That's why I asked how hard it'd be to > do this on the fly. Well, it's doable, but fairly hard. But you can do it the symlink way without shutting it down, I think. :-) //Magnus
Magnus Hagander wrote:
> Decibel! wrote:
>> On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote:
>>> Tom Lane wrote:
>>>> Decibel! <decibel@decibel.org> writes:
>>>>> I disagree. While we don't guarantee stats are absolutely up-to-date,
>>>>> or atomic I don't think that gives license for them to just magically
>>>>> not exist sometimes.
>>>>> Would it really be that hard to have the system copy the file out
>>>>> before telling all the other backends of the change?
>>>> Well, there is no (zero, zilch, nada) use-case for changing this setting
>>>> on the fly. Why not make it a "frozen at postmaster start" GUC? Seems
>>>> like that gets all the functionality needed and most of the ease of use.
>>> Oh, there is a use-case. If you run your system and then only afterwards
>>> realize the I/O from the stats file is high enough to be an issue, and
>>> want to change it.
>>>
>>> That said, I'm not sure the use-case is anywhere near common enough to
>>> put a lot of code into it.
>>
>> Something to keep in mind as PG is used to build larger systems 'further
>> up the enterprise'... for us to bounce a database at work costs us a LOT
>> in lost revenue. I don't want to go into specifics, but it's more than
>> enough to buy a very nice car. :) That's why I asked how hard it'd be to
>> do this on the fly.
>
> Well, it's doable, but fairly hard.
>
> But you can do it the symlink way without shutting it down, I think. :-)
Actually, I think maybe not so hard. Attached is a patch that fixes
this. It's done by keeping the old filename around. When you change the
path, the stats collector will start writing the new file the next time
it writes something (which should be max 0.5 seconds later if something
is happening). The backends will immediately try to read from the new
filename, but if that one is not found, they will switch to reading the
old filename. This obviously fails if you change the temp directory
twice in less than half a second, but I really don't see a use-case for
that...
Or did I miss something here? :-)
//Magnus
Index: backend/postmaster/pgstat.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/pgstat.c,v
retrieving revision 1.179
diff -c -r1.179 pgstat.c
*** backend/postmaster/pgstat.c 15 Aug 2008 08:37:39 -0000 1.179
--- backend/postmaster/pgstat.c 19 Aug 2008 15:24:59 -0000
***************
*** 110,115 ****
--- 110,116 ----
*/
char *pgstat_stat_filename = NULL;
char *pgstat_stat_tmpname = NULL;
+ char *pgstat_stat_lastname = NULL;
/*
* BgWriter global statistics counters (unused in other processes).
***************
*** 203,208 ****
--- 204,210 ----
static volatile bool need_exit = false;
static volatile bool need_statwrite = false;
+ static volatile bool got_SIGHUP = false;
/*
* Total time charged to functions so far in the current backend.
***************
*** 224,229 ****
--- 226,232 ----
static void pgstat_exit(SIGNAL_ARGS);
static void force_statwrite(SIGNAL_ARGS);
static void pgstat_beshutdown_hook(int code, Datum arg);
+ static void pgstat_sighup_handler(SIGNAL_ARGS);
static PgStat_StatDBEntry *pgstat_get_db_entry(Oid databaseid, bool create);
static void pgstat_write_statsfile(bool permanent);
***************
*** 2571,2577 ****
* Ignore all signals usually bound to some action in the postmaster,
* except SIGQUIT and SIGALRM.
*/
! pqsignal(SIGHUP, SIG_IGN);
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
pqsignal(SIGQUIT, pgstat_exit);
--- 2574,2580 ----
* Ignore all signals usually bound to some action in the postmaster,
* except SIGQUIT and SIGALRM.
*/
! pqsignal(SIGHUP, pgstat_sighup_handler);
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
pqsignal(SIGQUIT, pgstat_exit);
***************
*** 2635,2640 ****
--- 2638,2652 ----
break;
/*
+ * Reload configuration if we got SIGHUP from the postmaster.
+ */
+ if (got_SIGHUP)
+ {
+ ProcessConfigFile(PGC_SIGHUP);
+ got_SIGHUP = false;
+ }
+
+ /*
* If time to write the stats file, do so. Note that the alarm
* interrupt isn't re-enabled immediately, but only after we next
* receive a stats message; so no cycles are wasted when there is
***************
*** 2834,2839 ****
--- 2846,2858 ----
need_statwrite = true;
}
+ /* SIGHUP handler for collector process */
+ static void
+ pgstat_sighup_handler(SIGNAL_ARGS)
+ {
+ got_SIGHUP = true;
+ }
+
/*
* Lookup the hash table entry for the specified database. If no hash
***************
*** 3018,3023 ****
--- 3037,3059 ----
if (permanent)
unlink(pgstat_stat_filename);
+ else
+ {
+ /*
+ * If the old filename exists, we need to unlink it now that we have written
+ * the new file. This indicates that we are done, and it also makes it possible
+ * to switch the directory back without getting the old data.
+ *
+ * Also do away with the old name here, so we don't try to delete it again.
+ */
+ if (pgstat_stat_lastname != NULL)
+ {
+ unlink(pgstat_stat_lastname);
+
+ free(pgstat_stat_lastname);
+ pgstat_stat_lastname = NULL;
+ }
+ }
}
***************
*** 3072,3080 ****
* Try to open the status file. If it doesn't exist, the backends simply
* return zero for anything and the collector simply starts from scratch
* with empty counters.
*/
if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
! return dbhash;
/*
* Verify it's of the expected format.
--- 3108,3128 ----
* Try to open the status file. If it doesn't exist, the backends simply
* return zero for anything and the collector simply starts from scratch
* with empty counters.
+ * If we are loading the temporary file, look at pgstat_stat_lastname as
+ * well, in case the filename changed recently and the collector hasn't
+ * written the new file yet.
*/
if ((fpin = AllocateFile(statfile, PG_BINARY_R)) == NULL)
! {
! if (!permanent && pgstat_stat_lastname != NULL)
! {
! /* attempt to load the old file if it's there */
! if ((fpin = AllocateFile(pgstat_stat_lastname, PG_BINARY_R)) == NULL)
! return dbhash;
! }
! else
! return dbhash;
! }
/*
* Verify it's of the expected format.
Index: backend/postmaster/postmaster.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.561
diff -c -r1.561 postmaster.c
*** backend/postmaster/postmaster.c 26 Jun 2008 02:47:19 -0000 1.561
--- backend/postmaster/postmaster.c 19 Aug 2008 15:24:59 -0000
***************
*** 1923,1932 ****
signal_child(PgArchPID, SIGHUP);
if (SysLoggerPID != 0)
signal_child(SysLoggerPID, SIGHUP);
! /* PgStatPID does not currently need SIGHUP */
/* Reload authentication config files too */
load_ident();
#ifdef EXEC_BACKEND
--- 1931,1944 ----
signal_child(PgArchPID, SIGHUP);
if (SysLoggerPID != 0)
signal_child(SysLoggerPID, SIGHUP);
! if (PgStatPID != 0)
! signal_child(PgStatPID, SIGHUP);
/* Reload authentication config files too */
load_ident();
#ifdef EXEC_BACKEND
Index: backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.466
diff -c -r1.466 guc.c
*** backend/utils/misc/guc.c 15 Aug 2008 08:37:40 -0000 1.466
--- backend/utils/misc/guc.c 19 Aug 2008 15:24:59 -0000
***************
*** 2470,2476 ****
},
{
! {"stats_temp_directory", PGC_POSTMASTER, STATS_COLLECTOR,
gettext_noop("Writes temporary statistics files to the specified directory."),
NULL,
GUC_SUPERUSER_ONLY
--- 2470,2476 ----
},
{
! {"stats_temp_directory", PGC_SIGHUP, STATS_COLLECTOR,
gettext_noop("Writes temporary statistics files to the specified directory."),
NULL,
GUC_SUPERUSER_ONLY
***************
*** 7390,7397 ****
{
if (pgstat_stat_tmpname)
free(pgstat_stat_tmpname);
if (pgstat_stat_filename)
! free(pgstat_stat_filename);
pgstat_stat_tmpname = guc_malloc(FATAL, strlen(newval) + 12); /* /pgstat.tmp */
pgstat_stat_filename = guc_malloc(FATAL, strlen(newval) + 13); /* /pgstat.stat */
--- 7390,7404 ----
{
if (pgstat_stat_tmpname)
free(pgstat_stat_tmpname);
+
+ /* save away the last filename used */
if (pgstat_stat_filename)
! {
! if (pgstat_stat_lastname)
! free(pgstat_stat_lastname);
!
! pgstat_stat_lastname = pgstat_stat_filename;
! }
pgstat_stat_tmpname = guc_malloc(FATAL, strlen(newval) + 12); /* /pgstat.tmp */
pgstat_stat_filename = guc_malloc(FATAL, strlen(newval) + 13); /* /pgstat.stat */
Index: include/pgstat.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/pgstat.h,v
retrieving revision 1.78
diff -c -r1.78 pgstat.h
*** include/pgstat.h 15 Aug 2008 08:37:40 -0000 1.78
--- include/pgstat.h 19 Aug 2008 15:24:59 -0000
***************
*** 578,583 ****
--- 578,584 ----
extern int pgstat_track_activity_query_size;
extern char *pgstat_stat_tmpname;
extern char *pgstat_stat_filename;
+ extern char *pgstat_stat_lastname;
/*
* BgWriter statistics counters are updated directly by bgwriter and bufmgr
Magnus Hagander <magnus@hagander.net> writes:
> Actually, I think maybe not so hard. Attached is a patch that fixes
> this. It's done by keeping the old filename around. When you change the
> path, the stats collector will start writing the new file the next time
> it writes something (which should be max 0.5 seconds later if something
> is happening). The backends will immediately try to read from the new
> filename, but if that one is not found, they will switch to reading the
> old filename. This obviously fails if you change the temp directory
> twice in less than half a second, but I really don't see a use-case for
> that...
I think this is introducing complication and race conditions to solve a
problem that no one will really care about. Just let people change the
filename at SIGHUP and document that doing that on-the-fly may cause
stats queries to fail for a short interval.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Actually, I think maybe not so hard. Attached is a patch that fixes >> this. It's done by keeping the old filename around. When you change the >> path, the stats collector will start writing the new file the next time >> it writes something (which should be max 0.5 seconds later if something >> is happening). The backends will immediately try to read from the new >> filename, but if that one is not found, they will switch to reading the >> old filename. This obviously fails if you change the temp directory >> twice in less than half a second, but I really don't see a use-case for >> that... > > I think this is introducing complication and race conditions to solve a > problem that no one will really care about. Just let people change the > filename at SIGHUP and document that doing that on-the-fly may cause > stats queries to fail for a short interval. Ok, I'll do it that way. //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> I think this is introducing complication and race conditions to solve a
>> problem that no one will really care about. Just let people change the
>> filename at SIGHUP and document that doing that on-the-fly may cause
>> stats queries to fail for a short interval.
> Ok, I'll do it that way.
BTW, it might be worth tweaking the stats collector to dump stats
immediately after responding to SIGHUP, just to narrow this window
as much as possible.
[ squint... ] Actually, it looks like the stats collector SIG_IGNores
SIGHUP? That can't be right (any more) can it?
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Tom Lane wrote: >>> I think this is introducing complication and race conditions to solve a >>> problem that no one will really care about. Just let people change the >>> filename at SIGHUP and document that doing that on-the-fly may cause >>> stats queries to fail for a short interval. > >> Ok, I'll do it that way. > > BTW, it might be worth tweaking the stats collector to dump stats > immediately after responding to SIGHUP, just to narrow this window > as much as possible. Pah, that is too obvious :-) Yeah, should've thought of that, will make that happen. > [ squint... ] Actually, it looks like the stats collector SIG_IGNores > SIGHUP? That can't be right (any more) can it? It did, until after my patch that was applied a couple of minutes ago. After the patch, it cares about SIGHUP. //Magnus
I wrote:
> [ squint... ] Actually, it looks like the stats collector SIG_IGNores
> SIGHUP? That can't be right (any more) can it?
Oh, never mind, I see my hour-old CVS pull is obsolete ...
regards, tom lane