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

2011/6/17 Cédric Villemain <cedric(dot)villemain(dot)debian(at)gmail(dot)com>:
> 2011/6/17 Mark Kirkwood <mark(dot)kirkwood(at)catalyst(dot)net(dot)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 hurt....but 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