Re: [HACKERS] pg_stat_statements temporary file
On Friday, May 25, 2012 05:19:28 PM Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On Friday, May 25, 2012 04:03:49 PM Peter Geoghegan wrote: Where do you suggest the file be written to? One could argue stats_temp_directory would be the correct place. No, that would be exactly the *wrong* place, because that directory can be on a RAM disk. We need to put this somewhere where it'll survive a shutdown. I had assumed it would do the writeout regularly to survive a database crash. As it does not do that my argument is clearly bogus, sorry for that. Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] pg_stat_statements temporary file
On Thu, May 24, 2012 at 2:19 PM, Magnus Hagander mag...@hagander.net wrote:
On Thu, May 24, 2012 at 2:16 PM, Peter Geoghegan pe...@2ndquadrant.com
wrote:
On 24 May 2012 12:42, Magnus Hagander mag...@hagander.net wrote:
What actually happens if it tries to repalloc() something huge? palloc
will throw an elog(ERROR), and since this happens during postmaster
startup, are you sure it won't prevent the server from starting?
Oh, yes, missed that.
/* Previous incarnation might have had a larger query_size */
if (temp.query_len = buffer_size)
{
buffer = (char *) repalloc(buffer, temp.query_len +
1);
buffer_size = temp.query_len + 1;
}
Here, temp receives its value from an fread().
This could probably be coded to be defensive against such things, but
a better fix would be preferred. I have to wonder how much of a
problem corruption is likely to be though, given that we only save to
disk in a corresponding pgss_shmem_shutdown() call, which actually has
more protections against corruption. The window for the saved file to
be corrupt seems rather small, though I accept that a better window
would be zero.
Right. But writing to a temp file and rename()ing it into place is trivial.
It's really the other issues raised that are bigger ;)
Here's a patch that does the two easy fixes:
1) writes the file to a temp file and rename()s it over the main file
as it writes down. This removes the (small) risk of corruption because
of a crash during write
2) unlinks the file after reading it. this makes sure it's not
included in online backups.
I still think we should consider the placement of this file to not be
in the global/ directory, but this is a quick (back-patchable) fix...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
pg_stat_statements_file.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] pg_stat_statements temporary file
On 25 May 2012 14:13, Magnus Hagander mag...@hagander.net wrote: Here's a patch that does the two easy fixes: 1) writes the file to a temp file and rename()s it over the main file as it writes down. This removes the (small) risk of corruption because of a crash during write 2) unlinks the file after reading it. this makes sure it's not included in online backups. Seems reasonable. It might be better to consistently concatenate the string literals PGSS_DUMP_FILE and .tmp statically. Also, I'd have updated the string in the errmsg callsite after the error tag too, to refer to the tmp file rather than the file proper. Forgive the pedantry, but I should mention that I believe that it is project policy to not use squiggly parenthesis following an if expression when that is unnecessary due to there only being a single statement. I still think we should consider the placement of this file to not be in the global/ directory, but this is a quick (back-patchable) fix... Where do you suggest the file be written to? -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- 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] pg_stat_statements temporary file
On Friday, May 25, 2012 04:03:49 PM Peter Geoghegan wrote: I still think we should consider the placement of this file to not be in the global/ directory, but this is a quick (back-patchable) fix... Where do you suggest the file be written to? One could argue stats_temp_directory would be the correct place. Andres -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- 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] pg_stat_statements temporary file
Peter Geoghegan pe...@2ndquadrant.com writes: On 25 May 2012 14:13, Magnus Hagander mag...@hagander.net wrote: I still think we should consider the placement of this file to not be in the global/ directory, but this is a quick (back-patchable) fix... Where do you suggest the file be written to? Given that pgstats keeps its permanent file in global/, I think the argument that pg_stat_statements should not do likewise is pretty thin. 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] pg_stat_statements temporary file
On Fri, May 25, 2012 at 4:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Geoghegan pe...@2ndquadrant.com writes: On 25 May 2012 14:13, Magnus Hagander mag...@hagander.net wrote: I still think we should consider the placement of this file to not be in the global/ directory, but this is a quick (back-patchable) fix... Where do you suggest the file be written to? Given that pgstats keeps its permanent file in global/, I think the argument that pg_stat_statements should not do likewise is pretty thin. Fair enough. As long as the file is unlinked after reading (per my patch), it doesn't cause issues on a standby anymore, so it's a lot less important, I guess. It's mostly namespace invasion at this time... -- 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] pg_stat_statements temporary file
Magnus Hagander mag...@hagander.net writes: On Fri, May 25, 2012 at 4:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: Given that pgstats keeps its permanent file in global/, I think the argument that pg_stat_statements should not do likewise is pretty thin. Fair enough. As long as the file is unlinked after reading (per my patch), it doesn't cause issues on a standby anymore, so it's a lot less important, I guess. It's mostly namespace invasion at this time... Well, I could support moving both of those stats files someplace else, but it seems neatnik-ism more than something we have a definable need for. 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] pg_stat_statements temporary file
Andres Freund and...@2ndquadrant.com writes: On Friday, May 25, 2012 04:03:49 PM Peter Geoghegan wrote: Where do you suggest the file be written to? One could argue stats_temp_directory would be the correct place. No, that would be exactly the *wrong* place, because that directory can be on a RAM disk. We need to put this somewhere where it'll survive a shutdown. One could imagine creating a PGDATA subdirectory just for permanent (not temp) stats files, but right at the moment that seems like overkill. If we accumulate a few more similar files, I'd start to think it was worth doing. 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] pg_stat_statements temporary file
On 5/25/12 8:19 AM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On Friday, May 25, 2012 04:03:49 PM Peter Geoghegan wrote: Where do you suggest the file be written to? One could argue stats_temp_directory would be the correct place. No, that would be exactly the *wrong* place, because that directory can be on a RAM disk. We need to put this somewhere where it'll survive a shutdown. Mind you, I can imagine a busy system wanting to keep PSS on a ram disk as well. But users should be able to make that decision separately from the stats file. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] pg_stat_statements temporary file
On Fri, May 25, 2012 at 6:49 PM, Josh Berkus j...@agliodbs.com wrote: On 5/25/12 8:19 AM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On Friday, May 25, 2012 04:03:49 PM Peter Geoghegan wrote: Where do you suggest the file be written to? One could argue stats_temp_directory would be the correct place. No, that would be exactly the *wrong* place, because that directory can be on a RAM disk. We need to put this somewhere where it'll survive a shutdown. Mind you, I can imagine a busy system wanting to keep PSS on a ram disk as well. But users should be able to make that decision separately from the stats file. Why would they want that? PSS only writes the tempfile on shutdown and reads it on startup. Unlike pgstats which reads and writes it all the time. -- 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] pg_stat_statements temporary file
Why would they want that? PSS only writes the tempfile on shutdown and reads it on startup. Unlike pgstats which reads and writes it all the time. Ah, ok! Didn't know that. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] pg_stat_statements temporary file
On 24 May 2012 11:43, Magnus Hagander mag...@hagander.net wrote: In general, should a contrib module really store data in the global/ directory? Seems pretty ugly to me... I think the case could be made for moving pg_stat_statements into core, as an optionally enabled view, like pg_stat_user_functions, since pg_stat_statements is now rather a lot more useful than it used to be. That would solve that problem, as well as putting pg_stat_statements into the hands of the largest possible number of people, which would be a positive development, in my humble and fairly predictable opinion. However, pg_stat_statements will not prevent the database from starting if the file is corrupt. It makes some basic attempts to detect that within pgss_shmem_startup(), and will simply log the problem and unlink the file in the event of detecting corruption. Otherwise, I suppose you might get garbage values in pg_stat_statements, which, while rather annoying and possibly unacceptable, is hardly the end of the world. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- 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] pg_stat_statements temporary file
On Thu, May 24, 2012 at 1:36 PM, Peter Geoghegan pe...@2ndquadrant.com wrote: On 24 May 2012 11:43, Magnus Hagander mag...@hagander.net wrote: In general, should a contrib module really store data in the global/ directory? Seems pretty ugly to me... I think the case could be made for moving pg_stat_statements into core, as an optionally enabled view, like pg_stat_user_functions, since pg_stat_statements is now rather a lot more useful than it used to be. That would solve that problem, as well as putting pg_stat_statements into the hands of the largest possible number of people, which would be a positive development, in my humble and fairly predictable opinion. Well, it would solve the problem for this specific case - but there will always be yet another extension. Actually, it would only solve the *ugliness*, and not the actual problem. (That's not to say tha tI don't agree that moving it into core would be a good idea, but that's not happening for 9.2 - and the problem exists in 9.1 as well) However, pg_stat_statements will not prevent the database from starting if the file is corrupt. It makes some basic attempts to detect that within pgss_shmem_startup(), and will simply log the problem and unlink the file in the event of detecting corruption. Otherwise, I suppose you might get garbage values in pg_stat_statements, which, while rather annoying and possibly unacceptable, is hardly the end of the world. Ok. I was worried it might crash on loading the data when it was corrupt - say a size field that ended up specifying gigabytes that it then tries to allocate, or something like that. What actually happens if it tries to repalloc() something huge? palloc will throw an elog(ERROR), and since this happens during postmaster startup, are you sure it won't prevent the server from starting? -- 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] pg_stat_statements temporary file
On 24 May 2012 12:42, Magnus Hagander mag...@hagander.net wrote:
What actually happens if it tries to repalloc() something huge? palloc
will throw an elog(ERROR), and since this happens during postmaster
startup, are you sure it won't prevent the server from starting?
Oh, yes, missed that.
/* Previous incarnation might have had a larger query_size */
if (temp.query_len = buffer_size)
{
buffer = (char *) repalloc(buffer, temp.query_len + 1);
buffer_size = temp.query_len + 1;
}
Here, temp receives its value from an fread().
This could probably be coded to be defensive against such things, but
a better fix would be preferred. I have to wonder how much of a
problem corruption is likely to be though, given that we only save to
disk in a corresponding pgss_shmem_shutdown() call, which actually has
more protections against corruption. The window for the saved file to
be corrupt seems rather small, though I accept that a better window
would be zero.
--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services
--
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] pg_stat_statements temporary file
On Thu, May 24, 2012 at 2:16 PM, Peter Geoghegan pe...@2ndquadrant.com wrote:
On 24 May 2012 12:42, Magnus Hagander mag...@hagander.net wrote:
What actually happens if it tries to repalloc() something huge? palloc
will throw an elog(ERROR), and since this happens during postmaster
startup, are you sure it won't prevent the server from starting?
Oh, yes, missed that.
/* Previous incarnation might have had a larger query_size */
if (temp.query_len = buffer_size)
{
buffer = (char *) repalloc(buffer, temp.query_len + 1);
buffer_size = temp.query_len + 1;
}
Here, temp receives its value from an fread().
This could probably be coded to be defensive against such things, but
a better fix would be preferred. I have to wonder how much of a
problem corruption is likely to be though, given that we only save to
disk in a corresponding pgss_shmem_shutdown() call, which actually has
more protections against corruption. The window for the saved file to
be corrupt seems rather small, though I accept that a better window
would be zero.
Right. But writing to a temp file and rename()ing it into place is trivial.
It's really the other issues raised that are bigger ;)
--
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
