From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Peter Geoghegan <peter(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_stat_statements temporary file |
Date: | 2012-05-25 13:13:20 |
Message-ID: | CABUevEwfoV_Az4bsceET0y_573ztDEXKonEBkSHz-qGURca0WA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, May 24, 2012 at 2:19 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, May 24, 2012 at 2:16 PM, Peter Geoghegan <peter(at)2ndquadrant(dot)com> wrote:
>> On 24 May 2012 12:42, Magnus Hagander <magnus(at)hagander(dot)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/
Attachment | Content-Type | Size |
---|---|---|
pg_stat_statements_file.patch | application/octet-stream | 1.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Sergey Koposov | 2012-05-25 13:18:04 | Re: 9.2beta1, parallel queries, ReleasePredicateLocks, CheckForSerializableConflictIn in the oprofile |
Previous Message | Merlin Moncure | 2012-05-25 13:06:47 | Re: 9.2beta1, parallel queries, ReleasePredicateLocks, CheckForSerializableConflictIn in the oprofile |