pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Antonin Houska <ah@cybertec.at>
To: Tatsuo Ishii <ishii@sraoss.co.jp>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Memory leaks in BufFileOpenShared()
Date: Fri, 15 Jun 2018 09:59:26 +0200
Message-ID: <16139.1529049566@localhost> (raw )
In-Reply-To: <20180615.162003.2258832502225295119.t-ishii@sraoss.co.jp >
References: <11329.1529045692@localhost >
<20180615.162003.2258832502225295119.t-ishii@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:
[text/x-diff] buffile_refactor.patch (3.2K, ../16139.1529049566@localhost/2-buffile_refactor.patch)
download | inline diff:
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);
view thread (8+ messages) latest in thread
Message-ID: <16139.1529049566@localhost>
Permalink: ../16139.1529049566@localhost/
Also on: postgresql.org/message-id/16139.1529049566@localhost
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ah@cybertec.at, ishii@sraoss.co.jp
Subject: Re: Memory leaks in BufFileOpenShared()
In-Reply-To: <16139.1529049566@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox