Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-20 Thread Mark Kirkwood
And I thought I should mention: a big thank you to the the reviewers and interested parties, Cedric, Tatsuo, Robert and Tom who did a review + fixes as he committed the patch - nice work guys! regards Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-17 Thread Tom Lane
Mark Kirkwood mark.kirkw...@catalyst.net.nz writes: [ temp-files-v6.patch.gz ] I've applied this patch with some editorialization, notably: I changed the datatype of temporary_files_size to uint64 so as to avoid worries about accumulating roundoff error over a long transaction. This is probably

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-17 Thread Tatsuo Ishii
Could you please elaborate why Current usage 8000kB can bigger than temp file limit 8kB? I undertstand the point that temp files are allocated by 8kB at once, but I don't understand why those numbers you suggested could happen. Actually I tried with the modified patches and got: test=#

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-17 Thread Tom Lane
Tatsuo Ishii is...@postgresql.org writes: You didn't show us how you computed those numbers, but I'm really dubious that FileWrite() has got any ability to produce numbers that are helpful. Like Cedric, I think the write amount in any one call is usually going to be one block. Here it

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-17 Thread Tatsuo Ishii
ereport(ERROR, (errcode(ERRCODE_QUERY_CANCELED), errmsg(aborting due to exceeding temp file limit, current usage %dkB, requested size %dkB, thus it will exceed temp file limit %dkB,

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-17 Thread Mark Kirkwood
On 18/07/11 06:25, Tom Lane wrote: Mark Kirkwoodmark.kirkw...@catalyst.net.nz writes: [ temp-files-v6.patch.gz ] I've applied this patch with some editorialization, notably: Awesome, the changes look great! Cheers Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-16 Thread Tatsuo Ishii
I modeled the original message on what happens when statement timeout is exceeded, which doesn't state its limit in the error message at all - actually I did wonder if there is was informal standard for *not* stating the value of the limit that is being exceeded! However, I agree with you and

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-16 Thread Tom Lane
Mark Kirkwood mark.kirkw...@catalyst.net.nz writes: This version moves the check *before* we write the new buffer, so should take care of issues about really large write buffers, plugins etc. This logic seems pretty obviously wrong: + if (temp_file_limit = 0 VfdCache[file].fdstate

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-16 Thread Tom Lane
Tatsuo Ishii is...@postgresql.org writes: Remember that what will happens is probably: ERROR: aborting due to exceeding temp file limit. Current usage 8000kB, requested size 8008kB, thus it will exceed temp file limit 8kB. Could you please elaborate why Current usage 8000kB can bigger than

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-15 Thread Mark Kirkwood
On 15/07/11 14:57, Tatsuo Ishii wrote: Maybe we could add more info regarding current usage and requested amount in addition to the temp file limit value. I mean something like: ERROR: aborting due to exceeding temp file limit. Current usage 9000kB, requested size 1024kB, thus it will exceed

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-15 Thread Cédric Villemain
2011/7/15 Mark Kirkwood mark.kirkw...@catalyst.net.nz: On 15/07/11 14:57, Tatsuo Ishii wrote: Maybe we could add more info regarding current usage and requested amount in addition to the temp file limit value. I mean something like: ERROR:  aborting due to exceeding temp file limit. Current

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-15 Thread Tom Lane
=?ISO-8859-1?Q?C=E9dric_Villemain?= cedric.villemain.deb...@gmail.com writes: On 15/07/11 14:57, Tatsuo Ishii wrote: Maybe we could add more info regarding current usage and requested amount in addition to the temp file limit value. I mean something like: ERROR:  aborting due to exceeding

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-14 Thread Tatsuo Ishii
Hackers, This patch needs a new reviewer, per Cedric. Please help! Hi I am the new reviewer:-) I have looked into the v6 patches. One thing I would like to suggest is, enhancing the error message when temp_file_limit will be exceeded. ERROR: aborting due to exceeding temp file limit Is

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-14 Thread Mark Kirkwood
On 14/07/11 18:48, Tatsuo Ishii wrote: Hi I am the new reviewer:-) I have looked into the v6 patches. One thing I would like to suggest is, enhancing the error message when temp_file_limit will be exceeded. ERROR: aborting due to exceeding temp file limit Is it possible to add the current

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-14 Thread Tatsuo Ishii
I have looked into the v6 patches. One thing I would like to suggest is, enhancing the error message when temp_file_limit will be exceeded. ERROR: aborting due to exceeding temp file limit Is it possible to add the current temp file limit to the message? For example, ERROR: aborting due

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-07-10 Thread Josh Berkus
Hackers, This patch needs a new reviewer, per Cedric. Please help! -- 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] Re: patch review : Add ability to constrain backend temporary file space

2011-07-07 Thread Cédric Villemain
2011/6/24 Mark Kirkwood mark.kirkw...@catalyst.net.nz: This  version moves the check *before* we write the new buffer, so should take care of issues about really large write buffers, plugins etc. Also I *think* that means there is no need to amend the documentation. Cheers Mark P.s:

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-24 Thread Mark Kirkwood
On 22/06/11 11:13, Mark Kirkwood wrote: On 21/06/11 02:39, Cédric Villemain wrote: 2011/6/20 Robert Haasrobertmh...@gmail.com: On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain cedric.villemain.deb...@gmail.com wrote: The feature does not work exactly as expected because the write limit is

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-21 Thread Mark Kirkwood
On 21/06/11 02:39, Cédric Villemain wrote: 2011/6/20 Robert Haasrobertmh...@gmail.com: On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain cedric.villemain.deb...@gmail.com wrote: The feature does not work exactly as expected because the write limit is rounded per 8kB because we write before

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-20 Thread Cédric Villemain
2011/6/17 Cédric Villemain cedric.villemain.deb...@gmail.com: 2011/6/17 Mark Kirkwood mark.kirkw...@catalyst.net.nz: On 17/06/11 13:08, Mark Kirkwood wrote: On 17/06/11 09:49, Cédric Villemain wrote: I have issues applying it. Please can you remove trailing space? Also, you can generate a

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-20 Thread Robert Haas
On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain cedric.villemain.deb...@gmail.com wrote: The feature does not work exactly as expected because the write limit is rounded per 8kB because we write before checking. I believe if one write a file of 1GB in one pass (instead of repetitive 8kB

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-20 Thread Cédric Villemain
2011/6/20 Robert Haas robertmh...@gmail.com: On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain cedric.villemain.deb...@gmail.com wrote: The feature does not work exactly as expected because the write limit is rounded per 8kB because we write before checking. I believe if one write a file of

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-17 Thread Cédric Villemain
2011/6/17 Mark Kirkwood mark.kirkw...@catalyst.net.nz: On 17/06/11 13:08, Mark Kirkwood wrote: On 17/06/11 09:49, Cédric Villemain wrote: I have issues applying it. Please can you remove trailing space? Also, you can generate a cool patch like this : get git-external-diff from

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-16 Thread Cédric Villemain
2011/6/15 Mark Kirkwood mark.kirkw...@catalyst.net.nz: On 15/06/11 02:52, Cédric Villemain wrote: 2011/6/3 Mark Kirkwoodmark.kirkw...@catalyst.net.nz: Corrected v4 patch with the test files, for completeness. Note that discussion has moved on and there will be a v5 :-) Mark, can you submit

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-16 Thread Mark Kirkwood
On 17/06/11 09:49, Cédric Villemain wrote: I have issues applying it. Please can you remove trailing space? Also, you can generate a cool patch like this : get git-external-diff from postgres/src/tools to /usr/lib/git-core/ chmod +x it export GIT_EXTERNAL_DIFF=git-external-diff git

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-16 Thread Mark Kirkwood
On 17/06/11 13:08, Mark Kirkwood wrote: On 17/06/11 09:49, Cédric Villemain wrote: I have issues applying it. Please can you remove trailing space? Also, you can generate a cool patch like this : get git-external-diff from postgres/src/tools to /usr/lib/git-core/ chmod +x it export

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-14 Thread Cédric Villemain
2011/6/3 Mark Kirkwood mark.kirkw...@catalyst.net.nz: On 02/06/11 18:34, Jaime Casanova wrote: - the patch adds this to serial_schedule but no test has been added... diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule index bb654f9..325cb3d 100644 ---

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-14 Thread Mark Kirkwood
On 15/06/11 02:52, Cédric Villemain wrote: 2011/6/3 Mark Kirkwoodmark.kirkw...@catalyst.net.nz: Corrected v4 patch with the test files, for completeness. Note that discussion has moved on and there will be a v5 :-) Mark, can you submit your updated patch ? Thanks for the reminder! Here is

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Jaime Casanova
On Wed, Jun 1, 2011 at 6:35 PM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: On 01/06/11 09:24, Cédric Villemain wrote:  Submission review     * The patch is not in context diff format.     * The patch apply, but contains some extra whitespace.     * Documentation is

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Mark Kirkwood
On 02/06/11 18:34, Jaime Casanova wrote: On Wed, Jun 1, 2011 at 6:35 PM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: I've created a new patch (attached) Hi Mark, A few comments: - why only superusers can set this? if this is a per-backend setting, i don't see the problem in allowing

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Robert Haas
On Wed, Jun 1, 2011 at 7:35 PM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: Done - 'work_disk' it is to match 'work_mem'. I guess I'm bikeshedding here, but I'm not sure I really buy this parallel. work_mem is primarily a query planner parameter; it says, if you're going to need more

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: So I'm not sure work_disk is a great name. I agree. Maybe something along the lines of temp_file_limit? Also, once you free yourself from the analogy to work_mem, you could adopt some more natural unit than KB. I'd think MB would be a practical unit

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Cédric Villemain
2011/6/2 Robert Haas robertmh...@gmail.com: On Wed, Jun 1, 2011 at 7:35 PM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: Done - 'work_disk' it is to match 'work_mem'. I guess I'm bikeshedding here, but I'm not sure I really buy this parallel.  work_mem is primarily a query planner

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Robert Haas
On Thu, Jun 2, 2011 at 10:58 AM, Cédric Villemain cedric.villemain.deb...@gmail.com wrote: I am not specially attached to a name, idea was not to use work_disk but backend_work_disk. I agree with you anyway, and suggestion from Tom is fine for me (temp_file_limit). Yeah, I like that too. --

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Mark Kirkwood
On 03/06/11 02:36, Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: So I'm not sure work_disk is a great name. I agree. Maybe something along the lines of temp_file_limit? Also, once you free yourself from the analogy to work_mem, you could adopt some more natural unit than KB. I'd

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-02 Thread Mark Kirkwood
On 02/06/11 18:34, Jaime Casanova wrote: - the patch adds this to serial_schedule but no test has been added... diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule index bb654f9..325cb3d 100644 --- a/src/test/regress/serial_schedule +++

Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space

2011-06-01 Thread Mark Kirkwood
On 02/06/11 11:35, Mark Kirkwood wrote: On 01/06/11 09:24, Cédric Villemain wrote: Simple Feature test == either explain buffers is wrong or the patch is wrong: cedric=# explain (analyze,buffers) select * from foo order by 1 desc ;