Memory leaks in BufFileOpenShared()
Memory is allocated twice for "file" and "files" variables. Possible fix:
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index d8a18dd3dc..00f61748b3 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
BufFile *
BufFileOpenShared(SharedFileSet *fileset, const char *name)
{
- BufFile *file = (BufFile *) palloc(sizeof(BufFile));
+ BufFile *file;
char segment_name[MAXPGPATH];
Size capacity = 16;
- File *files = palloc(sizeof(File) * capacity);
+ File *files;
int nfiles = 0;file = (BufFile *) palloc(sizeof(BufFile));
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
Memory is allocated twice for "file" and "files" variables. Possible fix:
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c index d8a18dd3dc..00f61748b3 100644 --- a/src/backend/storage/file/buffile.c +++ b/src/backend/storage/file/buffile.c @@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name) BufFile * BufFileOpenShared(SharedFileSet *fileset, const char *name) { - BufFile *file = (BufFile *) palloc(sizeof(BufFile)); + BufFile *file; char segment_name[MAXPGPATH]; Size capacity = 16; - File *files = palloc(sizeof(File) * capacity); + File *files; int nfiles = 0;file = (BufFile *) palloc(sizeof(BufFile));
Good catch. Thanks.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
Now I see that BufFileCreateShared() has similar problem with file->name.
More generic problem I see is that the common initialization of BufFile is
repeated a few times. The attached patch tries to improve that (it also fixes
the duplicate allocation of file->name).
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
Memory is allocated twice for "file" and "files" variables. Possible fix:
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c index d8a18dd3dc..00f61748b3 100644 --- a/src/backend/storage/file/buffile.c +++ b/src/backend/storage/file/buffile.c @@ -277,10 +277,10 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name) BufFile * BufFileOpenShared(SharedFileSet *fileset, const char *name) { - BufFile *file = (BufFile *) palloc(sizeof(BufFile)); + BufFile *file; char segment_name[MAXPGPATH]; Size capacity = 16; - File *files = palloc(sizeof(File) * capacity); + File *files; int nfiles = 0;file = (BufFile *) palloc(sizeof(BufFile));
Good catch. Thanks.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
Attachments:
buffile_refactor.patchtext/x-diffDownload
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index d8a18dd3dc..e345d008b5 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -99,6 +99,7 @@ struct BufFile
char buffer[BLCKSZ];
};
+static BufFile *makeBufFileCommon(int nfiles);
static BufFile *makeBufFile(File firstfile);
static void extendBufFile(BufFile *file);
static void BufFileLoadBuffer(BufFile *file);
@@ -106,21 +107,16 @@ static void BufFileDumpBuffer(BufFile *file);
static int BufFileFlush(BufFile *file);
static File MakeNewSharedSegment(BufFile *file, int segment);
-
/*
- * Create a BufFile given the first underlying physical file.
- * NOTE: caller must set isInterXact if appropriate.
+ * Create BufFile and perform the common initialization.
*/
static BufFile *
-makeBufFile(File firstfile)
+makeBufFileCommon(int nfiles)
{
BufFile *file = (BufFile *) palloc(sizeof(BufFile));
- file->numFiles = 1;
- file->files = (File *) palloc(sizeof(File));
- file->files[0] = firstfile;
- file->offsets = (off_t *) palloc(sizeof(off_t));
- file->offsets[0] = 0L;
+ file->numFiles = nfiles;
+ file->offsets = (off_t *) palloc0(sizeof(off_t) * nfiles);
file->isInterXact = false;
file->dirty = false;
file->resowner = CurrentResourceOwner;
@@ -128,6 +124,21 @@ makeBufFile(File firstfile)
file->curOffset = 0L;
file->pos = 0;
file->nbytes = 0;
+
+ return file;
+}
+
+/*
+ * Create a BufFile given the first underlying physical file.
+ * NOTE: caller must set isInterXact if appropriate.
+ */
+static BufFile *
+makeBufFile(File firstfile)
+{
+ BufFile *file = makeBufFileCommon(1);
+
+ file->files = (File *) palloc(sizeof(File));
+ file->files[0] = firstfile;
file->readOnly = false;
file->fileset = NULL;
file->name = NULL;
@@ -246,23 +257,12 @@ BufFileCreateShared(SharedFileSet *fileset, const char *name)
{
BufFile *file;
- file = (BufFile *) palloc(sizeof(BufFile));
+ file = makeBufFileCommon(1);
file->fileset = fileset;
file->name = pstrdup(name);
- file->numFiles = 1;
file->files = (File *) palloc(sizeof(File));
file->files[0] = MakeNewSharedSegment(file, 0);
- file->offsets = (off_t *) palloc(sizeof(off_t));
- file->offsets[0] = 0L;
- file->isInterXact = false;
- file->dirty = false;
- file->resowner = CurrentResourceOwner;
- file->curFile = 0;
- file->curOffset = 0L;
- file->pos = 0;
- file->nbytes = 0;
file->readOnly = false;
- file->name = pstrdup(name);
return file;
}
@@ -286,4 +286,3 @@
- file = (BufFile *) palloc(sizeof(BufFile));
files = palloc(sizeof(File) * capacity);
/*
@@ -317,16 +316,8 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name)
(errcode_for_file_access(),
errmsg("could not open BufFile \"%s\"", name)));
- file->numFiles = nfiles;
+ file = makeBufFileCommon(nfiles);
file->files = files;
- file->offsets = (off_t *) palloc0(sizeof(off_t) * nfiles);
- file->isInterXact = false;
- file->dirty = false;
- file->resowner = CurrentResourceOwner; /* Unused, can't extend */
- file->curFile = 0;
- file->curOffset = 0L;
- file->pos = 0;
- file->nbytes = 0;
file->readOnly = true; /* Can't write to files opened this way */
file->fileset = fileset;
file->name = pstrdup(name);
Now I see that BufFileCreateShared() has similar problem with file->name.
Right.
More generic problem I see is that the common initialization of BufFile is
repeated a few times. The attached patch tries to improve that (it also fixes
the duplicate allocation of file->name).
The changes were made by this commit to add infrastructure for sharing
temporary files between backends, according to the author (Thomas
Munro).
https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
Your proposal looks reasonable but I would like to hear from Thomas's
opinion as well.
Thomas?
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
The changes were made by this commit to add infrastructure for sharing
temporary files between backends, according to the author (Thomas
Munro).https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
Your proposal looks reasonable but I would like to hear from Thomas's
opinion as well.
o.k. He can actually have different feeling about details, e.g. if a new
function makeBufFileCommon() should be introduced or if makeBufFile() should
do the common settings. In the latter case, BufFileCreateTemp() would have to
do more work than it does now.
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
On Fri, Jun 15, 2018 at 8:42 PM, Antonin Houska <ah@cybertec.at> wrote:
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
The changes were made by this commit to add infrastructure for sharing
temporary files between backends, according to the author (Thomas
Munro).https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
Your proposal looks reasonable but I would like to hear from Thomas's
opinion as well.o.k. He can actually have different feeling about details, e.g. if a new
function makeBufFileCommon() should be introduced or if makeBufFile() should
do the common settings. In the latter case, BufFileCreateTemp() would have to
do more work than it does now.
Thanks for finding these accidental duplications, and to Ishii-san for
committing the fix. Oops.
+1 for this refactoring.
--
Thomas Munro
http://www.enterprisedb.com
Hi Thomas,
On Fri, Jun 15, 2018 at 8:42 PM, Antonin Houska <ah@cybertec.at> wrote:
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
The changes were made by this commit to add infrastructure for sharing
temporary files between backends, according to the author (Thomas
Munro).https://git.postgresql.org/pg/commitdiff/dc6c4c9dc2a111519b76b22daaaac86c5608223b
Your proposal looks reasonable but I would like to hear from Thomas's
opinion as well.o.k. He can actually have different feeling about details, e.g. if a new
function makeBufFileCommon() should be introduced or if makeBufFile() should
do the common settings. In the latter case, BufFileCreateTemp() would have to
do more work than it does now.Thanks for finding these accidental duplications, and to Ishii-san for
committing the fix. Oops.+1 for this refactoring.
Thanks for confirming. I will go ahead commit/push the patch.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
Thanks for finding these accidental duplications, and to Ishii-san for
committing the fix. Oops.+1 for this refactoring.
Thanks for confirming. I will go ahead commit/push the patch.
Done.
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp