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 t
On 18/07/11 06:25, Tom Lane wrote:
Mark Kirkwood 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)
To make changes to your subsc
>> 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",
>>
Tatsuo Ishii 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 is(fd.c).
>
>> 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:
Mark Kirkwood 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 just paranoia, since on most
Tatsuo Ishii 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
> "temp f
Mark Kirkwood 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 & FD_TEMPORARY)
+ {
+
>> 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 y
=?ISO-8859-1?Q?C=E9dric_Villemain?= 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 temp file limit. C
2011/7/15 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,
>>
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
>> 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,
>>
>> ERR
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 te
> 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
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
2011/6/24 Mark Kirkwood :
>
> 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: Hopefully I've got a con
On 22/06/11 11:13, Mark Kirkwood wrote:
On 21/06/11 02:39, Cédric Villemain wrote:
2011/6/20 Robert Haas:
On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain
wrote:
The feature does not work exactly as expected because the write limit
is rounded per 8kB because we write before checking. I beli
On 21/06/11 02:39, Cédric Villemain wrote:
2011/6/20 Robert Haas:
On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain
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 pas
2011/6/20 Robert Haas :
> On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain
> 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 incre
On Mon, Jun 20, 2011 at 9:15 AM, Cédric Villemain
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 increment),
> and the temp_file_limi
2011/6/17 Cédric Villemain :
> 2011/6/17 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 g
2011/6/17 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
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 GIT_EXTERNA
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 format-patch
2011/6/15 Mark Kirkwood :
> On 15/06/11 02:52, Cédric Villemain wrote:
>>
>> 2011/6/3 Mark Kirkwood:
>>>
>>> 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
On 15/06/11 02:52, Cédric Villemain wrote:
2011/6/3 Mark Kirkwood:
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 v5. The changes are:
- guc i
2011/6/3 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/se
On 03/06/11 12:33, Cédric Villemain wrote:
2011/6/2 Mark Kirkwood:
On 01/06/11 09:24, Cédric Villemain wrote:
* I am not sure it is better to add a fileSize like you did or use
relationgetnumberofblock() when file is about to be truncated or
unlinked, this way the seekPos should be enough to in
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
+++ b/src/test/regress/serial_sc
On 03/06/11 02:36, Tom Lane wrote:
Robert Haas 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
On Thu, Jun 2, 2011 at 7:36 AM, Tom Lane wrote:
> 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 size, and would avoid (at least for the near term) the need to make
> the parameter a float.
As
On Thu, Jun 2, 2011 at 10:58 AM, Cédric Villemain
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.
--
Robert Haas
EnterpriseDB: http:
2011/6/2 Robert Haas :
> On Wed, Jun 1, 2011 at 7:35 PM, Mark Kirkwood
> 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 mo
Robert Haas 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 size, and would avoi
On Wed, Jun 1, 2011 at 7:35 PM, Mark Kirkwood
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 memory than this, then you have
On 02/06/11 18:34, Jaime Casanova wrote:
On Wed, Jun 1, 2011 at 6:35 PM, Mark Kirkwood
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 normal users to restrict thei
On Wed, Jun 1, 2011 at 6:35 PM, Mark Kirkwood
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 here but not e
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 ;
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 here but not explicit about 'temp tables',
maybe worth adding that this won't limit
On 01/06/11 12:32, Mark Kirkwood wrote:
On 01/06/11 12:27, Mark Kirkwood wrote:
Re the code comments - I agree with most of them. However with
respect to the Guc units, I copied the setup for work_mem as that
seemed the most related.
Also - forget to mention - I *thought* you could specify
On 01/06/11 12:27, Mark Kirkwood wrote:
Re the code comments - I agree with most of them. However with respect
to the Guc units, I copied the setup for work_mem as that seemed the
most related.
Also - forget to mention - I *thought* you could specify the temp files
size GUC as KB, MB, GB or
On 01/06/11 09:24, Cédric Villemain wrote:
Hello
here is a partial review of your patch, better than keeping it
sleeping in the commitfest queue I hope.
Submission review
* The patch is not in context diff format.
* The patch apply, but contains some extra whitespa
43 matches
Mail list logo
