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

Mark Kirkwood <mark(dot)kirkwood(at)catalyst(dot)net(dot)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