Date: Thu, 22 Feb 2024 11:46:00 +0900 (JST)
Message-Id: <20240222.114600.1019580904613326727.horikyota.ntt@gmail.com>
To: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18354: Aborted transaction aborted during cleanup when
 temp_file_limit exceeded
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <18354-140864c09686b5a6@postgresql.org>
References: <18354-140864c09686b5a6@postgresql.org>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Thu_Feb_22_11_46_00_2024_016)--"
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20240222.114600.1019580904613326727.horikyota.ntt%40gmail.com>
Precedence: bulk

----Next_Part(Thu_Feb_22_11_46_00_2024_016)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

At Wed, 21 Feb 2024 12:00:01 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in 
> triggers two errors, a warning, and an assertion failure:
> ERROR:  temporary file size exceeds temp_file_limit (100kB)
> WARNING:  AbortTransaction while in ABORT state
> ERROR:  temporary file size exceeds temp_file_limit (100kB)
> server closed the connection unexpectedly
...
> For the second error:
> 2024-02-21 11:40:07.001 UTC|law|regression|65d5e116.13cfcb|ERROR:  temporary
> file size exceeds temp_file_limit (100kB)
> 2024-02-21 11:40:07.001 UTC|law|regression|65d5e116.13cfcb|BACKTRACE:  
> FileWrite at fd.c:2183:5
> BufFileDumpBuffer at buffile.c:537:18
> BufFileFlush at buffile.c:723:3
> BufFileClose at buffile.c:419:9
> tuplestore_end at tuplestore.c:459:5
> MemoryContextSwitchTo at palloc.h:142:23
>  (inlined by) PortalDrop at portalmem.c:587:3
> AtCleanup_Portals at portalmem.c:907:3

Therefore, BufFileClose should not flush the content during error
handling.  In the first place, tuplestore doesn't need to flush the
underlying files in _end and _clear. In this case, I would choose to
change the general behavior of tuplestore. The attached PoC patch
fixes the issue for me. It introduces a new "extended" function to
control flushing, avoiding the addition of an unnatural parameter to
BufFileClose. I suspect that it is usable in some other places, but I
haven't checked that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

----Next_Part(Thu_Feb_22_11_46_00_2024_016)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="don_t_flush_tuplestore_at_end.diff"

diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 5315d8a714..9dae2e512f 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -410,11 +410,21 @@ BufFileExportFileSet(BufFile *file)
  */
 void
 BufFileClose(BufFile *file)
+{
+	BufFileCloseExtended(file, true);
+}
+
+/*
+ * Close a BufFile with specifying wheter the content is flushed out
+ */
+void
+BufFileCloseExtended(BufFile *file, bool flush)
 {
 	int			i;
 
-	/* flush any unwritten data */
-	BufFileFlush(file);
+	/* flush any unwritten data if requested */
+	if (flush)
+		BufFileFlush(file);
 	/* close and delete the underlying file(s) */
 	for (i = 0; i < file->numFiles; i++)
 		FileClose(file->files[i]);
diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
index 947a868e56..08c1bcced9 100644
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -421,7 +421,7 @@ tuplestore_clear(Tuplestorestate *state)
 	TSReadPointer *readptr;
 
 	if (state->myfile)
-		BufFileClose(state->myfile);
+		BufFileCloseExtended(state->myfile, false);
 	state->myfile = NULL;
 	if (state->memtuples)
 	{
@@ -455,7 +455,7 @@ tuplestore_end(Tuplestorestate *state)
 	int			i;
 
 	if (state->myfile)
-		BufFileClose(state->myfile);
+		BufFileCloseExtended(state->myfile, false);
 	if (state->memtuples)
 	{
 		for (i = state->memtupdeleted; i < state->memtupcount; i++)
diff --git a/src/include/storage/buffile.h b/src/include/storage/buffile.h
index 5f6d7c8e3f..d4796f9cb4 100644
--- a/src/include/storage/buffile.h
+++ b/src/include/storage/buffile.h
@@ -38,6 +38,7 @@ typedef struct BufFile BufFile;
 
 extern BufFile *BufFileCreateTemp(bool interXact);
 extern void BufFileClose(BufFile *file);
+extern void BufFileCloseExtended(BufFile *file, bool flush);
 extern pg_nodiscard size_t BufFileRead(BufFile *file, void *ptr, size_t size);
 extern void BufFileReadExact(BufFile *file, void *ptr, size_t size);
 extern size_t BufFileReadMaybeEOF(BufFile *file, void *ptr, size_t size, bool eofOK);

----Next_Part(Thu_Feb_22_11_46_00_2024_016)----