Re: [HACKERS] temporary statistics option at initdb time
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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 -- 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] temporary statistics option at initdb time
Magnus Hagander [EMAIL PROTECTED] 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 -- 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] temporary statistics option at initdb time
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] 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 -- 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] temporary statistics option at initdb time
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 -- 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] temporary statistics option at initdb time
Magnus Hagander wrote:
Decibel! wrote:
On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote:
Tom Lane wrote:
Decibel! [EMAIL PROTECTED] 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 - 1.179
--- backend/postmaster/pgstat.c 19 Aug 2008 15:24:59 -
***
*** 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)
+
Re: [HACKERS] temporary statistics option at initdb time
Magnus Hagander [EMAIL PROTECTED] 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 -- 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] temporary statistics option at initdb time
Tom Lane wrote: Decibel! [EMAIL PROTECTED] 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 -- 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] temporary statistics option at initdb time
Magnus Hagander wrote:
Tom Lane wrote:
Decibel! [EMAIL PROTECTED] 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 - 1.178
--- src/backend/postmaster/pgstat.c 13 Aug 2008 13:32:02 -
***
*** 70,77
*/
#define PGSTAT_STAT_PERMANENT_FILENAME global/pgstat.stat
#define PGSTAT_STAT_PERMANENT_TMPFILE global/pgstat.tmp
- #define PGSTAT_STAT_FILENAMEpg_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 - 1.465
--- src/backend/utils/misc/guc.c 13 Aug 2008 13:32:03 -
***
*** 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
},
Re: [HACKERS] temporary statistics option at initdb time
On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote: Tom Lane wrote: Decibel! [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Give your computer some brain candy! www.distributed.net Team #1828 smime.p7s Description: S/MIME cryptographic signature
Re: [HACKERS] temporary statistics option at initdb time
Decibel! wrote: On Aug 13, 2008, at 4:12 AM, Magnus Hagander wrote: Tom Lane wrote: Decibel! [EMAIL PROTECTED] 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 -- 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] temporary statistics option at initdb time
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
--
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] temporary statistics option at initdb time
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 -- 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] temporary statistics option at initdb time
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 [EMAIL PROTECTED] Give your computer some brain candy! www.distributed.net Team #1828 smime.p7s Description: S/MIME cryptographic signature
Re: [HACKERS] temporary statistics option at initdb time
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/ -- 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] temporary statistics option at initdb time
Decibel! [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
