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 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
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">
+ <title>Disk</title>
+ <variablelist>
+
+ <varlistentry id="guc-temp-file-limit" xreflabel="temp_file_limit">
+ <term><varname>temp_file_limit</varname> (<type>integer</type>)</term>
+ <indexterm>
+ <primary><varname>temp_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 <emphasis>not</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">
<title>Kernel 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 increase the file size, add it to the total, then check it is
+ * not too big
+ */
+ if (VfdCache[file].seekPos >= VfdCache[file].fileSize )
+ {
+ temporary_files_size += (double)(VfdCache[file].seekPos - VfdCache[file].fileSize);
+ VfdCache[file].fileSize = VfdCache[file].seekPos;
+
+ if (temporary_files_size / 1024.0 > (double)temp_file_limit)
+ ereport(ERROR,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg("aborting due to exceeding temp file limit")));
+ }
+ }
+ }
else
{
/*
@@ -1887,6 +1921,7 @@ CleanupTempFiles(bool isProcExit)
}
have_pending_fd_cleanup = false;
+ temporary_files_size = 0.0;
}
/* Clean up "allocated" stdio files and dirs. */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 92391ed..8ba6dce 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -423,6 +423,7 @@ int log_min_messages = WARNING;
int client_min_messages = NOTICE;
int log_min_duration_statement = -1;
int log_temp_files = -1;
+int temp_file_limit = -1;
int trace_recovery_messages = LOG;
int num_temp_buffers = 1024;
@@ -535,6 +536,8 @@ const char *const config_group_names[] =
gettext_noop("Resource Usage"),
/* RESOURCES_MEM */
gettext_noop("Resource Usage / Memory"),
+ /* RESOURCES_DISK */
+ gettext_noop("Resource Usage / Disk"),
/* RESOURCES_KERNEL */
gettext_noop("Resource Usage / Kernel Resources"),
/* RESOURCES_VACUUM_DELAY */
@@ -2185,6 +2188,16 @@ static struct config_int ConfigureNamesInt[] =
RELSEG_SIZE, RELSEG_SIZE, RELSEG_SIZE,
NULL, NULL, NULL
},
+ {
+ {"temp_file_limit", PGC_SUSET, RESOURCES_DISK,
+ gettext_noop("Sets the maximum size of all temp files used by each session."),
+ gettext_noop("-1 turns this feature off."),
+ GUC_UNIT_KB
+ },
+ &temp_file_limit,
+ -1, -1, MAX_KILOBYTES,
+ NULL, NULL, NULL
+ },
{
{"wal_block_size", PGC_INTERNAL, PRESET_OPTIONS,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 655dad4..cc468c7 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -118,6 +118,7 @@
#work_mem = 1MB # min 64kB
#maintenance_work_mem = 16MB # min 1MB
#max_stack_depth = 2MB # min 100kB
+#temp_file_limit = -1 # limit backend temp file space
# - Kernel Resource Usage -
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index ee52cd7..35321ee 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -208,6 +208,7 @@ extern int log_min_messages;
extern int client_min_messages;
extern int log_min_duration_statement;
extern int log_temp_files;
+extern int temp_file_limit;
extern int num_temp_buffers;
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index a1ca012..3d9ab37 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -59,6 +59,7 @@ enum config_group
CONN_AUTH_SECURITY,
RESOURCES,
RESOURCES_MEM,
+ RESOURCES_DISK,
RESOURCES_KERNEL,
RESOURCES_VACUUM_DELAY,
RESOURCES_BGWRITER,
diff --git a/src/test/regress/expected/resource.out b/src/test/regress/expected/resource.out
new file mode 100644
index 0000000..571fa8f
--- /dev/null
+++ b/src/test/regress/expected/resource.out
@@ -0,0 +1,18 @@
+--
+-- RESOURCE
+-- Test resource management capabilities
+--
+-- temp_file_limit
+CREATE TEMPORARY TABLE resourcetemp1 AS SELECT generate_series(1,100000);
+-- Should succeed
+SET temp_file_limit = 10000;
+SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
+ count
+--------
+ 100000
+(1 row)
+
+-- Should fail
+SET temp_file_limit = 1000;
+SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
+ERROR: aborting due to exceeding temp file limit
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
diff --git a/src/test/regress/sql/resource.sql b/src/test/regress/sql/resource.sql
new file mode 100644
index 0000000..c593a58
--- /dev/null
+++ b/src/test/regress/sql/resource.sql
@@ -0,0 +1,17 @@
+--
+-- RESOURCE
+-- Test resource management capabilities
+--
+
+-- temp_file_limit
+CREATE TEMPORARY TABLE resourcetemp1 AS SELECT generate_series(1,100000);
+
+-- Should succeed
+SET temp_file_limit = 10000;
+
+SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
+
+-- Should fail
+SET temp_file_limit = 1000;
+
+SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index 794aef4..19dc440
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** SET ENABLE_SEQSCAN TO OFF;
*** 1025,1030 ****
--- 1025,1067 ----
</variablelist>
</sect2>
+ <sect2 id="runtime-config-resource-disk">
+ <title>Disk</title>
+ <variablelist>
+
+ <varlistentry id="guc-temp-file-limit" xreflabel="temp_file_limit">
+ <term><varname>temp_file_limit</varname> (<type>integer</type>)</term>
+ <indexterm>
+ <primary><varname>temp_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 <emphasis>not</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">
<title>Kernel Resource Usage</title>
<variablelist>
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
new file mode 100644
index 820e6db..5c00889
*** a/src/backend/storage/file/fd.c
--- b/src/backend/storage/file/fd.c
*************** static int max_safe_fds = 32; /* default
*** 131,136 ****
--- 131,141 ----
/* 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 */
*************** typedef struct vfd
*** 140,145 ****
--- 145,151 ----
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 */
*************** PathNameOpenFile(FileName fileName, int
*** 887,892 ****
--- 893,899 ----
vfdP->fileFlags = fileFlags & ~(O_CREAT | O_TRUNC | O_EXCL);
vfdP->fileMode = fileMode;
vfdP->seekPos = 0;
+ vfdP->fileSize = 0;
vfdP->fdstate = 0x0;
vfdP->resowner = NULL;
*************** FileClose(File file)
*** 1123,1128 ****
--- 1130,1142 ----
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 */
*************** retry:
*** 1251,1257 ****
--- 1265,1291 ----
errno = ENOSPC;
if (returnCode >= 0)
+ {
VfdCache[file].seekPos += returnCode;
+
+ if (temp_file_limit >= 0 && VfdCache[file].fdstate & FD_TEMPORARY)
+ {
+ /*
+ * if we increase the file size, add it to the total, then check it is
+ * not too big
+ */
+ if (VfdCache[file].seekPos >= VfdCache[file].fileSize )
+ {
+ temporary_files_size += (double)(VfdCache[file].seekPos - VfdCache[file].fileSize);
+ VfdCache[file].fileSize = VfdCache[file].seekPos;
+
+ if (temporary_files_size / 1024.0 > (double)temp_file_limit)
+ ereport(ERROR,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg("aborting due to exceeding temp file limit")));
+ }
+ }
+ }
else
{
/*
*************** CleanupTempFiles(bool isProcExit)
*** 1887,1892 ****
--- 1921,1927 ----
}
have_pending_fd_cleanup = false;
+ temporary_files_size = 0.0;
}
/* Clean up "allocated" stdio files and dirs. */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 92391ed..8ba6dce
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** int log_min_messages = WARNING;
*** 423,428 ****
--- 423,429 ----
int client_min_messages = NOTICE;
int log_min_duration_statement = -1;
int log_temp_files = -1;
+ int temp_file_limit = -1;
int trace_recovery_messages = LOG;
int num_temp_buffers = 1024;
*************** const char *const config_group_names[] =
*** 535,540 ****
--- 536,543 ----
gettext_noop("Resource Usage"),
/* RESOURCES_MEM */
gettext_noop("Resource Usage / Memory"),
+ /* RESOURCES_DISK */
+ gettext_noop("Resource Usage / Disk"),
/* RESOURCES_KERNEL */
gettext_noop("Resource Usage / Kernel Resources"),
/* RESOURCES_VACUUM_DELAY */
*************** static struct config_int ConfigureNamesI
*** 2185,2190 ****
--- 2188,2203 ----
RELSEG_SIZE, RELSEG_SIZE, RELSEG_SIZE,
NULL, NULL, NULL
},
+ {
+ {"temp_file_limit", PGC_SUSET, RESOURCES_DISK,
+ gettext_noop("Sets the maximum size of all temp files used by each session."),
+ gettext_noop("-1 turns this feature off."),
+ GUC_UNIT_KB
+ },
+ &temp_file_limit,
+ -1, -1, MAX_KILOBYTES,
+ NULL, NULL, NULL
+ },
{
{"wal_block_size", PGC_INTERNAL, PRESET_OPTIONS,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
new file mode 100644
index 655dad4..cc468c7
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 118,123 ****
--- 118,124 ----
#work_mem = 1MB # min 64kB
#maintenance_work_mem = 16MB # min 1MB
#max_stack_depth = 2MB # min 100kB
+ #temp_file_limit = -1 # limit backend temp file space
# - Kernel Resource Usage -
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
new file mode 100644
index ee52cd7..35321ee
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** extern int log_min_messages;
*** 208,213 ****
--- 208,214 ----
extern int client_min_messages;
extern int log_min_duration_statement;
extern int log_temp_files;
+ extern int temp_file_limit;
extern int num_temp_buffers;
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
new file mode 100644
index a1ca012..3d9ab37
*** a/src/include/utils/guc_tables.h
--- b/src/include/utils/guc_tables.h
*************** enum config_group
*** 59,64 ****
--- 59,65 ----
CONN_AUTH_SECURITY,
RESOURCES,
RESOURCES_MEM,
+ RESOURCES_DISK,
RESOURCES_KERNEL,
RESOURCES_VACUUM_DELAY,
RESOURCES_BGWRITER,
diff --git a/src/test/regress/expected/resource.out b/src/test/regress/expected/resource.out
new file mode 100644
index ...571fa8f
*** a/src/test/regress/expected/resource.out
--- b/src/test/regress/expected/resource.out
***************
*** 0 ****
--- 1,18 ----
+ --
+ -- RESOURCE
+ -- Test resource management capabilities
+ --
+ -- temp_file_limit
+ CREATE TEMPORARY TABLE resourcetemp1 AS SELECT generate_series(1,100000);
+ -- Should succeed
+ SET temp_file_limit = 10000;
+ SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
+ count
+ --------
+ 100000
+ (1 row)
+
+ -- Should fail
+ SET temp_file_limit = 1000;
+ SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
+ ERROR: aborting due to exceeding temp file limit
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
new file mode 100644
index bb654f9..325cb3d
*** a/src/test/regress/serial_schedule
--- b/src/test/regress/serial_schedule
*************** test: largeobject
*** 127,129 ****
--- 127,130 ----
test: with
test: xml
test: stats
+ test: resource
diff --git a/src/test/regress/sql/resource.sql b/src/test/regress/sql/resource.sql
new file mode 100644
index ...c593a58
*** a/src/test/regress/sql/resource.sql
--- b/src/test/regress/sql/resource.sql
***************
*** 0 ****
--- 1,17 ----
+ --
+ -- RESOURCE
+ -- Test resource management capabilities
+ --
+
+ -- temp_file_limit
+ CREATE TEMPORARY TABLE resourcetemp1 AS SELECT generate_series(1,100000);
+
+ -- Should succeed
+ SET temp_file_limit = 10000;
+
+ SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
+
+ -- Should fail
+ SET temp_file_limit = 1000;
+
+ SELECT count(*) FROM (select * FROM resourcetemp1 ORDER BY 1) AS a;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
