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) with 'git diff -c' so hopefully the format is ok now. I've added a paragraph about how temporary *table* storage is not constrained, only temp work files. I made the point that the *query* that creates a temp table will have its work files constrained too. I've added a test too.
The major visible change is that the guc has been renamed to 'work_disk', to gel better with the idea that it is the disk spill for 'work_mem'.
Code review ========= * in fd.c, I think that "temporary_files_size -= (double)vfdP->fileSize;" should be done later in the function once we have successfully unlink the file, not before.
Agreed, I've moved it.
* 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 increase the global counter.
The temp files are not relations so I'd have to call stat I guess. Now truncate/unlink can happen quite a lot (e.g hash with many files) and I wanted to avoid adding too many library calls to this code for performance reasons, so on balance I'm thinking it is gonna be more efficient to remember the size in the Vfd.
* temporary_files_size, I think it is better to have a number of pages à la postgresql than a kilobyte size
The temp file related stuff is worked on in bytes or kbytes in the fd.c and other code (e.g log_temp_files), so it seems more natural to stay in kbytes. Also work_disk is really only a spillover from work_mem (in kbytes) so seems logical to match its units.
* max_temp_files_size, I'll prefer an approach like shared_buffers GUC: you can use pages, or KB, MB, ...
We can use KB, MB, GB - just not pages, again like work_mem. These files are not relations so I'm not sure pages is an entirely appropriate unit for them.
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=100000 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=100000 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...
Architecture review ============== max_temp_files_size is used for the global space used per backend. Based on how work_mem work I expect something like "work_disk" to limit per file and maybe a backend_work_disk (and yes maybe a backend_work_mem ?!) per backend. So I propose to rename the current GUC to something like backend_work_disk.
Done - 'work_disk' it is to match 'work_mem'.
Patch is not large and easy to read. I like the idea and it sounds useful.
Great! Cheers Mark
temp-files-v3.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
