public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Control temporary files removal after crash 4+ 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; 4+ 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] 4+ messages in thread
* [PATCH] Expose port->authn_id to extensions and triggers @ 2022-02-24 00:15 Jacob Champion <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Jacob Champion @ 2022-02-24 00:15 UTC (permalink / raw) To: pgsql-hackers Hi all, Stephen pointed out [1] that the authenticated identity that's stored in MyProcPort can't be retrieved by extensions or triggers. Attached is a patch that provides both a C API and a SQL function for retrieving it. GetAuthenticatedIdentityString() is a mouthful but I wanted to differentiate it from the existing GetAuthenticatedUserId(); better names welcome. It only exists as an accessor because I wasn't sure if extensions outside of contrib were encouraged to rely on the internal layout of struct Port. (If they can, then that call can go away entirely.) Thanks, --Jacob [1] https://www.postgresql.org/message-id/CAOuzzgpFpuroNRabEvB9kST_TSyS2jFicBNoXvW7G2pZFixyBw%40mail.gma... Attachments: [text/x-patch] 0001-Add-APIs-to-retrieve-authn_id-from-C-and-SQL.patch (5.0K, ../../[email protected]/2-0001-Add-APIs-to-retrieve-authn_id-from-C-and-SQL.patch) download | inline diff: From 155a490d28cecf161d994e6d8824cbe967f4d469 Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Mon, 14 Feb 2022 08:10:53 -0800 Subject: [PATCH] Add APIs to retrieve authn_id from C and SQL The authn_id field in MyProcPort is currently only accessible to the backend itself. Add a getter in C, GetAuthenticatedIdentityString(), and a corresponding SQL function, session_authn_id(), to expose the field to extensions and triggers that may want to make use of it. --- src/backend/utils/adt/name.c | 14 +++++++++++++- src/backend/utils/init/miscinit.c | 14 ++++++++++++++ src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.dat | 3 +++ src/include/miscadmin.h | 1 + src/test/authentication/t/001_password.pl | 11 +++++++++++ 6 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c index e8bba3670c..d605b1e116 100644 --- a/src/backend/utils/adt/name.c +++ b/src/backend/utils/adt/name.c @@ -257,7 +257,7 @@ namestrcmp(Name name, const char *str) /* - * SQL-functions CURRENT_USER, SESSION_USER + * SQL-functions CURRENT_USER, SESSION_USER, SESSION_AUTHN_ID */ Datum current_user(PG_FUNCTION_ARGS) @@ -271,6 +271,18 @@ session_user(PG_FUNCTION_ARGS) PG_RETURN_DATUM(DirectFunctionCall1(namein, CStringGetDatum(GetUserNameFromId(GetSessionUserId(), false)))); } +Datum +session_authn_id(PG_FUNCTION_ARGS) +{ + char *authn_id; + + authn_id = GetAuthenticatedIdentityString(); + if (!authn_id) + PG_RETURN_NULL(); + + PG_RETURN_CSTRING(authn_id); +} + /* * SQL-functions CURRENT_SCHEMA, CURRENT_SCHEMAS diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index bdc77af719..9fea7f0be3 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -929,6 +929,20 @@ GetUserNameFromId(Oid roleid, bool noerr) return result; } +/* + * Get a palloc'd string representing the authenticated identity of the client + * (see documentation for Port's authn_id field). Returns NULL if the client has + * not been authenticated. + */ +char * +GetAuthenticatedIdentityString(void) +{ + if (!MyProcPort || !MyProcPort->authn_id) + return NULL; + + return pstrdup(MyProcPort->authn_id); +} + /*------------------------------------------------------------------------- * Interlock-file support diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index 1addb568ef..ca52e6889c 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 202202221 +#define CATALOG_VERSION_NO 202202231 #endif diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 7f1ee97f55..b76d357bee 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -1508,6 +1508,9 @@ { oid => '746', descr => 'session user name', proname => 'session_user', provolatile => 's', prorettype => 'name', proargtypes => '', prosrc => 'session_user' }, +{ oid => '9774', descr => 'session authenticated identity', + proname => 'session_authn_id', provolatile => 's', prorettype => 'cstring', + proargtypes => '', prosrc => 'session_authn_id' }, { oid => '744', proname => 'array_eq', prorettype => 'bool', diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 0abc3ad540..25731ce977 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -347,6 +347,7 @@ extern void SetDataDir(const char *dir); extern void ChangeToDataDir(void); extern char *GetUserNameFromId(Oid roleid, bool noerr); +extern char *GetAuthenticatedIdentityString(void); extern Oid GetUserId(void); extern Oid GetOuterUserId(void); extern Oid GetSessionUserId(void); diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl index 3e3079c824..2aa28ed547 100644 --- a/src/test/authentication/t/001_password.pl +++ b/src/test/authentication/t/001_password.pl @@ -82,6 +82,11 @@ test_role($node, 'scram_role', 'trust', 0, test_role($node, 'md5_role', 'trust', 0, log_unlike => [qr/connection authenticated:/]); +my $res = $node->safe_psql('postgres', + "SELECT session_authn_id() IS NULL;" +); +is($res, 't', "users with trust authentication have NULL authn_id"); + # For plain "password" method, all users should also be able to connect. reset_pg_hba($node, 'password'); test_role($node, 'scram_role', 'password', 0, @@ -91,6 +96,12 @@ test_role($node, 'md5_role', 'password', 0, log_like => [qr/connection authenticated: identity="md5_role" method=password/]); +$res = $node->safe_psql('postgres', + "SELECT session_authn_id();", + connstr => "user=md5_role" +); +is($res, 'md5_role', "users with md5 authentication have authn_id matching role name"); + # For "scram-sha-256" method, user "scram_role" should be able to connect. reset_pg_hba($node, 'scram-sha-256'); test_role( -- 2.25.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Expose port->authn_id to extensions and triggers @ 2022-02-24 11:39 Michael Paquier <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michael Paquier @ 2022-02-24 11:39 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: pgsql-hackers On Thu, Feb 24, 2022 at 12:15:40AM +0000, Jacob Champion wrote: > Stephen pointed out [1] that the authenticated identity that's stored > in MyProcPort can't be retrieved by extensions or triggers. Attached is > a patch that provides both a C API and a SQL function for retrieving > it. > > GetAuthenticatedIdentityString() is a mouthful but I wanted to > differentiate it from the existing GetAuthenticatedUserId(); better > names welcome. It only exists as an accessor because I wasn't sure if > extensions outside of contrib were encouraged to rely on the internal > layout of struct Port. (If they can, then that call can go away > entirely.) +char * +GetAuthenticatedIdentityString(void) +{ + if (!MyProcPort || !MyProcPort->authn_id) + return NULL; + + return pstrdup(MyProcPort->authn_id); I don't quite see the additional value that this API brings as MyProcPort is directly accessible, and contrib modules like postgres_fdw and sslinfo just use that to find the data of the current backend. Cannot a module like pgaudit, through the elog hook, do its work with what we have already in place? What's the use case for a given session to be able to report back only its own authn through SQL? I could still see a use case for that at a more global level with beentrys, but it looked like there was not much interest the last time I dropped this idea. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../YhduhD%[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Expose port->authn_id to extensions and triggers @ 2022-02-24 16:50 Jacob Champion <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Jacob Champion @ 2022-02-24 16:50 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: pgsql-hackers; [email protected] <[email protected]> On Thu, 2022-02-24 at 20:39 +0900, Michael Paquier wrote: > I don't quite see the additional value that this API brings as > MyProcPort is directly accessible, and contrib modules like > postgres_fdw and sslinfo just use that to find the data of the current > backend. Right -- I just didn't know if third-party modules were actually able to rely on the internal layout of struct Port. Is that guaranteed to remain constant for a major release line? If so, this new API is superfluous. > Cannot a module like pgaudit, through the elog hook, do its > work with what we have already in place? Given the above, I would hope so. Stephen mentioned that pgaudit only had access to the logged-in role, and when I proposed a miscadmin.h API he said that would help. CC'ing him to see what he meant; I don't know if pgaudit has additional constraints. > What's the use case for a given session to be able to report back > only its own authn through SQL? That's for triggers to be able to grab the current ID for logging/auditing. Is there a better way to expose this for that use case? > I could still see a use case for that at a more global level with > beentrys, but it looked like there was not much interest the last time > I dropped this idea. I agree that this would be useful to have in the stats. From my outside perspective, it seems like it's difficult to get strings of arbitrary length in there; is that accurate? Thanks, --Jacob ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-02-24 16:50 UTC | newest] Thread overview: 4+ 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]> 2022-02-24 00:15 [PATCH] Expose port->authn_id to extensions and triggers Jacob Champion <[email protected]> 2022-02-24 11:39 ` Re: [PATCH] Expose port->authn_id to extensions and triggers Michael Paquier <[email protected]> 2022-02-24 16:50 ` Re: [PATCH] Expose port->authn_id to extensions and triggers Jacob Champion <[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