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

diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 9cdddba510e2c4fb2d3299e85765519b61627a2f..d8a18dd3dcb3f2127c55654265c7082403eae9c5 100644 (file)
--- 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 afce5dadc0993cdaae753a72f01fe05b01635dcf..441f18dcf56e6fd679269806da46f7281c58d85d 100644 (file)
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2255,16 +2255,6 @@ FileGetRawMode(File file)
    return VfdCache[file].fileMode;
 }
 
-/*
- * 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 19eb2fddcada166b980adc4542c23381d5643d44..a0d6c75c37e074f4d680a4ddedf47dafc76889e6 100644 (file)
--- 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 e6a8d22feb281e24ce1594ce7302e5f7d839cd12..9fb33b9035e198782cc8e02efdc36485e4001641 100644 (file)
--- 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 548a832be94e081bcd6c48e0c0f71205daafd1c3..8e7c9728f4b3c3c969d8ec15617854e51df4bb12 100644 (file)
--- 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 9bf1d8014240cc36a66f76c51e6ce4bfe1bf7b99..06dc734eb620289865bcbcf5eaae7955c65949c2 100644 (file)
--- 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;
 
 /*