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
[HACKERS] pg_stat_statements temporary file
If pg_stat_statements is set to store it's data across restarts, it stores it in global/pg_stat_statements.stat. This causes some interesting things to happen in combination with a base backup - namely, if you take a base backup *after* you have restarted th emaster, the slave will get a snapshot of whatever was in the temporary file at the time of the restart. This is quite unpredictable - particularly in relation to a slave where it gets a snapshot from the last restart, not from the base backup, after which it diverges. AFAICT, it also has the property that if the server crashes, it will reload the latest snapshot - not reset to 0 or anything like that. Finally, if the server were to crash *while* the file is being written, it will get a corrupt file (I haven't tested this part, but it's rather obvious from the code). I'm pretty sure this could lead to a situation where the database wouldn't restart. Fixing the last part is easy - we need to write the file to a temporary file and then rename() it into place, like we do with the stats collectors file. Fixing the first one, I can think of a few things: 1) unlink() the file after we've read it. 2) forcibly exclude the file from base backups taken with pg_basebackup. We'd still have the problem when it comes to backups taken manually. 3) avoid loading the file on a standby (that wouldn't fix the similar problem on the master of course) And perhaps some other solution I haven't thought of? If we want to go with option 2, we have another problem - it's in the global directory. But the name is dependent on what's in a contrib module. Meaning we'd have to teach core postgresql about a contrib modules filename, which pretty much breaks the abstraction layer. Perhaps a better choice here would be to create another directory under the data directory that is always excluded from base backup, and store it there? That would also work for third party modules that core can never learn about... In general, should a contrib module really store data in the global/ directory? Seems pretty ugly to me... -- 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 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
