Fix some sloppiness in the new BufFileSize() and BufFileAppend() functions.

There were three related issues:

* BufFileAppend() incorrectly reset the seek position on the 'source' file.
  As a result, if you had called BufFileRead() on the file before calling
  BufFileAppend(), it got confused, and subsequent calls would read/write
  at wrong position.

* BufFileSize() did not work with files opened with BufFileOpenShared().

* FileGetSize() only worked on temporary files.

To fix, change the way BufFileSize() works so that it works on shared
files. Remove FileGetSize() altogether, as it's no longer needed. Remove
buffilesize from TapeShare struct, as the leader process can simply call
BufFileSize() to get the tape's size, there's no need to pass it through
shared memory anymore.

Discussion: https://www.postgresql.org/message-id/CAH2-WznEDYe_NZXxmnOfsoV54oFkTdMy7YLE2NPBLuttO96vTQ@mail.gmail.com

6 files changed, 24 insertions, 24 deletions

diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 9cdddba510e..d8a18dd3dcb 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -802,14 +802,24 @@ BufFileTellBlock(BufFile *file)
 #endif
 
 /*
- * Return the current file size.  Counts any holes left behind by
- * BufFileViewAppend as part of the size.
+ * Return the current file size.
+ *
+ * Counts any holes left behind by BufFileAppend as part of the size.
+ * Returns -1 on error.
  */
 off_t
 BufFileSize(BufFile *file)
 {
+	off_t		lastFileSize;
+
+	/* Get the size of the last physical file by seeking to end. */
+	lastFileSize = FileSeek(file->files[file->numFiles - 1], 0, SEEK_END);
+	if (lastFileSize < 0)
+		return -1;
+	file->offsets[file->numFiles - 1] = lastFileSize;
+
 	return ((file->numFiles - 1) * (off_t) MAX_PHYSICAL_FILESIZE) +
-		FileGetSize(file->files[file->numFiles - 1]);
+		lastFileSize;
 }
 
 /*
@@ -853,7 +863,7 @@ BufFileAppend(BufFile *target, BufFile *source)
 	for (i = target->numFiles; i < newNumFiles; i++)
 	{
 		target->files[i] = source->files[i - target->numFiles];
-		target->offsets[i] = 0L;
+		target->offsets[i] = source->offsets[i - target->numFiles];
 	}
 	target->numFiles = newNumFiles;
 
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index afce5dadc09..441f18dcf56 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2256,16 +2256,6 @@ FileGetRawMode(File file)
 }
 
 /*
- * FileGetSize - returns the size of file
- */
-off_t
-FileGetSize(File file)
-{
-	Assert(FileIsValid(file));
-	return VfdCache[file].fileSize;
-}
-
-/*
  * Make room for another allocatedDescs[] array entry if needed and possible.
  * Returns true if an array element is available.
  */
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index 19eb2fddcad..a0d6c75c37e 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -426,11 +426,17 @@ ltsConcatWorkerTapes(LogicalTapeSet *lts, TapeShare *shared,
 	{
 		char		filename[MAXPGPATH];
 		BufFile    *file;
+		off_t		filesize;
 
 		lt = &lts->tapes[i];
 
 		pg_itoa(i, filename);
 		file = BufFileOpenShared(fileset, filename);
+		filesize = BufFileSize(file);
+		if (filesize < 0)
+			ereport(ERROR,
+					(errcode_for_file_access(),
+					 errmsg("could not determine size of temporary file \"%s\"", filename)));
 
 		/*
 		 * Stash first BufFile, and concatenate subsequent BufFiles to that.
@@ -447,8 +453,8 @@ ltsConcatWorkerTapes(LogicalTapeSet *lts, TapeShare *shared,
 			lt->offsetBlockNumber = BufFileAppend(lts->pfile, file);
 		}
 		/* Don't allocate more for read buffer than could possibly help */
-		lt->max_size = Min(MaxAllocSize, shared[i].buffilesize);
-		tapeblocks = shared[i].buffilesize / BLCKSZ;
+		lt->max_size = Min(MaxAllocSize, filesize);
+		tapeblocks = filesize / BLCKSZ;
 		nphysicalblocks += tapeblocks;
 	}
 
@@ -938,7 +944,6 @@ LogicalTapeFreeze(LogicalTapeSet *lts, int tapenum, TapeShare *share)
 	{
 		BufFileExportShared(lts->pfile);
 		share->firstblocknumber = lt->firstBlockNumber;
-		share->buffilesize = BufFileSize(lts->pfile);
 	}
 }
 
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index e6a8d22feb2..9fb33b9035e 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -4395,7 +4395,6 @@ tuplesort_initialize_shared(Sharedsort *shared, int nWorkers, dsm_segment *seg)
 	for (i = 0; i < nWorkers; i++)
 	{
 		shared->tapes[i].firstblocknumber = 0L;
-		shared->tapes[i].buffilesize = 0;
 	}
 }
 
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index 548a832be94..8e7c9728f4b 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -78,7 +78,6 @@ extern char *FilePathName(File file);
 extern int	FileGetRawDesc(File file);
 extern int	FileGetRawFlags(File file);
 extern mode_t FileGetRawMode(File file);
-extern off_t FileGetSize(File file);
 
 /* Operations used for sharing named temporary files */
 extern File PathNameCreateTemporaryFile(const char *name, bool error_on_failure);
diff --git a/src/include/utils/logtape.h b/src/include/utils/logtape.h
index 9bf1d801424..06dc734eb62 100644
--- a/src/include/utils/logtape.h
+++ b/src/include/utils/logtape.h
@@ -44,13 +44,10 @@ typedef struct LogicalTapeSet LogicalTapeSet;
 typedef struct TapeShare
 {
 	/*
-	 * firstblocknumber is first block that should be read from materialized
-	 * tape.
-	 *
-	 * buffilesize is the size of associated BufFile following freezing.
+	 * Currently, all the leader process needs is the location of the
+	 * materialized tape's first block.
 	 */
 	long		firstblocknumber;
-	off_t		buffilesize;
 } TapeShare;
 
 /*