public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Control temporary files removal after crash 26+ messages / 3 participants [nested] [flat]
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* [PATCH] Control temporary files removal after crash @ 2020-05-25 03:08 Euler Taveira <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Euler Taveira @ 2020-05-25 03:08 UTC (permalink / raw) A new GUC remove_temp_files_after_crash controls whether temporary files are removed after a crash. Successive crashes could result in useless storage usage until service is restarted. It could be the case on host with inadequate resources. This manual intervention for some environments is not desirable. This GUC is marked as SIGHUP hence you don't have to interrupt the service to change it. The current behavior introduces a backward incompatibility (minor one) which means Postgres will reclaim temporary file space after a crash. The main reason is that we favor service continuity over debugging. --- doc/src/sgml/config.sgml | 18 ++ src/backend/postmaster/postmaster.c | 5 + src/backend/storage/file/fd.c | 12 +- src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/postmaster/postmaster.h | 1 + src/test/recovery/t/022_crash_temp_files.pl | 194 ++++++++++++++++++ 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/test/recovery/t/022_crash_temp_files.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 967de73596..1b102960d6 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9665,6 +9665,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' </listitem> </varlistentry> + <varlistentry id="guc-remove-temp-files-after-crash" xreflabel="remove_temp_files_after_crash"> + <term><varname>remove_temp_files_after_crash</varname> (<type>boolean</type>) + <indexterm> + <primary><varname>remove_temp_files_after_crash</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + When set to on, <productname>PostgreSQL</productname> will automatically + remove temporary files after a backend crash. When disabled, the temporary + files are retained after a crash, which may be useful in some circumstances + (e.g. during debugging). It may however result in accumulation of many + useless files, or possibly even running out of disk space. + It defaults to <literal>on</literal>. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-data-sync-retry" xreflabel="data_sync_retry"> <term><varname>data_sync_retry</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index edab95a19e..2338892c33 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -242,6 +242,7 @@ bool Db_user_namespace = false; bool enable_bonjour = false; char *bonjour_name; bool restart_after_crash = true; +bool remove_temp_files_after_crash = true; /* PIDs of special child processes; 0 when not running */ static pid_t StartupPID = 0, @@ -3975,6 +3976,10 @@ PostmasterStateMachine(void) ereport(LOG, (errmsg("all server processes terminated; reinitializing"))); + /* remove leftover temporary files after a crash */ + if (remove_temp_files_after_crash) + RemovePgTempFiles(); + /* allow background workers to immediately restart */ ResetBackgroundWorkerCrashTimes(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index b58502837a..dfc3c2fe3e 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -3024,11 +3024,13 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * remove any leftover files created by OpenTemporaryFile and any leftover * temporary relation files created by mdcreate. * - * NOTE: we could, but don't, call this during a post-backend-crash restart - * cycle. The argument for not doing it is that someone might want to examine - * the temp files for debugging purposes. This does however mean that - * OpenTemporaryFile had better allow for collision with an existing temp - * file name. + * During post-backend-crash restart cycle, this routine could be called if + * remove_temp_files_after_crash GUC is enabled. Multiple crashes while + * queries are using temp files could result in useless storage usage that can + * only be reclaimed by a service restart. The argument against enabling it is + * that someone might want to examine the temp files for debugging purposes. + * This does however mean that OpenTemporaryFile had better allow for + * collision with an existing temp file name. * * NOTE: this function and its subroutines generally report syscall failures * with ereport(LOG) and keep going. Removing temp files is not so critical diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 3fd1a5fbe2..64ac48b2e5 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1361,6 +1361,15 @@ static struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, + { + {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS, + gettext_noop("Remove temporary files after backend crash."), + NULL + }, + &remove_temp_files_after_crash, + true, + NULL, NULL, NULL + }, { {"log_duration", PGC_SUSET, LOGGING_WHAT, diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index ee06528bb0..25c1b89ffc 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -759,6 +759,8 @@ #exit_on_error = off # terminate session on any error? #restart_after_crash = on # reinitialize after backend crash? +#remove_temp_files_after_crash = on # remove temporary files after +# # backend crash? #data_sync_retry = off # retry or panic on failure to fsync # data? # (change requires restart) diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h index cfa59c4dc0..0efdd7c232 100644 --- a/src/include/postmaster/postmaster.h +++ b/src/include/postmaster/postmaster.h @@ -29,6 +29,7 @@ extern bool log_hostname; extern bool enable_bonjour; extern char *bonjour_name; extern bool restart_after_crash; +extern bool remove_temp_files_after_crash; #ifdef WIN32 extern HANDLE PostmasterHandle; diff --git a/src/test/recovery/t/022_crash_temp_files.pl b/src/test/recovery/t/022_crash_temp_files.pl new file mode 100644 index 0000000000..45a70a368e --- /dev/null +++ b/src/test/recovery/t/022_crash_temp_files.pl @@ -0,0 +1,194 @@ +# +# Test cleanup of temporary files after a crash. +# +# This is a description that I will add later. +# +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More; +use Config; +use Time::HiRes qw(usleep); + +plan tests => 9; + + +# To avoid hanging while expecting some specific input from a psql +# instance being driven by us, add a timeout high enough that it +# should never trigger even on very slow machines, unless something +# is really wrong. +my $psql_timeout = IPC::Run::timer(60); + +my $node = get_new_node('node_crash'); +$node->init(); +$node->start(); + +# By default, PostgresNode doesn't restart after crash +# Reduce work_mem to generate temporary file with a few number of rows +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = on; + ALTER SYSTEM SET log_connections = 1; + ALTER SYSTEM SET work_mem = '64kB'; + ALTER SYSTEM SET restart_after_crash = on; + SELECT pg_reload_conf();]); + +# create table, insert rows +$node->safe_psql( + 'postgres', + q[CREATE TABLE tab_crash (a text); + INSERT INTO tab_crash (a) SELECT gen_random_uuid() FROM generate_series(1, 500);]); + +# +# Test cleanup temporary files after crash +# + +# Run psql, keeping session alive, so we have an alive backend to kill. +my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +my $killme = IPC::Run::start( + [ + 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', + $node->connstr('postgres') + ], + '<', + \$killme_stdin, + '>', + \$killme_stdout, + '2>', + \$killme_stderr, + $psql_timeout); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +my $pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +my $ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'no temporary files'); + +# +# Test old behavior (doesn't cleanup temporary file after crash) +# +$node->safe_psql( + 'postgres', + q[ALTER SYSTEM SET remove_temp_files_after_crash = off; + SELECT pg_reload_conf();]); + +# Restart psql session +($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', ''); +$killme->run(); + +# Get backend pid +$killme_stdin .= q[ +SELECT pg_backend_pid(); +]; +ok(pump_until($killme, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid for SIGKILL'); +$pid = $killme_stdout; +chomp($pid); +$killme_stdout = ''; +$killme_stderr = ''; + +# Run the query that generates a temporary file and that will be killed before +# it finishes. Since the query that generates the temporary file does not +# return before the connection is killed, use a SELECT before to trigger +# pump_until. +$killme_stdin .= q[ +BEGIN; +SELECT $$in-progress-before-sigkill$$; +WITH foo AS (SELECT a FROM tab_crash ORDER BY a) SELECT a, $$in-progress-before-sigkill$$, pg_sleep(1) FROM foo; +]; +ok(pump_until($killme, \$killme_stdout, qr/in-progress-before-sigkill/m), + 'select in-progress-before-sigkill'); +$killme_stdout = ''; +$killme_stderr = ''; + +# Kill with SIGKILL +$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid); +is($ret, 0, 'killed process with KILL'); + +# Close psql session +$killme->finish; + +# Wait till server restarts +$node->poll_query_until('postgres', 'SELECT 1', '1'); + +# Check for temporary files -- should be there +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(1), 'one temporary file'); + +# Restart should remove the temporary files +$node->restart(); + +# Check the temporary files -- should be gone +is($node->safe_psql( + 'postgres', + 'SELECT COUNT(1) FROM pg_ls_dir($$base/pgsql_tmp$$)'), + qq(0), 'temporary file was removed'); + +$node->stop(); + +# Pump until string is matched, or timeout occurs +sub pump_until +{ + my ($proc, $stream, $untl) = @_; + $proc->pump_nb(); + while (1) + { + last if $$stream =~ /$untl/; + if ($psql_timeout->is_expired) + { + diag("aborting wait: program timed out"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + if (not $proc->pumpable()) + { + diag("aborting wait: program died"); + diag("stream contents: >>", $$stream, "<<"); + diag("pattern searched for: ", $untl); + + return 0; + } + $proc->pump(); + } + return 1; +} -- 2.29.2 --------------002C097FE66DA941AEB9F7C8-- ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Collecting statistics about contents of JSONB columns @ 2022-01-01 15:33 Zhihong Yu <[email protected]> 2022-01-06 19:12 ` Re: Collecting statistics about contents of JSONB columns Tomas Vondra <[email protected]> 0 siblings, 1 reply; 26+ messages in thread From: Zhihong Yu @ 2022-01-01 15:33 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Nikita Glukhov <[email protected]>; Mahendra Thalor <[email protected]>; Oleg Bartunov <[email protected]> On Fri, Dec 31, 2021 at 2:07 PM Tomas Vondra <[email protected]> wrote: > Hi, > > One of the complaints I sometimes hear from users and customers using > Postgres to store JSON documents (as JSONB type, of course) is that the > selectivity estimates are often pretty poor. > > Currently we only really have MCV and histograms with whole documents, > and we can deduce some stats from that. But that is somewhat bogus > because there's only ~100 documents in either MCV/histogram (with the > default statistics target). And moreover we discard all "oversized" > values (over 1kB) before even calculating those stats, which makes it > even less representative. > > A couple weeks ago I started playing with this, and I experimented with > improving extended statistics in this direction. After a while I noticed > a forgotten development branch from 2016 which tried to do this by > adding a custom typanalyze function, which seemed like a more natural > idea (because it's really a statistics for a single column). > > But then I went to pgconf NYC in early December, and I spoke to Oleg > about various JSON-related things, and he mentioned they've been working > on this topic some time ago too, but did not have time to pursue it. So > he pointed me to a branch [1] developed by Nikita Glukhov. > > I like Nikita's branch because it solved a couple architectural issues > that I've been aware of, but only solved them in a rather hackish way. > > I had a discussion with Nikita about his approach what can we do to move > it forward. He's focusing on other JSON stuff, but he's OK with me > taking over and moving it forward. So here we go ... > > Nikita rebased his branch recently, I've kept improving it in various > (mostly a lot of comments and docs, and some minor fixes and tweaks). > I've pushed my version with a couple extra commits in [2], but you can > ignore that except if you want to see what I added/changed. > > Attached is a couple patches adding adding the main part of the feature. > There's a couple more commits in the github repositories, adding more > advanced features - I'll briefly explain those later, but I'm not > including them here because those are optional features and it'd be > distracting to include them here. > > There are 6 patches in the series, but the magic mostly happens in parts > 0001 and 0006. The other parts are mostly just adding infrastructure, > which may be a sizeable amount of code, but the changes are fairly > simple and obvious. So let's focus on 0001 and 0006. > > To add JSON statistics we need to do two basic things - we need to build > the statistics and then we need to allow using them while estimating > conditions. > > > 1) building stats > > Let's talk about building the stats first. The patch does one of the > things I experimented with - 0006 adds a jsonb_typanalyze function, and > it associates it with the data type. The function extracts paths and > values from the JSONB document, builds the statistics, and then stores > the result in pg_statistic as a new stakind. > > I've been planning to store the stats in pg_statistic too, but I've been > considering to use a custom data type. The patch does something far more > elegant - it simply uses stavalues to store an array of JSONB documents, > each describing stats for one path extracted from the sampled documents. > > One (very simple) element of the array might look like this: > > {"freq": 1, > "json": { > "mcv": { > "values": [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], > "numbers": [0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1, 0.1]}, > "width": 19, > "distinct": 10, > "nullfrac": 0, > "correlation": 0.10449}, > "path": "$.\"a\"", > "freq_null": 0, "freq_array": 0, "freq_object": 0, > "freq_string": 0, "freq_boolean": 0, "freq_numeric": 0} > > In this case there's only a MCV list (represented by two arrays, just > like in pg_statistic), but there might be another part with a histogram. > There's also the other columns we'd expect to see in pg_statistic. > > In principle, we need pg_statistic for each path we extract from the > JSON documents and decide it's interesting enough for estimation. There > are probably other ways to serialize/represent this, but I find using > JSONB for this pretty convenient because we need to deal with a mix of > data types (for the same path), and other JSON specific stuff. Storing > that in Postgres arrays would be problematic. > > I'm sure there's plenty open questions - for example I think we'll need > some logic to decide which paths to keep, otherwise the statistics can > get quite big, if we're dealing with large / variable documents. We're > already doing something similar for MCV lists. > > One of Nikita's patches not included in this thread allow "selective" > statistics, where you can define in advance a "filter" restricting which > parts are considered interesting by ANALYZE. That's interesting, but I > think we need some simple MCV-like heuristics first anyway. > > Another open question is how deep the stats should be. Imagine documents > like this: > > {"a" : {"b" : {"c" : {"d" : ...}}}} > > The current patch build stats for all possible paths: > > "a" > "a.b" > "a.b.c" > "a.b.c.d" > > and of course many of the paths will often have JSONB documents as > values, not simple scalar values. I wonder if we should limit the depth > somehow, and maybe build stats only for scalar values. > > > 2) applying the statistics > > One of the problems is how to actually use the statistics. For @> > operator it's simple enough, because it's (jsonb @> jsonb) so we have > direct access to the stats. But often the conditions look like this: > > jsonb_column ->> 'key' = 'value' > > so the condition is actually on an expression, not on the JSONB column > directly. My solutions were pretty ugly hacks, but Nikita had a neat > idea - we can define a custom procedure for each operator, which is > responsible for "calculating" the statistics for the expression. > > So in this case "->>" will have such "oprstat" procedure, which fetches > stats for the JSONB column, extracts stats for the "key" path. And then > we can use that for estimation of the (text = text) condition. > > This is what 0001 does, pretty much. We simply look for expression stats > provided by an index, extended statistics, and then - if oprstat is > defined for the operator - we try to derive the stats. > > This opens other interesting opportunities for the future - one of the > parts adds oprstat for basic arithmetic operators, which allows deducing > statistics for expressions like (a+10) from statistics on column (a). > > Which seems like a neat feature on it's own, but it also interacts with > the extended statistics in somewhat non-obvious ways (especially when > estimating GROUP BY cardinalities). > > Of course, there's a limit of what we can reasonably estimate - for > example, there may be statistical dependencies between paths, and this > patch does not even attempt to deal with that. In a way, this is similar > to correlation between columns, except that here we have a dynamic set > of columns, which makes it much much harder. We'd need something like > extended stats on steroids, pretty much. > > > I'm sure I've forgotten various important bits - many of them are > mentioned or explained in comments, but I'm sure others are not. And I'd > bet there are things I forgot about entirely or got wrong. So feel free > to ask. > > > In any case, I think this seems like a good first step to improve our > estimates for JSOB columns. > > regards > > > [1] https://github.com/postgrespro/postgres/tree/jsonb_stats > > [2] https://github.com/tvondra/postgres/tree/jsonb_stats_rework > > -- > Tomas Vondra > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company Hi, For patch 1: + List *statisticsName = NIL; /* optional stats estimat. procedure */ I think if the variable is named estimatorName (or something similar), it would be easier for people to grasp its purpose. + /* XXX perhaps full "statistics" wording would be better */ + else if (strcmp(defel->defname, "stats") == 0) I would recommend (stats sounds too general): + else if (strcmp(defel->defname, "statsestimator") == 0) + statisticsOid = ValidateStatisticsEstimator(statisticsName); statisticsOid -> statsEstimatorOid For get_oprstat(): + } + else + return (RegProcedure) InvalidOid; keyword else is not needed (considering the return statement in if block). For patch 06: + /* FIXME Could be before the memset, I guess? Checking vardata->statsTuple. */ + if (!data->statsTuple) + return false; I would agree the check can be lifted above the memset call. + * XXX This does not really extract any stats, it merely allocates the struct? + */ +static JsonPathStats +jsonPathStatsGetSpecialStats(JsonPathStats pstats, JsonPathStatsType type) As comments says, I think allocJsonPathStats() would be better name for the func. + * XXX Why doesn't this do jsonPathStatsGetTypeFreq check similar to what + * jsonPathStatsGetLengthStats does? I think `jsonPathStatsGetTypeFreq(pstats, jbvArray, 0.0) <= 0.0` check should be added for jsonPathStatsGetArrayLengthStats(). To be continued ... ^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Collecting statistics about contents of JSONB columns 2022-01-01 15:33 Re: Collecting statistics about contents of JSONB columns Zhihong Yu <[email protected]> @ 2022-01-06 19:12 ` Tomas Vondra <[email protected]> 0 siblings, 0 replies; 26+ messages in thread From: Tomas Vondra @ 2022-01-06 19:12 UTC (permalink / raw) To: Zhihong Yu <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Nikita Glukhov <[email protected]>; Mahendra Thalor <[email protected]>; Oleg Bartunov <[email protected]> On 1/1/22 16:33, Zhihong Yu wrote: > Hi, > For patch 1: > > + List *statisticsName = NIL; /* optional stats estimat. > procedure */ > > I think if the variable is named estimatorName (or something similar), > it would be easier for people to grasp its purpose. > I agree "statisticsName" might be too generic or confusing, but I'm not sure "estimator" is an improvement. Because this is not an "estimator" (in the sense of estimating selectivity). It "transforms" statistics to match the expression. > + /* XXX perhaps full "statistics" wording would be better */ > + else if (strcmp(defel->defname, "stats") == 0) > > I would recommend (stats sounds too general): > > + else if (strcmp(defel->defname, "statsestimator") == 0) > > + statisticsOid = ValidateStatisticsEstimator(statisticsName); > > statisticsOid -> statsEstimatorOid > Same issue with the "estimator" bit :-( > For get_oprstat(): > > + } > + else > + return (RegProcedure) InvalidOid; > > keyword else is not needed (considering the return statement in if block). > True, but this is how the other get_ functions in lsyscache.c do it. Like get_oprjoin(). > For patch 06: > > + /* FIXME Could be before the memset, I guess? Checking > vardata->statsTuple. */ > + if (!data->statsTuple) > + return false; > > I would agree the check can be lifted above the memset call. > OK. > + * XXX This does not really extract any stats, it merely allocates the > struct? > + */ > +static JsonPathStats > +jsonPathStatsGetSpecialStats(JsonPathStats pstats, JsonPathStatsType type) > > As comments says, I think allocJsonPathStats() would be better name for > the func. > > + * XXX Why doesn't this do jsonPathStatsGetTypeFreq check similar to what > + * jsonPathStatsGetLengthStats does? > > I think `jsonPathStatsGetTypeFreq(pstats, jbvArray, 0.0) <= 0.0` check > should be added for jsonPathStatsGetArrayLengthStats(). > > To be continued ... OK. I'll see if Nikita has some ideas about the naming changes. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2022-01-06 19:12 UTC | newest] Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2020-05-25 03:08 [PATCH] Control temporary files removal after crash Euler Taveira <[email protected]> 2022-01-01 15:33 Re: Collecting statistics about contents of JSONB columns Zhihong Yu <[email protected]> 2022-01-06 19:12 ` Re: Collecting statistics about contents of JSONB columns Tomas Vondra <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox