Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space
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 to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space
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 just paranoia, since on most machines double can store integers exactly up to 2^52 which is more than the largest possible temp_file_limit, but it seemed safer. fileSize of a temp file, and temporary_files_size, must be updated correctly whether or not temp_file_limit is currently being enforced. Otherwise things do not behave sanely if temp_file_limit is turned on mid-transaction. Fixed bogus calculation in enforcement check, per my note yesterday. Added a new SQLSTATE code, ERRCODE_CONFIGURATION_LIMIT_EXCEEDED, because I felt that ERRCODE_QUERY_CANCELED wasn't at all appropriate for this. (Maybe we should think about changing the statement_timeout code to use that too, although I'm not entirely sure that we can easily tell statement_timeout apart from a query cancel.) Rephrased the error message to temporary file size exceeds temp_file_limit (%dkB), as per Tatsuo's first suggestion but not his second. There's still room for bikeshedding here, of course. Removed the reset of temporary_files_size in CleanupTempFiles. It was inappropriate because some temp files can survive CleanupTempFiles, and unnecessary anyway because the counter is correctly decremented when unlinking a temp file. (This was another reason for wanting temporary_files_size to be exact, because we're now doing dead reckoning over the life of the session.) Set the maximum value of temp_file_limit to INT_MAX not MAX_KILOBYTES. There's no reason for the limit to be less on 32-bit machines than 64-bit, since we are doing arithmetic in uint64 not size_t. Minor rewrite of documentation. Also, I didn't add the proposed regression test, because it seems shaky to me. In an installation with larger than default work_mem, the sort might not spill to disk. I don't think this feature is large enough to deserve its very own, rather expensive, regression test anyway. 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] Re: patch review : Add ability to constrain backend temporary file space
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=# CREATE TEMPORARY TABLE resourcetemp1 AS SELECT
generate_series(1,10);
SELECT 10
test=# SET temp_file_limit = 578;
SET
test=# SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
ERROR: aborting due to exceeding temp file limit, current usage 576kB,
requested size 8192kB, thus it will exceed temp file limit 578kB
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).
/*
* If performing this write will increase the file size, then abort if
it will
* exceed temp_file_limit
*/
if (temp_file_limit = 0 VfdCache[file].fdstate FD_TEMPORARY)
{
if (VfdCache[file].seekPos + amount = VfdCache[file].fileSize)
{
increaseSize = true;
if ((temporary_files_size + (double)amount) / 1024.0
(double)temp_file_limit)
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,
(int)(temporary_files_size/1024),
amount,
temp_file_limit)));
}
}
I also attached the whole patch against fd.c at the point when Mark
proposed the changes.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp
fd.c.patch.gz
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] Re: patch review : Add ability to constrain backend temporary file space
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 is(fd.c). 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, (int)(temporary_files_size/1024), amount, temp_file_limit))); The thing is that unless amount is really large, you're just going to have two numbers that are very close to each other. I think this is just useless complication, because amount is almost always going to be 8kB or less. 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] Re: patch review : Add ability to constrain backend temporary file space
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, (int)(temporary_files_size/1024), amount, temp_file_limit))); The thing is that unless amount is really large, you're just going to have two numbers that are very close to each other. I think this is just useless complication, because amount is almost always going to be 8kB or less. Oops. My bad. I should have divide amount by 1024. Now I understand your point. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp -- 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
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) 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
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 think it makes sense to include it here. I wonder if the additional detail you are suggesting above might be better added to a HINT - what do you think? If it is a better idea to just add it in the message as above I can certainly do that. 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. because temp file are increased by 8kb at once, rarely more (and by rare I mean that it can happens via an extension or in the future, not with current core postgresql). 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=# CREATE TEMPORARY TABLE resourcetemp1 AS SELECT generate_series(1,10); SELECT 10 test=# SET temp_file_limit = 578; SET test=# SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a; ERROR: aborting due to exceeding temp file limit, current usage 576kB, requested size 8192kB, thus it will exceed temp file limit 578kB Here temp_file_limit is not specified by 8kB unit, so current usage becomes 576kB, which is 8kB unit. I don't think those numbers will terribly confuse DBAs.. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp -- 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
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 FD_TEMPORARY)
+ {
+ if (VfdCache[file].seekPos + amount = VfdCache[file].fileSize)
+ {
+ increaseSize = true;
+ if ((temporary_files_size + (double)amount) / 1024.0
(double)temp_file_limit)
+ ereport(ERROR,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg(aborting due to exceeding temp file limit)));
+ }
+ }
It's supposing that the write length (amount) is exactly the amount by
which the file length will grow, which would only be true if seekPos is
exactly equal to fileSize before the write. I think that would often be
the case in our usage patterns, but surely code at this low level should
not be assuming that.
BTW, the definition of the GUC seems to constrain the total temp file
size to be no more than 2GB on 32-bit machines ... do we really want
it to act like that?
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] Re: patch review : Add ability to constrain backend temporary file space
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 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=# CREATE TEMPORARY TABLE resourcetemp1 AS SELECT generate_series(1,10); SELECT 10 test=# SET temp_file_limit = 578; SET test=# SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a; ERROR: aborting due to exceeding temp file limit, current usage 576kB, requested size 8192kB, thus it will exceed temp file limit 578kB 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. 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] Re: patch review : Add ability to constrain backend temporary file space
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 temp file limit 1kB. 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 think it makes sense to include it here. I wonder if the additional detail you are suggesting above might be better added to a HINT - what do you think? If it is a better idea to just add it in the message as above I can certainly do that. Cheers Mark -- 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/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 usage 9000kB, requested size 1024kB, thus it will exceed temp file limit 1kB. 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 think it makes sense to include it here. I wonder if the additional detail you are suggesting above might be better added to a HINT - what do you think? If it is a better idea to just add it in the message as above I can certainly do that. 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. because temp file are increased by 8kb at once, rarely more (and by rare I mean that it can happens via an extension or in the future, not with current core postgresql). This kind of message can also trouble the DBA by suggesting that the query doesn't need more than what was asking at this moment. That's being said I have no strong opinion for the HINT message if the documentation is clear enough to not confuse DBA. -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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
=?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 temp file limit. Current usage 9000kB, requested size 1024kB, thus it will exceed temp file limit 1kB. 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. because temp file are increased by 8kb at once, rarely more Yes. I think the extra detail Tatsuo proposes is useless and possibly confusing. I don't object to stating the current limit, but the other numbers are not likely to be helpful to anyone. (If we had some way of knowing what the file would ultimately grow to if we allowed the query to continue, that would be useful; but of course we don't know that.) 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] Re: patch review : Add ability to constrain backend temporary file space
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 it possible to add the current temp file limit to the message? For example, ERROR: aborting due to exceeding temp file limit 1kB I know the current setting of temp_file_limit can be viewd in other ways, but I think this will make admin's or application developer's life a little bit easier. -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp -- 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
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 temp file limit to the message? For example, ERROR: aborting due to exceeding temp file limit 1kB I know the current setting of temp_file_limit can be viewd in other ways, but I think this will make admin's or application developer's life a little bit easier. Hi Tatsuo, Yeah, good suggestion - I agree that it would be useful to supply extra detail, I'll amend and resubmit a new patch (along with whatever review modifications we come up with in the meantime)! Cheers Mark -- 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
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 to exceeding temp file limit 1kB I know the current setting of temp_file_limit can be viewd in other ways, but I think this will make admin's or application developer's life a little bit easier. Hi Tatsuo, Yeah, good suggestion - I agree that it would be useful to supply extra detail, I'll amend and resubmit a new patch (along with whatever review modifications we come up with in the meantime)! Cheers Mark 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 temp file limit 1kB. What do you think? -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp -- 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
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/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: Hopefully I've got a context diff this time, just realized that git diff -c does *not* produce a context diff (doh). Mark, I have this (hopefully) final review in my TODO list, I won't be able to check until CHAR(11) is done, and probably not before the 19th of July. So... if one want to make progress in the meantime, please do. -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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
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 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_limit is 0, then the server will write the 1GB before aborting. Can we rearrange thing so we check first, and then write? probably but it needs more work to catch corner cases. We may be safe to just document that (and also in the code). The only way I see so far to have a larger value than 8kB here is to have a plugin doing the sort instead of the postgresql core sort algo. Thanks guys - will look at moving the check, and adding some documentation about the possible impacts of plugins (or new executor methods) that might write in chunks bigger than blocksz. Maybe a few days - I'm home sick ATM, plus looking after these http://www.maftet.co.nz/kittens.html 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 context diff this time, just realized that git diff -c does *not* produce a context diff (doh). temp-files-v6.patch.gz Description: GNU Zip compressed 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] Re: patch review : Add ability to constrain backend temporary file space
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 checking. I believe if one write a file of 1GB in one pass (instead of repetitive 8kB increment), and the temp_file_limit is 0, then the server will write the 1GB before aborting. Can we rearrange thing so we check first, and then write? probably but it needs more work to catch corner cases. We may be safe to just document that (and also in the code). The only way I see so far to have a larger value than 8kB here is to have a plugin doing the sort instead of the postgresql core sort algo. Thanks guys - will look at moving the check, and adding some documentation about the possible impacts of plugins (or new executor methods) that might write in chunks bigger than blocksz. Maybe a few days - I'm home sick ATM, plus looking after these http://www.maftet.co.nz/kittens.html Cheers Mark -- 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/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 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 --ext-diff origin I think I have the trailing spaces removed, and patch is updated for the variable renaming recently done in fd.c I have no idea why I can't get the git apply to work (obviously I have exceeded by git foo by quite a ways), but it should apply for you I hope (as it patches fine). If I didn't made mistake the attached patch does not have trailling space anymore and I did a minor cosmetic in FileClose. It is not in the expected format required by postgresql commiters but can be applyed with git apply... It looks like the issue is that patch generated with the git-ext-diff can not be git applyed (they need to use patch). Either I did something wrong or git-ext-diff format is not so great. I didn't test and all yet. From reading, the patch looks sane. I'll review it later this day or this week-end. Submission review === I have updated the patch for cosmetic. (I attach 2 files: 1 patch for 'git-apply' and 1 for 'patch') I have also updated the guc.c entry (lacks of a NULL for the last Hooks handler and make the pg_settings comment more like other GUC) The patch includes doc and test. Usability review = The patch implements what it claims modulo the BLK size used by PostgreSQL: the filewrite operation happens before the test on file limit and PostgreSQL write per 8kB. The result is that a temp_file_limit=0 does not prevent temp_file writing or creation, but stop the writing at 8192 bytes. I've already argue to keep the value as a BLKSZ, but I don't care to have kB by default. The doc may need to be updated to reflect this behavior. Maybe we want to disable the temp_file writing completely with the value set to 0 ? (it looks useless to me atm but someone may have a usecase for that ?!) I don't believe it follows any SQL specs, and so far the community did not refuse the idea, so looks in the good way. I believe we want it, my only grippe is that a large value don't prevent server to use a lot of IO before the operation is aborted, I don't see real alternative at this level though. After initial testing some corner cases have been outline (in previous versions of the patch), so I believe the current code fix them well as I didn't have issue again. Feature test 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_limit is 0, then the server will write the 1GB before aborting. Also, the patch does not handle a !(returnCode =0) in FileWrite. I don't know if it is needed or not. As the counter is per session and we can have very long session, it may hurtbut the problem will be larger than just this counter in such case so...is it worth the trouble to handle it ? Performance review === not done. Coding review = I have done some update to the patch to make it follow the coding guidelines. I don't think it can exist a portability issue. Tom did say: 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. Architecture review IMO, the code used to log_temp_file is partially deprecated by this feature: the system call to stat() looks now useles and can be removed as well as the multiple if () around temp_file_limit or log_temp_file can be merged. Review review = I didn't test for performances, I have not tryed to overflow temporary_files_size but looks hard to raise. (this might be possible if the temp_file_limit is not used but the temporary_files_size incremented, but current code does not allow to increment one without using the feature, so it is good I think) Mark, please double check that my little updates of your patch did not remove anything or modify the behavior in a wrong way. Thanks for your good job, -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 794aef4..19dc440 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1025,6 +1025,43 @@ SET ENABLE_SEQSCAN TO OFF; /variablelist /sect2 + sect2 id=runtime-config-resource-disk + titleDisk/title + variablelist + + varlistentry id=guc-temp-file-limit xreflabel=temp_file_limit + termvarnametemp_file_limit/varname
Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space
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 increment), and the temp_file_limit is 0, then the server will write the 1GB before aborting. Can we rearrange thing so we check first, and then write? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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/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 1GB in one pass (instead of repetitive 8kB increment), and the temp_file_limit is 0, then the server will write the 1GB before aborting. Can we rearrange thing so we check first, and then write? probably but it needs more work to catch corner cases. We may be safe to just document that (and also in the code). The only way I see so far to have a larger value than 8kB here is to have a plugin doing the sort instead of the postgresql core sort algo. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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/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 postgres/src/tools to /usr/lib/git-core/
chmod +x it
export GIT_EXTERNAL_DIFF=git-external-diff
git format-patch --ext-diff origin
I think I have the trailing spaces removed, and patch is updated for the
variable renaming recently done in fd.c
I have no idea why I can't get the git apply to work (obviously I have
exceeded by git foo by quite a ways), but it should apply for you I hope (as
it patches fine).
If I didn't made mistake the attached patch does not have trailling
space anymore and I did a minor cosmetic in FileClose. It is not in
the expected format required by postgresql commiters but can be
applyed with git apply...
It looks like the issue is that patch generated with the git-ext-diff
can not be git applyed (they need to use patch).
Either I did something wrong or git-ext-diff format is not so great.
I didn't test and all yet. From reading, the patch looks sane. I'll
review it later this day or this week-end.
--
Cédric Villemain 2ndQuadrant
http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e835e4b..80d7c35 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1025,6 +1025,43 @@ SET ENABLE_SEQSCAN TO OFF;
/variablelist
/sect2
+ sect2 id=runtime-config-resource-disk
+ titleDisk/title
+ variablelist
+
+ varlistentry id=guc-temp-file-limit xreflabel=temp_file_limit
+ termvarnametemp_file_limit/varname (typeinteger/type)/term
+ indexterm
+ primaryvarnametemp_file_limit/ configuration parameter/primary
+ /indexterm
+ listitem
+ para
+Specifies the amount of disk space used by internal sort operations
+and hash tables whist writing to temporary disk files. The value
+defaults to unlimited (literal-1/). Values larger than zero
+constraint the temporary file space usage to be that number of
+kilobytes.
+ /para
+ para
+A given sort or hash operation may write a number of temporary files,
+the total space used by all the files produced by one backend is
+constrained to be this value or less. If further bytes are written
+the current query is canceled. Only superusers can change this
+setting.
+ /para
+ para
+It should be noted that this parameter does emphasisnot/emphasis
+constrain disk space used for temporary table storage. However if
+the temporary table is created from a query then the any sort
+and hash files used in query execution will have their space
+controlled as above.
+ /para
+ /listitem
+ /varlistentry
+
+ /variablelist
+ /sect2
+
sect2 id=runtime-config-resource-kernel
titleKernel Resource Usage/title
variablelist
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 820e6db..5c00889 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -131,6 +131,11 @@ static int max_safe_fds = 32; /* default if not changed */
/* Flag to tell whether there are files to close/delete at end of transaction */
static bool have_pending_fd_cleanup = false;
+/*
+ * Track the total size of all temporary files
+ */
+static double temporary_files_size = 0.0;
+
typedef struct vfd
{
int fd;/* current FD, or VFD_CLOSED if none */
@@ -140,6 +145,7 @@ typedef struct vfd
File lruMoreRecently; /* doubly linked recency-of-use list */
File lruLessRecently;
off_t seekPos; /* current logical file position */
+ off_t fileSize; /* current size of file */
char *fileName; /* name of file, or NULL for unused VFD */
/* NB: fileName is malloc'd, and must be free'd when closing the VFD */
int fileFlags; /* open(2) flags for (re)opening the file */
@@ -887,6 +893,7 @@ PathNameOpenFile(FileName fileName, int fileFlags, int fileMode)
vfdP-fileFlags = fileFlags ~(O_CREAT | O_TRUNC | O_EXCL);
vfdP-fileMode = fileMode;
vfdP-seekPos = 0;
+ vfdP-fileSize = 0;
vfdP-fdstate = 0x0;
vfdP-resowner = NULL;
@@ -1123,6 +1130,13 @@ FileClose(File file)
if (unlink(vfdP-fileName))
elog(LOG, could not unlink file \%s\: %m, vfdP-fileName);
}
+
+ if (temp_file_limit = 0)
+ {
+ /* subtract the unlinked file size from the total */
+ temporary_files_size -= (double)vfdP-fileSize;
+ vfdP-fileSize = 0;
+ }
}
/* Unregister it from the resource owner */
@@ -1251,7 +1265,27 @@ retry:
errno = ENOSPC;
if (returnCode = 0)
+ {
VfdCache[file].seekPos += returnCode;
+
+ if (temp_file_limit = 0 VfdCache[file].fdstate FD_TEMPORARY)
+ {
+ /*
+ * if we
Re: [HACKERS] Re: patch review : Add ability to constrain backend temporary file space
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 your updated patch ? Thanks for the reminder! Here is v5. The changes are: 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 --ext-diff origin my log: $ git apply temp-files-v5.patch temp-files-v5.patch:22: trailing whitespace. defaults to unlimited (literal-1/). Values larger than zero temp-files-v5.patch:23: trailing whitespace. constraint the temporary file space usage to be that number of temp-files-v5.patch:28: trailing whitespace. the total space used by all the files produced by one backend is temp-files-v5.patch:35: trailing whitespace. constrain disk space used for temporary table storage. However if temp-files-v5.patch:105: trailing whitespace. /* error: patch failed: src/backend/storage/file/fd.c:132 error: src/backend/storage/file/fd.c: patch does not apply error: src/test/regress/expected/resource.out: No such file or directory error: src/test/regress/sql/resource.sql: No such file or directory - guc is called temp_file_limit, which seems like the best choice to date :-) - removed code to do with truncating files, as after testing I agree with you that temp work files don't seem to get truncated. I have not done anything about the business on units - so we are in KB still - there is no MB unit avaiable in the code as yet - I'm not sure we need one at this point, as most folk who use this feature will find 4096GB a big enough *per backend* limit. Obviously it might be a good investment to plan to have MB, and GB as possible guc units too. Maybe this could be a separate piece of work since any *other* resource limiters we add might find it convenient to have these available? Cheers Mark -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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
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 --ext-diff origin my log: $ git apply temp-files-v5.patch temp-files-v5.patch:22: trailing whitespace. defaults to unlimited (literal-1/). Values larger than zero temp-files-v5.patch:23: trailing whitespace. constraint the temporary file space usage to be that number of temp-files-v5.patch:28: trailing whitespace. the total space used by all the files produced by one backend is temp-files-v5.patch:35: trailing whitespace. constrain disk space used for temporary table storage. However if temp-files-v5.patch:105: trailing whitespace. /* error: patch failed: src/backend/storage/file/fd.c:132 error: src/backend/storage/file/fd.c: patch does not apply error: src/test/regress/expected/resource.out: No such file or directory error: src/test/regress/sql/resource.sql: No such file or directory I can generate a patch that way no problem - however it does not apply using the above method: $ git apply /data0/download/postgres/patches/temp/temp-files-v5.1.patch error: doc/src/sgml/config.sgml: already exists in working directory error: src/backend/storage/file/fd.c: already exists in working directory error: src/backend/utils/misc/guc.c: already exists in working directory error: src/backend/utils/misc/postgresql.conf.sample: already exists in working directory error: src/include/utils/guc.h: already exists in working directory error: src/include/utils/guc_tables.h: already exists in working directory error: src/test/regress/serial_schedule: already exists in working directory However it applies just fine using the normal method: $ patch --dry-run -p1 /data0/download/postgres/patches/temp/temp-files-v5.1.patch patching file doc/src/sgml/config.sgml patching file src/backend/storage/file/fd.c patching file src/backend/utils/misc/guc.c patching file src/backend/utils/misc/postgresql.conf.sample patching file src/include/utils/guc.h patching file src/include/utils/guc_tables.h patching file src/test/regress/expected/resource.out patching file src/test/regress/serial_schedule patching file src/test/regress/sql/resource.sql Any idea? regards Mark -- 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
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_EXTERNAL_DIFF=git-external-diff git format-patch --ext-diff origin I think I have the trailing spaces removed, and patch is updated for the variable renaming recently done in fd.c I have no idea why I can't get the git apply to work (obviously I have exceeded by git foo by quite a ways), but it should apply for you I hope (as it patches fine). regards Mark temp-files-v5.1.patch.gz Description: GNU Zip compressed 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] Re: patch review : Add ability to constrain backend temporary file space
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 --- a/src/test/regress/serial_schedule +++ b/src/test/regress/serial_schedule @@ -127,3 +127,4 @@ test: largeobject test: with test: xml test: stats +test: resource 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 ? -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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
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 v5. The changes are: - guc is called temp_file_limit, which seems like the best choice to date :-) - removed code to do with truncating files, as after testing I agree with you that temp work files don't seem to get truncated. I have not done anything about the business on units - so we are in KB still - there is no MB unit avaiable in the code as yet - I'm not sure we need one at this point, as most folk who use this feature will find 4096GB a big enough *per backend* limit. Obviously it might be a good investment to plan to have MB, and GB as possible guc units too. Maybe this could be a separate piece of work since any *other* resource limiters we add might find it convenient to have these available? Cheers Mark temp-files-v5.patch.gz Description: GNU Zip compressed 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] Re: patch review : Add ability to constrain backend temporary file space
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 here but not explicit about 'temp tables', maybe worth adding that this won't limit temporary table size ? * There is no test provided. One can be expected to check that the feature work. 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 their own queries - why the calculations are done as double? + if (temporary_files_size / 1024.0 (double)work_disk) - 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_schedule @@ -127,3 +127,4 @@ test: largeobject test: with test: xml test: stats +test: resource -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte y capacitación de PostgreSQL -- 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
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 normal users to restrict their own queries Yeah, that's a good point, I was thinking that as a resource control parameter it might be good to prevent casual users increasing their limit. However the most likely use case would be ad-hoc query tools that don't have the ability to do SET anyway. Hmm - what do you think? - why the calculations are done as double? + if (temporary_files_size / 1024.0 (double)work_disk) I originally coded this with the idea that the guc would (or could) be a double - to allow for seriously big limits in data warehousing situations etc. But subsequent discussion led to that being discarded. However work_disk can go to INT_MAX * 1024 bytes so I need to make sure that whatever datatype I use can handle that - double seemed sufficient. - 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_schedule @@ -127,3 +127,4 @@ test: largeobject test: with test: xml test: stats +test: resource Bugger - I think I forgot to 'git add' em before doing the diff. I can sense a v5 coming on. -- 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
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 memory than this, then you have to execute the plan some other way. This new parameter is not a query planner paramater AIUI - its job is to KILL things if they exceed the limit. In that sense it's more like statement_timeout. I can imagine us wanting more parameters like this too. Kill the query if it... ...takes too long (statement_timeout) ...uses too much temporary file space (the current patch) ...uses too much CPU time ...uses too much RAM ...generates too much disk I/O ...has too high an estimated cost ...others? So I'm not sure work_disk is a great name. Actually, work_mem is already not a great name even for what it is, but at any rate I think this is something different. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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
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 size, and would avoid (at least for the near term) the need to make the parameter a float. 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] Re: patch review : Add ability to constrain backend temporary file space
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 parameter; it says, if you're going to need more memory than this, then you have to execute the plan some other way. This new parameter is not a query planner paramater AIUI - its job is to KILL things if they exceed the limit. In that sense it's more like statement_timeout. I can imagine us wanting more parameters like this too. Kill the query if it... ...takes too long (statement_timeout) ...uses too much temporary file space (the current patch) ...uses too much CPU time ...uses too much RAM ...generates too much disk I/O ...has too high an estimated cost ...others? you're sorting limits for 'executor' and limits for 'planner': uses too much CPU time VS has too high an estimated cost. (backend)_work_(disk|mem) looks good also for the 'has too high an estimated cost' series: limiter at the planner level should allow planner to change its strategy, I think... But probably not something to consider too much right now. So I'm not sure work_disk is a great name. Actually, work_mem is already not a great name even for what it is, but at any rate I think this is something different. 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). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- 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
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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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
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 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. I agree, and I like your name suggestion (and also agree with Robert that 'work_mem' was not such a great name). I'd be quite happy to use MB (checks guc.h) but looks like we don't have a GUC_UNIT_MB, not sure if adding it would be an issue (suggests on a suitable mask if people think it is a great idea). Cheers Mark -- 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
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_schedule @@ -127,3 +127,4 @@ test: largeobject test: with test: xml test: stats +test: resource Corrected v4 patch with the test files, for completeness. Note that discussion has moved on and there will be a v5 :-) temp-files-v4.1.patch.gz Description: GNU Zip compressed 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] Re: patch review : Add ability to constrain backend temporary file space
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 ; QUERY PLAN - Sort (cost=10260.02..10495.82 rows=94320 width=4) (actual time=364.373..518.940 rows=10 loops=1) Sort Key: generate_series Sort Method: external merge Disk: 1352kB Buffers: local hit=393, temp read=249 written=249 - Seq Scan on foo (cost=0.00..1336.20 rows=94320 width=4) (actual time=0.025..138.754 rows=10 loops=1) Buffers: local hit=393 Total runtime: 642.874 ms (7 rows) cedric=# set max_temp_files_size to 1900; SET cedric=# explain (analyze,buffers) select * from foo order by 1 desc ; ERROR: aborting due to exceeding max temp files size STATEMENT: explain (analyze,buffers) select * from foo order by 1 desc ; ERROR: aborting due to exceeding max temp files size Do you have some testing method I can apply to track that without explain (analyze, buffers) before going to low-level monitoring ? We're looking at this... Arg - I was being dense. FileWrite can write *anywhere* in the file, so need to be smart about whether to add to the total filesize or not. I'll update the Commitfest page as soon as it sends me my password reset mail... Cheers Mark temp-files-v4.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
