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

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 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.
>
> 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.
>
> * 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.
>
> * temporary_files_size, I think it is better to have a number of pages
> à la postgresql than a kilobyte size
>
> * max_temp_files_size, I'll prefer an approach like shared_buffers
> GUC: you can use pages, or KB, MB, ...
>
>
> 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 ?
>
> 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.
>
> Patch is not large and easy to read.
> I like the idea and it sounds useful.
>

Hi Cédric,

Thanks for reviewing!

The diff format is odd - I'm sure I told git to do a context diff,
however running 'file' on the v2 patch results it it saying 'HTML
document'... hmm, I'll check more carefully for the next one I do.

Yeah, I agree about explicitly mentioning how it does not constraint
temp tables.

Hmm, test - good idea - I'll look into it.

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 I used bytes for the internal variable in fd.c to
limit the number of arithmetic operations while comparing.

For testing I set 'log_temp_files = 0' in postgresql.conf and the
numbers seemed to agree exactly - I didn't think to use EXPLAIN +
buffers... not sure why they would disagree. Have a go with
log_temp_files set and see what you find!

I like yhour suggesting for the name. Given that 'work_mem' is per
backend, I'm leaning towards the idea of 'work_disk' being sufficiently
descriptive.

Thanks again, and I'll come up with a v3 patch.

Mark