Subject: Re: cleanup temporary files after crash
To: Michael Paquier <michael@paquier.xyz>,
 Euler Taveira <euler.taveira@2ndquadrant.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Tom Lane <tgl@sss.pgh.pa.us>
References: 
 <CAH503wDKdYzyq7U-QJqGn=Gm6XmoK+6_6xTJ-Yn5WSvoHLY1Ww@mail.gmail.com>
 <20201028184252.5vpoe65kcfebbmmf@development>
 <CAH503wCg+yne7hKAAFFHsVPe+tPb-aF5VGaWG0ETqyB3GP=Q+g@mail.gmail.com>
 <20201101012555.GC3000@paquier.xyz>
From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Message-ID: <1982ee3c-54c3-280a-806a-fd28ad8f19e6@postgrespro.ru>
Date: Thu, 26 Nov 2020 11:48:46 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201101012555.GC3000@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------C09641F20688A9325582CA14"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------C09641F20688A9325582CA14
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 01.11.2020 04:25, Michael Paquier wrote:
> On Sat, Oct 31, 2020 at 09:01:15PM -0300, Euler Taveira wrote:
>> I thought about not providing a GUC at all or provide it in the developer
>> section. I've never heard someone saying that they use those temporary
>> files to investigate an issue. Regarding a crash, all information is already
>> available and temporary files don't provide extra details. This new
>> GUC is just to keep the previous behavior. I'm fine without the GUC, though.
> The original behavior is as old as 4a5f38c4, and last time we talked
> about that there were arguments about still keeping the existing
> behavior to not cleanup files during a restart-after-crash scenario
> for the sake of being useful just "in case".  I have never used that
> property myself, TBH, and I have seen much more cases of users caring
> about the data folder not facing an ENOSPC particularly if they don't
> use different partitions for pg_wal/ and the main data folder.
> --
> Michael

Thank you, Euler for submitting this.
+1 for the feature. One of the platforms we support uses temp files a 
lot and we faced the problem, while never actually used these orphan 
files for debugging purposes.

I also think that the GUC is not needed here. This 'feature' was 
internal from the very beginning, so users shouldn't care about 
preserving old behavior. Without the GUC the patch is very simple, 
please see attached version. I also omit the test, because I am not sure 
it will be stable given that the RemovePgTempFiles() allows the 
possibility of failure.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


--------------C09641F20688A9325582CA14
Content-Type: text/x-patch; charset=UTF-8;
 name="0001_remove_temporary_files_after_crash.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="0001_remove_temporary_files_after_crash.patch"

commit 417581b287f4060178595cd96465adc639639290
Author: anastasia <a.lubennikova@postgrespro.ru>
Date:   Thu Nov 26 11:45:05 2020 +0300

    Remove temporary files after a backend 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.
    
    The only reason we kept temp files was that someone might want to examine
    them for debugging purposes. With this patch we favor service continuity
    over debugging.
    
    Author: Euler Taveira
    Reviewer: Anastasia Lubennikova

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b7799ed1d24..a151250734f 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -3999,6 +3999,9 @@ PostmasterStateMachine(void)
 		ereport(LOG,
 				(errmsg("all server processes terminated; reinitializing")));
 
+		/* remove leftover temporary files after a 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 05abcf72d68..d30bfc28541 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2992,15 +2992,15 @@ 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.
+ * This is also called during a post-backend-crash restart cycle. The argument
+ * for not doing it is that crashes of queries using temp files can result in
+ * useless storage usage that can only be reclaimed by a service restart.
  *
  * NOTE: this function and its subroutines generally report syscall failures
  * with ereport(LOG) and keep going.  Removing temp files is not so critical
  * that we should fail to start the database when we can't do it.
+ * Because this routine is not strict, OpenTemporaryFile allows for collision
+ * with an existing temp file name.
  */
 void
 RemovePgTempFiles(void)

--------------C09641F20688A9325582CA14--