Date: Tue, 28 Jul 2020 20:37:24 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
	"Bossart, Nathan" <bossartn@amazon.com>
Subject: Re: shared tempfile was not removed on statement_timeout
Message-ID: <20200729013724.GM20393@telsasoft.com>
References: 
 <CA+hUKGJf5uPTPz-9FoxnaOfeu74Hi494HG8EyVAmPUatPjPL9A@mail.gmail.com>
 <CA+hUKGJStr-3B6qNnFEOpES8HHc3Wwe3wSrYYQJcQhHuTB9SdQ@mail.gmail.com>
 <20200721043258.GE5748@telsasoft.com>
 <CA+hUKGJPzYo-qz63tp9052i8FKehRyUw4djo8a474xvRCZ+vHQ@mail.gmail.com>
 <20200727103902.GM4286@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="St7VIuEGZ6dlpu13"
Content-Disposition: inline
In-Reply-To: <20200727103902.GM4286@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--St7VIuEGZ6dlpu13
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jul 27, 2020 at 05:39:02AM -0500, Justin Pryzby wrote:
> On Mon, Jul 27, 2020 at 08:00:46PM +1200, Thomas Munro wrote:
> > Why can't tuplesort_end do it?
> 
> Because then I think the parallel workers remove their own files, with tests
> failing like:
> 
> +ERROR:  could not open temporary file "0.0" from BufFile "0": No such file or directory
> 
> I look around a bit more and came up with this, which works, but I don't know
> enough to say if it's right.

I convinced myself this is right, since state->nParticipants==-1 for workers.
Only the leader should do the cleanup.

Added here:
https://commitfest.postgresql.org/29/2657/

-- 
Justin

--St7VIuEGZ6dlpu13
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v1-0001-explicity-remove-shared-fileset.patch"

From 3d9db664d86f9a1f4745c19d1fb620dae5f9e42a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 27 Jul 2020 05:50:34 -0500
Subject: [PATCH v1] explicity remove shared fileset..

..to avoid leaving it around if a timeout occurs *during* the path that would
normally release resources.

Discussion: https://www.postgresql.org/message-id/20191212180506.GR2082%40telsasoft.com
Analysis: Thomas Munro
---
 src/backend/utils/sort/tuplesort.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 3c49476483..b2b3ff789c 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1387,6 +1387,15 @@ tuplesort_free(Tuplesortstate *state)
 void
 tuplesort_end(Tuplesortstate *state)
 {
+	/*
+	 * Leader explicitly cleans up shared filesets.
+	 * This would normally be cleaned up by DestroyParallelContext(), but
+	 * doing it now avoids leaving files behind if an error/timeout happens
+	 * *during* cleanup.
+	 */
+	if (state->shared && state->shared->workersFinished == state->nParticipants)
+		SharedFileSetDeleteAll(&state->shared->fileset);
+
 	tuplesort_free(state);
 
 	/*
-- 
2.17.0


--St7VIuEGZ6dlpu13--